WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
67883
Move root layer creation semantics to the outside of chromium compositor
https://bugs.webkit.org/show_bug.cgi?id=67883
Summary
Move root layer creation semantics to the outside of chromium compositor
Shawn Singh
Reported
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.
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
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
Shawn Singh
Comment 1
2011-09-09 22:36:53 PDT
Created
attachment 106963
[details]
Patch
WebKit Review Bot
Comment 2
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.
James Robinson
Comment 3
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.
Shawn Singh
Comment 4
2011-09-10 14:35:14 PDT
Created
attachment 106984
[details]
Patch
Adrienne Walker
Comment 5
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. :)
James Robinson
Comment 6
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()
Antoine Labour
Comment 7
2011-09-13 16:54:01 PDT
Created
attachment 107260
[details]
Patch
Antoine Labour
Comment 8
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.
James Robinson
Comment 9
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)
Antoine Labour
Comment 10
2011-09-14 17:47:33 PDT
Created
attachment 107429
[details]
Patch
Antoine Labour
Comment 11
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.
James Robinson
Comment 12
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.
James Robinson
Comment 13
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.
Antoine Labour
Comment 14
2011-09-14 18:15:35 PDT
Created
attachment 107434
[details]
Patch
Antoine Labour
Comment 15
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).
James Robinson
Comment 16
2011-09-14 18:19:17 PDT
Comment on
attachment 107434
[details]
Patch LGTM
Antoine Labour
Comment 17
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.
James Robinson
Comment 18
2011-09-14 19:33:12 PDT
Committed
r95152
: <
http://trac.webkit.org/changeset/95152
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug