Remote WebInspector should enable mock capture devices in UIProcess if doing it in WebProcess
<rdar://problem/50552067>
Created attachment 374430 [details] Patch
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.
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.
Created attachment 374471 [details] Patch
Created attachment 374473 [details] Patch
[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; ^~~~~~~~
Created attachment 374479 [details] Patch
Created attachment 374490 [details] Patch
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
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
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.
> 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.
Created attachment 374525 [details] Patch for landing
Comment on attachment 374525 [details] Patch for landing Clearing flags on attachment: 374525 Committed r247662: <https://trac.webkit.org/changeset/247662>
All reviewed patches have been landed. Closing bug.
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.
(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