758212 - Allow swapping of docShells from standalone and tabbrowser <browser> elements

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, defect)

Description

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v1

We need to detach the docShells from the nsIFormFillController before swapping the frame loaders. onPageShow() calls browser.attachFormFill() - if the new parent browser has the 'autocompletepopup' attribute then the docShell will be re-attached but with the right popup element. This works only if we already swapped the _docShell property.

By swapping all properties before the swapFrameLoaders() call we also don't need to take care of the _fastFind property. browser.onPageHide() will call this.fastFind.setDocShell() if needed to not have the old docShell attached to the fastFind instance.

I'm not sure if swapping properties after swapFrameLoaders() was done intentionally and if there are any side-effects I'm missing here. I've done some quick tests with the patch from bug 753448 and with moving around some tabs between windows and everything seems to work fine.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v2

With patch v1 browser/base/content/test/browser_bug477014.js was failing permanently. This new patch includes a fix for the test. The erroneous part was:

>    if (!tabToDetach ||
>        tabToDetach.linkedBrowser.contentDocument != event.target)

After the gBrowser.swapBrowsersAndCloseOther() was called the linkedBrowser should not point to the same contentDocument anymore. That's a timing issue and we can fix this by simply saving the contentDocument to a variable when the tab has loaded and use this for comparison instead.

Comment 2

[:Gavin Sharp [email: [e-mail]]]

(In reply to Tim Taubert [:ttaubert] from comment #0)
> By swapping all properties before the swapFrameLoaders() call we also don't
> need to take care of the _fastFind property. browser.onPageHide() will call
> this.fastFind.setDocShell() if needed to not have the old docShell attached
> to the fastFind instance.

This seems to only be true if the browser is currently selected. I don't really like relying on this, it seems simpler and more reliable to just swap _fastFind if there's no tabbrowser parent. Is this the only reason you switched to swapping first?

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #2)
> (In reply to Tim Taubert [:ttaubert] from comment #0)
> > By swapping all properties before the swapFrameLoaders() call we also don't
> > need to take care of the _fastFind property. browser.onPageHide() will call
> > this.fastFind.setDocShell() if needed to not have the old docShell attached
> > to the fastFind instance.
> 
> This seems to only be true if the browser is currently selected.

If the <browser> has a tabbrowser parent then the fastFind docShell is set only if it's currently selected because we have a fastFind instance for the whole <tabbrowser>. If it's a standalone browser then it's always set because the fastFind instance is a custom one for the <browser>.

> Is this the only reason you
> switched to swapping first?

Both the nsIFormFillController and nsITypeAheadFind require a valid "this.docShell" property so this is for both of them to work correctly.

> I don't
> really like relying on this, it seems simpler and more reliable to just swap
> _fastFind if there's no tabbrowser parent.

Is swapping the _fastFind properties the right thing to do here? We're swapping the docShells of the browsers but the fastFind instance is custom to the browser itself, no?

So when swapping two tabbrowser browsers they will both have a custom fastFind instance which is specific to their tabbrowser parent. Shouldn't we rather make sure that .setDocShell() is called on those instances?

Comment 4

[[e-mail]]

If both browsers are child elements of tabbrowser then neither of them manage their own fast find, instead each tabbrowser's fast find is used by its current browser.

On the other hand if neither browsers are child elements of tabbrowser then each of them manages their own fast find. The fast find instances reference the docShell so currently they are swapped after the frame loaders have been swapped. Unfortunately we can't swap the fast find instances in advance as the page hide event will reinit the fast find with the wrong docShell.

The other alternative is to arrange somehow to reinit the fast find after the frame loaders have been swapped, perhaps by using the page show event. (I don't know why the page hide event was chosen.) This would the also fix the problem of switching a mixture of types of browser, as you would then continue to associate the fast find with the (tab)browser rather than the docShell.

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

(In reply to [e-mail] from comment #4)
> (I don't know why the page hide event was chosen.)

Good question. This unfortunately exists since the old CVS days...

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v3

Ok, this patch should handle the form fill controllers and fast find instances correctly after swapping.

* We can just reset browser._fastFind to null as that's a lazy getter and a new nsITypeAheadFind instance will be created as needed.

* The docShells may need to be detached and attached again to the nsIFormFillController service if browser.mFormFillAttached.

Comment 7

[[e-mail]]

Comment on attachment 639300 [details] [diff] [review]
patch v3

Why not unconditionally detach the form fill before swapping the frame loaders?

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

(In reply to [e-mail] from comment #7)
> Why not unconditionally detach the form fill before swapping the frame
> loaders?

Because they're re-attached on PageShow :(

http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/browser.xml#524

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Tim Taubert [:ttaubert] from comment #8)
> (In reply to [e-mail] from comment #7)
> > Why not unconditionally detach the form fill before swapping the frame
> > loaders?
> 
> Because they're re-attached on PageShow :(

... which is fired before the frameLoaders get swapped.

http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsFrameLoader.cpp#1126

Comment 10

[[e-mail]]

(In reply to Tim Taubert from comment #8)
> (In reply to from comment #7)
> > Why not unconditionally detach the form fill before swapping the frame
> > loaders?
> 
> Because they're re-attached on PageShow :(

In that case,  would it work to a) swap the frame loaders b) detach form fill c) swap the properties d) reattach form fill?

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v4

(In reply to [e-mail] from comment #10)
> In that case,  would it work to a) swap the frame loaders b) detach form
> fill c) swap the properties d) reattach form fill?

Good idea, that works. Reverted the patch to keep the old behavior of swapping the properties after the frame loaders.

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/d103f74ecd5c

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/d103f74ecd5c