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.
Created attachment 52732 [details] patch v0
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.
Removed [Chromium] from the summary because the patch modifies non-Chromium code too.
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.
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.
Created attachment 53031 [details] took feedbacks
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.
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.
Committed r58943: <http://trac.webkit.org/changeset/58943>