Bug 89264 - Plumb Scrollbar button dimensions down to WebThemeEngine
Summary: Plumb Scrollbar button dimensions down to WebThemeEngine
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Scott Graham
URL:
Keywords:
Depends on: 89934
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-15 19:05 PDT by Scott Graham
Modified: 2012-06-25 18:49 PDT (History)
6 users (show)

See Also:


Attachments
Patch (6.51 KB, patch)
2012-06-18 12:36 PDT, Scott Graham
no flags Details | Formatted Diff | Diff
Patch (6.46 KB, patch)
2012-06-18 12:43 PDT, Scott Graham
no flags Details | Formatted Diff | Diff
Patch (6.32 KB, patch)
2012-06-18 13:25 PDT, Scott Graham
no flags Details | Formatted Diff | Diff
Patch (6.24 KB, patch)
2012-06-18 13:56 PDT, Scott Graham
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Scott Graham 2012-06-15 19:05:14 PDT
[Chromium] Plumb Scrollbar button dimensions down to native_theme
Comment 1 Scott Graham 2012-06-18 12:36:09 PDT
Created attachment 148149 [details]
Patch
Comment 2 WebKit Review Bot 2012-06-18 12:41:02 PDT
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 3 Scott Graham 2012-06-18 12:43:18 PDT
Created attachment 148151 [details]
Patch
Comment 4 Scott Graham 2012-06-18 12:43:53 PDT
(Requires https://chromiumcodereview.appspot.com/10532207/ to land on Chromium side first.)
Comment 5 James Robinson 2012-06-18 12:47:29 PDT
Comment on attachment 148149 [details]
Patch

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

> Source/Platform/chromium/public/win/WebThemeEngine.h:85
> +    virtual int getScrollbarArrowSize(int partId) = 0;

the rest of this file seems to juse use "int part", not partId.

is there a particular reason you want this to work only for the scrollbar arrow part IDs? android and linux let you ask for the size of any part

> Source/WebCore/platform/chromium/ScrollbarThemeChromiumWin.cpp:254
> +    bool horz = scrollbar->orientation() == HorizontalScrollbar;
> +    int standardGirth = PlatformSupport::getScrollbarArrowSize(horz ? DFCS_SCROLLLEFT : DFCS_SCROLLUP);

WebKit style is not to abbreviate variable names (and there's no column limit), so I'd expect "horz" into "horizontal"
Comment 6 James Robinson 2012-06-18 12:47:42 PDT
Comment on attachment 148149 [details]
Patch

stupid bugzilla
Comment 7 Scott Graham 2012-06-18 13:25:42 PDT
Created attachment 148159 [details]
Patch
Comment 8 Scott Graham 2012-06-18 13:26:56 PDT
(In reply to comment #5)
> (From update of attachment 148149 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=148149&action=review
> 
> > Source/Platform/chromium/public/win/WebThemeEngine.h:85
> > +    virtual int getScrollbarArrowSize(int partId) = 0;
> 
> the rest of this file seems to juse use "int part", not partId.

Done.

> is there a particular reason you want this to work only for the scrollbar arrow part IDs? android and linux let you ask for the size of any part

Done.

> 
> > Source/WebCore/platform/chromium/ScrollbarThemeChromiumWin.cpp:254
> > +    bool horz = scrollbar->orientation() == HorizontalScrollbar;
> > +    int standardGirth = PlatformSupport::getScrollbarArrowSize(horz ? DFCS_SCROLLLEFT : DFCS_SCROLLUP);
> 
> WebKit style is not to abbreviate variable names (and there's no column limit), so I'd expect "horz" into "horizontal"

Copied from above.

I realized I was using the wrong defines anyway per http://msdn.microsoft.com/en-us/library/windows/desktop/bb773210(v=vs.85).aspx so there's no distinction between horizontal/vertical anyway.
Comment 9 James Robinson 2012-06-18 13:34:32 PDT
Comment on attachment 148159 [details]
Patch

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

> Source/Platform/chromium/public/win/WebThemeEngine.h:85
> +    virtual int getSize(int part) = 0;

Add a comment saying what the size means and what parts it can be queried for
Comment 10 Scott Graham 2012-06-18 13:56:07 PDT
Created attachment 148166 [details]
Patch
Comment 11 Scott Graham 2012-06-18 13:56:47 PDT
(In reply to comment #9)
> (From update of attachment 148159 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=148159&action=review
> 
> > Source/Platform/chromium/public/win/WebThemeEngine.h:85
> > +    virtual int getSize(int part) = 0;
> 
> Add a comment saying what the size means and what parts it can be queried for

Done. Also made it a WebSize/IntSize because on writing the comment realized it made no sense to be an int.
Comment 12 James Robinson 2012-06-18 16:20:40 PDT
Comment on attachment 148166 [details]
Patch

R=me. Guessing we want to hold off on commit-queue until the chromium side lands, right?
Comment 13 Scott Graham 2012-06-18 17:37:15 PDT
(In reply to comment #12)
> (From update of attachment 148166 [details])
> R=me. Guessing we want to hold off on commit-queue until the chromium side lands, right?

Thanks. Yes, I'll ping you when this is safe to cq+.
Comment 14 James Robinson 2012-06-19 14:00:17 PDT
What chromium rev had the change?  Is it rolled into Source/WebKit/chromium/DEPS in the WebKit repo yet? (I'm cautious since we don't have any cr-win EWS coverage).
Comment 15 Scott Graham 2012-06-19 15:06:32 PDT
(In reply to comment #14)
> What chromium rev had the change?  Is it rolled into Source/WebKit/chromium/DEPS in the WebKit repo yet? (I'm cautious since we don't have any cr-win EWS coverage).

Shoot, you're right. I always forget about the backwards roll.

ftr, http://crrev.com/142993
Comment 16 James Robinson 2012-06-19 17:00:11 PDT
Comment on attachment 148166 [details]
Patch

CQ minus since http://trac.webkit.org/browser/trunk/Source/WebKit/chromium/DEPS shows me 142842. Set cq? when it's ahead.
Comment 17 Scott Graham 2012-06-25 10:11:17 PDT
Comment on attachment 148166 [details]
Patch

143776 now
Comment 18 WebKit Review Bot 2012-06-25 12:48:12 PDT
Comment on attachment 148166 [details]
Patch

Clearing flags on attachment: 148166

Committed r121176: <http://trac.webkit.org/changeset/121176>
Comment 19 WebKit Review Bot 2012-06-25 12:48:21 PDT
All reviewed patches have been landed.  Closing bug.
Comment 20 WebKit Review Bot 2012-06-25 18:49:29 PDT
Re-opened since this is blocked by 89934