[webkit-reviews] review requested: [Bug 35775] [chromium linux] Improve look of scrollbars : [Attachment 59563] try2

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Jun 23 14:44:21 PDT 2010


Evan Stade <estade at chromium.org> has asked  for review:
Bug 35775: [chromium linux] Improve look of scrollbars
https://bugs.webkit.org/show_bug.cgi?id=35775

Attachment 59563: try2
https://bugs.webkit.org/attachment.cgi?id=59563&action=review

------- Additional Comments from Evan Stade <estade at chromium.org>
(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.


More information about the webkit-reviews mailing list