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

RESOLVED FIXED

Status

RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: mark, Assigned: bugzilla-graveyard)

Tracking

Details

Attachments

(5 attachments)

(Reporter)

Description

11 years ago
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.
(Assignee)

Comment 1

11 years ago
I'll take a look at this tomorrow.
Assignee: nobody → cl-bugs-new
(Assignee)

Updated

11 years ago
Version: 1.8 Branch → unspecified
(Assignee)

Comment 2

11 years ago
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?
(Assignee)

Comment 3

10 years ago
Created attachment 335938 [details] [diff] [review]
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.
Attachment #335938 - Flags: review?(murph)
(Assignee)

Comment 4

10 years ago
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.
(Assignee)

Updated

10 years ago
Status: NEW → ASSIGNED
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.
Attachment #335938 - Flags: review+
(Assignee)

Updated

10 years ago
Attachment #335938 - Flags: superreview?(stuart.morgan+bugzilla)

Comment 6

10 years ago
(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

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

Removing sr request until the warning is figured out.
Attachment #335938 - Flags: superreview?(stuart.morgan+bugzilla)
(Assignee)

Comment 8

10 years ago
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.
Assignee: cl-bugs-new → nobody
nobody's bugs shouldn't be ASSI.
Status: ASSIGNED → NEW
Summary: Option-return in location bar doesn't work without scheme:// → [patch] Option-return in location bar doesn't work without scheme://
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.
Attachment #335938 - Flags: review?(alqahira)
Created attachment 343649 [details]
NSPR log of Opt-Return in the location bar with no scheme
Created attachment 343650 [details]
NSPR log of Opt-Return in the location bar with the loaded page's URL
Created attachment 343655 [details]
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).
(Assignee)

Comment 15

10 years ago
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

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

sr=smorgan
Attachment #335938 - Flags: review?(murph) → superreview+
Landed on cvs trunk, and filed bug 462181 on the warning/error in the Save code.
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.