1214007 - Implement chrome.tabs.move

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect)

Description

[doncodes]

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/46.0.2490.64 Safari/537.36

Steps to reproduce:

Tried to use chrome.tabs.move from my background page.


Actual results:

Got an error:  chome.tabs.move is not a function


Expected results:

We need this function for our extension.  We use it to swap in a tab that has been rendered in a different window.

Comment 1

[Tom S [:evilpie]]

This whole "rendered in a different window" approach sounds a bit weird to me, but here is the existing code to move a tab from a window: https://dxr.mozilla.org/mozilla-central/rev/1a157155a4fe0074b3d03b54fe9e466472c2cd56/browser/base/content/tabbrowser.xml#5677

Moving a tab in the same window is just tabBrowser.moveTabTo.

Comment 2

[Andy McKay]

Attachment: MozReview Request: Bug 1214007 - implement chrome.tabs.move; r?billm

Bug 1214007 - implement chrome.tabs.move

Comment 3

[:Gijs (he/him)]

https://reviewboard.mozilla.org/r/24561/#review22117

::: browser/components/extensions/ext-tabs.js:542
(Diff revision 1)
> +          // Ignore invalid tab ids.

I'm not familiar with this API, but this implies that if you pass:

[valid, invalid, valid]

the first one gets moved and the third doesn't? Whereas:

[invalid, valid, valid]

gets you nothing? Seems like it should continue; rather than return, or validate all of the tabs before proceeding:

let tabs = tabIds.map(id => TabManager.getTab(id));
if (tabs.some(tab => !tab))
  return;

Comment 4

[:Gijs (he/him)]

Let me know if you need help with the move-tab-to-another-window thing.

Comment 5

[Andy McKay]

https://reviewboard.mozilla.org/r/24561/#review22117

> I'm not familiar with this API, but this implies that if you pass:
> 
> [valid, invalid, valid]
> 
> the first one gets moved and the third doesn't? Whereas:
> 
> [invalid, valid, valid]
> 
> gets you nothing? Seems like it should continue; rather than return, or validate all of the tabs before proceeding:
> 
> let tabs = tabIds.map(id => TabManager.getTab(id));
> if (tabs.some(tab => !tab))
>   return;

Good point, will fix.

Comment 6

[Andy McKay]

Attachment: MozReview Request: cope with invalid tab ids correctly

cope with invalid tab ids correctly

Comment 7

[Andy McKay]

> Let me know if you need help with the move-tab-to-another-window thing.

Thanks, might make that a seperate bug. Still learning how this thing works and all that.. :)

Comment 8

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8684413 [details]
MozReview Request: Bug 1214007 - implement chrome.tabs.move; r?billm

https://reviewboard.mozilla.org/r/24561/#review22627

::: browser/components/extensions/ext-tabs.js:535
(Diff revision 1)
> +        let tabs_moved = [];

Please use camel case: tabsMoved.

::: browser/components/extensions/ext-tabs.js:549
(Diff revision 1)
> +          index = index === -1 ? window.gBrowser.tabs.length : index;

This is going to get messed up if the tabs come from different windows. I'm not too concerned about this case if the index is non-negative. But I could imagine someone wanting to move a set of tabs from different windows to position -1. Can you fix that and add a test to ensure that it works as expected?

::: browser/components/extensions/ext-tabs.js:557
(Diff revision 1)
> +          runSafe(context, callback, tabs_moved.map(function(tab) {TabManager.convert(extension, tab)}));

Please use an arrow function: map(tab => TabManager.convert(extension, tab)).

::: browser/components/extensions/test/browser/browser_ext_tabs_move.js:33
(Diff revision 1)
> +        chrome.tabs.move(tabs.map(function(i) { return i.id; } ), {index: 0});

An arrow function would be good here too. Also, please use "tab" instead of "i".

::: browser/components/extensions/test/browser/browser_ext_tabs_move.js:36
(Diff revision 1)
> +          browser.test.assertEq(tabs[1].url, "about:config", "should be second tab");

Can we check about:robots is third?

Comment 9

[Andy McKay]

https://reviewboard.mozilla.org/r/24561/#review22627

> This is going to get messed up if the tabs come from different windows. I'm not too concerned about this case if the index is non-negative. But I could imagine someone wanting to move a set of tabs from different windows to position -1. Can you fix that and add a test to ensure that it works as expected?

