Bug 163502 – [Modern Media Controls] Add a MediaControlsHost API to retrieve images as base64

• Component: Media
• Status: RESOLVED FIXED

[Antoine Quint]

The previous media controls codebase would inline images as base64 in stylesheets, making it hard to version those resources. We could add a new method to MediaControlsHost to obtain the base64 encoded version of a given image so that we could obtain images at runtime from the WebCore bundle.

[Radar WebKit Bug Importer]

<rdar://problem/28792017>

[Antoine Quint]

Created attachment 291747 [details]
Patch

[Darin Adler]

Comment on attachment 291747 [details]
Patch

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

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

Early return means error case comes first. Please use emptyString(), rather than "".

> Source/WebCore/Modules/mediacontrols/MediaControlsHost.h:85
> +    String base64StringForIconAndPlatform(String iconName, String platform) const;

Argument types should be const String&, not String.

> Source/WebCore/Modules/mediacontrols/MediaControlsHost.idl:63
> +    [EnabledAtRuntime=ModernMediaControls] DOMString base64StringForIconAndPlatform(DOMString iconName, DOMString platform);

Could be a dangerous API for performance because it loads the file from disk and encodes it every time with no caching.

[Antoine Quint]

(In reply to comment #3)
> Comment on attachment 291747 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=291747&action=review
> 
> > Source/WebCore/Modules/mediacontrols/MediaControlsHost.cpp:292
> > +    if (page)
> > +        return RenderTheme::themeForPage(page)->mediaControlsBase64StringForIconAndPlatform(iconName, platform);
> > +    return "";
> 
> Early return means error case comes first. Please use emptyString(), rather
> than "".

Sure thing, will change when landing.

> > Source/WebCore/Modules/mediacontrols/MediaControlsHost.h:85
> > +    String base64StringForIconAndPlatform(String iconName, String platform) const;
> 
> Argument types should be const String&, not String.

Thanks, will change when landing.

> > Source/WebCore/Modules/mediacontrols/MediaControlsHost.idl:63
> > +    [EnabledAtRuntime=ModernMediaControls] DOMString base64StringForIconAndPlatform(DOMString iconName, DOMString platform);
> 
> Could be a dangerous API for performance because it loads the file from disk
> and encodes it every time with no caching.

True. I will add caching on the JS side as part of the iconService singleton (see Source/WebCore/Modules/modern-media-controls/controls/icon-service.js). Currently caching is done based on the image URL, but we need to do it based on the iconName / platform pair.

[Antoine Quint]

Created attachment 291856 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 291856 [details]
Patch for landing

Clearing flags on attachment: 291856

Committed r207423: <http://trac.webkit.org/changeset/207423>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.