Summary: | [chromium] Prepare to making WebThemeEngine cross-platform | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | John Abd-El-Malek <jam> | ||||||||||
Component: | New Bugs | Assignee: | John Abd-El-Malek <jam> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | abarth, dglazkov, fishd, levin, webkit.review.bot | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Other | ||||||||||||
OS: | OS X 10.5 | ||||||||||||
Attachments: |
|
Description
John Abd-El-Malek
2010-04-23 18:54:43 PDT
Created attachment 54211 [details]
Proposed patch
This just moves the WebThemeEngine file so that I can update the Chromium code with the new locations. Comment on attachment 54211 [details]
Proposed patch
We need fishd to review this patch, but seems reasonable to me.
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?
Created attachment 54314 [details]
Updated per comments
(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 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.
(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 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).
Created attachment 54324 [details]
updated
Comment on attachment 54324 [details]
updated
This is missing the new header file.
Created attachment 54342 [details]
patch
please have another look. svn is crashing randomly for me.
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.
Committed in r 58294. |