Bug 26148 - Unfork RenderThemeChromium{Win,Linux}.{h,cc}
Summary: Unfork RenderThemeChromium{Win,Linux}.{h,cc}
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-06-02 15:09 PDT by Albert J. Wong
Modified: 2009-06-18 00:12 PDT (History)
2 users (show)

See Also:


Attachments
Step 1: copying RenderThemeChromiumLinux -> RenderThemeChromiumSkia (29.95 KB, patch)
2009-06-02 15:34 PDT, Albert J. Wong
no flags Details | Formatted Diff | Diff
copy RenderThemeChromiumLinux -> RenderThemeChromiumSkia (29.86 KB, patch)
2009-06-02 15:47 PDT, Albert J. Wong
eric: review-
Details | Formatted Diff | Diff
Adding in empty skia files for step 1 of patch. (1.56 KB, patch)
2009-06-04 13:20 PDT, Albert J. Wong
no flags 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 PDT, Albert J. Wong
eric: review-
Details | Formatted Diff | Diff
Factor out RenderThemeChromiumSkia from just the RenderThemeChromiumLinux portion. (28.94 KB, patch)
2009-06-04 20:10 PDT, Albert J. Wong
eric: review-
Details | Formatted Diff | Diff
Fix typo and make defaultGUIFont into function again (28.59 KB, patch)
2009-06-05 12:21 PDT, Albert J. Wong
abarth: review-
Details | Formatted Diff | Diff
Move RenderThemeChromiumSkia into its own file. (60.51 KB, patch)
2009-06-05 15:40 PDT, Albert J. Wong
no flags Details | Formatted Diff | Diff
Remove common code from RenderThemeChromiumWin (32.03 KB, patch)
2009-06-09 17:56 PDT, Albert J. Wong
no flags 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 PDT, Albert J. Wong
no flags 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 PDT, Albert J. Wong
eric: review+
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 PDT, Albert J. Wong
eric: review+
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 PDT, Albert J. Wong
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Albert J. Wong 2009-06-02 15:09:17 PDT
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 Albert J. Wong 2009-06-02 15:34:27 PDT
Created attachment 30882 [details]
Step 1: copying RenderThemeChromiumLinux -> RenderThemeChromiumSkia
Comment 2 Albert J. Wong 2009-06-02 15:47:52 PDT
Created attachment 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 Eric Seidel (no email) 2009-06-04 12:14:00 PDT
Comment on attachment 30884 [details]
copy RenderThemeChromiumLinux -> RenderThemeChromiumSkia

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 Eric Seidel (no email) 2009-06-04 12:15:55 PDT
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 Albert J. Wong 2009-06-04 12:38:12 PDT
(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 Albert J. Wong 2009-06-04 13:20:43 PDT
Created attachment 30958 [details]
Adding in empty skia files for step 1 of patch.
Comment 7 Eric Seidel (no email) 2009-06-04 13:43:10 PDT
Comment on attachment 30958 [details]
Adding in empty skia files for step 1 of patch.

Looks fine.
Comment 8 Eric Seidel (no email) 2009-06-04 13:44:49 PDT
Comment on attachment 30958 [details]
Adding in empty skia files for step 1 of patch.

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 Eric Seidel (no email) 2009-06-04 13:45:21 PDT
Comment on attachment 30958 [details]
Adding in empty skia files for step 1 of patch.

Clearing r+ now that this is landed.
Comment 10 Albert J. Wong 2009-06-04 17:16:32 PDT
Created attachment 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 Eric Seidel (no email) 2009-06-04 19:21:56 PDT
Comment on attachment 30974 [details]
Factors out a common RenderThemeChromiumSkia class and makes RenderThemeChromiumLinux and RenderThemeChromiumWin depend on it.

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 Eric Seidel (no email) 2009-06-04 19:27:23 PDT
Comment on attachment 30974 [details]
Factors out a common RenderThemeChromiumSkia class and makes RenderThemeChromiumLinux and RenderThemeChromiumWin depend on it.

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 Albert J. Wong 2009-06-04 20:10:42 PDT
Created attachment 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 Eric Seidel (no email) 2009-06-04 20:50:57 PDT
Comment on attachment 30983 [details]
Factor out RenderThemeChromiumSkia from just the RenderThemeChromiumLinux portion.

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 Albert J. Wong 2009-06-05 12:21:06 PDT
Created attachment 31007 [details]
Fix typo and make defaultGUIFont into function again

this time for sure! :D :D :D
Comment 16 Albert J. Wong 2009-06-05 15:40:24 PDT
Created attachment 31015 [details]
Move RenderThemeChromiumSkia into its own file.
Comment 17 Eric Seidel (no email) 2009-06-06 01:19:26 PDT
Comment on attachment 31007 [details]
Fix typo and make defaultGUIFont into function again

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 Eric Seidel (no email) 2009-06-06 01:20:06 PDT
Comment on attachment 31015 [details]
Move RenderThemeChromiumSkia into its own file.

Assuming you're *only* moving code, then you have my rubber-stamp.
Comment 19 Albert J. Wong 2009-06-06 08:42:30 PDT
(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 Albert J. Wong 2009-06-09 17:56:08 PDT
Created attachment 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 Eric Seidel (no email) 2009-06-09 18:00:46 PDT
Comment on attachment 31119 [details]
Remove common code from RenderThemeChromiumWin

I like minus lines.

r=me
Comment 22 Adam Barth 2009-06-13 20:37:21 PDT
Looks complicated, but I'll try to land.
Comment 23 Adam Barth 2009-06-13 21:06:48 PDT
Comment on attachment 31007 [details]
Fix typo and make defaultGUIFont into function again

The patch does not apply to TOT.  The fixup might be easy or hard.  Not sure.
Comment 24 Adam Barth 2009-06-13 21:07:39 PDT
Unassigning.  First patch does not apply cleanly.
Comment 25 Albert J. Wong 2009-06-15 10:45:19 PDT
Created attachment 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 Albert J. Wong 2009-06-15 11:31:02 PDT
Created attachment 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 Albert J. Wong 2009-06-15 12:42:05 PDT
Created attachment 31302 [details]
Move RenderThemeChromiumSkia into its own file.  (Rebased to tip of tree after patch 31299)
Comment 28 Albert J. Wong 2009-06-15 12:55:57 PDT
Created attachment 31303 [details]
Remove common code from RenderThemeChromiumWin (Rebased to tip of tree after patch 31302)
Comment 29 Eric Seidel (no email) 2009-06-17 00:20:56 PDT
Comment on attachment 31299 [details]
Rebased patch to tip of tree for extracting RenderThemeChromiumSkia out of RenderThemeChromiumLinux, with Eric's landing fixes. Other patches to follow.

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 Eric Seidel (no email) 2009-06-17 00:22:34 PDT
Comment on attachment 31302 [details]
Move RenderThemeChromiumSkia into its own file.  (Rebased to tip of tree after patch 31299)

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 Eric Seidel (no email) 2009-06-17 00:28:46 PDT
Comment on attachment 31303 [details]
Remove common code from RenderThemeChromiumWin (Rebased to tip of tree after patch 31302)

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 David Levin 2009-06-17 12:19:47 PDT
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 David Levin 2009-06-18 00:12:21 PDT
Subsequent patches that fixed merge mistakes:
http://trac.webkit.org/changeset/44777
http://trac.webkit.org/changeset/44783
http://trac.webkit.org/changeset/44798