Bug 37204

Summary: RenderThemeChromiumMac.mm should share code with RenderThemeMac.mm
Product: WebKit Reporter: Hajime Morrita <morrita>
Component: PlatformAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: tkent
Priority: P3    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Bug Depends on:    
Bug Blocks: 37309    
Attachments:
Description Flags
patch v0
none
took feedbacks none

Hajime Morrita
Reported 2010-04-07 05:37:24 PDT
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.
Attachments
patch v0 (83.10 KB, patch)
2010-04-07 05:55 PDT, Hajime Morrita 		no flags
DetailsFormatted DiffDiff
took feedbacks (83.20 KB, patch)
2010-04-09 22:24 PDT, Hajime Morrita 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Hajime Morrita
Comment 1 2010-04-07 05:55:57 PDT
Created attachment 52732 [details] patch v0
Hajime Morrita
Comment 2 2010-04-07 06:07:53 PDT
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
Comment 3 2010-04-08 00:48:41 PDT
Removed [Chromium] from the summary because the patch modifies non-Chromium code too.
Eric Seidel (no email)
Comment 4 2010-04-09 13:36:24 PDT
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 5 2010-04-09 14:01:09 PDT
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
Comment 6 2010-04-09 22:24:17 PDT
Created attachment 53031 [details] took feedbacks
Hajime Morrita
Comment 7 2010-04-09 22:29:26 PDT
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 8 2010-05-06 22:31:54 PDT
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
Comment 9 2010-05-07 05:31:37 PDT
Committed r58943: <http://trac.webkit.org/changeset/58943>
Note You need to log in before you can comment on or make changes to this bug.