WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
199924
Remote WebInspector should enable mock capture devices in UIProcess if doing it in WebProcess
https://bugs.webkit.org/show_bug.cgi?id=199924
Summary
Remote WebInspector should enable mock capture devices in UIProcess if doing ...
youenn fablet
Reported
2019-07-18 16:58:38 PDT
Remote WebInspector should enable mock capture devices in UIProcess if doing it in WebProcess
Attachments
Patch
(12.69 KB, patch)
2019-07-18 17:07 PDT
,
youenn fablet
no flags
Details
Formatted Diff
Diff
Patch
(2.61 KB, patch)
2019-07-19 09:51 PDT
,
youenn fablet
no flags
Details
Formatted Diff
Diff
Patch
(16.59 KB, patch)
2019-07-19 10:55 PDT
,
youenn fablet
no flags
Details
Formatted Diff
Diff
Patch
(16.72 KB, patch)
2019-07-19 11:37 PDT
,
youenn fablet
no flags
Details
Formatted Diff
Diff
Patch
(16.95 KB, patch)
2019-07-19 13:24 PDT
,
youenn fablet
no flags
Details
Formatted Diff
Diff
Archive of layout-test-results from ews215 for win-future
(13.70 MB, application/zip)
2019-07-19 14:53 PDT
,
EWS Watchlist
no flags
Details
Patch for landing
(17.05 KB, patch)
2019-07-19 16:59 PDT
,
youenn fablet
no flags
Details
Formatted Diff
Diff
Show Obsolete
(5)
View All
Add attachment
proposed patch, testcase, etc.
youenn fablet
Comment 1
2019-07-18 16:59:13 PDT
<
rdar://problem/50552067
>
youenn fablet
Comment 2
2019-07-18 17:07:04 PDT
Created
attachment 374430
[details]
Patch
Devin Rousso
Comment 3
2019-07-18 17:47:23 PDT
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 4
2019-07-19 09:32:02 PDT
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
Comment 5
2019-07-19 09:51:05 PDT
Created
attachment 374471
[details]
Patch
youenn fablet
Comment 6
2019-07-19 10:55:49 PDT
Created
attachment 374473
[details]
Patch
Geoffrey Garen
Comment 7
2019-07-19 11:17:06 PDT
[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
Comment 8
2019-07-19 11:37:04 PDT
Created
attachment 374479
[details]
Patch
youenn fablet
Comment 9
2019-07-19 13:24:24 PDT
Created
attachment 374490
[details]
Patch
EWS Watchlist
Comment 10
2019-07-19 14:53:36 PDT
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
Comment 11
2019-07-19 14:53:39 PDT
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 12
2019-07-19 15:43:00 PDT
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
Comment 13
2019-07-19 16:38:20 PDT
> 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
Comment 14
2019-07-19 16:59:24 PDT
Created
attachment 374525
[details]
Patch for landing
WebKit Commit Bot
Comment 15
2019-07-19 17:55:22 PDT
Comment on
attachment 374525
[details]
Patch for landing Clearing flags on attachment: 374525 Committed
r247662
: <
https://trac.webkit.org/changeset/247662
>
WebKit Commit Bot
Comment 16
2019-07-19 17:55:24 PDT
All reviewed patches have been landed. Closing bug.
youenn fablet
Comment 17
2019-07-22 13:42:10 PDT
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
Comment 18
2019-07-22 15:28:12 PDT
(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
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug