Closed Bug 1474440 Opened 6 years ago Closed 6 years ago

Implement support for the 'onHighlighted' API for multiselect tabs

Categories

(WebExtensions :: General, enhancement, P2)

enhancement

Tracking

(firefox63 fixed)

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: Oriol, Assigned: Oriol)

References

Details

(Keywords: dev-doc-complete)

Attachments

(2 files)

Summary: Implement support for the 'onHighlighted' API for multiselect tabs with tabs.query → Implement support for the 'onHighlighted' API for multiselect tabs
Oriol, is this something you can work on?
Flags: needinfo?(oriol-bugzilla)
Priority: -- → P2
If we want this in 63, we won't want to wait too long. Oriol -- let me know if this is something you can/will work on; if not, we should re-assign or push to 64.
Flags: needinfo?(oriol-bugzilla)
I will be able to take a look this weekend, but in order to dispatch the events at the right times (and not e.g. dispatch multiple ones for the same action), some refactoring of tabbrowser.js may be needed, and I don't know it very much. Maybe that part should be done in another bug.
Flags: needinfo?(oriol-bugzilla)
Do we have a bug for onHighlightChanged? If not, it should probably just be part of this bug.
(In reply to Shane Caraveo (:mixedpuppy) from comment #5) > Do we have a bug for onHighlightChanged? If not, it should probably just be > part of this bug. It's deprecated and not supported on Firefox (unlike onHighlighted which fallbacks to onActivated). So is onHighlightChanged really necessary?
Depends on: 1479130
Assignee: nobody → oriol-bugzilla
Status: NEW → ASSIGNED
(In reply to Oriol Brufau [:Oriol] from comment #6) > (In reply to Shane Caraveo (:mixedpuppy) from comment #5) > > Do we have a bug for onHighlightChanged? If not, it should probably just be > > part of this bug. > > It's deprecated and not supported on Firefox (unlike onHighlighted which > fallbacks to onActivated). > So is onHighlightChanged really necessary? Depends on if it's actually used a bunch. Maybe Mike can look.
Flags: needinfo?(mconca)
Comment on attachment 8995705 [details] Bug 1474440 - Implement support for the 'onHighlighted' API for multiselect tabs https://reviewboard.mozilla.org/r/260092/#review267334 As the only consumer of this is an extension with an onHighlighted listener, I'd like to see this implemented in a way where we don't listen/emit unless we need to. Perhaps implement this similar to tabs.onMoved or tabs.onUpdated, and maybe check for the listener before emitting from the window open case. It looks like activated (and perhaps others) might also benefit from a change like this, but not necessary to land this patch (a followup bug would be nice). EventEmitter.has(event) { return !!this[LISTENERS].get(event); } tabtracker._handleWindowOpen(window) { ... all the stuff ... if (this.has("tabs-highlighted")) { this.emitHighlighted(window); } } ::: browser/components/extensions/test/browser/browser_ext_tabs_onHighlighted.js:5 (Diff revision 1) > /* -*- Mode: indent-tabs-mode: nil; js-indent-level: 2 -*- */ > /* vim: set sts=2 sw=2 et tw=80: */ > /* eslint-disable mozilla/no-arbitrary-setTimeout */ > "use strict"; > Please make this a new file, perhaps call it browser_ext_tabs_event_onHighlighted.js I spent time figuring out why these changes in the diff were made until I realized it wasn't really "changes". renaming browser_ext_tabs_onHighlighted.js as you did is otherwise fine. If you prefer to maintain the filename, another option is a seperate diff (that this one is on top of) that renames the prior file. ::: browser/components/extensions/test/browser/browser_ext_tabs_onHighlighted.js:81 (Diff revision 1) > > // Wait up to 5000 ms for the expected number of events. > - for (let i = 0; i < 50 && (!events[tabId] || events[tabId].length < expectedEvents.length); i++) { > + for (let i = 0; i < 50 && (events.length < expected.length); i++) { > await new Promise(resolve => setTimeout(resolve, 100)); > } > - > + browser.test.assertEq(expected.length, events.length, "Check number of events"); This is awkward and we dont catch an error till the end and the log messages wont match with the error, and we have to count the event number to see which one failed. I'd rather see something like: let p = promiseExpected(expectedData) let window = await browser.windows.create({...}); ok(await p, logMessage);
Attachment #8995705 - Flags: review?(mixedpuppy) → review-
Comment on attachment 8995705 [details] Bug 1474440 - Implement support for the 'onHighlighted' API for multiselect tabs https://reviewboard.mozilla.org/r/260092/#review267334 > Please make this a new file, perhaps call it browser_ext_tabs_event_onHighlighted.js > > I spent time figuring out why these changes in the diff were made until I realized it wasn't really "changes". > > renaming browser_ext_tabs_onHighlighted.js as you did is otherwise fine. > > If you prefer to maintain the filename, another option is a seperate diff (that this one is on top of) that renames the prior file. I mantained the filename for consistency with browser_ext_tabs_onUpdated.js > This is awkward and we dont catch an error till the end and the log messages wont match with the error, and we have to count the event number to see which one failed. I'd rather see something like: > > let p = promiseExpected(expectedData) > let window = await browser.windows.create({...}); > ok(await p, logMessage); Not that easy, because the windowId of the expected data is provided by `browser.windows.create`, but once the promise is resolved the onHighlighted event listener could have been called already. I hope you will like it more after the edit
Comment on attachment 8995705 [details] Bug 1474440 - Implement support for the 'onHighlighted' API for multiselect tabs https://reviewboard.mozilla.org/r/260092/#review268480 ::: browser/components/extensions/test/browser/browser_ext_tabs_onHighlighted.js:89 (Diff revision 2) > + await browser.tabs.highlight({tabs: [2, 0, 1]}); > + > + browser.test.log("Move tab into a new window"); > + let window = await browser.windows.create({tabId: tabs[2]}); > + windows.push(window.id); > + await expect({tabIds: [tabs[1], tabs[0]], windowId: windows[0]}, For some reason now the test needs `[tabs[0]]` instead of `[tabs[1], tabs[0]]`. This was not the case when I wrote the first patch, then the behavior was like in Chrome. I will investigate if there has been some regression.
Comment on attachment 8995705 [details] Bug 1474440 - Implement support for the 'onHighlighted' API for multiselect tabs https://reviewboard.mozilla.org/r/260092/#review268500 Code analysis found 1 defect in this patch: - 1 defect found by mozlint You can run this analysis locally with: - `./mach lint path/to/file` (JS/Python/etc) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: toolkit/components/extensions/ExtensionCommon.jsm:222 (Diff revision 2) > /** > + * Checks whether there is some listener for the given event. > + * > + * @param {string} event > + * The name of the event to listen for. > + * @returns {bool} Error: Use 'boolean' instead of 'bool'. [eslint: valid-jsdoc]
(In reply to Shane Caraveo (:mixedpuppy) from comment #8) > (In reply to Oriol Brufau [:Oriol] from comment #6) > > (In reply to Shane Caraveo (:mixedpuppy) from comment #5) > > > Do we have a bug for onHighlightChanged? If not, it should probably just be > > > part of this bug. > > > > It's deprecated and not supported on Firefox (unlike onHighlighted which > > fallbacks to onActivated). > > So is onHighlightChanged really necessary? > > Depends on if it's actually used a bunch. Maybe Mike can look. Better late than never, but... onHighlightChanged is used by 22 extensions, out of about 85K on the Chrome web store. Please do not spend another second thinking about onHighlightChanged.
Flags: needinfo?(mconca)
Comment on attachment 8997746 [details] Bug 1474440 - Rename browser_ext_tabs_onHighlighted.js to browser_ext_tabs_events_order.js https://reviewboard.mozilla.org/r/261476/#review268588
Attachment #8997746 - Flags: review?(mixedpuppy) → review+
Comment on attachment 8995705 [details] Bug 1474440 - Implement support for the 'onHighlighted' API for multiselect tabs https://reviewboard.mozilla.org/r/260092/#review268590 ::: browser/components/extensions/parent/ext-browser.js:283 (Diff revision 3) > windowTracker.addListener("TabSelect", this); > + windowTracker.addListener("TabMultiSelect", this); I wasn't clear the last time, I'd like to avoid adding the listener at all until an extension adds a listener. We could probably just override on/off as well with something like: on(name, listener) if (name == "tabs-highlighted" && !this.has(name)) windowTracker.addListener("TabMultiSelect", this); super(on ...args) etc. ::: browser/components/extensions/parent/ext-browser.js:458 (Diff revision 3) > this.emitActivated(nativeTab); > }); > break; > + > + case "TabMultiSelect": > + if (this.has("tabs-highlighted")) { With the on/off handling, this isn't necessary here (though it is on the other location) ::: browser/components/extensions/test/browser/browser_ext_tabs_onHighlighted.js:26 (Diff revision 3) > + for (let i = 0; i < 50 && (events.length < expected.length); i++) { > + await new Promise(resolve => setTimeout(resolve, 100)); > + } This feels like an intermittent in the making. how about something like: promiseHighlighted(numexpected = 1) events = [] return new promise((resolve) => { onHighlighted.addListener(fn = (highlightInfo) => { events.push(hightlightinfo) if (--numexpected == 0) { onHighlighted.removeListener(fn) resolve(events) } } } listener = promiseHighlighted() await tabs.create await expect(expected, await listener, testMessage) ::: browser/components/extensions/test/browser/browser_ext_tabs_onHighlighted.js:29 (Diff revision 3) > + browser.test.assertEq(expected.length, events.length, "Check number of events"); > + for (let i = 0, n = Math.min(expected.length, events.length); i < n; ++i) { No need for the math.min, you're failing above if they are not the same. ::: browser/components/extensions/test/browser/browser_ext_tabs_onHighlighted.js:44 (Diff revision 3) > + browser.test.log("Create a new active tab"); > + let tab = await browser.tabs.create({active: true, url: "about:blank?1"}); > + tabs.push(tab.id); > + await expect({tabIds: [tabs[1]], windowId: windows[0]}); Drop the log, send the text via expect. await expect({message: "new active tab should be highlighted", tabIds ..., windowId ...}) And use that message in the test inside the loop.
Attachment #8995705 - Flags: review?(mixedpuppy)
(In reply to Shane Caraveo (:mixedpuppy) from comment #18) > I wasn't clear the last time, I'd like to avoid adding the listener at all > until an extension adds a listener. What's the point? The "TabMultiSelect" listener does almost no work if there is no "tabs-highlighted" listener. > We could probably just override on/off as well with something like: > > on(name, listener) > if (name == "tabs-highlighted" && !this.has(name)) > windowTracker.addListener("TabMultiSelect", this); > super(on ...args) But windowTracker is not defined in that context. > With the on/off handling, this isn't necessary here (though it is on the > other location) I don't see why the other location is different. > This feels like an intermittent in the making. I copied that code from https://searchfox.org/mozilla-central/rev/f0c15db995198a1013e1c5f5b5bea54ef83f1049/browser/components/extensions/test/browser/browser_ext_tabs_onHighlighted.js#55-58 > how about something like: > > promiseHighlighted(numexpected = 1) > events = [] > return new promise((resolve) => { > onHighlighted.addListener(fn = (highlightInfo) => { > events.push(hightlightinfo) > if (--numexpected == 0) { > onHighlighted.removeListener(fn) > resolve(events) > } > } > } > > listener = promiseHighlighted() > await tabs.create > await expect(expected, await listener, testMessage) If multiple events are dispatched for the same action, this won't detect it. But I guess this can be handled with another event listener. > No need for the math.min, you're failing above if they are not the same. But failing above doesn't terminate the test, so I was trying to avoid potential out-of-bounds problems. Well, JSON.stringify(undefined) doesn't throw so it's OK.
I hope this fixes your concerns about the test.
Comment on attachment 8995705 [details] Bug 1474440 - Implement support for the 'onHighlighted' API for multiselect tabs https://reviewboard.mozilla.org/r/260092/#review269350 Tests are much better, thanks!
Attachment #8995705 - Flags: review?(mixedpuppy) → review+
Comment on attachment 8995705 [details] Bug 1474440 - Implement support for the 'onHighlighted' API for multiselect tabs https://reviewboard.mozilla.org/r/260092/#review268590 > I wasn't clear the last time, I'd like to avoid adding the listener at all until an extension adds a listener. > > We could probably just override on/off as well with something like: > > on(name, listener) > if (name == "tabs-highlighted" && !this.has(name)) > windowTracker.addListener("TabMultiSelect", this); > super(on ...args) > > etc. I'll let go of this, with the this.has check, this is primarily a nit.
Keywords: checkin-needed
(In reply to Oriol Brufau [:Oriol] from comment #22) > I hope this fixes your concerns about the test. So.... both patches need landing?
Yes, first "Rename browser_ext_tabs_onHighlighted.js to browser_ext_tabs_events_order.js" and then "Implement support for the 'onHighlighted' API for multiselect tabs".
Pushed by btara@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/0b0cb02af895 Rename browser_ext_tabs_onHighlighted.js to browser_ext_tabs_events_order.js r=mixedpuppy https://hg.mozilla.org/integration/autoland/rev/776150decf44 Implement support for the 'onHighlighted' API for multiselect tabs r=mixedpuppy
Keywords: checkin-needed
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Keywords: dev-doc-needed
Can you please add some STRs for QA(and a test webextension if possible) or add the "qe-verify-" flag ?
Flags: needinfo?(oriol-bugzilla)
Covered by automated tests
Flags: needinfo?(oriol-bugzilla) → qe-verify-
I've updated the compat data for tabs.onHighlighted, and the note at the start talking about Firefox support: https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/tabs/onHighlighted Marking dev-doc-complete, but please let me know if you need anything else.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: