Bug 67750 – Create a delegate class to help cleanly isolate the chromium compositor API

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Shawn Singh]

First-attempt patch will be uploaded in a moment.

Essentially this delegate hides the layer stuff in WebCore from the chromium compositor.  For now, it only has functions that the compositor was actually using.  (had to rename them to avoid name conflicts between GraphicsLayer and CCLayerDelegate).   Additionally this delegate will make stubbing/mocking easier for unit testing the compositor.

In theory, there are no changes to the actual semantics of the code.  All tests passed on OS X.  Are there other tests we should include?

[Shawn Singh]

Created attachment 106676 [details]
Patch

[Nat Duca]

Comment on attachment 106676 [details]
Patch

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

I think there are references to GraphicsLayer on CCLayerTreeHost as well for the root layer.

Did you add CCLayerDelegate.h? I dont see the actual class...

I don think you need to call the virtual methods on those doPaintGraphicsLayerContents... paintContents should work just fine. GraphicsLayer is supposed to go away from the Layer tree completely.

git grep for graphicslayer --- it should be gone from all compositor related files when this patch lands.

> Source/WebCore/ChangeLog:3
> +        Added a pure virtual class CCLayerDelegate which helps to isolate

I think the traditional format of changelog entries is a short sumary, the bug# ,then a detailed summary if need. i might be wrong...

> Source/WebCore/WebCore.gypi:3537
> +            'platform/graphics/chromium/cc/CCLayerDelegate.h',

My preference would be for this to be in LayerChromium.h file, which is sort of how we've doing client classes as well, c.f. CCLayerTreeHost and CCLayerTreeHostClient, which are the same file.

> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:704
> +void GraphicsLayerChromium::doPaintGraphicsLayerContents(GraphicsContext& context, const IntRect& clip)

Is there a way to do this without to doPaint?

[Shawn Singh]

