[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