Closed
Bug 1341304
Opened 8 years ago
Closed 7 years ago
Implement devtools.panels.elements.onSelectionChanged
Categories
(WebExtensions :: Developer Tools, defect, P2)
WebExtensions
Developer Tools
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.
Assignee | ||
Updated•8 years ago
|
Summary: Implement devtools.elements.onSelectionChanged → Implement devtools.panels.elements.onSelectionChanged
Assignee | ||
Comment 1•8 years ago
|
||
Errata corrige: - the `elements` property should be inside the `devtools.panels` namespace (like in `devtools.panels.elements`)
Updated•8 years ago
|
Priority: -- → P2
Whiteboard: [devtools, triaged]
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8883597 -
Flags: review?(aswan)
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.
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → lgreco
Status: NEW → ASSIGNED
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.
Flags: needinfo?(poirot.alex)
Comment hidden (mozreview-request) |
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?
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?
Attachment #8883597 -
Flags: review?(aswan) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8884015 -
Flags: review?(poirot.alex)
Attachment #8884015 -
Flags: review?(jdescottes)
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
Assignee | ||
Comment 11•7 years ago
|
||
Clearing the needinfo (nod needed anymore because there is already an r? on the new patch)
Flags: needinfo?(poirot.alex)
Assignee | ||
Comment 12•7 years ago
|
||
typo: s/nod/not/
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.
Attachment #8884015 -
Flags: review?(jdescottes) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
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.
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•7 years ago
|
||
patches rebased on a recent mozilla-central tip (but there were no conflicts to resolve manually)
Assignee | ||
Comment 22•7 years ago
|
||
one more push to try for the rebased patches: - https://treeherder.mozilla.org/#/jobs?repo=try&revision=d0f67117002d38c03a8346e4d247f150789fa441
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
Comment 24•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/bf290e0676b5 https://hg.mozilla.org/mozilla-central/rev/2fb0f93b7b5b
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Updated•7 years ago
|
Keywords: dev-doc-needed
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!
Flags: needinfo?(lgreco)
Assignee | ||
Comment 26•7 years ago
|
||
Thanks Will, the above MDN doc pages looks great.
Flags: needinfo?(lgreco)
Updated•6 years ago
|
Keywords: dev-doc-needed → dev-doc-complete
Updated•6 years ago
|
Product: Toolkit → WebExtensions
You need to log in
before you can comment on or make changes to this bug.
Description
•