Bug 199924 – Remote WebInspector should enable mock capture devices in UIProcess if doing it in WebProcess

• Component: WebRTC
• Status: RESOLVED FIXED

[youenn fablet]

Remote WebInspector should enable mock capture devices in UIProcess if doing it in WebProcess

[youenn fablet]

<rdar://problem/50552067>

[youenn fablet]

Created attachment 374430 [details]
Patch

[Devin Rousso]

Comment on attachment 374430 [details]
Patch

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

r-, due to the comment below

> Source/WebKit/UIProcess/WebInspectorProxy.cpp:602
> +    m_inspectedPage->preferences().setMockCaptureDevicesEnabled(enabled);
> +    MockRealtimeMediaSourceCenter::setMockRealtimeMediaSourceCenterEnabled(enabled);

This has a different effect than what `InspectorPageAgent::overrideSetting` does.  It uses a generated override `Optional<bool>` value that's added to `WebCore::Settings` to make sure that changing the inspector value doesn't actually change the underlying setting value.  It's an override on top of the value.

I think that we should do the same when generating `WebKit::Preferences` as well (probably by generating something like `FOR_EACH_WEBKIT_PREFERENCE_WITH_INSPECTOR_OVERRIDE`).  This way, we aren't overriding the preference's underlying value.

See <https://webkit.org/b/193813> for how it was done for `WebCore::Settings`.

FYI, I'd also have the same sort of logic/thinking as <https://webkit.org/b/199925> too.

[youenn fablet]

Comment on attachment 374430 [details]
Patch

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

>> Source/WebKit/UIProcess/WebInspectorProxy.cpp:602
>> +    MockRealtimeMediaSourceCenter::setMockRealtimeMediaSourceCenterEnabled(enabled);
> 
> This has a different effect than what `InspectorPageAgent::overrideSetting` does.  It uses a generated override `Optional<bool>` value that's added to `WebCore::Settings` to make sure that changing the inspector value doesn't actually change the underlying setting value.  It's an override on top of the value.
> 
> I think that we should do the same when generating `WebKit::Preferences` as well (probably by generating something like `FOR_EACH_WEBKIT_PREFERENCE_WITH_INSPECTOR_OVERRIDE`).  This way, we aren't overriding the preference's underlying value.
> 
> See <https://webkit.org/b/193813> for how it was done for `WebCore::Settings`.
> 
> FYI, I'd also have the same sort of logic/thinking as <https://webkit.org/b/199925> too.

I'll change the plumbery to pass an Optional<bool> and add override specifics for mock capture.

[youenn fablet]

Created attachment 374471 [details]
Patch

[youenn fablet]

Created attachment 374473 [details]
Patch

[Geoffrey Garen]

