1235571 - Move some of chrome.tabs.move down in tabbrowser.xml

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect, P5)

Description

[Andy McKay]

The implementation of chrome.tabs.move in bug 1214007 uses some code that's pretty similar to the code in tabbrowser.xml about moving tabs. It's a method to move tabs between windows that could be re-used. 

To allow that re-use, let's move the code (and corresponding tests) down from browser/components/extensions/ext-tabs.js into browser/base/content/tabbrowser.xml.

Comment 1

[Kris Maglione [:kmag]]

Attachment: MozReview Request: Bug 1235571: Add an `adoptTab` method to <tabbrowser> and remove duplicated code. r?billm

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

Comment 2

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/33143/#review29885

::: browser/components/extensions/ext-tabs.js:603
(Diff revision 1)
> +          if (!ok) {

This matches Chrome's behavior. This change was necessary to make the tab reparenting logic compatible with the logic used for drag and drop.

Comment 3

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

Comment on attachment 8714544 [details]
MozReview Request: Bug 1235571: Add an `adoptTab` method to <tabbrowser> and remove duplicated code. r?billm

https://reviewboard.mozilla.org/r/33143/#review30793

Thanks!

::: browser/base/content/tabbrowser.xml:2965
(Diff revision 1)
> +          if (aIndex < numPinned || aTab.pinned && aIndex == numPinned)

Please use braces here and also parenthesize the && expression so it's clear that it has precedence.

::: browser/base/content/tabbrowser.xml:2970
(Diff revision 1)
> +          // We need to select the tab before calling swapBrowsersAndCloseOther
> +          // so that window.content in chrome windows points to the right tab
> +          // when pagehide/show events are fired.

This comment is misleading at this point. Our browser chrome no longer relies on pagehide/show events. I guess we might need to do it for add-on compat or older tests. Maybe say that?

::: browser/base/content/tabbrowser.xml:2973
(Diff revision 1)
> +          if (aSelectTab)

Please use parens.

::: browser/base/content/tabbrowser.xml:2979
(Diff revision 1)
> +          // Call updateCurrentBrowser to make sure the URL bar is up to date
> +          // for our new tab after we've done swapBrowsersAndCloseOther.
> +          this.updateCurrentBrowser(true);

We should only do this if |aSelectedTab|.

::: browser/components/extensions/ext-tabs.js:609
(Diff revision 1)
> -            tab.parentNode._finishAnimateTabMove();
> +          if (WindowManager.getId(tab.ownerDocument.defaultView) !== windowId) {

Probably faster to compare tab.ownerDocument.defaultView to window.

Comment 4

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/fx-team/rev/9ea6b7becbd398de4563a02bbad832200dca1453
Bug 1235571: Add an `adoptTab` method to <tabbrowser> and remove duplicated code. r=billm

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9ea6b7becbd3