Bug 7012 - convert JavaScript objects to appropriate AppleScript types, instead of only strings
Summary: convert JavaScript objects to appropriate AppleScript types, instead of only ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit API (show other bugs)
Version: 420+
Hardware: Mac OS X 10.4
: P2 Enhancement
Assignee: Nobody
URL:
Keywords:
Depends on: 7107
Blocks: 10693
  Show dependency treegraph
 
Reported: 2006-02-01 14:23 PST by tim bates
Modified: 2006-02-07 09:01 PST (History)
2 users (show)

See Also:


Attachments
proposed patch (9.83 KB, patch)
2006-02-03 11:51 PST, Alexey Proskuryakov
timothy: review-
Details | Formatted Diff | Diff
proposed patch (8.78 KB, patch)
2006-02-04 00:51 PST, Alexey Proskuryakov
no flags Details | Formatted Diff | Diff
proposed patch (6.57 KB, patch)
2006-02-05 03:26 PST, Alexey Proskuryakov
darin: review-
Details | Formatted Diff | Diff
proposed patch (23.29 KB, patch)
2006-02-06 11:53 PST, Alexey Proskuryakov
timothy: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description tim bates 2006-02-01 14:23:19 PST
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.
Comment 1 Alexey Proskuryakov 2006-02-01 22:10:30 PST
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?
Comment 2 Alexey Proskuryakov 2006-02-03 11:51:58 PST
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().
Comment 3 Alexey Proskuryakov 2006-02-03 12:17:26 PST
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
Comment 4 Timothy Hatcher 2006-02-03 14:47:28 PST
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.
Comment 5 Timothy Hatcher 2006-02-03 14:51:43 PST
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.
Comment 6 Alexey Proskuryakov 2006-02-04 00:51:49 PST
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?
Comment 7 Darin Adler 2006-02-04 08:33:58 PST
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.
Comment 8 Alexey Proskuryakov 2006-02-04 10:47:03 PST
(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).
Comment 9 Darin Adler 2006-02-04 16:50:02 PST
(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.
Comment 10 Alexey Proskuryakov 2006-02-05 03:26:54 PST
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.
Comment 11 Darin Adler 2006-02-05 03:55:51 PST
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.
Comment 12 Alexey Proskuryakov 2006-02-05 06:00:41 PST
(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.
Comment 13 Alexey Proskuryakov 2006-02-06 11:53:11 PST
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.
Comment 14 Timothy Hatcher 2006-02-06 17:27:26 PST
Comment on attachment 6296 [details]
proposed patch

Good idea about the LayoutTests. This looks good. r=me
Comment 15 Alexey Proskuryakov 2006-02-07 09:01:54 PST
Tweaked the date test when landing - made it timezone-agnostic.

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