Bug 7012 – convert JavaScript objects to appropriate AppleScript types, instead of only strings

• Component: WebKit API
• Status: RESOLVED FIXED

[tim bates]

The "do javaScript" AppleScript command for Safari currently fails to return anything that is not a string.

As access to the  Safari DOM via AppleScript is entirely restricted to calls to "do javaScript', it would be a helpful convenience to enhance  this call by automatically coercing any non-string results to string, so that the user does not have to do this manually.

i.e., allow do javaScript " document.getSelection; " to return the selection as a string, rather than requiring the current workaround of

do javaScript " document.getSelection + '' ; "

In the case of javaScript lists, these should be coerced to applescript lists, same for records would be nice.

[Alexey Proskuryakov]

Looks like this cannot be done entirely in open-source WebKit - Safari calls -[WebView stringByEvaluatingJavaScriptFromString:] to execute "do JavaScript", so returning lists doesn't appear possible at the moment.

My suggestion is to add a method returning NSAppleEventDescriptor to WebView, and to make Safari call it. Does this sound right?

[Alexey Proskuryakov]

Created attachment 6223 [details]
proposed patch

This adds conversion for Boolean, String, Number and Date, and implicitly calls toString() for unknown object types.

I am not sure if Array can really be converted to an AppleScript list reliably... JavaScript arrays can be sparse, associated, or even contain circular references (if I'm not mistaken) - so, I don't see any straightforward (as in "no surprises for the user") way to convert them, other than toString().

[Alexey Proskuryakov]

A script that I used for testing:

tell application "Safari"
	do JavaScript "new Boolean();" in document 1
	do JavaScript "2*2;" in document 1
	do JavaScript "2/3;" in document 1
	do JavaScript "'a string'" in document 1
	do JavaScript "new String('a string')" in document 1
	do JavaScript "new Date" in document 1
	do JavaScript "window.getSelection()" in document 1
	do JavaScript "window.getSelection() + \"\"" in document 1
	do JavaScript "function Polygon() {this.edges = 8;} new Polygon;" in document 1
	do JavaScript "new Array(1, 2, 'three');" in document 1
end tell

[Timothy Hatcher]

Great idea! Coercing to list descriptors is easy to do using [NSAppleEventDescriptor listDescriptor] and insertDescriptor:atIndex:. Unfortunatly you can only make AppleScript records from dicrionaries with string keys if you drop down to Carbon to contruct them.

[Timothy Hatcher]

Comment on attachment 6223 [details]
proposed patch

The patch looks good, only a few minor details.

I would fall back to always returning a string in the "default" case instead of the "Unknown JavaScript type: %d", jsValue->type()" error, unless this is never reached.

The WebView.h header can't be changed until we get the API approved. You will need to use WebViewPrivate.h for this interface.

[Alexey Proskuryakov]

Created attachment 6237 [details]
proposed patch

(In reply to comment #5)
> I would fall back to always returning a string in the "default" case instead of
> the "Unknown JavaScript type: %d", jsValue->type()" error, unless this is never
> reached.

  Yes, it should be never reached - the switch has cases for all primitive JavaScript types (I think).

> The WebView.h header can't be changed until we get the API approved. You will
> need to use WebViewPrivate.h for this interface.

  Done.

> Coercing to list descriptors is easy to do using
> [NSAppleEventDescriptor listDescriptor] and insertDescriptor:atIndex:.

  Yes, that's easy - I just don't see how the end result should look like for non-trivial JavaScript arrays. And adding a conversion that only works "most of the time" is IMHO not a good idea...

  Still, one option to consider: only convert arrays that are not sparse, for which element indices start with zero, and don't descend into them recursively (to avoid circular references). What do you think?

[Darin Adler]

Comment on attachment 6237 [details]
proposed patch

I'd like Tim Hatcher to review this, but I have a comment:

I don't like having so much code copied and pasted out of JavaScriptCore. We should add a suitable function to JavaScriptCore that gets us the appropriate JSValue. It's fine for aeDescFromJSValue to convert from JSValue to AEDesc, but it shouldn't convert from, say, an object instance to a boolean as it does in the ObjectType case. We should create something on the JavaScriptCore side that converts to a boolean, number, or string, and also returns an "isDate" flag. Then the AE conversion can concentrate on things like converting the date format, etc.

This would prevent us from having to expose bool_object.h as a Private header and give us slightly better encapsulation for JavaScriptCore.

[Alexey Proskuryakov]

(In reply to comment #7)
> This would prevent us from having to expose bool_object.h as a Private header
> and give us slightly better encapsulation for JavaScriptCore.

Ouch, I thought that bool_object.h not being exposed was unintentional (the other *_object.h are already exposed).

[Darin Adler]

(In reply to comment #8)
> Ouch, I thought that bool_object.h not being exposed was unintentional (the
> other *_object.h are already exposed).

On the one hand, it's not mportant that bool_object.h not be exposed. On the other hand, those are details I'd prefer not be depended on by WebCore. I'd like to study why those other headers are needed in WebCore.

[Alexey Proskuryakov]

Created attachment 6267 [details]
proposed patch

Looks like I was doing the object->value convertion in a needlessly complex way, and toPrimitive() already takes care of this stuff. However, I don't really see where an isDate flag could be added, and if it would be less of an implementation detail than DateInstance::info.

[Darin Adler]

Comment on attachment 6267 [details]
proposed patch

Please don't put a copy of tryGetAndCallProperty into WebCore. And if you feel you must put a copy of it there, use ALWAYS_INLINE, not another copy of that ifdef. Since you're using this specifically just to get a number, the function can be simplified to handle just that case and leave out the type() stuff. The code really needs to check exceptions, and not look at the return value if there's an exception, so it's a pasted copy of incorrect code!

I think it's reasonable for some file in JavaScriptCore to provide a function that gives you a date as a number or pair of numbers, and a boolean that tells you whether it's a date, in a way analogous to functions like getString(), rather than digging it out with multiple function calls to the date object.

On a separate note, one way to recognize the date class is by its class name, which would avoid the need to include DateInstance and link to DateInstance::info. You can just say if o->className() == "Date".

How are you testing this function? I think we need to hook this up to DumpRenderTree so we can test that it's working.

[Alexey Proskuryakov]

(In reply to comment #11)
> How are you testing this function? I think we need to hook this up to
> DumpRenderTree so we can test that it's working.

I just had some custom code for debugging. Using DumpRenderTree is indeed so much better; I've added code for this and a test - will be available in the next revision.

[Alexey Proskuryakov]

Created attachment 6296 [details]
proposed patch

Besides the already discussed improvements, changed the way how milliseconds are converted to LongDateTime - the new code works for dates in the past, where POSIX and Mac date routines disagree about time zone handling.

[Timothy Hatcher]

Comment on attachment 6296 [details]
proposed patch

Good idea about the LayoutTests. This looks good. r=me

[Alexey Proskuryakov]

Tweaked the date test when landing - made it timezone-agnostic.

Filed an enchancement request for Safari to use this new API: <rdar://4435959>.