Bug 180005

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?

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

Created attachment 327550 [details] Patch

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

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

Created attachment 327627 [details] Patch

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

Created attachment 327639 [details] Patch

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.

Created attachment 327650 [details] Patch

Comment on attachment 327650 [details] Patch Clearing flags on attachment: 327650 Committed r225182: <https://trac.webkit.org/changeset/225182>

All reviewed patches have been landed. Closing bug.