409679 - [patch] Option-return in location bar doesn't work without scheme://

Status: RESOLVED FIXED

(Camino Graveyard :: Location Bar & Autocomplete, defect)

Description

[Mark Mentovai]

I typed google.com into the location bar and pressed option-return.  I expected to be offered a save dialog allowing me to save google.com to disk (the page, not the index and the trade secrets.)  Instead, nothing happened.  The operation silently fails unless the location bar contains a more well-formed URL, like http://google.com.  This is inconsistent with the location bar's ordinary mode of behavior, where google.com is accepted and results in an attempt at loading http://google.com.

Comment 1

[Chris Lawson (gone)]

I'll take a look at this tomorrow.

Comment 2

[Chris Lawson (gone)]

Would you expect this to work without a TLD? I only ask because that'll make a difference -- we *could* send everything through the fixup service first and then pass it off to the downloader, but it seems like simply looking for 

(if ![string hasPrefix:@"http://"])
  string = [string with "http://";

would be simpler and would address this problem for the vast majority of use-cases. Does anyone really expect typing "google" in the location bar and hitting Option-return to do anything useful? (That doesn't work in the Command-return case, for example, whereas "google.com" does, so I would say we don't have to worry about a full fixup.)

The only problem I potentially foresee with the above is that it doesn't do anything to differentiate between FTP and HTTP(S). However, I think it's enough of an edge case that we probably don't need to worry about it. How many people are going to type in the entire URL of something they want to download via FTP, leave off the ftp:// part, and then hit Option-return to complete the download?

Comment 3

[Chris Lawson (gone)]

Attachment: fix v1.0

Murph, wanna take a look at this? Pinkerton said prepending "http://" was a satisfactory way to solve this. I just hope I didn't outsmart myself here trying to ensure validity.

Comment 4

[Chris Lawson (gone)]

With this patch, I get the following in Console if I hit Option-return after typing "google.com" in the location bar:

28/08/2008 15:01:18 [0x0-0x44a44a].org.mozilla.camino[83817] WARNING: malformed url: no scheme: file /Users/clawson/mozilla/netwerk/base/src/nsStandardURL.cpp, line 704 

but the URL string we're passing into the saveURL:url:suggestedFilename: method is "http://google.com".

Not sure why that's happening, or whether it's a problem we really want to worry about.

Comment 5

[Christopher Henderson]

Comment on attachment 335938 [details] [diff] [review]
fix v1.0

Tried to do some debugging to find the source of that warning, but no luck. Seems like something's happening when Gecko tries to parse the URL. Doesn't affect the behaviour of saving a URL, though.

Comment 6

[Stuart Morgan]

(In reply to comment #4)
> Not sure why that's happening

What is the value of |spec| at that point, and what is the call stack?

Comment 7

[Stuart Morgan]

Comment on attachment 335938 [details] [diff] [review]
fix v1.0

Removing sr request until the warning is figured out.

Comment 8

[Chris Lawson (gone)]

As far as anyone can tell, this warning is coming from something Gecko is doing. Frankly, I don't care to pursue it further, but if someone else does, be my guest.

Comment 9

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

nobody's bugs shouldn't be ASSI.

Comment 10

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Comment on attachment 335938 [details] [diff] [review]
fix v1.0

Nick pointed out that nsStandardURL has NSPR logging; putting this in my queue to remind me to take a look and see if NSPR logging reports the |spec| successfully.

If someone else wants to take a look before I get to it and doesn't know about NSPR logging, ping me.

Comment 11

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: NSPR log of Opt-Return in the location bar with no scheme

Comment 12

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: NSPR log of Opt-Return in the location bar with the loaded page's URL

Comment 13

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: NSPR log of Cmd-S of the loaded page

Comment 14

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: NSPR log of clicking a link that triggers a file download

I ran Chris's patch with nsStandardURL NSPR logging and looked at what happens when we opt-return with no scheme, when we opt-return with a full URL (the current page), when we Cmd-S the current page, and when we download a file.

In the first three logs, you'll notice that right above the point where the Save dialogue shows, we get the malformed URL error, and indeed we have no scheme, just a "filename".  To me this seems to imply that our Save code has a bug in it (assuming the Save code is called for both Cmd-S and Opt-Return)--and I noted on irc the other night that I've often seen this error when testing other patches (bug 154580 comment 53, and possibly bug 159336, too).

Comment 15

[Chris Lawson (gone)]

Since this is a problem with our standard Save, do we really care about introducing another call to that code? My vote would be that we just go ahead and fix this bug, since Save obviously works fine, and file a new bug to figure out what's causing the warning in our Save code.

Comment 16

[Stuart Morgan]

Comment on attachment 335938 [details] [diff] [review]
fix v1.0

sr=smorgan

Comment 17

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Landed on cvs trunk, and filed bug 462181 on the warning/error in the Save code.