[9/181] Building CXX object Source/WebCore/CMakeFiles/WebCore.dir/__/__/DerivedSources/WebCore/unified-sources/UnifiedSource-84c9f43f-2.cpp.o
FAILED: Source/WebCore/CMakeFiles/WebCore.dir/__/__/DerivedSources/WebCore/unified-sources/UnifiedSource-84c9f43f-2.cpp.o 
/usr/lib/ccache/c++  -DBUILDING_GTK__=1 -DBUILDING_WEBKIT -DBUILDING_WITH_CMAKE=1 -DBUILDING_WebCore -DBWRAP_EXECUTABLE=\"/usr/bin/bwrap\" -DDBUS_PROXY_EXECUTABLE=\"/home/ews/worker/GTK-Webkit2-EWS/build/WebKitBuild/DependenciesGTK/Root/bin/xdg-dbus-proxy\" -DGETTEXT_PACKAGE=\"WebKit2GTK-4.0\" -DHAVE_CONFIG_H=1 -DJSC_GLIB_API_ENABLED -DSTATICALLY_LINKED_WITH_PAL=1 -DWEBKITGTK_API_VERSION_STRING=\"4.0\" -IDerivedSources/ForwardingHeaders -I../../Source/WebCore/platform/graphics/libwpe -I. -IDerivedSources/WebCore -I../../Source/WebCore -I../../Source/WebCore/Modules/airplay -I../../Source/WebCore/Modules/applepay -I../../Source/WebCore/Modules/applepay/paymentrequest -I../../Source/WebCore/Modules/applicationmanifest -I../../Source/WebCore/Modules/beacon -I../../Source/WebCore/Modules/cache -I../../Source/WebCore/Modules/credentialmanagement -I../../Source/WebCore/Modules/encryptedmedia -I../../Source/WebCore/Modules/encryptedmedia/legacy -I../../Source/WebCore/Modules/entriesapi -I../../Source/WebCore/Modules/fetch -I../../Source/WebCore/Modules/geolocation -I../../Source/WebCore/Modules/indexeddb -I../../Source/WebCore/Modules/indexeddb/client -I../../Source/WebCore/Modules/indexeddb/server -I../../Source/WebCore/Modules/indexeddb/shared -I../../Source/WebCore/Modules/mediacapabilities -I../../Source/WebCore/Modules/mediacontrols -I../../Source/WebCore/Modules/mediarecorder -I../../Source/WebCore/Modules/mediasession -I../../Source/WebCore/Modules/mediasource -I../../Source/WebCore/Modules/mediastream -I../../Source/WebCore/Modules/mediastream/libwebrtc -I../../Source/WebCore/Modules/navigatorcontentutils -I../../Source/WebCore/Modules/notifications -I../../Source/WebCore/Modules/paymentrequest -I../../Source/WebCore/Modules/plugins -I../../Source/WebCore/Modules/quota -I../../Source/WebCore/Modules/speech -I../../Source/WebCore/Modules/streams -I../../Source/WebCore/Modules/webaudio -I../../Source/WebCore/Modules/webauthn -I../../Source/WebCore/Modules/webauthn/cbor -I../../Source/WebCore/Modules/webauthn/fido -I../../Source/WebCore/Modules/webdatabase -I../../Source/WebCore/Modules/webdriver -I../../Source/WebCore/Modules/webgpu -I../../Source/WebCore/Modules/webgpu/WHLSL -I../../Source/WebCore/Modules/webgpu/WHLSL/AST -I../../Source/WebCore/Modules/websockets -I../../Source/WebCore/Modules/webvr -I../../Source/WebCore/accessibility -I../../Source/WebCore/accessibility/isolatedtree -I../../Source/WebCore/animation -I../../Source/WebCore/bindings -I../../Source/WebCore/bindings/js -I../../Source/WebCore/bridge -I../../Source/WebCore/bridge/c -I../../Source/WebCore/bridge/jsc -I../../Source/WebCore/contentextensions -I../../Source/WebCore/crypto -I../../Source/WebCore/crypto/algorithms -I../../Source/WebCore/crypto/keys -I../../Source/WebCore/crypto/parameters -I../../Source/WebCore/css -I../../Source/WebCore/css/parser -I../../Source/WebCore/css/typedom -I../../Source/WebCore/cssjit -I../../Source/WebCore/dom -I../../Source/WebCore/dom/messageports -I../../Source/WebCore/domjit -I../../Source/WebCore/editing -I../../Source/WebCore/fileapi -I../../Source/WebCore/history -I../../Source/WebCore/html -I../../Source/WebCore/html/canvas -I../../Source/WebCore/html/forms -I../../Source/WebCore/html/parser -I../../Source/WebCore/html/shadow -I../../Source/WebCore/html/track -I../../Source/WebCore/inspector -I../../Source/WebCore/inspector/agents -I../../Source/WebCore/inspector/agents/page -I../../Source/WebCore/inspector/agents/worker -I../../Source/WebCore/layout -I../../Source/WebCore/layout/blockformatting -I../../Source/WebCore/layout/displaytree -I../../Source/WebCore/layout/floats -I../../Source/WebCore/layout/inlineformatting -I../../Source/WebCore/layout/inlineformatting/text -I../../Source/WebCore/layout/layouttree -I../../Source/WebCore/loader -I../../Source/WebCore/loader/appcache -I../../Source/WebCore/loader/archive -I../../Source/WebCore/loader/archive/mhtml -I../../Source/WebCore/loader/cache -I../../Source/WebCore/loader/icon -I../../Source/WebCore/mathml -I../../Source/WebCore/page -I../../Source/WebCore/page/animation -I../../Source/WebCore/page/csp -I../../Source/WebCore/page/scrolling -I../../Source/WebCore/platform -I../../Source/WebCore/platform/animation -I../../Source/WebCore/platform/audio -I../../Source/WebCore/platform/encryptedmedia -I../../Source/WebCore/platform/gamepad -I../../Source/WebCore/platform/graphics -I../../Source/WebCore/platform/graphics/cpu/arm -I../../Source/WebCore/platform/graphics/cpu/arm/filters -I../../Source/WebCore/platform/graphics/displaylists -I../../Source/WebCore/platform/graphics/filters -I../../Source/WebCore/platform/graphics/iso -I../../Source/WebCore/platform/graphics/opentype -I../../Source/WebCore/platform/graphics/transforms -I../../Source/WebCore/platform/mediacapabilities -I../../Source/WebCore/platform/mediarecorder -I../../Source/WebCore/platform/mediasession -I../../Source/WebCore/platform/mediastream -I../../Source/WebCore/platform/mediastream/libwebrtc -I../../Source/WebCore/platform/mock -I../../Source/WebCore/platform/mock/mediasource -I../../Source/WebCore/platform/network -I../../Source/WebCore/platform/sql -I../../Source/WebCore/platform/text -I../../Source/WebCore/platform/vr -I../../Source/WebCore/plugins -I../../Source/WebCore/rendering -I../../Source/WebCore/rendering/line -I../../Source/WebCore/rendering/mathml -I../../Source/WebCore/rendering/shapes -I../../Source/WebCore/rendering/style -I../../Source/WebCore/rendering/svg -I../../Source/WebCore/rendering/updating -I../../Source/WebCore/replay -I../../Source/WebCore/storage -I../../Source/WebCore/style -I../../Source/WebCore/svg -I../../Source/WebCore/svg/animation -I../../Source/WebCore/svg/graphics -I../../Source/WebCore/svg/graphics/filters -I../../Source/WebCore/svg/properties -I../../Source/WebCore/websockets -I../../Source/WebCore/workers -I../../Source/WebCore/workers/service -I../../Source/WebCore/workers/service/context -I../../Source/WebCore/workers/service/server -I../../Source/WebCore/worklets -I../../Source/WebCore/xml -I../../Source/WebCore/xml/parser -I../../Source/WebCore/platform/graphics/gpu -I../../Source/ThirdParty/xdgmime/src -I../../Source/WebCore/platform/graphics/cairo -I../../Source/WebCore/platform/graphics/freetype -I../../Source/WebCore/platform/graphics/harfbuzz -I../../Source/WebCore/platform/graphics/harfbuzz/ng -I../../Source/WebCore/platform/graphics/gstreamer -I../../Source/WebCore/platform/graphics/gstreamer/mse -I../../Source/WebCore/platform/graphics/gstreamer/eme -I../../Source/WebCore/platform/audio/gstreamer -I../../Source/WebCore/platform/encryptedmedia/clearkey -I../../Source/WebCore/platform/image-decoders -I../../Source/WebCore/platform/image-decoders/bmp -I../../Source/WebCore/platform/image-decoders/gif -I../../Source/WebCore/platform/image-decoders/ico -I../../Source/WebCore/platform/image-decoders/jpeg -I../../Source/WebCore/platform/image-decoders/jpeg2000 -I../../Source/WebCore/platform/image-decoders/png -I../../Source/WebCore/platform/image-decoders/webp -I../../Source/WebCore/platform/network/soup -I../../Source/WebCore/platform/graphics/texmap -I../../Source/WebCore/page/scrolling/nicosia -I../../Source/WebCore/platform/graphics/texmap/coordinated -I../../Source/WebCore/platform/graphics/nicosia -I../../Source/WebCore/platform/graphics/nicosia/cairo -I../../Source/WebCore/platform/graphics/nicosia/texmap -I../../Source/WebCore/accessibility/atk -I../../Source/WebCore/editing/atk -I../../Source/WebCore/page/gtk -I../../Source/WebCore/platform/generic -I../../Source/WebCore/platform/gtk -I../../Source/WebCore/platform/graphics/egl -I../../Source/WebCore/platform/graphics/glx -I../../Source/WebCore/platform/graphics/gtk -I../../Source/WebCore/platform/graphics/opengl -I../../Source/WebCore/platform/graphics/wayland -I../../Source/WebCore/platform/graphics/x11 -I../../Source/WebCore/platform/mediastream/gtk -I../../Source/WebCore/platform/mediastream/gstreamer -I../../Source/WebCore/platform/network/gtk -I../../Source/WebCore/platform/text/gtk -IDerivedSources -I../../Source/ThirdParty -isystem ../DependenciesGTK/Root/include/libxml2 -isystem ../../Source/ThirdParty/libwebrtc/Source -isystem ../../Source/ThirdParty/libwebrtc/Source/webrtc -isystem ../../Source/ThirdParty/libwebrtc/Source/third_party/abseil-cpp -isystem ../DependenciesGTK/Root/include/cairo -isystem ../DependenciesGTK/Root/include/freetype2 -isystem ../DependenciesGTK/Root/include/harfbuzz -isystem ../DependenciesGTK/Root/include/gstreamer-1.0 -isystem ../DependenciesGTK/Root/include/glib-2.0 -isystem ../DependenciesGTK/Root/lib/glib-2.0/include -isystem ../DependenciesGTK/Root/include/orc-0.4 -isystem ../DependenciesGTK/Root/lib/gstreamer-1.0/include -isystem ../DependenciesGTK/Root/include/openjpeg-2.3 -isystem ../DependenciesGTK/Root/include/libsoup-2.4 -isystem ../DependenciesGTK/Root/include/atk-1.0 -isystem /usr/include/enchant -isystem ../DependenciesGTK/Root/include/gtk-3.0 -isystem ../DependenciesGTK/Root/include/gio-unix-2.0 -isystem ../DependenciesGTK/Root/include/pango-1.0 -isystem ../DependenciesGTK/Root/include/gdk-pixbuf-2.0 -isystem ../DependenciesGTK/Root/include/pixman-1 -isystem /usr/include/libpng16 -isystem /usr/include/uuid -isystem /usr/include/libdrm -isystem ../DependenciesGTK/Root/include/at-spi2-atk/2.0 -isystem ../DependenciesGTK/Root/include/at-spi-2.0 -isystem /usr/include/dbus-1.0 -isystem /usr/lib/x86_64-linux-gnu/dbus-1.0/include -isystem /usr/include/libsecret-1 -isystem ../DependenciesGTK/Root/include/wpe-1.0 -fdiagnostics-color=always -Wextra -Wall -Wno-expansion-to-defined -Wno-psabi -Wno-noexcept-type -Wno-maybe-uninitialized -Wwrite-strings -Wundef -Wpointer-arith -Wmissing-format-attribute -Wformat-security -Wcast-align  -fno-strict-aliasing -fno-exceptions -fno-rtti -O3 -DNDEBUG -fPIC   -std=c++17 -MD -MT Source/WebCore/CMakeFiles/WebCore.dir/__/__/DerivedSources/WebCore/unified-sources/UnifiedSource-84c9f43f-2.cpp.o -MF Source/WebCore/CMakeFiles/WebCore.dir/__/__/DerivedSources/WebCore/unified-sources/UnifiedSource-84c9f43f-2.cpp.o.d -o Source/WebCore/CMakeFiles/WebCore.dir/__/__/DerivedSources/WebCore/unified-sources/UnifiedSource-84c9f43f-2.cpp.o -c DerivedSources/WebCore/unified-sources/UnifiedSource-84c9f43f-2.cpp
In file included from ../../Source/WebCore/inspector/InspectorClient.cpp:32,
                 from DerivedSources/WebCore/unified-sources/UnifiedSource-84c9f43f-2.cpp:1:
