Closed Bug 1341304 Opened 3 years ago Closed 2 years ago

Implement devtools.panels.elements.onSelectionChanged

Categories

(WebExtensions :: Developer Tools, defect, P2)

defect

Tracking

(firefox56 fixed)

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: rpl, Assigned: rpl)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete, Whiteboard: [devtools, triaged])

Attachments

(2 files)

The devtools API namespace should contain the `devtools.elements` property, which provides a simple API object that represents the DevTools Inspector panel.

The `devtools.elements` property should provide the `onSelectionChanged` event, fired every time the user change the current selected page element in the inspector panel.
Summary: Implement devtools.elements.onSelectionChanged → Implement devtools.panels.elements.onSelectionChanged
Errata corrige: 
- the `elements` property should be inside the `devtools.panels` namespace (like in `devtools.panels.elements`)
Depends on: 1293298
Priority: -- → P2
Whiteboard: [devtools, triaged]
Blocks: 1370525
Attachment #8883597 - Flags: review?(aswan)
Comment on attachment 8883597 [details]
Bug 1341304 - Implement devtools.panels.elements.onSelectionChanged.

https://reviewboard.mozilla.org/r/154528/#review159618

::: browser/components/extensions/ext-devtools-panels.js:249
(Diff revision 1)
> +      // Wait the toolbox to be ready if it's still loading.
> +      if (!this.toolbox.isReady) {
> +        await new Promise(resolve => this.toolbox.once("ready", resolve));
> +      }
> +
> +      // Init the inspector if the toolbox selection is not yet defined.
> +      if (!this.toolbox.selection) {
> +        await this.toolbox.initInspector();
> +      }
> +
> +      this.toolbox.selection.on("new-node-front", this.onSelected);

This fragment is where the `DevToolsSelectionObserver` lazily subscribes the "new-node-front" event emitted by the toolbox selection (the idea is to subscribe the underlying DevTools event only if at least a listener has been subscribed by an extension context on the `devtools.panels.elements.onSelectionChanged` WebExtensions API event).

I'm going to needinfo Alex on this fragment, so that he can take a look at it and he can provide his review comments and advices on this integration point.
Assignee: nobody → lgreco
Status: NEW → ASSIGNED
Hi Alex,
in comment 3 I've highlighted the fragment of this patch that contains the integration point between the WebExtensions API implementation and the underlying DevTools "new-node-front" event emitted by the toolbox selection.

I've set on the patch an r? assigned to aswan, so that he can review the WebExtensions internals side of the patch and its related test, but I'd like to also get your review comments and your advice on the usage of the DevTools internal API.
Flags: needinfo?(poirot.alex)
An alternative approach for the integration with the DevTools internals can be to emit a new "selection-changed" event on the toolbox object, so that the DevToolsSelectionObserver doesn't need to interact directly with the `toolbox.selection` property.

How that sounds to you Alex?
Comment on attachment 8883597 [details]
Bug 1341304 - Implement devtools.panels.elements.onSelectionChanged.

https://reviewboard.mozilla.org/r/154530/#review159982

r=me with the initialize race addressed...

::: browser/components/extensions/ext-devtools-panels.js:274
(Diff revision 2)
> +
> +    if (this.destroyed) {
> +      throw new Error("Unable to close a destroyed DevToolsSelectionObserver");
> +    }
> +
> +    this.selection.off("new-node-front", this.onSelected);

Is there a possible race here where an extension is loaded and uses this event, so `lazyInit()` has been called, but then the extension is unloaded before the toolbox or inspector is fully initialized to `this.selection` isn't yet set?
Attachment #8883597 - Flags: review?(aswan) → review+
Attachment #8884015 - Flags: review?(poirot.alex)
Attachment #8884015 - Flags: review?(jdescottes)
Comment on attachment 8883597 [details]
Bug 1341304 - Implement devtools.panels.elements.onSelectionChanged.

https://reviewboard.mozilla.org/r/154530/#review159982

> Is there a possible race here where an extension is loaded and uses this event, so `lazyInit()` has been called, but then the extension is unloaded before the toolbox or inspector is fully initialized to `this.selection` isn't yet set?

yeah, that's not unlikely, one more reason to prefer the other option:

- DevTools Toolbox: emit a new "selection-changed" event on the toolbox object
- WE APIs: subscribe/unsubscribe an "selection-changed" event listener on the toolbox object
Clearing the needinfo (nod needed anymore because there is already an r? on the new patch)
Flags: needinfo?(poirot.alex)
typo: s/nod/not/
Comment on attachment 8884015 [details]
Bug 1341304 - Emit "selection-changed" event on the DevTools toolbox object.

https://reviewboard.mozilla.org/r/154970/#review160238

Looks good for me but I would like to have a simple test in devtools checking that this event is fired.

::: devtools/client/framework/toolbox.js:2239
(Diff revision 1)
>      }
>      return this._initInspector;
>    },
>  
> +  _onNewSelectedNodeFront: function (evt) {
> +    this.emit("selection-changed");

There is no DevTools consumer for this event so it at least needs a comment explaining why we maintain this event.
Attachment #8884015 - Flags: review?(jdescottes) → review+
Comment on attachment 8884015 [details]
Bug 1341304 - Emit "selection-changed" event on the DevTools toolbox object.

https://reviewboard.mozilla.org/r/154970/#review160610

Good call, yes it is better to expose an explicit API for that.

Thanks Luca.
Attachment #8884015 - Flags: review?(poirot.alex) → review+
patches rebased on a recent mozilla-central tip (but there were no conflicts to resolve manually)
Pushed by luca.greco@alcacoop.it:
https://hg.mozilla.org/integration/autoland/rev/bf290e0676b5
Emit "selection-changed" event on the DevTools toolbox object. r=jdescottes,ochameau
https://hg.mozilla.org/integration/autoland/rev/2fb0f93b7b5b
Implement devtools.panels.elements.onSelectionChanged. r=aswan
https://hg.mozilla.org/mozilla-central/rev/bf290e0676b5
https://hg.mozilla.org/mozilla-central/rev/2fb0f93b7b5b
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Thanks Will, the above MDN doc pages looks great.
Flags: needinfo?(lgreco)
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.