Bug 26148 – Unfork RenderThemeChromium{Win,Linux}.{h,cc}

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Albert J. Wong]

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.

[Albert J. Wong]

Created attachment 30882 [details]
Step 1: copying RenderThemeChromiumLinux -> RenderThemeChromiumSkia

[Albert J. Wong]

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.

[Eric Seidel (no email)]

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...

[Eric Seidel (no email)]

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.

[Albert J. Wong]

(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.

[Albert J. Wong]

Created attachment 30958 [details]
Adding in empty skia files for step 1 of patch.

[Eric Seidel (no email)]

Comment on attachment 30958 [details]
Adding in empty skia files for step 1 of patch.

Looks fine.

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

Comment on attachment 30958 [details]
Adding in empty skia files for step 1 of patch.

Clearing r+ now that this is landed.

[Albert J. Wong]

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.

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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. :(

[Albert J. Wong]

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.

[Eric Seidel (no email)]

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.

[Albert J. Wong]

Created attachment 31007 [details]
Fix typo and make defaultGUIFont into function again

this time for sure! :D :D :D

[Albert J. Wong]

Created attachment 31015 [details]
Move RenderThemeChromiumSkia into its own file.

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

Comment on attachment 31015 [details]
Move RenderThemeChromiumSkia into its own file.

Assuming you're *only* moving code, then you have my rubber-stamp.

[Albert J. Wong]

(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.

[Albert J. Wong]

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.

[Eric Seidel (no email)]

Comment on attachment 31119 [details]
Remove common code from RenderThemeChromiumWin

I like minus lines.

r=me

[Adam Barth]

Looks complicated, but I'll try to land.

[Adam Barth]

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.

[Adam Barth]

Unassigning.  First patch does not apply cleanly.

[Albert J. Wong]

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.

[Albert J. Wong]

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.

[Albert J. Wong]

Created attachment 31302 [details]
Move RenderThemeChromiumSkia into its own file.  (Rebased to tip of tree after patch 31299)

[Albert J. Wong]

Created attachment 31303 [details]
Remove common code from RenderThemeChromiumWin (Rebased to tip of tree after patch 31302)

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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.

[David Levin]

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.

[David Levin]

Subsequent patches that fixed merge mistakes:
http://trac.webkit.org/changeset/44777
http://trac.webkit.org/changeset/44783
http://trac.webkit.org/changeset/44798