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)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 29

People

(Reporter: ttaubert, Assigned: ttaubert)

Details

Attachments

(2 files)

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.
This patch implements browser.permanentKey as proposed by Bill.
Assignee: nobody → ttaubert
Status: NEW → ASSIGNED
Attachment #8362641 - Flags: review?(felipc)
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 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+
(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...
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)
(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?
(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 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+
(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.
(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.
Attachment #8362646 - Flags: review?(smacleod)
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.

Attachment

General

Created:
Updated:
Size: