[webkit-reviews] review granted: [Bug 70160] [chromium] Route requestAnimationFrame through CCProxy in threaded mode : [Attachment 111112] Patch

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has granted Nat Duca
<nduca at chromium.org>'s request for review:
Bug 70160: [chromium] Route requestAnimationFrame through CCProxy in threaded
mode
https://bugs.webkit.org/show_bug.cgi?id=70160

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

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


R=me - have some nits to address while landing.

> Source/WebCore/platform/graphics/chromium/cc/CCSingleThreadProxy.cpp:144
> +    ASSERT(0);

we have ASSERT_NOT_REACHED() for this

> Source/WebKit/chromium/src/ChromeClientImpl.cpp:542
> +#if USE(ACCELERATED_COMPOSITING)
> +    if (!m_webView->isAcceleratedCompositingActive())
> +#endif
> +	   m_webView->client()->scheduleAnimation();
> +#if USE(ACCELERATED_COMPOSITING)
> +    else
> +	   m_webView->scheduleAnimation();
> +#endif

i think things would be slightly cleaner if
ChromeClientImpl::scheduleAnimation() simply delegated to
m_webView->scheduleAnimation() and all of the #if USE(ACCELE...) etc crap was
inside WebViewImpl.cpp. WebViewImpl::scheduleAnimation() call call out to its
client if it needs to

> Source/WebKit/chromium/src/WebViewImpl.h:-384
> -    void doUpdateAndComposite();

guessing this function wasn't actually defined or referenced before?