Bug 94145

Created attachment 158633 [details] Patch

Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

Comment on attachment 158633 [details] Patch Attachment 158633 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/13504713

Created attachment 158635 [details] Patch

Created attachment 160489 [details] Patch

Comment on attachment 160489 [details] Patch I'm confused why you want to land the boilerplate separately. I mean, it seems likely that these classes are probably going to be the ones you need, but it's hard to review something with no functionality.

Oh, okay. Just trying to keep patch sizes smaller and do this incrementally. I can do it along with stuff inside the layer instead.

Created attachment 161781 [details] Patch

This is what I am picturing for the DRLayer. It has some tests showing what works (and the last test showing what doesn't yet). It depends on the copy() mechanisms for quads and renderpasses, as well as the new renderpass id pair. There's some fixmes in the CCDRLayerImpl.cpp file where things need to get filled in still. Also the logic in CCLTHI needs to be improved a bit..

Rough design sketch is also available here, though it doesn't match 100% what came out in the end. https://docs.google.com/a/google.com/document/d/1R8HOhHD6fFsqtSmH6a9bSlMOruCu5ufF7p5Og-eQGTM/edit

Created attachment 162908 [details] Patch

Here's a fully functioning ubercomp layer for your enjoyment :D

Created attachment 162910 [details] Patch fixing a typo in assert

Comment on attachment 162910 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=162910&action=review In general, looks great. A few minor nits: > Source/WebCore/platform/graphics/chromium/RenderSurfaceChromium.h:82 > + void addContributingDelegatedRenderPassLayer(LayerChromium*) { } Maybe add a comment about why this can be a no-op? > Source/WebCore/platform/graphics/chromium/cc/CCDelegatedRendererLayerImpl.cpp:52 > + // FIXME: This could possibly return false even though there are some > + // quads present as they could all be from a single layer (or set of > + // layers without children). If this happens, then make a test that > + // ensures the opacity is being changed on quads in the root RenderPass > + // when this layer doesn't own a RenderSurface. Yeah, I think this is an ok false positive for now. If that FIXME gets addressed, we're probably going to need to rename descendantDrawsContent, or split it up into the three different cases that CCLTHCommon is looking for when implicitly creating a render surface. > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:321 > + if (it->hasContributingDelegatedRenderPasses()) { This block of code seriously needs at least a comment or two. > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:322 > + for (size_t i = 0; i < frame.renderPasses.size(); ++i) { Do you have to walk through every render pass here? > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:325 > + if (renderPass == targetRenderPass) > + continue; This continue is because the delegated layer would have already been asked to append quads for its root pass as a part of whatever target that is drawing into? > Source/WebKit/chromium/tests/CCDelegatedRendererLayerImplTest.cpp:90 > +class DrawingLayerImpl : public CCSolidColorLayerImpl { > +public: > + static PassOwnPtr<DrawingLayerImpl> create(int id) { return adoptPtr(new DrawingLayerImpl(id)); } > +protected: > + explicit DrawingLayerImpl(int id) : CCSolidColorLayerImpl(id) { setDrawsContent(true); } > +}; Where does this get used?

Comment on attachment 162910 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=162910&action=review Awesome! > Source/WebKit/chromium/tests/CCDelegatedRendererLayerImplTest.cpp:190 > + OwnPtr<CCDelegatedRendererLayerImpl> m_delegatedRendererLayer; nit: it could be confusing to have all of these as members even though they're all NULL in tests. They could just be local variables in the constructor, right? > Source/WebKit/chromium/tests/CCLayerTreeHostImplTest.cpp:2580 > + delegatedRendererLayer->setLayerTreeHostImpl(m_hostImpl.get()); As is, this currently doesn't test much. How hard would it be to add non-trivial render passes to the delegatedRendererLayer here? I think CCDelegatedRendererLayerImpl would need to clear its render passes when losing the context (and wait for the next commit to get the new render passes). > Source/WebKit/chromium/tests/CCLayerTreeHostImplTest.cpp:2714 > + delegatedRendererLayer->setLayerTreeHostImpl(m_hostImpl.get()); Same here.

(In reply to comment #14) > (From update of attachment 162910 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=162910&action=review > > In general, looks great. A few minor nits: ty! > > Source/WebCore/platform/graphics/chromium/RenderSurfaceChromium.h:82 > > + void addContributingDelegatedRenderPassLayer(LayerChromium*) { } > > Maybe add a comment about why this can be a no-op? Sure. > > Source/WebCore/platform/graphics/chromium/cc/CCDelegatedRendererLayerImpl.cpp:52 > > + // FIXME: This could possibly return false even though there are some > > + // quads present as they could all be from a single layer (or set of > > + // layers without children). If this happens, then make a test that > > + // ensures the opacity is being changed on quads in the root RenderPass > > + // when this layer doesn't own a RenderSurface. > > Yeah, I think this is an ok false positive for now. If that FIXME gets addressed, we're probably going to need to rename descendantDrawsContent, or split it up into the three different cases that CCLTHCommon is looking for when implicitly creating a render surface. > > > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:321 > > + if (it->hasContributingDelegatedRenderPasses()) { > > This block of code seriously needs at least a comment or two. k. > > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:322 > > + for (size_t i = 0; i < frame.renderPasses.size(); ++i) { > > Do you have to walk through every render pass here? No, we can do better.. It occurs to me that the RenderPass::id structure means we can just iterate by index and check contains() in the hash map to get them all. > > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:325 > > + if (renderPass == targetRenderPass) > > + continue; > > This continue is because the delegated layer would have already been asked to append quads for its root pass as a part of whatever target that is drawing into? Yep. > > Source/WebKit/chromium/tests/CCDelegatedRendererLayerImplTest.cpp:90 > > +class DrawingLayerImpl : public CCSolidColorLayerImpl { > > +public: > > + static PassOwnPtr<DrawingLayerImpl> create(int id) { return adoptPtr(new DrawingLayerImpl(id)); } > > +protected: > > + explicit DrawingLayerImpl(int id) : CCSolidColorLayerImpl(id) { setDrawsContent(true); } > > +}; > > Where does this get used? Right, I made everything use solid color layer to not need this. Removed. (In reply to comment #15) > (From update of attachment 162910 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=162910&action=review > > Awesome! > > > Source/WebKit/chromium/tests/CCDelegatedRendererLayerImplTest.cpp:190 > > + OwnPtr<CCDelegatedRendererLayerImpl> m_delegatedRendererLayer; > > nit: it could be confusing to have all of these as members even though they're all NULL in tests. They could just be local variables in the constructor, right? Yeh true. Will do. > > Source/WebKit/chromium/tests/CCLayerTreeHostImplTest.cpp:2580 > > + delegatedRendererLayer->setLayerTreeHostImpl(m_hostImpl.get()); > > As is, this currently doesn't test much. How hard would it be to add non-trivial render passes to the delegatedRendererLayer here? I think CCDelegatedRendererLayerImpl would need to clear its render passes when losing the context (and wait for the next commit to get the new render passes). Ya absolutely, thanks for pointing this out. > > Source/WebKit/chromium/tests/CCLayerTreeHostImplTest.cpp:2714 > > + delegatedRendererLayer->setLayerTreeHostImpl(m_hostImpl.get()); > > Same here. Same :)

Created attachment 163961 [details] Patch the WebDelegatedRendererLayer header for Platform/. the rest will move to src/cc

Comment on attachment 163961 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=163961&action=review > Source/Platform/chromium/public/WebDelegatedRendererLayer.h:34 > +// This class represents a layer that renders the output of another > +// delegating compositor. Could you expand a bit on how this is used and what it does? Do we even need this to be exposed in WebKit APIs? I.e. would WebKit ever construct one of these, or would it only be created by the browser compositor?

Comment on attachment 163961 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=163961&action=review >> Source/Platform/chromium/public/WebDelegatedRendererLayer.h:34 >> +// delegating compositor. > > Could you expand a bit on how this is used and what it does? > > Do we even need this to be exposed in WebKit APIs? I.e. would WebKit ever construct one of these, or would it only be created by the browser compositor? The layer holds a set of RenderPasses given to it from another compositor (via RenderWidgetHostViewAura). The current implementation of the layer is expecting to receive the passes directly on the impl thread so there's no methods here. If that changes (based on meeting just now it might) then we would add a setRenderPasses() method to this for the main thread to access from RWHVA. In my foggy planning mind it is owned by a RenderWidgetViewHostAura. I figured it needs a Web version to create and own the layer, similar to a WebContentLayer or something in the UI. It would be a child of some other aura UI layer. WebKit will never be constructing them (though I'm not sure where in WebKit we could possibly make one..WebView?).

I meant to put that sort of information in a comment in the header file so people reading the file know what is going on.

(In reply to comment #20) > I meant to put that sort of information in a comment in the header file so people reading the file know what is going on. Oh :)

Created attachment 164005 [details] Patch

Comment on attachment 164005 [details] Patch R=me.

Created attachment 164186 [details] Patch for landing

Comment on attachment 164186 [details] Patch for landing Clearing flags on attachment: 164186 Committed r128630: <http://trac.webkit.org/changeset/128630>

All reviewed patches have been landed. Closing bug.