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

Currently tabs.query({highlighted: true}) returns the activeTab [1] in Firefox, but with multiselected tabs being worked on, we can have it return the collection of tabs that are multiselected.

We can also have onHighlighted work as expected instead of being an alias for onActivated [2].

[1] https://searchfox.org/mozilla-central/rev/5a744713370ec47969595e369fd5125f123e6d24/toolkit/components/extensions/parent/ext-tabs-base.js#403
[2] https://searchfox.org/mozilla-central/rev/5a744713370ec47969595e369fd5125f123e6d24/browser/components/extensions/parent/ext-tabs.js#405

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

tabs.query({highlighted: false}) should work after bug 1464862, but the fast path needs to be updated for highlighted:true

I will do this, onHighlighted is unrelated and should be done in another bug.

	Shane Caraveo (:mixedpuppy)
	Comment 3 • 6 years ago
mozreview-review

Comment on attachment 8987791 [details]
Bug 1465170 - Implement support for the 'highlighted' API for multiselect tabs with tabs.query

https://reviewboard.mozilla.org/r/253068/#review259640

r+ given my comment.

::: browser/components/extensions/parent/ext-browser.js:948
(Diff revision 1)
> +    let {selectedTab, _multiSelectedTabsSet} = this.window.gBrowser;
> +    if (!selectedTab.multiselected) {
> +      yield tabManager.getWrapper(selectedTab);
> +      return;
> +    }
> +    for (let tab of ChromeUtils.nondeterministicGetWeakSetKeys(_multiSelectedTabsSet)) {

Rather than accessing \_multiSelectedTabsSet, add a tabbrowser.selectedTabs getter:

  get selectedTabs() \{
    return ChromeUtils.nondeterministicGetWeakSetKeys(this.\_multiSelectedTabsSet)
      .filter(tab => tab.isConnected && \!tab.closing);
  \}
  
And get a r? from jaws or someone who is working on multi select.

	Jared Wein [:jaws] (please needinfo? me) Reporter
	Comment 5 • 6 years ago
mozreview-review

Comment on attachment 8987791 [details]
Bug 1465170 - Implement support for the 'highlighted' API for multiselect tabs with tabs.query

https://reviewboard.mozilla.org/r/253068/#review259724

::: browser/base/content/tabbrowser.js:2629
(Diff revision 2)
>    removeMultiSelectedTabs() {
>      if (!this.warnAboutClosingTabs(this.closingTabsEnum.MULTI_SELECTED)) {
>        return;
>      }
>  
> -    let selectedTabs = ChromeUtils.nondeterministicGetWeakSetKeys(this._multiSelectedTabsSet)
> +    this.removeCollectionOfTabs(this.selectedTabs);

Can you please rename removeCollectionOfTabs to removeTabs?

::: browser/base/content/tabbrowser.js:3684
(Diff revision 2)
>      if (updatePositionalAttributes) {
>        this.tabContainer._setPositionalAttributes();
>      }
>    },
>  
> -  get multiSelectedTabsCount() {
> +  get selectedTabs() {

Per the name of this, I think it would be reasonable for people to think that if there is no multiselection present that this would sitll return the selectedTab.

Can we return this.selectedTab if there is no multiselection? How might that affect other code that is using selectedTabs to determine if a multiselection is active?

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

(In reply to (away 7/1-7/7) Jared Wein [:jaws] (please needinfo? me) from comment #5)
> Per the name of this, I think it would be reasonable for people to think
> that if there is no multiselection present that this would sitll return the
> selectedTab.

That's precisely what the Webext API wants.

> Can we return this.selectedTab if there is no multiselection? How might that
> affect other code that is using selectedTabs to determine if a
> multiselection is active?

I guess Abdoulaye knows better than me.

	Abdoulaye O. LY
	Comment 8 • 6 years ago
mozreview-review-reply

Comment on attachment 8987791 [details]
Bug 1465170 - Implement support for the 'highlighted' API for multiselect tabs with tabs.query

https://reviewboard.mozilla.org/r/253068/#review259724

> Per the name of this, I think it would be reasonable for people to think that if there is no multiselection present that this would sitll return the selectedTab.
> 
> Can we return this.selectedTab if there is no multiselection? How might that affect other code that is using selectedTabs to determine if a multiselection is active?

This will affect all code using gBrowser.multiSelectedTabsCount getter https://searchfox.org/mozilla-central/search?q=multiSelectedTabsCount&case=true&path=. If we return the selectedTab in case of no multiselection, then 1 should be the lower value returned from this getter. 

Hence, we should change whenever we see "if(mutliSelectedTabsCount)" to "if(multiSelectionTabsCount > 1" and in test files change "is(gBrowser.multiSelectionTabsCount, 0, ..." to "is(gBrowser.multiSelectionTabsCount, 1, ...".

Re-running all browser_multi_select_tabs* tests in browser/base/content/test/tabs directory should ensure we are not leaving something broken.

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

(In reply to Abdoulaye O. LY from comment #8)
> Comment on attachment 8987791 [details]
> Bug 1465170 - Implement support for the 'highlighted' API for multiselect
> tabs with tabs.query
> 
> https://reviewboard.mozilla.org/r/253068/#review259724

> Hence, we should change whenever we see "if(mutliSelectedTabsCount)" to
> "if(multiSelectionTabsCount > 1" and in test files change
> "is(gBrowser.multiSelectionTabsCount, 0, ..." to
> "is(gBrowser.multiSelectionTabsCount, 1, ...".

It seems much cleaner to me to have multiSelectionTabsCount not rely on tabsSelected, or at least rename multiSelectionTabsCount to selectedTabsCount.

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

Not sure if I'm doing something wrong, but browser_multiselect_tabs_mute_unmute.js uses https://example.com/browser/browser/base/content/test/general/file_mediaPlayback.html, and for me this produces 404 Not Found and the test fails.

	Abdoulaye O. LY
	Comment 11 • 6 years ago

Its curious it doesn't work to you. Check if all supported files for this test file are well listed on https://searchfox.org/mozilla-central/source/browser/base/content/test/tabs/browser.ini#21. 
 
Maybe you can leave multiSelectedTabsCount unmodified so that it doesn't rely on tabsSelected as Shane suggested. This will help us avoid doing all these changes at least for now.

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

(In reply to Oriol Brufau [:Oriol] from comment #10)
> Not sure if I'm doing something wrong, but
> browser_multiselect_tabs_mute_unmute.js uses
> https://example.com/browser/browser/base/content/test/general/
> file_mediaPlayback.html, and for me this produces 404 Not Found and the test
> fails.

Do you still get that error if you run `./mach test browser/base/content/test/general` first, then run `./mach test browser/base/content/test/tabs` ?

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

(In reply to (away 7/1-7/7) Jared Wein [:jaws] (please needinfo? me) from comment #12)
> Do you still get that error if you run `./mach test
> browser/base/content/test/general` first, then run `./mach test
> browser/base/content/test/tabs` ?

No, then it works.

(In reply to Abdoulaye O. LY from comment #11)
> Its curious it doesn't work to you. Check if all supported files for this
> test file are well listed on
> https://searchfox.org/mozilla-central/source/browser/base/content/test/tabs/
> browser.ini#21. 

The ini has

[browser_multiselect_tabs_mute_unmute.js]
support-files =
  ../general/audio.ogg
  ../general/file_mediaPlayback.html

	Jared Wein [:jaws] (please needinfo? me) Reporter
	Comment 15 • 6 years ago
mozreview-review

Comment on attachment 8987791 [details]
Bug 1465170 - Implement support for the 'highlighted' API for multiselect tabs with tabs.query

https://reviewboard.mozilla.org/r/253068/#review260024

	Pulsebot
	Comment 18 • 6 years ago

Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5ac6067017a8
Implement support for the 'highlighted' API for multiselect tabs with tabs.query r=jaws,mixedpuppy

	Cosmin Sabou [:CosminS]
	Comment 19 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/5ac6067017a8

	Vlad Jiman
	Comment 20 • 6 years ago

Is manual testing required on this bug? If yes, please provide some STR and the proper extension(if required) or set the “qe-verify -“ flag.

Thank you!

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

Covered by automated tests

	Irene Smith
	Comment 22 • 6 years ago

Added the following to the release notes:

tabs.query now returns an array of tabs.Tab objects if multiple tabs are selected (bug 1465170).

The documentation already describes getting an array of tab objects, and BCD says it is supported by Firefox, so no change seems to be necessary.

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

It's OK, before 63 there was no way to select multiple tabs, so it wasn't observable whether 'highlighted' in tabs.query was fully implemented or not.