| Summary: | Fix crbug 117957 - get settings scrolling with the wheel again. | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Scott Byer <scottbyer> | ||||||||
| Component: | New Bugs | Assignee: | Scott Byer <scottbyer> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | jamesr, rjkroege, wjmaclean | ||||||||
| Priority: | P2 | ||||||||||
| Version: | 528+ (Nightly build) | ||||||||||
| Hardware: | Unspecified | ||||||||||
| OS: | Unspecified | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Scott Byer
2012-03-15 11:34:58 PDT
Created attachment 132087 [details]
Patch
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.
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?
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.
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.
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 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. Created attachment 132122 [details]
Patch
Ah, here we go. Much cleaner, still gets both bugs.
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.
Comment on attachment 132122 [details] Patch Clearing flags on attachment: 132122 Committed r110889: <http://trac.webkit.org/changeset/110889> All reviewed patches have been landed. Closing bug. |