If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

open location's direct-parameter should not be optional

RESOLVED FIXED

Status

Camino Graveyard
OS Integration
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Smokey Ardisson (offline for a while; not following bugs - do not email), Assigned: Smokey Ardisson (offline for a while; not following bugs - do not email))

Tracking

Details

(Whiteboard: [camino-2.1.1])

Attachments

(1 attachment, 1 obsolete attachment)

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.)
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.
Created attachment 589636 [details] [diff] [review]
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
Assignee: nobody → alqahira
Status: NEW → ASSIGNED
Attachment #589636 - Flags: superreview?(stuart.morgan+bugzilla)
Created attachment 589656 [details] [diff] [review]
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).
Attachment #589636 - Attachment is obsolete: true
Attachment #589636 - Flags: superreview?(stuart.morgan+bugzilla)
Attachment #589656 - Flags: superreview?(stuart.morgan+bugzilla)

Comment 4

6 years ago
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.
Attachment #589656 - Flags: superreview?(stuart.morgan+bugzilla) → superreview+
(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.
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [camino-2.1.1]
You need to log in before you can comment on or make changes to this bug.