Bug 66807 – [chromium] Introduce CCSingleThreadProxy in order to move LayerRenderer to CCLayerTreeHostImpl

• Component: New Bugs
• Status: RESOLVED FIXED

[Nat Duca]

[chromium] Introduce CCLayerTreeHostImplMainThreadProxy in order to move LayerRenderer to CCLayerTreeHostImpl

[Nat Duca]

Created attachment 104906 [details]
Patch

[Nat Duca]

Just FYI for now.

[James Robinson]

Comment on attachment 104906 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=104906&action=review

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.h:78
> +    // CCLayerTreeHostImplProxyClient implemenation

implementation

[Nat Duca]

Created attachment 104965 [details]
Splitting out committing.

[WebKit Review Bot]

Attachment 104965 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImplCCThreadProxy.cpp:86:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImplCCThreadProxy.cpp:257:  Missing space before ( in if(  [whitespace/parens] [5]
Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImplMainThreadProxy.h:44:  The parameter name "rect" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImplMainThreadProxy.h:53:  The parameter name "visible" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImplProxy.h:85:  The parameter name "visible" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImplMainThreadProxy.cpp:213:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:75:  Missing space before ( in if(  [whitespace/parens] [5]
Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.h:52:  The parameter name "settings" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.h:65:  The parameter name "context" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.h:72:  The parameter name "rect" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.h:75:  The parameter name "layer" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.h:80:  The parameter name "visible" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.h:91:  The parameter name "settings" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommitter.cpp:31:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImplCCThreadProxy.h:45:  The parameter name "rect" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImplCCThreadProxy.h:54:  The parameter name "visible" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 16 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Comment on attachment 104965 [details]
Splitting out committing.

Attachment 104965 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9481749

[Adrienne Walker]

Comment on attachment 104965 [details]
Splitting out committing.

View in context: https://bugs.webkit.org/attachment.cgi?id=104965&action=review

I really like hiding the differences between the single threaded and multithreaded versions of the compositor with a runtime proxy class.  I think that'll make maintaining both configurations a lot easier.  :)

> Source/WebCore/ChangeLog:12
> +        (WebCore::LayerRendererCapabilities::LayerRendererCapabilities):

I said this in a chat conversation, but I think the capabilities could be its own patch.  It'd help make this patch not be so huge.

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:-562
> -    // Recheck that we still have a root layer. This may become null if
> -    // compositing gets turned off during a paint operation.
> -    if (!rootLayer()) {
> -        m_rootCCLayerImpl.clear();
> -        return;
> -    }
> -

Why is this check going away?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:61
> +    if (false) // TODO: make thread a runtime flag in CCSettings

Just file another bug for this, rather than leaving a TODO.  :)

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:-182
> -        layer->platformLayer()->setLayerRenderer(m_layerRenderer.get());

I had mentioned offline that this might not be safe to remove, but I think there's another setLayerRenderer call that happens in paintLayerContents.  So, I think this particular call is redundant and safe to remove.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:85
> +    if (m_layerRenderer->owner()->rootLayer()) {
> +        m_layerRenderer->updateLayers();
> +        m_layerRenderer->drawLayers();
>      }

It seems like this updateLayers call should go elsewhere.  In the BlahBlahMainThreadProxy::compositeIfNeeded case, it looks like updateLayers would get called twice.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:189
> +    // GraphicsContext3D::create might fail and return 0, in that case fall back to software.
> +    RefPtr<GraphicsContext3D> context = m_client->createLayerTreeHostContext3D();
> +    if (!context)
> +        return 0;
> +

There's a lot of plumbing to get the GraphicsContext3D from the WebView across to the CCLayerTreeHostImpl.  This is especially the case where we'll fail to be able to do anything if we don't have a GC3D.  It just seems like it'd be easier to pass this through up front, unless I'm missing a reason for making GC3D a constructor argument here.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImplCCThreadProxy.cpp:58
> +        ccThread = m_client->createCompositorThread().leakPtr();

Same thing here.  Why so much plumbing here to create the compositor thread in the WebViewImpl? It looks like WebViewImpl just calls the static CCThreadImpl::create function.  I just don't see the value of adding this function to a number of client interfaces.

[Nat Duca]

Created attachment 105123 [details]
Complete enough for initial review. Dont try running it.

[Nat Duca]

(In reply to comment #7)
> I said this in a chat conversation, but I think the capabilities could be its own patch.
I agree, for what its worth. If it proves that we're having a hard time getting our heads around this patch, I'll do my best to split it up. However, if we have a chance of getting a quality review without breaking it up, I'd definitely love the reduction of work it'd entail.

> > -    // Recheck that we still have a root layer. This may become null if
> Why is this check going away?
It moved to the CCLayerTreeHostCommitter. Its still there. :)

> > +    if (false) // TODO: make thread a runtime flag in CCSettings
https://bugs.webkit.org/show_bug.cgi?id=66922

> > -        layer->platformLayer()->setLayerRenderer(m_layerRenderer.get());
> So, I think this particular call is redundant and safe to remove.
I added a TODO to the current code to remove this before landing. My next few patch revs will leave it in place just because there are so many other bugs, I can't cleanly verify whether we're certain that matters yet. ~crying~

> > +        ccThread = m_client->createCompositorThread().leakPtr();
> > +    RefPtr<GraphicsContext3D> context = m_client->createLayerTreeHostContext3D();
> There's a lot of plumbing to get the...

Yeah, there is a Ton of plumbing to get these.

One problem is that constructing a GraphicsContext requires the HostWindow, which is the WebView, which is WebKit side. Compositor is WebCore-side, so I don't have access to it. Also complicating things is the (old) need for the WebView to create the compositor context before the CCLayerTreeHost in order to set parent contexts appropriately. The CCThread is similar --- a WebThread is WebKit-only, so we can't create it on the compositor side. 

There are two cleanups we can do to make this less gross:
1. we could change the compositor so you instantiate it with a HostWindow. And, remove all the need for the temporaryGraphicsContext in the webview. I dont think its a lot of work, its just work.
2. We make a WebCore::MessageLoop, which has a platform/chromium implementation in terms of WebThread. CCThread would then be implemented as a WebCore message loop. @levin and I have talked about this but I think that process is best decoupled from "get threaded compositor done" pressures. :)

One other problem leading to excessive plumbing is that we have to create GraphicsContexts using main thread pointers(HostWindow, namely) but try to pevent main-thread concepts from ever leaking into the impl side. This prevents us from just creating a graphicsContexts in the LayerRenderer, for instance. So, though thats where they end up, to prevent the pointers needed to construct them from accidentally moving into the impl-side, we add ... um... pipes.

Eewies. I think CCThread plumbing is unavoidable. We could fix the graphics  context plumbing a bit. The LTH->LTHP->LTHI->LRC plumbing of the contexts feels unavoidable, but maybe there's a better way???

[Nat Duca]

Created attachment 105197 [details]
All features work as advertised.

[James Robinson]

Comment on attachment 105197 [details]
All features work as advertised.

View in context: https://bugs.webkit.org/attachment.cgi?id=105197&action=review

One pass of comments for ya.  I don't think I understand settings/capabilities yet, so I'll try to understand that next.

> Source/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:125
> +#if 0 // AcceleratedPainting is broken by this patch. Will fix in a followon patch.

can you open a bug?

> Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:103
> +    const Vector<RefPtr<LayerChromium> >& c = children();
> +    for (size_t i = 0; i < c.size(); ++i)
> +        c[i]->setLayerRendererRecursive(renderer);

nit: we try not to use one-char variables names except for loop counters (i, j, k, ...). maybe just access children() throughout this?

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.h:194
> +    // FIXME: Remove this
>      CCLayerTreeHost* m_owner;

by "this" you mean the m_owner pointer, right? can haz bug?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:62
> +    if (false) // TODO: make thread a runtime flag in CCSettings

TODO->FIXME. can we just make the USE(THREADED_COMPOSITING) path always make a CCThreadProxy for now, or something like that? seems odd to have compile-time and runtime flags for the same thing

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:71
> +    // Create the renderer...

comment not so useful

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:77
> +    if (m_settings.acceleratePainting && !m_proxy->layerRendererCapabilities().usingAcceleratedPainting)
> +        m_settings.acceleratePainting = false;

i'm a bit surprised this isn't m_settings.acceleratePainting = m_proxy->layerRendererCapabilities().usingAcceleratedPainting

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:116
> +        // TODO: this is rumored to be redundant. If we can get things working, remove it.

can we figure this out before landing?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:117
> +    if (m_layerRenderer)
> +        m_layerRenderer->releaseTextures();

do we always want to releaseTextures(), or just when going !visible?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:133
> +        // If creation failed, and we had asked for accelerated painting, disable accelerated painting
> +        // and try creating the renderer again.
> +        if (!m_layerRenderer && m_settings.acceleratePainting) {
> +            m_settings.acceleratePainting = false;
> +            m_layerRenderer = LayerRendererChromium::create(implHack, this, context);

since we aren't supporting acceleratedPainting yet can we take this out for now?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:145
> +        // If creation failed, and we had asked for accelerated painting, disable accelerated painting
> +        // and try creating the renderer again.
> +        if (!layerRenderer && m_settings.acceleratePainting) {
> +            m_settings.acceleratePainting = false;
> +            layerRenderer = LayerRendererChromium::create(m_layerRenderer->owner(), this, context);
> +        }

same here

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:148
> +        if (layerRenderer)
> +            m_layerRenderer->rootLayer()->platformLayer()->setLayerRendererRecursive(layerRenderer.get());

should this go outside the else{} block (do we need to set this on initial creation) ?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:151
> +    return !!m_layerRenderer;

return m_layerRenderer; is fine, we have a conversion operator to bool on smart pointer types for a reason

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:154
> +// FIXME: move all this code to LTH

LTH->CCLayerTreeHost. I know you'll probably be the one addressing this FIXME but no need to leave mysteries in case someone else picks this up.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImplCCThreadProxy.cpp:114
> +    // Make a blocking call to initializeLayerRendererOnCCThread. The results of that call
> +    // are pushed into the ok and caps local varaibles.
> +    CCCompletionEvent completion;
> +    bool ok;
> +    LayerRendererCapabilities capabilities;

caps -> capabilities
varaibles -> variables

I have no idea what "ok" means, can you use a more descriptive variable name for this?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImplMainThreadProxy.cpp:158
> +    // We don't optimize this case in the main threaded case.

not sure i grok this, commits aren't free in the main thread case (although they aren't hugely expensive either)

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImplMainThreadProxy.cpp:231
> +        bool ok = !m_layerTreeHostImpl->initializeLayerRenderer(0, context);
> +        m_layerTreeHost->didRecreateGraphicsContext(ok);

what does 'ok' mean? something about context creation failing?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImplMainThreadProxy.h:68
> +    bool doComposite(bool presentResult);

nit: rather than having a bool param why not just have this never present and the one callsite that really does want to present can call present() itself

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImplProxy.cpp:62
> +void CCLayerTreeHostImplProxy::setImplThread(WTF::ThreadIdentifier value)
>  {
> -    ASSERT(isImplThread());
> -    TRACE_EVENT("CCLayerTreeHostImplProxy::commitOnCCThread", this, 0);
> -    m_layerTreeHostImpl->beginCommit();
> -    {
> -        TRACE_EVENT("CCLayerTreeHost::commit", this, 0);
> -        committer->commit(m_layerTreeHost, m_layerTreeHostImpl.get());
> -    }
> -    completion->signal();
> -
> -    m_layerTreeHostImpl->commitComplete();
> -}
> -
> -void CCLayerTreeHostImplProxy::drawLayersOnCCThread()
> -{
> -    TRACE_EVENT("CCLayerTreeHostImplProxy::drawLayersOnCCThread", this, 0);
> -    ASSERT(isImplThread());
> -    if (m_layerTreeHostImpl)
> -        m_layerTreeHostImpl->drawLayers();
> -}
> -
> -void CCLayerTreeHostImplProxy::setNeedsCommitAndRedrawOnCCThread()
> -{
> -    TRACE_EVENT("CCLayerTreeHostImplProxy::setNeedsCommitAndRedrawOnCCThread", this, 0);
> -    ASSERT(isImplThread() && m_layerTreeHostImpl);
> -    m_layerTreeHostImpl->setNeedsCommitAndRedraw();
> -}
> -
> -void CCLayerTreeHostImplProxy::setNeedsRedrawOnCCThread()
> -{
> -    TRACE_EVENT("CCLayerTreeHostImplProxy::setNeedsRedrawOnCCThread", this, 0);
> -    ASSERT(isImplThread() && m_layerTreeHostImpl);
> -    m_layerTreeHostImpl->setNeedsRedraw();
> -}
> -
> -void CCLayerTreeHostImplProxy::initImplOnCCThread(CCCompletionEvent* completion)
> -{
> -    TRACE_EVENT("CCLayerTreeHostImplProxy::initImplOnCCThread", this, 0);
> -    ASSERT(isImplThread());
> -    m_layerTreeHostImpl = m_layerTreeHost->createLayerTreeHostImpl(this);
> -    completion->signal();
> +    implThreadID = value;

nit: value->identifier or something like that

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImplProxy.h:73
> +    virtual GraphicsContext3D* context() = 0; // Temporary hack until the scheduling inversion is completed.
>  
> -    void start(); // Must be called before using the proxy.
> -    void stop(); // Must be called before deleting the proxy.
> +    virtual void finishAllRendering() = 0;
>  
> -    // CCLayerTreeHostImplCient -- called on CCThread
> -    virtual void postDrawLayersTaskOnCCThread();
> -    virtual void requestFrameAndCommitOnCCThread(double frameBeginTime);
> +    virtual bool isStarted() const = 0;
> +
> +    // The ownerHack argument is to allow introducing the ImplProxy subclasses
> +    // without also trying to change LayerRendererChromium to point at
> +    // CCLayerTreeHostImpl.
> +    virtual bool initializeLayerRenderer(CCLayerTreeHost* ownerHack) = 0;
> +
> +    virtual const LayerRendererCapabilities& layerRendererCapabilities() const = 0;
> +
> +    // Testing hook
> +    virtual void loseCompositorContext() = 0;
> +
> +    virtual void setNeedsCommitAndRedraw() = 0;
> +    virtual void setNeedsRedraw() = 0;
> +
> +    virtual void setVisible(bool) = 0;
> +
> +    virtual void start() = 0; // Must be called before using the proxy.
> +    virtual void stop() = 0; // Must be called before deleting the proxy.

can you reorder this to be:

actual interface (with doc)

testing hooks

hack stuff for now

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImplProxy.h:84
> +    static void setImplThread(WTF::ThreadIdentifier value);

nit: don't name the parameter here

> Source/WebCore/platform/graphics/chromium/cc/CCMainThreadTask.h:162
> +template<typename T, typename P1, typename MP1, typename P2, typename MP2, typename P3, typename MP3, typename P4, typename MP4>

we need to write a script to write these

[Adrienne Walker]

Comment on attachment 105197 [details]
All features work as advertised.

View in context: https://bugs.webkit.org/attachment.cgi?id=105197&action=review

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:-561
> -    if (!rootLayer()) {
> -        m_rootCCLayerImpl.clear();
> -        return;
> -    }

Maybe I'm being blind, but I still don't see where this check for a null root layer has moved to.  Can you point that out to me? I don't see where on the path to updateLayers this gets checked.

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.h:68
>  class CCLayerTreeHostCommitter;

If you're removing this class, can you remove this and the other few forward declarations to it?

[Adrienne Walker]

(In reply to comment #9)
> (In reply to comment #7)
> > I said this in a chat conversation, but I think the capabilities could be its own patch.
> I agree, for what its worth. If it proves that we're having a hard time getting our heads around this patch, I'll do my best to split it up. However, if we have a chance of getting a quality review without breaking it up, I'd definitely love the reduction of work it'd entail.

That sounds reasonable.

> > There's a lot of plumbing to get the...
> 
> Yeah, there is a Ton of plumbing to get these.

Nat and I talked a bit offline.  I guess I didn't realize how much work there would be to remove this plumbing.  Filing separate bugs to postpone this work is totally reasonable to keep this patch from getting even bigger.

[Nat Duca]

(In reply to comment #12)
> Maybe I'm being blind, but I still don't see where this check for a null root layer has moved to.


You're not blind. You right. :) My bad!


(In reply to comment #11)
> can you open a bug?
Bugs filed.

> TODO->FIXME. can we just make the USE(THREADED_COMPOSITING) path always make a CCThreadProxy for now
Yep! Done.

> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:116
> > +        // TODO: this is rumored to be redundant. If we can get things working, remove it.
> can we figure this out before landing?
Yep, I'm still chasing a few uglies so this is lower on my panic-radar than a few other things. :)

> > +        // If creation failed, and we had asked for accelerated painting, disable accelerated painting
> since we aren't supporting acceleratedPainting yet can we take this out for now?
I tried to reduce clean this up a lot. But, this code serves both the threaded and non-threaded cases, so "hacking for thread" doesn't really work.


> > Source/WebCore/platform/graphics/chromium/cc/CCMainThreadTask.h:162
> > +template<typename T, typename P1, typename MP1, typename P2, typename MP2, typename P3, typename MP3, typename P4, typename MP4>
> 
> we need to write a script to write these

https://bugs.webkit.org/show_bug.cgi?id=66962 will probably force such a discussion.

[Nat Duca]

Created attachment 105271 [details]
Incorporate feedback. Awesome names care-of Enne.

[Nat Duca]

*** Bug 58850 has been marked as a duplicate of this bug. ***

[James Robinson]

Comment on attachment 105271 [details]
Incorporate feedback. Awesome names care-of Enne.

View in context: https://bugs.webkit.org/attachment.cgi?id=105271&action=review

This looks really good.  Left a bunch of comments, but most of the nitpick variety.  This patch also doesn't apply cleanly to ToT, so please merge up and make sure that the tests still pass in debug and release.

Also make sure that when this lands it does not have the memory leaks https://bugs.webkit.org/show_bug.cgi?id=66981 fixes.  It looks like the shutdown path of this code will need at least some of the code in that patch to avoid leaking like crazy.

I'm going to be out of town for a few days, so Enne is going to take over reviewing this.  Would you mind rubber stamping when Enne's satisfied, Ken? Much appreciated.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:112
> +        // TODO: this is rumored to be redundant. If we can get things working, remove it.

please file a bug + link to it here before landing (or just remove the calls, if you can prove it's safe)

we use FIXME in WebKit, not TODO

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:153
> +GraphicsContext3D* CCLayerTreeHost::context()
> +{
> +    return m_proxy->context();
> +}

kinda surprised to see this here - the context lives over on the impl most of the time.  I'm guessing this is gonna go away, or maybe it's only supposed to be called during commit?  can it get thread ASSERT()s and/or FIXMEs?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:238
>  void CCLayerTreeHost::composite(bool finish)
>  {
> -    TRACE_EVENT("CCLayerTreeHost::composite", this, 0);
> -
> -    if (m_recreatingGraphicsContext) {
> -        // reallocateRenderer will request a repaint whether or not it succeeded
> -        // in creating a new context.
> -        reallocateRenderer();
> -        m_recreatingGraphicsContext = false;
> -        return;
> -    }
> -
> -    // Do not composite if the compositor context is already lost.
> -    if (!m_layerRenderer->isCompositorContextLost()) {
> -        doComposite();
> -
> -        // Put result onscreen.
> -        m_layerRenderer->present();
> -    }
> -
> -    if (m_layerRenderer->isCompositorContextLost()) {
> -        // Trying to recover the context right here will not work if GPU process
> -        // died. This is because GpuChannelHost::OnErrorMessage will only be
> -        // called at the next iteration of the message loop, reverting our
> -        // recovery attempts here. Instead, we detach the root layer from the
> -        // renderer, recreate the renderer at the next message loop iteration
> -        // and request a repaint yet again.
> -        m_recreatingGraphicsContext = true;
> -        setNeedsCommitAndRedraw();
> -    }
> -}
> -
> -void CCLayerTreeHost::loseCompositorContext()
> -{
> -    m_recreatingGraphicsContext = true;
> -}
> -
> -void CCLayerTreeHost::reallocateRenderer()
> -{
> -    RefPtr<LayerRendererChromium> layerRenderer = createLayerRenderer();
> -    if (!layerRenderer) {
> -        m_client->didRecreateGraphicsContext(false);
> -        return;
> -    }
> -
> -    layerRenderer->setLayerRendererRecursive(m_rootLayer->platformLayer());
> -    m_layerRenderer = layerRenderer;
> -
> -    m_client->didRecreateGraphicsContext(true);
> +    static_cast<CCSingleThreadProxy*>(m_proxy.get())->compositeImmediately();

looks like we're ignoring the 'finish' parameter, which seems right to me (i never knew what it was for anyway).  if so leave out the param name or at least comment it out so we don't get unused variable warnings.

can we get ASSERT()s that we aren't calling this with a threaded proxy?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.h:123
> +    bool compositeAndReadback(void *pixels, const IntRect&);

what's the return value for? since this is the public interface to the outside world (WebViewImpl) it'd be good to document what the caller is supposed to do here.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.h:125
>      void finishAllRendering();

this API is pretty problematic (i see you ASSERT_NOT_REACHED it in the threaded case).  do we have a bug to track this, or does it just fall into the 'need to invert scheduling' bucket?

> Source/WebCore/platform/graphics/chromium/cc/CCProxy.cpp:57
> +void CCProxy::setImplThread(bool value)
> +{
> +    fakeImplThread = value;

use something other than 'value'

> Source/WebCore/platform/graphics/chromium/cc/CCProxy.h:90
> +

nit: extra newline

> Source/WebCore/platform/graphics/chromium/cc/CCSingleThreadProxy.h:42
> +    // CCLayerTreeHostImplProxy implementation

CCLayterlkjdflkjsdfkljsdfProxy -> CCProxy

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:63
> +

nit: extra newline here

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:111
> +    // are pushed into the ok and caps local variables.

code is updated (thank you) but the comment isn't.
ok->initializeSucceeded
caps->capabilities

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:253
> +    ASSERT(isImplThread() && m_layerTreeHostImpl);

if you don't mind too much, can you make this 2 ASSERT()s instead of &&'ing the conditions? when a compound ASSERT() like this fails on the bots it's always a big pain in the butt to figure out which side failed. same goes for the rest of the file

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:254
> +    // TODO: should this pass through via the commit?

TODO -> FIXME

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.h:43
> +    // CCLayerTreeHostImplProxy implementation

CCLayerTresldkfjsdlkfjsdflkjProxy -> CCProxy

[Nat Duca]

(In reply to comment #17)
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.h:125
> >      void finishAllRendering();
> 
> this API is pretty problematic (i see you ASSERT_NOT_REACHED it in the threaded case).  do we have a bug to track this, or does it just fall into the 'need to invert scheduling' bucket?

I think this is actually safe. Its OK to block the main thread on the compositor. Its not ok to block in the reverse direction.

To eliminate this, we need to do this:
http://code.google.com/p/chromium/issues/detail?id=85086

[Nat Duca]

Created attachment 105385 [details]
Remove nits, rebaseline.

[WebKit Review Bot]

Comment on attachment 105385 [details]
Remove nits, rebaseline.

Attachment 105385 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9549067

[Nat Duca]

Created attachment 105505 [details]
Add precommit step and make sure we dont leak memory.

[Eric Seidel (no email)]

Comment on attachment 105505 [details]
Add precommit step and make sure we dont leak memory.

Attachment 105505 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/9563228

[Adrienne Walker]

View in context: https://bugs.webkit.org/attachment.cgi?id=105385&action=review

I guess I had previously assumed prior to looking at this too closely that the CCThreadProxy was in a workable state.  But, given some conversation, it sounds like it has a ways to go before it's fully implemented, but the CCSingleThreadProxy is workable.

I'll admit that I'm not really comfortable with the idea of landing dead and broken code in the tree.  There's some advantage to having it compile-checked, but I think that's outweighed by any confusion others might have about its state of functionality.  So, I think I'd prefer to just wait until CCThreadProxy worked to land it and it can get reviewed in a working state.  (Maybe we can also add at least a layout test to make sure it runs too.)  It doesn't look that tricky to remove from this patch, since there's maybe 4 lines of code and 2 files to cut and you can just revert that commit locally.

/me puts on eir WebKit nitpicking pants.

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:-562
> -    // Recheck that we still have a root layer. This may become null if
> -    // compositing gets turned off during a paint operation.
> -    if (!rootLayer()) {
> -        m_rootCCLayerImpl.clear();
> -        return;
> -    }
> -

...I still think we need a "if (!rootLayer()) return;" check here.

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.h:67
> +class CCLayerTreeHost; // TODO: remove pointers to this

TODO -> FIXME

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.h:45
> +    virtual void setNeedsRedrawOnImplThread() = 0;

Where is this used?

> Source/WebCore/platform/graphics/chromium/cc/CCProxy.cpp:58
> +void CCProxy::setImplThread(bool value)
> +{
> +    fakeImplThread = value;
> +}

s/value/isImplThread/

> Source/WebCore/platform/graphics/chromium/cc/CCSingleThreadProxy.cpp:70
> +

Unneeded whitespace.

> Source/WebCore/platform/graphics/chromium/cc/CCSingleThreadProxy.cpp:148
> +    m_layerTreeHost->scheduleComposite();

I assume this won't compile with threaded compositing turned on? Shouldn't there be a guard here?

I know you said you were also thinking about inverting where the scheduling happened?

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:263
> +    // TODO: should this pass through via the commit?

TODO -> FIXME.

I'm not sure if this should pass through the commit or not.  Would a setVisible(false) abort compositing a frame on the cc thread?

[Adrienne Walker]

(In reply to comment #23)

> I'll admit that I'm not really comfortable with the idea of landing dead and broken code in the tree.  There's some advantage to having it compile-checked, but I think that's outweighed by any confusion others might have about its state of functionality.  So, I think I'd prefer to just wait until CCThreadProxy worked to land it and it can get reviewed in a working state.  (Maybe we can also add at least a layout test to make sure it runs too.)  It doesn't look that tricky to remove from this patch, since there's maybe 4 lines of code and 2 files to cut and you can just revert that commit locally.

I had some conversation with nduca about this offline, and I realize that I'm coming into this patch sequence in the middle.  CCThreadProxy, previously known as ReallyLongNameProxy, already existed and the THREADED_COMPOSITING mode flag wasn't being built anywhere so probably didn't compile either.  So, none of this complaint is introduced by this patch.

Maybe I'll just express my frustration that even when moving quickly, I don't think it's worthwhile to land code that doesn't work at all.  Those things are better kept in a longer-running local branch, with any refactoring to other classes pushed upstream where that can be tested.  That process also keeps patches smaller and easier to review.

However, we're well on the way through this process and this will be fixed by a future patch soon enough, so I'll just let it be rather than create more work to clean up a mess that was already here.

[Nat Duca]

Created attachment 105524 [details]
Remove CCLayerTreeHostImplClient as it was unused. Pass visibility via commit. Fix compile with THREADED_COMPOSITING. Nit fixes.

[Adrienne Walker]

Comment on attachment 105524 [details]
Remove CCLayerTreeHostImplClient as it was unused. Pass visibility via commit. Fix compile with THREADED_COMPOSITING. Nit fixes.

View in context: https://bugs.webkit.org/attachment.cgi?id=105524&action=review

Thanks for adding the pre-commit step.  It's a necessary short-term band-aid.  :(

I think I'm down to just nits at this point.

> Source/WebCore/ChangeLog:41
> +        (WebCore::CCLayerTreeHost::commitTo):

ChangeLog needs a refresh.  :(

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.h:67
> +class CCLayerTreeHost; // TODO: remove pointers to this

nit: TODO -> FIXME

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.h:120
> +#if !USE(THREADED_COMPOSITING)
> +    void composite(bool finish);
> +#endif

nit: This doesn't look like it needs a finish parameter.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:43
> +    return adoptPtr(new CCLayerTreeHostImpl( settings));

nit: needless whitespace.  Kind of surprised this passed the style bot.

[Nat Duca]

Created attachment 105536 [details]
Patch

[Adrienne Walker]

Comment on attachment 105536 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=105536&action=review

> Source/WebCore/ChangeLog:3
> +        [chromium] Introduce CCLayerTreeHostImplMainThreadProxy in order to move LayerRenderer to CCLayerTreeHostImpl

nit: s/ReallyLongNameProxy/CCProxy/

[Nat Duca]

Created attachment 105537 [details]
Edit changelog for nit

[Adrienne Walker]

(In reply to comment #29)
> Created an attachment (id=105537) [details]
> Edit changelog for nit

Unofficially, this looks good to me.

[Adrienne Walker]

(In reply to comment #30)
> (In reply to comment #29)
> > Created an attachment (id=105537) [details] [details]
> > Edit changelog for nit
> 
> Unofficially, this looks good to me.

...so long as it doesn't make cr-mac-ews cry.

[Nat Duca]

Created attachment 105589 [details]
Win ews failure likely flake

[Nat Duca]

Created attachment 105853 [details]
Fixes for lost context, video, readback.

[WebKit Review Bot]

Comment on attachment 105853 [details]
Fixes for lost context, video, readback.

Attachment 105853 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9570777

[Nat Duca]

Created attachment 105871 [details]
make ews happy

[Adrienne Walker]

(In reply to comment #35)
> Created an attachment (id=105871) [details]
> make ews happy

This (unofficially) looks awesome to me.  Can this get an official review by somebody?

[James Robinson]

Comment on attachment 105871 [details]
make ews happy

R=me

[Nat Duca]

Created attachment 105998 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 105998 [details]
Patch for landing

Rejecting attachment 105998 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
8b9618a02581e43138353a742366cf80bdef5ff0
r94329 = 9eb422c0b4bfca80a17f63a1e3d849c8b5b38f19
Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc
First, rewinding head to replay your work on top of it...
Fast-forwarded master to refs/remotes/origin/master.
Updating chromium port dependencies using gclient...

________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
Updating webkit projects from gyp files...

Full output: http://queues.webkit.org/results/9580351

[James Robinson]

Comment on attachment 105998 [details]
Patch for landing

cq barfed in a non-obvious way, flippin' bit to try again

[WebKit Review Bot]

Comment on attachment 105998 [details]
Patch for landing

Rejecting attachment 105998 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
tching file Source/WebCore/platform/graphics/chromium/cc/CCThreadTask.h
patching file Source/WebCore/platform/graphics/chromium/cc/CCTiledLayerImpl.cpp
patching file Source/WebCore/platform/graphics/chromium/cc/CCVideoLayerImpl.cpp
patching file Source/WebKit/chromium/ChangeLog
patching file Source/WebKit/chromium/src/WebViewImpl.cpp
patching file Source/WebKit/chromium/tests/CCThreadTaskTest.cpp

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force']" exit_code: 1

Full output: http://queues.webkit.org/results/9578517

[Nat Duca]

Committed r94353: <http://trac.webkit.org/changeset/94353>

[James Robinson]

I think this patch re-introduced https://bugs.webkit.org/show_bug.cgi?id=66981.  Try this:

1) open poster circle
2) open task manager, look at GPU memory use
3) right click on the poster circle tab, hit 'duplicate tab' a bunch of times. each time the GPU memory use should increase (since the new tabs have textures)
4) close all tabs except the first one. the GPU memory use should return to the value in step (2), but on ToT it doesn't go down at all when the duplicated tabs are closed

:(  I'm looking

[Nat Duca]

> 
> :(  I'm looking

Womp. Sorry! :/ Let me know if you need me to take this on since its my regression. Sleep is overrated.