Status

enhancement
P3
normal
RESOLVED FIXED
a year ago
11 months ago

People

(Reporter: Oriol, Assigned: Oriol)

Tracking

({dev-doc-complete})

unspecified
mozilla61
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox61 fixed)

Details

Attachments

(2 attachments)

Assignee

Description

a year ago
Right now I have 1894 open tabs. It makes no sense that, if an extension does

    browser.tabs.query({active: true})

all of them will be iterated.

I guess having lots of windows is less usual but it can also be optimized, i.e. if you specify a windowId all windows will be checked, but this should just be a hash lookup.

So I think all of these can be optimized:

 - If windowId is specified, only iterate that window
 - If currentWindow is true, only iterate the current window
 - If active is true, only iterate the selected tabs
 - If index is true, only iterate the tabs at that index
If there are optimizations that can happen with the current api, we can do a bug for that.  design-decision-needed for new functionality.
Whiteboard: [design-decision-needed]
Assignee

Comment 2

a year ago
Yes this is all with the current API, there is no new functionality. It's just that we shouldn't bother checking windows or tabs that are not going to match.
I think this is all engineering/implementation decisions, no api design questions
Whiteboard: [design-decision-needed]
Assignee

Comment 4

a year ago
OK, it seems optimizing this for windows would require getWindow to be smarter
https://searchfox.org/mozilla-central/rev/99df6dad057b94c2ac4d8b12f7b0b3a7fdf25d04/toolkit/components/extensions/ext-tabs-base.js#1387-1398

Like getTab, which uses a map
https://searchfox.org/mozilla-central/rev/99df6dad057b94c2ac4d8b12f7b0b3a7fdf25d04/browser/components/extensions/ext-browser.js#303-312

But if not many people use lots of windows, maybe it's not worth and tabs.query should only be optimized for tabs.
Assignee

Comment 6

a year ago
I think this should do the trick. windowId will only be optimized if it's WINDOW_ID_CURRENT, but getWindow may be tweaked in another bug if necessary.
Assignee: nobody → oriol-bugzilla
Status: NEW → ASSIGNED
FYI.  I'm not convinced on the additional code to shortcut the window query. I'm trying to find any data on how many open windows are typical.  I'm fine with the shortcut query for tabs.

Comment 8

a year ago
mozreview-review
Comment on attachment 8958624 [details]
Bug 1445316 - Optimize tabs.query with 'active', 'currentWindow', 'highlighted', 'index', 'lastFocusedWindow' or 'windowId'

https://reviewboard.mozilla.org/r/227528/#review235836

