Bug 74376 – [chromium] CCLayerDelegate and WebLayerClient do not need notifySyncRequired

• Component: New Bugs
• Status: RESOLVED FIXED

[James Robinson]

[chromium] CCLayerDelegate and WebLayerClient do not need notifySyncRequired

[James Robinson]

Created attachment 118930 [details]
Patch

[James Robinson]

This patch looks big but it's 5% code and 95% fighting with test infrastructure, so it isn't as bad as it looks.

The next step here (which either Shawn or myself will tackle) is removing CCLayerDelegate::drawsContent() by pushing that bit to LayerChromiums at the appropriate time rather than pulling it.  Then the only thing left on CCLayerDelegate will be paintContents() at which point the interface is renamed ContentLayerChromiumDelegate (or CCContentLayerClient or whatever) and is only set on ContentLayerChromium instances instead of all layers.  I think this will really help make it clearer WTF is going on with our various layer trees and back pointers.

I also did a fair amount of cleanup to our test harnesses, such as extracting the two (!) copies of CompositorMockWebGraphicsContext3D into a shared location.

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[Antoine Labour]

Comment on attachment 118930 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:152
> +    child->setLayerTreeHost(m_layerTreeHost.get());

I suspect we have more than just this to propagate the m_layerTreeHost on every layer.
For example, if we first create the layer tree, and only then set the root layer, it won't propagate.
Also, should we propagate to the mask and replica layers.

Finally, for completeness, we probably want to clean up the LTH when we remove layers from the tree.

[James Robinson]

I'm pretty minimal in propagating LayerChromium::m_layerTreeHost pointers in this patch to keep it managable, although I think cleaning that up will be a great follow-up.  We do a pass over all layers (including masks, replicas, etc) in the paint loop that sets the layerTreeHost pointer when actually compositing, so I only did the minimal to set up LTH pointers to pass our unit tests.  I think a better strategy would be to propagate the LTH pointer to subtrees whenever they attach to the tree and clear it when they detach.

[Darin Fisher (:fishd, Google)]

Comment on attachment 118930 [details]
Patch

API changes LGTM... happy to officially R+ the rest if Antoine/Nat say that they are happy with the change.

[Nat Duca]

