Bug 231338 - [Cocoa] Web Inspector: provide a way for _WKInspectorExtension clients to be notified when the inspected page navigates
Summary: [Cocoa] Web Inspector: provide a way for _WKInspectorExtension clients to be ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: BJ Burg
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-10-06 16:26 PDT by BJ Burg
Modified: 2021-10-15 17:17 PDT (History)
7 users (show)

See Also:


Attachments
Patch v1.0 (33.79 KB, patch)
2021-10-07 12:45 PDT, BJ Burg
no flags Details | Formatted Diff | Diff
Patch v1.1 (32.89 KB, patch)
2021-10-08 08:45 PDT, BJ Burg
no flags Details | Formatted Diff | Diff
Patch v1.1.1 (for landing) (44.34 KB, patch)
2021-10-08 17:39 PDT, BJ Burg
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description BJ Burg 2021-10-06 16:26:38 PDT
.
Comment 1 BJ Burg 2021-10-06 16:27:33 PDT
<rdar://71200338>
Comment 2 BJ Burg 2021-10-07 12:45:11 PDT
Created attachment 440528 [details]
Patch v1.0
Comment 3 Patrick Angle 2021-10-07 13:21:49 PDT
Comment on attachment 440528 [details]
Patch v1.0

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

> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:38
> +        // No need to set up this listener until at least one extension is registered.
> +        this._mainFrameDidChangeListener = null;

I don't think you need to keep the event listener around here. See :81.

> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:56
> +            this._mainFrameDidChangeListener = WI.Frame.addEventListener(WI.Frame.Event.MainResourceDidChange, this._handleMainResourceDidChange, this);

Ditto :37-38

> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:81
> +            WI.Frame.removeEventListener(this._mainFrameDidChangeListener);

`removeEventListener` takes that same three parameters that `addEventListener` takes (e.g. `WI.Frame.removeEventListener(WI.Frame.Event.MainResourceDidChange, this._handleMainResourceDidChange, this)`)
Comment 4 Devin Rousso 2021-10-07 13:24:40 PDT
Comment on attachment 440528 [details]
Patch v1.0

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

>> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:56
>> +            this._mainFrameDidChangeListener = WI.Frame.addEventListener(WI.Frame.Event.MainResourceDidChange, this._handleMainResourceDidChange, this);
> 
> Ditto :37-38

There's no reason to save the return value as it's the same value as the 2nd `listener` argument that's passed in (i.e. `this._mainFrameDidChange` will be the same thing as `this._handleMainResourceDidChange`.  Instead, I'd just have a `_didAddMainResourceDidChangeListener` boolean flag.

>> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:81
>> +            WI.Frame.removeEventListener(this._mainFrameDidChangeListener);
> 
> `removeEventListener` takes that same three parameters that `addEventListener` takes (e.g. `WI.Frame.removeEventListener(WI.Frame.Event.MainResourceDidChange, this._handleMainResourceDidChange, this)`)

Yeah you need all three.  There used to be a way to do `null, null, this` to remove all listeners for all events with `this` as the context, but we removed that when moving to using `WeakRef`.

> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:278
> +        InspectorFrontendHost.inspectedPageDidNavigate(WI.networkManager.mainFrame.url);

Thinking about this a bit more, it seems like more work to add/remove the event listener based on the presence of the extension.  Instead, why don't we just always listen and only call `InspectorFrontendHost.inspectedPageDidNavigate` if we know we have some (e.g. `this._extensionForExtensionIDMap.size`)?
Comment 5 BJ Burg 2021-10-07 16:41:30 PDT
Comment on attachment 440528 [details]
Patch v1.0

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

>> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:278
>> +        InspectorFrontendHost.inspectedPageDidNavigate(WI.networkManager.mainFrame.url);
> 
> Thinking about this a bit more, it seems like more work to add/remove the event listener based on the presence of the extension.  Instead, why don't we just always listen and only call `InspectorFrontendHost.inspectedPageDidNavigate` if we know we have some (e.g. `this._extensionForExtensionIDMap.size`)?

OK
Comment 6 BJ Burg 2021-10-08 08:45:16 PDT
Created attachment 440620 [details]
Patch v1.1
Comment 7 Devin Rousso 2021-10-08 11:21:04 PDT
Comment on attachment 440620 [details]
Patch v1.1

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

r=me

> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:37
> +        this._mainFrameDidChangeListener = WI.Frame.addEventListener(WI.Frame.Event.MainResourceDidChange, this._handleMainResourceDidChange, this);

you don't need to create `this._mainFrameDidChangeListener` for reasons I described previously

> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:264
> +        InspectorFrontendHost.inspectedPageDidNavigate(WI.networkManager.mainFrame.url);

NIT: It feels slightly odd to have this be so generically named and yet only used by `WI.WebInspectorExtensionController`.  How awful would it be to just remove these changes here and instead _always_ invoke this inside `WI._mainResourceDidChange` in `Source/WebInspectorUI/UserInterface/Base/Main.js` and then just let the extension code in the UIProcess handle whether or not to actually do things?
Comment 8 BJ Burg 2021-10-08 17:39:33 PDT
Created attachment 440692 [details]
Patch v1.1.1 (for landing)
Comment 9 EWS 2021-10-08 18:38:51 PDT
Committed r283857 (242735@main): <https://commits.webkit.org/242735@main>

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