[ChromeDriver] Select the main frame if a non-existant child frame is targeted.

[ChromeDriver] Select the main frame if a non-existant child frame is targeted.

BUG=169092
TEST=run_py_tests.py


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

[craigdh, 2013-02-06 00:55:59]

https://codereview.chromium.org/12226026/diff/1/chrome/test/chromedriver/sess...
File chrome/test/chromedriver/session_command.cc (right):

https://codereview.chromium.org/12226026/diff/1/chrome/test/chromedriver/sess...
chrome/test/chromedriver/session_command.cc:8: #include "base/logging.h"
Not needed, I'll remove it in the next upload.

[chrisgao (Use stgao instead), 2013-02-06 01:32:14]

https://codereview.chromium.org/12226026/diff/1/chrome/test/chromedriver/run_...
File chrome/test/chromedriver/run_py_tests.py (right):

https://codereview.chromium.org/12226026/diff/1/chrome/test/chromedriver/run_...
chrome/test/chromedriver/run_py_tests.py:114: lambda:
self._driver.ExecuteScript('return window.top == window'), 5))
Are 5 seconds long enough if the bot is in heavy load? I suggest to make it a
little longer.

https://codereview.chromium.org/12226026/diff/1/chrome/test/chromedriver/sess...
File chrome/test/chromedriver/session_command.cc (right):

https://codereview.chromium.org/12226026/diff/1/chrome/test/chromedriver/sess...
chrome/test/chromedriver/session_command.cc:38: if (status.IsError() &&
status.code() == kNoSuchFrame) {
I think status.IsError() is unnecessary.

[kkania, 2013-02-06 01:41:25]

https://codereview.chromium.org/12226026/diff/1/chrome/test/chromedriver/devt...
File chrome/test/chromedriver/devtools_client_impl.h (right):

https://codereview.chromium.org/12226026/diff/1/chrome/test/chromedriver/devt...
chrome/test/chromedriver/devtools_client_impl.h:32: kContextIdNotFound = -32000,
-32000 just means generic server error.  devtools doesn't have very specific
error codes.  Here's our options:

1) add error codes
This will take a bit of work and you should probably consult with the devtools
guys first.  It might be hard to make this in before next week's branch.

2) compare message strings instead of the codes
This is a bit hacky, but probably fairly stable.  I think this one is "Execution
context with given id not found."  You can look at the webkit history to see if
this has ever changed.  If they do happen to change in the future, we can just
map multiple messages to the same error.  I think eventually we should fix this
though...

3) after receiving a generic error, check and see if the frame is detached; if
so, report error
This should work, but it doesn't seem that clean to me either.  This assumes
we'll receive the detach message before the command response, which probably
works now.

WHat do you think?

https://codereview.chromium.org/12226026/diff/1/chrome/test/chromedriver/run_...
File chrome/test/chromedriver/run_py_tests.py (right):

https://codereview.chromium.org/12226026/diff/1/chrome/test/chromedriver/run_...
chrome/test/chromedriver/run_py_tests.py:102: '}, 1000);')
is there a non racy way to do this?

[craigdh, 2013-02-06 18:52:49]

https://codereview.chromium.org/12226026/diff/1/chrome/test/chromedriver/devt...
File chrome/test/chromedriver/devtools_client_impl.h (right):

https://codereview.chromium.org/12226026/diff/1/chrome/test/chromedriver/devt...
chrome/test/chromedriver/devtools_client_impl.h:32: kContextIdNotFound = -32000,
On 2013/02/06 01:41:25, kkania wrote:
> -32000 just means generic server error.  devtools doesn't have very specific
> error codes.  Here's our options:
> 
> 1) add error codes
> This will take a bit of work and you should probably consult with the devtools
> guys first.  It might be hard to make this in before next week's branch.
> 
> 2) compare message strings instead of the codes
> This is a bit hacky, but probably fairly stable.  I think this one is
"Execution
> context with given id not found."  You can look at the webkit history to see
if
> this has ever changed.  If they do happen to change in the future, we can just
> map multiple messages to the same error.  I think eventually we should fix
this
> though...
> 
> 3) after receiving a generic error, check and see if the frame is detached; if
> so, report error
> This should work, but it doesn't seem that clean to me either.  This assumes
> we'll receive the detach message before the command response, which probably
> works now.
> 
> WHat do you think?

