Implement chrome.tabs.move

RESOLVED FIXED in Firefox 46

Status

defect
RESOLVED FIXED
4 years ago
Last year

People

(Reporter: doncodes, Assigned: andy+bugzilla)

Tracking

({dev-doc-complete})

44 Branch
mozilla46
Dependency tree / graph
Bug Flags:
blocking-webextensions +

Firefox Tracking Flags

(firefox46 fixed)

Details

(Whiteboard: [tabs])

Attachments

(1 attachment, 4 obsolete attachments)

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.
Blocks: webext-tabs
Whiteboard: [tabs]
Status: UNCONFIRMED → NEW
Ever confirmed: true
Blocks: webext
Assignee: nobody → evilpies
Assignee: evilpies → nobody
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.
Flags: blocking-webextensions+
Assignee: nobody → amckay
Iteration: --- → 45.1 - Nov 16
Bug 1214007 - implement chrome.tabs.move
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;
Let me know if you need help with the move-tab-to-another-window thing.
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.
cope with invalid tab ids correctly
> 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.. :)
Attachment #8684413 - Flags: review?(wmccloskey)
Attachment #8684471 - Flags: review?(wmccloskey)
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?
Attachment #8684413 - Flags: review?(wmccloskey)
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.
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.
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.
Iteration: 45.1 - Nov 16 → 45.2 - Nov 30
Comment on attachment 8684471 [details]
MozReview Request: cope with invalid tab ids correctly

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

This part looks fine.
Attachment #8684471 - Flags: review?(wmccloskey) → review+
Iteration: 45.2 - Nov 30 → 45.3 - Dec 14
[mq]: move.windows.patch
[mq]: move.windows-fixed.patch
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/
Attachment #8684413 - Attachment description: MozReview Request: Bug 1214007 - implement chrome.tabs.move → MozReview Request: Bug 1214007 - implement chrome.tabs.move; r?billm
Attachment #8684413 - Flags: review?(wmccloskey)
Attachment #8684471 - Attachment is obsolete: true
Attachment #8695070 - Attachment is obsolete: true
Attachment #8695071 - Attachment is obsolete: true
Sorry for the noise, first time using reviewboard and mercurial.
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.
Attachment #8684413 - Flags: review?(wmccloskey)
Bug 1214007 - fix up chrome.tabs.move as per feedback; r?billm
Attachment #8695598 - Flags: review?(wmccloskey)
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 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.
Attachment #8695598 - Flags: review?(wmccloskey) → review+
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/
Attachment #8684413 - Flags: review?(wmccloskey)
Attachment #8695598 - Attachment is obsolete: true
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.
Attachment #8684413 - Flags: review?(wmccloskey) → review+
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.
See Also: → 1235571
Keywords: checkin-needed
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.
Attachment #8684413 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/e905065699ef
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.