Bug 231338 – [Cocoa] Web Inspector: provide a way for _WKInspectorExtension clients to be notified when the inspected page navigates

• Component: Web Inspector
• Status: RESOLVED FIXED

[Blaze Burg]

.

[Blaze Burg]

<rdar://71200338>

[Blaze Burg]

Created attachment 440528 [details]
Patch v1.0

[Patrick Angle]

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)`)

[Devin Rousso]

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

[Blaze Burg]

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

[Blaze Burg]

Created attachment 440620 [details]
Patch v1.1

[Devin Rousso]

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?

[Blaze Burg]

Created attachment 440692 [details]
Patch v1.1.1 (for landing)

[EWS]

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].