[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