Bug 35775 – [chromium linux] Improve look of scrollbars

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Evan Stade]

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.

[Evan Stade]

Created attachment 50072 [details]
patch

will post screenshots shortly

[WebKit Review Bot]

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.

[Evan Stade]

Created attachment 50073 [details]
screenshots in default chrome theme + gtk themes

[Evan Stade]

Created attachment 50076 [details]
style fixes

[Eric Seidel (no email)]

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?

[Evan Stade]

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.

[Darin Fisher (:fishd, Google)]

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

[Evan Stade]

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).

[Evan Stade]

sorry, posted this on the wrong thread.

[xiyuan]

*** Bug 40848 has been marked as a duplicate of this bug. ***

[Evan Stade]

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.

[WebKit Review Bot]

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.

[Evan Stade]

Created attachment 59578 [details]
style fixes

[Darin Fisher (:fishd, Google)]

Comment on attachment 59578 [details]
style fixes

r=me

[Adam Barth]

Comment on attachment 59578 [details]
style fixes

Should we land this patch?

[Evan Stade]

patch was landed in r61908