[chromedriver] Implement command: executeAsyncScript and setScriptTimeout

[chromedriver] Implement command: executeAsyncScript and setScriptTimeout

NOTRY=true

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=187187

[chrisgao (Use stgao instead), 2013-03-08 16:31:55]

ptal

Is the code sufficient to deal with page or frame unload?

[kkania, 2013-03-08 17:25:11]

https://codereview.chromium.org/12675002/diff/1/chrome/test/chromedriver/js/e...
File chrome/test/chromedriver/js/execute_async_script.js (right):

https://codereview.chromium.org/12675002/diff/1/chrome/test/chromedriver/js/e...
chrome/test/chromedriver/js/execute_async_script.js:6: * Execute the given
script and save its result.
saves its asynchronous result

https://codereview.chromium.org/12675002/diff/1/chrome/test/chromedriver/js/e...
chrome/test/chromedriver/js/execute_async_script.js:11: * this callback will be
saved in window.chromedriverAsyncScriptResult. Variable
try not to include unnecessary implementation details in the comment for the
function, which should just describe the interface and any prerequisites.  You
can move such a comment into the body of the function, but actually I'd be fine
with just dropping this part.

https://codereview.chromium.org/12675002/diff/1/chrome/test/chromedriver/js/e...
chrome/test/chromedriver/js/execute_async_script.js:22: var idName =
'chromedriverAsyncScriptId';
how about:

function getAsyncScriptInfo() {
  var key = 'chromedriverAsyncScriptInfo';
  if (!(key in document))
    document[key] = {id: 0, finished:false};
  return document[key];
}

then you can call this here and in your tests

https://codereview.chromium.org/12675002/diff/1/chrome/test/chromedriver/js/e...
chrome/test/chromedriver/js/execute_async_script.js:23: var finishFlagName =
'chromedriverAsyncScriptFinsished';
i think technically you don't need the finish flag since we autoconvert
undefined to null, but let's keep it in case we change that down the road.

https://codereview.chromium.org/12675002/diff/1/chrome/test/chromedriver/js/e...
chrome/test/chromedriver/js/execute_async_script.js:27: document[idName] ++;
style says no space i thnik

https://codereview.chromium.org/12675002/diff/1/chrome/test/chromedriver/js/e...
chrome/test/chromedriver/js/execute_async_script.js:28: document[resultName] =
null;
what's the point of this?

https://codereview.chromium.org/12675002/diff/1/chrome/test/chromedriver/js/e...
chrome/test/chromedriver/js/execute_async_script.js:33: if (id ==
document[idName]) {
i can think of two improvements here:
1) if the user specified a timeout of 0, which is default, we should only return
a result if the callback is called synchronously.
2) we should automatically remove the reference to the result when it is no
longer needed

https://codereview.chromium.org/12675002/diff/1/chrome/test/chromedriver/js/e...
chrome/test/chromedriver/js/execute_async_script.js:40: new
window['Function'](script).apply(null, args);
I don't think you need  the window.  new Function(script) should be fine...

https://codereview.chromium.org/12675002/diff/1/chrome/test/chromedriver/wind...
File chrome/test/chromedriver/window_commands.cc (right):

https://codereview.chromium.org/12675002/diff/1/chrome/test/chromedriver/wind...
chrome/test/chromedriver/window_commands.cc:145: return Status(kJavaScriptError,
"page or frame unload", status);
this should be unknown error, JavaScriptError is only if their script causes an
exception to be thrown

https://codereview.chromium.org/12675002/diff/1/chrome/test/chromedriver/wind...
chrome/test/chromedriver/window_commands.cc:156: if (!dict->Get("result",
&script_result))
i think this will fail if the user calls the callback with no arguments...

https://codereview.chromium.org/12675002/diff/1/chrome/test/chromedriver/wind...
File chrome/test/chromedriver/window_commands.h (right):

https://codereview.chromium.org/12675002/diff/1/chrome/test/chromedriver/wind...
chrome/test/chromedriver/window_commands.h:49: // Evaluates a given script with
arguments asynchronously.
technically, it doesn't evaluate a script asynchronously, it just expects the
result to be given asynchronously, and thus they call the script asynchronous