Ah, that's unfortunate. I think for now the best choice is probably (2) as a
one-off, but we should reevaluate (1) if we find ourselves needing to watch for
more devtools errors in the future. I'm also not a big fan of (3), it seems
unnecessarily messy.

https://codereview.chromium.org/12226026/diff/1/chrome/test/chromedriver/run_...
File chrome/test/chromedriver/run_py_tests.py (right):

https://codereview.chromium.org/12226026/diff/1/chrome/test/chromedriver/run_...
chrome/test/chromedriver/run_py_tests.py:102: '}, 1000);')
On 2013/02/06 01:41:25, kkania wrote:
> is there a non racy way to do this?

Yeah, just realized I can probably do it with a postMessage() across the frame
boundary.

https://codereview.chromium.org/12226026/diff/1/chrome/test/chromedriver/run_...
chrome/test/chromedriver/run_py_tests.py:114: lambda:
self._driver.ExecuteScript('return window.top == window'), 5))
On 2013/02/06 01:32:14, Shuotao Gao wrote:
> Are 5 seconds long enough if the bot is in heavy load? I suggest to make it a
> little longer.

Done.

https://codereview.chromium.org/12226026/diff/1/chrome/test/chromedriver/sess...
File chrome/test/chromedriver/session_command.cc (right):

https://codereview.chromium.org/12226026/diff/1/chrome/test/chromedriver/sess...
chrome/test/chromedriver/session_command.cc:38: if (status.IsError() &&
status.code() == kNoSuchFrame) {
On 2013/02/06 01:32:14, Shuotao Gao wrote:
> I think status.IsError() is unnecessary.

Done.

[craigdh, 2013-02-06 19:54:02]

ptal.

[kkania, 2013-02-06 23:45:26]

lgtm

https://codereview.chromium.org/12226026/diff/5015/chrome/test/chromedriver/r...
File chrome/test/chromedriver/run_py_tests.py (right):

https://codereview.chromium.org/12226026/diff/5015/chrome/test/chromedriver/r...
chrome/test/chromedriver/run_py_tests.py:12: import time
x

https://codereview.chromium.org/12226026/diff/5015/chrome/test/chromedriver/s...
File chrome/test/chromedriver/session_command.cc (right):

https://codereview.chromium.org/12226026/diff/5015/chrome/test/chromedriver/s...
chrome/test/chromedriver/session_command.cc:37: if (status.code() ==
kNoSuchFrame) {
I'm worried this might not be correct for all circumstances, but I can't think
of anything, so ok!

[craigdh, 2013-02-07 00:05:37]

https://codereview.chromium.org/12226026/diff/5015/chrome/test/chromedriver/r...
File chrome/test/chromedriver/run_py_tests.py (right):

https://codereview.chromium.org/12226026/diff/5015/chrome/test/chromedriver/r...
chrome/test/chromedriver/run_py_tests.py:12: import time
On 2013/02/06 23:45:26, kkania wrote:
> x

Done.

https://codereview.chromium.org/12226026/diff/5015/chrome/test/chromedriver/s...
File chrome/test/chromedriver/session_command.cc (right):

https://codereview.chromium.org/12226026/diff/5015/chrome/test/chromedriver/s...
chrome/test/chromedriver/session_command.cc:37: if (status.code() ==
kNoSuchFrame) {
On 2013/02/06 23:45:26, kkania wrote:
> I'm worried this might not be correct for all circumstances, but I can't think
> of anything, so ok!

When the inevitable bugs raise their heads assign them to me and I shall squash
them.

[commit-bot: I haz the power, 2013-02-07 00:20:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/craigdh@chromium.org/12226026/11004

[commit-bot: I haz the power, 2013-02-07 06:17:36]

Change committed as 181233