::: toolkit/components/extensions/ext-tabs-base.js:1812
(Diff revision 1)
> +          yield window.activeTab;
> +          return;
> +        }
> +        if (index != null) {
> +          let nativeTabs = window.window.gBrowser.tabs;
> +          if (index < nativeTabs.length) {

does query otherwise protect against index < 0?  if not lets add the check.

::: toolkit/components/extensions/ext-tabs-base.js:1921
(Diff revision 1)
>     *        Used to determine the current window for relevant properties.
>     *
>     * @returns {Iterator<WindowBase>}
>     */
>    * query(queryInfo = null, context = null) {
> -    for (let window of this.getAll()) {
> +    function* candidates(windowManager) {

After seeing the numbers, this particular change is optimizing for a *tiny* fraction of users, so I'm really not clear of the value.  Less than 1% have more than 5 windows.

OTOH an extension (or set of extensions) running a lot of window queries would hit that all the time, so perhaps it's fine to go with it.
Attachment #8958624 - Flags: review?(mixedpuppy) → review+
Assignee

Comment 9

a year ago
mozreview-review
Comment on attachment 8958624 [details]
Bug 1445316 - Optimize tabs.query with 'active', 'currentWindow', 'highlighted', 'index', 'lastFocusedWindow' or 'windowId'

https://reviewboard.mozilla.org/r/227528/#review235842

::: toolkit/components/extensions/ext-tabs-base.js:1812
(Diff revision 1)
> +          yield window.activeTab;
> +          return;
> +        }
> +        if (index != null) {
> +          let nativeTabs = window.window.gBrowser.tabs;
> +          if (index < nativeTabs.length) {

The schema has `minimum: 0` (https://searchfox.org/mozilla-central/rev/c217fbde244344fedfd07b57a740c694a456dbca/browser/components/extensions/schemas/tabs.json#718), so I think if it's `< 0` an error would have been thrown before this.
Priority: -- → P3
Assignee

Comment 10

a year ago
(In reply to Oriol Brufau [:Oriol] from comment #9)
> The schema has `minimum: 0`, so I think if it's `< 0` an error would have been thrown before this.

Ugh, it seems this is not the case. Then I wonder what's the point of `minimum: 0`.
Comment hidden (mozreview-request)
Assignee

Comment 12

a year ago
I filed bug 1448120 about the minimum not respected, but added the condition anyways since it seems a fragile assumption.
Keywords: checkin-needed

Comment 13

a year ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/3ec1117a37e1
Optimize tabs.query with 'active', 'currentWindow', 'index' or 'windowId' r=mixedpuppy
Keywords: checkin-needed
Backed out changeset 3ec1117a37e1 (bug 1445316) for c3 & c6 failures in mobile/android/components/extensions/test/mochitest/test_ext_activeTab_permission.html on a CLOSED TREE

backout - https://hg.mozilla.org/integration/autoland/rev/18a7ed496352d44335009571d032da7102892c12
problematic push - https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=3ec1117a37e1f167ad26245ab37ad54bd981ba57&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified
log - https://treeherder.mozilla.org/logviewer.html#?job_id=169748283&repo=autoland&lineNumber=2174

[task 2018-03-22T22:12:20.058Z] 22:12:20     INFO -  172 INFO TEST-UNEXPECTED-FAIL | mobile/android/components/extensions/test/mochitest/test_ext_activeTab_permission.html | Test timed out.
[task 2018-03-22T22:12:20.059Z] 22:12:20     INFO -  reportError@chrome://mochikit/content/tests/SimpleTest/TestRunner.js:121:7
[task 2018-03-22T22:12:20.060Z] 22:12:20     INFO -  TestRunner._checkForHangs@chrome://mochikit/content/tests/SimpleTest/TestRunner.js:142:7
[task 2018-03-22T22:12:20.061Z] 22:12:20     INFO -  173 INFO TEST-UNEXPECTED-FAIL | mobile/android/components/extensions/test/mochitest/test_ext_activeTab_permission.html | Extension left running at test shutdown
[task 2018-03-22T22:12:20.061Z] 22:12:20     INFO -  ExtensionTestUtils.loadExtension/<@chrome://mochikit/content/tests/SimpleTest/ExtensionTestUtils.js:109:7
[task 2018-03-22T22:12:20.062Z] 22:12:20     INFO -  executeCleanupFunction@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:1217:19
[task 2018-03-22T22:12:20.062Z] 22:12:20     INFO -  SimpleTest.finish@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:1230:5
[task 2018-03-22T22:12:20.063Z] 22:12:20     INFO -  killTest@chrome://mochikit/content/tests/SimpleTest/TestRunner.js:130:7
[task 2018-03-22T22:12:20.063Z] 22:12:20     INFO -  delayedKillTest@chrome://mochikit/content/tests/SimpleTest/TestRunner.js:157:47
[task 2018-03-22T22:12:30.174Z] 22:12:30     INFO -  174 INFO TEST-UNEXPECTED-FAIL | mobile/android/components/extensions/test/mochitest/test_ext_activeTab_permission.html | no tasks awaiting on messages - got "[\"extension_popup.activeTab\"]", expected "[]"
[task 2018-03-22T22:12:30.175Z] 22:12:30     INFO -  SimpleTest.is@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:312:5
[task 2018-03-22T22:12:30.175Z] 22:12:30     INFO -  ExtensionTestUtils.loadExtension/<@chrome://mochikit/content/tests/SimpleTest/ExtensionTestUtils.js:31:7
[task 2018-03-22T22:12:30.175Z] 22:12:30     INFO -  executeCleanupFunction@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:1217:19
[task 2018-03-22T22:12:30.176Z] 22:12:30     INFO -  175 INFO TEST-UNEXPECTED-FAIL | mobile/android/components/extensions/test/mochitest/test_ext_activeTab_permission.html | Test left extra windows or tabs: {"extraWindows":[],"extraTabs":["http://example.com/#test_activeTab_pageAction_popup"]}
Flags: needinfo?(oriol-bugzilla)
Assignee

Comment 15

a year ago
Ugh, I didn't consider android.

It's strange because that test only uses browser.tabs.query({active: true}), and activeTab seems properly defined for android in https://searchfox.org/mozilla-central/rev/de5c4376b89c3603341d226a65acea12f8851ec5/mobile/android/components/extensions/ext-utils.js#694-698

However, I suspect window.window.gBrowser.tabs may not work at all.
Comment hidden (mozreview-request)
Assignee

Comment 19

a year ago
Comment on attachment 8958624 [details]
Bug 1445316 - Optimize tabs.query with 'active', 'currentWindow', 'highlighted', 'index', 'lastFocusedWindow' or 'windowId'

I did some changes to make it work on android. Currently window.activeTab.active may be false! Also added some tests about tabs.query with an index parameter.
Attachment #8958624 - Flags: review+ → review?(mixedpuppy)
Assignee

Comment 20

a year ago
By the way, I opened 1000 tabs and ran this code:

  let tabs = 1e3;
  let n = 1e3;
  let arr = new Array(n);
  for (let i = 0; i<n; ++i) {
    let index = Math.floor(Math.random()*tabs);
    arr[i] = browser.tabs.query({index});
  }
  let t = performance.now();
  await Promise.all(arr);
  console.log(performance.now() - t);

Currently it takes ~4000 ms, with my patch it's only ~370 ms. That's 90% less time!

Comment 21

a year ago
mozreview-review
Comment on attachment 8958624 [details]
Bug 1445316 - Optimize tabs.query with 'active', 'currentWindow', 'highlighted', 'index', 'lastFocusedWindow' or 'windowId'

https://reviewboard.mozilla.org/r/227528/#review236662

::: browser/components/extensions/ext-browser.js:885
(Diff revision 3)
>      let {tabManager} = this.extension;
>  
>      return tabManager.getWrapper(this.window.gBrowser.selectedTab);
>    }
>  
> +  getTabAtIndex(index) {

Also add this to WindowBase in ext-tab-base.js.

::: mobile/android/components/extensions/ext-utils.js:698
(Diff revision 3)
> +    // If the current tab is an extension popup tab, we use the parentId to retrieve
> +    // and return the tab that was selected when the popup tab has been opened.
> +    if (selectedTab === tabTracker.extensionPopupTab) {

seems like we need a test for this.
Attachment #8958624 - Flags: review?(mixedpuppy) → review+
Assignee

Comment 22

a year ago
(In reply to Shane Caraveo (:mixedpuppy) from comment #21)
> Also add this to WindowBase in ext-tab-base.js.

Do you mean one of these generic

  throw new Error("Not implemented");

??


> ::: mobile/android/components/extensions/ext-utils.js:698
> (Diff revision 3)
> > +    // If the current tab is an extension popup tab, we use the parentId to retrieve
> > +    // and return the tab that was selected when the popup tab has been opened.
> > +    if (selectedTab === tabTracker.extensionPopupTab) {
> 
> seems like we need a test for this.

test_ext_activeTab_permission.html tests this in test_activeTab_pageAction_popup function. That's why it was failing (the only candidate tab was the popup, but the tab that was considered to be active was its parent). Do you want another test in test_ext_tabs_query.html?
(In reply to Oriol Brufau [:Oriol] from comment #22)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #21)
> > Also add this to WindowBase in ext-tab-base.js.
> 
> Do you mean one of these generic
> 
>   throw new Error("Not implemented");
> 
> ??

Yes.


> > ::: mobile/android/components/extensions/ext-utils.js:698

> > seems like we need a test for this.
> 
> That's why it was failing

cool, that's fine then.
Comment hidden (mozreview-request)

Comment 25

a year ago
mozreview-review
Comment on attachment 8958624 [details]
Bug 1445316 - Optimize tabs.query with 'active', 'currentWindow', 'highlighted', 'index', 'lastFocusedWindow' or 'windowId'

https://reviewboard.mozilla.org/r/227528/#review237072

Hi Oriol,
Thanks for taking care of the changes needed on the Firefox for Android side as well.

Follows some additional review comments, most of them are just small optional nits, 
but I would also like to evaluate with you if we can change that `if (!(error instanceof ExtensionError))` check a bit and prevent it from turning unrelated `ExtensionError` (that may be raised from `windowManager.get` in the future when we apply further changes) to become silent errors.

::: browser/components/extensions/test/browser/browser_ext_tabs_query.js:50
(Diff revision 4)
>            /-1 is too small \(must be at least 0\)/,
>            "tab indices must be non-negative");
>  
> +        let {index, windowId} = tabs[0];
> +        let tabs2 = await browser.tabs.query({index, windowId});
> +        browser.test.assertEq(tabs2.length, 1, "Got one tab at index " + index);

Nit, let's use a template string here.

::: mobile/android/components/extensions/ext-utils.js:709
(Diff revision 4)
> +    let {tabs} = this.window.BrowserApp;
> +    if (index < 0 || index >= tabs.length) {
> +      return null;
> +    }
> +    let {tabManager} = this.extension;
> +    return tabManager.getWrapper(tabs[index]);

Nit, how about just retrieving the nativeTab for the given index and check if it is defined? something like:

```
let nativeTab = this.window.BrowserApp.tabs[index];

if (nativeTab) {
  return this.extension.tabManager.getWrapper(nativeTab);
}

return undefined;
```

(And we should also do it the same for ext-browser.js if we chose to change this one)

::: mobile/android/components/extensions/test/mochitest/test_ext_tabs_query.html:63
(Diff revision 4)
> +    },
> +
> +    background: async function() {
> +      browser.tabs.onCreated.addListener(async function({index, windowId, id}) {
> +        let tabs = await browser.tabs.query({index, windowId});
> +        browser.test.assertEq(tabs.length, 1, "Got one tab at index " + index);

Nit, Let's use a template string here.

::: toolkit/components/extensions/ext-tabs-base.js:1815
(Diff revision 4)
>     *        Used to determine the current window for relevant properties.
>     *
>     * @returns {Iterator<TabBase>}
>     */
>    * query(queryInfo = null, context = null) {
> +    function* candidates(window) {

Nit, `window` is actually a wrapper (an instance of the Window class that inherits the WindowBase class defined in ext-tabs-base.js and implements the platform specific bits, in ext-utils.js for Firefox for Android and in ext-browser.js for Firefox Desktop), maybe it could be better to rename it to `windowWrapper` (or `winWrapper`) to make it immediately clear.

(I know that your change is actually following the convention already used in this method, but given that we are already here it could be the right time to make this method easier to be read correctly).

::: toolkit/components/extensions/ext-tabs-base.js:1943
(Diff revision 4)
> +            // Ignore "Invalid window ID" errors
> +            if (!(error instanceof ExtensionError)) {
> +              throw error;
> +            }

This worries me a bit, I see that the reason may usually be that the windowId doesn't exist (e.g. the window have been destroyed in the meantime, or the window id passed by the extension never existed) but, at least in my opinion, it would be better if we could indentify the conditions in a more reliable way then just checking if any `ExtensionError` has been raised, eg. two possible approaches could be:

- add a third parameter to `windowManager.get` which ensure that it will just return `undefined` if it is unable to find retrieve a windowWrapper for the given windowId

- define a more specific error class that extends ExtensionError but it is only used for the error condition that we want to catch here (e.g. a new `InvalidWindowIdExtensionError` or something like that)

How that sounds to you?
Assignee

Comment 26

a year ago
OK! I also noticed tabs.query can be optimized when `highlighed` or `lastFocusedWindow` are true. Will need some more tests.
Just a note on some irc chat about the exception.  adding a windowManager.has might be the better way to go, check if it exists before getting.  That way the exception can be avoided.  I don't think we've come to a final place with what should be done here.
Comment hidden (mozreview-request)
Assignee

Comment 29

a year ago
I added a third parameter to getWindow, I can also try windowManager.has if you prefer.
Assignee

Updated

a year ago
Attachment #8958624 - Flags: review+ → review?(mixedpuppy)

Comment 30

a year ago
mozreview-review
Comment on attachment 8958624 [details]
Bug 1445316 - Optimize tabs.query with 'active', 'currentWindow', 'highlighted', 'index', 'lastFocusedWindow' or 'windowId'

https://reviewboard.mozilla.org/r/227528/#review237552

::: toolkit/components/extensions/ext-tabs-base.js:1401
(Diff revision 5)
>     * @param {integer} id
>     *        The ID of the window to return.
>     * @param {BaseContext} context
>     *        The extension context for which the matching is being performed.
>     *        Used to determine the current window for relevant properties.
> +   * @param {boolean} [strict = true]

not super happy with a new param here, but it's fine for now, we can always change it later.
Attachment #8958624 - Flags: review?(mixedpuppy) → review+
Comment hidden (mozreview-request)

Comment 32

a year ago
mozreview-review
Comment on attachment 8958624 [details]
Bug 1445316 - Optimize tabs.query with 'active', 'currentWindow', 'highlighted', 'index', 'lastFocusedWindow' or 'windowId'

https://reviewboard.mozilla.org/r/227528/#review237954

Currently r-, because I think we should make the results of querying the highlighted tab to behave (as much as possible) in the same way on both Desktop and Android (and in the process of fixing this issue, I'm for killing the new `highlightedTab` getter in favor of just using the existent `activeTab` one)

Anyway, this is going to become an r+ as soon as we fixed these issues.

::: commit-message-718e0:1
(Diff revision 6)
> +Bug 1445316 - Optimize tabs.query with 'active', 'currentWindow', 'highlighed', 'index', 'lastFocusedWindow' or 'windowId'

Let's fix this typo in the commit message: s/highlighed/highlighted/

::: browser/components/extensions/ext-browser.js:885
(Diff revision 6)
>      let {tabManager} = this.extension;
>  
>      return tabManager.getWrapper(this.window.gBrowser.selectedTab);
>    }
>  
> +  get highlighedTab() {

As in the above comment, this should be `highlightedTab` and not `highlighedTab` (same typo is also present elsewhere in this patch where this new helper is used).

But to be fair, I would really prefer to remove this new getter and just reuse the `activeTab` getter to shortcut both the query for the active and the highlighted tabs

::: mobile/android/components/extensions/ext-utils.js:708
(Diff revision 6)
> +  get highlighedTab() {
>      let {tabManager} = this.extension;
>  
>      return tabManager.getWrapper(this.window.BrowserApp.selectedTab);
>    }

Besides the same typo in the getter name (as for the same typo in the related base class and the other similar class defined in "browser/components/extensions/ext-browser.js"), I'm worried that on Firefox Desktop both `activeTab` and `highlightedTab` getter return the same tab, while on Firefox for Android they will return different tabs when a popup is currently selected (and there is even a new test that would enforce this behavior in this patch).

In my opinion we could just avoid to define a new getter for the highlighted tab, especially because there is no concept of highlighted tab in Firefox, the WebExtensions APIs consider it as just an alias of the active tab, and so we could just use the existent `activeTab` getter to shortcut the query related to both the highlighted and/or active tabs.

If we want to add an explicit `highlightedTab` getter (but I can't really think of any reasons for doing it), it would be better that `highlightedTab` behaves the same on both Desktop and Android, it could just return the value of the selectedTab getter:

```
get highlightedTab() {
  return this.activeTab;
}
```

::: mobile/android/components/extensions/test/mochitest/test_ext_tabs_query.html:50
(Diff revision 6)
> +        let active = await browser.tabs.query({active: true});
> +        let highlighted = await browser.tabs.query({highlighted: true});
> +
> +        browser.test.assertEq(1, active.length, "should have one active tab");
> +        browser.test.assertEq(1, highlighted.length, "should have one highlighted tab");
> +        browser.test.assertTrue(active[0].id !== highlighted[0].id, "the active and highlighted tabs are different");

Like described in the other comment related to the behavior of `tabs.query({highlighted: true})`, this should actually test the opposite: we want that the active and highlighted tab is actually the same one.

::: toolkit/components/extensions/ext-tabs-base.js:1831
(Diff revision 6)
> -          yield tab;
> +        if (active === true) {
> +          yield windowWrapper.activeTab;
> +          return;
> +        }
> +        if (highlighed === true) {
> +          yield windowWrapper.highlighedTab;
> +          return;
> +        }

Like I described above, I'm for removing the new `highlightedTab` getter completely and change this fragment to just use the existent `windowWrapper.activeTab` for both `active === true` and `highlighted === true`.
Attachment #8958624 - Flags: review?(lgreco) → review-
Assignee

Comment 33

a year ago
(In reply to Luca Greco [:rpl] from comment #32)
> If we want to add an explicit `highlightedTab` getter (but I can't really
> think of any reasons for doing it), it would be better that `highlightedTab`
> behaves the same on both Desktop and Android, it could just return the value
> of the selectedTab getter:

JFYI `highlighted` and `active` have different behaviors because you recommended so in bug 1438666 comment 4 XD. I added a `highlightedTab` so that `windowWrapper.highlightedTab.highlighted` is always true. So I guess `highlighted` should be changed to be a synonym of `active`.
Comment hidden (mozreview-request)
(In reply to Oriol Brufau [:Oriol] from comment #33)
> (In reply to Luca Greco [:rpl] from comment #32)
> > If we want to add an explicit `highlightedTab` getter (but I can't really
> > think of any reasons for doing it), it would be better that `highlightedTab`
> > behaves the same on both Desktop and Android, it could just return the value
> > of the selectedTab getter:
> 
> JFYI `highlighted` and `active` have different behaviors because you
> recommended so in bug 1438666 comment 4 XD. I added a `highlightedTab` so
> that `windowWrapper.highlightedTab.highlighted` is always true. So I guess
> `highlighted` should be changed to be a synonym of `active`.

Oh, I'm sorry, you are definitely right, unfortunately this "extension popup as a tab"
is as annoying and it is confusing (with any behavior we may choose).

Nevertheless, aliasing highlighted and active more clearly makes the shared and 
Firefox Desktop simpler and less confusing, and so I definitely prefer this version,
even if it sightly changes highlighted property behavior on Firefox for Android 
(especially given that the "extension popup as a tab" is by itself  a sub-optimal 
workaround, I would prefer that we would not make the Desktop and shared versions more
complicate just for that), and so in the end I was wrong in Bug 1438666 comment 4 :-)

Comment 36

a year ago
mozreview-review
Comment on attachment 8958624 [details]
Bug 1445316 - Optimize tabs.query with 'active', 'currentWindow', 'highlighted', 'index', 'lastFocusedWindow' or 'windowId'

https://reviewboard.mozilla.org/r/227528/#review238056

r+ on green try
Attachment #8958624 - Flags: review?(lgreco) → review+
Comment hidden (mozreview-request)
Assignee

Comment 38

a year ago
Some minor edits to the test, hopefully now it will be green.
Comment hidden (mozreview-request)
Assignee

Comment 40

a year ago
Oh, it seems the failure was because Firefox for android does not have any browser.windows API. I removed that test, testing that on desktop should be enough.
(In reply to Oriol Brufau [:Oriol] from comment #40)
> Oh, it seems the failure was because Firefox for android does not have any
> browser.windows API. I removed that test, testing that on desktop should be
> enough.

oh, yes, currently there is no browser.windows API available on Firefox for Android.

Firefox for Android doesn't actually have the concept of multiple windows (but visually,
it has two "stacks" of tabs, one for the regular tabs and one for the private browsing ones), 
to be fair it would be nice if we could provide a browser.windows API which still provides the API
methods and keeps into account the Firefox for Android characteristics, but that is definitely 
a separate issue (which is tracked by Bug 1404014).
Assignee

Comment 42

a year ago
OK, try is green now!
Keywords: checkin-needed
Comment hidden (mozreview-request)
Assignee

Comment 44

a year ago
Just a rebase, ext-browser.js and ext-tabs-base.js were moved into a subfolder.

Comment 45

a year ago
Pushed by csabou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e21ef2d2f2de
Optimize tabs.query with 'active', 'currentWindow', 'highlighted', 'index', 'lastFocusedWindow' or 'windowId' r=mixedpuppy,rpl
Keywords: checkin-needed

Comment 46

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e21ef2d2f2de
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Assignee

Comment 47

a year ago
Bug 1445316 added support for 'highlighted' in firefox 60, but it wasn't a perfect alias of 'active'. I guess this reduced patch should be uplifted.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c1bd12541ec1f2ce7b6fd92eb566dadafa30344e
Attachment #8964176 - Flags: review?(lgreco)
Comment on attachment 8964176 [details] [diff] [review]
smaller-patch-for-beta.patch

Review of attachment 8964176 [details] [diff] [review]:
-----------------------------------------------------------------

Hi Oriol,
I just discussed about this with Shane and we both agreed that uplifting the reduced patch it doesn't really seem worth it (e.g. we are going to diverge more from what we landed in m-c and the part we are uplifting is only going to potentially affect a less commonly used property of the tabs API on a platform with a much lower number of extensions used, and extensions users).
Uplifting the tabs API optimization could have been a more compelling reason to uplift a patch to beta, but we agreed (Shane and I) that it would be a larger and potentially more risky change at this point of the beta cycle, and so we are very inclined to don't uplift any of it on beta.
Attachment #8964176 - Flags: review?(lgreco) → review-
Assignee

Comment 49

a year ago
Then I guess it should be documented that highlighted in tabs.query is not a perfect alias of active in firefox60 for android, but it is in firefox61.
Keywords: dev-doc-needed

Comment 50

a year 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.

Thanks!
Flags: needinfo?(mixedpuppy)
has tests.
Flags: needinfo?(mixedpuppy) → qe-verify-
(In reply to Oriol Brufau [:Oriol] from comment #49)
> Then I guess it should be documented that highlighted in tabs.query is not a
> perfect alias of active in firefox60 for android, but it is in firefox61.

Is this correct?

* from Firefox 61 onwards, the `highlighted` option in tabs.query is an alias for `active`, in both Desktop and Android Firefoxes
* in Firefox 60, the `highlighted` option is an alias for `active` in Desktop, but in Android they are different when an extension has a popup open. In this situation `highlighted` will return the popup, while `active` will return the tab that was selected before the popup opened.

(As you noted in https://bugzilla.mozilla.org/show_bug.cgi?id=1438666#c4 though, does this also mean that, from Firefox 61, querying for `active` from an extension popup will behave differently on Desktop and on Android?)
Flags: needinfo?(oriol-bugzilla)
Flags: needinfo?(lgreco)
Assignee

Comment 53

a year ago
(In reply to Will Bamberg [:wbamberg] from comment #52)
> Is this correct?
> 
> * from Firefox 61 onwards, the `highlighted` option in tabs.query is an
> alias for `active`, in both Desktop and Android Firefoxes
> * in Firefox 60, the `highlighted` option is an alias for `active` in
> Desktop, but in Android they are different when an extension has a popup
> open. In this situation `highlighted` will return the popup, while `active`
> will return the tab that was selected before the popup opened.

Yes, that's it.

> (As you noted in https://bugzilla.mozilla.org/show_bug.cgi?id=1438666#c4
> though, does this also mean that, from Firefox 61, querying for `active`
> from an extension popup will behave differently on Desktop and on Android?)

On desktop, extension popups are not opened in a new tab, so the active/highlighted tab is just the selected tab.
On android they open in a new tab, selected. But active/highlighted are not supposed to refer to the popup, so instead they refer to the parent of the selected tab.
Having said that, no, querying for `active` in 61 should behave like in 60 (just faster). It's querying for `highlighted` what has changed in 61 for android.
Flags: needinfo?(oriol-bugzilla)
I've updated the compat data for tabs.query with this (in particular, the notes attached to changeInfo.highlighted: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/tabs/query#Browser_compatibility

Marking this one dev-doc-complete, but please let me know if you need anything else.
I confirm what Oriol has described in Comment 53 (thanks Oriol).

Thanks Will, the updated notes in the docs' Browser compatibility table look good to me.
Flags: needinfo?(lgreco)

Updated

11 months ago
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.