[webkit-reviews] review granted: [Bug 73350] [chromium] Allow scrolling non-root layers in the compositor thread : [Attachment 119898] Patch

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has granted Sami Kyostila
<skyostil at google.com>'s request for review:
Bug 73350: [chromium] Allow scrolling non-root layers in the compositor thread
https://bugs.webkit.org/show_bug.cgi?id=73350

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

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


R=me, but I'd appreciate it if you let aelias@ take a pass over the page scale
apply/unapply logic to make sure it's all consistent.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:626
> +    bool didMove = m_scrollLayerImpl &&
(didScrollSubtree(m_scrollLayerImpl.get()) || m_pageScaleDelta != 1.0f);

webkit style nit: the comparison should be written as "m_pageScaleDelta != 1",
without the ".0f" qualifier.

> Source/WebKit/chromium/src/ChromeClientImpl.cpp:68
> +#include "RenderLayer.h"

pretty sure you don't need this #include any more now either (it isn't wrong in
terms of code layering, just unnecessary)

> Source/WebKit/chromium/tests/CCLayerTreeHostImplTest.cpp:172
> +    RefPtr<CCLayerImpl> root = CCLayerImpl::create(0);
> +    root->setScrollable(false);
> +    RefPtr<CCLayerImpl> child = CCLayerImpl::create(0);

nit: i think it's generally a bad idea to construct CCLayerImpls with
overlapping IDs in tests since we maintain the invariant that we never reuse
IDs in the normal code.