[Webkit-unassigned] [Bug 35775] [chromium linux] Improve look of scrollbars

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=35775


Darin Fisher (:fishd, Google) <fishd at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #50076|review?                     |review-
               Flag|                            |




--- Comment #7 from Darin Fisher (:fishd, Google) <fishd at chromium.org>  2010-03-05 14:53:51 PST ---
(From update of attachment 50076)
> 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

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.