Bug 71209 – [Chromium] Fix drawsContent and contentsVisible in accelerated compositor

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Andrey Kosyakov]

dashboard:

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40ToT%20GPU%20Hardware%20-%20chromium.org&showExpectations=true&tests=compositing%2Fvisibility%2Fvisibility-image-layers.html

[Shawn Singh]

Finally found this while trying to investigate http://code.google.com/p/chromium/issues/detail?id=102865

It turns out that all layer types (except content layers) disobey the CSS visibility flag.

I'll take this bug and use it to submit a patch that hopefully resolves the issue for all layer types.

[Shawn Singh]

Created attachment 119035 [details]
work in progress, this version crashes

[Shawn Singh]

(In reply to comment #2)
> Created an attachment (id=119035) [details]
> work in progress, this version crashes

James suggested I upload this just in case someone can easily spot why this crashes.  It crashes in RenderLayerBacking.cpp.  partial stack trace is pasted below.

SHOULD NEVER BE REACHED
/Users/shawnsingh/src/chromium/src/third_party/WebKit/Source/WebCore/WebCore.gyp/../rendering/RenderLayerBacking.cpp(1092) : void WebCore::RenderLayerBacking::paintIntoLayer(WebCore::RenderLayer *, WebCore::GraphicsContext *, const LayoutRect &, PaintBehavior, WebCore::GraphicsLayerPaintingPhase, WebCore::RenderObject *)
1   0x55f3f8b2 WebCore::RenderLayerBacking::paintIntoLayer(WebCore::RenderLayer*, WebCore::GraphicsContext*, WebCore::IntRect const&, unsigned int, WebCore::GraphicsLayerPaintingPhase, WebCore::RenderObject*)
2   0x55f4052c WebCore::RenderLayerBacking::paintContents(WebCore::GraphicsLayer const*, WebCore::GraphicsContext&, WebCore::GraphicsLayerPaintingPhase, WebCore::IntRect const&)
3   0x551770a5 WebCore::GraphicsLayer::paintGraphicsLayerContents(WebCore::GraphicsContext&, WebCore::IntRect const&)
4   0x551b05e7 WebCore::GraphicsLayerChromium::paintContents(WebCore::GraphicsContext&, WebCore::IntRect const&)
5   0x551b0646 non-virtual thunk to WebCore::GraphicsLayerChromium::paintContents(WebCore::GraphicsContext&, WebCore::IntRect const&)
6   0x551a1cd4 WebCore::ContentLayerPainter::paint(WebCore::GraphicsContext&, WebCore::IntRect const&)
7   0x551a0698 WebCore::CanvasLayerTextureUpdater::paintContents(WebCore::GraphicsContext&, WebCore::IntRect const&, float)
8   0x5519d3d9 WebCore::BitmapCanvasLayerTextureUpdater::prepareToUpdate(WebCore::IntRect const&, WebCore::IntSize const&, int, float)
9   0x551e5d2c WebCore::TiledLayerChromium::prepareToUpdate(WebCore::IntRect const&)
10  0x551a0abb WebCore::ContentLayerChromium::paintContentsIfDirty()

[James Robinson]

Comment on attachment 119035 [details]
work in progress, this version crashes

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

> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:-677
> -bool GraphicsLayerChromium::drawsContent() const
> -{
> -    return GraphicsLayer::drawsContent();
> -}

you still need to route GraphicsLayer::setDrawsContent() to the appropriate LayerChromium(s), looks like that isn't happening here. I think the crash fix may be as simple as having GLC::updateLayerDrawsContent() push the correct bit to LayerChromium.

[James Robinson]

(In reply to comment #3)
> (In reply to comment #2)
> > Created an attachment (id=119035) [details] [details]
> > work in progress, this version crashes
> 
> James suggested I upload this just in case someone can easily spot why this crashes.  It crashes in RenderLayerBacking.cpp.  partial stack trace is pasted below.
> 
> SHOULD NEVER BE REACHED
> /Users/shawnsingh/src/chromium/src/third_party/WebKit/Source/WebCore/WebCore.gyp/../rendering/RenderLayerBacking.cpp(1092) : void WebCore::RenderLayerBacking::paintIntoLayer(WebCore::RenderLayer *, WebCore::GraphicsContext *, const LayoutRect &, PaintBehavior, WebCore::GraphicsLayerPaintingPhase, WebCore::RenderObject *)
> 1   0x55f3f8b2 WebCore::RenderLayerBacking::paintIntoLayer(WebCore::RenderLayer*, WebCore::GraphicsContext*, WebCore::IntRect const&, unsigned int, WebCore::GraphicsLayerPaintingPhase, WebCore::RenderObject*)
> 2   0x55f4052c WebCore::RenderLayerBacking::paintContents(WebCore::GraphicsLayer const*, WebCore::GraphicsContext&, WebCore::GraphicsLayerPaintingPhase, WebCore::IntRect const&)
> 3   0x551770a5 WebCore::GraphicsLayer::paintGraphicsLayerContents(WebCore::GraphicsContext&, WebCore::IntRect const&)
> 4   0x551b05e7 WebCore::GraphicsLayerChromium::paintContents(WebCore::GraphicsContext&, WebCore::IntRect const&)
> 5   0x551b0646 non-virtual thunk to WebCore::GraphicsLayerChromium::paintContents(WebCore::GraphicsContext&, WebCore::IntRect const&)
> 6   0x551a1cd4 WebCore::ContentLayerPainter::paint(WebCore::GraphicsContext&, WebCore::IntRect const&)
> 7   0x551a0698 WebCore::CanvasLayerTextureUpdater::paintContents(WebCore::GraphicsContext&, WebCore::IntRect const&, float)
> 8   0x5519d3d9 WebCore::BitmapCanvasLayerTextureUpdater::prepareToUpdate(WebCore::IntRect const&, WebCore::IntSize const&, int, float)
> 9   0x551e5d2c WebCore::TiledLayerChromium::prepareToUpdate(WebCore::IntRect const&)
> 10  0x551a0abb WebCore::ContentLayerChromium::paintContentsIfDirty()


This is because you're calling paintContents() on a GraphicsLayer that returns false to drawsContent(). That bit is still important, but you need to push it to the LayerChromium from GraphicsLayer::setDrawsContent() calls rather than having it pull.

[Shawn Singh]

Wow, I really should have seen that.  Thanks... saved me a lot of frustration. =)

[Shawn Singh]

Created attachment 119380 [details]
code changes only, for easier review

[Shawn Singh]

Created attachment 119382 [details]
exact same code, but this is the complete patch including tests

[Shawn Singh]

The latest two patches are for review please.  (the first patch is code only, the second patch is complete, with exact same code).   James and Antoine, can you please review it, even though I will need to merge with tip of tree?

This patch makes the following changes:

(1) removes CCLayerDelegate::drawsContent() callback

(2) replaces that callback with pushing drawsContent to LayerChromium
 - the push occurs in GraphicsLayerChromium (explicitly), WebLayerImpl (implicitly since its initialized to false), WebContentLayerImpl (explicitly), and WebExternalTextureLayerImpl (implicitly, because drawscontent() was never actually used here).

(3) Also adds pushing contentsVisible from GraphicsLayerChromium to LayerChromium, to fix the bug itself.

(4) cleans up some redunant m_contentsLayer setup code in GraphicsLayerChromium

(5) provides 3 new tests and re-baselined an existing test.

[WebKit Review Bot]

Comment on attachment 119382 [details]
exact same code, but this is the complete patch including tests

Attachment 119382 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10895133

[Vangelis Kokkevis]

Comment on attachment 119382 [details]
exact same code, but this is the complete patch including tests

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

> Source/WebCore/ChangeLog:18
> +        callback, and instead makes sure that drawsContent and

Is there an advantage to pushing the property down rather than having the compositor reach back to get it ?

> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:-319
> -            m_contentsLayer = imageLayer;

Not sure I understand why this is removed.  Same for setContentsToCanvas and setContentsToMedia.

[Shawn Singh]

Comment on attachment 119382 [details]
exact same code, but this is the complete patch including tests

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

>> Source/WebCore/ChangeLog:18
>> +        callback, and instead makes sure that drawsContent and
> 
> Is there an advantage to pushing the property down rather than having the compositor reach back to get it ?

In terms of readability and robustness of code, I think there are advantages:
 - uniformity, everything else is being "pushed", so it seems appropriate that drawsContent and contentsVisible should be, too.
 - James has in mind that we probably want to distill the CCLayerDelegate so that it only does painting, or maybe disappears. we had agreed that this patch would remove drawsContent() from the CCLayerDelegate.

In terms of performance/memory, no, there should be no benefit (and no harm).

>> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:-319
>> -            m_contentsLayer = imageLayer;
> 
> Not sure I understand why this is removed.  Same for setContentsToCanvas and setContentsToMedia.

These were removed because it is redundant with the same line of code inside setupContentsLayer().

Similarly, I removed updateContentsRect() from setupContentsLayer because it was redundant with its usage in these three functions; but its usage in these three functions supercedes the usage in setupContentsLayer().

> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:-643
> -        updateContentsRect();

FYI, previous comment refers to this.

> Source/WebKit/chromium/src/WebExternalTextureLayerImpl.cpp:-54
> -bool WebExternalTextureLayerImpl::drawsContent() const

Looking again, I have a feeling this is wrong.  It does override the PluginLayerChromium drawsContent, even though it is no longer part of CCLayerDelegate.  So I'll add this back.

> Source/WebKit/chromium/src/WebLayerImpl.cpp:-49
> -bool WebLayerImpl::drawsContent() const

In retrospect, I think we should add "setIsDrawable(false)" in the constructor, so that we dont have to trust that LayerChromium initializes it to false.

[Vangelis Kokkevis]

Comment on attachment 119382 [details]
exact same code, but this is the complete patch including tests

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

> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:222
> +    // contentsVisible whenever m_contentsLayer is changed.

Did you mean to say "drawsContent" instead of "contentsVisible" here?  Now that you've flagged this, is there something that the code needs to be doing to make sure the early out is accurate?

>>> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:-319
>>> -            m_contentsLayer = imageLayer;
>> 
>> Not sure I understand why this is removed.  Same for setContentsToCanvas and setContentsToMedia.
> 
> These were removed because it is redundant with the same line of code inside setupContentsLayer().
> 
> Similarly, I removed updateContentsRect() from setupContentsLayer because it was redundant with its usage in these three functions; but its usage in these three functions supercedes the usage in setupContentsLayer().

Oh, right, I see now.  Thanks.

> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:593
> +    m_layer->setIsDrawable(m_drawsContent);

Won't this also make "transform" layers (ones that are just parents to content layers) return true for drawsContent()?  Previously drawsContent() would (correctly) makereturn false for these layers.

[Shawn Singh]

Comment on attachment 119382 [details]
exact same code, but this is the complete patch including tests

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

>> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:222
>> +    // contentsVisible whenever m_contentsLayer is changed.
> 
> Did you mean to say "drawsContent" instead of "contentsVisible" here?  Now that you've flagged this, is there something that the code needs to be doing to make sure the early out is accurate?

Oops, I did mean to say drawsContent =)  The two lines added in setupContentsLayer are the "proper initialization", but I guess the word "changed" is misleading.  I'll re-word this to say "whenever m_contentsLayer is set to a different layer"

>> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:593
>> +    m_layer->setIsDrawable(m_drawsContent);
> 
> Won't this also make "transform" layers (ones that are just parents to content layers) return true for drawsContent()?  Previously drawsContent() would (correctly) makereturn false for these layers.

In LayerChromium, isDrawable is initialized to false. So any transform layers (m_transformLayer) should always return false for drawsContent(), since we don't change it here. So that should have the same correct behavior, right?

[Antoine Labour]

Comment on attachment 119382 [details]
exact same code, but this is the complete patch including tests

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

> Source/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:114
> +    return LayerChromium::drawsContent() && TiledLayerChromium::drawsContent();

Could we have TiledLayerChromium::drawsContent call LayerChromium::drawsContent and remove ContentLayerChromium::drawsContent ?
As it is now, it's certainly very confusing.

> Source/WebCore/platform/graphics/chromium/LayerChromium.h:260
> +    // chromium compositor to avoid mangling the existing meaning of drawsContent().

Could we document the meaning of m_isDrawable and m_contentsVisible (and their relationship / difference) in a self-contained way, without reference to WebCore? I have to say I have no idea what they mean beyond "if it's false, then it doesn't draw the layer".
Also, I would put them next to each other, so that the naive user doesn't find one and decides to use it, instead of the other one that may be more appropriate.

>> Source/WebKit/chromium/src/WebExternalTextureLayerImpl.cpp:-54
>> -bool WebExternalTextureLayerImpl::drawsContent() const
> 
> Looking again, I have a feeling this is wrong.  It does override the PluginLayerChromium drawsContent, even though it is no longer part of CCLayerDelegate.  So I'll add this back.

Ah, yes, that was the thing.
Definitely the previous mechanism was icky and I'm glad you're fixing this.
Calling setIsDrawable (or setContentsVisible? I do not understand the difference) when we change the texture sounds like the correct thing to do.

>> Source/WebKit/chromium/src/WebLayerImpl.cpp:-49
>> -bool WebLayerImpl::drawsContent() const
> 
> In retrospect, I think we should add "setIsDrawable(false)" in the constructor, so that we dont have to trust that LayerChromium initializes it to false.

Possibly I would just ASSERT that it's the case in the constructor. IMO that gives better information, and you don't pay the (minuscule) cost.

[Shawn Singh]

Comment on attachment 119382 [details]
exact same code, but this is the complete patch including tests

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

>> Source/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:114
>> +    return LayerChromium::drawsContent() && TiledLayerChromium::drawsContent();
> 
> Could we have TiledLayerChromium::drawsContent call LayerChromium::drawsContent and remove ContentLayerChromium::drawsContent ?
> As it is now, it's certainly very confusing.

OK I'll do that.   It should work, but doing this also implies we are changing the behavior of ImageLayerChromium, which previously was never asking CCDelegate::drawsContent().

>> Source/WebCore/platform/graphics/chromium/LayerChromium.h:260
>> +    // chromium compositor to avoid mangling the existing meaning of drawsContent().
> 
> Could we document the meaning of m_isDrawable and m_contentsVisible (and their relationship / difference) in a self-contained way, without reference to WebCore? I have to say I have no idea what they mean beyond "if it's false, then it doesn't draw the layer".
> Also, I would put them next to each other, so that the naive user doesn't find one and decides to use it, instead of the other one that may be more appropriate.

OK, will do that =)

>>> Source/WebKit/chromium/src/WebExternalTextureLayerImpl.cpp:-54
>>> -bool WebExternalTextureLayerImpl::drawsContent() const
>> 
>> Looking again, I have a feeling this is wrong.  It does override the PluginLayerChromium drawsContent, even though it is no longer part of CCLayerDelegate.  So I'll add this back.
> 
> Ah, yes, that was the thing.
> Definitely the previous mechanism was icky and I'm glad you're fixing this.
> Calling setIsDrawable (or setContentsVisible? I do not understand the difference) when we change the texture sounds like the correct thing to do.

contentsVisible is only for the CSS visibility property, as far as I know.  isDrawable is more of an internal property, since many layers may be created that simply are not intended to be drawn (like transform layers or some containers).  Hopefully better comments on my part will make that clear =)

So after making that clarification... I don't think we should change isDrawable when the texture changes.  Instead, the return value of drawsContent() should change.  So we just override the PluginLayerChromium::drawsContent virtual function, exactly like it was before.  In other words, this drawsContent() is unrelated to CCLayerDelegate, and its appropriate to keep it.  I'm pretty sure this is correct - does it sound correct to you?

>>> Source/WebKit/chromium/src/WebLayerImpl.cpp:-49
>>> -bool WebLayerImpl::drawsContent() const
>> 
>> In retrospect, I think we should add "setIsDrawable(false)" in the constructor, so that we dont have to trust that LayerChromium initializes it to false.
> 
> Possibly I would just ASSERT that it's the case in the constructor. IMO that gives better information, and you don't pay the (minuscule) cost.