../../Source/WebCore/inspector/InspectorClient.h:65:47: error: ‘<anonymous>’ has incomplete type
     virtual void setMockCaptureDevicesEnabled(Optional<bool>) { }
                                               ^~~~~~~~~~~~~~
In file included from ../../Source/WebCore/inspector/InspectorClient.h:29,
                 from ../../Source/WebCore/inspector/InspectorClient.cpp:32,
                 from DerivedSources/WebCore/unified-sources/UnifiedSource-84c9f43f-2.cpp:1:
DerivedSources/ForwardingHeaders/wtf/Forward.h:58:26: note: declaration of ‘class WTF::Optional<bool>’
 template<typename> class Optional;
                          ^~~~~~~~

[youenn fablet]

Created attachment 374479 [details]
Patch

[youenn fablet]

Created attachment 374490 [details]
Patch

[EWS Watchlist]

Comment on attachment 374490 [details]
Patch

Attachment 374490 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/12775072

New failing tests:
http/tests/css/filters-on-iframes-transform.html

[EWS Watchlist]

Created attachment 374497 [details]
Archive of layout-test-results from ews215 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews215  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

[Devin Rousso]

Comment on attachment 374490 [details]
Patch

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

r=me, with some fixes (before you land)

> Source/WebCore/inspector/InspectorClient.h:66
> +    virtual void setMockCaptureDevicesEnabled(Optional<bool>) { }

