Bug 84873

Summary: Make ScrollView::scrollSize scrollbar-independent
Product: WebKit Reporter: Antonio Gomes <tonikitoo>
Component: PlatformAssignee: Antonio Gomes <tonikitoo>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, darin, jamesr, manyoso, simon.fraser
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 85002    
Bug Blocks:    
Attachments:
Description Flags
(r115345 + r115401, r=andersca) patch v1
andersca: review+
patch v2 - implementing what Adam questioned none

Antonio Gomes
Reported 2012-04-25 10:21:53 PDT
This method is so far tied to the existence of scrollbars to identify a scrollview as scrollable. for ports that disable scrollbars at FrameView creation it makes it impossible to use any ScrollAnimatorXXX method.
Attachments
(r115345 + r115401, r=andersca) patch v1 (2.70 KB, patch)
2012-04-25 10:37 PDT, Antonio Gomes
andersca: review+
patch v2 - implementing what Adam questioned (2.68 KB, patch)
2012-04-25 21:09 PDT, Antonio Gomes
no flags
Antonio Gomes
Comment 1 2012-04-25 10:37:06 PDT
Created attachment 138840 [details] (r115345 + r115401, r=andersca) patch v1
Adam Treat
Comment 2 2012-04-25 13:52:33 PDT
Why have the conditional at all? Why not just calculate in your fashion irregardless of whether there are scrollbars or not?
Antonio Gomes
Comment 3 2012-04-25 14:00:30 PDT
(In reply to comment #2) > Why have the conditional at all? Why not just calculate in your fashion irregardless of whether there are scrollbars or not? Fair enough.
Antonio Gomes
Comment 4 2012-04-25 21:09:44 PDT
Created attachment 138927 [details] patch v2 - implementing what Adam questioned
Adam Treat
Comment 5 2012-04-26 09:51:53 PDT
(In reply to comment #4) > Created an attachment (id=138927) [details] > patch v2 - implementing what Adam questioned LGTM, but since I haven't touched this code in a long time (at least not in upstream repo) I think someone else should review too. Thanks Antonio!
Antonio Gomes
Comment 6 2012-04-26 13:31:15 PDT
Comment on attachment 138840 [details] (r115345 + r115401, r=andersca) patch v1 Committed: <http://trac.webkit.org/changeset/115345>
Darin Adler
Comment 7 2012-04-27 10:50:01 PDT
It seems that http://trac.webkit.org/changeset/115401 went in without any comments here explaining why it’s needed.
Antonio Gomes
Comment 8 2012-04-27 11:20:54 PDT
Ooh my :( I did a bad job at first (pushed the wrong patch) and at second (amended the proper patch as a diff against what was committed originally) without a changelog entry :/ Darin, want me to revert both, and push it properly?
Darin Adler
Comment 9 2012-04-27 18:09:49 PDT
I’m not sure there is any significant benefit to trying to fix this now; I just would have liked to have a comment here in the bug explaining what happened.
Note You need to log in before you can comment on or make changes to this bug.