Bug 180005 - [GTK][WPE] Add "enable-encryptedmedia" property to WebKitWebSettings
Summary: [GTK][WPE] Add "enable-encryptedmedia" property to WebKitWebSettings
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-11-24 07:17 PST by Yacine Bandou
Modified: 2017-11-27 11:36 PST (History)
10 users (show)

See Also:


Attachments
Patch (9.15 KB, patch)
2017-11-24 08:30 PST, Yacine Bandou
no flags Details | Formatted Diff | Diff
Patch (9.63 KB, patch)
2017-11-27 03:18 PST, Yacine Bandou
no flags Details | Formatted Diff | Diff
Patch (9.63 KB, patch)
2017-11-27 08:47 PST, Yacine Bandou
no flags Details | Formatted Diff | Diff
Patch (9.66 KB, patch)
2017-11-27 10:56 PST, Yacine Bandou
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yacine Bandou 2017-11-24 07:17:30 PST
Add "enable-encryptedmedia" property to WebKitWebSettings, include the new API 
(webkit_settings_get_enable_encryptedmedia and webkit_settings_set_enable_encryptedmedia) 
in Glib unit tests and in Gtk documentation.
Comment 1 Michael Catanzaro 2017-11-24 07:30:13 PST
I don't think we should do this unless ENABLE_ENCRYPTED_MEDIA is enabled by default at build time, because we don't want to expose runtime settings in our API that don't actually work. That would be very confusing.

And I believe the EME developers have no plans to turn on ENABLE_ENCRYPTED_MEDIA by default.

