[webkit-reviews] review granted: [Bug 230923] Web Inspector: add settings option for 'Show Mock Web Extension Tab' in engineering builds : [Attachment 439543] Patch v1.0

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted BJ Burg <bburg at apple.com>'s
request for review:
Bug 230923: Web Inspector: add settings option for 'Show Mock Web Extension
Tab' in engineering builds
https://bugs.webkit.org/show_bug.cgi?id=230923

Attachment 439543: Patch v1.0

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




--- Comment #2 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 439543
  --> https://bugs.webkit.org/attachment.cgi?id=439543
Patch v1.0

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

r=me with some fixes

> Source/WebInspectorUI/UserInterface/Debug/Bootstrap.js:153
> +	   let enabled = WI.settings.engineeringShowMockWebExtensionTab.value;
> +	   if (!enabled) {

NIT: you don't need to save this to a variable if it's only used once

> Source/WebInspectorUI/UserInterface/Debug/Bootstrap.js:155
> +	       mockWebExtension = null;

this doesn't appear to be created anywhere

> Source/WebInspectorUI/UserInterface/Debug/Bootstrap.js:161
> +	       console.error("Problem creating mock web extension: " + error);

Do you wanna `WI.reportInternalError` so that the Uncaught Exception Reporter
is shown?

> Source/WebInspectorUI/UserInterface/Debug/Bootstrap.js:167
> +	       console.error("Problem creating mock web extension tab: " +
result);

ditto (:161)

> Source/WebInspectorUI/UserInterface/Debug/Bootstrap.js:171
> +   
WI.settings.engineeringShowMockWebExtensionTab.addEventListener(WI.Setting.Even
t.Changed, updateMockWebExtensionTab, null);

I think the `null` will cause an assertion to fail, as we need to have a
`thisObject` in order for `WeakRef` to work properly.  You can use
`WI.settings.engineeringShowMockWebExtensionTab` as the `thisObject` since it's
the first thing checked inside the listener (and it matches other things).