(In reply to comment #5)
> ... propagate the LTH pointer to subtrees whenever they attach to the tree and clear it when they detach.

Yes please. Will look over this in detail tomorrow. This direction feels really promising overall. Thanks James.

[Nat Duca]

Comment on attachment 118930 [details]
Patch

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

LGTM with nits

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:106
> +    if (m_proxy)

When can you not have a proxy? Iirw, you always have a proxy.

> Source/WebKit/chromium/WebKit.gypi:72
> +            'tests/CompositorMockGraphicsContext3D.h',

Not loving this name. If we have a CCRenderer [which Enne is promising me they will make someday soon], does that help with a name?

Also is it mock or fake?

> Source/WebKit/chromium/tests/CompositorMockWebGraphicsContext3D.h:32
> +// Test stub for WebGraphicsContext3D. Returns canned values needed for compositor initialization.

Yeah, definitely not a mock. Fake.

> Source/WebKit/chromium/tests/LayerChromiumTest.cpp:135
> +        m_parent = LayerChromium::create(0);

Should we be using the new style constructor here?

[James Robinson]

(In reply to comment #8)
> (From update of attachment 118930 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=118930&action=review
> 
> LGTM with nits
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:106
> > +    if (m_proxy)
> 
> When can you not have a proxy? Iirw, you always have a proxy.

In some tests we create a CCLayerTreeHost but don't use the create() and thus don't call initialize(), so we don't construct a proxy.

But now that I think about it that's probably a bug in the test.  Will remove and fix up the test if it looks reasonable.

> 
> > Source/WebKit/chromium/WebKit.gypi:72
> > +            'tests/CompositorMockGraphicsContext3D.h',
> 
> Not loving this name. If we have a CCRenderer [which Enne is promising me they will make someday soon], does that help with a name?
> 
> Also is it mock or fake?

It's a fake - will rename.  It's a fake GraphicsContext3D that is capable of passing compositor initialization.

I'm not sure how CCRenderer is related.

> 
> > Source/WebKit/chromium/tests/CompositorMockWebGraphicsContext3D.h:32
> > +// Test stub for WebGraphicsContext3D. Returns canned values needed for compositor initialization.
> 
> Yeah, definitely not a mock. Fake.
> 
> > Source/WebKit/chromium/tests/LayerChromiumTest.cpp:135
> > +        m_parent = LayerChromium::create(0);
> 
> Should we be using the new style constructor here?

not sure what you mean by 'new style' - i haven't changed LayerChromium::create()

[James Robinson]

(In reply to comment #9)
> (In reply to comment #8)
> > (From update of attachment 118930 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=118930&action=review
> > 
> > LGTM with nits
> > 
> > > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:106
> > > +    if (m_proxy)
> > 
> > When can you not have a proxy? Iirw, you always have a proxy.
> 
> In some tests we create a CCLayerTreeHost but don't use the create() and thus don't call initialize(), so we don't construct a proxy.
> 
> But now that I think about it that's probably a bug in the test.  Will remove and fix up the test if it looks reasonable.
> 

Aha, LayerChromiumTest had a MockLayerTreeHost that wasn't calling initialize(). Will fix.

[Nat Duca]

(In reply to comment #9)
> > > Source/WebKit/chromium/WebKit.gypi:72
> > > +            'tests/CompositorMockGraphicsContext3D.h',
> I'm not sure how CCRenderer is related.

Oh, I was off in the clouds thinking this was a CCRendererGraphicsContext or something. Don't mind me. :)

> > > Source/WebKit/chromium/tests/LayerChromiumTest.cpp:135
> > > +        m_parent = LayerChromium::create(0);
> > 
> > Should we be using the new style constructor here?
> 
> not sure what you mean by 'new style' - i haven't changed LayerChromium::create()

Was suggesting defaulting, eg LayerChromium::create(LayerDelegate* delegate = 0) to save diffs down the road. NBD TBH.

[James Robinson]

Created attachment 119042 [details]
Patch

[James Robinson]

Addressed nits.

The patch looks even bigger now because I've renamed (Compositor|)Mock(Web|)GraphicsContext3D(Test|) to *Fake*, but that's just a do-webcore-rename.

[Antoine Labour]

(In reply to comment #5)
> I'm pretty minimal in propagating LayerChromium::m_layerTreeHost pointers in this patch to keep it managable, although I think cleaning that up will be a great follow-up.  We do a pass over all layers (including masks, replicas, etc) in the paint loop that sets the layerTreeHost pointer when actually compositing, so I only did the minimal to set up LTH pointers to pass our unit tests.  I think a better strategy would be to propagate the LTH pointer to subtrees whenever they attach to the tree and clear it when they detach.

I'm fairly uncomfortable leaving things in this state. There is a race condition between the painting and the tree modification (which then won't setNeedsCommit). Even then, the painting phase doesn't set the LTH on all layers, only on those that paint and that are visible.
TBH, I wouldn't be surprised it breaks Aura in some cases.

[Adrienne Walker]

Comment on attachment 119042 [details]
Patch

This all looks great to me, except I agree with piman's worries about the LTH changes.  For example, what if you add a non-visible child, then do the first commit (caused by addChild), and then change that child's transform such that it's now visible.  That second change won't cause a commit, because the LTH isn't hooked up to that child, because it didn't paint the first time around, even though it should.

[James Robinson]

Setting up the LTH properly doesn't look hard, I'll do that first (as a separate patch to land before this one).

[James Robinson]

Comment on attachment 119042 [details]
Patch

Clearing review? flag until the LTH pointer is set up on all LayerChromiums properly.

[James Robinson]

Created attachment 119538 [details]
Patch

[James Robinson]

Same patch rebased on top of http://trac.webkit.org/changeset/103011 which makes sure that the m_layerTreeHost is accurate for LayerChromiums all the time, so setNeedsCommit() calls should always trigger a new frame when a layer in the tree is modified.  Modifying layers that are not in a LTH does not trigger a new frame, but adding the layer to the tree will.

[Vangelis Kokkevis]

Comment on attachment 119538 [details]
Patch

It looks fine to me.

[Kenneth Russell]

Comment on attachment 119538 [details]
Patch

rs=me based on Vangelis' unofficial review.

[James Robinson]

Committed r103135: <http://trac.webkit.org/changeset/103135>

[Adrienne Walker]

Reverted in http://trac.webkit.org/changeset/103150 because of its dependencies on bug 74477, which was also rolled out.

[James Robinson]

After relanding https://bugs.webkit.org/show_bug.cgi?id=74477 and resolving conflicts, this patch appears to pass all of (webkit|aura|views|compositor)_unittests. Will post a patch against ToT for posterity and reland.

[James Robinson]

Created attachment 119971 [details]
Patch

[James Robinson]

Committed r103293: <http://trac.webkit.org/changeset/103293>

[Ryosuke Niwa]

This patch appears to have broken Win builds:
http://build.webkit.org/builders/Chromium%20Win%20Release/builds/37102/steps/compile-webkit/logs/stdio

[Ryosuke Niwa]

NotNull error again:

15>e:\google-windows-1\chromium-win-release\build\Source\JavaScriptCore\wtf/Vector.h(930) : error C2872: 'NotNull' : ambiguous symbol
15>        could be 'e:\google-windows-1\chromium-win-release\build\source\javascriptcore\wtf\StdLibExtras.h(279) : NotNullTag NotNull'
15>        or       'e:\google-windows-1\chromium-win-release\build\Source\WebKit\chromium\testing\gmock\include\gmock/gmock-matchers.h(2644) : testing::PolymorphicMatcher<Impl> testing::NotNull(void)'
15>        with
15>        [
15>            Impl=testing::internal::NotNullMatcher
15>        ]
15>        e:\google-windows-1\chromium-win-release\build\source\javascriptcore\wtf\text\WTFString.h(582) : see reference to function template instantiation 'void WTF::Vector<T>::append<UChar>(const U *,size_t)' being compiled
15>        with
15>        [
15>            T=UChar,
15>            U=UChar
15>        ]
15>e:\google-windows-1\chromium-win-release\build\Source\JavaScriptCore\wtf/Vector.h(972) : error C2872: 'NotNull' : ambiguous symbol
15>        could be 'e:\google-windows-1\chromium-win-release\build\source\javascriptcore\wtf\StdLibExtras.h(279) : NotNullTag NotNull'
15>        or       'e:\google-windows-1\chromium-win-release\build\Source\WebKit\chromium\testing\gmock\include\gmock/gmock-matchers.h(2644) : testing::PolymorphicMatcher<Impl> testing::NotNull(void)'
15>        with
15>        [
15>            Impl=testing::internal::NotNullMatcher
15>        ]
15>        e:\google-windows-1\chromium-win-release\build\source\webcore\platform\graphics\GraphicsLayer.h(174) : see reference to function template instantiation 'void WTF::Vector<T>::append<WebCore::AnimationValue*>(const U &)' being compiled
15>        with
15>        [
15>            T=const WebCore::AnimationValue *,
15>            U=WebCore::AnimationValue *
15>        ]
15>WebGLLayerChromiumTest.cpp

[James Robinson]

I think the previous error was because of an overzealous using namespace; declaration, but I don't see anything like that in WebGLLayerChromiumTest.cpp

[James Robinson]

Ah I see, it's not necessary WebGLLayerChromiumTest - that just happened to be the next file in the output log.

[James Robinson]

I think this is in WebLayerTest.cpp, which is one of the two files (along with MockGraphicsContext3DTest.cpp) that has a using namespace testing; declaration.  This patch added some additional #includes to WebLayerTest.cpp one of which probably pulled in Vector.h causing the conflict.

I think the fix is to tighten up the using declaration in WebLayerTest.cpp to not pull in the entire testing:: namespace into the global namespace.

[James Robinson]

Sadly I don't have a quickfix ready and need to go. If someone could try replacing 'using namespace testing' in WebLayerTest.cpp with 'using testing::Mock' and see if that compiles anywhere, that would probably fix windows.  Otherwise please revert.  Sorry :/

[James Robinson]

http://trac.webkit.org/changeset/103300 will hopefully fix the chromium windows compile.

[James Robinson]

Seems that worked: http://build.webkit.org/builders/Chromium%20Win%20Release/builds/37108