[chromedriver] Implement reconnection to DevTools.

[chromedriver] Implement reconnection to DevTools.

When user debugs a testcase with DevTools frontend UI, DevToolsClient will be disconnected. This CL is to automatically close DevTools frontend and reconnect afterwards.

Only try to reconnect before processing a command received from the chromedriver client.

There are two types of frontend UI: one is docked in page, opened through Shift+Ctrl+I or "Tools"--"Developer Tools"; the other is in a new tab, opened with url "chrome-devtools:///...".
For the tab one, we can close it by json/close of DevTools, just like closing a normal page. But for the docked one, we could only close it by clicking a button through Javascript.

BUG=169098
NOTRY=true

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

[chrisgao (Use stgao instead), 2013-02-22 00:09:41]

ptal

[kkania, 2013-02-22 01:35:24]

https://codereview.chromium.org/12321057/diff/11011/chrome/test/chromedriver/...
File chrome/test/chromedriver/chrome_impl.cc (right):

https://codereview.chromium.org/12321057/diff/11011/chrome/test/chromedriver/...
chrome/test/chromedriver/chrome_impl.cc:29: const ParserFunc& parser) {
i think this might be simpler to understand if instead of passing a custom
parsing function we just:

struct WebViewInfo {
  enum Type {
    kPage,
    kOther
  };
  bool IsFrontend() const;

  std::string id;
  std::string debugger_url;
  std::string url;
  Type type;
};

This separates the parsing aspect from the various analyses we need to do, and
may make the tests a bit simpler.

https://codereview.chromium.org/12321057/diff/11011/chrome/test/chromedriver/...
chrome/test/chromedriver/chrome_impl.cc:99: std::list<std::string> frontend_ids;
 // In-page DevTools frontend.
the naming here is a bit confusing. how about call the first one
'tab_frontend_ids' and the second one 'docked_frontend_ids'? Then drop the
comments

https://codereview.chromium.org/12321057/diff/11011/chrome/test/chromedriver/...
chrome/test/chromedriver/chrome_impl.cc:138: // Ignore Disconnected error,
because the DevTools frontend is closed.
disconnected, D->d

https://codereview.chromium.org/12321057/diff/11011/chrome/test/chromedriver/...
File chrome/test/chromedriver/devtools_client_impl.cc (right):

https://codereview.chromium.org/12321057/diff/11011/chrome/test/chromedriver/...
chrome/test/chromedriver/devtools_client_impl.cc:125: Status
DevToolsClientImpl::SendCommandToDevTools(
the naming of SendCommandToDevTools and SendCommandInternal are a bit arbitrary.
see if you can come up with something that makes more sense.

https://codereview.chromium.org/12321057/diff/11011/chrome/test/chromedriver/...
chrome/test/chromedriver/devtools_client_impl.cc:159: Status status =
SendCommandToDevTools(method, params, result, &command_id);
have you tried this manually? can you try sending a bunch of execute scripts to
a page while you try opening and closing devtools repeatedly and see if you get
any crashes in chrome or errors on the chromedriver side?

https://codereview.chromium.org/12321057/diff/11011/chrome/test/chromedriver/...
chrome/test/chromedriver/devtools_client_impl.cc:167: return
Status(kDisconnected, "failed to reconnect to DevTools");
this error message may not be accurate, right? this may be the first time it
tried to connect and failed.

https://codereview.chromium.org/12321057/diff/11011/chrome/test/chromedriver/...
File chrome/test/chromedriver/net/sync_websocket_impl.cc (right):

https://codereview.chromium.org/12321057/diff/11011/chrome/test/chromedriver/...
chrome/test/chromedriver/net/sync_websocket_impl.cc:40: closed_(true),
if you're going to have closed_ = true on start, you should change the name to
open_ instead, and default it to false. it's not normal to say that a socket is
closed if it has never been opened in the first place

https://codereview.chromium.org/12321057/diff/11011/chrome/test/chromedriver/...
chrome/test/chromedriver/net/sync_websocket_impl.cc:53: closed_ = false;
i think this is a race; the socket could have been closed quickly right before
this, and we'd think it was open.

https://codereview.chromium.org/12321057/diff/11011/chrome/test/chromedriver/...
File chrome/test/chromedriver/net/sync_websocket_impl_unittest.cc (right):

https://codereview.chromium.org/12321057/diff/11011/chrome/test/chromedriver/...
chrome/test/chromedriver/net/sync_websocket_impl_unittest.cc:120:
ASSERT_TRUE(sock.Connect(server_.web_socket_url()));
how about check in this test that the message queue is cleared on reconnect?

https://codereview.chromium.org/12321057/diff/11011/chrome/test/chromedriver/...
File chrome/test/chromedriver/net/test_http_server.cc (right):

https://codereview.chromium.org/12321057/diff/11011/chrome/test/chromedriver/...
chrome/test/chromedriver/net/test_http_server.cc:46: if (!server_)
although net::HttpServer is ThreadSafeRefCounted, the scoped_refptr isn't

[chrisgao (Use stgao instead), 2013-02-27 19:29:44]

ptal

https://codereview.chromium.org/12321057/diff/11011/chrome/test/chromedriver/...
File chrome/test/chromedriver/chrome_impl.cc (right):

https://codereview.chromium.org/12321057/diff/11011/chrome/test/chromedriver/...
chrome/test/chromedriver/chrome_impl.cc:29: const ParserFunc& parser) {
On 2013/02/22 01:35:24, kkania wrote:
> i think this might be simpler to understand if instead of passing a custom
> parsing function we just:
> 
> struct WebViewInfo {
>   enum Type {
>     kPage,
>     kOther
>   };
>   bool IsFrontend() const;
> 
>   std::string id;
>   std::string debugger_url;
>   std::string url;
>   Type type;
> };
> 
> This separates the parsing aspect from the various analyses we need to do, and
> may make the tests a bit simpler.

Done.

https://codereview.chromium.org/12321057/diff/11011/chrome/test/chromedriver/...
chrome/test/chromedriver/chrome_impl.cc:99: std::list<std::string> frontend_ids;
 // In-page DevTools frontend.
On 2013/02/22 01:35:24, kkania wrote:
> the naming here is a bit confusing. how about call the first one
> 'tab_frontend_ids' and the second one 'docked_frontend_ids'? Then drop the
> comments

Good suggestion.

https://codereview.chromium.org/12321057/diff/11011/chrome/test/chromedriver/...
chrome/test/chromedriver/chrome_impl.cc:138: // Ignore Disconnected error,
because the DevTools frontend is closed.
On 2013/02/22 01:35:24, kkania wrote:
> disconnected, D->d

Done.

https://codereview.chromium.org/12321057/diff/11011/chrome/test/chromedriver/...
File chrome/test/chromedriver/devtools_client_impl.cc (right):

https://codereview.chromium.org/12321057/diff/11011/chrome/test/chromedriver/...
chrome/test/chromedriver/devtools_client_impl.cc:125: Status
DevToolsClientImpl::SendCommandToDevTools(
On 2013/02/22 01:35:24, kkania wrote:
> the naming of SendCommandToDevTools and SendCommandInternal are a bit
arbitrary.
> see if you can come up with something that makes more sense.
Change SendCommandInternal to SendCommandWithRetry

https://codereview.chromium.org/12321057/diff/11011/chrome/test/chromedriver/...
chrome/test/chromedriver/devtools_client_impl.cc:159: Status status =
SendCommandToDevTools(method, params, result, &command_id);
On 2013/02/22 01:35:24, kkania wrote:
> have you tried this manually? can you try sending a bunch of execute scripts
to
> a page while you try opening and closing devtools repeatedly and see if you
get
> any crashes in chrome or errors on the chromedriver side?

If DevTools frontend is opened during re-connection (interrupt the four commands
from trackers' OnConnected event), our code will hang.

If DevTools frontend is opened between commands from client side, it is ok
(tested 20 times with FindElement script) expect that the docked DevTools
frontend UI becomes larger and larger.

https://codereview.chromium.org/12321057/diff/11011/chrome/test/chromedriver/...
chrome/test/chromedriver/devtools_client_impl.cc:167: return
Status(kDisconnected, "failed to reconnect to DevTools");
On 2013/02/22 01:35:24, kkania wrote:
> this error message may not be accurate, right? this may be the first time it
> tried to connect and failed.

Oh, yes. Updated message and including the old status.

https://codereview.chromium.org/12321057/diff/11011/chrome/test/chromedriver/...
File chrome/test/chromedriver/net/sync_websocket_impl.cc (right):

https://codereview.chromium.org/12321057/diff/11011/chrome/test/chromedriver/...
chrome/test/chromedriver/net/sync_websocket_impl.cc:40: closed_(true),
On 2013/02/22 01:35:24, kkania wrote:
> if you're going to have closed_ = true on start, you should change the name to
> open_ instead, and default it to false. it's not normal to say that a socket
is
> closed if it has never been opened in the first place

Done.

https://codereview.chromium.org/12321057/diff/11011/chrome/test/chromedriver/...
chrome/test/chromedriver/net/sync_websocket_impl.cc:53: closed_ = false;
On 2013/02/22 01:35:24, kkania wrote:
> i think this is a race; the socket could have been closed quickly right before
> this, and we'd think it was open.

As Core::OnConnectCompletedOnIO and Core::OnClose can be called asynchronously,
added an enum for state: disconnected, connecting, and connected.

https://codereview.chromium.org/12321057/diff/11011/chrome/test/chromedriver/...
File chrome/test/chromedriver/net/sync_websocket_impl_unittest.cc (right):

https://codereview.chromium.org/12321057/diff/11011/chrome/test/chromedriver/...
chrome/test/chromedriver/net/sync_websocket_impl_unittest.cc:120:
ASSERT_TRUE(sock.Connect(server_.web_socket_url()));
On 2013/02/22 01:35:24, kkania wrote:
> how about check in this test that the message queue is cleared on reconnect?

Done.

https://codereview.chromium.org/12321057/diff/11011/chrome/test/chromedriver/...
File chrome/test/chromedriver/net/test_http_server.cc (right):

https://codereview.chromium.org/12321057/diff/11011/chrome/test/chromedriver/...
chrome/test/chromedriver/net/test_http_server.cc:46: if (!server_)
On 2013/02/22 01:35:24, kkania wrote:
> although net::HttpServer is ThreadSafeRefCounted, the scoped_refptr isn't

Good catch.

The check is to avoid calling thread_.message_loop_proxy()->PostTask after the
thread is stopped.
Instead, check whether the thread is still running or not.

[kkania, 2013-03-04 23:33:57]

change name of EnsureAlive too

https://codereview.chromium.org/12321057/diff/15041/chrome/test/chromedriver/...
File chrome/test/chromedriver/chrome_impl.cc (right):

https://codereview.chromium.org/12321057/diff/15041/chrome/test/chromedriver/...
chrome/test/chromedriver/chrome_impl.cc:29: Status
FetchPagesInfo(URLRequestContextGetter* context_getter,
this naming is no longer consistent since we've changed the page_ids argument to
web_view_info_list. Either change the name of the function, or the name of the
third param (and its type too)

https://codereview.chromium.org/12321057/diff/15041/chrome/test/chromedriver/...
chrome/test/chromedriver/chrome_impl.cc:40: bool WebViewExist(const std::string&
web_view_id,
convention is to name this as DoesWebViewExist

https://codereview.chromium.org/12321057/diff/15041/chrome/test/chromedriver/...
File chrome/test/chromedriver/devtools_client_impl.cc (right):

https://codereview.chromium.org/12321057/diff/15041/chrome/test/chromedriver/...
chrome/test/chromedriver/devtools_client_impl.cc:86: Status status =
frontend_closer_func_.Run();
do we want to try to connect before closing all the frontends?

https://codereview.chromium.org/12321057/diff/15041/chrome/test/chromedriver/...
File chrome/test/chromedriver/net/sync_websocket_impl.cc (right):

https://codereview.chromium.org/12321057/diff/15041/chrome/test/chromedriver/...
chrome/test/chromedriver/net/sync_websocket_impl.cc:107: base::AutoLock
lock(lock_);
unlock after the queue is cleared, in case WebSocket::Connect calls the callback
synchronously

https://codereview.chromium.org/12321057/diff/15041/chrome/test/chromedriver/...
File chrome/test/chromedriver/net/sync_websocket_impl.h (right):

https://codereview.chromium.org/12321057/diff/15041/chrome/test/chromedriver/...
chrome/test/chromedriver/net/sync_websocket_impl.h:93: bool open_;
how about change this to is_connected_ to be consistent with IsConnected?

[chrisgao (Use stgao instead), 2013-03-05 06:58:17]

comments are addressed.
ptal

https://codereview.chromium.org/12321057/diff/15041/chrome/test/chromedriver/...
File chrome/test/chromedriver/chrome_impl.cc (right):

https://codereview.chromium.org/12321057/diff/15041/chrome/test/chromedriver/...
chrome/test/chromedriver/chrome_impl.cc:29: Status
FetchPagesInfo(URLRequestContextGetter* context_getter,
On 2013/03/04 23:33:57, kkania wrote:
> this naming is no longer consistent since we've changed the page_ids argument
to
> web_view_info_list. Either change the name of the function, or the name of the
> third param (and its type too)

Done. Changed name of function.

https://codereview.chromium.org/12321057/diff/15041/chrome/test/chromedriver/...
chrome/test/chromedriver/chrome_impl.cc:40: bool WebViewExist(const std::string&
web_view_id,
On 2013/03/04 23:33:57, kkania wrote:
> convention is to name this as DoesWebViewExist

Done.

https://codereview.chromium.org/12321057/diff/15041/chrome/test/chromedriver/...
File chrome/test/chromedriver/devtools_client_impl.cc (right):

https://codereview.chromium.org/12321057/diff/15041/chrome/test/chromedriver/...
chrome/test/chromedriver/devtools_client_impl.cc:86: Status status =
frontend_closer_func_.Run();
On 2013/03/04 23:33:57, kkania wrote:
> do we want to try to connect before closing all the frontends?

Done.

https://codereview.chromium.org/12321057/diff/15041/chrome/test/chromedriver/...
File chrome/test/chromedriver/net/sync_websocket_impl.cc (right):

https://codereview.chromium.org/12321057/diff/15041/chrome/test/chromedriver/...
chrome/test/chromedriver/net/sync_websocket_impl.cc:107: base::AutoLock
lock(lock_);
On 2013/03/04 23:33:57, kkania wrote:
> unlock after the queue is cleared, in case WebSocket::Connect calls the
callback
> synchronously

Done.

https://codereview.chromium.org/12321057/diff/15041/chrome/test/chromedriver/...
File chrome/test/chromedriver/net/sync_websocket_impl.h (right):

https://codereview.chromium.org/12321057/diff/15041/chrome/test/chromedriver/...
chrome/test/chromedriver/net/sync_websocket_impl.h:93: bool open_;
On 2013/03/04 23:33:57, kkania wrote:
> how about change this to is_connected_ to be consistent with IsConnected?

Done.

[chrisgao (Use stgao instead), 2013-03-05 07:00:12]

https://codereview.chromium.org/12321057/diff/26001/chrome/test/chromedriver/...
File chrome/test/chromedriver/run_py_tests.py (right):

https://codereview.chromium.org/12321057/diff/26001/chrome/test/chromedriver/...
chrome/test/chromedriver/run_py_tests.py:188: import pdb; pdb.set_trace()
To be removed in next patch.

[kkania, 2013-03-05 17:01:13]

lgtm

https://codereview.chromium.org/12321057/diff/26001/chrome/test/chromedriver/...
File chrome/test/chromedriver/chrome_impl.cc (right):

https://codereview.chromium.org/12321057/diff/26001/chrome/test/chromedriver/...
chrome/test/chromedriver/chrome_impl.cc:51: return ParsePagesInfo(data,
web_view_info_list);
how does this know which function without the internal:: qualifier?

https://codereview.chromium.org/12321057/diff/26001/chrome/test/chromedriver/...
chrome/test/chromedriver/chrome_impl.cc:51: return ParsePagesInfo(data,
web_view_info_list);
adjust the naming here too

https://codereview.chromium.org/12321057/diff/26001/chrome/test/chromedriver/...
chrome/test/chromedriver/chrome_impl.cc:54: bool DoesWebViewExist(const
std::string& web_view_id,
You could change this to:
WebViewInfo* GetWebViewInfoFromList(const std::string& id, const
WebViewInfoList& info_list)
and have it return NULL if the view does not exist. Then you could use it to
check for existence or to get the info

https://codereview.chromium.org/12321057/diff/26001/chrome/test/chromedriver/...
chrome/test/chromedriver/chrome_impl.cc:55: const WebViewInfoList& info_list) {
indent

https://codereview.chromium.org/12321057/diff/26001/chrome/test/chromedriver/...
File chrome/test/chromedriver/devtools_client_impl.cc (right):

https://codereview.chromium.org/12321057/diff/26001/chrome/test/chromedriver/...
chrome/test/chromedriver/devtools_client_impl.cc:145: return
Status(kDisconnected, "unable to connect to renderer");
fix the error message; the error isn't that it was unable to connect; the error
is that it isn't connected

[chrisgao (Use stgao instead), 2013-03-06 01:19:54]

https://codereview.chromium.org/12321057/diff/26001/chrome/test/chromedriver/...
File chrome/test/chromedriver/chrome_impl.cc (right):

https://codereview.chromium.org/12321057/diff/26001/chrome/test/chromedriver/...
chrome/test/chromedriver/chrome_impl.cc:51: return ParsePagesInfo(data,
web_view_info_list);
On 2013/03/05 17:01:13, kkania wrote:
> adjust the naming here too

Done.

https://codereview.chromium.org/12321057/diff/26001/chrome/test/chromedriver/...
chrome/test/chromedriver/chrome_impl.cc:51: return ParsePagesInfo(data,
web_view_info_list);
On 2013/03/05 17:01:13, kkania wrote:
> how does this know which function without the internal:: qualifier?

whooops...the compiler even didn't raise an error...
Done.

https://codereview.chromium.org/12321057/diff/26001/chrome/test/chromedriver/...
chrome/test/chromedriver/chrome_impl.cc:54: bool DoesWebViewExist(const
std::string& web_view_id,
On 2013/03/05 17:01:13, kkania wrote:
> You could change this to:
> WebViewInfo* GetWebViewInfoFromList(const std::string& id, const
> WebViewInfoList& info_list)
> and have it return NULL if the view does not exist. Then you could use it to
> check for existence or to get the info

Done.

https://codereview.chromium.org/12321057/diff/26001/chrome/test/chromedriver/...
chrome/test/chromedriver/chrome_impl.cc:55: const WebViewInfoList& info_list) {
On 2013/03/05 17:01:13, kkania wrote:
> indent

Done.

https://codereview.chromium.org/12321057/diff/26001/chrome/test/chromedriver/...
File chrome/test/chromedriver/devtools_client_impl.cc (right):

https://codereview.chromium.org/12321057/diff/26001/chrome/test/chromedriver/...
chrome/test/chromedriver/devtools_client_impl.cc:145: return
Status(kDisconnected, "unable to connect to renderer");
On 2013/03/05 17:01:13, kkania wrote:
> fix the error message; the error isn't that it was unable to connect; the
error
> is that it isn't connected

Done.

https://codereview.chromium.org/12321057/diff/26001/chrome/test/chromedriver/...
File chrome/test/chromedriver/run_py_tests.py (right):

https://codereview.chromium.org/12321057/diff/26001/chrome/test/chromedriver/...
chrome/test/chromedriver/run_py_tests.py:188: import pdb; pdb.set_trace()
On 2013/03/05 07:00:12, Shuotao Gao wrote:
> To be removed in next patch.

Done.

[commit-bot: I haz the power, 2013-03-06 17:02:25]

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

[commit-bot: I haz the power, 2013-03-06 17:08:53]

Change committed as 186452