Oriol Brufau [:Oriol] Assignee
	Description • 6 years 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

	Jared Wein [:jaws] (please needinfo? me)
	Comment 1 • 6 years ago

Oriol, is this something you can work on?

	Oriol Brufau [:Oriol] 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.

	David Durst [:ddurst]
	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.

	Oriol Brufau [:Oriol] 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.

	Shane Caraveo (:mixedpuppy)
	Comment 5 • 6 years ago

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

	Oriol Brufau [:Oriol] 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?

	Shane Caraveo (:mixedpuppy)
	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.

	Shane Caraveo (:mixedpuppy)
	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);

	Oriol Brufau [:Oriol] 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

	Oriol Brufau [:Oriol] 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.

	Code Review Bot [:reviewbot]
	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]

	Mike Conca [:mconca]
	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.

	Shane Caraveo (:mixedpuppy)
	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

	Shane Caraveo (:mixedpuppy)
	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.

	Oriol Brufau [:Oriol] 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.

	Oriol Brufau [:Oriol] Assignee
	Comment 22 • 6 years ago

I hope this fixes your concerns about the test.

	Shane Caraveo (:mixedpuppy)
	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!

	Shane Caraveo (:mixedpuppy)
	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.

	Bogdan Tara[:bogdan_tara | bogdant]
	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?

	Oriol Brufau [:Oriol] 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".

	Pulsebot
	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

	Daniel Varga [:dvarga]
	Comment 28 • 6 years ago
bugherder

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

	Victor Carciu
	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 ?

	Oriol Brufau [:Oriol] Assignee
	Comment 30 • 6 years ago

Covered by automated tests

	Will Bamberg [:wbamberg]
	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.