1394376 - Resolve browser.tabs.duplicate() promise right away (before tabs are completely loaded)

Status: RESOLVED FIXED

(WebExtensions :: General, enhancement, P5)

Description

[YUKI "Piro" Hiroshi]

browser.tabs.duplicate() returns a promise which is resolved after all duplicated tabs are completely loaded.

https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/tabs/duplicate
https://developer.chrome.com/extensions/tabs#method-duplicate

But we sometime need to do something for duplicated tabs immediately. For example, my Tree Style Tab addon provides ability to duplicate a tree (multiple tabs) at dropped position by drag-and-drop with Ctrl key.

https://github.com/piroor/treestyletab/tree/master/webextensions

Currently I have to wait all duplicated tabs are loaded before reorganize them as a duplicated tree at the dropped position. It is painfully slow for me...

So I propose a new option to resolve the returned promise just after tabs are inserted into the tab bar, like:

var duplicatedTabs = await browser.tabs.duplicate([1, 2, 3], {
                       immediately: true });

The option name "immediately" is just an example and I need more better naming.

How about this?

Comment 1

[YF (Yang)]

https://hg.mozilla.org/mozilla-central/annotate/db7f19e26e571ae1dd309f5d2f387b06ba670c30/browser/components/extensions/ext-tabs.js#l619

This promise be resolved when the tab has been restored, which may in order to the index and pinned properties is correct. But this brings hundreds of ms or higher delay. So I think a new option is reasonable.


In addition, the select tab behavior may be unnecessary for some cases and lead to unnecessary UI jumps, maybe also require a new option, and a new bug? Maybe an option to return immediately and allow to override the default behavior is simple.

Comment 2

[YUKI "Piro" Hiroshi]

I've tried to do something when the duplicated tab appears in the tab bar with a combination of browser.tabs.onCreated and browser.sessions.getTabValue (getTabValue returns something value copied from the original tab but the tab has different id, so it means the tab is duplicated.) to accelerate my addon, but it doesn't work as expected because tabs can be moved after browser.tabs.onCreated, before the promise is resolved. So I think that this is a too hard problem for addon developers. I cannot find out any stable workaround yet...

Comment 3

[Caitlin Neiman [:caitmuenster]]

Hi Piro, this has been added to the agenda for the January 30, 2018 WebExtensions APIs triage. Would you be able to join us? 

Here’s a quick overview of what to expect at the triage: 

* We normally spend 5 minutes per bug
* The more information in the bug, the better
* The goal of the triage is to give a general thumbs up or thumbs down on a proposal; we won't be going deep into implementation details

Relevant Links: 

* Wiki for the meeting: https://wiki.mozilla.org/WebExtensions/Triage#Next_Meeting
* Meeting agenda: https://docs.google.com/document/d/1x80jYXicAotNjlitY5RZDcSRpRM3lmaSHp_q4co4OEg/edit#
* Vision doc for WebExtensions: https://wiki.mozilla.org/WebExtensions/Vision

Comment 4

[YUKI "Piro" Hiroshi]

OK, I'll join to IRC channel for the meeting.

Comment 5

[Tim Nguyen :ntim]

In the meeting, it was agreed that this should be the default behaviour for tabs.duplicate() (without needing any option).

Comment 6

[Tim Nguyen :ntim]

The code for tabs.duplicate is here: https://dxr.mozilla.org/mozilla-central/source/browser/components/extensions/ext-tabs.js#715

Seems like the solution would be resolving just after adding both listeners, or resolving at the end of the `SSTabRestoring` listener as opposed to the `SSTabRestored` listener.

Comment 7

[ariasuni]

I still have this problem, there’s a workaround in TreeStyleTab but we also hit the problem in Tab Center Redux.

Comment 8

[Tim Nguyen :ntim]

Updated link: https://dxr.mozilla.org/mozilla-central/rev/4e56a2f51ad739ca52046723448f3129a58f1666/browser/components/extensions/parent/ext-tabs.js#893-921

Comment 9

[Firefox Bug Husbandry Bot]

Bulk move of bugs per https://bugzilla.mozilla.org/show_bug.cgi?id=1483958

Comment 10

[Caitlin Neiman [:caitmuenster]]

If this is your first contribution, please refer to https://wiki.mozilla.org/WebExtensions/Contribution_Onramp on how to get started. 

Mentor: zombie

Comment 11

[ariasuni]

Attachment: Bug 1394376 - Resolve browser.tabs.duplicate() promise as soon as possible

Don’t wait for tab to be fully loaded to resolve the promise.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/88d980179ff9
Resolve browser.tabs.duplicate() promise as soon as possible r=zombie

Comment 13

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/88d980179ff9

Comment 14

[Caitlin Neiman [:caitmuenster]]

Thanks for the patch, ariasuni! Your contribution has been added to our recognition wiki: https://wiki.mozilla.org/Add-ons/Contribute/Recognition

Would you be interested in creating an account on mozillians.org? I'd be happy to vouch for you!

Comment 15

[Tomislav Jovanovic :zombie]

Hi Mike, this might break some extensions that expect the duplicated tab to be fully loaded when the promise resolves, and should probably be mentioned in the release notes blog post.

Comment 16

[ariasuni]

(In reply to Caitlin Neiman [:caitmuenster] from comment #14)

Thanks for the patch, ariasuni! Your contribution has been added to our recognition wiki: https://wiki.mozilla.org/Add-ons/Contribute/Recognition

Would you be interested in creating an account on mozillians.org? I'd be happy to vouch for you!

I created a profile here: https://mozillians.org/fr/u/ariasuni/ :)

Comment 17

[Caitlin Neiman [:caitmuenster]]

(In reply to ariasuni from comment #16)

I created a profile here: https://mozillians.org/fr/u/ariasuni/ :)

You are vouched! 🎉 Welcome onboard! I look forward to seeing you around the project. :)

Comment 18

[Mike Conca [:mconca]]

(In reply to Tomislav Jovanovic :zombie from comment #15)

Hi Mike, this might break some extensions that expect the duplicated tab to be fully loaded when the promise resolves, and should probably be mentioned in the release notes blog post.

Thanks. Let's make sure this behavior is explicitly noted in the documentation on MDN, too.

Comment 19

[Madalin Cotetiu]

Can you please provide some STR for this issue so we can check it manually? If no manual testing is needed please mark it as "qe-verify- "

Comment 20

[Tomislav Jovanovic :zombie]

No need, covered by automated tests.

Comment 21

[Irene Smith]

Added the following note to the page tabs.duplicate:

Note: Beginning with Firefox 68, the promise returned by browser.tabs.duplicate() resolves immediately for performance reasons. Your extension should not depend on the duplicated tab to be fully loaded when the promise resolves.

If this isn't enough information, please let me know.

Comment 22

[Rob Wu [:robwu]]

There is no performance reason for this; the main motivation for this feature is to allow tab manager extensions to update their own UI as soon as possible.

The next note,

Your extension should not depend on the duplicated tab to be fully loaded when the promise resolves.

... is a bug. This has been fixed in Firefox 69 and uplifted to ESR 68 - see bug 1559216

I'd change the note to "... resolves as soon as the tab has been duplicated. Previously, the promise only resolved once the tab had fully been loaded."

If you can find a concise way to mention the regression/fix from bug 1559216, include it, but otherwise you may omit it.

Comment 23

[Irene Smith]

Made the change. Also, the BCD information is now showing on the page.