Bug 103635 – [chromium] Create GraphicsLayerChromiums using a factory

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

Here's the context for this change:

With impl thread painting, we'll have two impl trees, both of which will need to have up-to-date animated values. Things become tricky of both trees have their own copies of the animation controllers. The basic idea is that the impl trees will share a single copy (the layers will have a scoped_refptr to their controller).

To accomplish this, I plan to create an auxiliary list of active animation controllers*. This list will contain raw pointers (no ref) that we guarantee won't go stale. When the layer tree host impl services its animations, it will simply traverse the auxiliary list rather than one if its two trees. This solves another big problem we currently have with accelerated animations: detached layers cannot start accelerated animations**. Once we're creating graphics layers with a factory, we can (in a future CL) attempt to register animation controllers with the layer tree hosts, even before the layers are added to the tree. To be more specific, the way I see these future CLs panning out is as follows:

On the webkit side,
 - The factory knows about the root layer (the ChromeClientImpl will keep the factory's m_rootLayer member updated).
 - GraphicsLayerChromium and Layer will take a (possibly null) pointer to the root layer as a ctor parameter, passing it along as usual.

On the chromium side, 
 - Layer::Layer, when constructing its layer animator will, if things are non-null, pass the root layer's layer tree host to the layer animation controllers constructor.
 - Layer will communicate to its animation controller whenever its layer tree host changes.
 - LayerTreeHost and LayerTreeHostImpl will both inherit from AnimationRegistrar (which only defines ::Register(controller*) and ::Unregister(controller*))
 - Commit will sync the auxiliary lists as required.
 - Layers will pull animated values from their controllers rather than them being pushed like they used to be.

* Another suggestion, rather than the layer tree hosts maintaining auxiliary lists, is that we create a global list of active controllers.

** This is a VERY common case (most transitions). We had initially assumed that detached layers would always become attached shortly, so we said 'yes' when asked to accelerate the animation thinking that we could just start it up shortly when we were added to the layer tree. It turned out that this was not always the case -- sometimes we remained detached until some user interaction happened (hovering over a particular element, say). This causes problems because sometimes webcore blocks main thread animation waiting to hear news of the accelerated animation starting. To get around this correctness issue we decided to reject accelerated animation while detached.

Created attachment 176721 [details]
Patch

[James Robinson]

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?

[James Robinson]

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

[WebKit Review Bot]

Comment on attachment 182912 [details]
Patch for landing

Clearing flags on attachment: 182912

Committed r139829: <http://trac.webkit.org/changeset/139829>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.