Bug 163501 - [Modern Media Controls] Add a MediaControlsHost API to retrieve the shadow root CSS
Summary: [Modern Media Controls] Add a MediaControlsHost API to retrieve the shadow ro...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: Safari 10
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Antoine Quint
URL:
Keywords: InRadar
Depends on: 163500
Blocks:
  Show dependency treegraph
 
Reported: 2016-10-16 06:42 PDT by Antoine Quint
Modified: 2016-10-17 12:14 PDT (History)
3 users (show)

See Also:


Attachments
Patch (4.71 KB, patch)
2016-10-16 06:55 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch for landing (5.24 KB, patch)
2016-10-17 11:37 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch for landing (5.24 KB, patch)
2016-10-17 11:39 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antoine Quint 2016-10-16 06:42:00 PDT
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.
Comment 1 Antoine Quint 2016-10-16 06:42:32 PDT
Antti added support for scoped stylesheets in user agent shadow roots with https://bugs.webkit.org/show_bug.cgi?id=163212.
Comment 2 Radar WebKit Bug Importer 2016-10-16 06:42:59 PDT
<rdar://problem/28792014>
Comment 3 Antoine Quint 2016-10-16 06:55:02 PDT
Created attachment 291746 [details]
Patch
Comment 4 Darin Adler 2016-10-16 10:10:25 PDT
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?
Comment 5 Antoine Quint 2016-10-16 15:05:56 PDT
(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.
Comment 6 Antoine Quint 2016-10-17 11:37:34 PDT
Created attachment 291844 [details]
Patch for landing
Comment 7 Antoine Quint 2016-10-17 11:39:14 PDT
Created attachment 291845 [details]
Patch for landing
Comment 8 WebKit Commit Bot 2016-10-17 12:14:01 PDT
Comment on attachment 291845 [details]
Patch for landing

Clearing flags on attachment: 291845

Committed r207421: <http://trac.webkit.org/changeset/207421>
Comment 9 WebKit Commit Bot 2016-10-17 12:14:05 PDT
All reviewed patches have been landed.  Closing bug.