If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED in mozilla16

Status

()

Toolkit
XUL Widgets
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: ttaubert, Assigned: ttaubert)

Tracking

Trunk
mozilla16
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

5 years ago
Created attachment 626794 [details] [diff] [review]
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.
Attachment #626794 - Flags: review?(gavin.sharp)
(Assignee)

Comment 1

5 years ago
Created attachment 626943 [details] [diff] [review]
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.
Attachment #626794 - Attachment is obsolete: true
Attachment #626794 - Flags: review?(gavin.sharp)
Attachment #626943 - Flags: review?(gavin.sharp)
(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?
(Assignee)

Comment 3

5 years ago
(In reply to :Gavin Sharp (use gavin@gavinsharp.com 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?
Attachment #626943 - Flags: feedback?(neil)

Comment 4

5 years ago
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.

Updated

5 years ago
Attachment #626943 - Flags: feedback?(neil) → feedback-
(Assignee)

Comment 5

5 years ago
(In reply to neil@parkwaycc.co.uk from comment #4)
> (I don't know why the page hide event was chosen.)

Good question. This unfortunately exists since the old CVS days...
(Assignee)

Comment 6

5 years ago
Created attachment 639300 [details] [diff] [review]
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.
Attachment #626943 - Attachment is obsolete: true
Attachment #626943 - Flags: review?(gavin.sharp)
Attachment #639300 - Flags: review?(neil)

Comment 7

5 years ago
Comment on attachment 639300 [details] [diff] [review]
patch v3

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

Comment 8

5 years ago
(In reply to neil@parkwaycc.co.uk 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
(Assignee)

Comment 9

5 years ago
(In reply to Tim Taubert [:ttaubert] from comment #8)
> (In reply to neil@parkwaycc.co.uk 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
(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?
(Assignee)

Comment 11

5 years ago
Created attachment 639638 [details] [diff] [review]
patch v4

(In reply to neil@parkwaycc.co.uk 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.
Attachment #639300 - Attachment is obsolete: true
Attachment #639300 - Flags: review?(neil)
Attachment #639638 - Flags: review?(neil)

Updated

5 years ago
Attachment #639638 - Flags: review?(neil) → review+
(Assignee)

Comment 12

5 years ago
https://hg.mozilla.org/integration/fx-team/rev/d103f74ecd5c
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla16
(Assignee)

Comment 13

5 years ago
https://hg.mozilla.org/mozilla-central/rev/d103f74ecd5c
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
You need to log in before you can comment on or make changes to this bug.