Bug 51728 – [Chromium] Add WebThemeEngineDRTMac so that Chromium DRT scrollbar rendering can match the Mac port's

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Mihai Parparita]

[Chromium] Add WebThemeEngineDRTMac so that Chromium DRT scrollbar rendering can match the Mac port's

[Mihai Parparita]

Created attachment 77649 [details]
Patch

[Mihai Parparita]

Created attachment 77657 [details]
Patch

[Mihai Parparita]

This depends on the changes in http://codereview.chromium.org/6090002/. I'll need to wait for that to be checked in and then adjust the DEPS roll to at least that revision.

[WebKit Review Bot]

Attachment 77657 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7318277

[Mihai Parparita]

Created attachment 77693 [details]
Patch

[Mihai Parparita]

(In reply to comment #4)
> Attachment 77657 [details] did not build on chromium:
> Build output: http://queues.webkit.org/results/7318277

The Chromium changes that this depends on landed as http://crrev.com/70319, so I updated the DEPS roll to that. The patch should build now.

[Kent Tamura]

Comment on attachment 77693 [details]
Patch

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.

[Mihai Parparita]

Created attachment 77701 [details]
Patch for landing

[Mihai Parparita]

Thanks for the review, I've fixed all the style issues (I'm still not used to switching between Chromium and WebKit styles when making double-sided changes).

[WebKit Commit Bot]

Comment on attachment 77701 [details]
Patch for landing

Clearing flags on attachment: 77701

Committed r74821: <http://trac.webkit.org/changeset/74821>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.