Bug 37204 - RenderThemeChromiumMac.mm should share code with RenderThemeMac.mm
Summary: RenderThemeChromiumMac.mm should share code with RenderThemeMac.mm
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P3 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 37309
  Show dependency treegraph
 
Reported: 2010-04-07 05:37 PDT by Hajime Morrita
Modified: 2010-05-09 17:44 PDT (History)
1 user (show)

See Also:


Attachments
patch v0 (83.10 KB, patch)
2010-04-07 05:55 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
took feedbacks (83.20 KB, patch)
2010-04-09 22:24 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hajime Morrita 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.
Comment 1 Hajime Morrita 2010-04-07 05:55:57 PDT
Created attachment 52732 [details]
patch v0
Comment 2 Hajime Morrita 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.
Comment 3 Kent Tamura 2010-04-08 00:48:41 PDT
Removed [Chromium] from the summary because the patch modifies non-Chromium code too.
Comment 4 Eric Seidel (no email) 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.
Comment 5 Eric Seidel (no email) 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.
Comment 6 Hajime Morrita 2010-04-09 22:24:17 PDT
Created attachment 53031 [details]
took feedbacks
Comment 7 Hajime Morrita 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.
Comment 8 Eric Seidel (no email) 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.
Comment 9 Hajime Morrita 2010-05-07 05:31:37 PDT
Committed r58943: <http://trac.webkit.org/changeset/58943>