Closed Bug 1284947 Opened 5 years ago Closed 5 years ago

Update tab's permanentKey as part of _swapBrowserDocShells

Categories

(Firefox :: Tabbed Browser, enhancement, P3)

x86
All
enhancement

Tracking

()

RESOLVED FIXED
Firefox 50
Tracking Status
firefox50 --- fixed

People

(Reporter: jryans, Assigned: u462496)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
Attached patch Patch (obsolete) — Splinter Review
Attachment #8768519 - Flags: review?(dao+bmo)
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)
Should I upload another patch?
Flags: needinfo?(allassopraise)
Yes, we need a patch that can be applied without conflicts.
Attached patch Patch V2 (obsolete) — Splinter Review
Attachment #8768519 - Attachment is obsolete: true
Attachment #8769165 - Flags: review?(dao+bmo)
Attachment #8769165 - Flags: review?(dao+bmo) → review+
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)
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`.
(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.
Attached patch Patch V3Splinter Review
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+
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/57d42a4e2ff1
Update tab's permanentKey in _swapBrowserDocShells. r=dao
https://hg.mozilla.org/mozilla-central/rev/57d42a4e2ff1
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
You need to log in before you can comment on or make changes to this bug.