scrollbars need better default coloring to match the stock chrome theme, need arrows at top and bottom of the track, and need better contrast between thumb and track.
Created attachment 50072 [details] patch will post screenshots shortly
Attachment 50072 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 WebCore/platform/chromium/ScrollbarThemeChromiumLinux.cpp:182: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] WebCore/platform/chromium/ScrollbarThemeChromiumLinux.cpp:182: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] WebCore/platform/chromium/ScrollbarThemeChromiumLinux.cpp:187: track_hsv is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] WebCore/platform/chromium/ScrollbarThemeChromiumLinux.cpp:189: button_color is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] WebCore/platform/chromium/ScrollbarThemeChromiumLinux.cpp:190: background_color is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] WebCore/platform/chromium/ScrollbarThemeChromiumLinux.cpp:192: button_hsv is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] WebCore/platform/chromium/ScrollbarThemeChromiumLinux.cpp:196: button_hsv is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] WebCore/platform/chromium/ScrollbarThemeChromiumLinux.cpp:251: thumb_hsv is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Total errors found: 8 in 6 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 50073 [details] screenshots in default chrome theme + gtk themes
Created attachment 50076 [details] style fixes
Comment on attachment 50076 [details] style fixes I don't like the changes to Scrollbar.cpp. Is ChromiumLinux the only platform that would need this change? Are we sure that that's the right place to put at change?
Other platforms might need/want that change as well, in particular gtk. I am pretty sure that's the right place to put the change though.
Comment on attachment 50076 [details] style fixes > Index: WebCore/platform/Scrollbar.cpp ... > void Scrollbar::updateThumbPosition() > { > +#if PLATFORM(CHROMIUM) && OS(LINUX) > + invalidate(); > +#else > theme()->invalidateParts(this, ForwardTrackPart | BackTrackPart | ThumbPart); > +#endif > } > > void Scrollbar::updateThumbProportion() > { > +#if PLATFORM(CHROMIUM) && OS(LINUX) > + invalidate(); > +#else > theme()->invalidateParts(this, ForwardTrackPart | BackTrackPart | ThumbPart); > +#endif ^^^ this forking is unfortunate. it would be nice if your comment from the ChangeLog appeared in the code. also, perhaps it would be good to write a helper function: void Scrollbar::invalidateForThumbChange() { #if PLATFORM(CHROMIUM) && OS(LINUX) // Comments... invalidate(); #else theme()->invalidateParts(this, ForwardTrackPart | BackTrackPart | ThumbPart); #endif } Another approach would be to define a macro at the top of this file like: THUMB_POSITION_EFFECTS_BUTTONS Then, you can just #ifdef the code w/ that, and place the comment where the macro is defined. This might be the best solution. > Index: WebCore/platform/chromium/ScrollbarThemeChromiumLinux.cpp > =================================================================== > --- WebCore/platform/chromium/ScrollbarThemeChromiumLinux.cpp (revision 55563) > +++ WebCore/platform/chromium/ScrollbarThemeChromiumLinux.cpp (working copy) > @@ -40,6 +40,9 @@ > > namespace WebCore { > > +static const int kScrollbarThickness = 15; > +static const int kButtonLength = 14; file statics should be named like variables. scrollbarThickness and buttonLength. > + // Calculate button color. > + SkScalar trackHSV[3]; > + SkColorToHSV(RenderThemeChromiumLinux::trackColor(), trackHSV); The platform/ layer should not depend on the rendering/ layer. Can you move this trackColor into the platform/ layer somehow? maybe a file in platform/chromium/ or platform/graphics/chromium/? > + // If the button is disabled, the arrow is drawn with the outline color. > + if (enabled) > + paint.setColor(SK_ColorBLACK); nit: indentation > IntSize ScrollbarThemeChromiumLinux::buttonSize(Scrollbar* scrollbar) > { > - // On Linux, we don't use buttons > - return IntSize(0, 0); > + if (scrollbar->orientation() == VerticalScrollbar) > + return IntSize(kScrollbarThickness, kButtonLength); > + else > + return IntSize(kButtonLength, kScrollbarThickness); nit: indentation
see also <https://bugs.webkit.org/show_bug.cgi?id=35775>. I didn't really intend to let that patch die, but I moved on to other things and have been delinquent in getting back to it. As long as we are adding the steppers, we should fix http://code.google.com/p/chromium/issues/detail?id=36919 as well (due to the difficulty of layout tests).
sorry, posted this on the wrong thread.
*** Bug 40848 has been marked as a duplicate of this bug. ***
Created attachment 59563 [details] try2 (In reply to comment #7) > (From update of attachment 50076 [details]) > > Index: WebCore/platform/Scrollbar.cpp > ... > > void Scrollbar::updateThumbPosition() > > { > > +#if PLATFORM(CHROMIUM) && OS(LINUX) > > + invalidate(); > > +#else > > theme()->invalidateParts(this, ForwardTrackPart | BackTrackPart | ThumbPart); > > +#endif > > } > > > > void Scrollbar::updateThumbProportion() > > { > > +#if PLATFORM(CHROMIUM) && OS(LINUX) > > + invalidate(); > > +#else > > theme()->invalidateParts(this, ForwardTrackPart | BackTrackPart | ThumbPart); > > +#endif > > ^^^ this forking is unfortunate. it would be nice if your comment > from the ChangeLog appeared in the code. also, perhaps it would be > good to write a helper function: > > void Scrollbar::invalidateForThumbChange() { > #if PLATFORM(CHROMIUM) && OS(LINUX) > // Comments... > invalidate(); > #else > theme()->invalidateParts(this, ForwardTrackPart | BackTrackPart | ThumbPart); > #endif > } > > Another approach would be to define a macro at the top of this file like: > > THUMB_POSITION_EFFECTS_BUTTONS > > Then, you can just #ifdef the code w/ that, and place the comment where the > macro is defined. This might be the best solution. Done. > > > > Index: WebCore/platform/chromium/ScrollbarThemeChromiumLinux.cpp > > =================================================================== > > --- WebCore/platform/chromium/ScrollbarThemeChromiumLinux.cpp (revision 55563) > > +++ WebCore/platform/chromium/ScrollbarThemeChromiumLinux.cpp (working copy) > > @@ -40,6 +40,9 @@ > > > > namespace WebCore { > > > > +static const int kScrollbarThickness = 15; > > +static const int kButtonLength = 14; > > file statics should be named like variables. scrollbarThickness and buttonLength. Done. > > > > + // Calculate button color. > > + SkScalar trackHSV[3]; > > + SkColorToHSV(RenderThemeChromiumLinux::trackColor(), trackHSV); > > The platform/ layer should not depend on the rendering/ layer. Can > you move this trackColor into the platform/ layer somehow? maybe a > file in platform/chromium/ or platform/graphics/chromium/? really? it seems a lot of files in the platform layer do this: WebKit/WebCore/platform$ ack "RenderTheme\.h" win/PopupMenuWin.cpp 37:#include "RenderTheme.h" haiku/RenderThemeHaiku.h 28:#include "RenderTheme.h" gtk/RenderThemeGtk.h 32:#include "RenderTheme.h" efl/RenderThemeEfl.h 33:#include "RenderTheme.h" chromium/ScrollbarThemeChromiumLinux.cpp 36:#include "RenderTheme.h" chromium/PopupMenuChromium.cpp 52:#include "RenderTheme.h" qt/TemporaryLinkStubsQt.cpp 59:#include "RenderTheme.h" qt/RenderThemeQt.h 25:#include "RenderTheme.h" qt/RenderThemeQt.cpp 57:#include "RenderTheme.h" wx/RenderThemeWx.cpp 27:#include "RenderTheme.h" android/RenderThemeAndroid.h 29:#include "RenderTheme.h" > > > > + // If the button is disabled, the arrow is drawn with the outline color. > > + if (enabled) > > + paint.setColor(SK_ColorBLACK); > > nit: indentation Done. > > > > IntSize ScrollbarThemeChromiumLinux::buttonSize(Scrollbar* scrollbar) > > { > > - // On Linux, we don't use buttons > > - return IntSize(0, 0); > > + if (scrollbar->orientation() == VerticalScrollbar) > > + return IntSize(kScrollbarThickness, kButtonLength); > > + else > > + return IntSize(kButtonLength, kScrollbarThickness); > > nit: indentation Done.
Attachment 59563 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1 WebCore/platform/chromium/ScrollbarThemeChromiumLinux.cpp:354: An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4] Total errors found: 1 in 6 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 59578 [details] style fixes
Comment on attachment 59578 [details] style fixes r=me
Comment on attachment 59578 [details] style fixes Should we land this patch?
patch was landed in r61908