Applescript: return value from execute javascript command

Applescript: return value from execute javascript command

BUG=78283
TEST=


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=107046

[keishi, 2011-10-17 09:06:34]

I added RenderViewHost::ExecuteJavascriptAndGetValue that could get the return
value from JavaScript.

[Avi (use Gerrit), 2011-10-17 14:20:38]

http://codereview.chromium.org/8124024/diff/4001/chrome/browser/ui/cocoa/appl...
File chrome/browser/ui/cocoa/applescript/tab_applescript.mm (right):

http://codereview.chromium.org/8124024/diff/4001/chrome/browser/ui/cocoa/appl...
chrome/browser/ui/cocoa/applescript/tab_applescript.mm:43: -
(NSAppleEventDescriptor *) scriptingAnyDescriptor {
nit: no space after )

http://codereview.chromium.org/8124024/diff/4001/chrome/browser/ui/cocoa/appl...
chrome/browser/ui/cocoa/applescript/tab_applescript.mm:65: descriptor =
[NSAppleEventDescriptor descriptorWithInt32:int_value];
Why int32? Is this future proof for 64-bits?

http://codereview.chromium.org/8124024/diff/4001/chrome/browser/ui/cocoa/appl...
chrome/browser/ui/cocoa/applescript/tab_applescript.mm:101: [descriptor
setDescriptor:userRecord forKeyword:keyASUserRecordFields];
Can you find a link to put in here for keyASUserRecordFields? That's a bit
obscure and I had to search for a while.

http://codereview.chromium.org/8124024/diff/4001/chrome/browser/ui/cocoa/appl...
chrome/browser/ui/cocoa/applescript/tab_applescript.mm:111: [descriptor
insertDescriptor:valueToDescriptor(item) atIndex:0];
Doesn't this reverse the order of the items in the list?

http://codereview.chromium.org/8124024/diff/4001/chrome/browser/ui/cocoa/appl...
chrome/browser/ui/cocoa/applescript/tab_applescript.mm:399: return
[[[AnyResultValue alloc] initWithDescriptor:descriptor] autorelease];
I'm a bit lost. We're returning a custom object? How does the scripting
environment know how to get data out?

http://codereview.chromium.org/8124024/diff/4001/content/browser/renderer_hos...
File content/browser/renderer_host/render_view_host.cc (right):

http://codereview.chromium.org/8124024/diff/4001/content/browser/renderer_hos...
content/browser/renderer_host/render_view_host.cc:488:
ExecuteNotificationObserver::ExecuteNotificationObserver(int id) : id_(id) { }
adhere to style guide

http://codereview.chromium.org/8124024/diff/4001/content/browser/renderer_hos...
content/browser/renderer_host/render_view_host.cc:489:
ExecuteNotificationObserver::~ExecuteNotificationObserver() { }
ditto

http://codereview.chromium.org/8124024/diff/4001/content/browser/renderer_hos...
content/browser/renderer_host/render_view_host.cc:493: {
opening brace always on previous line

http://codereview.chromium.org/8124024/diff/4001/content/browser/renderer_hos...
content/browser/renderer_host/render_view_host.cc:494: int id =
(static_cast<Details<ExecuteDetailType > >(details))->first;
no space before first >

http://codereview.chromium.org/8124024/diff/4001/content/browser/renderer_hos...
content/browser/renderer_host/render_view_host.cc:497: Value* value =
(static_cast<Details<ExecuteDetailType > >(details))->second;
ditto

http://codereview.chromium.org/8124024/diff/4001/content/browser/renderer_hos...
content/browser/renderer_host/render_view_host.cc:504: const string16& jscript)
{
align params

http://codereview.chromium.org/8124024/diff/4001/content/browser/renderer_hos...
content/browser/renderer_host/render_view_host.cc:513: return
observer.value()->DeepCopy();
We make two copies?

http://codereview.chromium.org/8124024/diff/4001/content/browser/renderer_hos...
File content/browser/renderer_host/render_view_host.h (right):

http://codereview.chromium.org/8124024/diff/4001/content/browser/renderer_hos...
content/browser/renderer_host/render_view_host.h:82:
ExecuteNotificationObserver(int id);
one param constructors should be "explicit"

http://codereview.chromium.org/8124024/diff/4001/content/browser/renderer_hos...
content/browser/renderer_host/render_view_host.h:286: const string16& jscript);
align params, add comment; do we want to merge this with ExecuteJSInWebFrame?

