Bug 180005 – [GTK][WPE] Add "enable-encryptedmedia" property to WebKitWebSettings

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Yacine Bandou]

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.

[Michael Catanzaro]

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?

[Olivier Blin]

(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.

[Yacine Bandou]

Created attachment 327550 [details]
Patch

[EWS Watchlist]

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

[Carlos Alberto Lopez Perez]

(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.

[Michael Catanzaro]

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)

[Yacine Bandou]

Created attachment 327627 [details]
Patch

[Yacine Bandou]

(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

[Michael Catanzaro]

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.

[Yacine Bandou]

Created attachment 327639 [details]
Patch

[Michael Catanzaro]

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.

[Yacine Bandou]

Created attachment 327650 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 327650 [details]
Patch

Clearing flags on attachment: 327650

Committed r225182: <https://trac.webkit.org/changeset/225182>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.