Bug 113318 – [GTK][AC] Support preserves3D css property for clutter ac backend.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[ChangSeok Oh]

Support preserves3D css property for clutter ac backend.

[ChangSeok Oh]

Created attachment 195763 [details]
Patch

[ChangSeok Oh]

Created attachment 195839 [details]
Patch

[ChangSeok Oh]

For webkit-clutter port, http://cgit.collabora.com/git/webkit-clutter.git/commit/?h=wip/changseok/unreviewed&id=84b0e7866bd8581d4d021ef852f6924d3ee80f92

[kov's GTK+ EWS bot]

Comment on attachment 195839 [details]
Patch

Attachment 195839 [details] did not pass gtk-ews (gtk):
Output: http://webkit-commit-queue.appspot.com/results/17370048

[ChangSeok Oh]

(In reply to comment #4)
> (From update of attachment 195839 [details])
> Attachment 195839 [details] did not pass gtk-ews (gtk):
> Output: http://webkit-commit-queue.appspot.com/results/17370048

It seems jhbuild issue..

[Gustavo Noronha (kov)]

Comment on attachment 195839 [details]
Patch

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.

[Joone Hur]

Comment on attachment 195839 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=195839&action=review

>> Source/WebCore/platform/graphics/clutter/GraphicsLayerClutter.cpp:712
>> +        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.

All elements transform-style property has false value by default, so preserve3D() usually returns false. I think we can change the comment as follws:
"If the preserve3D() of this layer returns true, its children should not be flattened"

By the way, do we really need to check currentChild->preserves3D()?

[Gustavo Noronha (kov)]

If we change the code to not check currentChild->preserves3D(), then the comment you propose makes more sense =). But that would make the code obvious enough that I think we should just remove the comment. Why we check currentChild->preserves3D() is one of the doubts I had, I don't know why. Does it work without that check?

[ChangSeok Oh]

(In reply to comment #8)
> If we change the code to not check currentChild->preserves3D(), then the comment you propose makes more sense =). But that would make the code obvious enough that I think we should just remove the comment. Why we check currentChild->preserves3D() is one of the doubts I had, I don't know why. Does it work without that check?

No. it doesn't. I know a sample working wrong without it.
Please run http://www.webkit.org/blog-files/3d-transforms/poster-circle.html

If a layer has preserve-3d true, and it should not be flattened though its parent has preserve-3d false. In other words, if we check only current layer's preserve-3d(false), children layers might be flattened even though children layers have preserve-3d true.

[ChangSeok Oh]

Comment on attachment 195839 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=195839&action=review

Thank you for the comments!

>> 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!

Done.

>> Source/WebCore/platform/graphics/clutter/GraphicsLayerClutter.cpp:584
>> +    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?

O.K. Make sense. We're not using them, so we don't need to keep them as local variables.

>>> Source/WebCore/platform/graphics/clutter/GraphicsLayerClutter.cpp:712
>>> +        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.
> 
> All elements transform-style property has false value by default, so preserve3D() usually returns false. I think we can change the comment as follws:
> "If the preserve3D() of this layer returns true, its children should not be flattened"
> 
> By the way, do we really need to check currentChild->preserves3D()?

"If the preserve3D() of this layer returns true, itself and its children should not be flattened" is better to me.
But I don't mind removing the comment if it causes that you're confused. :p

[ChangSeok Oh]

Created attachment 196080 [details]
Patch

[Gustavo Noronha (kov)]

Comment on attachment 196080 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=196080&action=review

cq- because of the suggestion

> Source/WebCore/platform/graphics/clutter/GraphicsLayerClutter.cpp:707
> -        newSublayers.append(childLayer);
> +        graphicsLayerActorSetFlatten(childLayer, !(preserves3D() || currentChild->preserves3D()));

> "If the preserve3D() of this layer returns true, itself and its children should not be flattened" is better to me.
> But I don't mind removing the comment if it causes that you're confused. :p

The thing is that comment you suggest does not describe what the code is doing, do you agree with that? What the code does is set flatten to true on the child layer if both the current layer and the child do *not* preserve 3D. So I think you could say this:

"The child layer only preserves 3D if both itself and its parent have preserves3D set."

But feel free to omit the comment, I think the code speaks for itself.

[ChangSeok Oh]

(In reply to comment #12)
> (From update of attachment 196080 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=196080&action=review
> 
> cq- because of the suggestion
> 
> > Source/WebCore/platform/graphics/clutter/GraphicsLayerClutter.cpp:707
> > -        newSublayers.append(childLayer);
> > +        graphicsLayerActorSetFlatten(childLayer, !(preserves3D() || currentChild->preserves3D()));
> 
> > "If the preserve3D() of this layer returns true, itself and its children should not be flattened" is better to me.
> > But I don't mind removing the comment if it causes that you're confused. :p
> 
> The thing is that comment you suggest does not describe what the code is doing, do you agree with that? What the code does is set flatten to true on the child layer if both the current layer and the child do *not* preserve 3D. So I think you could say this:
> 
> "The child layer only preserves 3D if both itself and its parent have preserves3D set."
> 
> But feel free to omit the comment, I think the code speaks for itself.

kov, I think "The child layer only preserves 3D if either itself or its parent has preserves3D set." is correct, not *both*

[Gustavo Noronha (kov)]

(In reply to comment #13)
> (In reply to comment #12)
> > > +        graphicsLayerActorSetFlatten(childLayer, !(preserves3D() || currentChild->preserves3D()));
[...]
> > "The child layer only preserves 3D if both itself and its parent have preserves3D set."
> > 
> > But feel free to omit the comment, I think the code speaks for itself.
> 
> kov, I think "The child layer only preserves 3D if either itself or its parent has preserves3D set." is correct, not *both*

Oops, yes, go ahead!

[ChangSeok Oh]

Created attachment 196166 [details]
Patch

[WebKit Review Bot]

Comment on attachment 196166 [details]
Patch

Clearing flags on attachment: 196166

Committed r147443: <http://trac.webkit.org/changeset/147443>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.