Bug 230923 – Web Inspector: add settings option for 'Show Mock Web Extension Tab' in engineering builds

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Blaze Burg]

.

[Blaze Burg]

Created attachment 439543 [details]
Patch v1.0

[Devin Rousso]

Comment on attachment 439543 [details]
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.Event.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).

[Radar WebKit Bug Importer]

<rdar://problem/83647450>

[Blaze Burg]

Comment on attachment 439543 [details]
Patch v1.0

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

>> Source/WebInspectorUI/UserInterface/Debug/Bootstrap.js:155
>> +            mockWebExtension = null;
> 
> this doesn't appear to be created anywhere

Oops.

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

Good idea.

>> Source/WebInspectorUI/UserInterface/Debug/Bootstrap.js:171
>> +    WI.settings.engineeringShowMockWebExtensionTab.addEventListener(WI.Setting.Event.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).

Oh, I totally forgot about that change, but it's coming back to me now. Thanks for explaining.

[Blaze Burg]

Created attachment 439567 [details]
Patch v1.1 (for landing)

[EWS]

Committed r283212 (242254@main): <https://commits.webkit.org/242254@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 439567 [details].