Bug 26148 - Unfork RenderThemeChromium{Win,Linux}.{h,cc}
: Unfork RenderThemeChromium{Win,Linux}.{h,cc}
Status: RESOLVED FIXED
: WebKit
WebCore Misc.
: 528+ (Nightly build)
: All Mac OS X 10.5
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2009-06-02 15:09 PST by
Modified: 2009-06-18 00:12 PST (History)


Attachments
Step 1: copying RenderThemeChromiumLinux -> RenderThemeChromiumSkia (29.95 KB, patch)
2009-06-02 15:34 PST, Albert J. Wong
no flags Review Patch | Details | Formatted Diff | Diff
copy RenderThemeChromiumLinux -> RenderThemeChromiumSkia (29.86 KB, patch)
2009-06-02 15:47 PST, Albert J. Wong
eric: review-
Review Patch | Details | Formatted Diff | Diff
Adding in empty skia files for step 1 of patch. (1.56 KB, patch)
2009-06-04 13:20 PST, Albert J. Wong
no flags Review Patch | Details | Formatted Diff | Diff
Factors out a common RenderThemeChromiumSkia class and makes RenderThemeChromiumLinux and RenderThemeChromiumWin depend on it. (59.54 KB, patch)
2009-06-04 17:16 PST, Albert J. Wong
eric: review-
Review Patch | Details | Formatted Diff | Diff
Factor out RenderThemeChromiumSkia from just the RenderThemeChromiumLinux portion. (28.94 KB, patch)
2009-06-04 20:10 PST, Albert J. Wong
eric: review-
Review Patch | Details | Formatted Diff | Diff
Fix typo and make defaultGUIFont into function again (28.59 KB, patch)
2009-06-05 12:21 PST, Albert J. Wong
abarth: review-
Review Patch | Details | Formatted Diff | Diff
Move RenderThemeChromiumSkia into its own file. (60.51 KB, patch)
2009-06-05 15:40 PST, Albert J. Wong
no flags Review Patch | Details | Formatted Diff | Diff
Remove common code from RenderThemeChromiumWin (32.03 KB, patch)
2009-06-09 17:56 PST, Albert J. Wong
no flags Review Patch | Details | Formatted Diff | Diff
Rebased patch to tip of tree for extracting RenderThemeChromiumSkia out of RenderThemeChromiumLinux. Other patches to follow. (34.45 KB, patch)
2009-06-15 10:45 PST, Albert J. Wong
no flags Review Patch | Details | Formatted Diff | Diff
Rebased patch to tip of tree for extracting RenderThemeChromiumSkia out of RenderThemeChromiumLinux, with Eric's landing fixes. Other patches to follow. (34.47 KB, patch)
2009-06-15 11:31 PST, Albert J. Wong
eric: review+
Review Patch | Details | Formatted Diff | Diff
Move RenderThemeChromiumSkia into its own file. (Rebased to tip of tree after patch 31299) (62.00 KB, patch)
2009-06-15 12:42 PST, Albert J. Wong
eric: review+
Review Patch | Details | Formatted Diff | Diff
Remove common code from RenderThemeChromiumWin (Rebased to tip of tree after patch 31302) (32.57 KB, patch)
2009-06-15 12:55 PST, Albert J. Wong
eric: review+
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 2009-06-02 15:09:17 PST
There is a lot of duplicated code between RenderThemeChromiumWin and Linux.  Currently the Linux branch is completely generic Skia code. Thus, rename RenderThemeChromiumLinux -> RenderThemeChromiumSkia. Then have RenderThemeChromiumWin inherit off of RenderThemeChromiumSkia.

Since this code's dependencies straddle the chromium and webkit trees, the refactor will require multiple commits as to keep both trees green.

Expected Steps:
  1) _copy_ RenderThemeChromiumLinux to RenderThemeChromiumSkia with appropriate renamings internally.
  2) Update chromium code to break dependency on RenderThemeChromiumLinux (send announcement to chromium-dev), and to build RenderThemeChromiumSkia files.
  3) Make RenderThemeChromiumWin inherit off of RenderThemeChromiumSkia. Delete RenderThemeChromiumLinux files.
