Bug 74376 - [chromium] CCLayerDelegate and WebLayerClient do not need notifySyncRequired
[chromium] CCLayerDelegate and WebLayerClient do not need notifySyncRequired
 Status: RESOLVED FIXED None WebKit Unclassified New Bugs (show other bugs) 528+ (Nightly build) Unspecified Unspecified P2 Normal James Robinson 74477 Show dependency tree / graph

 Reported: 2011-12-12 18:28 PST by James Robinson 2011-12-19 20:44 PST (History) 12 users (show) cc-bugs danakj enne fishd kbr nduca piman reveman rniwa shawnsingh vangelis webkit.review.bot

Attachments
Patch (89.55 KB, patch)
2011-12-12 18:37 PST, James Robinson
no flags Details | Formatted Diff | Diff
Patch (133.84 KB, patch)
2011-12-13 11:02 PST, James Robinson
no flags Details | Formatted Diff | Diff
Patch (135.44 KB, patch)
2011-12-15 18:55 PST, James Robinson
no flags Details | Formatted Diff | Diff
Patch (133.55 KB, patch)
2011-12-19 18:54 PST, James Robinson
no flags Details | Formatted Diff | Diff

 Note You need to log in before you can comment on or make changes to this bug.
 James Robinson 2011-12-12 18:28:55 PST [chromium] CCLayerDelegate and WebLayerClient do not need notifySyncRequired James Robinson 2011-12-12 18:37:35 PST Created attachment 118930 [details] Patch James Robinson 2011-12-12 18:40:13 PST 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 2011-12-12 18:43:20 PST Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API. Antoine Labour 2011-12-12 19:46:16 PST 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 2011-12-12 20:36:05 PST 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) 2011-12-12 21:43:53 PST 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 2011-12-13 02:18:41 PST (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 2011-12-13 10:19:50 PST 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 2011-12-13 10:34:05 PST (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 2011-12-13 10:40:24 PST (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 2011-12-13 10:41:07 PST (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 2011-12-13 11:02:59 PST Created attachment 119042 [details] Patch James Robinson 2011-12-13 11:05:30 PST 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 2011-12-13 11:16:21 PST (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 2011-12-13 11:30:31 PST 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 2011-12-13 11:31:28 PST 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 2011-12-13 11:31:50 PST Comment on attachment 119042 [details] Patch Clearing review? flag until the LTH pointer is set up on all LayerChromiums properly. James Robinson 2011-12-15 18:55:29 PST Created attachment 119538 [details] Patch James Robinson 2011-12-15 18:56:53 PST 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 2011-12-16 15:24:09 PST Comment on attachment 119538 [details] Patch It looks fine to me. Kenneth Russell 2011-12-16 15:43:12 PST Comment on attachment 119538 [details] Patch rs=me based on Vangelis' unofficial review. James Robinson 2011-12-16 18:11:59 PST Committed r103135:  Adrienne Walker 2011-12-17 10:36:48 PST Reverted in http://trac.webkit.org/changeset/103150 because of its dependencies on bug 74477, which was also rolled out. James Robinson 2011-12-19 18:51:16 PST 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 2011-12-19 18:54:11 PST Created attachment 119971 [details] Patch James Robinson 2011-12-19 18:55:02 PST Committed r103293:  Ryosuke Niwa 2011-12-19 19:13:20 PST 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 2011-12-19 19:13:45 PST 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 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::append(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 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::append(const U &)' being compiled 15> with 15> [ 15> T=const WebCore::AnimationValue *, 15> U=WebCore::AnimationValue * 15> ] 15>WebGLLayerChromiumTest.cpp James Robinson 2011-12-19 19:40:09 PST 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 2011-12-19 19:41:25 PST Ah I see, it's not necessary WebGLLayerChromiumTest - that just happened to be the next file in the output log. James Robinson 2011-12-19 19:45:38 PST 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 2011-12-19 20:01:11 PST 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 2011-12-19 20:34:57 PST http://trac.webkit.org/changeset/103300 will hopefully fix the chromium windows compile. James Robinson 2011-12-19 20:44:29 PST Seems that worked: http://build.webkit.org/builders/Chromium%20Win%20Release/builds/37108