[webkit-reviews] review denied: [Bug 47473] Use new WebThemeEngine api on chromium / linux to draw scrollbars. : [Attachment 70410] Patch

[bugzilla-daemon at webkit.org]

Tony Chang <tony at chromium.org> has denied Dave Moore <davemoore at google.com>'s
request for review:
Bug 47473: Use new WebThemeEngine api on chromium / linux to draw scrollbars.
https://bugs.webkit.org/show_bug.cgi?id=47473

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

------- Additional Comments from Tony Chang <tony at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=70410&action=review

Overall, looks ok.  Mostly naming and style nits.

> WebCore/platform/chromium/ChromiumBridge.h:259
> +	   enum Part {

Can we pick a more descriptive name than Part?	ChromiumBridge has a lot of
stuff in it so it's not obvious what WebCore::ChromiumBridge::Part is.	Maybe
ThemeScrollbarPart?

> WebCore/platform/chromium/ChromiumBridge.h:271
> +	   enum State {

Same thing, maybe name this ThemePaintState or something.

> WebCore/platform/chromium/ChromiumBridge.h:286
> +	   union ExtraParams {

Maybe ThemePaintExtraParams?

> WebCore/platform/chromium/ChromiumBridge.h:293
> +	   static IntSize getSize(Part);

getThemePartSize?

> WebCore/platform/chromium/ChromiumBridge.h:295
> +	   static void paint(GraphicsContext*, Part, State, const IntRect&,
const ExtraParams*);

paintThemePart?

> WebCore/platform/chromium/ScrollbarThemeChromiumLinux.cpp:56
> +    ChromiumBridge::State state =
> +	   scrollbar->hoveredPart() == partType ? ChromiumBridge::StateHover :
ChromiumBridge::StateNormal;

Nit: I would just put this on a single line.

> WebCore/platform/chromium/ScrollbarThemeChromiumLinux.cpp:67
> +	   scrollbar->orientation() == HorizontalScrollbar ?
> +	       ChromiumBridge::PartScrollbarHoriztonalTrack :
> +	       ChromiumBridge::PartScrollbarVerticalTrack,

Nit: single line

> WebCore/platform/chromium/ScrollbarThemeChromiumLinux.cpp:79
>      if (scrollbar->orientation() == HorizontalScrollbar) {

FWIW, checking the orientation doesn't seem to matter here.  Would be nice to
remove this check if we can.

> WebCore/platform/chromium/ScrollbarThemeChromiumLinux.cpp:97
> +    if (checkMin && (scrollbar->currentPos() <= 0)
> +	   || (checkMax && scrollbar->currentPos() == scrollbar->maximum())) {

I think you need another pair of () here or the
  checkMin && () || ()
will cause a gcc warning.

> WebCore/platform/chromium/ScrollbarThemeChromiumLinux.cpp:122
> +	   scrollbar->orientation() == HorizontalScrollbar ?
> +	       ChromiumBridge::PartScrollbarHorizontalThumb :
> +	       ChromiumBridge::PartScrollbarVerticalThumb,

nit: single line

> WebCore/platform/chromium/ScrollbarThemeChromiumLinux.cpp:-196
> -    if (scrollbar->orientation() == VerticalScrollbar)
> -	   return IntSize(scrollbarThicknessValue, scrollbar->height() < 2 *
buttonLength ? scrollbar->height() / 2 : buttonLength);
> -
> -    // HorizontalScrollbar
> -    return IntSize(scrollbar->width() < 2 * buttonLength ?
scrollbar->width() / 2 : buttonLength, scrollbarThicknessValue);

This is relatively new code.  It looks like the code on the chrome side doesn't
include the 2x check.

> WebKit/chromium/src/ChromiumBridge.cpp:756
> +    default: return WebThemeEngine::PartScrollbarDownArrow;

Please remove the default: and just enumerate all the cases.  It avoids errors
when new values are added.

> WebKit/chromium/src/ChromiumBridge.cpp:767
> +    default: return WebThemeEngine::StateDisabled;

no default