Bug 101634 – Sever Chromium's dependence on WebKitSystemInterface media control drawing functions in RenderThemeMac

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Robert Sesek]

Chromium has RenderThemeChromiumMac that inherits from RenderThemeMac. In RenderThemeMac::paintMedia*, there are various calls to functions in WebKitSystemInterface that Chromium doesn't actually use because it has RenderMediaControlsChromium to paint its media controls. However, since RenderThemeChromiumMac inherits the base implementation in RenderThemeMac, those functions are needed for linking. If we just #if PLATFORM(MAC) guard the impls in RenderThemeMac, the Chromium port will no longer depend on those functions.

This pattern was established in https://bugs.webkit.org/show_bug.cgi?id=37204.

[Robert Sesek]

Created attachment 173105 [details]
Patch

[Adam Barth]

Should we add a RenderThemeAppleMac.mm to hold this apple-mac specific code?

[Robert Sesek]

There was some discussion of that on bug 37204, but this was done instead. There's one or two other functions that do this #if-guarding, so it could make sense to have RenderThemeMac be pure virtual for the functions that differ between PLATFORM(MAC) and PLATFORM(CHROMIUM).

[Adam Barth]

Comment on attachment 173105 [details]
Patch

Ok.  We might want to revisit the relationship between these classes, but that doesn't need to block your work on removing Chromium's WebKitSystemInterface dependency.

[WebKit Review Bot]

Comment on attachment 173105 [details]
Patch

Clearing flags on attachment: 173105

Committed r134004: <http://trac.webkit.org/changeset/134004>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Fraser (smfr)]

This is RenderThemeMac. Why does it need PLATFORM(MAC) #idefs?

[Simon Fraser (smfr)]

Is there any point in trying to share RenderThemeChromiumMac and RenderThemeMac code?

[Adam Barth]

> This is RenderThemeMac. Why does it need PLATFORM(MAC) #idefs?

Because the code is shared by apple-mac and chromium-mac.  I've just added you to a thread on this topic.  I thought I had sent it to webkit-dev, but apparently I only sent it to the people I meant to CC.  We might want to forward the thread to webkit-dev.

> Is there any point in trying to share RenderThemeChromiumMac and RenderThemeMac code?

That topic was discussed in bug 37204.  Apparently sharing code allowed us to delete a large amount of code.

Personally, I find these names confusing.  We might want to restructure things a bit is that it's more clear what code is shared between apple-mac and chromium-mac.

[WebKit Review Bot]

Re-opened since this is blocked by bug 101713

[Maciej Stachowiak]

For this specific case, maybe there should be a RenderMediaControlsMac.cpp to split out the media-control-related calls, along the lines of RenderMediaControlsChromium.cpp. That would perhaps reduce the amount of ifdefing required.

[Robert Sesek]

There's three other non-video methods that are currently #if-guarded, so I like Adam's suggestion of splitting out into RenderThemeAppleMac. If you'd prefer, though, I could create a RenderMediaControlsMac.mm/cpp.

[Robert Sesek]

Created attachment 173385 [details]
RenderThemeAppleMac,v1

[Maciej Stachowiak]

Instead of making the Mac port version be "AppleMac", I'd suggest using "MacShared" for the base class to be shared between "Mac" and "ChromiumMac". The Mac port has always been identified by "mac", PLATFORM(MAC), etc, and I don't think we should introduce the term "AppleMac" in only this one place.

Also let's use a separate bug for distinct changes, especially since the new patch doesn't even do what the title of this bug says.

[Robert Sesek]

(In reply to comment #14)
> Instead of making the Mac port version be "AppleMac", I'd suggest using "MacShared" for the base class to be shared between "Mac" and "ChromiumMac". The Mac port has always been identified by "mac", PLATFORM(MAC), etc, and I don't think we should introduce the term "AppleMac" in only this one place.

Done.

> Also let's use a separate bug for distinct changes, especially since the new patch doesn't even do what the title of this bug says.

The reason why I repurposed this bug is because the first patch was rolled out. I've retitled the bug to be more accurate description of what this is.

[Robert Sesek]

Created attachment 173677 [details]
RenderThemeMacShared,v1

[Adam Barth]

Comment on attachment 173677 [details]
RenderThemeMacShared,v1

View in context: https://bugs.webkit.org/attachment.cgi?id=173677&action=review

I didn't verify that you've moved all this code identically, but the overall shape of this change looks like it matches what mjs asked for.

> Source/WebCore/rendering/RenderThemeMac.mm:56
> +    : RenderThemeMacShared()

Do you need to explicitly call the base class constructor?  Usually the compiler will do that for you.

> Source/WebCore/rendering/RenderThemeMacShared.h:30
> +

Looks like you've got an extra blank line here.

> Source/WebCore/rendering/RenderThemeMacShared.mm:67
> +

Here too

[Robert Sesek]

Comment on attachment 173677 [details]
RenderThemeMacShared,v1

View in context: https://bugs.webkit.org/attachment.cgi?id=173677&action=review

Thanks for the review!

>> Source/WebCore/rendering/RenderThemeMac.mm:56
>> +    : RenderThemeMacShared()
> 
> Do you need to explicitly call the base class constructor?  Usually the compiler will do that for you.

Nope, left over from the old ctor.

>> Source/WebCore/rendering/RenderThemeMacShared.h:30
>> +
> 
> Looks like you've got an extra blank line here.

Done.

>> Source/WebCore/rendering/RenderThemeMacShared.mm:67
>> +
> 
> Here too

Done.

[Robert Sesek]

Created attachment 173983 [details]
RenderThemeMacShared,v2

[WebKit Review Bot]

Comment on attachment 173983 [details]
RenderThemeMacShared,v2

Clearing flags on attachment: 173983

Committed r134520: <http://trac.webkit.org/changeset/134520>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.