Implement support for the 'onHighlighted' API for multiselect tabs

RESOLVED FIXED in Firefox 63

Status

P2
normal
RESOLVED FIXED
4 months ago
2 months ago

People

(Reporter: Oriol, Assigned: Oriol)

Tracking

({dev-doc-complete})

unspecified
mozilla63
dev-doc-complete
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox63 fixed)

Details

Attachments

(2 attachments)

(Assignee)

Description

4 months ago
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

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

Comment 4

4 months 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)
Do we have a bug for onHighlightChanged?  If not, it should probably just be part of this bug.
(Assignee)

Comment 6

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

Updated

4 months ago
Depends on: 1479130
Comment hidden (mozreview-request)
(Assignee)

Updated

4 months ago
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 9

4 months 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

4 months 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

4 months 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

4 months 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

4 months 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

4 months 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

4 months 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

3 months 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

3 months ago
I hope this fixes your concerns about the test.

Comment 23

3 months 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

3 months 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

3 months ago
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?
(Assignee)

Comment 26

3 months 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

3 months 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

3 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/0b0cb02af895
https://hg.mozilla.org/mozilla-central/rev/776150decf44
Status: ASSIGNED → RESOLVED
Last Resolved: 3 months ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63

Updated

3 months ago
Keywords: dev-doc-needed

Comment 29

3 months 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

3 months ago
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.
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.