[GTK][WPE] Make sure MediaDeviceEnabled and PeerConnectionEnabled are always synced with enable-media-stream
Created attachment 347376 [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
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.
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?
(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.
`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.
(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.
Created attachment 347492 [details] Patch
(In reply to Thibault Saunier from comment #8) > Created attachment 347492 [details] > Patch Just did what you asked for, makes sense indeed.
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
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
Unrelated issue?
yes
Created attachment 350252 [details] Patch for landing
Comment on attachment 350252 [details] Patch for landing Clearing flags on attachment: 350252 Committed r236279: <https://trac.webkit.org/changeset/236279>
All reviewed patches have been landed. Closing bug.
<rdar://problem/44653748>