Bug 73711 – [chromium] setNeedsCommit on non-composited host layers should trigger commit

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Adrienne Walker]

Calling LayerChromium::setNeedsCommit on the root layer is a no-op, because NonCompositedContentHost::notifySyncRequired doesn't do anything.

[Adrienne Walker]

Created attachment 117707 [details]
Patch

[Adrienne Walker]

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.

[WebKit Review Bot]

Comment on attachment 117707 [details]
Patch

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

[Adrienne Walker]

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

[Eric Penner]

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

[Adrienne Walker]

Created attachment 117725 [details]
Rebased against ToT

[Adrienne Walker]

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

[WebKit Review Bot]

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

[Adrienne Walker]

Created attachment 117806 [details]
Patch

[Adrienne Walker]

(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.  :)

[WebKit Review Bot]

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

[Adrienne Walker]

Created attachment 117842 [details]
Always set commit for replica layer

[Adrienne Walker]

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

[James Robinson]

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?

[James Robinson]

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.

[Adrienne Walker]

Created attachment 117930 [details]
Remove GraphicsLayerChromium checks

[James Robinson]

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

[James Robinson]

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

[Adrienne Walker]

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

[WebKit Review Bot]

Comment on attachment 117930 [details]
Remove GraphicsLayerChromium checks

Clearing flags on attachment: 117930

Committed r102091: <http://trac.webkit.org/changeset/102091>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Dimitri Glazkov (Google)]

Reverted r102091 for reason:

Caused Clang Linux compile failure.

Committed r102165: <http://trac.webkit.org/changeset/102165>

[James Robinson]

(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

[James Robinson]

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?

[Adrienne Walker]

Created attachment 118122 [details]
OwnPtr workaround from thakis

[Adrienne Walker]

I sent this out for a linux_clang try job to double check.  Will update this bug with the results once it comes back.

[Adrienne Walker]

(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.  :)

[WebKit Review Bot]

Comment on attachment 118122 [details]
OwnPtr workaround from thakis

Clearing flags on attachment: 118122

Committed r102196: <http://trac.webkit.org/changeset/102196>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Nico Weber]

For reference, here's a reduced version of the problem: http://pastie.org/2977068