WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
71209
[Chromium] Fix drawsContent and contentsVisible in accelerated compositor
https://bugs.webkit.org/show_bug.cgi?id=71209
Summary
[Chromium] Fix drawsContent and contentsVisible in accelerated compositor
Andrey Kosyakov
Reported
2011-10-31 01:18:41 PDT
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
Attachments
work in progress, this version crashes
(11.06 KB, patch)
2011-12-13 10:44 PST
,
Shawn Singh
no flags
Details
Formatted Diff
Diff
code changes only, for easier review
(17.54 KB, patch)
2011-12-14 22:32 PST
,
Shawn Singh
no flags
Details
Formatted Diff
Diff
exact same code, but this is the complete patch including tests
(153.84 KB, patch)
2011-12-14 22:47 PST
,
Shawn Singh
no flags
Details
Formatted Diff
Diff
addressed feedback from all reviewers
(157.65 KB, patch)
2011-12-15 18:55 PST
,
Shawn Singh
no flags
Details
Formatted Diff
Diff
removed text from pixel tests
(130.68 KB, patch)
2011-12-15 19:32 PST
,
Shawn Singh
no flags
Details
Formatted Diff
Diff
addressed feedback, updated a few unit tests
(129.93 KB, patch)
2011-12-20 13:39 PST
,
Shawn Singh
no flags
Details
Formatted Diff
Diff
updated to close to tip of tree
(129.83 KB, patch)
2012-01-03 11:03 PST
,
Shawn Singh
no flags
Details
Formatted Diff
Diff
really updated to tip of tree to fix merge conflict
(129.84 KB, patch)
2012-01-03 11:21 PST
,
Shawn Singh
no flags
Details
Formatted Diff
Diff
Show Obsolete
(7)
View All
Add attachment
proposed patch, testcase, etc.
Shawn Singh
Comment 1
2011-12-09 16:10:39 PST
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
Comment 2
2011-12-13 10:44:09 PST
Created
attachment 119035
[details]
work in progress, this version crashes
Shawn Singh
Comment 3
2011-12-13 10:45:50 PST
(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 4
2011-12-13 11:09:29 PST
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
Comment 5
2011-12-13 11:10:34 PST
(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
Comment 6
2011-12-13 11:12:45 PST
Wow, I really should have seen that. Thanks... saved me a lot of frustration. =)
Shawn Singh
Comment 7
2011-12-14 22:32:08 PST
Created
attachment 119380
[details]
code changes only, for easier review
Shawn Singh
Comment 8
2011-12-14 22:47:14 PST
Created
attachment 119382
[details]
exact same code, but this is the complete patch including tests
Shawn Singh
Comment 9
2011-12-14 22:51:49 PST
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 10
2011-12-14 22:54:14 PST
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 11
2011-12-14 23:28:10 PST
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 12
2011-12-15 01:36:05 PST
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 13
2011-12-15 09:17:38 PST
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 14
2011-12-15 09:50:45 PST
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 15
2011-12-15 10:10:40 PST
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 16
2011-12-15 10:57:20 PST
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
Comment 17
2011-12-15 11:10:33 PST
(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
Comment 18
2011-12-15 13:13:25 PST
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
Comment 19
2011-12-15 18:55:53 PST
Created
attachment 119539
[details]
addressed feedback from all reviewers
James Robinson
Comment 20
2011-12-15 19:01:43 PST
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
Comment 21
2011-12-15 19:32:21 PST
Created
attachment 119543
[details]
removed text from pixel tests
James Robinson
Comment 22
2011-12-15 23:46:06 PST
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 23
2011-12-16 13:13:47 PST
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
Comment 24
2011-12-16 13:42:10 PST
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
Comment 25
2011-12-16 14:01:04 PST
(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 26
2011-12-19 17:40:13 PST
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
Comment 27
2011-12-20 13:39:07 PST
Created
attachment 120070
[details]
addressed feedback, updated a few unit tests no obvious regressions on osx layout tests.
Shawn Singh
Comment 28
2012-01-03 11:03:07 PST
Created
attachment 120961
[details]
updated to close to tip of tree
Shawn Singh
Comment 29
2012-01-03 11:21:10 PST
Created
attachment 120968
[details]
really updated to tip of tree to fix merge conflict
James Robinson
Comment 30
2012-01-03 12:29:51 PST
Comment on
attachment 120968
[details]
really updated to tip of tree to fix merge conflict This looks great, thanks!
WebKit Review Bot
Comment 31
2012-01-03 16:54:02 PST
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
Comment 32
2012-01-03 16:54:10 PST
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug