.
Created attachment 439543 [details] Patch v1.0
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).
<rdar://problem/83647450>
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.
Created attachment 439567 [details] Patch v1.1 (for landing)
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].