[webkit-reviews] review denied: [Bug 35775] [chromium linux] Improve look of scrollbars : [Attachment 50076] style fixes

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied Evan Stade
<estade at chromium.org>'s request for review:
Bug 35775: [chromium linux] Improve look of scrollbars
https://bugs.webkit.org/show_bug.cgi?id=35775

Attachment 50076: style fixes
https://bugs.webkit.org/attachment.cgi?id=50076&action=review

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
> 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