718931 - open location's direct-parameter should not be optional

Status: RESOLVED FIXED

(Camino Graveyard :: OS Integration, defect)

Description

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

tell app "Camino"
  open location
end tell
--> Camino got an error: AppleEvent handler failed.

Why in the world is this marked optional in the .sdef?  (DeprecatedOpenURL has the same problem, since it's a copy of open location.)

Comment 1

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

Worse, this causes

An exception was thrown during execution of an NSScriptCommand...
*** -[NSURL initWithString:relativeToURL:]: nil string parameter

Last night by removing 'optional="yes"' I was able to get Script Editor to give a more sane error message (something about a parameter being missing), but that's not happening now.

At the very least, we should also nil-check right after |NSString* urlString = [self directParameter];|.  We might want to try and throw an NSScriptCommand error, too, in that case.

Comment 2

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

Attachment: Fix, v1

This "fixes" the problem and prevents the exception.

When calling "open location" with no URL, AppleScript coerces a parameter of "current application" :P  So Script Editor won't error about a missing param on its own (no idea why it did last night, unless I accidentally removed the 'optional="yes"' from "using referrer").

However, we now catch a nil urlString before passing it to NSURL, throw the appropriate? NSScriptCommand error with an error message, and then return nil, which prevents the exception and tells the user helpfully what they're doing wrong. :P

Comment 3

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

Attachment: Fix, v1.1

Chris thought we ought to do something about a blank location, too, so this version also catches 'open location ""' and amends the error string to "missing or empty".

There's probably some argument for additionally catching strings that are entirely whitespace/Unicode space characters, but given our history of inadvertent breakages of edge-case things people use, I'd rather just catch missing and empty locations right now.

Chris also thought if the error message was being displayed anywhere reasonably obvious, we should use a "nicer" string, but he didn't offer any suggestions ;-)  http://mxr.mozilla.org/camino/search?string=setScriptErrorString&tree=camino are our current script error strings; this seems on par with them (but I am open to any proffered nicer alternatives).

Comment 4

[Stuart Morgan]

Comment on attachment 589656 [details] [diff] [review]
Fix, v1.1

Review of attachment 589656 [details] [diff] [review]:
-----------------------------------------------------------------

sr=smorgan

::: src/appleevents/GetURLCommand.mm
@@ +56,5 @@
>    // We can get here before the application is fully initialized and the previous
>    // session is restored.  We want to avoid opening URLs before that happens.
>    if ([mainController isInitialized]) {
>      NSString* urlString = [self directParameter];
> +    if (!urlString || [urlString isEqualToString:@""]) {

This could just be:
  if ([urlString length] == 0)
which covers both nil and empty at the same time.

Comment 5

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

(In reply to Stuart Morgan from comment #4)
> This could just be:
>   if ([urlString length] == 0)
> which covers both nil and empty at the same time.

Ah, nice; I'll try to remember that for the future :-)

http://hg.mozilla.org/camino/rev/747d01ac0395 with that change.