(In reply to comment #2)

After clarifying with Nat offline, here's what I understand should be fixed:

- make sure other instances of GraphicsLayer / GraphicsLayerChromium are also replaced

- move the CCLayerDelegate class into the LayerChromium file

- name the functions more simply and without the words "GraphicsLayer"

Will upload a new patch soon.


> 
> I don think you need to call the virtual methods on those doPaintGraphicsLayerContents... paintContents should work just fine. GraphicsLayer is supposed to go away from the Layer tree completely.
> 
> git grep for graphicslayer --- it should be gone from all compositor related files when this patch lands.
> 
> > Source/WebCore/ChangeLog:3
> > +        Added a pure virtual class CCLayerDelegate which helps to isolate
> 
> I think the traditional format of changelog entries is a short sumary, the bug# ,then a detailed summary if need. i might be wrong...
> 
> > Source/WebCore/WebCore.gypi:3537
> > +            'platform/graphics/chromium/cc/CCLayerDelegate.h',
> 
> My preference would be for this to be in LayerChromium.h file, which is sort of how we've doing client classes as well, c.f. CCLayerTreeHost and CCLayerTreeHostClient, which are the same file.
> 
> > Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:704
> > +void GraphicsLayerChromium::doPaintGraphicsLayerContents(GraphicsContext& context, const IntRect& clip)
> 
> Is there a way to do this without to doPaint?

[WebKit Review Bot]

Comment on attachment 106676 [details]
Patch

Attachment 106676 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9623323

[Antoine Labour]

Created attachment 107132 [details]
Patch

[Antoine Labour]

I think this addresses the review comments, and also removes the use of GraphicsLayerClient from the VideoLayerChromium.

Along with a follow up patch to https://bugs.webkit.org/show_bug.cgi?id=67883 the LayerChromium/CCLayer is free of GraphicsLayer.

[James Robinson]

Comment on attachment 107132 [details]
Patch

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

This change looks really good - R-'ing for some naming nitpicks and because of some unsafe code in GraphicsLayerChromium (the old code was safe for the specific case of video layers, but still skeevy).

I also think that having the LayerChromium back pointer be called 'm_owner' doesn't really make much sense with the new type.  We use m_delegate elsewhere in WebKit.  It might also make sense to call it CCLayerClient and have the pointer be m_client but I leave it to up to you.

> Source/WebCore/ChangeLog:12
> +        No new tests. (OOPS!)

There's an SVN presubmit hook that rejects checkins with this line.  You should remove this line and instead say that these changes don't change behavior and the code is covered by tests in compositing/

> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:704
> +bool GraphicsLayerChromium::paintingGoesToWindow() const
> +{
> +    RenderLayerBacking* backing = static_cast<RenderLayerBacking*>(client());
> +    return !backing || backing->paintingGoesToWindow();
> +}

this downcast isn't actually safe, we do create GraphicsLayerChromium instances with clients that are not RenderLayerBackings - for instance, for overflow controls - and we may do more of this in the future. I'm pretty sure you can kill this completely - see the comments for VideoLayerChromium.

> Source/WebCore/platform/graphics/chromium/LayerChromium.h:62
> +    virtual bool getDrawsContent() const = 0;
> +    virtual bool getPreserves3D() const = 0;

in WebKit style, simple getters do not have the 'get' prefix - so these should be drawsContent() and preserves3D()

> Source/WebCore/platform/graphics/chromium/LayerChromium.h:63
> +    virtual bool paintingGoesToWindow() const = 0;

I think you can (and should) remove this

> Source/WebCore/platform/graphics/chromium/VideoLayerChromium.cpp:82
> +    if (!m_contentsDirty || !m_owner || m_owner->paintingGoesToWindow())

the paintingGoesToWindow() check is old and I'm pretty sure it was never necessary. can you verify our tests pass without this and remove it?

[Antoine Labour]

Created attachment 107198 [details]
Patch

[Antoine Labour]

(In reply to comment #7)
> (From update of attachment 107132 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=107132&action=review
> 
> This change looks really good - R-'ing for some naming nitpicks and because of some unsafe code in GraphicsLayerChromium (the old code was safe for the specific case of video layers, but still skeevy).
> 
> I also think that having the LayerChromium back pointer be called 'm_owner' doesn't really make much sense with the new type.  We use m_delegate elsewhere in WebKit.  It might also make sense to call it CCLayerClient and have the pointer be m_client but I leave it to up to you.

Done (m_delegate).

> 
> > Source/WebCore/ChangeLog:12
> > +        No new tests. (OOPS!)
> 
> There's an SVN presubmit hook that rejects checkins with this line.  You should remove this line and instead say that these changes don't change behavior and the code is covered by tests in compositing/

Done.

> 
> > Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:704
> > +bool GraphicsLayerChromium::paintingGoesToWindow() const
> > +{
> > +    RenderLayerBacking* backing = static_cast<RenderLayerBacking*>(client());
> > +    return !backing || backing->paintingGoesToWindow();
> > +}
> 
> this downcast isn't actually safe, we do create GraphicsLayerChromium instances with clients that are not RenderLayerBackings - for instance, for overflow controls - and we may do more of this in the future. I'm pretty sure you can kill this completely - see the comments for VideoLayerChromium.

Done. I removed it and the compositing layout tests still pass.

> 
> > Source/WebCore/platform/graphics/chromium/LayerChromium.h:62
> > +    virtual bool getDrawsContent() const = 0;
> > +    virtual bool getPreserves3D() const = 0;
> 
> in WebKit style, simple getters do not have the 'get' prefix - so these should be drawsContent() and preserves3D()

Done. It may get a little confusing because in GraphicsLayerChromium, the CCLayerDelegate overrides hide the GraphicsLayers's versions, but semantically it's consistent.

[James Robinson]

Comment on attachment 107198 [details]
Patch

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

R=me

> Source/WebCore/platform/graphics/chromium/ContentLayerChromium.h:48
> +    static PassRefPtr<ContentLayerChromium> create(CCLayerDelegate* = 0);

nit: we're inconsistent in the various *LayerChromium::create() calls about whether there's a default param. I don't think a null delegate makes sense for this layer type so probably better to remove it.

> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:56
> +#include "RenderLayerBacking.h"

i don't think you need this #include any more, and it's actually a layering violation (things in WebCore/platform should not depend on anything in WebCore/ outside of WebCore/platform)

> Source/WebCore/platform/graphics/chromium/ImageLayerChromium.h:52
> +    static PassRefPtr<ImageLayerChromium> create(CCLayerDelegate* = 0);

nit: no default 0

> Source/WebCore/platform/graphics/chromium/ImageLayerChromium.h:61
> +    ImageLayerChromium(CCLayerDelegate*);

nit: explicit

[Antoine Labour]

Created attachment 107377 [details]
Patch

[James Robinson]

Comment on attachment 107377 [details]
Patch

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

Looks good.  Left one comment FYI but this is fine to land as-is

> Source/WebCore/platform/graphics/chromium/LayerChromium.h:60
> +    virtual ~CCLayerDelegate() { }

another thing: we normally put the d'tor in the protected section, since it rarely makes sense to delete a delegate through a CCLayerDelegate*

[Antoine Labour]

I'll fix in a follow-up.

[James Robinson]

Comment on attachment 107377 [details]
Patch

Clearing flags on attachment: 107377

Committed r95148: <http://trac.webkit.org/changeset/95148>

[James Robinson]

All reviewed patches have been landed.  Closing bug.

[Shawn Singh]

Now that we have this CCLayerDelegate... 

The CCLayerDelegate has a drawsContent() interface which the user should provide.

The base class "LayerChromium" default implementation returns false.

should it return  "(m_delegate) ? m_delegate->drawsContent() : false"   instead?

I can imagine that users may want to explicitly create a new type of layer, so they use the delegate to implement their own paintContents function... and set drawsContent() to true thinking that it will be called appropriately.   But as the code is now, it wouldn't be called.

Thoughts?

[Nat Duca]

Sounds like another bug to me... or a conversation you might have with piman or jamesr in person. It'd be even cooler if we could get rid of this drawscontent crap entirely.

[James Robinson]

We're definitely doing something wrong here, I'm just not sure yet exactly what it is.

[Antoine Labour]

Well, today several layers share the same delegate, e.g. the transform layer and the content layer of a given GraphicsLayerChromium. Problem is, you don't want to assume the transform layer (which is a LayerChromium) draws anything.
I've been trying to factor out the delegate from LayerChromium altogether, and move it into a CCContentLayerDelegate, only applicable to ContentLayerChromium, but it's more complicated than it appears, because GLC doesn't know anything about the types of layers (hence no safe down casting), yet wants to set itself as the delegate.

But in general, I agree with the premise that drawsContent should have nothing to do with LayerChromium in general.

[James Robinson]

(In reply to comment #19)
> Well, today several layers share the same delegate, e.g. the transform layer and the content layer of a given GraphicsLayerChromium. Problem is, you don't want to assume the transform layer (which is a LayerChromium) draws anything.
> I've been trying to factor out the delegate from LayerChromium altogether, and move it into a CCContentLayerDelegate, only applicable to ContentLayerChromium, but it's more complicated than it appears, because GLC doesn't know anything about the types of layers (hence no safe down casting), yet wants to set itself as the delegate.

GraphicsLayerChromium does know about ContentLayerChromium, and provides itself as the delegate here:
http://trac.webkit.org/browser/trunk/Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp#L77

I like the idea of having a separate delegate type for providing content than for general CCLayer delegation.  notifySyncRequired() applies to all layer types, and once we hook up animation stuff we'll need a set of callbacks on the layer delegate for that, but paintContents() is definitely something that is only relevant for content layers.  This could be a subclass of CCLayerDelegate or an orthogonal type that's passed in to ContentLayerChromium::create().  Image/Video/WebGL/etc layers wouldn't need this.



> 
> But in general, I agree with the premise that drawsContent should have nothing to do with LayerChromium in general.

At the GraphicsLayer interface drawsContent only applies to content layers.  In our implementation we use it in a bunch of other cases, though, that are internal to the compositor implementation - for example if you try to make an mask layer that's crazy sized, when we internally decide that drawsContent() is false for that layer.  That shouldn't be part of the external interface but it is something that it handy for our internals.

Maybe it's time to formalize the split between stuff that is on LayerChromium that is public interface and the stuff that is implementation detail we're using in the compositor.  We use friend declarations / protected for some things, like createCCLayerImpl(), but we aren't nearly as careful as we should be.  LayerChromium::pushPropertiesTo() and LayerChromium::id() for instance are 100% internal and shouldn't be in the public header at all.

[Antoine Labour]

(In reply to comment #20)
> (In reply to comment #19)
> > Well, today several layers share the same delegate, e.g. the transform layer and the content layer of a given GraphicsLayerChromium. Problem is, you don't want to assume the transform layer (which is a LayerChromium) draws anything.
> > I've been trying to factor out the delegate from LayerChromium altogether, and move it into a CCContentLayerDelegate, only applicable to ContentLayerChromium, but it's more complicated than it appears, because GLC doesn't know anything about the types of layers (hence no safe down casting), yet wants to set itself as the delegate.
> 
> GraphicsLayerChromium does know about ContentLayerChromium, and provides itself as the delegate here:
> http://trac.webkit.org/browser/trunk/Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp#L77

The problem is the other cases like seContentsToCanvas or setContentsToMedia, where the GLC sets itself as the delegate of a possibly non-CLC layer.

> 
> I like the idea of having a separate delegate type for providing content than for general CCLayer delegation.  notifySyncRequired() applies to all layer types, and once we hook up animation stuff we'll need a set of callbacks on the layer delegate for that, but paintContents() is definitely something that is only relevant for content layers.  This could be a subclass of CCLayerDelegate or an orthogonal type that's passed in to ContentLayerChromium::create().  Image/Video/WebGL/etc layers wouldn't need this.
> 
> 
> 
> > 
> > But in general, I agree with the premise that drawsContent should have nothing to do with LayerChromium in general.
> 
> At the GraphicsLayer interface drawsContent only applies to content layers.  In our implementation we use it in a bunch of other cases, though, that are internal to the compositor implementation - for example if you try to make an mask layer that's crazy sized, when we internally decide that drawsContent() is false for that layer.  That shouldn't be part of the external interface but it is something that it handy for our internals.

I think in general drawsContent is overloaded for various uses, both internal and external, which is what is causing the complexity to get it out...

> 
> Maybe it's time to formalize the split between stuff that is on LayerChromium that is public interface and the stuff that is implementation detail we're using in the compositor.  We use friend declarations / protected for some things, like createCCLayerImpl(), but we aren't nearly as careful as we should be.  LayerChromium::pushPropertiesTo() and LayerChromium::id() for instance are 100% internal and shouldn't be in the public header at all.

We could add a pure-virtual base class that would define the public API, and only have that visible from GLC. That'd keep us honest.