------- Comment #1 From 2009-06-02 15:34:27 PST -------
Created an attachment (id=30882) [details]
Step 1: copying RenderThemeChromiumLinux -> RenderThemeChromiumSkia
------- Comment #2 From 2009-06-02 15:47:52 PST -------
Created an attachment (id=30884) [details]
copy RenderThemeChromiumLinux -> RenderThemeChromiumSkia

This patch modifies the copied RenderThemeChromiumLinux in the following ways:
  (1) RenderThemeChromiumLinux renamed RenderThemeChromiumSkia
  (2) Some trailing whitespace was deleted.
  (3) RenderThemer* theme(){} was removed.  This will need to stay in RenderThemeChromiumLinux after all since this is how the compilation determines which version of render theme to instantiate.
  (4) Moved the destructor definition into the implementation file, and added a missing "virtual".
  (5) Fixed an #endif comment.
------- Comment #3 From 2009-06-04 12:14:00 PST -------
(From update of attachment 30884 [details])
I think this would be less error-prone if you renamed the class inside RenderThemeChromiumLinux.* first.  Then you could make Windows depend on it.  And then you could finally move the file instead of copying it, as the last step.  If you feel strongly this order is better, that's OK with me, but you'll just have to be sure to check extra careful at the end to make sure no other modifications to the file slipped in during your move...
------- Comment #4 From 2009-06-04 12:15:55 PST -------
No file outside of RenderThemeChromiumLinux knows that it's called RenderThemeChromiumLinux.  So you can rename the class inside the file w/o anyone notcing.  When you make Windows depend on it, it will have a strange header name for a bit, but fixing that name is a much smaller diff than changing the class name.  moving the file at the same time as you're changing the class internals makes it hard to tell from the diff if its' correct.
------- Comment #5 From 2009-06-04 12:38:12 PST -------
(In reply to comment #4)
> No file outside of RenderThemeChromiumLinux knows that it's called
> RenderThemeChromiumLinux.  So you can rename the class inside the file w/o
> anyone notcing.  When you make Windows depend on it, it will have a strange
> header name for a bit, but fixing that name is a much smaller diff than
> changing the class name.  moving the file at the same time as you're changing
> the class internals makes it hard to tell from the diff if its' correct.
> 

The file that I was having problems with is actually webkit.gyp in the chromium tree.  There needs to be a point where it can list both RenderThemeChromiumSkia.cpp and RenderThemeChromiumLinux.cpp.

I agree with the diffing issue if someone updates RenderThemeChromiumLinux during the migration.

Now that I've gotten further into the merging, I actually don't think I can remove RenderThemeChromiumLinux.cpp completely.  At the very least, it needs to contain the RenderTheme* renderTheme() factory function since that cannot be colocated in the RenderThemeChromiumSkia.cpp file.

How about this:
  1) Create a blank RenderThemeChromiumSkia.cpp and .h file to make gyp happy.
  2) Extract a RenderThemeChromiumSkia out of RenderThemeChromiumLinux, but leave it in the linux file for sane diffing.  (I've already got most things done locally, so I know which functions need to go where).
  3) Move RenderThemeChromiumSkia into it's file.  This should be a pure move at this point because all the style and factoring issues would have been hammered out in #2
  4) Clean up RenderThemeChromiumWin.

will upload a patch with empty files shorty.
------- Comment #6 From 2009-06-04 13:20:43 PST -------
Created an attachment (id=30958) [details]
Adding in empty skia files for step 1 of patch.
------- Comment #7 From 2009-06-04 13:43:10 PST -------
(From update of attachment 30958 [details])
Looks fine.
------- Comment #8 From 2009-06-04 13:44:49 PST -------
(From update of attachment 30958 [details])
Committing to http://svn.webkit.org/repository/webkit/trunk ...
    M    WebCore/ChangeLog
    A    WebCore/rendering/RenderThemeChromiumSkia.cpp
    A    WebCore/rendering/RenderThemeChromiumSkia.h
Committed r44426
------- Comment #9 From 2009-06-04 13:45:21 PST -------
(From update of attachment 30958 [details])
Clearing r+ now that this is landed.
------- Comment #10 From 2009-06-04 17:16:32 PST -------
Created an attachment (id=30974) [details]
Factors out a common RenderThemeChromiumSkia class and makes RenderThemeChromiumLinux and RenderThemeChromiumWin depend on it.

