Calling LayerChromium::setNeedsCommit on the root layer is a no-op, because NonCompositedContentHost::notifySyncRequired doesn't do anything.
Created attachment 117707 [details] Patch
I'm uploading this as an initial patch. Unfortunately, the combination of this patch and the latest from 72686 together (but not each individually) cause DRT to assert on m_paintRect.isEmpty() in WebViewHost.cpp. I'm investigating.
Comment on attachment 117707 [details] Patch Attachment 117707 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10729201
(In reply to comment #2) > I'm uploading this as an initial patch. > > Unfortunately, the combination of this patch and the latest from 72686 together (but not each individually) cause DRT to assert on m_paintRect.isEmpty() in WebViewHost.cpp. I'm investigating. I think it's the fault of this patch. If LayerChromium::setNeedsCommit calls LayerTreeHost::setNeedsCommit directly, then any change to the root layer (like, say, hypothetically setting the background color in WebViewImplContentPainter) will cause another commit every time you paint. Oy. I suppose we should probably be less promiscuous in calling setNeedsCommit. I can fix that too.
(In reply to comment #4) > (In reply to comment #2) > > I'm uploading this as an initial patch. > > > > Unfortunately, the combination of this patch and the latest from 72686 together (but not each individually) cause DRT to assert on m_paintRect.isEmpty() in WebViewHost.cpp. I'm investigating. > > I think it's the fault of this patch. If LayerChromium::setNeedsCommit calls LayerTreeHost::setNeedsCommit directly, then any change to the root layer (like, say, hypothetically setting the background color in WebViewImplContentPainter) will cause another commit every time you paint. > > Oy. I suppose we should probably be less promiscuous in calling setNeedsCommit. I can fix that too. Sorry I didn't catch this in the initial CL. Keep me posted how I can help.
Created attachment 117725 [details] Rebased against ToT
(In reply to comment #6) > Created an attachment (id=117725) [details] > Rebased against ToT I temporarily removed all the other unnecessary setNeedsCommits, and after that then the prepainting setNeedsCommit itself started triggering asserts in DRT. So, that's not quite the right approach. Prepainting itself is causing the "too many paints causing other paints" assertion in WebViewHost. My feeling is that something like this patch can land first, but then the prepainting patch needs to be adjusted to not do idle prepainting in the compositeAndReadback path. I think that's the easiest path to make the tests happy, and it seems like a fairly reasonable decision. I did verify that this patch successfully allows prepainting to happen, even if there's no scrolling going on. I'm not that happy that I have to expose the zoom animator transform so it can be pulled from CCLayerTreeHost, however I couldn't find a better place to push it from WebViewImpl.
Comment on attachment 117725 [details] Rebased against ToT Attachment 117725 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10734176
Created attachment 117806 [details] Patch
(In reply to comment #9) > Created an attachment (id=117806) [details] > Patch I reconsidered how much logic I was changing in this patch. I just decided to pipe NCH::notifySyncRequired to LayerTreeHost::setNeedsCommit directly rather than adjust LayerChromium and having to play with zoom animator transform. I think this ends up being simpler. Sorry for the flux. I also added some tests. :)
Comment on attachment 117806 [details] Patch Attachment 117806 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10690570 New failing tests: compositing/iframes/iframe-content-flipping.html
Created attachment 117842 [details] Always set commit for replica layer
(In reply to comment #12) > Created an attachment (id=117842) [details] > Always set commit for replica layer I'm not really sure why this is needed, but the iframe flipped test clearly fails with the wrong background color if setNeedsCommit isn't always called.
We have a very similar caching layer in GraphicsLayerChromium. I think we should ditch that if we have this here - there's no reason to be checking the backface/double sided flag here: http://google.com/codesearch#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp&q=GraphicsLayerChromium::setBackfaceVisibility&exact_package=chromium&l=284 and in LayerChromium. Can we just make GraphicsLayerChromium a pure pass-through for property sets with this change?
Comment on attachment 117842 [details] Always set commit for replica layer View in context: https://bugs.webkit.org/attachment.cgi?id=117842&action=review > Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:125 > void LayerChromium::setNeedsCommit() > { > - // Call notifySyncRequired(), which for non-root layers plumbs through to > - // call setRootLayerNeedsDisplay() on the WebView, which will cause LayerRendererChromium > - // to render a frame. > - // This function has no effect on root layers. > if (m_delegate) > m_delegate->notifySyncRequired(); > } this is a larger change, but I think it'd be better if we moved this notification up to CCLayerTreeHost instead of having it on layers. Then the LTH can route this call to either the proxy or the CCLTHClient as necessary. It's a larger change, though, so maybe not in this patch.
Created attachment 117930 [details] Remove GraphicsLayerChromium checks
Comment on attachment 117930 [details] Remove GraphicsLayerChromium checks View in context: https://bugs.webkit.org/attachment.cgi?id=117930&action=review > Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:177 > updateAnchorPoint(); i think the follow-up here is to fold updateAnchorPoint(), etc, into this function
(In reply to comment #17) > (From update of attachment 117930 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=117930&action=review > > > Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:177 > > updateAnchorPoint(); > > i think the follow-up here is to fold updateAnchorPoint(), etc, into this function Not that I want to do that in this patch - let's land this to unblock other work first.
(In reply to comment #18) > (In reply to comment #17) > > (From update of attachment 117930 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=117930&action=review > > > > > Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:177 > > > updateAnchorPoint(); > > > > i think the follow-up here is to fold updateAnchorPoint(), etc, into this function > > Not that I want to do that in this patch - let's land this to unblock other work first. Sounds good. I'll write a follow-up patch to clean up needless functions in GraphicsLayerChromium.
Comment on attachment 117930 [details] Remove GraphicsLayerChromium checks Clearing flags on attachment: 117930 Committed r102091: <http://trac.webkit.org/changeset/102091>
All reviewed patches have been landed. Closing bug.
Reverted r102091 for reason: Caused Clang Linux compile failure. Committed r102165: <http://trac.webkit.org/changeset/102165>
(In reply to comment #22) > Reverted r102091 for reason: > > Caused Clang Linux compile failure. > > Committed r102165: <http://trac.webkit.org/changeset/102165> D'oh, sorry about that - can you link to a failing log? I can't find a linux clang canary
Found this: http://build.chromium.org/p/chromium/builders/Linux%20Clang%20%28dbg%29/builds/17108/steps/compile/logs/stdio#error1 In file included from third_party/WebKit/Source/WebKit/chromium/tests/LayerChromiumTest.cpp:27: In file included from third_party/WebKit/Source/WebCore/platform/graphics/chromium/LayerChromium.h:38: In file included from third_party/WebKit/Source/WebCore/platform/graphics/GraphicsContext.h:31: In file included from third_party/WebKit/Source/WebCore/platform/graphics/DashArray.h:29: In file included from third_party/WebKit/Source/JavaScriptCore/wtf/Vector.h:30: In file included from third_party/WebKit/Source/JavaScriptCore/wtf/VectorTraits.h:24: third_party/WebKit/Source/JavaScriptCore/wtf/OwnPtr.h:52:9:error: function 'WTF::OwnPtr<<anonymous>::MockNonCompositedContentHost>::OwnPtr' has internal linkage but is not defined [-Werror] OwnPtr(const OwnPtr<ValueType>&); ^ third_party/WebKit/Source/WebKit/chromium/tests/LayerChromiumTest.cpp:691:42: note: used here OwnPtr<MockNonCompositedContentHost> nonCompositedContentHost = MockNonCompositedContentHost::create(); ^ 1 error generated. make: *** [out/Debug/obj.target/webkit_unit_tests/third_party/WebKit/Source/WebKit/chromium/tests/LayerChromiumTest.o] Error 1 huh?
Created attachment 118122 [details] OwnPtr workaround from thakis
I sent this out for a linux_clang try job to double check. Will update this bug with the results once it comes back.
(In reply to comment #26) > I sent this out for a linux_clang try job to double check. Will update this bug with the results once it comes back. "You are awesome! Try succeeded!" jamesr: feel free to R+/CQ+ this if you get a chance. :)
Comment on attachment 118122 [details] OwnPtr workaround from thakis Clearing flags on attachment: 118122 Committed r102196: <http://trac.webkit.org/changeset/102196>
For reference, here's a reduced version of the problem: http://pastie.org/2977068