Bug 94145 – [chromium] Add the ubercomp WebDelegatedRendererLayer

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dana Jansens]

[chromium] Add boilderplate for ubercomp DelegatingRendererLayer

[Dana Jansens]

Created attachment 158633 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Peter Beverloo (cr-android ews)]

Comment on attachment 158633 [details]
Patch

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

[Dana Jansens]

Created attachment 158635 [details]
Patch

[Dana Jansens]

Created attachment 160489 [details]
Patch

[Adrienne Walker]

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.

[Dana Jansens]

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

[Dana Jansens]

Created attachment 161781 [details]
Patch

[Dana Jansens]

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..

[Dana Jansens]

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

[Dana Jansens]

Created attachment 162908 [details]
Patch

[Dana Jansens]

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

[Dana Jansens]

Created attachment 162910 [details]
Patch

fixing a typo in assert

[Adrienne Walker]

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?

[Antoine Labour]

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.

[Dana Jansens]

(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 :)

[Dana Jansens]

Created attachment 163961 [details]
Patch

the WebDelegatedRendererLayer header for Platform/. the rest will move to src/cc

[James Robinson]

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?

[Dana Jansens]

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?).

[James Robinson]

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.

[Dana Jansens]

(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 :)

[Dana Jansens]

Created attachment 164005 [details]
Patch

[Adrienne Walker]

Comment on attachment 164005 [details]
Patch

R=me.

[Dana Jansens]

Created attachment 164186 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 164186 [details]
Patch for landing

Clearing flags on attachment: 164186

Committed r128630: <http://trac.webkit.org/changeset/128630>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.