[webkit-reviews] review denied: [Bug 67883] Move root layer creation semantics to the outside of chromium compositor : [Attachment 106984] Patch

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has denied Shawn Singh
<shawnsingh at chromium.org>'s request for review:
Bug 67883: Move root layer creation semantics to the outside of chromium
compositor
https://bugs.webkit.org/show_bug.cgi?id=67883

Attachment 106984: Patch
https://bugs.webkit.org/attachment.cgi?id=106984&action=review

------- Additional Comments from James Robinson <jamesr at chromium.org>
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()