stephen wedgwood Reporter
	Description • 19 years ago

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8.0.1) Gecko/20060117 Camino/1.0b2+
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8.0.1) Gecko/20060117 Camino/1.0b2+

"The URL is not valid and cannot be loaded." 
But this is before any URL has been chosen. And it doesn't interfere with opening any URL.
It's just a nuisance.

Reproducible: Always

Steps to Reproduce:
1.Open a "new window" in Camino
2.
3.

Actual Results:  
A pop-up window appears with the message: The URL is not valid and cannot be loaded.

Expected Results:  
A new browser window.

The pop-up has to be closed before browsing can continue. The message doesn't refer to any URL. The window itself is still empty. It's just a nuisance.

	Chris Lawson (gone) Assignee
	Comment 1 • 19 years ago

I can cause this if I set the homepage to a broken link (I used ttP://blaklsdlnf.awewen.3n2p in my test).

Make sure your homepage link isn't broken ;)

cl

	Chris Lawson (gone) Assignee
	Comment 2 • 19 years ago

We could do a better job of validating input here, though. Checking for a valid protocol on the URL would be a good idea. (The other potential source of this problem is related to proxies and wouldn't be something we could check for.)

I think validating the URL itself is probably overkill, though.

cl

	stephen wedgwood Reporter
	Comment 3 • 19 years ago

Okay, it wasn't a "bug" but a corruption in my Preferences page. I fixed it by entering a good address for my homepage. Thanks for the clue. It was "bug"ging me.

	Chris Lawson (gone) Assignee
	Comment 4 • 19 years ago

As discussed on IRC last night, this is now a catch-all for input validation issues in the prefs.

Homepage needs to have its protocol validated. Right now, that's the only thing I see looking through our pref panes, but feel free to add anything else you happen to find.

Reopening and taking.

cl

	Chris Lawson (gone) Assignee
	Comment 5 • 19 years ago

We'll need to get a strings change for this too. Smokey and I are working on the verbiage at the moment. I'll post here when we've agreed on something.

cl

	Mark Mentovai
	Comment 6 • 19 years ago

Comment on attachment 214970 [details] [diff] [review]
validates protocol via necko, throws an alert sheet on failure

>Index: mozilla/camino/PreferencePanes/Navigation/Navigation.mm
>+- (BOOL)shouldUnselect

The type of shouldUnselect is not BOOL, it's NSPreferencePaneUnselectReply.  Values of that type are listed at http://developer.apple.com/documentation/UserExperience/Reference/PreferencePanes/ObjC_classic/Classes/NSPreferencePane.html#//apple_ref/doc/uid/20001823-CH3-18526 .

>+{
>+  NSString* homepageURL = [textFieldHomePage stringValue];
>+  
>+  NSRange delimiter = [homepageURL rangeOfString:@":"];
>+  if (delimiter.location == NSNotFound)
>+    return YES; // punt

First of all, you want NSUnselectNow, not YES.

Second of all, this isn't really a punt like the punting you've got in GeckoUtils.  It's permissible to let a delimiterless value pass because a schemeless URL is implicitly file://, and file:// is OK.  Your comment should say so.

>+  NSString* schemeStr = [homepageURL substringToIndex:delimiter.location];
>+  const char* protocol = [schemeStr UTF8String];

Recommend consistency: if schemeStr and protocol both refer to the same string but are different types, then the should have the same base name, for example, scheme and schemeAsCString.  That's a little wordy, but you see what I'm saying.

>+  if (!GeckoUtils::isProtocolValid(protocol)) {
>+    NSBeginInformationalAlertSheet(NSLocalizedString(@"UnregisteredProtocolTitle", @"Unregistered Protocol"),

The second argument to NSLocalizedString is just a comment, you can leave it at |@""|.

>+                                   NSLocalizedString(@"OKButtonText", @"OK"), nil, nil, [textFieldHomePage window], self, nil, nil, nil,

Whoa, this is starting to get pretty wide!  You shouldn't need to bust 80 chars for this.  At the very least, I recommend breaking to a new line after each NSLocalizedString().

Actually, this call takes a lot of arguments that are difficult to keep straight, so it's a good idea to add comments where it's not clear what the arguments are doing, especially where they're null.

Also, you aren't passing in any selectors to be called, so there's no reason to pass a delegate.  You can make that nil too.

>+                                   NSLocalizedString(@"UnregisteredProtocolWarning", @"UnregisteredProtocolWarning"), protocol);

You want your UnregisteredProtocolWarning to be a format string using the scheme name as the string to replace the token with, right?  You need to use [NSString stringWithFormat:NSLocalizedString(@"key",@""), schemeStr], where schemeStr is your NSString* (not char*), and the localized string for "key" contains "%@".  You don't get format-string behavior unless you ask for it explicitly, and what's written above actually sends protocol (the char*) as an additional argument to NSBeginInformationalAlertSheet.

>+    return NSUnselectCancel;
>+  } 
>+  return YES;

NSUnselectNow.

>+}

And now, welcome to the wonderful world of xpcom.

>Index: mozilla/camino/src/extensions/GeckoUtils.h
>+  static bool isProtocolValid(const char* aProtocol)

This is a GeckoUtil, so let's Geckoize it.  The return type should be PRBool, and it should return PR_TRUE or PR_FALSE.  (Your extHandler == nsnull return is compatible with the PRBool return type.)

>+  {
>+    nsCOMPtr<nsIIOService> ioService = do_GetService("@mozilla.org/network/io-service;1");
>+    if (!ioService)
>+      return true; // punt
>+    nsresult rv;
>+    nsCOMPtr<nsIProtocolHandler> handler;
>+    rv = ioService->GetProtocolHandler(aProtocol, getter_AddRefs(handler));

You can wait to declare rv until you use it:

    nsresult rv = ioService->GetProtocolHandler(aProtocol, getter_AddRefs(handler));

>+    if (NS_FAILED(rv))
>+      return true; // punt
>+    nsCOMPtr<nsIExternalProtocolHandler> extHandler = do_QueryInterface(handler);
>+    // Protocol is valid if its handler is not the external one
>+    return (extHandler == nsnull);
>+  }

	Simon Fraser [no longer active]
	Comment 8 • 19 years ago

Comment on attachment 215093 [details] [diff] [review]
fixed comments

>Index: mozilla/camino/PreferencePanes/Navigation/Navigation.mm
>===================================================================

>-- (void) didUnselect
>+- (NSPreferencePaneUnselectReply)shouldUnselect
>+{
>+  NSString* homepageURL = [textFieldHomePage stringValue];
>+  
>+  NSRange delimiter = [homepageURL rangeOfString:@":"];
>+  if (delimiter.location == NSNotFound)
>+    return NSUnselectNow; // implicitly file:// if no delimiter is found, so this is OK
>+  NSString* schemeStr = [homepageURL substringToIndex:delimiter.location];
>+  const char* schemeAsCString = [schemeStr UTF8String];
>+  if (!GeckoUtils::isProtocolValid(schemeAsCString)) {

I think this should just use NSURL to try to make a URL from the string, as we do for drag and drop (via -containsURLData).

For example, it should be OK to enter "www.apple.com" as my home page, just as it is in the url bar.

Also, maybe giving the user a button to choose "Current page", "blank page" etc would help in 90% of the cases. We really should not force them to have to type "http://".

In addition, showing error sheets seems rather rude. It would be much nicer to show a little status indication next to the field, with gray status text underneath it.

	Chris Lawson (gone) Assignee
	Comment 9 • 19 years ago

(In reply to comment #8)
> (From update of attachment 215093 [details] [diff] [review] [edit])
> >Index: mozilla/camino/PreferencePanes/Navigation/Navigation.mm
> >===================================================================
> 
> >+  NSRange delimiter = [homepageURL rangeOfString:@":"];
> >+  if (delimiter.location == NSNotFound)
> >+    return NSUnselectNow; // implicitly file:// if no delimiter is found, so this is OK
> >+  NSString* schemeStr = [homepageURL substringToIndex:delimiter.location];
> >+  const char* schemeAsCString = [schemeStr UTF8String];
> >+  if (!GeckoUtils::isProtocolValid(schemeAsCString)) {
> 
> I think this should just use NSURL to try to make a URL from the string, as we
> do for drag and drop (via -containsURLData).
> 
> For example, it should be OK to enter "www.apple.com" as my home page, just as
> it is in the url bar.

This works fine, actually. Maybe my comment in the code should've been a little clearer, but we bail if we don't find a : in the URL string anywhere. The browser handles the rest. I've thoroughly tested this with a fresh profile and can't find any problems with it in this regard.

> Also, maybe giving the user a button to choose "Current page", "blank page" etc
> would help in 90% of the cases.

I believe that's another (filed) bug. :)

> In addition, showing error sheets seems rather rude. It would be much nicer to
> show a little status indication next to the field, with gray status text
> underneath it.

The main reason I did it the way I did was so it would be obvious why the pref pane couldn't be closed. I suppose this could work too, but it's gonna take some work -- suggestions for how to implement it would be good. :)

cl

	Samuel Sidler (old account; do not CC)
	Comment 10 • 19 years ago

(In reply to comment #9)
> I believe that's another (filed) bug. :)

bug 287790

	Chris Lawson (gone) Assignee
	Comment 11 • 19 years ago

Also, the other reason I chose to throw the error sheet was to match our behaviour if you type a similar URL into the Location bar and press return. Consistency is important, you know ;)

cl

	Chris Lawson (gone) Assignee
	Comment 12 • 19 years ago

Stuart suggested on IRC this evening that the proper way to fix this would be for Core to throw pretty error pages instead of a big rude sheet when encountering an unregistered protocol, to better match the behaviour of invalid domain names. I'm duping this to bug 312680 per that conversation, since we won't need to do anything Camino-specific for it once that lands in Core.

cl

*** This bug has been marked as a duplicate of 312680 ***