[webkit-reviews] review denied: [Bug 56131] [chromium] Compositor thread infrastructure : [Attachment 85364] 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 56131: [chromium] Compositor thread infrastructure
https://bugs.webkit.org/show_bug.cgi?id=56131

Attachment 85364: Patch
https://bugs.webkit.org/attachment.cgi?id=85364&action=review

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

Cool start.  Left comments and cleared the review bit so it doesn't show up in
the review queue.

At a design level I think it's a bit weird that the message objects themselves
implement run().  The chrome model is that messages are essentially enums with
parameters and the receiving code has to switch on the message type in order to
figure out what to run.  Tasks in chrome-land are data bags and pointers to
functions that execute the task itself.

I think that the message model would be a better model to follow here since it
more clearly separates what one thread wants to say to the other (for example
"begin paint with this timestamp parameter") from how the thread responds to
that message.

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.h:70
> +// Callback interface for CCViews
>  // Class that handles drawing of composited render layers using GL.
> -class LayerRendererChromium : public RefCounted<LayerRendererChromium> {
> +class LayerRendererChromium : public CCView {

not sure what the comment means here. do you need it?

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.h:72
>      static PassRefPtr<LayerRendererChromium>
create(PassRefPtr<GraphicsContext3D> graphicsContext3D);

since you are editing lines close by...this parameter shouldn't be named

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.h:81
> +    // sets viewport parameters
> +    void setViewport(const IntRect& visibleRect, const IntRect& contentRect,


this comment doesn't really add anything - i'd say kill it

> Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.h:16
> - * THIS SOFTWARE IS PROVIDED BY APPLE INC. AND ITS CONTRIBUTORS ``AS IS''
AND ANY
> + * THIS SOFTWARE IS PROVIDED BY GOOGLE INC. AND ITS CONTRIBUTORS ``AS IS''
AND ANY
>   * EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED

>   * WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
> - * DISCLAIMED. IN NO EVENT SHALL APPLE INC. OR ITS CONTRIBUTORS BE LIABLE
FOR ANY
> + * DISCLAIMED. IN NO EVENT SHALL GOOGLE INC. OR ITS CONTRIBUTORS BE LIABLE
FOR ANY

don't change this

> Source/WebCore/platform/graphics/chromium/cc/CCThread.cpp:13
> + * THIS SOFTWARE IS PROVIDED BY GOOGLE INC. AND ITS CONTRIBUTORS ``AS IS''
AND ANY

nope - this should say APPLE INC.  use this header exactly:
http://trac.webkit.org/browser/trunk/Source/WebCore/LICENSE-APPLE changing only
the top line and not anything else

> Source/WebCore/platform/graphics/chromium/cc/CCThread.cpp:61
> +    void* unused;
> +    waitForThreadCompletion(m_threadId, &unused);

you can pass NULL as the second parameter

this is a little scary actually since the value of 'unused' will be some random
garbage from the stack and waitForThreadCompletion may try to write to the
memory at that location.

> Source/WebCore/platform/graphics/chromium/cc/CCThread.cpp:78
> +void CCThread::postMessage(CCThreadMessage* msg)
> +{
> +    m_messageQueue.append(msg);

is the idea that this will only be called from the main thread to post a
message to the compositor thread? could use some ASSERT()s to that effect.

> Source/WebCore/platform/graphics/chromium/cc/CCThread.h:13
> + * THIS SOFTWARE IS PROVIDED BY GOOGLE INC. AND ITS CONTRIBUTORS ``AS IS''
AND ANY

nope

> Source/WebCore/platform/graphics/chromium/cc/CCThread.h:56
> +    // Called just before run... Allows an opportunity to convert
> +    // MainThread objects, e.g. LayerRenderChromium, to their
> +    // compositor thread counterpart.
> +    virtual void beforeRun() { }

Not sure I totally understand why this needs to be separate - couldn't all the
logic in beforeRun() also happen at the top of the subclass's run()
implementation?

> Source/WebCore/platform/graphics/chromium/cc/CCThread.h:84
> +    void postMessage(CCThreadMessage* msg);
> +    void postMessage(CCMainThreadMessage* msg);

nit: no need to name these params

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

++ ?

> Source/WebCore/platform/graphics/chromium/cc/CCViewManager.cpp:204
> +    // Right now, this condition variable is build using a simple
> +    // Mutex object, exploiting the fact that WTF Mutex lock calls

in general you can't build a condition variable with a mutex so this comment is
pretty confusing.

in general i'd rather we just use the textbook synchronization techniques
instead of trying to be clever unless we know that something is going to be a
performance bottleneck. synchronization is hard and i want to try to spend as
few brain cells on it as possible while we bootstrap everything else

> Source/WebCore/platform/graphics/chromium/cc/NonThreadSafe.h:33
> +template <class T>
> +class NonThreadSafe {

this looks nifty, but it really probably belongs in wtf/. can you make a patch
to add it there?

> Source/WebKit/chromium/public/WebWidget.h:84
> +    // Temporary (do not commit) hack until Darin can land his inversion
code
> +    virtual void scheduleComposite()  { }

should this be guarded by the #if?

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

this seems wrong - we still need a way to implement WebViewImpl::paint() even
in the threaded case so we can handle thumbnails and whatnot. is this just
temporary?

> Source/WebKit/chromium/src/WebViewImpl.h:338
> +    void doUpdateAndComposite();

we don't normally put "do" in functions like this as it doesn't really convey
any information. I think updateAndComposite() would be a good name for this

> Source/WebKit/chromium/src/WebViewImpl.h:402
> +    void doAnimate(double frameBeginTime);

what's the difference between animate() and doAnimate()? just the provided
frame time?  in that case it might make more sense to have animate() take a
double param with a default value of 0 meaning 'use whatever time you want'. 
the "do" is not super informative.