Bug 81247 – Fix crbug 117957 - get settings scrolling with the wheel again.

• Component: New Bugs
• Status: RESOLVED FIXED

[Scott Byer]

Fix crbug 117957 - get settings scrolling with the wheel again.

[Scott Byer]

Created attachment 132087 [details]
Patch

[James Robinson]

Comment on attachment 132087 [details]
Patch

Should this be iterating through ScrollableAreas or FrameViews? Do we have non-FrameView users of this?  Something about this doesn't seem quite right.

[Scott Byer]

Created attachment 132115 [details]
Patch

James, something more like this? This also fixes crbug.com/118125. However, I'm also thinking that it might be better to revert the original rAF patch and add this to that along with a couple of layout tests to cover the two bugs (something I don't think I could get done today, and this is blocking other work). Thoughts?

[James Robinson]

Comment on attachment 132115 [details]
Patch

FrameView::serviceScriptedAnimations iterates through all child Frames, so I was hoping this could piggyback on a similar iteration.  I think actually your earlier patch would be better than iterating through Widgets, sorry for the confusion. I'll R+ that patch.

Reverting is also a find option - we should figure out if we can cover up the gap in test coverage to avoid doing this again.

[James Robinson]

Comment on attachment 132087 [details]
Patch

I misread this at first and thought it would call service..() multiple times on subframes, but I see now it's directly calling serviceScrollAnimations() on the scrollable areas.

I think this is OK.  I think it might be better to iterate through the Frame tree to just hit FrameViews.  In any case, I think it's really important to get better test coverage.

I'm fine with landing this patch as-is, or reverting the other patch so we can shore it up a bit more.  Your call.

[Scott Byer]

Patch 1 doesn't fix the entire problem - it takes care of crbug.com/117957 but not crbug.com/118125. I'll follow this up with

[James Robinson]

Ah, I didn't see the other one.  Messy :/

Something lke your patch 2 is good but it should be using the Frame tree and then grabbing their views - see how serviceScriptedAnimations hits all frames.

[Scott Byer]

Created attachment 132122 [details]
Patch

Ah, here we go. Much cleaner, still gets both bugs.

[James Robinson]

Comment on attachment 132122 [details]
Patch

Cool! I feel a bit more comfortable about this one without additional testing since we have fairly good coverage for the other caller in this branch.

[James Robinson]

Comment on attachment 132122 [details]
Patch

Clearing flags on attachment: 132122

Committed r110889: <http://trac.webkit.org/changeset/110889>

[James Robinson]

All reviewed patches have been landed.  Closing bug.