Bug 188704 – [GTK][WPE] Make sure MediaDeviceEnabled and PeerConnectionEnabled are always synced with enable-media-stream

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Thibault Saunier]

[GTK][WPE] Make sure MediaDeviceEnabled and PeerConnectionEnabled are always synced with enable-media-stream

[Thibault Saunier]

Created attachment 347376 [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

[Michael Catanzaro]

Comment on attachment 347376 [details]
Patch

If the previous value was enabled, and the other settings were not already enabled, there is a bug. If the previous value was disabled, and the other settings were not already disabled, there is another bug.

Looks to me like the problem here was the initial states of these settings, which should be addressed by changing the default values of the member variables, right?

Might also be worth considering whether it makes sense to have three different settings here.

[Thibault Saunier]

Those a(In reply to Michael Catanzaro from comment #3)
> Comment on attachment 347376 [details]
> Patch
> 
> If the previous value was enabled, and the other settings were not already
> enabled, there is a bug. If the previous value was disabled, and the other
> settings were not already disabled, there is another bug.
> 
> Looks to me like the problem here was the initial states of these settings,
> which should be addressed by changing the default values of the member
> variables, right?

Those are the defaults in RuntimeEnabledFeatures.h:

```
#if ENABLE(MEDIA_STREAM)
    bool m_isMediaDevicesEnabled { false };
    bool m_isMediaStreamEnabled { true };
    bool m_isScreenCaptureEnabled { false };
#endif
``` 

> Might also be worth considering whether it makes sense to have three
> different settings here.

It is API now?

[Michael Catanzaro]

(In reply to Thibault Saunier from comment #4) 
> Those are the defaults in RuntimeEnabledFeatures.h:
> 
> ```
> #if ENABLE(MEDIA_STREAM)
>     bool m_isMediaDevicesEnabled { false };
>     bool m_isMediaStreamEnabled { true };
>     bool m_isScreenCaptureEnabled { false };
> #endif
> ``` 

If they only need to be synced at the GLib API layer, then I would do it in WebKitSettings's constructed function.

> > Might also be worth considering whether it makes sense to have three
> > different settings here.
> 
> It is API now?

There are two ways to read your question:

(a) Is it an API guarantee that webkit_settings_set_enable_media_stream() should also call setMediaDevicesEnabled() and setPeerConnectionEnabled()? No it is not, because webkit_settings_set_enable_media_stream() has never done anything in releases, because media stream has been disabled at build time in every release we have ever made. We've never even made a release with libwebrtc included. So no, we can change its behavior however we want without breaking anything. (Adding this function to the API before it was ready was a huge mistake that we should not repeat. It's very confusing to have API functions that do nothing, especially when the documentation doesn't indicate that at all. And it's been like this for four years now.)

(b) Are the setMediaDevicesEnabled/setMediaStreamEnabled/setPeerConnectionEnabled  themselves API? I see mediaDevicesEnabled exposed in WKPreferencesPrivate.h, which tells me it's private for Cocoa (used only by the TestRunner). Same for mediaStreamEnabled and screenCaptureEnabled. Unless I'm missing something, I think we can change it as desired. Don't be confused by WKPreferencesRef.h: the C API is also private API for the TestRunner. Only the Cocoa and GLib APIs are public.

So we should consider:

 * Merging the three internal settings together, if it makes sense to have only one exposed in the public API
 * Or making the other two settings public, if it makes sense to have all three internally

as avoiding mismatch between the public and internal settings is desirable.

But anyway, you don't have to do any of that in this bug, or even at all. It's just an idle thought.

[Thibault Saunier]

`enable-media-stream` is a Construct property, so basically here I am guaranteeing that properties are in sync at construct time, even though lower might decide otherwise.

To me having 3 settings is overkill and having 1 is enough.

[Michael Catanzaro]

(In reply to Thibault Saunier from comment #6)
> `enable-media-stream` is a Construct property, so basically here I am
> guaranteeing that properties are in sync at construct time, even though
> lower might decide otherwise.

TBH I don't understand your argument. Removing the early return is not good. I understand that it works here since it gets called at construct time, but it would be much better to be explicit and sync the settings in constructed instead.

That will also avoid the future situation where somebody notices the early return is missing and adds it back.

[Thibault Saunier]

Created attachment 347492 [details]
Patch

[Thibault Saunier]

(In reply to Thibault Saunier from comment #8)
> Created attachment 347492 [details]
> Patch

Just did what you asked for, makes sense indeed.

[EWS Watchlist]

Comment on attachment 347492 [details]
Patch

Attachment 347492 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/8916583

New failing tests:
fetch/fetch-worker-crash.html

[EWS Watchlist]

Created attachment 347493 [details]
Archive of layout-test-results from ews102 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Thibault Saunier]

Unrelated issue?

[Michael Catanzaro]

yes

[Thibault Saunier]

Created attachment 350252 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 350252 [details]
Patch for landing

Clearing flags on attachment: 350252

Committed r236279: <https://trac.webkit.org/changeset/236279>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/44653748>