I'd call this `setMockCaptureDevicesEnabledOverride` to match the naming "convention" of the inspector `WebCore::Settings` overrides.

Should this also be guarded by `ENABLE(MEDIA_STREAM)`?

Both of these also apply to all of the other occurrences in other files as well.

> Source/WebCore/inspector/agents/InspectorPageAgent.cpp:391
> +    m_client->setMockCaptureDevicesEnabled({ });

Please use `WTF::nullopt`, so it's clear that you're trying to remove the override (this also matches other uses like this in this file).

> Source/WebCore/inspector/agents/InspectorPageAgent.cpp:-451
> -    ASSERT_NOT_REACHED();

Please add a `default:` case to ensure that we don't miss any `enum` values.

> Source/WebKit/WebProcess/WebPage/WebInspector.cpp:289
> +    WebProcess::singleton().parentProcessConnection()->send(Messages::WebInspectorProxy::SetMockCaptureDevicesEnabled(enabled), m_page->pageID());

Rather than have a special message just for this, we could also have an `enum` of all the `WebKit::WebPreferences` inspector overrides and pass that all to a single `Messages::WebInspectorProxy::SetWebPreferenceOverride(MockCaptureDevicesEnabled, enabled)`.

I could do this in a followup though, so it's fine as is for now.

[youenn fablet]

