Closed
Bug 941540
Opened 11 years ago
Closed 10 years ago
Make docShell swapping easier to handle by letting browsers define a field that represents their key/identity
Categories
(Firefox :: Tabbed Browser, defect)
Firefox
Tabbed Browser
Tracking
()
RESOLVED
FIXED
Firefox 29
People
(Reporter: ttaubert, Assigned: ttaubert)
Details
Attachments
(2 files)
2.23 KB,
patch
|
Felipe
:
review+
|
Details | Diff | Splinter Review |
13.79 KB,
patch
|
Yoric
:
review+
|
Details | Diff | Splinter Review |
In bug 930967 Bill suggested the following: > Every time we create a <browser> > element, we could do something like |browser.permanentKey = {}| (where > permanentKey is an XBL-defined field). swapDocShells would be responsible > for swapping the permanentKey objects. Then anyone wanting to use weakmaps > of <browser> elements could just use browser.permanentKey for lookups. This would be a great and easy way to handle docShell swaps without having to do anything for most of the code affected.
Assignee | ||
Comment 1•10 years ago
|
||
This patch implements browser.permanentKey as proposed by Bill.
Assignee | ||
Comment 2•10 years ago
|
||
This patch uses browser.permanentKey to avoid listening for SwapDocShells. It's based off of bug 960903 because I didn't want to bitrot myself. I updated _browserEpochs as well to have consistent code although we really don't support swapping docShells of pending tabs.
Attachment #8362646 -
Flags: review?(smacleod)
Comment 3•10 years ago
|
||
Comment on attachment 8362641 [details] [diff] [review] 0004-Bug-941540-Add-browser.permanentKey.patch Review of attachment 8362641 [details] [diff] [review]: ----------------------------------------------------------------- Nice! I seem to recall that there's some other code around using WeakMaps with <browsers> (maybe even written by me?).. When we change them to use these keys that means they'll become swapDocShells aware.
Attachment #8362641 -
Flags: review?(felipc) → review+
Assignee | ||
Comment 4•10 years ago
|
||
(In reply to :Felipe Gomes from comment #3) > Nice! I seem to recall that there's some other code around using WeakMaps > with <browsers> (maybe even written by me?).. When we change them to use > these keys that means they'll become swapDocShells aware. At least for SessionStore there don't seem to be any more WeakMaps. Now for the rest of the browser there probably is quite some code left to update...
Assignee | ||
Comment 5•10 years ago
|
||
Comment on attachment 8362646 [details] [diff] [review] 0005-Bug-941540-Use-browser.permanentKey-to-handle-docShe.patch Just found out that Steven is on PTO :)
Attachment #8362646 -
Flags: review?(dteller)
Comment 6•10 years ago
|
||
(In reply to Tim Taubert [:ttaubert] from comment #4) > (In reply to :Felipe Gomes from comment #3) > > Nice! I seem to recall that there's some other code around using WeakMaps > > with <browsers> (maybe even written by me?).. When we change them to use > > these keys that means they'll become swapDocShells aware. > > At least for SessionStore there don't seem to be any more WeakMaps. Now for > the rest of the browser there probably is quite some code left to update... Are you sure? What about TabStateCache?
Assignee | ||
Comment 7•10 years ago
|
||
(In reply to David Rajchenbach Teller [:Yoric] (please use "needinfo?") from comment #6) > > At least for SessionStore there don't seem to be any more WeakMaps. Now for > > the rest of the browser there probably is quite some code left to update... > > Are you sure? What about TabStateCache? TabStateCache is addressed in the patch. Please see comment #2: > This patch uses browser.permanentKey to avoid listening for SwapDocShells. > It's based off of bug 960903 because I didn't want to bitrot myself.
Comment 8•10 years ago
|
||
Comment on attachment 8362646 [details] [diff] [review] 0005-Bug-941540-Use-browser.permanentKey-to-handle-docShe.patch Review of attachment 8362646 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. I wonder, would there by any interest in saving/restoring the permanent key? ::: browser/components/sessionstore/src/SessionStore.jsm @@ -713,5 @@ > - case "SwapDocShells": > - browser = aEvent.currentTarget; > - let otherBrowser = aEvent.detail; > - TabState.onBrowserContentsSwapped(browser, otherBrowser); > - TabStateCache.onBrowserContentsSwapped(browser, otherBrowser); By the way, how comes we don't save when we swap docShells?
Attachment #8362646 -
Flags: review?(dteller) → review+
Assignee | ||
Comment 9•10 years ago
|
||
(In reply to David Rajchenbach Teller [:Yoric] (please use "needinfo?") from comment #8) > I wonder, would there by any interest in saving/restoring the permanent key? Saving and restoring how? It's only an empty object that we use as an identity. It's only per-session permanent :) > ::: browser/components/sessionstore/src/SessionStore.jsm > @@ -713,5 @@ > > - case "SwapDocShells": > > - browser = aEvent.currentTarget; > > - let otherBrowser = aEvent.detail; > > - TabState.onBrowserContentsSwapped(browser, otherBrowser); > > - TabStateCache.onBrowserContentsSwapped(browser, otherBrowser); > > By the way, how comes we don't save when we swap docShells? Good question! We should totally do that.
Assignee | ||
Comment 10•10 years ago
|
||
(In reply to Tim Taubert [:ttaubert] from comment #9) > > > - case "SwapDocShells": > > > - browser = aEvent.currentTarget; > > > - let otherBrowser = aEvent.detail; > > > - TabState.onBrowserContentsSwapped(browser, otherBrowser); > > > - TabStateCache.onBrowserContentsSwapped(browser, otherBrowser); > > > > By the way, how comes we don't save when we swap docShells? > > Good question! We should totally do that. Filed bug 963042.
Assignee | ||
Updated•10 years ago
|
Attachment #8362646 -
Flags: review?(smacleod)
Assignee | ||
Comment 11•10 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/dcee48651880 https://hg.mozilla.org/integration/fx-team/rev/0bc8711e7dd2
Whiteboard: [fixed-in-fx-team]
Comment 12•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/dcee48651880 https://hg.mozilla.org/mozilla-central/rev/0bc8711e7dd2
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 29
You need to log in
before you can comment on or make changes to this bug.
Description
•