Bug 37204 – RenderThemeChromiumMac.mm should share code with RenderThemeMac.mm

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Hajime Morrita]

RenderThemeChromiumMac.mm/h is almost a clone of RnederThemeMac.mm/h with difference of few tens of lines.
So RenderThemeChromiumMac should share the codebase with RenderThemeMac instead of copying it.

[Hajime Morrita]

Created attachment 52732 [details]
patch v0

[Hajime Morrita]

Another idea would be extracting base class for both RenderThemeMac and RenderThemeChromiumMac.
But it feels overdone for me because RenderThemeChromiumMac is essentially patched RenderThemeMac.

RenderThemeMac.mm and RenderThemeChromium.mm mac become included 
Chromium mac build, so this change enclose some Mac Webkit specific mehods into 
PLATFORM(MAC) ifdefs. But even with that,
it looks better than extracting something like RenderThemeMacBase, thanks for smaller the size of change.

It might be massy to have chromium-aware hooks for RenderThemeMac. 
But having copy-n-pasted clone should be worse. 
It would be happy to remove large code duplication in return for small hooks like this.

[Kent Tamura]

Removed [Chromium] from the summary because the patch modifies non-Chromium code too.

[Eric Seidel (no email)]

Comment on attachment 52732 [details]
patch v0

OK.  This is WAY better than what we have now.  The only worry I have is burned on the Mac folks and/or regressions to the Mac code.

In general I think this looks great.

I think the new methods you're adding to RenderThemeMac.h should be commented/documented in the header.

[Eric Seidel (no email)]

Comment on attachment 52732 [details]
patch v0

This guard seems unecessary:
60 #if PLATFORM(CHROMIUM)
15961 PassRefPtr<RenderTheme> RenderTheme::themeForPage(Page*)
16062 {
I'm ready to r+ this with additional comments in the header explaining the new methods.

[Hajime Morrita]

Created attachment 53031 [details]
took feedbacks

[Hajime Morrita]

Hi Eric, Thank you for reviewing!

> OK.  This is WAY better than what we have now.  The only worry I have is burned
> on the Mac folks and/or regressions to the Mac code.
I agree your concern. 
Fortunately changes on RenderThemeMac.mm is small and is (or should be) trivial -
It just added empty hooks, extract a part of code blocks to methods.
I hope these changes would be acceptable...

> I think the new methods you're adding to RenderThemeMac.h should be
> commented/documented in the header.
Added comments to explain their purpose.

>This guard seems unecessary:
>60 #if PLATFORM(CHROMIUM)
>15961 PassRefPtr<RenderTheme> RenderTheme::themeForPage(Page*)
>16062 {
Removed.

[Eric Seidel (no email)]

Comment on attachment 53031 [details]
took feedbacks

LGTM.  Please be careful to update to tip of tree before landing.  There were likely changes to RenderThemeMac while this was out for review.

[Hajime Morrita]

Committed r58943: <http://trac.webkit.org/changeset/58943>