[webkit-reviews] review denied: [Bug 58408] [chromium] Implement CCLayerTreeHost and CCLayerTreeHostImpl portions of threaded compositor : [Attachment 90116] Patch

[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 90116: Patch
https://bugs.webkit.org/attachment.cgi?id=90116&action=review

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

R- because I think this will make the !USE(THREADED_COMPOSITOR) code crash in
~LayerRendererChromium() and because I think some parts can be simplified.  I
read the whole patch this time, though, and think it's looking really good
overall.  Left lots of comments to digest.

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:-191
> -    ASSERT(m_hardwareCompositing);
> -

dropping this ASSERT() from updateLayers() makes sense, but please add it back
to drawLayers()

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:213
> +    ASSERT(m_computedRenderSurfaceLayerList);

should we clear this vector out at the end of drawLayers() or its caller?
doesn't make much sense to leave a vector around between frames if we always
throw it away on the next frame

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:1096
> +class LayerRendererChromiumCommitter : public CCLayerTreeHostCommitter {

why does this exist?  i don't think the eventual implementation of
CCLayerTreeHostCommitter will have anything LayerRendererChromium-specific in
it, so i think we should just put the commit logic inside
CCLayerTreeHostCommitter itself and not subclass it.

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:1124
> +class LayerRendererChromiumImpl : public CCLayerTreeHostImpl {

it's very strange to have this class in LayerRendererChromium.cpp.  this should
go into its own file with its own header

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:1166
> +class LayerRendererChromiumImplProxy : public CCLayerTreeHostImplProxy {

this also deserves its own file and header

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.h:149
> +    explicit LayerRendererChromium(CCLayerTreeHostClient*,
PassRefPtr<GraphicsContext3D>, PassOwnPtr<TilePaintInterface> contentPaint);

don't need explicit when the c'tor takes 3 arguments (didn't need it with 2
either, but might as well fix it now)

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

since this bool is just for debugging please guard the defn and uses with with
#ifndef NDEBUG/#endif.	do you think it's likely that we'll accidentally make
commit() reentrant?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:36
> +    , m_frameNumber(0)

why does the frame number start at 0 here and at -1 on the impl?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:53
> +#if USE(THREADED_COMPOSITING)
> +    m_proxy = createLayerTreeHostImplProxy();
> +    m_proxy->start();
> +#endif
> +}
> +
> +CCLayerTreeHost::~CCLayerTreeHost()
> +{
> +    TRACE_EVENT("CCLayerTreeHost::~CCLayerTreeHost", this, 0);
> +    m_proxy->stop();
> +    m_proxy.clear();
> +}

hmm, seems a bit inconsistent to guard the init but not the shutdown.  won't
this cause a crash when creating and destroying a LayerRendererChromium without
USE(THREADED_COMPOSITING) set?

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

this level of indirection doesn't seem necessary or helpful.  In the non-mock
codepath WebViewImpl passes a GraphicsContext3D to
LayerRendererChromium::create().  In the mock case, you can just pass 0 in to
LRC::create().

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:76
> +    m_frameNumber += 1;

++m_frameNumber; ?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.h:70
> +    bool m_redrawEnqueued;

m_redrawPending would be a more consistent and a bit easier to spell for
simpletons like me :)

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImplProxy.cpp:27
> +#include "CCLayerTreeHostImplProxy.h"

files in the /cc/ directory in this patch are inconsistent about whether they
prefix #includes with "cc/".  pick a convention and stick to it (the files i've
added in cc/ include the prefix, it looks like most of the files you've added
do not).  i don't feel strongly about which convention is better but we need to
be consistent.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImplProxy.cpp:39
> +CCThread* ccThread;

should this be an OwnPtr<>?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImplProxy.cpp:49
> +    numProxies += 1;

++ ?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImplProxy.cpp:67
> +    ASSERT(!m_layerTreeHostImpl && !m_layerTreeHost);

feels a bit weird to poke at the m_layerTreeHostImpl ptr from the main thread.
add a comment for whatever threading convention makes this query threadsafe

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImplProxy.cpp:69
> +    numProxies -= 1;

-- ?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImplProxy.cpp:72
> +	   delete ccThread;
> +	   ccThread = 0;

if ccThread was an OwnPtr<> then this would be one line

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImplProxy.cpp:102

> +    ASSERT(!m_layerTreeHostImpl); // layerTreeHostClosedOnCCThread() should
have already been called

feels a little dodgy to be querying this value from the main thread since it's
written from the compositor thread.  are we protected here by the
CCCompletionEvent's condvar?  whatever the answer is it deserves a comment

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImplProxy.h:38
> +class CCLayerTreeHostImplProxy : public CCLayerTreeHostImplClient {
> +public:

if this isn't meant to be refcounted it should have WTF_MAKE_NONCOPYABLE() blah
blah.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImplProxy.h:44
> +    void start(); // Must be called before using the proxy.

this is normally called init(). is there any reason that the c'tor or the
::create() function for CCLayerTreeHostImplProxy can't call this for you?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImplProxy.h:45
> +    void stop(); // Must be called before deleting the proxy.

why can't this happen in the d'tor? no virtual calls are being made in the
implementation

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImplProxy.h:73
> +    OwnPtr<CCLayerTreeHostImpl> m_layerTreeHostImpl;

can you add comments for each of these members indicating which thread the data
can be accessed on? i'm pretty sure the answer is
mainthread/mainthread/ccthread but it doesn't hurt to be sure. might even be
worth suffixing the member names just to make it super clear what is going on

also, the class will pack better when more members are added if you sort by
size (larger members like ptrs first, smaller members like bools later).

> Source/WebKit/chromium/ChangeLog:8
> +	   Stress tests for CCLayerTreeHost.

there are a number of changes here other than just stress tests (the WebWidget
and WebViewImpl changes). please describe those in the changelog as well

> Source/WebKit/chromium/public/WebWidget.h:88
> +    // In threaded compositing mode, indicates that the widget should redraw

> +    // itself, for example due to window damage. The redraw will begin
> +    // asynchronously.
> +    virtual void scheduleComposite()  { }
> +

how is this different from WebWidget::composite(false) ?

> Source/WebKit/chromium/src/WebViewImpl.cpp:999
>  void WebViewImpl::animate()
>  {
> +    doAnimate(currentTime());
> +}
> +
> +void WebViewImpl::doAnimate(double frameBeginTime)

oh how awkward :(.  a better solution would be to have animate() always take
the timestamp parameter and have the caller be responsible for providing it.
even in the software case we really should have the caller provide the
timestamp - i wish i had done this way first. feel free to file a cleanup bug
on me to sort that out.

in the meantime, a cleaner solution to this problem is to add a default
parameter value to WebViewImpl::animate(). callers who specify a timestamp have
that value used, callers who don't just get currentTime()

> Source/WebKit/chromium/src/WebViewImpl.cpp:1078
> +	   ASSERT(0);

ASSERT_NOT_REACHED();

> Source/WebKit/chromium/src/WebViewImpl.cpp:1121
> +#if USE(THREADED_COMPOSITING)
> +    return; // FIXME: when the RenderWidget is updated, switch to
ASSERT_NOT_REACHED.
> +#else

why does this early out? if finish==false, then this should just do
m_layerRenderer->setNeedsRedraw(). if finish==true then we definitely need an
ASSERT_NOT_REACHED just like we do in ::paint()

> Source/WebKit/chromium/src/WebViewImpl.cpp:2492
> +#if USE(THREADED_COMPOSITING)
> +    updateLayerRendererSettings();
> +#endif

why are we setting these bits only in the USE(THREADED_COMPOSITOR) case?

> Source/WebKit/chromium/src/WebViewImpl.cpp:2531
>  void WebViewImpl::updateLayerRendererViewport()
>  {
> -    ASSERT(m_layerRenderer);
> +    if (!m_layerRenderer)
> +	   return;

why does this change?

> Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:314
> +// Shortlived layerTreeHosts shoudn't die with a commit in flight.

typo: shouldn't

> Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:334
> +// Shortlived layerTreeHosts shoudn't die with a redraw in flight.

typo: shouldn't

> Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:372
> +	   m_numCompleteCommits += 1;

++

> Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:399
> +
> +
> +

weird # of newlines

> Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:419
> +	   if (!layerTreeHostImpl->sourceFrameNumber()) // frame 0

comment isn't really that helpful, i'd remove

> Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:445
> +// first commited frame draws should lead to another commit.

typo: committed

> Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:461
> +	   if (!layerTreeHostImpl->sourceFrameNumber()) // frame 0

comment unnecessary

> Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:531
> +
> +
> +

weird # of newlines