[webkit-reviews] review denied: [Bug 113318] [GTK][AC] Support preserves3D css property for clutter ac backend. : [Attachment 195839] 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 113318: [GTK][AC] Support preserves3D css property for clutter ac backend.
https://bugs.webkit.org/show_bug.cgi?id=113318

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

------- Additional Comments from Gustavo Noronha (kov) <gns at gnome.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=195839&action=review


r- because of the comments

> Source/WebCore/ChangeLog:9
> +	   Most of all codes are based on Mac port's implementation. The core
concept is that

s/Most of all codes are/Most of the code is/

> Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:48
> +    gboolean flattening;

flatten is a better name since it's imperative, or flattens to follow the
drawContents convention, present continuous indicates things are happening at
that time, usually we use that only for tracking whether something is happening
- like stating that layout is underway to avoid triggering it again; btw, we
can use bool here instead of gboolean, I think we should!

> Source/WebCore/platform/graphics/clutter/GraphicsLayerActor.cpp:517
> +void graphicsLayerActorSetFlattening(GraphicsLayerActor* layer, gboolean
flattening)

We should change the method name too.

> Source/WebCore/platform/graphics/clutter/GraphicsLayerClutter.cpp:584
>      // FIXME: Save the state before sending down to kids and restore it
after
>      TransformState localState = state;
> +    CommitState childCommitState = commitState;

Looks like the state is not restored anywhere. Is this what the FIXME is about?
Is it because the child tries to change the state that is passed to it, so we
need a modifiable copy of those? Could we drop the assignments for now and add
them when we are actually restoring state?

> Source/WebCore/platform/graphics/clutter/GraphicsLayerClutter.cpp:712
> +	   // If the preserves3D of a layer is false, itself and its immediate
children should be flattened.
> +	   graphicsLayerActorSetFlattening(childLayer, !(preserves3D() ||
currentChild->preserves3D()));

Not sure I understand this comment. What this is doing is telling the child to
flatten if neither the "this" layer nor the currentChild has preserve3D set. If
that is correct behaviour, then I think we can do away with the comment. The
comment implies the layer and its children should be flattened if the "this"
layer has preserves3D set to false.