OK, will do =)

[Shawn Singh]

(In reply to comment #16)
> (From update of attachment 119382 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=119382&action=review
> 
> >> Source/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:114
> >> +    return LayerChromium::drawsContent() && TiledLayerChromium::drawsContent();
> > 
> > Could we have TiledLayerChromium::drawsContent call LayerChromium::drawsContent and remove ContentLayerChromium::drawsContent ?
> > As it is now, it's certainly very confusing.
> 
> OK I'll do that.   It should work, but doing this also implies we are changing the behavior of ImageLayerChromium, which previously was never asking CCDelegate::drawsContent().
> 
> >> Source/WebCore/platform/graphics/chromium/LayerChromium.h:260
> >> +    // chromium compositor to avoid mangling the existing meaning of drawsContent().
> > 
> > Could we document the meaning of m_isDrawable and m_contentsVisible (and their relationship / difference) in a self-contained way, without reference to WebCore? I have to say I have no idea what they mean beyond "if it's false, then it doesn't draw the layer".
> > Also, I would put them next to each other, so that the naive user doesn't find one and decides to use it, instead of the other one that may be more appropriate.
> 
> OK, will do that =)
> 
> >>> Source/WebKit/chromium/src/WebExternalTextureLayerImpl.cpp:-54
> >>> -bool WebExternalTextureLayerImpl::drawsContent() const
> >> 
> >> Looking again, I have a feeling this is wrong.  It does override the PluginLayerChromium drawsContent, even though it is no longer part of CCLayerDelegate.  So I'll add this back.
> > 
> > Ah, yes, that was the thing.
> > Definitely the previous mechanism was icky and I'm glad you're fixing this.
> > Calling setIsDrawable (or setContentsVisible? I do not understand the difference) when we change the texture sounds like the correct thing to do.
> 
> contentsVisible is only for the CSS visibility property, as far as I know.  isDrawable is more of an internal property, since many layers may be created that simply are not intended to be drawn (like transform layers or some containers).  Hopefully better comments on my part will make that clear =)
> 
> So after making that clarification... I don't think we should change isDrawable when the texture changes.  Instead, the return value of drawsContent() should change.  So we just override the PluginLayerChromium::drawsContent virtual function, exactly like it was before.  In other words, this drawsContent() is unrelated to CCLayerDelegate, and its appropriate to keep it.  I'm pretty sure this is correct - does it sound correct to you?
> 
> >>> Source/WebKit/chromium/src/WebLayerImpl.cpp:-49
> >>> -bool WebLayerImpl::drawsContent() const
> >> 
> >> In retrospect, I think we should add "setIsDrawable(false)" in the constructor, so that we dont have to trust that LayerChromium initializes it to false.
> > 
> > Possibly I would just ASSERT that it's the case in the constructor. IMO that gives better information, and you don't pay the (minuscule) cost.
> 
> OK, will do =)