[chrisgao (Use stgao instead), 2013-03-08 21:06:24]

ptal

https://codereview.chromium.org/12675002/diff/1/chrome/test/chromedriver/js/e...
File chrome/test/chromedriver/js/execute_async_script.js (right):

https://codereview.chromium.org/12675002/diff/1/chrome/test/chromedriver/js/e...
chrome/test/chromedriver/js/execute_async_script.js:6: * Execute the given
script and save its result.
On 2013/03/08 17:25:12, kkania wrote:
> saves its asynchronous result

Done.

https://codereview.chromium.org/12675002/diff/1/chrome/test/chromedriver/js/e...
chrome/test/chromedriver/js/execute_async_script.js:11: * this callback will be
saved in window.chromedriverAsyncScriptResult. Variable
On 2013/03/08 17:25:12, kkania wrote:
> try not to include unnecessary implementation details in the comment for the
> function, which should just describe the interface and any prerequisites.  You
> can move such a comment into the body of the function, but actually I'd be
fine
> with just dropping this part.

Done.

https://codereview.chromium.org/12675002/diff/1/chrome/test/chromedriver/js/e...
chrome/test/chromedriver/js/execute_async_script.js:22: var idName =
'chromedriverAsyncScriptId';
On 2013/03/08 17:25:12, kkania wrote:
> how about:
> 
> function getAsyncScriptInfo() {
>   var key = 'chromedriverAsyncScriptInfo';
>   if (!(key in document))
>     document[key] = {id: 0, finished:false};
>   return document[key];
> }
> 
> then you can call this here and in your tests

Done.

https://codereview.chromium.org/12675002/diff/1/chrome/test/chromedriver/js/e...
chrome/test/chromedriver/js/execute_async_script.js:23: var finishFlagName =
'chromedriverAsyncScriptFinsished';
On 2013/03/08 17:25:12, kkania wrote:
> i think technically you don't need the finish flag since we autoconvert
> undefined to null, but let's keep it in case we change that down the road.
ok.

https://codereview.chromium.org/12675002/diff/1/chrome/test/chromedriver/js/e...
chrome/test/chromedriver/js/execute_async_script.js:27: document[idName] ++;
On 2013/03/08 17:25:12, kkania wrote:
> style says no space i thnik

Done.

https://codereview.chromium.org/12675002/diff/1/chrome/test/chromedriver/js/e...
chrome/test/chromedriver/js/execute_async_script.js:28: document[resultName] =
null;
On 2013/03/08 17:25:12, kkania wrote:
> what's the point of this?
If last script timeout, its result might still be saved.
So delete the result ref instead of setting it as null.

https://codereview.chromium.org/12675002/diff/1/chrome/test/chromedriver/js/e...
chrome/test/chromedriver/js/execute_async_script.js:33: if (id ==
document[idName]) {
On 2013/03/08 17:25:12, kkania wrote:
> i can think of two improvements here:
> 1) if the user specified a timeout of 0, which is default, we should only
return
> a result if the callback is called synchronously.
> 2) we should automatically remove the reference to the result when it is no
> longer needed
1) Raise Timeout error below if this case happens.
2) Remove ref to result in the script within window_commands.cc for pulling
result.

https://codereview.chromium.org/12675002/diff/1/chrome/test/chromedriver/js/e...
chrome/test/chromedriver/js/execute_async_script.js:40: new
window['Function'](script).apply(null, args);
On 2013/03/08 17:25:12, kkania wrote:
> I don't think you need  the window.  new Function(script) should be fine...

Done.
Also set error code here.

https://codereview.chromium.org/12675002/diff/1/chrome/test/chromedriver/wind...
File chrome/test/chromedriver/window_commands.cc (right):

