Bug 79133 – [chromium] Notify compositor of wheel event registration via ScrollingCoordinator

• Component: New Bugs
• Status: RESOLVED FIXED

[James Robinson]

[chromium] Notify compositor of wheel event registration via ScrollingCoordinator

[James Robinson]

Created attachment 128008 [details]
Patch

[WebKit Review Bot]

Comment on attachment 128008 [details]
Patch

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

New failing tests:
compositing/checkerboard.html
compositing/geometry/limit-layer-bounds-clipping-ancestor.html
compositing/geometry/foreground-layer.html
compositing/iframes/connect-compositing-iframe-delayed.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/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/layer-creation/overlap-transformed-layer.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]

Created attachment 128052 [details]
Patch

[James Robinson]

Test failures due to the setScrollingCompositorEnabled() bit, looks like we are not quite ready for that.  This patch has all the rest of the plumbing and is IMO covered enough by tests for now.

[James Robinson]

Created attachment 128089 [details]
rebase to ToT

[Adrienne Walker]

Comment on attachment 128089 [details]
rebase to ToT

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

LGTM.

> Source/WebCore/page/scrolling/chromium/ScrollingCoordinatorChromium.cpp:58
> +    delete m_private;

It's too bad OwnPtr doesn't work on forward-declared classes.  :(

[Dimitri Glazkov (Google)]

Comment on attachment 128089 [details]
rebase to ToT

looks pretty. Consider using pimpl thing that Eric and Adam did with HTMLTreeBuilder::ExternalCharacterTokenBuffer. ScrollingCoordinator::ForRealz

[James Robinson]

(In reply to comment #7)
> (From update of attachment 128089 [details])
> looks pretty. Consider using pimpl thing that Eric and Adam did with HTMLTreeBuilder::ExternalCharacterTokenBuffer. ScrollingCoordinator::ForRealz

I'm not sure that will quite work here, since the idea is to avoid exposing the details of ScrollingCoordinatorPrivate to everyone who uses ScrollingCoordinator. I think in a follow-up I'll look at moving some of the members currently guarded ENABLE(THREADED_SCROLLING) into this private thing, that seems like a good opportunity to see if we can figure out a name that'll work for everyone.

[James Robinson]

Committed r108698: <http://trac.webkit.org/changeset/108698>

[Sam Weinig]

In the future, can you please not put [chromium] on bugs that change cross platform code.  I realize it was only a little cross platform code, but I since it is new and under development, I would still like Anders to be informed and to at least give a once over, new code being added.

[James Robinson]

OK, noted.  I didn't think Anders would really need to see 2 lines of ScrollingCoordinator changes in a 30k patch but will ping him in the future.