Bug 109037

Created attachment 186812 [details] Patch

Comment on attachment 186812 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=186812&action=review > Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:218 > + if (priv->matrix) { > + float width = clutter_actor_get_width(actor); > + float height = clutter_actor_get_height(actor); > + if (width <= 1.0 || height <= 1.0) > + return; This is a change in behaviour, right? Would be good to separate "bringing stuff up-to-date" with behaviour changes. > Source/WebCore/platform/graphics/clutter/GraphicsLayerClutter.cpp:396 > - FloatPoint scaledPosition; > - FloatPoint3D scaledAnchorPoint; > - FloatSize scaledSize; > - > - // FIXME: Need to support scaling > - scaledPosition = m_position; > - scaledAnchorPoint = m_anchorPoint; > - scaledSize = m_size; > - > - FloatRect adjustedBounds(m_boundsOrigin , scaledSize); > - FloatPoint adjustedPosition(scaledPosition.x() + scaledAnchorPoint.x() * scaledSize.width(), scaledPosition.y() + scaledAnchorPoint.y() * scaledSize.height()); > - > - clutter_actor_set_size(CLUTTER_ACTOR(m_layer.get()), adjustedBounds.width(), adjustedBounds.height()); > - clutter_actor_set_position(CLUTTER_ACTOR(m_layer.get()), adjustedPosition.x(), adjustedPosition.y()); > - graphicsLayerActorSetAnchorPoint(m_layer.get(), scaledAnchorPoint.x(), scaledAnchorPoint.y(), scaledAnchorPoint.z()); > + // FIXME: Need to support page scaling. > + clutter_actor_set_size(CLUTTER_ACTOR(m_layer.get()), m_size.width(), m_size.height()); > + clutter_actor_set_position(CLUTTER_ACTOR(m_layer.get()), m_position.x(), m_position.y()); > + graphicsLayerActorSetAnchorPoint(m_layer.get(), m_anchorPoint.x(), m_anchorPoint.y(), m_anchorPoint.z()); Ditto. You're now ignoring the calculations done for adjusted*(), that might be OK (I think it might break AC on zoomed pages, though, did you check it doesn't?), but should be its own change.

Comment on attachment 186812 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=186812&action=review >> Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:218 >> + return; > > This is a change in behaviour, right? Would be good to separate "bringing stuff up-to-date" with behaviour changes. Right, let me open a new bug to handle this kind of stuff. >> Source/WebCore/platform/graphics/clutter/GraphicsLayerClutter.cpp:396 >> + graphicsLayerActorSetAnchorPoint(m_layer.get(), m_anchorPoint.x(), m_anchorPoint.y(), m_anchorPoint.z()); > > Ditto. You're now ignoring the calculations done for adjusted*(), that might be OK (I think it might break AC on zoomed pages, though, did you check it doesn't?), but should be its own change. O.K. Let's deal with this in an another bug. Current implementations doesn't support page scaling, too. I think using adjusted* values is wrong here for clutterActor position. Actually, clutter coordinate system doesn't need to recalculate ajusted* value because it has a different coordinate system over mac port. Basically, the position of a CALayer(?) is specified in terms of the location of the layer's anchorPoint in mac port, so mac port need to recalculate the layer position after changing anchor position. See https://developer.apple.com/library/ios/#documentation/Cocoa/Conceptual/CoreAnimation_guide/CoreAnimationBasics/CoreAnimationBasics.html#//apple_ref/doc/uid/TP40004514-CH2-SW15

Created attachment 187142 [details] Patch

Comment on attachment 187142 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=187142&action=review Looks good apart from the changelog. > ChangeLog:20 > +2013-02-07 ChangSeok Oh <shivamidow@gmail.com> > + > + [GTK][AC] Clutter required version up to 1.12 > + https://bugs.webkit.org/show_bug.cgi?id=109037 > + > + Reviewed by NOBODY (OOPS!). > + > + The clutter requried version is changed to 1.12. > + > + * configure.ac: > + You've got a double changelog here so you cannot use the commit queue. There must be a bug with the script because I saw this problem today as well.

Created attachment 187153 [details] Patch

(In reply to comment #3) > O.K. Let's deal with this in an another bug. Current implementations doesn't support page scaling, too. > I think using adjusted* values is wrong here for clutterActor position. Actually, clutter coordinate system doesn't need to recalculate ajusted* value because it has a different coordinate system over mac port. Basically, the position of a CALayer(?) is specified in terms of the location of the layer's anchorPoint in mac port, so mac port need to recalculate the layer position after changing anchor position. See https://developer.apple.com/library/ios/#documentation/Cocoa/Conceptual/CoreAnimation_guide/CoreAnimationBasics/CoreAnimationBasics.html#//apple_ref/doc/uid/TP40004514-CH2-SW15 Makes sense. We used to use an anchor point as well, but Emanuele Aina has made it use clutter's new pivot point instead like you've done here. Let me know when you post the other patch!

Comment on attachment 187153 [details] Patch Clearing flags on attachment: 187153 Committed r142172: <http://trac.webkit.org/changeset/142172>

All reviewed patches have been landed. Closing bug.