[webkit-reviews] review granted: [Bug 199924] Remote WebInspector should enable mock capture devices in UIProcess if doing it in WebProcess : [Attachment 374490] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Jul 19 15:43:00 PDT 2019
Devin Rousso <drousso at apple.com> has granted youenn fablet
<youennf at gmail.com>'s request for review:
Bug 199924: Remote WebInspector should enable mock capture devices in UIProcess
if doing it in WebProcess
https://bugs.webkit.org/show_bug.cgi?id=199924
Attachment 374490: Patch
https://bugs.webkit.org/attachment.cgi?id=374490&action=review
--- Comment #12 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 374490
--> https://bugs.webkit.org/attachment.cgi?id=374490
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::WebInspectorP
roxy::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(MockCaptureDevicesEnable
d, enabled)`.
I could do this in a followup though, so it's fine as is for now.
More information about the webkit-reviews
mailing list