Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Implement chrome.tabs.move

RESOLVED FIXED in Firefox 46

Status

()

Toolkit
WebExtensions: Untriaged
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: doncodes, Assigned: andym)

Tracking

({dev-doc-complete})

44 Branch
mozilla46
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
blocking-webextensions +

Firefox Tracking Flags

(firefox46 fixed)

Details

(Whiteboard: [tabs])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

2 years ago
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.
(Assignee)

Updated

2 years ago
Blocks: 1213477
Whiteboard: [tabs]

Updated

2 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Updated

2 years ago
Blocks: 1214433
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.
(Assignee)

Updated

2 years ago
Flags: blocking-webextensions+
(Assignee)

Updated

2 years ago
Assignee: nobody → amckay
Iteration: --- → 45.1 - Nov 16
(Assignee)

Comment 2

2 years ago
Created attachment 8684413 [details]
MozReview Request: Bug 1214007 - implement chrome.tabs.move; r?billm

Bug 1214007 - implement chrome.tabs.move

Comment 3

2 years ago
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

2 years ago
Let me know if you need help with the move-tab-to-another-window thing.
(Assignee)

Comment 5

2 years ago
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.
(Assignee)

Comment 6

2 years ago
Created attachment 8684471 [details]
MozReview Request: cope with invalid tab ids correctly

cope with invalid tab ids correctly
(Assignee)

Comment 7

2 years ago
> 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.. :)
(Assignee)

Updated

2 years ago
Attachment #8684413 - Flags: review?(wmccloskey)
(Assignee)

Updated

2 years ago
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)
(Assignee)

Comment 9

2 years ago
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.
Keywords: dev-doc-needed
(Assignee)

Updated

2 years ago
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+
(Assignee)

Updated

2 years ago
Iteration: 45.2 - Nov 30 → 45.3 - Dec 14
(Assignee)

Comment 13

2 years ago
Created attachment 8695070 [details]
MozReview Request: [mq]: move.windows.patch

[mq]: move.windows.patch
(Assignee)

Comment 14

2 years ago
Created attachment 8695071 [details]
MozReview Request: [mq]: move.windows-fixed.patch

[mq]: move.windows-fixed.patch
(Assignee)

Comment 15

2 years ago
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)
(Assignee)

Updated

2 years ago
Attachment #8684471 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8695070 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8695071 - Attachment is obsolete: true
(Assignee)

Comment 16

2 years ago
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)
(Assignee)

Comment 18

2 years ago
Created attachment 8695598 [details]
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
Attachment #8695598 - Flags: review?(wmccloskey)
(Assignee)

Comment 19

2 years ago
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+
(Assignee)

Comment 21

2 years ago
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)
(Assignee)

Updated

2 years ago
Attachment #8695598 - Attachment is obsolete: true
(Assignee)

Comment 22

2 years ago
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.
(Assignee)

Updated

2 years ago
See Also: → bug 1235571
(Assignee)

Updated

2 years ago
Keywords: checkin-needed
(Assignee)

Comment 24

2 years ago
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+

Comment 25

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/e905065699ef
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e905065699ef
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox46: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
https://hg.mozilla.org/integration/fx-team/rev/a855336ab34a663b898089b0c9b83642962a0316
Bug 1214007: Follow-up: Fix ESLint errors. r=trivial

Comment 28

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a855336ab34a

Updated

2 years ago
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.