[webkit-reviews] review requested: [Bug 103635] [chromium] Create GraphicsLayerChromiums using a factory : [Attachment 181304] Patch

[bugzilla-daemon at webkit.org]

vollick at chromium.org has asked	for review:
Bug 103635: [chromium] Create GraphicsLayerChromiums using a factory
https://bugs.webkit.org/show_bug.cgi?id=103635

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

------- Additional Comments from vollick at chromium.org
(In reply to comment #2)
> (From update of attachment 176721 [details])
> View in context:
https://bugs.webkit.org/attachment.cgi?id=176721&action=review
> 
> With a few tweaks this looks fine.  I don't understand what this patch has to
do with the rest of the changes.  Is it just a prerequisite to getting a
pointer to a tree at layer creation time to bootstrap some of the animation
startup?  What happens when a layer moves trees?  I'm pretty sure we can keep a
layer always associated with some tree but not the same one for the layer's
lifetime.

Yes, this is to give a layer access to the tree so that the layer can get its
animation controller registered. This will allow us to operate on the
controllers even when the layer hasn't yet been added to the tree. When the
layer moves trees, we update its registration, so that shouldn't be a problem.

> 
> > Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:88
> >	 if (!factory)
> > -	     return adoptPtr(new GraphicsLayerChromium(client));
> > +	     ASSERT_NOT_REACHED();
> >  
> >	 return factory->createGraphicsLayer(client);
> 
> Would probably be better as
>     ASSERT(factory);
>     return factory->...
> 
> or even leave the ASSERT() out since we'll crash anyway
Done.
> 
> > Source/WebKit/chromium/src/ChromeClientImpl.h:50
> > +class GraphicsLayerClient;
> 
> you don't need this
Removed.
> 
> > Source/WebKit/chromium/src/ChromeClientImpl.h:183
> > +	 // Allows us to create our graphics layers however we wish, with
whatever
> > +	 // parameters we like.
> 
> this comment isn't  very helpful.
Removed.
> 
> could you keep the overrides in the same order as in ChromeClient.h?
graphicsLayerFactory() is above attachRootGraphicsLayer in the base interface
definition
> 
> > Source/WebKit/chromium/src/ChromeClientImpl.h:255
> > +#if USE(ACCELERATED_COMPOSITING)
> 
> USE(ACCELERATD_COMPOSITING) is always true for the chromium port so this
guard isn't very useful
Should we remove these guards elsewhere in the file?
> 
> > Source/WebKit/chromium/src/NonCompositedContentHost.cpp:51
> > +	 if (WebCore::Page* page = m_webView->page())
> > +	     graphicsLayerFactory =
page->chrome()->client()->graphicsLayerFactory();
> 
> ChromeClientImpl is a member of WebViewImpl (and can't be null) so this
pointer walk isn't needed.  Why not pass a GraphicsLayerFactory pointer to this
function along with the WebViewImpl* ?
Done.
> 
> > Source/WebKit/chromium/src/PageOverlay.cpp:133
> > +	     GraphicsLayerFactory* graphicsLayerFactory = 0;
> > +	     if (Page* page = m_viewImpl->page())
> > +		 graphicsLayerFactory =
page->chrome()->client()->graphicsLayerFactory();
> 
> same thing here - the GraphicsLayerFactory is owned by
WebViewImpl::m_chromeClientImpl, so you should figure out a way to access it
directly and make sure it isn't null here
Done -- I've made a graphicsLayerFactory accessor on WebViewImpl.