[keishi, 2011-10-21 05:05:43]

http://codereview.chromium.org/8124024/diff/4001/chrome/browser/ui/cocoa/appl...
File chrome/browser/ui/cocoa/applescript/tab_applescript.mm (right):

http://codereview.chromium.org/8124024/diff/4001/chrome/browser/ui/cocoa/appl...
chrome/browser/ui/cocoa/applescript/tab_applescript.mm:43: -
(NSAppleEventDescriptor *) scriptingAnyDescriptor {
On 2011/10/17 14:20:38, Avi wrote:
> nit: no space after )

Done.

http://codereview.chromium.org/8124024/diff/4001/chrome/browser/ui/cocoa/appl...
chrome/browser/ui/cocoa/applescript/tab_applescript.mm:65: descriptor =
[NSAppleEventDescriptor descriptorWithInt32:int_value];
On 2011/10/17 14:20:38, Avi wrote:
> Why int32? Is this future proof for 64-bits?

JS integers are 32 bits.

http://codereview.chromium.org/8124024/diff/4001/chrome/browser/ui/cocoa/appl...
chrome/browser/ui/cocoa/applescript/tab_applescript.mm:101: [descriptor
setDescriptor:userRecord forKeyword:keyASUserRecordFields];
I can't find the right documentation from Apple so I linked to this
http://google-toolbox-for-mac.googlecode.com/svn/tags/google-toolbox-for-mac-...

http://codereview.chromium.org/8124024/diff/4001/chrome/browser/ui/cocoa/appl...
chrome/browser/ui/cocoa/applescript/tab_applescript.mm:111: [descriptor
insertDescriptor:valueToDescriptor(item) atIndex:0];
On 2011/10/17 14:20:38, Avi wrote:
> Doesn't this reverse the order of the items in the list?

AppleScript lists are 1 based and specifying index 0 appends the object at the
end.
http://developer.apple.com/library/mac/documentation/Cocoa/Reference/Foundati...:

http://codereview.chromium.org/8124024/diff/4001/chrome/browser/ui/cocoa/appl...
chrome/browser/ui/cocoa/applescript/tab_applescript.mm:399: return
[[[AnyResultValue alloc] initWithDescriptor:descriptor] autorelease];
On 2011/10/17 14:20:38, Avi wrote:
> I'm a bit lost. We're returning a custom object? How does the scripting
> environment know how to get data out?

The AppleScript calls selectors on the object based on the type specified in the
sdef file.
In this case command "execute javascript" returns an "any" object so it is going
to call, @selector(scriptingAnyDescriptor).

http://codereview.chromium.org/8124024/diff/4001/content/browser/renderer_hos...
File content/browser/renderer_host/render_view_host.cc (right):

http://codereview.chromium.org/8124024/diff/4001/content/browser/renderer_hos...
content/browser/renderer_host/render_view_host.cc:488:
ExecuteNotificationObserver::ExecuteNotificationObserver(int id) : id_(id) { }
On 2011/10/17 14:20:38, Avi wrote:
> adhere to style guide

Done.

http://codereview.chromium.org/8124024/diff/4001/content/browser/renderer_hos...
content/browser/renderer_host/render_view_host.cc:489:
ExecuteNotificationObserver::~ExecuteNotificationObserver() { }
On 2011/10/17 14:20:38, Avi wrote:
> ditto

Done.