> I'd call this `setMockCaptureDevicesEnabledOverride` to match the naming
> "convention" of the inspector `WebCore::Settings` overrides.

OK

> Should this also be guarded by `ENABLE(MEDIA_STREAM)`?

It could.

> Both of these also apply to all of the other occurrences in other files as
> well.
> 
> > Source/WebCore/inspector/agents/InspectorPageAgent.cpp:391
> > +    m_client->setMockCaptureDevicesEnabled({ });
> 
> Please use `WTF::nullopt`, so it's clear that you're trying to remove the
> override (this also matches other uses like this in this file).

OK, I usually prefer { } though, it is smaller and works for both optional and Optional.

> > Source/WebCore/inspector/agents/InspectorPageAgent.cpp:-451
> > -    ASSERT_NOT_REACHED();
> 
> Please add a `default:` case to ensure that we don't miss any `enum` values.

We usually do not do that so that the compiler will tell us we are missing a value whenever we are adding a new enum value.

> > Source/WebKit/WebProcess/WebPage/WebInspector.cpp:289
> > +    WebProcess::singleton().parentProcessConnection()->send(Messages::WebInspectorProxy::SetMockCaptureDevicesEnabled(enabled), m_page->pageID());
> 
> Rather than have a special message just for this, we could also have an
> `enum` of all the `WebKit::WebPreferences` inspector overrides and pass that
> all to a single
> `Messages::WebInspectorProxy::
> SetWebPreferenceOverride(MockCaptureDevicesEnabled, enabled)`.
> 
> I could do this in a followup though, so it's fine as is for now.

Agreed this might be better once we have a generic WebPreferences override mechanism.

[youenn fablet]

Created attachment 374525 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 374525 [details]
Patch for landing

Clearing flags on attachment: 374525

Committed r247662: <https://trac.webkit.org/changeset/247662>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[youenn fablet]

https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=inspector/page/overrideSetting-MockCaptureDevicesEnabled.html indicates that inspector/page/overrideSetting-MockCaptureDevicesEnabled.html is broken now.

I cannot reproduce it locally.

[youenn fablet]

(In reply to youenn fablet from comment #17)
> https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.
> html#showAllRuns=true&tests=inspector/page/overrideSetting-
> MockCaptureDevicesEnabled.html indicates that
> inspector/page/overrideSetting-MockCaptureDevicesEnabled.html is broken now.
> 
> I cannot reproduce it locally.

Looking at the bots, they do not have cameras hence the difference.
Filed https://bugs.webkit.org/show_bug.cgi?id=200017