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.
Created attachment 310942 [details] Patch
Created attachment 310943 [details] [Image] After Patch is applied
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 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 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 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.
(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?
> How about making a debug button in the toolbar that opens the Debug view in > settings when clicked? That makes (1) more difficult.
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.
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 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 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
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 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.
Created attachment 311626 [details] Patch
Comment on attachment 311626 [details] Patch Clearing flags on attachment: 311626 Committed r217625: <http://trac.webkit.org/changeset/217625>
All reviewed patches have been landed. Closing bug.