Bug 91674 - [chromium] Implement scrollbar theme for Android
Summary: [chromium] Implement scrollbar theme for Android
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tien-Ren Chen
URL:
Keywords:
Depends on:
Blocks: 66687
  Show dependency treegraph
 
Reported: 2012-07-18 14:48 PDT by Tien-Ren Chen
Modified: 2012-07-23 16:39 PDT (History)
6 users (show)

See Also:


Attachments
Patch (8.79 KB, patch)
2012-07-18 15:28 PDT, Tien-Ren Chen
no flags Details | Formatted Diff | Diff
Patch for landing (8.71 KB, patch)
2012-07-23 15:50 PDT, Adam Barth
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tien-Ren Chen 2012-07-18 14:48:09 PDT
[chromium] Implement scrollbar theme for Android
Comment 1 Tien-Ren Chen 2012-07-18 15:28:22 PDT
Created attachment 153103 [details]
Patch
Comment 2 Peter Beverloo 2012-07-19 02:02:15 PDT
Please be sure that your patches block bug 66687, that helps us to keep track. Thanks!
Comment 3 Adam Barth 2012-07-19 09:08:44 PDT
Comment on attachment 153103 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=153103&action=review

Some minor style nits below.

> Source/WebCore/platform/chromium/ScrollbarThemeChromiumAndroid.cpp:66
> +    float proportion = (float)scrollbar->currentPos() / scrollbar->totalSize();

WebKit preferes C++ style casts, so static_cast<float>(...)

> Source/WebCore/platform/chromium/ScrollbarThemeChromiumAndroid.cpp:103
> +namespace {

WebKit tends to use static functions rather than anonymous namespaces.

> Source/WebCore/platform/chromium/ScrollbarThemeChromiumAndroid.h:53
> +    static const int s_scrollbarWidth = 8;
> +    static const int s_scrollbarMargin = 5;

WebKit doesn't use the s_ prefix for static variables.  We just use bare names like scrollbarWidth (and we tend to put them in the implementation file rather than the header if they're not used outside the implementation).
Comment 4 Adam Barth 2012-07-23 15:50:27 PDT
Created attachment 153885 [details]
Patch for landing
Comment 5 Adam Barth 2012-07-23 15:50:57 PDT
@trchen: I fixed the nits for you.  Please let me know if I've goofed them up.  Thanks for the patch.
Comment 6 WebKit Review Bot 2012-07-23 16:39:13 PDT
Comment on attachment 153885 [details]
Patch for landing

Clearing flags on attachment: 153885

Committed r123402: <http://trac.webkit.org/changeset/123402>
Comment 7 WebKit Review Bot 2012-07-23 16:39:17 PDT
All reviewed patches have been landed.  Closing bug.