Bug 172477 - Web Inspector: Add Debug view to Settings tab for debug settings and experimental features
Summary: Web Inspector: Add Debug view to Settings tab for debug settings and experime...
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:
Depends on:
Blocks:
 
Reported: 2017-05-22 15:57 PDT by Devin Rousso
Modified: 2017-05-31 13:54 PDT (History)
6 users (show)

See Also:


Attachments
Patch (23.32 KB, patch)
2017-05-22 16:00 PDT, Devin Rousso
joepeck: review-
Details | Formatted Diff | Diff
[Image] After Patch is applied (172.00 KB, image/png)
2017-05-22 16:01 PDT, Devin Rousso
no flags Details
Patch (23.08 KB, patch)
2017-05-31 10:56 PDT, Devin Rousso
joepeck: review+
joepeck: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews124 for ios-simulator-wk2 (10.80 MB, application/zip)
2017-05-31 13:11 PDT, Build Bot
no flags Details
Patch (23.09 KB, patch)
2017-05-31 13:25 PDT, 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 2017-05-22 15:57:01 PDT
Instead of using setters on InspectorBackend or items in the Toolbar, we should have a Debug view in the Settings tab for all development settings/options.
Comment 1 Devin Rousso 2017-05-22 16:00:55 PDT
Created attachment 310942 [details]
Patch
Comment 2 Devin Rousso 2017-05-22 16:01:22 PDT
Created attachment 310943 [details]
[Image] After Patch is applied
Comment 3 Matt Baker 2017-05-22 16:09:19 PDT
Looks good! Without a toolbar button, there will be visual cue when the Debug UI flag changes. My first thought was to show the Debug settings view when it becomes enabled, but that could be disruptive/annoying.

I agree that we need a settings page dedicated to this stuff, just a little hesitant about adding an extra step to reach it in the UI. Maybe this is a non-issue.
Comment 4 Joseph Pecoraro 2017-05-22 17:10:33 PDT
Comment on attachment 310942 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Debug/Bootstrap.js:-40
> -    let toolTip = "Enable dump inspector messages to console";

I still think having this toolbar button is a good idea.

    1. You can toggle it from anywhere, which is especially useful if you only want to debug protocol messages for a short period of time without switching to the settings tab and back.
    2. It is a global visual indicator that you have debug mode enabled.

What do others think?

