Bug 68295 - Remove preserves3D() from CCLayerDelegate
Summary: Remove preserves3D() from CCLayerDelegate
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-09-16 17:43 PDT by Antoine Labour
Modified: 2011-09-23 16:24 PDT (History)
3 users (show)

See Also:


Attachments
Patch (6.43 KB, patch)
2011-09-16 17:44 PDT, Antoine Labour
no flags Details | Formatted Diff | Diff
Patch (6.44 KB, patch)
2011-09-16 18:01 PDT, Antoine Labour
no flags Details | Formatted Diff | Diff
Patch (6.46 KB, patch)
2011-09-22 19:51 PDT, Antoine Labour
no flags Details | Formatted Diff | Diff
Patch (5.32 KB, patch)
2011-09-23 15:52 PDT, Antoine Labour
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antoine Labour 2011-09-16 17:43:06 PDT
Remove preserves3D() from CCLayerDelegate
Comment 1 Antoine Labour 2011-09-16 17:44:51 PDT
Created attachment 107755 [details]
Patch
Comment 2 Antoine Labour 2011-09-16 18:01:09 PDT
Created attachment 107757 [details]
Patch
Comment 3 Antoine Labour 2011-09-22 19:51:33 PDT
Created attachment 108436 [details]
Patch
Comment 4 Antoine Labour 2011-09-22 19:53:00 PDT
Rebased patch (and friendly ping)
Comment 5 James Robinson 2011-09-23 14:55:52 PDT
Comment on attachment 108436 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:367
> +        platformLayer->setPreserves3D(m_preserves3D);

I don't think we need to do this - the canvas contents layer can't have preserves3D set since it's a CSS property and would only apply to the container layer, and even if it could we should be able to rely on the normal property propagation code

> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:400
> +        layer->setPreserves3D(m_preserves3D);

same here - i don't think media layers can have this bit

> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:600
> +    if (m_contentsLayer)
> +        m_contentsLayer->setPreserves3D(m_preserves3D);

it looks like we used to propagate the preserves3D bit to the contents layer.  this change preserves that behavior but I think it's actually wrong. Can you see if any tests break if you only propagate the preserves3D bit to the m_layer?

> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:652
> +        m_contentsLayer->setPreserves3D(m_preserves3D);

do we need this?
Comment 6 James Robinson 2011-09-23 14:57:11 PDT
Comment on attachment 108436 [details]
Patch

R=me since this is preserving the existing behavior, so it's fine to land as-is.  I think we can simplify, however.
Comment 7 Antoine Labour 2011-09-23 15:52:44 PDT
Created attachment 108551 [details]
Patch
Comment 8 Antoine Labour 2011-09-23 15:54:08 PDT
View in context: https://bugs.webkit.org/attachment.cgi?id=108436&action=review

>> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:367
>> +        platformLayer->setPreserves3D(m_preserves3D);
> 
> I don't think we need to do this - the canvas contents layer can't have preserves3D set since it's a CSS property and would only apply to the container layer, and even if it could we should be able to rely on the normal property propagation code

Done.

>> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:400
>> +        layer->setPreserves3D(m_preserves3D);
> 
> same here - i don't think media layers can have this bit

Done.

>> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:600
>> +        m_contentsLayer->setPreserves3D(m_preserves3D);
> 
> it looks like we used to propagate the preserves3D bit to the contents layer.  this change preserves that behavior but I think it's actually wrong. Can you see if any tests break if you only propagate the preserves3D bit to the m_layer?

Done. Layout tests still pass.

>> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:652
>> +        m_contentsLayer->setPreserves3D(m_preserves3D);
> 
> do we need this?

Removed.
Comment 9 James Robinson 2011-09-23 15:57:04 PDT
Comment on attachment 108551 [details]
Patch

R=me
Comment 10 WebKit Review Bot 2011-09-23 16:24:10 PDT
Comment on attachment 108551 [details]
Patch

Clearing flags on attachment: 108551

Committed r95886: <http://trac.webkit.org/changeset/95886>
Comment 11 WebKit Review Bot 2011-09-23 16:24:15 PDT
All reviewed patches have been landed.  Closing bug.