Bug 41925

Created attachment 60997 [details] Patch

(In reply to comment #1) > Created an attachment (id=60997) [details] > Patch This patch contains no tests because nothing is drawn without the patch of Bug#41924. If the patch of Bug#41924 is committed before this, I'll add a test to this. If this patch is committed before Bug#41924, I'll add a test separately.

Created attachment 61008 [details] Screenshot

the code looks fine to me (I am not a reviewer though). The screenshots look a little funky imo---I think it's because the up/down arrows are not symmetric (the bottom being slightly larger, especially noticeable because the arrows are not the same distance from the middle line). I'm not sure how to fix that and it's certianly pretty nitpicky. Shrug.

Comment on attachment 60997 [details] Patch LGTM (also not a reviewer), just some nits. WebCore/rendering/RenderThemeChromiumLinux.cpp:187 + SkScalar* hsv, SkScalar saturateAmount, SkScalar brightenAmount) Nit: can we make |hsv| const? WebCore/rendering/RenderThemeChromiumLinux.cpp:196 + SkColor RenderThemeChromiumLinux::outlineColor(SkScalar* hsv1, SkScalar* hsv2) Nit: Can we make these const too?

Created attachment 61204 [details] Patch 2

(In reply to comment #6) > Created an attachment (id=61204) [details] > Patch 2 Changed parameter type: SkScalar* -> const SkScalar[3]

Comment on attachment 61204 [details] Patch 2 WebCore/platform/chromium/ScrollbarThemeChromiumLinux.cpp:90 + paint.setColor(RenderThemeChromiumLinux::saturateAndBrighten(trackHSV, 0, 0)); code in platform/ should not depend on code in rendering/. that's a layering violation. it seems like you should refactor this so that there is another file in platform/chromium/ that defines the shared theme code.

(In reply to comment #8) > (From update of attachment 61204 [details]) > WebCore/platform/chromium/ScrollbarThemeChromiumLinux.cpp:90 > + paint.setColor(RenderThemeChromiumLinux::saturateAndBrighten(trackHSV, 0, 0)); > code in platform/ should not depend on code in rendering/. > that's a layering violation. it seems like you should > refactor this so that there is another file in platform/chromium/ > that defines the shared theme code. I see. Do you think it's acceptable to remain these function in ScrollbarThemeChromiumLinux and call them from RenderThemeChroimiumLinux?

(In reply to comment #9) > Do you think it's acceptable to remain these function in > ScrollbarThemeChromiumLinux and call them from RenderThemeChroimiumLinux? That seems suboptimal since you plan to use them for things that aren't related to scrollbars, right? How about a new file: PlatformThemeChromium{Linux}?

Created attachment 61352 [details] Patch 3

(In reply to comment #10) > > Do you think it's acceptable to remain these function in > > ScrollbarThemeChromiumLinux and call them from RenderThemeChroimiumLinux? > > That seems suboptimal since you plan to use them for things that aren't related to scrollbars, right? Right :-) > How about a new file: PlatformThemeChromium{Linux}? ok, the updated patch introduced PlatformThemeChromiumGtk, and fixes other layer violations.

Comment on attachment 61352 [details] Patch 3 WebCore/platform/chromium/PlatformThemeChromiumGtk.h:52 + enum ArrowDirection { nit: usually ideal style-wise to put enum declarations at the top of the public section with a new line separating it from the methods. WebCore/platform/chromium/PlatformThemeChromiumGtk.h:68 + } } // namespace WebCore WebCore/platform/chromium/PlatformThemeChromiumGtk.cpp:220 + } } // namespace WebCore

Thank you for reviewing. (In reply to comment #13) > (From update of attachment 61352 [details]) > WebCore/platform/chromium/PlatformThemeChromiumGtk.h:52 > + enum ArrowDirection { > nit: usually ideal style-wise to put enum declarations at the top of the > public section with a new line separating it from the methods. Done. > WebCore/platform/chromium/PlatformThemeChromiumGtk.h:68 > + } > } // namespace WebCore Done. > WebCore/platform/chromium/PlatformThemeChromiumGtk.cpp:220 > + } > } // namespace WebCore Done. Landed as r63280.