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)
WebExtensions
General
Tracking
(firefox63 fixed)
RESOLVED
FIXED
mozilla63
Tracking | Status | |
---|---|---|
firefox63 | --- | fixed |
People
(Reporter: Oriol, Assigned: Oriol)
References
Details
(Keywords: dev-doc-complete)
Attachments
(2 files)
onHighlighted should work as expected instead of being an alias for onActivated.
https://searchfox.org/mozilla-central/rev/5a744713370ec47969595e369fd5125f123e6d24/browser/components/extensions/parent/ext-tabs.js#405
Assignee | ||
Updated•6 years ago
|
Summary: Implement support for the 'onHighlighted' API for multiselect tabs with tabs.query → Implement support for the 'onHighlighted' API for multiselect tabs
Assignee | ||
Comment 2•6 years ago
|
||
Currently there is no internal event that onHighlighted can use. I guess it should happen in
https://searchfox.org/mozilla-central/rev/a80651653faa78fa4dfbd238d099c2aad1cec304/browser/base/content/tabbrowser.js#3596
https://searchfox.org/mozilla-central/rev/a80651653faa78fa4dfbd238d099c2aad1cec304/browser/base/content/tabbrowser.js#3630
https://searchfox.org/mozilla-central/rev/a80651653faa78fa4dfbd238d099c2aad1cec304/browser/base/content/tabbrowser.js#3639
I may take a look when I have time.
Flags: needinfo?(oriol-bugzilla)
Updated•6 years ago
|
Priority: -- → P2
Comment 3•6 years ago
|
||
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)
Assignee | ||
Comment 4•6 years ago
|
||
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)
Comment 5•6 years ago
|
||
Do we have a bug for onHighlightChanged? If not, it should probably just be part of this bug.
Assignee | ||
Comment 6•6 years ago
|
||
(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?
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → oriol-bugzilla
Status: NEW → ASSIGNED
Comment 8•6 years ago
|
||
(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 9•6 years ago
|
||
mozreview-review |
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 12•6 years ago
|
||
mozreview-review-reply |
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
Assignee | ||
Comment 13•6 years ago
|
||
mozreview-review |
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 14•6 years ago
|
||
mozreview-review |
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]
Comment hidden (mozreview-request) |
Comment 16•6 years ago
|
||
(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 17•6 years ago
|
||
mozreview-review |
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 18•6 years ago
|
||
mozreview-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)
Assignee | ||
Comment 19•6 years ago
|
||
(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.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 22•6 years ago
|
||
I hope this fixes your concerns about the test.
Comment 23•6 years ago
|
||
mozreview-review |
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 24•6 years ago
|
||
mozreview-review-reply |
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.
Assignee | ||
Updated•6 years ago
|
Keywords: checkin-needed
Comment 25•6 years ago
|
||
(In reply to Oriol Brufau [:Oriol] from comment #22)
> I hope this fixes your concerns about the test.
So.... both patches need landing?
Assignee | ||
Comment 26•6 years ago
|
||
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".
Comment 27•6 years ago
|
||
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
Comment 28•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/0b0cb02af895
https://hg.mozilla.org/mozilla-central/rev/776150decf44
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox63:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Updated•6 years ago
|
Keywords: dev-doc-needed
Comment 29•6 years ago
|
||
Can you please add some STRs for QA(and a test webextension if possible) or add the "qe-verify-" flag ?
Flags: needinfo?(oriol-bugzilla)
Assignee | ||
Comment 30•6 years ago
|
||
Covered by automated tests
Flags: needinfo?(oriol-bugzilla) → qe-verify-
Comment 31•6 years ago
|
||
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.
Keywords: dev-doc-needed → dev-doc-complete
You need to log in
before you can comment on or make changes to this bug.
Description
•