Bug 84873 - Make ScrollView::scrollSize scrollbar-independent
Summary: Make ScrollView::scrollSize scrollbar-independent
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Antonio Gomes
URL:
Keywords:
Depends on: 85002
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-25 10:21 PDT by Antonio Gomes
Modified: 2012-04-27 18:09 PDT (History)
5 users (show)

See Also:


Attachments
(r115345 + r115401, r=andersca) patch v1 (2.70 KB, patch)
2012-04-25 10:37 PDT, Antonio Gomes
andersca: review+
Details | Formatted Diff | Diff
patch v2 - implementing what Adam questioned (2.68 KB, patch)
2012-04-25 21:09 PDT, Antonio Gomes
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antonio Gomes 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.
Comment 1 Antonio Gomes 2012-04-25 10:37:06 PDT
Created attachment 138840 [details]
(r115345 + r115401, r=andersca) patch v1
Comment 2 Adam Treat 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?
Comment 3 Antonio Gomes 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.
Comment 4 Antonio Gomes 2012-04-25 21:09:44 PDT
Created attachment 138927 [details]
patch v2 - implementing what Adam questioned
Comment 5 Adam Treat 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!
Comment 6 Antonio Gomes 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>
Comment 7 Darin Adler 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.
Comment 8 Antonio Gomes 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?
Comment 9 Darin Adler 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.