Update tab's permanentKey as part of _swapBrowserDocShells

RESOLVED FIXED in Firefox 50

Status

()

Firefox
Tabbed Browser
P3
enhancement
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: jryans, Assigned: Kevin Jones)

Tracking

unspecified
Firefox 50
x86
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

In bug 1276514, `permanentKey` was made available on the tab as well as its browser.

tabbrowser.xml's `_swapBrowserDocShells` currently swaps the `permanentKey` fields between the two browsers.  Since this field is now also on the tab, it seems likely that we'd want to update the tab's `permanentKey` to match its browser during swapping.
(Assignee)

Comment 1

a year ago
Created attachment 8768519 [details] [diff] [review]
Patch
Attachment #8768519 - Flags: review?(dao+bmo)
(Assignee)

Updated

a year ago
Attachment #8768519 - Attachment is patch: true
Attachment #8768519 - Flags: review?(dao+bmo) → review+
Priority: -- → P3
Assignee: nobody → allassopraise
I wanted to land this but the patch failed to apply.
Flags: needinfo?(allassopraise)
(Assignee)

Comment 3

a year ago
Should I upload another patch?
Flags: needinfo?(allassopraise)
Yes, we need a patch that can be applied without conflicts.
(Assignee)

Comment 5

a year ago
Created attachment 8769165 [details] [diff] [review]
Patch V2
Attachment #8768519 - Attachment is obsolete: true
Attachment #8769165 - Flags: review?(dao+bmo)
Attachment #8769165 - Flags: review?(dao+bmo) → review+

Comment 6

a year ago
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/f6f33ab815a1
Update tab's permanentKey in _swapBrowserDocShells. r=dao
backed out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=10384506&repo=fx-team#L1905
Flags: needinfo?(allassopraise)

Comment 8

a year ago
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/6cb3dc4bdbae
Backed out changeset f6f33ab815a1 for test bustages like in browser_bug477014.js
I think you need to use the gBrowser for other browser window when calling `getTabForBrowser(aOtherBrowser)`, instead of `this`.  The higher level method `swapBrowsersAndCloseOther` retrieves this as `var remoteBrowser = aOtherTab.ownerDocument.defaultView.gBrowser;`, so maybe it's simpler to update this field as part of `swapBrowsersAndCloseOther`.
(Assignee)

Comment 10

a year ago
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #9)
> I think you need to use the gBrowser for other browser window when calling
> `getTabForBrowser(aOtherBrowser)`, instead of `this`.  The higher level
> method `swapBrowsersAndCloseOther` retrieves this as `var remoteBrowser =
> aOtherTab.ownerDocument.defaultView.gBrowser;`, so maybe it's simpler to
> update this field as part of `swapBrowsersAndCloseOther`.

Actually remoteBrowser is already defined on aOtherBrowser right in _swapBrowserDocShells.  I'm also going to test for `otherTab` because we don't always know there will be a tab associated with aOtherBrowser.
(Assignee)

Comment 11

a year ago
Created attachment 8769295 [details] [diff] [review]
Patch V3
Attachment #8769165 - Attachment is obsolete: true
Flags: needinfo?(allassopraise)
Attachment #8769295 - Flags: review?(jryans)
Comment on attachment 8769295 [details] [diff] [review]
Patch V3

Review of attachment 8769295 [details] [diff] [review]:
-----------------------------------------------------------------

It looks reasonable to me, but I am not a reviewer for /browser code, so let's check with :dao.
Attachment #8769295 - Flags: review?(jryans)
Attachment #8769295 - Flags: review?(dao+bmo)
Attachment #8769295 - Flags: feedback+
Attachment #8769295 - Flags: review?(dao+bmo) → review+

Comment 13

a year ago
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/57d42a4e2ff1
Update tab's permanentKey in _swapBrowserDocShells. r=dao

Comment 14

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/57d42a4e2ff1
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
You need to log in before you can comment on or make changes to this bug.