Sorry for the noise.  It wouldn't change ImageLayerChromium, because its a virtual function that gets overridden.  However, I feel like that is very misleading for readability, too =)  if there's a way to make drawsContent more consistent across implementations, I'll do that.

[James Robinson]

Do we need a isDrawable and contentsVisible flag on LayerChromium? Can we collapse them to something that makes sense on its own?  As far as I can tell they do the same thing.

[Shawn Singh]

Created attachment 119539 [details]
addressed feedback from all reviewers

[James Robinson]

Comment on attachment 119539 [details]
addressed feedback from all reviewers

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

> LayoutTests/compositing/visibility/visibility-image-layers.html:67
> +  <p>(Pixel test only) The left and right should look the same.</p>

please don't put text in pixel tests - this will make the pixel output platform specific :(

> LayoutTests/compositing/visibility/visibility-simple-canvas2d-layer.html:45
> +    <p>(Pixel test only) Only the green canvas2d should be visible. The other two should be hidden.</p>

same here - please do NOT put text in the pixel output of pixel tests

[Shawn Singh]

Created attachment 119543 [details]
removed text from pixel tests

[James Robinson]

Comment on attachment 119543 [details]
removed text from pixel tests

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

Some comments, mostly nitpicks. Looks like this doesn't apply cleanly on ToT

> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:222
> +    // Note carefully this early-exit is only correct because we also properly initialize
> +    // LayerChromium::isDrawable() whenever m_contentsLayer is set to a new layer in setupContentsLayer().

It looks like this is describing an invariant that must be maintained by code in setupContentsLayer(). If so, can you please put the comment next to the code maintaining the invariant so that if people come along later to change that code around preserve the correct behavior? If someone were to rewrite setupContentsLayer() they are not likely to look here.

> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:233
> +    // Note carefully this early-exit is only correct because we also properly initialize
> +    // LayerChromium::isDrawable() whenever m_contentsLayer is set to a new layer in setupContentsLayer().

Same here as above

> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:604
> +    if (m_contentsLayer)
> +        m_contentsLayer->setIsDrawable(m_contentsVisible);
> +
>      if (m_drawsContent)
>          m_layer->setNeedsDisplay();

when transitioning from !m_contentsVisible -> m_contentsVisible, what's supposed to trigger a redraw? does that work correctly?

if it doesn't, is this possible to test? one way might be with an img tag in a simple compositing layer that goes from visibility:hidden to visibility:visible

> Source/WebCore/platform/graphics/chromium/LayerChromium.h:256
> +    bool m_isDrawable;

could this live with the other bools?

> LayoutTests/platform/chromium/test_expectations.txt:1207
> +BUGWK71209 : compositing/visibility/visibility-simple-webgl-layer.html = FAIL
> +BUGWK71209 : compositing/visibility/visibility-simple-canvas2d-layer.html = FAIL
> +BUGWK71209 : compositing/visibility/visibility-simple-video-layer.html = FAIL
> +BUGWK71209 : compositing/visibility/visibility-image-layers.html = FAIL

