Bug 163501 – [Modern Media Controls] Add a MediaControlsHost API to retrieve the shadow root CSS

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

[Antoine Quint]

Now that user agent shadow roots support scoped styles, we should no longer inject the modern media controls CSS as a user-agent stylesheet but rather insert the CSS directly in the shadow root in a <style> element. By exposing a new API on MediaControlsHost, the JS code creating media controls could perform this task.

[Antoine Quint]

Antti added support for scoped stylesheets in user agent shadow roots with https://bugs.webkit.org/show_bug.cgi?id=163212.

[Radar WebKit Bug Importer]

<rdar://problem/28792014>

[Antoine Quint]

Created attachment 291746 [details]
Patch

[Darin Adler]

Comment on attachment 291746 [details]
Patch

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

> Source/WebCore/Modules/mediacontrols/MediaControlsHost.cpp:292
> +    if (page)
> +        return RenderTheme::themeForPage(page)->mediaControlsStyleSheet();
> +    return "";

We use early return, so the null page case should come first.

Returning "" is an inefficient way to return the empty string. The efficient way is:

    return emptyString();

> Source/WebCore/css/CSSDefaultStyleSheets.cpp:187
> +                    mediaRules = emptyString();

Because of the isEmpty check below, this means we will use the media controls user agent style sheet and the extraMediaControlsStyleSheet. Is that really what we want in this case?

[Antoine Quint]

(In reply to comment #4)
> Comment on attachment 291746 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=291746&action=review
> 
> > Source/WebCore/Modules/mediacontrols/MediaControlsHost.cpp:292
> > +    if (page)
> > +        return RenderTheme::themeForPage(page)->mediaControlsStyleSheet();
> > +    return "";
> 
> We use early return, so the null page case should come first.

Will change when landing patch.

> Returning "" is an inefficient way to return the empty string. The efficient
> way is:
> 
>     return emptyString();

Will change when landing patch.

> > Source/WebCore/css/CSSDefaultStyleSheets.cpp:187
> > +                    mediaRules = emptyString();
> 
> Because of the isEmpty check below, this means we will use the media
> controls user agent style sheet and the extraMediaControlsStyleSheet. Is
> that really what we want in this case?

extraMediaControlsStyleSheet is only implemented on GTK, and presumably the runtime flag for modern-media-controls will only ever be true for Cocoa, but it would probably be safer to only have extraMediaControlsStyleSheet if the modern-media-controls flag is off. Will change when landing.

[Antoine Quint]

Created attachment 291844 [details]
Patch for landing

[Antoine Quint]

Created attachment 291845 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 291845 [details]
Patch for landing

Clearing flags on attachment: 291845

Committed r207421: <http://trac.webkit.org/changeset/207421>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.