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

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


It appears as if this patch will break threaded scrolling on any page with a
position:fixed element that happens to be composited (because of a transform,
overlap, or anything else) - the ScrollingCoordinator will not correctly mark
the view as needing updates on the main thread and the fixpos element will
jitter like crazy.  I'm concerned about the level of testing that's gone on if
this is the case.

> Source/WebCore/ChangeLog:17
> +	   Note that composition of fixed position elements is currently behind
a
> +	   setting (Settings::acceleratedCompositingForFixedPositionEnabled),
so
> +	   this functionality is not enabled by default.
> +

This comment is wrong and I'm not sure why it is here at all.  Fixed position
elements may or may not be composited completely independently of the
acceleratedCompositingForFixedPositionEnabled Setting - that setting simply
changes the compositing trigger so that some fixpos elements that otherwise
wouldn't be composited are.  The concern for ScrollingCoordinator is whether
the presence/absence of fixpos elements changes shouldScrollOnMainThread -
that's something that the ScrollingCoordinator implementation needs to figure
out in coordination (lol) with the compositor implementation it's talking to. 
That isn't something controlled by Settings at all

> Source/WebCore/page/scrolling/ScrollingCoordinator.cpp:348
> +	   RenderObject* o = *it;

Don't use one-letter variable names

> Source/WebCore/page/scrolling/ScrollingCoordinator.cpp:352
> +	   RenderBox* box = toRenderBox(o);
> +	   if (!box->hasLayer() || !box->layer()->backing())

I don't understand this downcast.  RenderBox doesn't expose hasLayer() or
layer(), those are on RenderObject.  Perhaps you meant RenderBoxModelObject
(since that's the root class of all ROs that could have layers)?  Even that
would be weird - why downcast at all?

> Source/WebCore/page/scrolling/ScrollingCoordinator.cpp:367
> +	   || hasNonLayerFixedObjects(frameView)

This check is insufficient to know if we should update the scroll layer
position on the main thread - we also have to know if this compositor
implementation knows how to handle fixpos on a non-main thread.  One way to do
this would be to have a function to query this capability that has a default
impl that returns false and let people provide other implementations in
ScrollingCoordinator*.cpp as they gain that capability.