Closed Bug 1235571 Opened 9 years ago Closed 9 years ago

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

Categories

(WebExtensions :: Untriaged, defect, P5)

defect

Tracking

(firefox47 fixed)

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: andy+bugzilla, Assigned: kmag)

References

Details

(Whiteboard: triaged)

Attachments

(1 file)

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.
Priority: -- → P5
Whiteboard: triaged
Assignee: nobody → kmaglione+bmo
Blocks: 1234086
Blocks: 1244496
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 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.
Attachment #8714544 - Flags: review?(wmccloskey) → review+
https://hg.mozilla.org/integration/fx-team/rev/9ea6b7becbd398de4563a02bbad832200dca1453
Bug 1235571: Add an `adoptTab` method to <tabbrowser> and remove duplicated code. r=billm
https://hg.mozilla.org/mozilla-central/rev/9ea6b7becbd3
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: