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

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has denied 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 119285: Patch
https://bugs.webkit.org/attachment.cgi?id=119285&action=review

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


R- for several layering violations. I think a good place to start for this is
to think about where the interface boundaries lie between the compositor and
the thing that's scrolling, then go from there to figure out the proper
interface boundaries.

> Source/WebCore/platform/graphics/chromium/LayerChromium.h:39
> +#include "Node.h"

no no no, you can't depend on Node in here. This is in WebCore/platform so it
can't depend on anything in WebCore outside of WebCore/platform.

are you sure you want to be talking about Nodes here anyway? there are things
that aren't Nodes that need to scroll. are you perhaps looking for
ScrollableArea? the fact that you are taking an actual reference here really
scares me

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:32
> +#include "RenderBox.h"

you can't use code from WebCore/rendering/ here (nor do i think you want to)