Bug 67883 - Move root layer creation semantics to the outside of chromium compositor
Summary: Move root layer creation semantics to the outside of chromium compositor
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 67440
Blocks:
  Show dependency treegraph
 
Reported: 2011-09-09 22:25 PDT by Shawn Singh
Modified: 2011-09-14 19:33 PDT (History)
6 users (show)

See Also:


Attachments
Patch (15.74 KB, patch)
2011-09-09 22:36 PDT, Shawn Singh
no flags Details | Formatted Diff | Diff
Patch (15.77 KB, patch)
2011-09-10 14:35 PDT, Shawn Singh
jamesr: review-
Details | Formatted Diff | Diff
Patch (20.20 KB, patch)
2011-09-13 16:54 PDT, Antoine Labour
no flags Details | Formatted Diff | Diff
Patch (20.97 KB, patch)
2011-09-14 17:47 PDT, Antoine Labour
no flags Details | Formatted Diff | Diff
Patch (20.83 KB, patch)
2011-09-14 18:15 PDT, Antoine Labour
jamesr: review+
jamesr: commit-queue+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Shawn Singh 2011-09-09 22:25:57 PDT
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.
Comment 1 Shawn Singh 2011-09-09 22:36:53 PDT
Created attachment 106963 [details]
Patch
Comment 2 WebKit Review Bot 2011-09-09 22:39:54 PDT
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.
Comment 3 James Robinson 2011-09-09 23:01:47 PDT
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.
Comment 4 Shawn Singh 2011-09-10 14:35:14 PDT
Created attachment 106984 [details]
Patch
Comment 5 Adrienne Walker 2011-09-12 10:19:32 PDT
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.  :)
Comment 6 James Robinson 2011-09-12 10:38:53 PDT
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()
Comment 7 Antoine Labour 2011-09-13 16:54:01 PDT
Created attachment 107260 [details]
Patch
Comment 8 Antoine Labour 2011-09-13 16:59:32 PDT
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.
Comment 9 James Robinson 2011-09-13 18:51:55 PDT
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)
Comment 10 Antoine Labour 2011-09-14 17:47:33 PDT
Created attachment 107429 [details]
Patch
Comment 11 Antoine Labour 2011-09-14 17:50:59 PDT
(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.
Comment 12 James Robinson 2011-09-14 17:52:44 PDT
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.
Comment 13 James Robinson 2011-09-14 18:00:14 PDT
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.
Comment 14 Antoine Labour 2011-09-14 18:15:35 PDT
Created attachment 107434 [details]
Patch
Comment 15 Antoine Labour 2011-09-14 18:16:28 PDT
(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).
Comment 16 James Robinson 2011-09-14 18:19:17 PDT
Comment on attachment 107434 [details]
Patch

LGTM
Comment 17 Antoine Labour 2011-09-14 18:35:44 PDT
(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.
Comment 18 James Robinson 2011-09-14 19:33:12 PDT
Committed r95152: <http://trac.webkit.org/changeset/95152>