Summary: | [chromium] Create GraphicsLayerChromiums using a factory | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | vollick | ||||||||
Component: | WebKit Misc. | Assignee: | vollick | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | cc-bugs, enne, jamesr, nduca, webkit.review.bot | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
vollick
2012-11-29 06:57:02 PST
Created attachment 176721 [details]
Patch
Comment on attachment 176721 [details] Patch 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 Created attachment 181304 [details] Patch (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. (In reply to comment #3) > Created an attachment (id=181304) [details] > Patch > > (In reply to comment #2) > > (From update of attachment 176721 [details] [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. The changes I made in the latest patch seemed non-trivial enough to warrant another r?. Could you please take another look when you have a chance, James? Comment on attachment 181304 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=181304&action=review Still r=me but still needs some cleanup > Source/WebKit/chromium/src/NonCompositedContentHost.cpp:36 > +#include "Chrome.h" > #include "FloatPoint.h" > #include "FloatRect.h" > #include "GraphicsLayer.h" > #include "GraphicsLayerChromium.h" > +#include "GraphicsLayerFactory.h" > +#include "Page.h" please trim includes - i don't think you need any of these new includes, just a forward declaration of WebCore::GraphicsLayerFactory somewhere > Source/WebKit/chromium/src/NonCompositedContentHost.h:68 > + explicit NonCompositedContentHost(WebViewImpl*, WebCore::GraphicsLayerFactory*); no explicit for 2-argument constructors > Source/WebKit/chromium/src/PageOverlay.cpp:32 > +#include "Chrome.h" trim unnecessary include > Source/WebKit/chromium/src/WebViewImpl.cpp:4035 > + m_nonCompositedContentHost = NonCompositedContentHost::create(this, m_chromeClientImpl.graphicsLayerFactory()); since you've added the WebViewImpl::graphicsLayerFactory() getter, i'd use that here as well Created attachment 182912 [details]
Patch for landing
Comment on attachment 182912 [details] Patch for landing Clearing flags on attachment: 182912 Committed r139829: <http://trac.webkit.org/changeset/139829> All reviewed patches have been landed. Closing bug. |