Is there a scenario in which you are using a custom build of WebKit yet need to be able to disable this feature at runtime?
Comment 2 Olivier Blin 2017-11-24 07:46:30 PST
(In reply to Michael Catanzaro from comment #1)
> I don't think we should do this unless ENABLE_ENCRYPTED_MEDIA is enabled by
> default at build time, because we don't want to expose runtime settings in
> our API that don't actually work. That would be very confusing.
> 
> And I believe the EME developers have no plans to turn on
> ENABLE_ENCRYPTED_MEDIA by default.
> 
> Is there a scenario in which you are using a custom build of WebKit yet need
> to be able to disable this feature at runtime?

ENABLE_ENCRYPTED_MEDIA is enabled by default in a developer build.
But it is not enabled at runtime by default, since EncryptedMediaAPIEnabled is set to false in WebPreferences.yaml.
AFAIK, it is only enabled by WebKitTestRunner/InjectedBundle using the C API.

So there is currently no way to actually use EME with a developer build of WPE or GTK.
Comment 3 Yacine Bandou 2017-11-24 08:30:28 PST
Created attachment 327550 [details]
Patch
Comment 4 Build Bot 2017-11-24 08:32:38 PST
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
Comment 5 Carlos Alberto Lopez Perez 2017-11-24 10:09:02 PST
(In reply to Olivier Blin from comment #2)
> (In reply to Michael Catanzaro from comment #1)
> > I don't think we should do this unless ENABLE_ENCRYPTED_MEDIA is enabled by
> > default at build time, because we don't want to expose runtime settings in
> > our API that don't actually work. That would be very confusing.
> > 
> > And I believe the EME developers have no plans to turn on
> > ENABLE_ENCRYPTED_MEDIA by default.
> > 
> > Is there a scenario in which you are using a custom build of WebKit yet need
> > to be able to disable this feature at runtime?
> 
> ENABLE_ENCRYPTED_MEDIA is enabled by default in a developer build.

This changed by accident a few days ago ( see bug 180004 )

> But it is not enabled at runtime by default, since EncryptedMediaAPIEnabled
> is set to false in WebPreferences.yaml.
> AFAIK, it is only enabled by WebKitTestRunner/InjectedBundle using the C API.
> 
> So there is currently no way to actually use EME with a developer build of
> WPE or GTK.

So this is a case like the one we have MediaStream/WebRTC.

We ship a run-time property that only can be used if a non-default build option is enabled https://webkitgtk.org/reference/webkit2gtk/stable/WebKitSettings.html#webkit-settings-set-enable-media-stream

Now that we have renamed the concept of "developer build" to "experimental features", I think is a good compromise to document on all this run-time settings that the feature is still experimental (e.g. not finished, under development) and that it needs to be enabled also at build time to be used.
Comment 6 Michael Catanzaro 2017-11-24 16:39:06 PST
Comment on attachment 327550 [details]
Patch

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

I was going to give this r- for the reason I mentioned above, and ask you to change the runtime default to depend on ENABLE_ENCRYPTED_MEDIA instead... but now that Carlos Lopez has pointed out there is precedent for having API that does not work except in custom builds, I guess we might as well. But please do document that it as Carlos Lopez requested.

By the way, it's interesting that you've pointed out that it is currently impossible to enable encrypted media support. I wonder how long it has been since anybody has tried to test this. Probably a long time.

> Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:1305
> +        g_param_spec_boolean("enable-encryptedmedia",

Please separate:

PROP_ENABLE_ENCRYPTED_MEDIA

"enable-encrypted-media"

> Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:3169
> + * webkit_settings_get_enable_encryptedmedia:

encrypted_media for the function names too.

> Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:3174
> + * Returns: %TRUE If EncryptedMedia support is enabled or %FALSE otherwise.

"if" (lowercase)
Comment 7 Yacine Bandou 2017-11-27 03:18:11 PST
Created attachment 327627 [details]
Patch
Comment 8 Yacine Bandou 2017-11-27 03:29:22 PST
(In reply to Michael Catanzaro from comment #6)
> Comment on attachment 327550 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=327550&action=review
> 
> I was going to give this r- for the reason I mentioned above, and ask you to
> change the runtime default to depend on ENABLE_ENCRYPTED_MEDIA instead...
> but now that Carlos Lopez has pointed out there is precedent for having API
> that does not work except in custom builds, I guess we might as well. But
> please do document that it as Carlos Lopez requested.
> 
> By the way, it's interesting that you've pointed out that it is currently
> impossible to enable encrypted media support. I wonder how long it has been
> since anybody has tried to test this. Probably a long time.
> 
> > Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:1305
> > +        g_param_spec_boolean("enable-encryptedmedia",
> 
> Please separate:
> 
> PROP_ENABLE_ENCRYPTED_MEDIA
> 
> "enable-encrypted-media"
> 
> > Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:3169
> > + * webkit_settings_get_enable_encryptedmedia:
> 
> encrypted_media for the function names too.
> 
> > Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:3174
> > + * Returns: %TRUE If EncryptedMedia support is enabled or %FALSE otherwise.
> 
> "if" (lowercase)

Thank you for the review, I have just attached a new patch with your proposals
Comment 9 Michael Catanzaro 2017-11-27 08:24:29 PST
Comment on attachment 327627 [details]
Patch

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

> Source/WebKit/ChangeLog:9
> +        This proprety is available only if the EncryptedMedia feature is enabled at build time by the ENABLE_ENCRYPTED_MEDIA flag.

property

> Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:1297
> +     * Enable or disable support for EncryptedMediaAPI on pages.

"encrypted media API"

> Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:1299
> +     * This proprety is available only if the EncryptedMedia feature is enabled at build time

property

> Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:1300
> +     * by the ENABLE_ENCRYPTED_MEDIA flag.

"with the"

> Source/WebKit/UIProcess/API/gtk/WebKitSettings.h:438
> +                                                                gboolean        enabled);

Looks like the alignment is a bit off here, the g in "gboolean" should be aligned with the W on the line above, not the (.

> Source/WebKit/UIProcess/API/wpe/WebKitSettings.h:422
> +                                                                gboolean        enabled);

Same here.
Comment 10 Yacine Bandou 2017-11-27 08:47:07 PST
Created attachment 327639 [details]
Patch
Comment 11 Michael Catanzaro 2017-11-27 10:20:38 PST
Comment on attachment 327639 [details]
Patch

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

> Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:1299
> +     * This property is available only if the EncryptedMedia feature is enabled at build time

One last change: "is available only" -> "will only work as intended". Since the property will actually always be available, it just won't do anything.
Comment 12 Yacine Bandou 2017-11-27 10:56:29 PST
Created attachment 327650 [details]
Patch
Comment 13 WebKit Commit Bot 2017-11-27 11:36:09 PST
Comment on attachment 327650 [details]
Patch

Clearing flags on attachment: 327650

Committed r225182: <https://trac.webkit.org/changeset/225182>
Comment 14 WebKit Commit Bot 2017-11-27 11:36:11 PST
All reviewed patches have been landed.  Closing bug.