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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sun Dec 2 20:34:41 PST 2012


James Robinson <jamesr at chromium.org> has granted vollick at chromium.org's request
for review:
Bug 103635: [chromium] Create GraphicsLayerChromiums using a factory
https://bugs.webkit.org/show_bug.cgi?id=103635

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

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

> 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

> Source/WebKit/chromium/src/ChromeClientImpl.h:50
> +class GraphicsLayerClient;

you don't need this

> 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.

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

> 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* ?

> 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


More information about the webkit-reviews mailing list