https://codereview.chromium.org/12675002/diff/1/chrome/test/chromedriver/wind...
chrome/test/chromedriver/window_commands.cc:145: return Status(kJavaScriptError,
"page or frame unload", status);
On 2013/03/08 17:25:12, kkania wrote:
> this should be unknown error, JavaScriptError is only if their script causes
an
> exception to be thrown
In the
protocol(https://code.google.com/p/selenium/wiki/JsonWireProtocol#/session/:sessionId/execute_async),
page unload is specified to be JavaScriptError.

https://codereview.chromium.org/12675002/diff/1/chrome/test/chromedriver/wind...
chrome/test/chromedriver/window_commands.cc:156: if (!dict->Get("result",
&script_result))
On 2013/03/08 17:25:12, kkania wrote:
> i think this will fail if the user calls the callback with no arguments...

In this case, the result is set as undefined in the callback, and it is
converted to null. In python, the result is None.

But, let us do a check here.

https://codereview.chromium.org/12675002/diff/1/chrome/test/chromedriver/wind...
File chrome/test/chromedriver/window_commands.h (right):

https://codereview.chromium.org/12675002/diff/1/chrome/test/chromedriver/wind...
chrome/test/chromedriver/window_commands.h:49: // Evaluates a given script with
arguments asynchronously.
On 2013/03/08 17:25:12, kkania wrote:
> technically, it doesn't evaluate a script asynchronously, it just expects the
> result to be given asynchronously, and thus they call the script asynchronous

Done.

https://codereview.chromium.org/12675002/diff/7001/chrome/test/chromedriver/s...
File chrome/test/chromedriver/status.h (right):

https://codereview.chromium.org/12675002/diff/7001/chrome/test/chromedriver/s...
chrome/test/chromedriver/status.h:25: kScriptTimeout = 28,
According to line 330 in
https://code.google.com/p/selenium/source/browse/javascript/atoms/inject.js, I
think the protocol specification
(https://code.google.com/p/selenium/wiki/JsonWireProtocol#/session/:sessionId/...)
should have a typo (Timeout should be ScriptTimeout).

[kkania, 2013-03-08 21:56:50]

https://codereview.chromium.org/12675002/diff/7001/chrome/test/chromedriver/j...
File chrome/test/chromedriver/js/execute_async_script.js (right):

https://codereview.chromium.org/12675002/diff/7001/chrome/test/chromedriver/j...
chrome/test/chromedriver/js/execute_async_script.js:29: info['id']++;
instead of info['id'] style, do info.id; same for other cases

https://codereview.chromium.org/12675002/diff/7001/chrome/test/chromedriver/j...
chrome/test/chromedriver/js/execute_async_script.js:36: info['result'] = result;
there's still a small bug where this will hold a reference to the result if the
script times out but the result is set after the timeout.

[chrisgao (Use stgao instead), 2013-03-08 23:28:25]

ptal

https://codereview.chromium.org/12675002/diff/7001/chrome/test/chromedriver/j...
File chrome/test/chromedriver/js/execute_async_script.js (right):

https://codereview.chromium.org/12675002/diff/7001/chrome/test/chromedriver/j...
chrome/test/chromedriver/js/execute_async_script.js:29: info['id']++;
On 2013/03/08 21:56:50, kkania wrote:
> instead of info['id'] style, do info.id; same for other cases

Done.

https://codereview.chromium.org/12675002/diff/7001/chrome/test/chromedriver/j...
chrome/test/chromedriver/js/execute_async_script.js:36: info['result'] = result;
On 2013/03/08 21:56:50, kkania wrote:
> there's still a small bug where this will hold a reference to the result if
the
> script times out but the result is set after the timeout.

Done.

https://codereview.chromium.org/12675002/diff/7001/chrome/test/chromedriver/j...
chrome/test/chromedriver/js/execute_async_script.js:51: if (timeout == 0 &&
!info['finished']) {
Removed, because this failed one java test.

executeAsyncScript {"script":"var callback = arguments[arguments.length -
1];window.setTimeout(function() { callback(123); }, 0)","args":[]}

And now we pass all java test on command executeAsyncScript.

https://codereview.chromium.org/12675002/diff/7001/chrome/test/chromedriver/w...
File chrome/test/chromedriver/window_commands.cc (right):

https://codereview.chromium.org/12675002/diff/7001/chrome/test/chromedriver/w...
chrome/test/chromedriver/window_commands.cc:221: "    toReturn['result'] =
info['result'];"
Javascript has an delayed function call to clear the ref.

https://codereview.chromium.org/12675002/diff/21001/chrome/test/chromedriver/...
File chrome/test/chromedriver/js/execute_async_script.js (right):

https://codereview.chromium.org/12675002/diff/21001/chrome/test/chromedriver/...
chrome/test/chromedriver/js/execute_async_script.js:56:
setTimeout(clearResultIfTimeout, timeout + 5000);
I think 5 seconds should be long enough for C++ to get the result.

Another reliable way to clear the result is in C++:
1. Clear the ref during pulling result if it is set.
2. Execute "var info = document.chromedriverAsyncScriptInfo; info.id++; delete
info.result;" if we timeout.

Which one do you prefer?

[kkania, 2013-03-09 00:31:02]

On 2013/03/08 23:28:25, Shuotao Gao wrote:
> ptal
> 
>
https://codereview.chromium.org/12675002/diff/7001/chrome/test/chromedriver/j...
> File chrome/test/chromedriver/js/execute_async_script.js (right):
> 
>
https://codereview.chromium.org/12675002/diff/7001/chrome/test/chromedriver/j...
> chrome/test/chromedriver/js/execute_async_script.js:29: info['id']++;
> On 2013/03/08 21:56:50, kkania wrote:
> > instead of info['id'] style, do info.id; same for other cases
> 
> Done.
> 
>
https://codereview.chromium.org/12675002/diff/7001/chrome/test/chromedriver/j...
> chrome/test/chromedriver/js/execute_async_script.js:36: info['result'] =
result;
> On 2013/03/08 21:56:50, kkania wrote:
> > there's still a small bug where this will hold a reference to the result if
> the
> > script times out but the result is set after the timeout.
> 
> Done.
> 
>
https://codereview.chromium.org/12675002/diff/7001/chrome/test/chromedriver/j...
> chrome/test/chromedriver/js/execute_async_script.js:51: if (timeout == 0 &&
> !info['finished']) {
> Removed, because this failed one java test.
> 
> executeAsyncScript {"script":"var callback = arguments[arguments.length -
> 1];window.setTimeout(function() { callback(123); }, 0)","args":[]}
> 
> And now we pass all java test on command executeAsyncScript.
> 
>
https://codereview.chromium.org/12675002/diff/7001/chrome/test/chromedriver/w...
> File chrome/test/chromedriver/window_commands.cc (right):
> 
>
https://codereview.chromium.org/12675002/diff/7001/chrome/test/chromedriver/w...
> chrome/test/chromedriver/window_commands.cc:221: "    toReturn['result'] =
> info['result'];"
> Javascript has an delayed function call to clear the ref.
> 
>
https://codereview.chromium.org/12675002/diff/21001/chrome/test/chromedriver/...
> File chrome/test/chromedriver/js/execute_async_script.js (right):
> 
>
https://codereview.chromium.org/12675002/diff/21001/chrome/test/chromedriver/...
> chrome/test/chromedriver/js/execute_async_script.js:56:
> setTimeout(clearResultIfTimeout, timeout + 5000);
> I think 5 seconds should be long enough for C++ to get the result.
> 
> Another reliable way to clear the result is in C++:
> 1. Clear the ref during pulling result if it is set.
> 2. Execute "var info = document.chromedriverAsyncScriptInfo; info.id++; delete
> info.result;" if we timeout.
> 
> Which one do you prefer?

let's talk offline

[kkania, 2013-03-09 00:55:20]

lgtm once you fix the arbitrary 5000 timeout problem

[kkania, 2013-03-09 05:52:01]

On 2013/03/09 00:55:20, kkania wrote:
> lgtm once you fix the arbitrary 5000 timeout problem

Hope you're having a good weekend.

[commit-bot: I haz the power, 2013-03-10 01:54:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/chrisgao@chromium.org/12675002/32013

[commit-bot: I haz the power, 2013-03-10 03:59:10]

Change committed as 187187