Bug 193863 – Web Inspector: provide a way to edit page WebRTC settings on a remote target

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Devin Rousso]

This includes:
 - Allow Media Capture on Insecure Sites
 - Disable ICE Candidate Restrictions
 - Use Mock Capture Devices

[Radar WebKit Bug Importer]

<rdar://problem/47572764>

[Devin Rousso]

Created attachment 360232 [details]
Patch

[EWS Watchlist]

Comment on attachment 360232 [details]
Patch

Attachment 360232 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/10899696

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 360233 [details]
Archive of layout-test-results from ews107 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 360232 [details]
Patch

Attachment 360232 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/10899759

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 360234 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[Devin Rousso]

Created attachment 360257 [details]
Patch

[Devin Rousso]

Created attachment 360259 [details]
Patch

[EWS Watchlist]

Comment on attachment 360259 [details]
Patch

Attachment 360259 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/10905906

New failing tests:
inspector/page/overrideSetting.html

[EWS Watchlist]

Created attachment 360260 [details]
Archive of layout-test-results from ews100 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 360259 [details]
Patch

Attachment 360259 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/10906871

New failing tests:
inspector/page/overrideSetting.html

[EWS Watchlist]

Created attachment 360267 [details]
Archive of layout-test-results from ews114 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[Devin Rousso]

Created attachment 360269 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 360269 [details]
Patch

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

Looks good but a few questions.

> Source/WebCore/inspector/agents/InspectorPageAgent.cpp:337
> +    m_page.enableICECandidateFiltering();

I don't think this is what we want. Shouldn't we be removing some kind of ICECandidateFiltering override? This seems to always enable it again when Web Inspector closes.

> Source/WebInspectorUI/UserInterface/Base/Main.js:2172
> +    if (PageAgent.Setting.ImagesEnabled || PageAgent.Setting.AuthorAndUserStylesEnabled || PageAgent.Setting.ScriptEnabled || PageAgent.Setting.NeedsSiteSpecificQuirks || PageAgent.Setting.WebSecurityEnabled) {

I don't think you need to check all of these. And maybe if you do they should be `InspectorBackend.domains.Page.Setting` checks instead of `PageAgent.Setting` checks.

> Source/WebKit/Shared/WebPreferencesDefaultValues.h:-119
> -#define DEFAULT_MOCK_CAPTURE_DEVICES_ENABLED true
>  #else
>  #define DEFAULT_ACCELERATED_DRAWING_ENABLED true
>  #define DEFAULT_CANVAS_USES_ACCELERATED_DRAWING true
> -#define DEFAULT_MOCK_CAPTURE_DEVICES_ENABLED false

Why are we changing this default value in this patch?

[Devin Rousso]

Comment on attachment 360269 [details]
Patch

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

>> Source/WebCore/inspector/agents/InspectorPageAgent.cpp:337
>> +    m_page.enableICECandidateFiltering();
> 
> I don't think this is what we want. Shouldn't we be removing some kind of ICECandidateFiltering override? This seems to always enable it again when Web Inspector closes.

The weird thing with this setting is that it only seemed to be called during the creation of a `WebPage`, and only ever to turn it off.  Changing the preference didn't seem to have any effect for existing pages.  I'll rework it with a setting (and see if I can write a test for it ).

>> Source/WebKit/Shared/WebPreferencesDefaultValues.h:-119
>> -#define DEFAULT_MOCK_CAPTURE_DEVICES_ENABLED false
> 
> Why are we changing this default value in this patch?

I noticed that the underlying `DeprecatedGlobalSettings` didn't follow this same pattern, so I figured it should match.  I'll change this back.

[Devin Rousso]

Created attachment 360356 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 360356 [details]
Patch

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

Nice. r=me

> Source/WebCore/page/SettingsBase.cpp:313
> +

Style: Unexpected blank line

[Devin Rousso]

Created attachment 360372 [details]
Patch

[Devin Rousso]

Created attachment 360428 [details]
Patch

Match `Settings` initial value with the original `DeprecatedGlobalSettings` initial value

[WebKit Commit Bot]

Comment on attachment 360428 [details]
Patch

Clearing flags on attachment: 360428

Committed r240644: <https://trac.webkit.org/changeset/240644>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Truitt Savell]

The test inspector/page/overrideSetting-MockCaptureDevicesEnabled.html

added in https://trac.webkit.org/changeset/240644/webkit

is failing on Mojave WK1 from introduction.

History:
https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=inspector%2Fpage%2FoverrideSetting-MockCaptureDevicesEnabled.html

Diff:
--- /Volumes/Data/slave/mojave-release-tests-wk1/build/layout-test-results/inspector/page/overrideSetting-MockCaptureDevicesEnabled-expected.txt
+++ /Volumes/Data/slave/mojave-release-tests-wk1/build/layout-test-results/inspector/page/overrideSetting-MockCaptureDevicesEnabled-actual.txt
@@ -3,9 +3,7 @@
 
 == Running test suite: Page.overrideSetting
 -- Running test case: Page.overrideSetting.MockCaptureDevicesEnabled
-Expected Error: The I/O read operation failed.
-Overriding MockCaptureDevicesEnabled to true...
-PASS: getUserMedia should not fail when no devices are available
-Removing MockCaptureDevicesEnabled override...
-Expected Error: The I/O read operation failed.
+!! EXCEPTION: TypeError: undefined is not an object (evaluating 'navigator.mediaDevices.getUserMedia')
+Stack Trace: #0: (anonymous) (TestCombined.js:1424:52)
+#1: promiseReactionJob [native code]

[Devin Rousso]

(In reply to Truitt Savell from comment #22)
I skipped the test on WK1, as it appears that `MEDIA_STREAM` isn't yet supported on WK1 .

<https://trac.webkit.org/changeset/240664>

[Truitt Savell]

inspector/page/overrideSetting-ICECandidateFilteringEnabled.html is also a flakey timeout on Mojave WK1 

History:
https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=inspector%2Fpage%2FoverrideSetting-ICECandidateFilteringEnabled.html