Bug 216872 – Introduce WKWebViewConfiguration mediaCaptureEnabled

• Component: WebRTC
• Status: RESOLVED FIXED

[youenn fablet]

Introduce WKWebViewConfiguration mediaDevicesEnabled

[youenn fablet]

Created attachment 409463 [details]
Patch

[youenn fablet]

Created attachment 409465 [details]
Patch

[Sam Weinig]

Comment on attachment 409465 [details]
Patch

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

> Source/WebKit/ChangeLog:11
> +        Add a new boolean configuration to enable/disable mediaDevices exposure.
> +        Value is off by default.
> +        A future patch will remove the WebPreferences based API, given there is no need
> +        to enable/disable mediaDevices throughout the course of a page.

Can you explain a bit more about why you are moving this? Most feature enablement is currently done through the preferences mechanism, so having this one be on the configuration seems odd and inconsistent. 

If there is new thought process around this, it would be good to have that articulated so we can start the process of migrating everything.

[youenn fablet]

(In reply to Sam Weinig from comment #3)
> Comment on attachment 409465 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=409465&action=review
> 
> > Source/WebKit/ChangeLog:11
> > +        Add a new boolean configuration to enable/disable mediaDevices exposure.
> > +        Value is off by default.
> > +        A future patch will remove the WebPreferences based API, given there is no need
> > +        to enable/disable mediaDevices throughout the course of a page.
> 
> Can you explain a bit more about why you are moving this? Most feature
> enablement is currently done through the preferences mechanism, so having
> this one be on the configuration seems odd and inconsistent. 

I am not sure how much this is inconsistent.
There is not much API available in WKPreferences.h.
There are some precedents in WKWebViewConfiguration.h, like allowsAirPlayForMediaPlayback.

> If there is new thought process around this, it would be good to have that
> articulated so we can start the process of migrating everything.

WebPreferences can be changed during the lifetime of the page.
This can be useful, say you want to enable/disable mock devices.

In that particular case, I see no use case for enabling/disabling this setting during the lifetime of the page. There are some downsides of allowing it (some contexts may have navigator.mediaDevices, some may not).
It seems best to enforce a fixed setting hence the idea to move it to WKWebViewConfiguration.
Wdyt?

[Alex Christensen]

Comment on attachment 409465 [details]
Patch

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

> Source/WebKit/UIProcess/API/Cocoa/WKWebViewConfiguration.mm:172
> +    BOOL _mediaDevicesEnabled;

This should go on API::PageConfiguration instead.

> Source/WebKit/UIProcess/API/Cocoa/WKWebViewConfigurationPrivate.h:123
> +@property (nonatomic, setter=_setMediaDevicesEnabled:) BOOL _mediaDevicesEnabled WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA));

Could you deprecate the preference SPI with this as a replacement?

[youenn fablet]

(In reply to Alex Christensen from comment #5)
> Comment on attachment 409465 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=409465&action=review
> 
> > Source/WebKit/UIProcess/API/Cocoa/WKWebViewConfiguration.mm:172
> > +    BOOL _mediaDevicesEnabled;
> 
> This should go on API::PageConfiguration instead.

OK.

> > Source/WebKit/UIProcess/API/Cocoa/WKWebViewConfigurationPrivate.h:123
> > +@property (nonatomic, setter=_setMediaDevicesEnabled:) BOOL _mediaDevicesEnabled WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA));
> 
> Could you deprecate the preference SPI with this as a replacement?

Sure, I plan to remove the preference SPI as a follow-up though.

[youenn fablet]

Created attachment 409475 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/69440832>

[Sam Weinig]

(In reply to youenn fablet from comment #4)
> (In reply to Sam Weinig from comment #3)
> > Comment on attachment 409465 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=409465&action=review
> > 
> > > Source/WebKit/ChangeLog:11
> > > +        Add a new boolean configuration to enable/disable mediaDevices exposure.
> > > +        Value is off by default.
> > > +        A future patch will remove the WebPreferences based API, given there is no need
> > > +        to enable/disable mediaDevices throughout the course of a page.
> > 
> > Can you explain a bit more about why you are moving this? Most feature
> > enablement is currently done through the preferences mechanism, so having
> > this one be on the configuration seems odd and inconsistent. 
> 
> I am not sure how much this is inconsistent.
> There is not much API available in WKPreferences.h.
> There are some precedents in WKWebViewConfiguration.h, like
> allowsAirPlayForMediaPlayback.

The preferences expose all the preferences set in the WebPreferences*.yaml files. There are a ton of them. They don't each necessarily get their own corresponding WKPreferences API call site, but most can accessed via the enumerations for internal debug or experimental features accessors. 

> 
> > If there is new thought process around this, it would be good to have that
> > articulated so we can start the process of migrating everything.
> 
> WebPreferences can be changed during the lifetime of the page.
> This can be useful, say you want to enable/disable mock devices.
> 
> In that particular case, I see no use case for enabling/disabling this
> setting during the lifetime of the page. There are some downsides of
> allowing it (some contexts may have navigator.mediaDevices, some may not).
> It seems best to enforce a fixed setting hence the idea to move it to
> WKWebViewConfiguration.
> Wdyt?

It's hard to judge as I don't know what your intended use case for this is. Who is the target audience of this SPI?

[Radar WebKit Bug Importer]

<rdar://problem/69441004>

[youenn fablet]

> > In that particular case, I see no use case for enabling/disabling this
> > setting during the lifetime of the page. There are some downsides of
> > allowing it (some contexts may have navigator.mediaDevices, some may not).
> > It seems best to enforce a fixed setting hence the idea to move it to
> > WKWebViewConfiguration.
> > Wdyt?
> 
> It's hard to judge as I don't know what your intended use case for this is.
> Who is the target audience of this SPI?

Goal is not to stay at SPI.

[Alex Christensen]

Comment on attachment 409475 [details]
Patch

Actually, we may want to put this on WKWebpagePreferences.

[Alex Christensen]

Comment on attachment 409475 [details]
Patch

The WKUIDelegate needs to give permission, and that can be per-site.  This is great.

[youenn fablet]

We might want to rename mediaDevicesEnabled to mediaCaptureEnabled which intent is clearer.

[youenn fablet]

Created attachment 409676 [details]
Patch for landing

[youenn fablet]

Created attachment 409678 [details]
Patch for landing

[EWS]

Committed r267563: <https://trac.webkit.org/changeset/267563>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 409678 [details].

[youenn fablet]

Removing preference in https://bugs.webkit.org/show_bug.cgi?id=216970