1474440 - Implement support for the 'onHighlighted' API for multiselect tabs

Status: RESOLVED FIXED

(WebExtensions :: General, enhancement, P2)

Description

[Oriol Brufau [:Oriol]]

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

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

Oriol, is this something you can work on?

Comment 2

[Oriol Brufau [:Oriol]]

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.

Comment 3

[David Durst [:ddurst]]

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.

Comment 4

[Oriol Brufau [:Oriol]]

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.

Comment 5

[Shane Caraveo (:mixedpuppy)]

Do we have a bug for onHighlightChanged?  If not, it should probably just be part of this bug.

Comment 6

[Oriol Brufau [:Oriol]]

(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 7

[Oriol Brufau [:Oriol]]

Attachment: Bug 1474440 - Implement support for the 'onHighlighted' API for multiselect tabs

Review commit: https://reviewboard.mozilla.org/r/260092/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/260092/

Comment 8

[Shane Caraveo (:mixedpuppy)]

(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.

Comment 9

[Shane Caraveo (:mixedpuppy)]

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);

Comment 10

[Oriol Brufau [:Oriol]]

Attachment: Bug 1474440 - Rename browser_ext_tabs_onHighlighted.js to browser_ext_tabs_events_order.js

Basically it tests in which order various tab events are dispatched, so the old name
didn't make sense, and it will be more suited for a new test specific to onHighlighted.

Review commit: https://reviewboard.mozilla.org/r/261476/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/261476/

Comment 11

[Oriol Brufau [:Oriol]]

Comment on attachment 8995705 [details]
Bug 1474440 - Implement support for the 'onHighlighted' API for multiselect tabs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/260092/diff/1-2/

Comment 12

[Oriol Brufau [:Oriol]]

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 13

[Oriol Brufau [:Oriol]]

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

[Code Review Bot [:reviewbot]]

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 15

[Oriol Brufau [:Oriol]]

Comment on attachment 8995705 [details]
Bug 1474440 - Implement support for the 'onHighlighted' API for multiselect tabs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/260092/diff/2-3/

Comment 16

[Mike Conca [:mconca]]

(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.

Comment 17

[Shane Caraveo (:mixedpuppy)]

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

Comment 18

[Shane Caraveo (:mixedpuppy)]

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.

Comment 19

[Oriol Brufau [:Oriol]]

(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 20

[Oriol Brufau [:Oriol]]

Comment on attachment 8997746 [details]
Bug 1474440 - Rename browser_ext_tabs_onHighlighted.js to browser_ext_tabs_events_order.js

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/261476/diff/1-2/

Comment 21

[Oriol Brufau [:Oriol]]

Comment on attachment 8995705 [details]
Bug 1474440 - Implement support for the 'onHighlighted' API for multiselect tabs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/260092/diff/3-4/

Comment 22

[Oriol Brufau [:Oriol]]

I hope this fixes your concerns about the test.

Comment 23

[Shane Caraveo (:mixedpuppy)]

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!

Comment 24

[Shane Caraveo (:mixedpuppy)]

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.

Comment 25

[Bogdan Tara[:bogdan_tara | bogdant]]

(In reply to Oriol Brufau [:Oriol] from comment #22)
> I hope this fixes your concerns about the test.

So.... both patches need landing?

Comment 26

[Oriol Brufau [:Oriol]]

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

[Pulsebot]

Pushed by [e-mail]:
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

Comment 28

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/0b0cb02af895
https://hg.mozilla.org/mozilla-central/rev/776150decf44

Comment 29

[Victor Carciu]

Can you please add some STRs for QA(and a test webextension if possible) or add the "qe-verify-" flag ?

Comment 30

[Oriol Brufau [:Oriol]]

Covered by automated tests

Comment 31

[Will Bamberg [:wbamberg]]

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.