Bug 38077 - [chromium] Prepare to making WebThemeEngine cross-platform
: [chromium] Prepare to making WebThemeEngine cross-platform
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: New Bugs
: 528+ (Nightly build)
: Other Mac OS X 10.5
: P2 Normal
Assigned To: John Abd-El-Malek
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-04-23 18:54 PDT by John Abd-El-Malek
Modified: 2010-04-26 23:29 PDT (History)
5 users (show)

See Also:


Attachments
Proposed patch (7.13 KB, patch)
2010-04-23 19:51 PDT, John Abd-El-Malek
fishd: review-
Details | Formatted Diff | Diff
Updated per comments (7.29 KB, patch)
2010-04-26 10:05 PDT, John Abd-El-Malek
fishd: review-
Details | Formatted Diff | Diff
updated (3.24 KB, patch)
2010-04-26 12:53 PDT, John Abd-El-Malek
fishd: review-
Details | Formatted Diff | Diff
patch (7.73 KB, patch)
2010-04-26 15:47 PDT, John Abd-El-Malek
fishd: review+
fishd: commit‑queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description John Abd-El-Malek 2010-04-23 18:54:43 PDT
[chromium] Prepare to making WebThemeEngine cross-platform
Comment 1 John Abd-El-Malek 2010-04-23 19:51:43 PDT
Created attachment 54211 [details]
Proposed patch
Comment 2 John Abd-El-Malek 2010-04-23 19:52:37 PDT
This just moves the WebThemeEngine file so that I can update the Chromium code with the new locations.
Comment 3 Adam Barth 2010-04-23 20:17:27 PDT
Comment on attachment 54211 [details]
Proposed patch

We need fishd to review this patch, but seems reasonable to me.
Comment 4 Darin Fisher (:fishd, Google) 2010-04-23 22:00:27 PDT
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?
Comment 5 John Abd-El-Malek 2010-04-26 10:05:11 PDT
Created attachment 54314 [details]
Updated per comments
Comment 6 John Abd-El-Malek 2010-04-26 10:09:47 PDT
(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
Comment 7 WebKit Review Bot 2010-04-26 10:10:46 PDT
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.
Comment 8 John Abd-El-Malek 2010-04-26 10:44:03 PDT
(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.
Comment 9 Darin Fisher (:fishd, Google) 2010-04-26 12:47:06 PDT
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).
Comment 10 John Abd-El-Malek 2010-04-26 12:53:25 PDT
Created attachment 54324 [details]
updated
Comment 11 Darin Fisher (:fishd, Google) 2010-04-26 15:33:01 PDT
Comment on attachment 54324 [details]
updated

This is missing the new header file.
Comment 12 John Abd-El-Malek 2010-04-26 15:47:19 PDT
Created attachment 54342 [details]
patch

please have another look.  svn is crashing randomly for me.
Comment 13 WebKit Review Bot 2010-04-26 15:51:22 PDT
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.
Comment 14 John Abd-El-Malek 2010-04-26 23:29:33 PDT
Committed in r 58294.