Implement devtools.panels.elements.onSelectionChanged

RESOLVED FIXED in Firefox 56

Status

()

Toolkit
WebExtensions: Developer Tools
P2
normal
RESOLVED FIXED
6 months ago
7 days ago

People

(Reporter: rpl, Assigned: rpl)

Tracking

(Blocks: 1 bug, {dev-doc-needed})

unspecified
mozilla56
dev-doc-needed
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: [devtools, triaged])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Assignee)

Description

6 months 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.
(Assignee)

Updated

6 months ago
Summary: Implement devtools.elements.onSelectionChanged → Implement devtools.panels.elements.onSelectionChanged
(Assignee)

Comment 1

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

Updated

6 months ago
Depends on: 1293298

Updated

6 months ago
Priority: -- → P2
Whiteboard: [devtools, triaged]
(Assignee)

Updated

3 months ago
Blocks: 1370525
Comment hidden (mozreview-request)
(Assignee)

Updated

2 months ago
Attachment #8883597 - Flags: review?(aswan)
(Assignee)

Comment 3

2 months 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.
(Assignee)

Updated

2 months ago
Assignee: nobody → lgreco
Status: NEW → ASSIGNED
(Assignee)

Comment 4

2 months 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.
Flags: needinfo?(poirot.alex)
Comment hidden (mozreview-request)
(Assignee)

Comment 6

2 months 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?

Comment 7

2 months 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?
Attachment #8883597 - Flags: review?(aswan) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 months ago
Attachment #8884015 - Flags: review?(poirot.alex)
Attachment #8884015 - Flags: review?(jdescottes)
(Assignee)

Comment 10

2 months 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
(Assignee)

Comment 11

2 months ago
Clearing the needinfo (nod needed anymore because there is already an r? on the new patch)
Flags: needinfo?(poirot.alex)
(Assignee)

Comment 12

2 months ago
typo: s/nod/not/

Comment 13

2 months 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.
Attachment #8884015 - Flags: review?(jdescottes) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 16

2 months 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.
Attachment #8884015 - Flags: review?(poirot.alex) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 21

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

Comment 22

a month ago
one more push to try for the rebased patches: 

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

Comment 23

a month 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

Comment 24

a month ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/bf290e0676b5
https://hg.mozilla.org/mozilla-central/rev/2fb0f93b7b5b
Status: ASSIGNED → RESOLVED
Last Resolved: a month ago
status-firefox56: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.