[Chromium] Plumb Scrollbar button dimensions down to native_theme
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