Bug 113318

Created attachment 195763 [details] Patch

Created attachment 195839 [details] Patch

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

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

(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 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 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()?

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?

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

Created attachment 196080 [details] Patch

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.

(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*

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

Created attachment 196166 [details] Patch

Comment on attachment 196166 [details] Patch Clearing flags on attachment: 196166 Committed r147443: <http://trac.webkit.org/changeset/147443>

All reviewed patches have been landed. Closing bug.