Bug 67883 – Move root layer creation semantics to the outside of chromium compositor

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Shawn Singh]

I will upload a patch right now.   Basically it creates a "RootLayerFactory" that is used by the WebViewImpl class.   This factory removes the need to explicitly instantiate GraphicsLayers for the rootLayer inside the chromium compositor.  This change is part of gradually isolating chromium compositor from the rest of WebKit.

There is one problem;  I am using PassOwnPtr objects where they shouldn't be...  I know this will fail the style guidlines, but I didn't see any examples in the code how to do it properly.  The problem is that I have 2 or 3 levels of indirection to pass the pointer, before it gets to the final owner.  What type of pointer should be used in that case?

If someone can please give me an "easy answer" of how to fix this problem, I'll do it.  Otherwise, I'll probably take far longer to figure it out given my upcoming schedule.

[Shawn Singh]

Created attachment 106963 [details]
Patch

[WebKit Review Bot]

Attachment 106963 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebKit/chromium/src/WebViewImpl.cpp:2659:  Local variables should never be PassOwnPtr (see http://webkit.org/coding/RefPtr.html).  [readability/pass_ptr] [5]
Source/WebKit/chromium/src/WebViewImpl.cpp:2660:  Local variables should never be PassOwnPtr (see http://webkit.org/coding/RefPtr.html).  [readability/pass_ptr] [5]
Source/WebKit/chromium/src/WebViewImpl.cpp:2662:  Local variables should never be PassOwnPtr (see http://webkit.org/coding/RefPtr.html).  [readability/pass_ptr] [5]
Source/WebKit/chromium/src/RootLayerFactory.cpp:40:  Local variables should never be PassOwnPtr (see http://webkit.org/coding/RefPtr.html).  [readability/pass_ptr] [5]
Source/WebKit/chromium/src/RootLayerFactory.cpp:51:  Local variables should never be PassOwnPtr (see http://webkit.org/coding/RefPtr.html).  [readability/pass_ptr] [5]
Total errors found: 5 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[James Robinson]

Keep it in an OwnPtr while storing the object in a local and use .release() in your return statement. In the middle of functions like this, the local variable has ownership so it is appropriate to use OwnPtr.

[Shawn Singh]

Created attachment 106984 [details]
Patch

[Adrienne Walker]

Comment on attachment 106984 [details]
Patch

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

This is a minor nit, because I have an aversion to "you have to set X before class Y works" complexity; eventually somebody will violate that calling convention constraint so it's always better to just make that not be possible.

If the goal is to be able to mock out CCLayerTreeHost without needing a GraphicsLayer, an alternate solution would be to have NonCompositedContentHost::create() create the graphics layers and pass them to the constructor rather than having the constructor do it.  MockNonCompositedContentHost::create() could skip that step and pass null to its base class constructor.  You would still pass a NonCompositedContentHost to the CCLayerTreeHost::create function, so that you could pass a mock instead, but you wouldn't need to create and set all these layers just to construct a CCLayerTreeHost.

Just a suggestion of an alternate approach.  :)

[James Robinson]

Comment on attachment 106984 [details]
Patch

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

This gets part of the way there, but it doesn't quite get there.  In particular I want to see the logic in CCLayerTreeHost::setRootLayer() moved out - we don't need that call at all if the GraphicsLayer manipulations are done elsewhere.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:43
> +PassRefPtr<CCLayerTreeHost> CCLayerTreeHost::create(CCLayerTreeHostClient* client, PassOwnPtr<GraphicsLayer> topLevelRootLayer, PassOwnPtr<NonCompositedContentHost> nonCompositedContentHost, const CCSettings& settings)

we really want to remove knowledge of NonCompositedContentHost completely from CCLayerTreeHost.  This patch removes part of the dependency, but these functions still rely on it:

CCLayerTreeHost::invalidateRootLayerRect()
CCLayerTreeHost::setRootLayer()
CCLayerTreeHost::setViewport()

which are all called from WebViewImpl, and a caller in LayerRendererChromium that I'm fixing separately.

> Source/WebKit/chromium/src/RootLayerFactory.h:47
> +class RootLayerFactory {

This isn't a very useful class - it's just 2 static functions without any state.  It isn't even really a factory, since with the factory pattern you create an object that vends objects.  It looks like you've moved the layer hierarchy management to WebViewImpl so it'd be better to go ahead and put these functions there as well (although I do hate growing that file).

What about making an instantiable class that can create the root and non composited layers and set up the tree?  WebViewImpl would create an instance of this class, and then pass that (and only that) to CCLayerTreeHost::create()

[Antoine Labour]

Created attachment 107260 [details]
Patch

[Antoine Labour]

This patch moves the root layer logic into NonCompositedContentHost. It seemed the most meaningful place to put it (I initially created a RootLayerHelper class, but it was just forwarding all calls to NonCompositedContentHost anyway).

One change this patch contains is that it removes the "root" layer, which doesn't seem used for anything except to add the NonCompositedContentHost's layer as a child. Removing it didn't seem to affect any test, but simplifies the code a bit. I have a version of this patch without this change if needed.

[James Robinson]

Comment on attachment 107260 [details]
Patch

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

This looks really great.  Unfortunately since I had to revert the contents texture manager patch this patch doesn't seem to apply to current ToT.  I have an updated version of that patch up here: https://bugs.webkit.org/show_bug.cgi?id=67440 so hopefully it'll land soon and unblock this one.

Left some comments, not touching review? flag yet until the dependent patch lands.

> Source/WebCore/platform/graphics/chromium/NonCompositedContentHost.cpp:82
> +void NonCompositedContentHost::clearRenderSurfacesRecursive(LayerChromium* layer)

this function feels misplaced - CCLayerTreeHost has a pointer to the root of the LayerChromium tree and should be the only class that has to deal with the implementation details of them

> Source/WebKit/chromium/src/WebViewImpl.cpp:2790
> +        m_nonCompositedContentHost->setVisible(visible);
> +        m_layerTreeHost->setVisible(visible);

this seems like a bit of a code smell - especially since the order of calls here is essential (NonCompositedContentHost::setVisible protects textures and then CCLayerTreeHost::setVisible deletes all unprotected textures beyond a given threshold).

Maybe instead of treating this as an abstract thing (tell the NCCH and LTH about the new visibility) it'd be better to make the nonCompositedContentHost more explicit? something like:

if (!visible)
  m_nonCompositedContentHost->protectVisibleRootTileTextures();
m_layerTreeHost->setVisible(visible)

[Antoine Labour]

Created attachment 107429 [details]
Patch

[Antoine Labour]

(In reply to comment #9)
> (From update of attachment 107260 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=107260&action=review
> 
> This looks really great.  Unfortunately since I had to revert the contents texture manager patch this patch doesn't seem to apply to current ToT.  I have an updated version of that patch up here: https://bugs.webkit.org/show_bug.cgi?id=67440 so hopefully it'll land soon and unblock this one.
> 
> Left some comments, not touching review? flag yet until the dependent patch lands.
> 
> > Source/WebCore/platform/graphics/chromium/NonCompositedContentHost.cpp:82
> > +void NonCompositedContentHost::clearRenderSurfacesRecursive(LayerChromium* layer)
> 
> this function feels misplaced - CCLayerTreeHost has a pointer to the root of the LayerChromium tree and should be the only class that has to deal with the implementation details of them

After discussion, what I implemented clears the renderSurfaces as soon as it can (in commitTo), or on destruction. The good thing is that we have the list of layers that need to be cleared without having to traverse the tree.

> 
> > Source/WebKit/chromium/src/WebViewImpl.cpp:2790
> > +        m_nonCompositedContentHost->setVisible(visible);
> > +        m_layerTreeHost->setVisible(visible);
> 
> this seems like a bit of a code smell - especially since the order of calls here is essential (NonCompositedContentHost::setVisible protects textures and then CCLayerTreeHost::setVisible deletes all unprotected textures beyond a given threshold).
> 
> Maybe instead of treating this as an abstract thing (tell the NCCH and LTH about the new visibility) it'd be better to make the nonCompositedContentHost more explicit? something like:
> 
> if (!visible)
>   m_nonCompositedContentHost->protectVisibleRootTileTextures();
> m_layerTreeHost->setVisible(visible)

Done.

[James Robinson]

I think I've given you the gift of merge conflicts, this patch doesn't seem to apply.  This is probably due to https://bugs.webkit.org/show_bug.cgi?id=68121 which I forgot to CC you on - sorry!

Can you double check this still works after http://trac.webkit.org/changeset/95135 ? I'll still review this iteration.

[James Robinson]

Comment on attachment 107429 [details]
Patch

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

R=me with a few nits.

> Source/WebCore/platform/graphics/chromium/NonCompositedContentHost.h:42
> +class LayerChromium;

I don't think you need this fwd decl

> Source/WebCore/platform/graphics/chromium/NonCompositedContentHost.h:72
> +    WebCore::IntSize m_viewportSize;

Just IntSize, we're in the WebCore namespace here.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:411
> +    for (int surfaceIndex = m_updateList.size() - 1; surfaceIndex >= 0 ; --surfaceIndex) {

nit: I don't think iteration order matters here, so I'd just loop through in the normal order [0, m_updateList.size()). Makes it a bit easier to spot check that this is supposed to hit every entry in the list.

[Antoine Labour]

Created attachment 107434 [details]
Patch

[Antoine Labour]

(In reply to comment #13)
> (From update of attachment 107429 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=107429&action=review
> 
> R=me with a few nits.
> 
> > Source/WebCore/platform/graphics/chromium/NonCompositedContentHost.h:42
> > +class LayerChromium;
> 
> I don't think you need this fwd decl

Done.

> 
> > Source/WebCore/platform/graphics/chromium/NonCompositedContentHost.h:72
> > +    WebCore::IntSize m_viewportSize;
> 
> Just IntSize, we're in the WebCore namespace here.

Done.

> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:411
> > +    for (int surfaceIndex = m_updateList.size() - 1; surfaceIndex >= 0 ; --surfaceIndex) {
> 
> nit: I don't think iteration order matters here, so I'd just loop through in the normal order [0, m_updateList.size()). Makes it a bit easier to spot check that this is supposed to hit every entry in the list.

Done (busted for copy&paste).

[James Robinson]

Comment on attachment 107434 [details]
Patch

LGTM

[Antoine Labour]

(In reply to comment #12)
> I think I've given you the gift of merge conflicts, this patch doesn't seem to apply.  This is probably due to https://bugs.webkit.org/show_bug.cgi?id=68121 which I forgot to CC you on - sorry!

That was trivial merge (in latest patch).

> 
> Can you double check this still works after http://trac.webkit.org/changeset/95135 ? I'll still review this iteration.

I tried merging that, and after solving the conflicts (and removing the setLayerTreeHost(0) call) everything seems to work just fine.

[James Robinson]

Committed r95152: <http://trac.webkit.org/changeset/95152>