Bug 51728

Summary:

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

Product:

WebKit

Reporter:

Mihai Parparita <mihaip>

Component:

Tools / Tests

Assignee:

Mihai Parparita <mihaip>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

avi, commit-queue, dglazkov, tkent, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Bug Depends on:

Bug Blocks:

51151

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch for landing	none

Mihai Parparita

Reported 2010-12-29 15:55:23 PST

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

Attachments
Patch (10.84 KB, patch) 2010-12-29 15:55 PST, Mihai Parparita	no flags	Details Formatted Diff Diff
Patch (14.35 KB, patch) 2010-12-29 20:00 PST, Mihai Parparita	no flags	Details Formatted Diff Diff
Patch (14.35 KB, patch) 2010-12-30 12:03 PST, Mihai Parparita	no flags	Details Formatted Diff Diff
Patch for landing (14.30 KB, patch) 2010-12-30 19:42 PST, Mihai Parparita	no flags	Details Formatted Diff Diff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Mihai Parparita

Comment 1 2010-12-29 15:55:54 PST

Created attachment 77649 [details] Patch

Mihai Parparita

Comment 2 2010-12-29 20:00:08 PST

Created attachment 77657 [details] Patch

Mihai Parparita

Comment 3 2010-12-29 20:04:50 PST

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

Comment 4 2010-12-29 22:33:33 PST

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

Mihai Parparita

Comment 5 2010-12-30 12:03:46 PST

Created attachment 77693 [details] Patch

Mihai Parparita

Comment 6 2010-12-30 12:04:31 PST

(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 7 2010-12-30 17:47:44 PST

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

Comment 8 2010-12-30 19:42:24 PST

Created attachment 77701 [details] Patch for landing

Mihai Parparita

Comment 9 2010-12-30 19:43:38 PST

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 10 2010-12-30 20:02:50 PST

Comment on attachment 77701 [details] Patch for landing Clearing flags on attachment: 77701 Committed r74821: <http://trac.webkit.org/changeset/74821>

WebKit Commit Bot

Comment 11 2010-12-30 20:02:57 PST

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.