Bug 172477 – Web Inspector: Add Debug view to Settings tab for debug settings and experimental features

• Component: Web Inspector
• Status: RESOLVED FIXED

[Devin Rousso]

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.

[Devin Rousso]

Created attachment 310942 [details]
Patch

[Devin Rousso]

Created attachment 310943 [details]
[Image] After Patch is applied

[Matt Baker]

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.

[Joseph Pecoraro]

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?

[Matt Baker]

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.

[Blaze Burg]

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.

[Devin Rousso]

(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?

[Joseph Pecoraro]

> How about making a debug button in the toolbar that opens the Debug view in
> settings when clicked?

That makes (1) more difficult.

[Joseph Pecoraro]

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.

[Devin Rousso]

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.

[Joseph Pecoraro]

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.

[Build Bot]

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

[Build Bot]

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

[Devin Rousso]

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.

[Devin Rousso]

Created attachment 311626 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 311626 [details]
Patch

Clearing flags on attachment: 311626

Committed r217625: <http://trac.webkit.org/changeset/217625>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.