> Source/WebInspectorUI/UserInterface/Views/SettingsTabContentView.js:173
> +    _createGeneralSettingsView()
> +    {
> +        let generalSettingsView = new WebInspector.SettingsView("general", WebInspector.UIString("General"));

I really like the idea of keeping these in their own file. It makes searching for the code much easier. You could have:

    • GeneralSettingsView
    • DebugSettingsView

And this code could all be lazy.

> Source/WebInspectorUI/UserInterface/Views/SettingsTabContentView.js:295
> +        this.setSettingsViewVisible(this._debugSettingsView, WebInspector.isDebugUIEnabled());

If the Debug tab is selected and then user triggers the keyboard shortcut to hide it, does selection properly change back to General?
Comment 5 Matt Baker 2017-05-22 17:16:54 PDT
Comment on attachment 310942 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/SettingsTabContentView.js:295
>> +        this.setSettingsViewVisible(this._debugSettingsView, WebInspector.isDebugUIEnabled());
> 
> If the Debug tab is selected and then user triggers the keyboard shortcut to hide it, does selection properly change back to General?

It should. I tested this scenario when implementing https://bugs.webkit.org/show_bug.cgi?id=171765.
Comment 6 BJ Burg 2017-05-23 10:18:13 PDT
Comment on attachment 310942 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Debug/Bootstrap.js:-40
>> -    let toolTip = "Enable dump inspector messages to console";
> 
> I still think having this toolbar button is a good idea.
> 
>     1. You can toggle it from anywhere, which is especially useful if you only want to debug protocol messages for a short period of time without switching to the settings tab and back.
>     2. It is a global visual indicator that you have debug mode enabled.
> 
> What do others think?

I have never used it dynamically like that. I always turn this on in conjunction with dumping console messages to stderr and read it via Terminal.app.

I do want to keep some sort of visual indicator of whether DebugUI is enabled or not. I had been using that icon, but we could add something else like a bug icon that jumps to debug settings or files a pre-filled bug report.
Comment 7 Devin Rousso 2017-05-23 11:19:49 PDT
(In reply to Brian Burg from comment #6)
> Comment on attachment 310942 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=310942&action=review
> 
> >> Source/WebInspectorUI/UserInterface/Debug/Bootstrap.js:-40
> >> -    let toolTip = "Enable dump inspector messages to console";
> > 
> > I still think having this toolbar button is a good idea.
> > 
> >     1. You can toggle it from anywhere, which is especially useful if you only want to debug protocol messages for a short period of time without switching to the settings tab and back.
> >     2. It is a global visual indicator that you have debug mode enabled.
> > 
> > What do others think?
> 
> I have never used it dynamically like that. I always turn this on in
> conjunction with dumping console messages to stderr and read it via
> Terminal.app.
> 
> I do want to keep some sort of visual indicator of whether DebugUI is
> enabled or not. I had been using that icon, but we could add something else
> like a bug icon that jumps to debug settings or files a pre-filled bug
> report.

How about making a debug button in the toolbar that opens the Debug view in settings when clicked?
Comment 8 Joseph Pecoraro 2017-05-23 13:22:44 PDT
> How about making a debug button in the toolbar that opens the Debug view in
> settings when clicked?

That makes (1) more difficult.
Comment 9 Joseph Pecoraro 2017-05-24 11:22:05 PDT
Comment on attachment 310942 [details]
Patch

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

>>>> Source/WebInspectorUI/UserInterface/Debug/Bootstrap.js:-40
>>>> -    let toolTip = "Enable dump inspector messages to console";
>>> 
>>> I still think having this toolbar button is a good idea.
>>> 
>>>     1. You can toggle it from anywhere, which is especially useful if you only want to debug protocol messages for a short period of time without switching to the settings tab and back.
>>>     2. It is a global visual indicator that you have debug mode enabled.
>>> 
>>> What do others think?
>> 
>> I have never used it dynamically like that. I always turn this on in conjunction with dumping console messages to stderr and read it via Terminal.app.
>> 
>> I do want to keep some sort of visual indicator of whether DebugUI is enabled or not. I had been using that icon, but we could add something else like a bug icon that jumps to debug settings or files a pre-filled bug report.
> 
> How about making a debug button in the toolbar that opens the Debug view in settings when clicked?

I'm going to r-. It doesn't cost us anything to keep this here. So lets just keep it here. Rest of the patch looks fine to me.
Comment 10 Devin Rousso 2017-05-31 10:56:39 PDT
Created attachment 311611 [details]
Patch

I re-added the toolbar item, but I also left the checkbox visible in the debug view.  I think it makes it clearer as to what debug settings are available by having them all in at least one place.
Comment 11 Joseph Pecoraro 2017-05-31 11:54:29 PDT
Comment on attachment 311611 [details]
Patch

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

r=me

> Source/WebInspectorUI/UserInterface/Views/SettingsTabContentView.js:240
> +        WebInspector.settings.autoLogProtocolMessages.addEventListener(WebInspector.Setting.Event.Changed, () => {
> +            autoLogProtocolMessagesEditor.value = InspectorBackend.dumpInspectorProtocolMessages;
> +        });

Is this necessary? Won't tracking the setting "just work" like the others? When InspectorBackend.dumpInspectorProtocolMessages changes, it changes the settings under the hood.

> Source/WebInspectorUI/UserInterface/Views/SettingsTabContentView.js:250
> +        // This setting is only ever shown in engineering builds, so its strings are unlocalized.

You could move this near the top of createDebugSettingsView since all of the strings in here will be unlocalized.
Comment 12 Build Bot 2017-05-31 13:11:10 PDT
Comment on attachment 311611 [details]
Patch

Attachment 311611 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3849594

New failing tests:
webrtc/peer-connection-audio-mute.html
Comment 13 Build Bot 2017-05-31 13:11:11 PDT
Created attachment 311623 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5
Comment 14 Devin Rousso 2017-05-31 13:24:49 PDT
Comment on attachment 311611 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/SettingsTabContentView.js:240
>> +        });
> 
> Is this necessary? Won't tracking the setting "just work" like the others? When InspectorBackend.dumpInspectorProtocolMessages changes, it changes the settings under the hood.

Yes, this is needed.  The majority of the settings are only editable from the Settings tab, so there is no need for them to listen to see if the underlying WI.Setting object changes.  This setting, however, can be changed from the toolbar, so we need to listen for changes from that and update the value of the editor accordingly.
Comment 15 Devin Rousso 2017-05-31 13:25:56 PDT
Created attachment 311626 [details]
Patch
Comment 16 WebKit Commit Bot 2017-05-31 13:54:53 PDT
Comment on attachment 311626 [details]
Patch

Clearing flags on attachment: 311626

Committed r217625: <http://trac.webkit.org/changeset/217625>
Comment 17 WebKit Commit Bot 2017-05-31 13:54:54 PDT
All reviewed patches have been landed.  Closing bug.