Luca Greco [:rpl] [:luca] [:lgreco] Assignee
	Description • 8 years ago

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.

	Luca Greco [:rpl] [:luca] [:lgreco] Assignee
	Comment 1 • 8 years ago

Errata corrige: 
- the `elements` property should be inside the `devtools.panels` namespace (like in `devtools.panels.elements`)

	Luca Greco [:rpl] [:luca] [:lgreco] Assignee
	Comment 3 • 7 years ago
mozreview-review

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.

	Luca Greco [:rpl] [:luca] [:lgreco] Assignee
	Comment 4 • 7 years ago

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.

	Luca Greco [:rpl] [:luca] [:lgreco] Assignee
	Comment 6 • 7 years ago

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?

	Andrew Swan [:aswan]
	Comment 7 • 7 years ago
mozreview-review

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?

	Luca Greco [:rpl] [:luca] [:lgreco] Assignee
	Comment 10 • 7 years ago
mozreview-review-reply

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

	Luca Greco [:rpl] [:luca] [:lgreco] Assignee
	Comment 11 • 7 years ago

Clearing the needinfo (nod needed anymore because there is already an r? on the new patch)

	Luca Greco [:rpl] [:luca] [:lgreco] Assignee
	Comment 12 • 7 years ago

typo: s/nod/not/

	Julian Descottes [:jdescottes]
	Comment 13 • 7 years ago
mozreview-review

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.

	Alexandre Poirot [:ochameau]
	Comment 16 • 7 years ago
mozreview-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.

	Luca Greco [:rpl] [:luca] [:lgreco] Assignee
	Comment 21 • 7 years ago

patches rebased on a recent mozilla-central tip (but there were no conflicts to resolve manually)

	Luca Greco [:rpl] [:luca] [:lgreco] Assignee
	Comment 22 • 7 years ago

one more push to try for the rebased patches: 

- https://treeherder.mozilla.org/#/jobs?repo=try&revision=d0f67117002d38c03a8346e4d247f150789fa441

	Pulsebot
	Comment 23 • 7 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 24 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/bf290e0676b5
https://hg.mozilla.org/mozilla-central/rev/2fb0f93b7b5b

	Will Bamberg [:wbamberg]
	Comment 25 • 7 years ago

Luca, I've added:
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/devtools.panels/ElementsPanel
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/devtools.panels/ElementsPanel/onSelectionChanged

Please let me know if you see anything else!

	Luca Greco [:rpl] [:luca] [:lgreco] Assignee
	Comment 26 • 7 years ago

Thanks Will, the above MDN doc pages looks great.