Bug 113318 - [GTK][AC] Support preserves3D css property for clutter ac backend.
Summary: [GTK][AC] Support preserves3D css property for clutter ac backend.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 110470 113317
Blocks: 105699
  Show dependency treegraph
 
Reported: 2013-03-26 10:06 PDT by ChangSeok Oh
Modified: 2013-04-02 10:14 PDT (History)
5 users (show)

See Also:


Attachments
Patch (23.98 KB, patch)
2013-03-29 10:53 PDT, ChangSeok Oh
no flags Details | Formatted Diff | Diff
Patch (23.95 KB, patch)
2013-03-29 21:12 PDT, ChangSeok Oh
no flags Details | Formatted Diff | Diff
Patch (24.26 KB, patch)
2013-04-01 23:44 PDT, ChangSeok Oh
gustavo: commit-queue-
Details | Formatted Diff | Diff
Patch (24.32 KB, patch)
2013-04-02 09:34 PDT, ChangSeok Oh
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description ChangSeok Oh 2013-03-26 10:06:09 PDT
Support preserves3D css property for clutter ac backend.
Comment 1 ChangSeok Oh 2013-03-29 10:53:35 PDT
Created attachment 195763 [details]
Patch
Comment 2 ChangSeok Oh 2013-03-29 21:12:13 PDT
Created attachment 195839 [details]
Patch
Comment 4 kov's GTK+ EWS bot 2013-03-30 02:37:29 PDT
Comment on attachment 195839 [details]
Patch

Attachment 195839 [details] did not pass gtk-ews (gtk):
Output: http://webkit-commit-queue.appspot.com/results/17370048
Comment 5 ChangSeok Oh 2013-03-30 09:57:32 PDT
(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..
Comment 6 Gustavo Noronha (kov) 2013-04-01 10:00:00 PDT
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.
Comment 7 Joone Hur 2013-04-01 12:58:22 PDT
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()?
Comment 8 Gustavo Noronha (kov) 2013-04-01 14:58:24 PDT
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?
Comment 9 ChangSeok Oh 2013-04-01 22:34:45 PDT
(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.
Comment 10 ChangSeok Oh 2013-04-01 22:45:18 PDT
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
Comment 11 ChangSeok Oh 2013-04-01 23:44:44 PDT
Created attachment 196080 [details]
Patch
Comment 12 Gustavo Noronha (kov) 2013-04-02 05:43:47 PDT
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.
Comment 13 ChangSeok Oh 2013-04-02 08:05:00 PDT
(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*
Comment 14 Gustavo Noronha (kov) 2013-04-02 08:21:20 PDT
(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!
Comment 15 ChangSeok Oh 2013-04-02 09:34:25 PDT
Created attachment 196166 [details]
Patch
Comment 16 WebKit Review Bot 2013-04-02 10:14:41 PDT
Comment on attachment 196166 [details]
Patch

Clearing flags on attachment: 196166

Committed r147443: <http://trac.webkit.org/changeset/147443>
Comment 17 WebKit Review Bot 2013-04-02 10:14:45 PDT
All reviewed patches have been landed.  Closing bug.