[chromium] CCLayerDelegate and WebLayerClient do not need notifySyncRequired
Created attachment 118930 [details] Patch
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.
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
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.
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.
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.
(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.
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?
(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()
(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.
(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.
Created attachment 119042 [details] Patch
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.
(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.
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.
Setting up the LTH properly doesn't look hard, I'll do that first (as a separate patch to land before this one).
Comment on attachment 119042 [details] Patch Clearing review? flag until the LTH pointer is set up on all LayerChromiums properly.
Created attachment 119538 [details] Patch
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.
Comment on attachment 119538 [details] Patch It looks fine to me.
Comment on attachment 119538 [details] Patch rs=me based on Vangelis' unofficial review.
Committed r103135: <http://trac.webkit.org/changeset/103135>
Reverted in http://trac.webkit.org/changeset/103150 because of its dependencies on bug 74477, which was also rolled out.
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.
Created attachment 119971 [details] Patch
Committed r103293: <http://trac.webkit.org/changeset/103293>
This patch appears to have broken Win builds: http://build.webkit.org/builders/Chromium%20Win%20Release/builds/37102/steps/compile-webkit/logs/stdio
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
I think the previous error was because of an overzealous using namespace; declaration, but I don't see anything like that in WebGLLayerChromiumTest.cpp
Ah I see, it's not necessary WebGLLayerChromiumTest - that just happened to be the next file in the output log.
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.
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 :/
http://trac.webkit.org/changeset/103300 will hopefully fix the chromium windows compile.
Seems that worked: http://build.webkit.org/builders/Chromium%20Win%20Release/builds/37108