[webkit-reviews] review denied: [Bug 70291] [chromium] Allow CCLayerTreeHostImpl to call back to proxy via CCLayerTreeHostImplClient : [Attachment 111360] Make LTHI the scroll controller.

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has denied Nat Duca <nduca at chromium.org>'s
request for review:
Bug 70291: [chromium] Allow CCLayerTreeHostImpl to call back to proxy via
CCLayerTreeHostImplClient
https://bugs.webkit.org/show_bug.cgi?id=70291

Attachment 111360: Make LTHI the scroll controller.
https://bugs.webkit.org/attachment.cgi?id=111360&action=review

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


R- cos I think this loops forever and thus isn't tested very well, but it's
looking great otherwise

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

why are these suffixed OnImplThread() instead of OnCCThread() like the
functions in CCThreadProxy are? It's confusing to be similar but slightly
different from the convention established there

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.h:54
> +class CCLayerTreeHostImpl : public CCScrollController {

this line makes CCLayerTreeHostImpl::scrollRootLayer() virtual and an
implementation of CCScrollController. Please document this in the header (make
it virtual and say what interface it's implementing)

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.h:99
> +    explicit CCLayerTreeHostImpl(const CCSettings&,
CCLayerTreeHostImplClient*);

nit: no need explicit any more

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.h:68
> +    virtual void setNeedsCommitOnImplThread() {
setNeedsCommitOnImplThread(); }

isn't this an infinite loop?

if these were suffixed OnCCThread() then you wouldn't need these thunks, you
could just make the actual functions virtual