[webkit-reviews] review denied: [Bug 78864] Register fixed position layers with ScrollingCoordinator : [Attachment 147081] 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 78864: Register fixed position layers with ScrollingCoordinator
https://bugs.webkit.org/show_bug.cgi?id=78864

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

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


You need to change the value that ScrollingCoordinatorChromium returns for now
and I have some naming concerns, but otherwise I think this looks pretty good. 
Can you verify whether that layout test failure on cr-ews is really due to this
patch?

> Source/WebCore/page/scrolling/ScrollingCoordinator.cpp:352
> +	   if (!fixedObject->isBoxModelObject())
> +	       return true;
> +	   RenderBoxModelObject* fixedBoxModelObject =
toRenderBoxModelObject(fixedObject);
> +	   if (!fixedBoxModelObject->layer() ||
!fixedBoxModelObject->layer()->backing())

check hasLayer() before doing the downcast - you can call hasLayer() on a
RenderObject - it doesn't even need a virtual call to do it!

> Source/WebCore/page/scrolling/ScrollingCoordinator.h:124
> +    void setLayerIsFixedToContainerLayerVisibleRect(GraphicsLayer*, bool);

I don't think "that ContainerLayerVisibleRect" makes any sense - what is
LayerVisibleRect and why does it matter?  Why not just
"setLayerIsFixedToContainer()"?

> Source/WebCore/page/scrolling/chromium/ScrollingCoordinatorChromium.cpp:166
> +    return true;

I think you need to say "false" here until we actually land the backend support
for this. Otherwise, we'll break scrolling on pages with fixpos elements today.