Bug 73711 - [chromium] setNeedsCommit on non-composited host layers should trigger commit
Summary: [chromium] setNeedsCommit on non-composited host layers should trigger commit
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Adrienne Walker
URL:
Keywords:
Depends on:
Blocks: 72686
  Show dependency treegraph
 
Reported: 2011-12-02 15:30 PST by Adrienne Walker
Modified: 2012-01-04 13:34 PST (History)
7 users (show)

See Also:


Attachments
Patch (7.94 KB, patch)
2011-12-02 15:45 PST, Adrienne Walker
no flags Details | Formatted Diff | Diff
Rebased against ToT (7.92 KB, patch)
2011-12-02 18:16 PST, Adrienne Walker
no flags Details | Formatted Diff | Diff
Patch (18.46 KB, patch)
2011-12-04 14:12 PST, Adrienne Walker
no flags Details | Formatted Diff | Diff
Always set commit for replica layer (18.27 KB, patch)
2011-12-04 22:15 PST, Adrienne Walker
no flags Details | Formatted Diff | Diff
Remove GraphicsLayerChromium checks (21.95 KB, patch)
2011-12-05 13:19 PST, Adrienne Walker
no flags Details | Formatted Diff | Diff
OwnPtr workaround from thakis (22.00 KB, patch)
2011-12-06 15:26 PST, Adrienne Walker
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adrienne Walker 2011-12-02 15:30:58 PST
Calling LayerChromium::setNeedsCommit on the root layer is a no-op, because NonCompositedContentHost::notifySyncRequired doesn't do anything.
Comment 1 Adrienne Walker 2011-12-02 15:45:35 PST
Created attachment 117707 [details]
Patch
Comment 2 Adrienne Walker 2011-12-02 15:46:58 PST
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 3 WebKit Review Bot 2011-12-02 15:58:45 PST
Comment on attachment 117707 [details]
Patch

Attachment 117707 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10729201
Comment 4 Adrienne Walker 2011-12-02 16:10:04 PST
(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.
Comment 5 Eric Penner 2011-12-02 17:55:35 PST
(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.
Comment 6 Adrienne Walker 2011-12-02 18:16:50 PST
Created attachment 117725 [details]
Rebased against ToT
Comment 7 Adrienne Walker 2011-12-02 18:25:11 PST
(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 8 WebKit Review Bot 2011-12-02 19:02:06 PST
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
Comment 9 Adrienne Walker 2011-12-04 14:12:54 PST
Created attachment 117806 [details]
Patch
Comment 10 Adrienne Walker 2011-12-04 14:17:12 PST
(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 11 WebKit Review Bot 2011-12-04 16:46:07 PST
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
Comment 12 Adrienne Walker 2011-12-04 22:15:09 PST
Created attachment 117842 [details]
Always set commit for replica layer
Comment 13 Adrienne Walker 2011-12-04 22:16:31 PST
(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.
Comment 14 James Robinson 2011-12-05 11:26:48 PST
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 15 James Robinson 2011-12-05 11:32:23 PST
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.
Comment 16 Adrienne Walker 2011-12-05 13:19:29 PST
Created attachment 117930 [details]
Remove GraphicsLayerChromium checks
Comment 17 James Robinson 2011-12-05 13:51:41 PST
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
Comment 18 James Robinson 2011-12-05 13:55:17 PST
(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.
Comment 19 Adrienne Walker 2011-12-05 14:41:27 PST
(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 20 WebKit Review Bot 2011-12-05 21:05:23 PST
Comment on attachment 117930 [details]
Remove GraphicsLayerChromium checks

Clearing flags on attachment: 117930

Committed r102091: <http://trac.webkit.org/changeset/102091>
Comment 21 WebKit Review Bot 2011-12-05 21:05:29 PST
All reviewed patches have been landed.  Closing bug.
Comment 22 Dimitri Glazkov (Google) 2011-12-06 12:28:07 PST
Reverted r102091 for reason:

Caused Clang Linux compile failure.

Committed r102165: <http://trac.webkit.org/changeset/102165>
Comment 23 James Robinson 2011-12-06 12:32:10 PST
(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
Comment 24 James Robinson 2011-12-06 12:37:09 PST
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?
Comment 25 Adrienne Walker 2011-12-06 15:26:34 PST
Created attachment 118122 [details]
OwnPtr workaround from thakis
Comment 26 Adrienne Walker 2011-12-06 15:29:06 PST
I sent this out for a linux_clang try job to double check.  Will update this bug with the results once it comes back.
Comment 27 Adrienne Walker 2011-12-06 16:36:54 PST
(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 28 WebKit Review Bot 2011-12-06 17:18:06 PST
Comment on attachment 118122 [details]
OwnPtr workaround from thakis

Clearing flags on attachment: 118122

Committed r102196: <http://trac.webkit.org/changeset/102196>
Comment 29 WebKit Review Bot 2011-12-06 17:18:12 PST
All reviewed patches have been landed.  Closing bug.
Comment 30 Nico Weber 2012-01-04 13:34:08 PST
For reference, here's a reduced version of the problem: http://pastie.org/2977068