Bug 193863 - Web Inspector: provide a way to edit page WebRTC settings on a remote target
Summary: Web Inspector: provide a way to edit page WebRTC settings on a remote target
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords: InRadar
Depends on: 193813
Blocks:
  Show dependency treegraph
 
Reported: 2019-01-26 00:53 PST by Devin Rousso
Modified: 2019-01-29 14:06 PST (History)
8 users (show)

See Also:


Attachments
Patch (22.57 KB, patch)
2019-01-26 01:33 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews107 for mac-highsierra-wk2 (2.43 MB, application/zip)
2019-01-26 02:41 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews122 for ios-simulator-wk2 (2.34 MB, application/zip)
2019-01-26 03:21 PST, EWS Watchlist
no flags Details
Patch (27.34 KB, patch)
2019-01-26 15:29 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (33.89 KB, patch)
2019-01-26 17:47 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-highsierra (2.42 MB, application/zip)
2019-01-26 18:55 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews114 for mac-highsierra (2.07 MB, application/zip)
2019-01-26 21:34 PST, EWS Watchlist
no flags Details
Patch (34.50 KB, patch)
2019-01-26 22:18 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (40.70 KB, patch)
2019-01-28 10:50 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (40.70 KB, patch)
2019-01-28 13:48 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (40.46 KB, patch)
2019-01-28 20:20 PST, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Devin Rousso 2019-01-26 00:53:24 PST
This includes:
 - Allow Media Capture on Insecure Sites
 - Disable ICE Candidate Restrictions
 - Use Mock Capture Devices
Comment 1 Radar WebKit Bug Importer 2019-01-26 00:53:39 PST
<rdar://problem/47572764>
Comment 2 Devin Rousso 2019-01-26 01:33:12 PST
Created attachment 360232 [details]
Patch
Comment 3 EWS Watchlist 2019-01-26 02:41:49 PST Comment hidden (obsolete)
Comment 4 EWS Watchlist 2019-01-26 02:41:50 PST Comment hidden (obsolete)
Comment 5 EWS Watchlist 2019-01-26 03:21:40 PST Comment hidden (obsolete)
Comment 6 EWS Watchlist 2019-01-26 03:21:41 PST Comment hidden (obsolete)
Comment 7 Devin Rousso 2019-01-26 15:29:29 PST
Created attachment 360257 [details]
Patch
Comment 8 Devin Rousso 2019-01-26 17:47:42 PST
Created attachment 360259 [details]
Patch
Comment 9 EWS Watchlist 2019-01-26 18:55:51 PST Comment hidden (obsolete)
Comment 10 EWS Watchlist 2019-01-26 18:55:53 PST Comment hidden (obsolete)
Comment 11 EWS Watchlist 2019-01-26 21:34:00 PST Comment hidden (obsolete)
Comment 12 EWS Watchlist 2019-01-26 21:34:01 PST Comment hidden (obsolete)
Comment 13 Devin Rousso 2019-01-26 22:18:24 PST
Created attachment 360269 [details]
Patch
Comment 14 Joseph Pecoraro 2019-01-27 12:28:06 PST
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?
Comment 15 Devin Rousso 2019-01-28 09:50:14 PST
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.
Comment 16 Devin Rousso 2019-01-28 10:50:50 PST
Created attachment 360356 [details]
Patch
Comment 17 Joseph Pecoraro 2019-01-28 13:39:41 PST
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
Comment 18 Devin Rousso 2019-01-28 13:48:35 PST
Created attachment 360372 [details]
Patch
Comment 19 Devin Rousso 2019-01-28 20:20:42 PST
Created attachment 360428 [details]
Patch

Match `Settings` initial value with the original `DeprecatedGlobalSettings` initial value
Comment 20 WebKit Commit Bot 2019-01-28 22:21:02 PST
Comment on attachment 360428 [details]
Patch

Clearing flags on attachment: 360428

Committed r240644: <https://trac.webkit.org/changeset/240644>
Comment 21 WebKit Commit Bot 2019-01-28 22:21:04 PST
All reviewed patches have been landed.  Closing bug.
Comment 22 Truitt Savell 2019-01-29 08:52:35 PST
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]
Comment 23 Devin Rousso 2019-01-29 10:40:41 PST
(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>
Comment 24 Truitt Savell 2019-01-29 14:06:04 PST
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