RESOLVED FIXED Bug 81247
Fix crbug 117957 - get settings scrolling with the wheel again. 
https://bugs.webkit.org/show_bug.cgi?id=81247
Summary Fix crbug 117957 - get settings scrolling with the wheel again.
Scott Byer
Reported 2012-03-15 11:34:58 PDT
Fix crbug 117957 - get settings scrolling with the wheel again.
Attachments
Patch (1.47 KB, patch)
2012-03-15 11:37 PDT, Scott Byer 		no flags
DetailsFormatted DiffDiff
Patch (2.23 KB, patch)
2012-03-15 13:52 PDT, Scott Byer 		no flags
DetailsFormatted DiffDiff
Patch (1.60 KB, patch)
2012-03-15 14:30 PDT, Scott Byer 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Scott Byer
Comment 1 2012-03-15 11:37:46 PDT
Created attachment 132087 [details] Patch
James Robinson
Comment 2 2012-03-15 11:48:59 PDT
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
Comment 3 2012-03-15 13:52:42 PDT
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 4 2012-03-15 14:03:10 PDT
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 5 2012-03-15 14:05:07 PDT
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
Comment 6 2012-03-15 14:06:08 PDT
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
Comment 7 2012-03-15 14:21:17 PDT
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
Comment 8 2012-03-15 14:30:42 PDT
Created attachment 132122 [details] Patch Ah, here we go. Much cleaner, still gets both bugs.
James Robinson
Comment 9 2012-03-15 14:37:15 PDT
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 10 2012-03-15 14:39:49 PDT
Comment on attachment 132122 [details] Patch Clearing flags on attachment: 132122 Committed r110889: <http://trac.webkit.org/changeset/110889>
James Robinson
Comment 11 2012-03-15 14:40:14 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.