[webkit-reviews] review granted: [Bug 172477] Web Inspector: Add Debug view to Settings tab for debug settings and experimental features : [Attachment 311611] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has granted Devin Rousso
<drousso at apple.com>'s request for review:
Bug 172477: Web Inspector: Add Debug view to Settings tab for debug settings
and experimental features
https://bugs.webkit.org/show_bug.cgi?id=172477

Attachment 311611: Patch

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




--- Comment #11 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 311611
  --> https://bugs.webkit.org/attachment.cgi?id=311611
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.Set
ting.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.