The only bits left in RenderThemeChromiumLinux are the methods taht the windows version does not override itself.

Here are the changes that were not just pure code shuffling:
  1) defaultFontSize was combined, and turned into a protected class member
  2) defaultGUIFont was combined, and turned into a protected class member.  The windows code needed some extra type conversion code added to make this work.
  3) caretBlinkInterval was split into caretBlinkInterval and caretBlinkIntervalInternal to centralize the layout test special code path.

The rest should be just shuffling of code around.

Note that there is a bit of a wart in that some resources referred to by RenderThemeChromiumSkia have names like themeWinUserAgentStyleSheet, or linuxCheckboxOn.  I dont' think the naming is hurtful enough to bother cleaning up at this point.
------- Comment #11 From 2009-06-04 19:21:56 PST -------
(From update of attachment 30974 [details])
This will be easier to review in two parts.  One part which is just the changes to RenderThemeChromiumLinux, and the other which is the changes to RenderThemeChromiumWin.  Sure you only get the big gains with the second part, but there is no change in behavior by separating them.
------- Comment #12 From 2009-06-04 19:27:23 PST -------
(From update of attachment 30974 [details])
Why re-order paintMediaButtonInternal ?  Leaving it where it was makes the patch easier to read.

Why re-order menuListInternalPadding?  Same as above...

Why wouldn't this just be a String or an AtomicString?
static const char* defaultGUIFont;
That would save conversions on every use.

Yeah, I think a little shuffling and splitting could make this diff much smaller.  Hard to review at this size. :(
------- Comment #13 From 2009-06-04 20:10:42 PST -------
Created an attachment (id=30983) [details]
Factor out RenderThemeChromiumSkia from just the RenderThemeChromiumLinux portion.

Hey Eric, here's another shot.

It's not a pure move, but went through the diff this time and listed all the non-move changes I made.

The reason the functions were shuffled last time was cause they were moved from RenderThemeChromiumLinux to RenderThemeChromiumSkia and I was trying to keep each classes methods together.  However, since it's all going to another file anwyays, no need to worry about that. :)

I don't actually know what the difference between String and AtomicString are so I just chose String. Let me know if you want me to use the other.  I assume that there WebKit doesn't ban using non-POD data types in globals/statics like Chromium does?

I'll upload the windows side of this tomorrow.
------- Comment #14 From 2009-06-04 20:50:57 PST -------
(From update of attachment 30983 [details])
Looks great!

In response to your IRC question, yes WebKit avoids non pod globals because they slow down library load/startup time.  So you're right that you'll need to fix defaultGUIFont to use a function or some other solution.

Typo:
+           1) Createion of a caretBlinkIntervalInternal.

Looks great otherwise.

r- because I'm worried the global constructor could cause build bots to break.
------- Comment #15 From 2009-06-05 12:21:06 PST -------
Created an attachment (id=31007) [details]
Fix typo and make defaultGUIFont into function again