can you mark these as = IMAGE, and only for the platforms that you can't generate baselines for locally?

[Shawn Singh]

Comment on attachment 119543 [details]
removed text from pixel tests

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

>> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:604

> 
> when transitioning from !m_contentsVisible -> m_contentsVisible, what's supposed to trigger a redraw? does that work correctly?
> 
> if it doesn't, is this possible to test? one way might be with an img tag in a simple compositing layer that goes from visibility:hidden to visibility:visible

I don't see a problem here... can you please clarify what you think might go wrong?  This function is called when there is any change in m_contentsLayer or m_drawsContent.  if the layer doesn't draw content, then there's no need to call setNeedsDisplay. In every other case (m_contentsVisibile is newly false or newly true) it calls setNeedsDisplay.

[James Robinson]

I'm not sure there's a problem either, but in the case of an <img src="cats.jpg"> that's composited with the simple container layer optimization, you end up with a situation where drawsContent=false but contentsVisible=true. When switching from visibility:hidden to visibility:visible, we need to change the isDrawable flag on the GraphicsLayer::m_contentsLayer but not the GraphicsLayer::m_layer. I want to make sure that we also kick a new frame in this case. It may be that we already do, but if so I'm not sure where that would happen.

What about having LayerChromium::setIsDrawable() call setNeedsCommit() whenever the flag changes value? We need to propagate that value over to the impl side and it seems that today most LayerChromium::set* functions do trigger a commit.

[Shawn Singh]

(In reply to comment #24)
> I'm not sure there's a problem either, but in the case of an <img src="cats.jpg"> that's composited with the simple container layer optimization, you end up with a situation where drawsContent=false but contentsVisible=true. When switching from visibility:hidden to visibility:visible, we need to change the isDrawable flag on the GraphicsLayer::m_contentsLayer but not the GraphicsLayer::m_layer. I want to make sure that we also kick a new frame in this case. It may be that we already do, but if so I'm not sure where that would happen.
> 
> What about having LayerChromium::setIsDrawable() call setNeedsCommit() whenever the flag changes value? We need to propagate that value over to the impl side and it seems that today most LayerChromium::set* functions do trigger a commit.

OK I'll do that; new patch coming soon.

FYI, I just saw that this situation is covered by compositing/visibility/visibility-image-layers-dynamic.html, so we dont have to add another test.

[James Robinson]

Comment on attachment 119543 [details]
removed text from pixel tests

Clearing review? since another patch is upcoming. Set it back if this patch is supposed to be ready to go (although it doesn't apply cleanly)

[Shawn Singh]

Created attachment 120070 [details]
addressed feedback, updated a few unit tests

no obvious regressions on osx layout tests.

[Shawn Singh]

Created attachment 120961 [details]
updated to close to tip of tree

[Shawn Singh]

Created attachment 120968 [details]
really updated to tip of tree to fix merge conflict

[James Robinson]

Comment on attachment 120968 [details]
really updated to tip of tree to fix merge conflict

This looks great, thanks!

[WebKit Review Bot]

Comment on attachment 120968 [details]
really updated to tip of tree to fix merge conflict

Clearing flags on attachment: 120968

Committed r103990: <http://trac.webkit.org/changeset/103990>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.