562649 - set and correctly handle userTypedValue when loading external URIs

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Dão Gottwald [:dao]]

Attachment: wip patch

From bug 556957 comment 34:

> > Created an attachment (id=441823) [details] [details]
> > patch?
> > 
> > This seems to work,
> 
> and it should fix this bug:
> 
> 1. `firefox http://gavinsharp.com/os/slow.php?time=50`
> 2. switch to a different tab
> 3. switch back to the previous tab
> 
> Expected result: The location bar displays
> http://gavinsharp.com/os/slow.php?time=50
> 
> Actual result: It doesn't.

From bug 556957 comment 37:

> The only use of userTypedClear I can see is
> in mTabProgressListener's onLocationChange, where a non-zero value allows the
> userTypedValue to be cleared, but I don't see how that could be a problem.
> Presumably if we've received an onLocationChange for the tab, clearing the
> userTypedValue shouldn't be a problem, since we'd fall back to the currentURI
> which should be correct. On the other hand, I don't really understand why we
> bother incrementing userTypedClear around calls to loadURI(), since I don't
> think it can have any relevant synchronous side effects.

Comment 1

[Dão Gottwald [:dao]]

We're apparently getting a bogus onLocationChange caused by LOAD_FLAGS_FROM_EXTERNAL...

Comment 2

[Dão Gottwald [:dao]]

(In reply to comment #1)
> We're apparently getting a bogus onLocationChange caused by
> LOAD_FLAGS_FROM_EXTERNAL...

from bug 298255

Comment 3

[Dão Gottwald [:dao]]

Attachment: patch

Comment 4

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

(In reply to comment #2)
> (In reply to comment #1)
> > We're apparently getting a bogus onLocationChange caused by
> > LOAD_FLAGS_FROM_EXTERNAL...
> 
> from bug 298255

Ah - from http://hg.mozilla.org/mozilla-central/annotate/1acce93e0198/docshell/base/nsDocShell.cpp#l7860 specifically, right? The about:blank load clobbers the user typed value? So the solution to that is to avoid setting the value until that load has already been completed (synchronously from under loadURIWithFlags()->loadURI()).

With that patch, what ensures that userTypedValue gets displayed before we get the onLocationChange for the relevant load (since it isn't startDocumentLoad anymore)? I.e., what replaces the call to URLBarSetURI() that you're removing?

Comment 5

[Dão Gottwald [:dao]]

(In reply to comment #4)
> With that patch, what ensures that userTypedValue gets displayed before we get
> the onLocationChange for the relevant load (since it isn't startDocumentLoad
> anymore)? I.e., what replaces the call to URLBarSetURI() that you're removing?

updateCurrentBrowser calling XULBrowserWindow.onLocationChange

Comment 6

[Dão Gottwald [:dao]]

And yes, the analysis in the first half of comment 4 is correct.

Comment 7

[Dão Gottwald [:dao]]

Attachment: patch

test added

Comment 8

[Dão Gottwald [:dao]]

Attachment: patch

updated to tip

Comment 9

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/6a71deda9822

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

This turned the tree orange, and the tree is ready to reopen except for that orange.

You starred tinderbox with:

[dao%mozilla.com - 2010/05/14 06:41:45]
tests are conflicting. will fix once the tree is open.

but this will probably get backed out since you're not on IRC and the fix isn't posted here.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Backed out: http://hg.mozilla.org/mozilla-central/rev/c861185afd4a

Sorry, but I did spend about 20 minutes looking for you first, and the test fix wasn't obvious to me.  :(

Comment 12

[Dão Gottwald [:dao]]

Attachment: patch v2

Turns out the browser_522545.js failure was real, as setting userTypedValue resets userTypedClear, so doing this after calling loadURIWithFlags is a problem. The solution is to let browser.xml set userTyedValue. The other option would be to let tabbrowser.xml fiddle further with userTypedClear or _userTypedValue (note the underscore), which would be fragile.

Comment 13

[Dão Gottwald [:dao]]

Comment on attachment 445595 [details] [diff] [review]
patch v2

>--- a/toolkit/content/widgets/browser.xml
>+++ b/toolkit/content/widgets/browser.xml

>+            if ((!this.currentURI || this.currentURI.spec == "about:blank") &&
>+                !this.canGoBack && !this.canGoForward) {
>+              // pretend the user typed this so it'll be available till
>+              // the document successfully loads
>+              var setUserTypedValue = true;
>+              this.userTypedValue = aURI;

Small flaw: tabbrowser.xml is still responsible for clearing userTypedValue (in onLocationChange). We could move this to browser.xml if we added another progress listener there.

Comment 14

[Robert Kaiser]

(In reply to comment #13)
> Small flaw: tabbrowser.xml is still responsible for clearing userTypedValue (in
> onLocationChange). We could move this to browser.xml if we added another
> progress listener there.

As there are consumers of browser.xml that are not consumers of the Firefox tabbrowser.xml (SeaMonkey has its own tabbrowser.xml, Thunderbird uses <browser> somewhat differently, add-ons or external XULRunner apps might do entirely different stuff), it probably makes most sense to keep things in a consistent place, and all in browser.xml might make sense.

Note that I don't know the actual code details, but just want to make sure other consumers of browser.xml can deal well with the changes.

Comment 15

[Dão Gottwald [:dao]]

This doesn't effectively affect SeaMonkey or other tabbrowser.xml forks, as they clear userTypedValue in onLocationChange just like Firefox.

Comment 16

[Dão Gottwald [:dao]]

Attachment: patch v3

Hybrid approach... let's tabbrowser.xml set userTypedValue and browser.xml maintain it.

Comment 17

[Dão Gottwald [:dao]]

Attachment: interdiff reviewed patch -> v3

Comment 18

[[e-mail]]

(In reply to comment #4)
> Ah - from
> http://hg.mozilla.org/mozilla-central/annotate/1acce93e0198/docshell/base/nsDocShell.cpp#l7860
> specifically, right? The about:blank load clobbers the user typed value? So the
> solution to that is to avoid setting the value until that load has already been
> completed (synchronously from under loadURIWithFlags()->loadURI()).
Unfortunately about:blank isn't the only possible synchronous load. For example, load the URL above, then change the line number in the fragment reference.

Comment 19

[Dão Gottwald [:dao]]

Attachment: patch v4

This should fix the issue from comment 18.
Neil, would you mind reviewing the browser.xml change?

Comment 20

[[e-mail]]

Comment on attachment 449811 [details] [diff] [review]
patch v4

>             try {
>               this.userTypedClear++;
Sorry if I haven't understood the bug correctly, but would it help to condition the increment on aFlags & this.webNavigation.LOAD_FLAGS_FROM_EXTERNAL instead?

Comment 21

[Dão Gottwald [:dao]]

Attachment: patch v5

Yeah, this works too.

Comment 22

[[e-mail]]

Comment on attachment 449845 [details] [diff] [review]
patch v5

r=me on browser.xml

Comment 23

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/f2070673fdbc