Bug 79133 - [chromium] Notify compositor of wheel event registration via ScrollingCoordinator
Summary: [chromium] Notify compositor of wheel event registration via ScrollingCoordin...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: James Robinson
URL:
Keywords:
Depends on:
Blocks: 79155
  Show dependency treegraph
 
Reported: 2012-02-21 11:27 PST by James Robinson
Modified: 2012-02-23 18:54 PST (History)
3 users (show)

See Also:


Attachments
Patch (31.72 KB, patch)
2012-02-21 11:30 PST, James Robinson
no flags Details | Formatted Diff | Diff
Patch (30.92 KB, patch)
2012-02-21 15:11 PST, James Robinson
no flags Details | Formatted Diff | Diff
rebase to ToT (29.64 KB, patch)
2012-02-21 17:14 PST, James Robinson
dglazkov: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description James Robinson 2012-02-21 11:27:19 PST
[chromium] Notify compositor of wheel event registration via ScrollingCoordinator
Comment 1 James Robinson 2012-02-21 11:30:54 PST
Created attachment 128008 [details]
Patch
Comment 2 WebKit Review Bot 2012-02-21 14:40:02 PST
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
Comment 3 James Robinson 2012-02-21 15:11:07 PST
Created attachment 128052 [details]
Patch
Comment 4 James Robinson 2012-02-21 15:12:14 PST
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.
Comment 5 James Robinson 2012-02-21 17:14:26 PST
Created attachment 128089 [details]
rebase to ToT
Comment 6 Adrienne Walker 2012-02-23 14:22:24 PST
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.  :(
Comment 7 Dimitri Glazkov (Google) 2012-02-23 15:55:53 PST
Comment on attachment 128089 [details]
rebase to ToT

looks pretty. Consider using pimpl thing that Eric and Adam did with HTMLTreeBuilder::ExternalCharacterTokenBuffer. ScrollingCoordinator::ForRealz
Comment 8 James Robinson 2012-02-23 16:28:32 PST
(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.
Comment 9 James Robinson 2012-02-23 16:29:33 PST
Committed r108698: <http://trac.webkit.org/changeset/108698>
Comment 10 Sam Weinig 2012-02-23 18:10:51 PST
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.
Comment 11 James Robinson 2012-02-23 18:54:27 PST
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.