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

[bugzilla-daemon at webkit.org]

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


Evan Stade <estade at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #50076|0                           |1
        is obsolete|                            |
  Attachment #59563|                            |review?
               Flag|                            |




--- Comment #11 from Evan Stade <estade at chromium.org>  2010-06-23 14:44:21 PST ---
Created an attachment (id=59563)
 --> (https://bugs.webkit.org/attachment.cgi?id=59563)
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.

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