Bug 78739 – [chromium] Wire up ScrollingCoordinator to chromium compositor

• Status: RESOLVED
• Resolution: INVALID
• Component: New Bugs

[James Robinson]

[chromium] Wire up ScrollingCoordinator to chromium compositor

[James Robinson]

Created attachment 127229 [details]
Patch

[James Robinson]

This depends on https://bugs.webkit.org/show_bug.cgi?id=78401

[Adrienne Walker]

Comment on attachment 127229 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=127229&action=review

> Source/WebKit/chromium/src/WebViewImpl.cpp:3050
> +            page()->settings()->setScrollingCoordinatorEnabled(true);
> +            page()->scrollingCoordinator()->setClient(scrollingCoordinatorClientImpl());

This is just a nit, but this function is getting to be a mess, and it's sad that you have to repeat these two sets of calls in two places.  Can you refactor it any to unify what needs to happen when compositing is successfully turned on or off?

[James Robinson]

(In reply to comment #3)
> (From update of attachment 127229 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=127229&action=review
> 
> > Source/WebKit/chromium/src/WebViewImpl.cpp:3050
> > +            page()->settings()->setScrollingCoordinatorEnabled(true);
> > +            page()->scrollingCoordinator()->setClient(scrollingCoordinatorClientImpl());
> 
> This is just a nit, but this function is getting to be a mess, and it's sad that you have to repeat these two sets of calls in two places.  Can you refactor it any to unify what needs to happen when compositing is successfully turned on or off?

Yes it's ugly.  I'm not completely in love with having to reach down to the ScrollingCoordinator from up here in WebKit, but don't see a better way to interface with what's there today.  If we go this route I'll add some helper functions to tighten this up.

[James Robinson]

Created attachment 127278 [details]
Patch

[James Robinson]

This moves wheel tracking down to be per layer and cleans up a few other things (including setting/clear the client).  I think this is looking fairly plausible to land, once 78401 or something like it lands.

[Adrienne Walker]

Comment on attachment 127278 [details]
Patch

This looks good to me.  I dig the wheel handlers on layers change.  Thanks also for cleaning up WebViewImpl a little.

[James Robinson]

I think I'll do a bit more chopping - ScrollingTreeState isn't really a useful intermediate for us since we buffer the tree via the commit flow, so I think I'll just route the setters on ScrollingCoordinator straight through to the LayerChromium tree.  That should get rid of a bit of gobble-dee-gook.

[James Robinson]

Created attachment 127510 [details]
Patch

[James Robinson]

Even svelter.  This pretty much completely ignored the ScrollingTreeState buffering, since we buffer changes through commit.  Doesn't fully work yet (esp. transitioning out of compositing mode mode) but getting closer.

[James Robinson]

Created attachment 127642 [details]
Patch

[James Robinson]

I'm pretty sure everything works here except that when we transition out of compositing mode we need to convince WebCore that the ScrollingCoordinator isn't in charge of that view any more.

[James Robinson]

Created attachment 127883 [details]
Patch

[James Robinson]

There are some bugs when switching pages but I think this is in pretty good shape.  I'll break it up into a few pieces to land (at the very least separating the cross-platform code from the Chromium-specific bits, and possibly separating the properties into their own patches).

[James Robinson]

Created attachment 127885 [details]
Patch

[James Robinson]

Created attachment 127888 [details]
Patch

[WebKit Review Bot]

Comment on attachment 127888 [details]
Patch

Attachment 127888 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11556051

New failing tests:
compositing/checkerboard.html
compositing/geometry/limit-layer-bounds-clipping-ancestor.html
compositing/geometry/foreground-layer.html
compositing/layer-creation/overlap-transformed-layer.html
compositing/images/clip-on-directly-composited-image.html
compositing/geometry/clip.html
compositing/iframes/connect-compositing-iframe2.html
compositing/geometry/fixed-position-transform-composited-page-scale.html
compositing/layer-creation/scroll-partial-update.html
accessibility/aria-describedby-on-input.html
compositing/geometry/fixed-position-composited-switch.html
compositing/iframes/become-overlapped-iframe.html
compositing/geometry/layer-due-to-layer-children-switch.html
compositing/geometry/fixed-position-iframe-composited-page-scale.html
compositing/iframes/connect-compositing-iframe3.html
compositing/iframes/enter-compositing-iframe.html
compositing/geometry/limit-layer-bounds-fixed-positioned.html
compositing/geometry/layer-due-to-layer-children-deep-switch.html
compositing/geometry/ancestor-overflow-change.html
compositing/layer-creation/overlap-clipping.html
compositing/geometry/fixed-position-composited-page-scale.html
compositing/iframes/connect-compositing-iframe.html
compositing/iframes/become-composited-nested-iframes.html
compositing/layer-creation/overflow-scroll-overlap.html
compositing/iframes/connect-compositing-iframe-delayed.html
compositing/layer-creation/rotate3d-overlap.html
compositing/layer-creation/overlap-child-layer.html
compositing/geometry/fixed-position-iframe-composited-page-scale-down.html
compositing/iframes/composited-parent-iframe.html
compositing/geometry/fixed-position-transform-composited-page-scale-down.html

[James Robinson]

Cross-platform changes here: https://bugs.webkit.org/show_bug.cgi?id=79126

Wheel event handler here: https://bugs.webkit.org/show_bug.cgi?id=79133

I'll do the remaining properties as separate patches, so closing this bug.