Closed Bug 562649 Opened 11 years ago Closed 11 years ago

set and correctly handle userTypedValue when loading external URIs

Categories

(Firefox :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 3.7a5

People

(Reporter: dao, Assigned: dao)

References

Details

Attachments

(1 file, 8 obsolete files)

Attached patch wip patch (obsolete) — Splinter Review
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.
Summary: set and correctl handle userTypedValue when loading external URIs → set and correctly handle userTypedValue when loading external URIs
We're apparently getting a bogus onLocationChange caused by LOAD_FLAGS_FROM_EXTERNAL...
(In reply to comment #1)
> We're apparently getting a bogus onLocationChange caused by
> LOAD_FLAGS_FROM_EXTERNAL...

from bug 298255
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → dao
Attachment #442384 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #442980 - Flags: review?(gavin.sharp)
(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?
(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
And yes, the analysis in the first half of comment 4 is correct.
Attached patch patch (obsolete) — Splinter Review
test added
Attachment #442980 - Attachment is obsolete: true
Attachment #443319 - Flags: review?(gavin.sharp)
Attachment #442980 - Flags: review?(gavin.sharp)
Attachment #443319 - Flags: review?(gavin.sharp) → review+
Attached patch patch (obsolete) — Splinter Review
updated to tip
Attachment #443319 - Attachment is obsolete: true
http://hg.mozilla.org/mozilla-central/rev/6a71deda9822
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.7a5
Blocks: FF2SM
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.
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.  :(
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch patch v2 (obsolete) — Splinter Review
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.
Attachment #445305 - Attachment is obsolete: true
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.
(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.
This doesn't effectively affect SeaMonkey or other tabbrowser.xml forks, as they clear userTypedValue in onLocationChange just like Firefox.
Attached patch patch v3 (obsolete) — Splinter Review
Hybrid approach... let's tabbrowser.xml set userTypedValue and browser.xml maintain it.
Attachment #445595 - Attachment is obsolete: true
Attachment #445614 - Flags: review?(gavin.sharp)
Attached patch interdiff reviewed patch -> v3 (obsolete) — Splinter Review
Attachment #449603 - Flags: review?(gavin.sharp)
Attachment #445614 - Flags: review?(gavin.sharp)
(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.
Attached patch patch v4 (obsolete) — Splinter Review
This should fix the issue from comment 18.
Neil, would you mind reviewing the browser.xml change?
Attachment #445614 - Attachment is obsolete: true
Attachment #449603 - Attachment is obsolete: true
Attachment #449811 - Flags: review?(neil)
Attachment #449603 - Flags: review?(gavin.sharp)
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?
Attached patch patch v5Splinter Review
Yeah, this works too.
Attachment #449811 - Attachment is obsolete: true
Attachment #449845 - Flags: review?(neil)
Attachment #449811 - Flags: review?(neil)
Comment on attachment 449845 [details] [diff] [review]
patch v5

r=me on browser.xml
Attachment #449845 - Flags: review?(neil) → review+
http://hg.mozilla.org/mozilla-central/rev/f2070673fdbc
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
Blocks: SMtabAPI
Blocks: 570981
Blocks: 586340
No longer blocks: SMtabAPI
Duplicate of this bug: 610357
No longer blocks: FF2SM
You need to log in before you can comment on or make changes to this bug.