Bug 38077 – [chromium] Prepare to making WebThemeEngine cross-platform

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[John Abd-El-Malek]

[chromium] Prepare to making WebThemeEngine cross-platform

[John Abd-El-Malek]

Created attachment 54211 [details]
Proposed patch

[John Abd-El-Malek]

This just moves the WebThemeEngine file so that I can update the Chromium code with the new locations.

[Adam Barth]

Comment on attachment 54211 [details]
Proposed patch

We need fishd to review this patch, but seems reasonable to me.

[Darin Fisher (:fishd, Google)]

Comment on attachment 54211 [details]
Proposed patch

WebKit/chromium/public/WebThemeEngine.h:107
 +      // TODO(jam): make these pure virtual once this is rolled into Chromium and
TODO(jam) should be FIXME in WebKit style.  also, it is actually preferred to just provide default implementations for methods that are implemented by the embedder.  that way it is easier to change the interface.  so, i would just delete this TODO :)

WebKit/chromium/public/WebThemeEngine.h:79
 +      enum ThemePart {
can we do without the Theme prefix here?  these are already scoped by WebThemeEngine which has the word Theme in it.  WebThemeEngine::Part* and WebThemeEngine::State* seems sufficient to me.

WebKit/chromium/public/WebThemeEngine.h:109
 +      virtual void getSize(ThemePart, ThemeState, WebRect*) {}
what is the purpose of the getSize function?

WebKit/chromium/public/WebThemeEngine.h:72
 +          const WebRect&, WebColor, bool fillContentArea, bool drawEdges) = 0;
how does the new API deal with this WebColor parameter and the bools?  will there be more added to the ThemeExtraParams union for these parameters?

[John Abd-El-Malek]

Created attachment 54314 [details]
Updated per comments

[John Abd-El-Malek]

(In reply to comment #4)
> (From update of attachment 54211 [details])
> WebKit/chromium/public/WebThemeEngine.h:107
>  +      // TODO(jam): make these pure virtual once this is rolled into Chromium
> and
> TODO(jam) should be FIXME in WebKit style.  also, it is actually preferred to
> just provide default implementations for methods that are implemented by the
> embedder.  that way it is easier to change the interface.  so, i would just
> delete this TODO :)

Done

> 
> WebKit/chromium/public/WebThemeEngine.h:79
>  +      enum ThemePart {
> can we do without the Theme prefix here?  these are already scoped by
> WebThemeEngine which has the word Theme in it.  WebThemeEngine::Part* and
> WebThemeEngine::State* seems sufficient to me.

good point, done

> 
> WebKit/chromium/public/WebThemeEngine.h:109
>  +      virtual void getSize(ThemePart, ThemeState, WebRect*) {}
> what is the purpose of the getSize function?

The user of this API needs this information to do layout.  For example, if the scrollbars are on the right,  we need to calculate how much space is left for the content.
> 
> WebKit/chromium/public/WebThemeEngine.h:72
>  +          const WebRect&, WebColor, bool fillContentArea, bool drawEdges) =
> 0;
> how does the new API deal with this WebColor parameter and the bools?  will
> there be more added to the ThemeExtraParams union for these parameters?


Exactly

[WebKit Review Bot]

Attachment 54314 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit/chromium/public/win/WebThemeEngine.h:31:  #ifndef header guard has wrong style, please use: WebThemeEngine_h  [build/header_guard] [5]
WebKit/chromium/public/WebThemeEngine.h:1:  One or more unexpected \r (^M) found; better to use only a \n  [whitespace/carriage_return] [1]
Suppressing further [whitespace/carriage_return] reports for this file.
Total errors found: 115 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[John Abd-El-Malek]

(In reply to comment #7)
> Attachment 54314 [details] did not pass style-queue:
> 
> Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
> WebKit/chromium/public/win/WebThemeEngine.h:31:  #ifndef header guard has wrong
> style, please use: WebThemeEngine_h  [build/header_guard] [5]
> WebKit/chromium/public/WebThemeEngine.h:1:  One or more unexpected \r (^M)
> found; better to use only a \n  [whitespace/carriage_return] [1]
> Suppressing further [whitespace/carriage_return] reports for this file.
> Total errors found: 115 in 4 files
> 
> 
> If any of these errors are false positives, please file a bug against
> check-webkit-style.

Note: this error is expected, since until we roll this change into Chromium we'll have two files with the same name and hence I can't use the same ifdef guard.

[Darin Fisher (:fishd, Google)]

Comment on attachment 54314 [details]
Updated per comments

WebKit/chromium/public/WebThemeEngine.h:78
 +  
please add a comment here explaining the transition plan.

WebKit/chromium/public/WebThemeEngine.h:107
 +      virtual void getSize(Part, WebRect*) {}
WebRect -> WebSize

it'd be good to add a comment explaining the meaning of the out param (as you did for pepper).

[John Abd-El-Malek]

Created attachment 54324 [details]
updated

[Darin Fisher (:fishd, Google)]

Comment on attachment 54324 [details]
updated

This is missing the new header file.

[John Abd-El-Malek]

Created attachment 54342 [details]
patch

please have another look.  svn is crashing randomly for me.

[WebKit Review Bot]

Attachment 54342 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebKit/chromium/public/win/WebThemeEngine.h:31:  #ifndef header guard has wrong style, please use: WebThemeEngine_h  [build/header_guard] [5]
WebKit/chromium/public/WebThemeEngine.h:1:  One or more unexpected \r (^M) found; better to use only a \n  [whitespace/carriage_return] [1]
Suppressing further [whitespace/carriage_return] reports for this file.
Total errors found: 122 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[John Abd-El-Malek]

Committed in r 58294.