Summary: | Plumb Scrollbar button dimensions down to WebThemeEngine | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Scott Graham <scottmg> | ||||||||||
Component: | New Bugs | Assignee: | Scott Graham <scottmg> | ||||||||||
Status: | REOPENED --- | ||||||||||||
Severity: | Normal | CC: | abarth, dglazkov, fishd, jamesr, tkent+wkapi, webkit.review.bot | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Bug Depends on: | 89934 | ||||||||||||
Bug Blocks: | |||||||||||||
Attachments: |
|
Description
Scott Graham
2012-06-15 19:05:14 PDT
Created attachment 148149 [details]
Patch
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. Created attachment 148151 [details]
Patch
(Requires https://chromiumcodereview.appspot.com/10532207/ to land on Chromium side first.) 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 on attachment 148149 [details]
Patch
stupid bugzilla
Created attachment 148159 [details]
Patch
(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 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 Created attachment 148166 [details]
Patch
(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 on attachment 148166 [details]
Patch
R=me. Guessing we want to hold off on commit-queue until the chromium side lands, right?
(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+. 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). (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 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 on attachment 148166 [details]
Patch
143776 now
Comment on attachment 148166 [details] Patch Clearing flags on attachment: 148166 Committed r121176: <http://trac.webkit.org/changeset/121176> All reviewed patches have been landed. Closing bug. Re-opened since this is blocked by 89934 |