[webkit-reviews] review granted: [Bug 51728] [Chromium] Add WebThemeEngineDRTMac so that Chromium DRT scrollbar rendering can match the Mac port's : [Attachment 77693] Patch

[bugzilla-daemon at webkit.org]

Kent Tamura <tkent at chromium.org> has granted Mihai Parparita
<mihaip at chromium.org>'s request for review:
Bug 51728: [Chromium] Add WebThemeEngineDRTMac so that Chromium DRT scrollbar
rendering can match the Mac port's
https://bugs.webkit.org/show_bug.cgi?id=51728

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

------- Additional Comments from Kent Tamura <tkent at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=77693&action=review

r+ though there are some style errors.

> Tools/DumpRenderTree/chromium/TestShellMac.mm:38
> +// Theme engine

This comment doesn't add any useful information.  Please remove it.

> Tools/DumpRenderTree/chromium/TestShellMac.mm:133
> +    // Set theme engine.

ditto.

> Tools/DumpRenderTree/chromium/WebThemeEngineDRTMac.h:43
> +private:

Please add a blank line before private:.

> Tools/DumpRenderTree/chromium/WebThemeEngineDRTMac.mm:67
> +    if (alwaysActiveWindow == nil) {
> +	   alwaysActiveWindow = [[self alloc] initWithActiveControls:YES];
> +    }

{} is not needed.

> Tools/DumpRenderTree/chromium/WebThemeEngineDRTMac.mm:75
> +    if (alwaysInactiveWindow == nil) {
> +	   alwaysInactiveWindow = [[self alloc] initWithActiveControls:NO];
> +    }

ditto.

> Tools/DumpRenderTree/chromium/WebThemeEngineDRTMac.mm:86
> +- (BOOL)_hasActiveControls {

"{" should be moved to a new line.

> Tools/DumpRenderTree/chromium/WebThemeEngineDRTMac.mm:97
> +    const WebThemeEngine::ScrollbarInfo& scrollbarInfo) {

ditto.

> Tools/DumpRenderTree/chromium/WebThemeEngineDRTMac.mm:105
> +static ThemeTrackEnableState stateToHIEnableState(WebThemeEngine::State
state) {

ditto.

> Tools/DumpRenderTree/chromium/WebThemeEngineDRTMac.mm:113
> +  switch (state) {
> +    case WebThemeEngine::StateDisabled:
> +	 return kThemeTrackDisabled;
> +    case WebThemeEngine::StateInactive:
> +	 return kThemeTrackInactive;
> +    default:
> +	 return kThemeTrackActive;
> +  }

indentation error.

> Tools/DumpRenderTree/chromium/WebThemeEngineDRTMac.mm:123
> +    const WebThemeEngine::ScrollbarInfo& scrollbarInfo) {

"{" should be moved to a new line.

> Tools/DumpRenderTree/chromium/WebThemeEngineDRTMac.mm:149
> +    const WebThemeEngine::ScrollbarInfo& scrollbarInfo) {

"{" should be moved to a new line.

> Tools/DumpRenderTree/chromium/WebThemeEngineDRTMac.mm:163
> +    double value =
double(scrollbarInfo.currentValue)/double(scrollbarInfo.maxValue);
> +    [scroller setDoubleValue: value];
> +
> +    float knobProportion =
float(scrollbarInfo.visibleSize)/float(scrollbarInfo.totalSize);
> +    [scroller setKnobProportion: knobProportion];

Need spaces around "/" operators.