If you move a tab from a different window (something I haven't implemented yet) to this window with an index of -1, where would you expect it to go? According to the docs "The position to move the window to. -1 will place the tab at the end of the window.", so I'm placing it at the end of the current window.

Figuring out how to move tabs between windows is my next job.

Comment 10

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Well, there isn't really a current window. Without a windowId parameter, each tab stays within its own window I think. The docs for move say "Moves one or more tabs to a new position within its window". So I think that if you have tabA in winA and tabB in winB and you call move([tabA, tabB], {index: -1}), then tabA should end up at the end of winA and tabB should end up at the end of winB.

Comment 11

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

If you want, I think it would be fine to do window moving in a separate bug. But I think comment 10 should be fixed.

Comment 12

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8684471 [details]
MozReview Request: cope with invalid tab ids correctly

https://reviewboard.mozilla.org/r/24575/#review22887

This part looks fine.

Comment 13

[Andy McKay]

Attachment: MozReview Request: [mq]: move.windows.patch

[mq]: move.windows.patch

Comment 14

[Andy McKay]

Attachment: MozReview Request: [mq]: move.windows-fixed.patch

[mq]: move.windows-fixed.patch

Comment 15

[Andy McKay]

Comment on attachment 8684413 [details]
MozReview Request: Bug 1214007 - implement chrome.tabs.move; r?billm

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

Comment 16

[Andy McKay]

Sorry for the noise, first time using reviewboard and mercurial.

Comment 17

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8684413 [details]
MozReview Request: Bug 1214007 - implement chrome.tabs.move; r?billm

https://reviewboard.mozilla.org/r/24561/#review24517

::: browser/components/extensions/ext-tabs.js:557
(Diff revision 2)
> +        function getIndex(wid, idx) {
> +          return indexMap[wid] || idx;
> +        }
> +
> +        function setIndex(wid, idx) {
> +          indexMap[wid] = indexMap[wid] || index;
> +          if (idx !== -1) {
> +            indexMap[wid]++;
> +          }
> +        }

This is close, but not quite right. Let's have a single function, getInsertionPoint. It should be nested inside the loop so it has access to |window|. It would look like:

let getInsertionPoint = () => {
  let point = indexMap.get(window) || index;
  if (point == -1) {
    point = gBrowser.tabs.length;
  }
  indexMap.set(window, point + 1);
  return point;
};

::: browser/components/extensions/ext-tabs.js:581
(Diff revision 2)
> +          index = index === -1 ? gBrowser.tabs.length : index;

...then you can remove this line.

::: browser/components/extensions/ext-tabs.js:584
(Diff revision 2)
> +            // If the window we are moving the tab in is different, then move the tab.
> +            let newTab = gBrowser.addTab('about:blank');
> +            let newBrowser = gBrowser.getBrowserForTab(newTab);
> +            gBrowser.updateBrowserRemotenessByURL(newBrowser, tab.linkedBrowser.currentURI.spec);
> +            newBrowser.stop();
> +            newBrowser.docShell;
> +
> +            if (tab.pinned) {
> +              gBrowser.pinTab(newTab);
> +            }
> +
> +            gBrowser.moveTabTo(newTab, getIndex(windowId, index));
> +            setIndex(windowId, index);
> +
> +            gBrowser.selectedTab = newTab;
> +            tab.parentNode._finishAnimateTabMove();
> +            gBrowser.swapBrowsersAndCloseOther(newTab, tab);

This code should be shared with the code in tabbrowser. You can add a method there called moveBrowserToWindow and then we can call it from the drop code and from here.

However, I think it would be fine to make this change in a separate bug.

::: browser/components/extensions/ext-tabs.js:601
(Diff revision 2)
> +            gBrowser.updateCurrentBrowser(true);

I don't think we want to do this.

::: browser/components/extensions/ext-tabs.js:604
(Diff revision 2)
> +            gBrowser.moveTabTo(tab, getIndex(windowId, index));

...and you only need to call getInsertionPoint here, and skip setIndex.

Comment 18

[Andy McKay]

Attachment: MozReview Request: Bug 1214007 - fix up chrome.tabs.move as per feedback; r?billm

Bug 1214007 - fix up chrome.tabs.move as per feedback; r?billm

Comment 19

[Andy McKay]

Comment on attachment 8695598 [details]
MozReview Request: Bug 1214007 - fix up chrome.tabs.move as per feedback; r?billm

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

Comment 20

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8695598 [details]
MozReview Request: Bug 1214007 - fix up chrome.tabs.move as per feedback; r?billm

https://reviewboard.mozilla.org/r/27151/#review25593

Please land this as a single patch. Don't forget to file something to consolidate the window switching code.

Comment 21

[Andy McKay]

Comment on attachment 8684413 [details]
MozReview Request: Bug 1214007 - implement chrome.tabs.move; r?billm

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

Comment 22

[Andy McKay]

The recent changes to type checking meant that when I updated to the latest and ran tests it failed, so I fixed up the patch. The main change is in the tests to use all_urls or lastFocusedWindow to get the windows. I figure with that many changes, a quick re-review would be in order if that's ok.

Comment 23

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8684413 [details]
MozReview Request: Bug 1214007 - implement chrome.tabs.move; r?billm

https://reviewboard.mozilla.org/r/24561/#review25843

This looks fine. It would be good to run eslint though. I see a lot of single quotes, which will definitely be flagged.

Comment 24

[Andy McKay]

Comment on attachment 8684413 [details]
MozReview Request: Bug 1214007 - implement chrome.tabs.move; r?billm

https://reviewboard.mozilla.org/r/24561/#review26273

billm gave it an r+ in bugzilla.

Comment 25

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e905065699ef

Comment 26

[Carsten Book [:Tomcat]]

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

Comment 27

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/fx-team/rev/a855336ab34a663b898089b0c9b83642962a0316
Bug 1214007: Follow-up: Fix ESLint errors. r=trivial

Comment 28

[Ryan VanderMeulen [:RyanVM]]

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