http://codereview.chromium.org/8124024/diff/4001/content/browser/renderer_hos...
content/browser/renderer_host/render_view_host.cc:493: {
On 2011/10/17 14:20:38, Avi wrote:
> opening brace always on previous line

Done.

http://codereview.chromium.org/8124024/diff/4001/content/browser/renderer_hos...
content/browser/renderer_host/render_view_host.cc:494: int id =
(static_cast<Details<ExecuteDetailType > >(details))->first;
On 2011/10/17 14:20:38, Avi wrote:
> no space before first >

Done.

http://codereview.chromium.org/8124024/diff/4001/content/browser/renderer_hos...
content/browser/renderer_host/render_view_host.cc:497: Value* value =
(static_cast<Details<ExecuteDetailType > >(details))->second;
On 2011/10/17 14:20:38, Avi wrote:
> ditto

Done.

http://codereview.chromium.org/8124024/diff/4001/content/browser/renderer_hos...
content/browser/renderer_host/render_view_host.cc:504: const string16& jscript)
{
On 2011/10/17 14:20:38, Avi wrote:
> align params

Done.

http://codereview.chromium.org/8124024/diff/4001/content/browser/renderer_hos...
content/browser/renderer_host/render_view_host.cc:513: return
observer.value()->DeepCopy();
On 2011/10/17 14:20:38, Avi wrote:
> We make two copies?

The value must outlive the observer so this copy is necessary.

http://codereview.chromium.org/8124024/diff/4001/content/browser/renderer_hos...
File content/browser/renderer_host/render_view_host.h (right):

http://codereview.chromium.org/8124024/diff/4001/content/browser/renderer_hos...
content/browser/renderer_host/render_view_host.h:82:
ExecuteNotificationObserver(int id);
On 2011/10/17 14:20:38, Avi wrote:
> one param constructors should be "explicit"

Done.

http://codereview.chromium.org/8124024/diff/4001/content/browser/renderer_hos...
content/browser/renderer_host/render_view_host.h:286: const string16& jscript);
On 2011/10/17 14:20:38, Avi wrote:
> align params, add comment; do we want to merge this with ExecuteJSInWebFrame?

Do you want me to add an argument to ExecuteJavascriptInWebFrame? Like
void ExecuteJavascriptInWebFrame(const string16& frame_xpath,
                                    const string16& jscript,
                                    bool return_value);

[Avi (use Gerrit), 2011-10-21 11:50:48]

LGTM

http://codereview.chromium.org/8124024/diff/4001/chrome/browser/ui/cocoa/appl...
File chrome/browser/ui/cocoa/applescript/tab_applescript.mm (right):

http://codereview.chromium.org/8124024/diff/4001/chrome/browser/ui/cocoa/appl...
chrome/browser/ui/cocoa/applescript/tab_applescript.mm:101: [descriptor
setDescriptor:userRecord forKeyword:keyASUserRecordFields];
A plaintext description?
http://www.mail-archive.com/cocoa-dev@lists.apple.com/msg40149.html

But yes, that's very underdocumented.

http://codereview.chromium.org/8124024/diff/4001/chrome/browser/ui/cocoa/appl...
chrome/browser/ui/cocoa/applescript/tab_applescript.mm:111: [descriptor
insertDescriptor:valueToDescriptor(item) atIndex:0];
Sigh. Been way too long since I've done this.

http://codereview.chromium.org/8124024/diff/4001/chrome/browser/ui/cocoa/appl...
chrome/browser/ui/cocoa/applescript/tab_applescript.mm:399: return
[[[AnyResultValue alloc] initWithDescriptor:descriptor] autorelease];
Gotcha.

http://codereview.chromium.org/8124024/diff/4001/content/browser/renderer_hos...
File content/browser/renderer_host/render_view_host.h (right):

http://codereview.chromium.org/8124024/diff/4001/content/browser/renderer_hos...
content/browser/renderer_host/render_view_host.h:286: const string16& jscript);
hmmmm... What fraction of the users bother to set up an environment to capture
the result and would use it? If low, then it's probably not worth bothering. In
any case, leave it for another changelist.

[keishi, 2011-10-24 02:12:15]

On 2011/10/21 11:50:48, Avi wrote:
> A plaintext description?
> http://www.mail-archive.com/cocoa-dev%40lists.apple.com/msg40149.html
> 
> But yes, that's very underdocumented.
This is better. I'll use this instead.

http://codereview.chromium.org/8124024/diff/4001/content/browser/renderer_hos...
> content/browser/renderer_host/render_view_host.h:286: const string16&
jscript);
> hmmmm... What fraction of the users bother to set up an environment to capture
> the result and would use it? If low, then it's probably not worth bothering.
In
> any case, leave it for another changelist.

OK.

Thanks!

[commit-bot: I haz the power, 2011-10-24 13:32:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/keishi@chromium.org/8124024/17001

[commit-bot: I haz the power, 2011-10-24 14:35:48]

The commit queue went berserk retrying too often for a
seemingly flaky test. Builder is linux_rel, revision is 106910, job name
was 8124024-17001 (previous was lost) (previous was lost) (previous was lost)
(previous was lost).

[commit-bot: I haz the power, 2011-10-25 01:19:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/keishi@chromium.org/8124024/17001

[commit-bot: I haz the power, 2011-10-25 02:34:15]

Change committed as 107046