Bug 38077 - [chromium] Prepare to making WebThemeEngine cross-platform
: [chromium] Prepare to making WebThemeEngine cross-platform
Status: RESOLVED FIXED
: WebKit
New Bugs
: 528+ (Nightly build)
: Other Mac OS X 10.5
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2010-04-23 18:54 PST by
Modified: 2010-04-26 23:29 PST (History)


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


Note

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


Description From 2010-04-23 18:54:43 PST
[chromium] Prepare to making WebThemeEngine cross-platform
------- Comment #1 From 2010-04-23 19:51:43 PST -------
Created an attachment (id=54211) [details]
Proposed patch
------- Comment #2 From 2010-04-23 19:52:37 PST -------
This just moves the WebThemeEngine file so that I can update the Chromium code with the new locations.
------- Comment #3 From 2010-04-23 20:17:27 PST -------
(From update of attachment 54211 [details])
We need fishd to review this patch, but seems reasonable to me.
------- Comment #4 From 2010-04-23 22:00:27 PST -------
(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 :)

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 From 2010-04-26 10:05:11 PST -------
Created an attachment (id=54314) [details]
Updated per comments
------- Comment #6 From 2010-04-26 10:09:47 PST -------
(In reply to comment #4)
> (From update of attachment 54211 [details] [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 From 2010-04-26 10:10:46 PST -------
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 From 2010-04-26 10:44:03 PST -------
(In reply to comment #7)
> Attachment 54314 [details] [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 From 2010-04-26 12:47:06 PST -------
(From update of attachment 54314 [details])
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 From 2010-04-26 12:53:25 PST -------
Created an attachment (id=54324) [details]
updated
------- Comment #11 From 2010-04-26 15:33:01 PST -------
(From update of attachment 54324 [details])
This is missing the new header file.
------- Comment #12 From 2010-04-26 15:47:19 PST -------
Created an attachment (id=54342) [details]
patch

please have another look.  svn is crashing randomly for me.
------- Comment #13 From 2010-04-26 15:51:22 PST -------
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 From 2010-04-26 23:29:33 PST -------
Committed in r 58294.