this time for sure! :D :D :D
------- Comment #16 From 2009-06-05 15:40:24 PST -------
Created an attachment (id=31015) [details]
Move RenderThemeChromiumSkia into its own file.
------- Comment #17 From 2009-06-06 01:19:26 PST -------
(From update of attachment 31007 [details])
const String& RenderThemeChromiumSkia::defaultGUIFont() {

{ should be on it's own line.  someone can fix that when landing this.
Also WebKit uses 4 spaces, not 2!
+  static String fontFace("Arial");


If this needed to be compiled on Mac, due to a GCC bug we would need to use DEFINE_STATIC_LOCAL for:
+  static String fontFace("Arial");
but it does not, so you're fine.

Needs tiny fixes when landing.
------- Comment #18 From 2009-06-06 01:20:06 PST -------
(From update of attachment 31015 [details])
Assuming you're *only* moving code, then you have my rubber-stamp.
------- Comment #19 From 2009-06-06 08:42:30 PST -------
(In reply to comment #18)
> (From update of attachment 31015 [details] [review])
> Assuming you're *only* moving code, then you have my rubber-stamp.
> 

The only extra change was to #include "Color.h" in RenderThemeChromiumLinux.cpp cause that class used the color class directly.  Otherwise, yes, pure move.
------- Comment #20 From 2009-06-09 17:56:08 PST -------
Created an attachment (id=31119) [details]
Remove common code from RenderThemeChromiumWin

Reviewing this one is going to be the difficult one.

The 2 checks I did was as follows:
  1) Verify that all functions in RenderThemeChromiumSkia were in the original RenderThemeChromiumWin.
  2) Verify that all removed functions from RenderThemeChromiumWin were identical to the Skai versions.

For check #1, I found two functions that slipped through last time.

  supportsControlTint -> Moved to RenderThemeChromiumLinux
  platformTextSearchHighlightColor -> This isn't in RenderTheme.  Removed.
------- Comment #21 From 2009-06-09 18:00:46 PST -------
(From update of attachment 31119 [details])
I like minus lines.

r=me
------- Comment #22 From 2009-06-13 20:37:21 PST -------
Looks complicated, but I'll try to land.
------- Comment #23 From 2009-06-13 21:06:48 PST -------
(From update of attachment 31007 [details])
The patch does not apply to TOT.  The fixup might be easy or hard.  Not sure.
------- Comment #24 From 2009-06-13 21:07:39 PST -------
Unassigning.  First patch does not apply cleanly.
------- Comment #25 From 2009-06-15 10:45:19 PST -------
Created an attachment (id=31297) [details]
Rebased patch to tip of tree for extracting RenderThemeChromiumSkia out of RenderThemeChromiumLinux.  Other patches to follow.

The updated patch has 3 changes:

  (1) Handles the addition of systemColor into RenderThemeChromiumLinux.  This was left in RenderThemeChromiumLinux and not put in RenderThemeChromiumSkia because it is not shared with windows.
  (2) Moved the adding of the linux specific user agent style sheet into RenderThemeChromiumLinux.  This required splitting the extraDefaultStyleSheet function
  (3) Updated the patch to respect the changes to paintButtonLike.  This has no effective changes since it is just a static function.
------- Comment #26 From 2009-06-15 11:31:02 PST -------
Created an attachment (id=31299) [details]
Rebased patch to tip of tree for extracting RenderThemeChromiumSkia out of RenderThemeChromiumLinux, with Eric's landing fixes. Other patches to follow.
------- Comment #27 From 2009-06-15 12:42:05 PST -------
Created an attachment (id=31302) [details]
Move RenderThemeChromiumSkia into its own file.  (Rebased to tip of tree after patch 31299)
------- Comment #28 From 2009-06-15 12:55:57 PST -------
Created an attachment (id=31303) [details]
Remove common code from RenderThemeChromiumWin (Rebased to tip of tree after patch 31302)
------- Comment #29 From 2009-06-17 00:20:56 PST -------
(From update of attachment 31299 [details])
The ChangeLog is doubled.

Indent:
bool RenderThemeChromiumSkia::supportsHover(const RenderStyle* style) const
 141 {
 142   return true;
 143 }

Unused parameters can cause warnings and thus compile failures:
bool RenderThemeChromiumSkia::supportsFocusRing(const RenderStyle* style) const
155146 {
156      return supportsFocus(style->appearance());
 147     // This causes WebKit to draw the focus rings for us.
 148     return false;
157149 }

Otherwise looks fine.  Someone could fix this when landing I guess.
------- Comment #30 From 2009-06-17 00:22:34 PST -------
(From update of attachment 31302 [details])
WARNING: NO TEST CASES ADDED OR CHANGED
should be replaced by an explanation as to why testing is impossible or why it does not need tests.  In this case, no tests are needed.  You should note that your'e only moving code.

The ChangeLog can be fixed on landing.  This looks fine.
------- Comment #31 From 2009-06-17 00:28:46 PST -------
(From update of attachment 31303 [details])
Looks fine.  WARNING: NO TEST CASES ADDED OR CHANGED should have been replaced by a sentence explaing why this does not need testing.  I filed bug 26475 about this common point of confusion.
------- Comment #32 From 2009-06-17 12:19:47 PST -------
The three r+ patches were committed in http://trac.webkit.org/changeset/44774, http://trac.webkit.org/changeset/44775, http://trac.webkit.org/changeset/44776.
------- Comment #33 From 2009-06-18 00:12:21 PST -------
Subsequent patches that fixed merge mistakes:
http://trac.webkit.org/changeset/44777
http://trac.webkit.org/changeset/44783
http://trac.webkit.org/changeset/44798