[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