[webkit-reviews] review denied: [Bug 109037] [GTK][AC] Clutter required version up to 1.12 : [Attachment 186812] Patch

[bugzilla-daemon at webkit.org]

Gustavo Noronha (kov) <gns at gnome.org> has denied ChangSeok Oh
<kevin.cs.oh at gmail.com>'s request for review:
Bug 109037: [GTK][AC] Clutter required version up to 1.12
https://bugs.webkit.org/show_bug.cgi?id=109037

Attachment 186812: Patch
https://bugs.webkit.org/attachment.cgi?id=186812&action=review

------- Additional Comments from Gustavo Noronha (kov) <gns at gnome.org>
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.