1238312 - Implement tabs.duplicate API

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect, P2)

Description

[Kris Maglione [:kmag]]

https://developer.chrome.com/extensions/tabs#method-duplicate

Comment 1

[Andy McKay]

Mind if I take this? Think I've got a working patch for it.

Comment 2

[Kris Maglione [:kmag]]

Fine with me

Comment 3

[Andy McKay]

Attachment: chrome.tabs.duplicate.patch

Thanks rpl for showing me the event handling code.

Comment 4

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8709208 [details] [diff] [review]
chrome.tabs.duplicate.patch

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

About the context.callOnClose/forgetOnClose helpers:

I put them in my previous suggestion because I'm a bit worried about the following scenario
  
   an addon context has a listener already subscribed to the event but the context is closed before the event could be delivered to the that context.

Nevertheless, Currently the context.callOnClose/forgetOnClose helpers are not directly used by any of the WebExtension "ext-API.js" files, and in the same file ("ext-tabs.js") tabs.create does something similar and it does not handle the above scenario explicitly:

https://dxr.mozilla.org/mozilla-central/source/browser/components/extensions/ext-tabs.js#302

On this I would like to hear Kris opinion.

Besides the callOnClose/forgetOnClose and the other small comments on the patch, it looks good to me.

::: browser/components/extensions/ext-tabs.js
@@ +609,5 @@
>        },
> +
> +      duplicate: function(tabId, callback) {
> +        let tab = TabManager.getTab(tabId);
> +        if (!tab) {

it could be helpful to log an error if the API method call is invalid (e.g. a non existent tabId), mainly to prevent silent errors (at least until we start to support runtime.lastError)

e.g. Cu.reportError(`Unknown tabId ${tabId} on tabs.duplicate`);

::: browser/components/extensions/test/browser/browser_ext_tabs_duplicate.js
@@ +1,1 @@
> +"use strict";

Nit, all the other test files has the following two lines of comments to pre-configure the indentation style on some editors:

/* -*- Mode: indent-tabs-mode: nil; js-indent-level: 2 -*- */
/* vim: set sts=2 sw=2 et tw=80: */

@@ +1,5 @@
> +"use strict";
> +
> +add_task(function* () {
> +  let tab = yield BrowserTestUtils.openNewForegroundTab(gBrowser, "http://example.net/");
> +  gBrowser.selectedTab = tab;

not needed, openNewForegroundTab already selects the new tab

@@ +16,5 @@
> +      }, function(tabs) {
> +        let source = tabs[1];
> +        // By moving it 0, we check that the new tab is created next
> +        // to the existing one.
> +        browser.tabs.move(source.id, {index: 0}, function(_) {

Nit, our eslint config do not currently checks for unused parameters, but if we do not need the parameter then I would prefer it to be omitted instead

Comment 5

[Kris Maglione [:kmag]]

Comment on attachment 8709208 [details] [diff] [review]
chrome.tabs.duplicate.patch

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

Looks good. Just a few nits.

::: browser/components/extensions/ext-tabs.js
@@ +612,5 @@
> +        let tab = TabManager.getTab(tabId);
> +        if (!tab) {
> +          if (callback) {
> +            runSafe(context, callback, undefined);
> +          }

I'd rather we just not call the callback, and add a TODO comment, until we have `lastError` support. Either way, please add a comment about lastError so when I implement it I'll know where it needs to be set.

@@ +618,5 @@
> +        }
> +
> +        let gBrowser = tab.ownerDocument.defaultView.gBrowser;
> +        let newTab = gBrowser.duplicateTab(tab);
> +        gBrowser.moveTabTo(newTab, gBrowser.visibleTabs.indexOf(tab) + 1);

I wonder if the tab will always be in |visibleTabs|... We should probably check |visibleTabs.includes(tab)| or that the index != -1 first.

@@ +619,5 @@
> +
> +        let gBrowser = tab.ownerDocument.defaultView.gBrowser;
> +        let newTab = gBrowser.duplicateTab(tab);
> +        gBrowser.moveTabTo(newTab, gBrowser.visibleTabs.indexOf(tab) + 1);
> +        gBrowser.selectedTab = newTab;

Does Chrome always select the new tab by default? It's not documented :(

@@ +629,5 @@
> +            if (newTab) {
> +              newTab.removeEventListener("SSTabRestored", onTabRestored);
> +            }
> +          },
> +        };

Let's just check |!context.closed| from the callback rather than adding the onClose listener. It doesn't exist yet, but the patch at the top of my queue adds it.

@@ +638,5 @@
> +          // Remove the one shot listener.
> +          newTab.removeEventListener("SSTabRestored", onTabRestored);
> +
> +          if (callback) {
> +            runSafe(context, callback, TabManager.convert(extension, newTab));

Let's check |if (callback)| before we add the listener. I don't think we have any use for it if there's no callback.

::: browser/components/extensions/test/browser/browser_ext_tabs_duplicate.js
@@ +16,5 @@
> +      }, function(tabs) {
> +        let source = tabs[1];
> +        // By moving it 0, we check that the new tab is created next
> +        // to the existing one.
> +        browser.tabs.move(source.id, {index: 0}, function(_) {

I generally prefer to include the argument and give it a useful name, in case future readers need to know what they are. Either way, please use arrow functions for both of these callbacks.

@@ +20,5 @@
> +        browser.tabs.move(source.id, {index: 0}, function(_) {
> +          browser.tabs.duplicate(source.id, function(tab) {
> +            browser.test.assertEq("http://example.net/", tab.url);
> +            browser.test.assertEq(1, tab.index);
> +            browser.test.assertEq(true, tab.selected);

Would be nice to have descriptions for these. It's pretty hard to make sense of test output when you're comparing against values like |1| and |true|.

@@ +32,5 @@
> +  yield extension.startup();
> +  yield extension.awaitFinish("tabs.duplicate.good");
> +  yield extension.unload();
> +
> +  extension = ExtensionTestUtils.loadExtension({

Please add a separate task for this. Also, it's good to use descriptive names for the task functions, since they show up in test output.

@@ +50,5 @@
> +  yield extension.awaitFinish("tabs.duplicate.invalid");
> +  yield extension.unload();
> +
> +  for (let tab of window.gBrowser.tabs) {
> +    yield BrowserTestUtils.removeTab(tab);

Make sure to only remove the tabs that you created. Trying to remove the first tab can cause problems for other tests.

Comment 6

[Andy McKay]

(In reply to Kris Maglione [:kmag] from comment #5)
> Comment on attachment 8709208 [details] [diff] [review]
> chrome.tabs.duplicate.patch
> 
> Review of attachment 8709208 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good. Just a few nits.
> 
> ::: browser/components/extensions/ext-tabs.js
> @@ +612,5 @@
> > +        let tab = TabManager.getTab(tabId);
> > +        if (!tab) {
> > +          if (callback) {
> > +            runSafe(context, callback, undefined);
> > +          }
> 
> I'd rather we just not call the callback, and add a TODO comment, until we
> have `lastError` support. Either way, please add a comment about lastError
> so when I implement it I'll know where it needs to be set.

In my testing Chrome does call the callback with undefined. lastError should be set as well though, you are right.

> @@ +618,5 @@
> > +        }
> > +
> > +        let gBrowser = tab.ownerDocument.defaultView.gBrowser;
> > +        let newTab = gBrowser.duplicateTab(tab);
> > +        gBrowser.moveTabTo(newTab, gBrowser.visibleTabs.indexOf(tab) + 1);
> 
> I wonder if the tab will always be in |visibleTabs|... We should probably
> check |visibleTabs.includes(tab)| or that the index != -1 first.

Good call.

> @@ +619,5 @@
> > +
> > +        let gBrowser = tab.ownerDocument.defaultView.gBrowser;
> > +        let newTab = gBrowser.duplicateTab(tab);
> > +        gBrowser.moveTabTo(newTab, gBrowser.visibleTabs.indexOf(tab) + 1);
> > +        gBrowser.selectedTab = newTab;
> 
> Does Chrome always select the new tab by default? It's not documented :(

It does.

> @@ +629,5 @@
> > +            if (newTab) {
> > +              newTab.removeEventListener("SSTabRestored", onTabRestored);
> > +            }
> > +          },
> > +        };
> 
> Let's just check |!context.closed| from the callback rather than adding the
> onClose listener. It doesn't exist yet, but the patch at the top of my queue
> adds it.

By the time I get to this closing it will probably be there, so that's good.

> @@ +638,5 @@
> > +          // Remove the one shot listener.
> > +          newTab.removeEventListener("SSTabRestored", onTabRestored);
> > +
> > +          if (callback) {
> > +            runSafe(context, callback, TabManager.convert(extension, newTab));
> 
> Let's check |if (callback)| before we add the listener. I don't think we
> have any use for it if there's no callback.
> 
> ::: browser/components/extensions/test/browser/browser_ext_tabs_duplicate.js
> @@ +16,5 @@
> > +      }, function(tabs) {
> > +        let source = tabs[1];
> > +        // By moving it 0, we check that the new tab is created next
> > +        // to the existing one.
> > +        browser.tabs.move(source.id, {index: 0}, function(_) {
> 
> I generally prefer to include the argument and give it a useful name, in
> case future readers need to know what they are. Either way, please use arrow
> functions for both of these callbacks.
> 
> @@ +20,5 @@
> > +        browser.tabs.move(source.id, {index: 0}, function(_) {
> > +          browser.tabs.duplicate(source.id, function(tab) {
> > +            browser.test.assertEq("http://example.net/", tab.url);
> > +            browser.test.assertEq(1, tab.index);
> > +            browser.test.assertEq(true, tab.selected);
> 
> Would be nice to have descriptions for these. It's pretty hard to make sense
> of test output when you're comparing against values like |1| and |true|.

Fair enough.

> @@ +32,5 @@
> > +  yield extension.startup();
> > +  yield extension.awaitFinish("tabs.duplicate.good");
> > +  yield extension.unload();
> > +
> > +  extension = ExtensionTestUtils.loadExtension({
> 
> Please add a separate task for this. Also, it's good to use descriptive
> names for the task functions, since they show up in test output.
> 
> @@ +50,5 @@
> > +  yield extension.awaitFinish("tabs.duplicate.invalid");
> > +  yield extension.unload();
> > +
> > +  for (let tab of window.gBrowser.tabs) {
> > +    yield BrowserTestUtils.removeTab(tab);
> 
> Make sure to only remove the tabs that you created. Trying to remove the
> first tab can cause problems for other tests.

Sounds good, thanks.

Comment 7

[Kris Maglione [:kmag]]

Comment on attachment 8709208 [details] [diff] [review]
chrome.tabs.duplicate.patch

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

::: browser/components/extensions/ext-tabs.js
@@ +612,5 @@
> +        let tab = TabManager.getTab(tabId);
> +        if (!tab) {
> +          if (callback) {
> +            runSafe(context, callback, undefined);
> +          }

We have support for lastError now, so please do something like |return Promise.reject({message: `Invalid tab ID`: ${tabId}`})|

I'm planning to change `getTab` to just throw an error when the tab ID is invalid some time soon, though, so this should only be temporary in any case.

@@ +629,5 @@
> +            if (newTab) {
> +              newTab.removeEventListener("SSTabRestored", onTabRestored);
> +            }
> +          },
> +        };

On second thought, we should really just check this from `wrapPromise`, before we call any callback for the context, so I wouldn't even worry about checking it here.

If you could file a follow-up bug, though, that would be helpful.

@@ +638,5 @@
> +          // Remove the one shot listener.
> +          newTab.removeEventListener("SSTabRestored", onTabRestored);
> +
> +          if (callback) {
> +            runSafe(context, callback, TabManager.convert(extension, newTab));

Since bug 1234020 has partially landed, we should be returning a promise rather than taking a callback here, so this comment isn't relevant anymore.

Please just add an `"async": "callback"` property to this method's definition in `schemas/tabs.json` and return a promise that resolves when the tab has been restored. You can remove the callback argument too.

Comment 8

[Andy McKay]

Attachment: MozReview Request: bug 1238312 implement tabs.duplicate r?kmag

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

Comment 9

[Kris Maglione [:kmag]]

Comment on attachment 8719066 [details]
MozReview Request: bug 1238312 implement tabs.duplicate r?kmag

https://reviewboard.mozilla.org/r/34857/#review31479

Please update the commit summary for the bug number comes at the start. Tree rules require that format.

::: browser/components/extensions/ext-tabs.js:658
(Diff revision 1)
> +        gBrowser.moveTabTo(newTab, gBrowser.visibleTabs.indexOf(tab) + 1);

I don't think we can depend on the tab being visible here. We should only do this if the tab is in `visibleTabs`, or it's going to wind up at position 0 (which, incidentally, might be illegal if the tab isn't pinned and other tabs are.

Comment 10

[Kris Maglione [:kmag]]

Can you also update browser_ext_tabs_audio.js to use this method rather than its current workaround?

Comment 11

[Andy McKay]

Comment on attachment 8719066 [details]
MozReview Request: bug 1238312 implement tabs.duplicate r?kmag

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

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/f9fc67134ca0

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f9fc67134ca0