[webkit-reviews] review denied: [Bug 58408] [chromium] Implement CCLayerTreeHost and CCLayerTreeHostImpl portions of threaded compositor : [Attachment 90631] Death by a thousand corner cases.

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has denied Nat Duca <nduca at chromium.org>'s
request for review:
Bug 58408: [chromium] Implement CCLayerTreeHost and CCLayerTreeHostImpl
portions of threaded compositor
https://bugs.webkit.org/show_bug.cgi?id=58408

Attachment 90631: Death by a thousand corner cases.
https://bugs.webkit.org/attachment.cgi?id=90631&action=review

------- Additional Comments from James Robinson <jamesr at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=90631&action=review

Close!	You'll need Darin approval for the WebView changes - probably a good
idea to start bothering him in parallel with fixing up the last nits.  Also, be
sure this applies to ToT - the draw part of updateAndDrawLayers() has changed a
bit and you might have some conflicts there.

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.h:187
> +    bool m_committing;

this version of m_committing is declared and initialized but never use. remove
or use

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.h:43
> +    virtual PassRefPtr<GraphicsContext3D> createLayerTreeHostContext3D() =
0;

This is still here but not needed in the current codepath since the LRC is
recreated on lost context.  I'm pretty sure you don't intend to rewrite lost
context support in this patch, so please remove this for now.  We can add it if
we need it later.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.h:71
> +#ifndef NDEBUG
> +    bool m_committing;
> +#endif

now this is declared and updated but never actually used (there aren't any
ASSERT()s or branches on this variable).  use it somewhere or nuke - it's just
confusing this way.

> Source/WebKit/chromium/public/WebWidget.h:67
> +    // FIXME: remove default argument once render_widget has been updated to

> +    // issue the correct value.
> +    virtual void animate(double frameBeginTime) = 0;

there's a FIXME here but no default argument - which is wrong?

> Source/WebKit/chromium/src/WebViewImpl.cpp:2347
> +#if USE(THREADED_COMPOSITyING)

typo here.  this probably doesn't work at all then, right?

> Source/WebKit/chromium/src/WebViewImpl.cpp:2517
> +    if (!m_layerRenderer)
> +	   return;

it's still not clear when this is called with a null m_layerRenderer - as far
as I can tell, the m_layerRenderer lifetime is not changed by this patch