[chromedriver] Implement connecting to devtools and loading a page.

[chromedriver] Implement connecting to devtools and loading a page.
BUG=none


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

[kkania, 2012-11-29 21:22:04]

This isn't the full implementation of navigate of course, since it doesn't obey
timeouts or wait for the load. Also, we just hardcode a port for the remote
debugger. We'll want to change that to use a random open port, and possibly to
let devtools pick the random open port.

This patch is based on https://chromiumcodereview.appspot.com/11316115/, which
is not yet committed.

[kkania, 2012-11-30 17:26:43]

On 2012/11/29 21:22:04, kkania wrote:
> This isn't the full implementation of navigate of course, since it doesn't
obey
> timeouts or wait for the load. Also, we just hardcode a port for the remote
> debugger. We'll want to change that to use a random open port, and possibly to
> let devtools pick the random open port.
> 
> This patch is based on https://chromiumcodereview.appspot.com/11316115/, which
> is not yet committed.

Ping. The websocket patch I mentioned above has been successfully landed.

[klundberg, 2012-11-30 20:58:06]

LGTM but I would really like someone more familiar with C++ to take a look.

I add Craig as reviewer in case he has time to look at this.

https://codereview.chromium.org/11415205/diff/13037/chrome/test/chromedriver/...
File chrome/test/chromedriver/net/net_util_unittest.cc (right):

https://codereview.chromium.org/11415205/diff/13037/chrome/test/chromedriver/...
chrome/test/chromedriver/net/net_util_unittest.cc:62: server_url_ =
GURL(base::StringPrintf("http://127.0.0.1:%d", address.port()));
Seems longer than 80 characters.

[craigdh, 2012-11-30 22:02:33]

On 2012/11/30 20:58:06, klundberg wrote:
> LGTM but I would really like someone more familiar with C++ to take a look.
> 
> I add Craig as reviewer in case he has time to look at this.
> 
>
https://codereview.chromium.org/11415205/diff/13037/chrome/test/chromedriver/...
> File chrome/test/chromedriver/net/net_util_unittest.cc (right):
> 
>
https://codereview.chromium.org/11415205/diff/13037/chrome/test/chromedriver/...
> chrome/test/chromedriver/net/net_util_unittest.cc:62: server_url_ =
> GURL(base::StringPrintf("http://127.0.0.1:%d", address.port()));
> Seems longer than 80 characters.

I won't have time today but I can review it Monday.

[chrisgao (Use stgao instead), 2012-12-01 08:37:19]

lgtm

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

https://codereview.chromium.org/11415205/diff/13037/chrome/test/chromedriver/...
chrome/test/chromedriver/chrome_impl.cc:9: #include "base/logging.h"
Remove logging?

https://codereview.chromium.org/11415205/diff/13037/chrome/test/chromedriver/...
File chrome/test/chromedriver/chrome_impl_unittest.cc (right):

https://codereview.chromium.org/11415205/diff/13037/chrome/test/chromedriver/...
chrome/test/chromedriver/chrome_impl_unittest.cc:31:
ASSERT_EQ("http://debugurl2", *(++urls.begin()));
How about using urls.back() instead?

https://codereview.chromium.org/11415205/diff/13037/chrome/test/chromedriver/...
File chrome/test/chromedriver/chromedriver.cc (right):

https://codereview.chromium.org/11415205/diff/13037/chrome/test/chromedriver/...
chrome/test/chromedriver/chromedriver.cc:12: #include "base/message_loop.h"
MessageLoop seems unused. The same for "base/threading/thread.h" below.

https://codereview.chromium.org/11415205/diff/13037/chrome/test/chromedriver/...
File chrome/test/chromedriver/command_executor_impl.h (right):

https://codereview.chromium.org/11415205/diff/13037/chrome/test/chromedriver/...
chrome/test/chromedriver/command_executor_impl.h:35: virtual void Init()
OVERRIDE;
Also overridden.
Move to after comment "// Overridden from CommandExecutor:".

https://codereview.chromium.org/11415205/diff/13037/chrome/test/chromedriver/...
File chrome/test/chromedriver/status.cc (right):

https://codereview.chromium.org/11415205/diff/13037/chrome/test/chromedriver/...
chrome/test/chromedriver/status.cc:9: #include "base/string_split.h"
It seems unused.

[craigdh, 2012-12-04 01:41:27]

lgtm with some questions

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

https://codereview.chromium.org/11415205/diff/13037/chrome/test/chromedriver/...
chrome/test/chromedriver/chrome_impl.cc:53: base::Time deadline =
base::Time::Now() + base::TimeDelta::FromSeconds(20);
It might be nice if the timeout was configurable.

https://codereview.chromium.org/11415205/diff/13037/chrome/test/chromedriver/...
File chrome/test/chromedriver/chrome_launcher_impl.cc (right):

https://codereview.chromium.org/11415205/diff/13037/chrome/test/chromedriver/...
chrome/test/chromedriver/chrome_launcher_impl.cc:38:
command.AppendSwitch("disable-extensions");
In Chrome OS we will certainly need to have extensions enabled as we intend to
use ChromeDriver to test bundled extensions. Of course, ChromeOS will also need
to launch Chrome indirectly though the session manager, but I suspect this could
also be a use case for desktop Chrome (the extensions often work on both).

https://codereview.chromium.org/11415205/diff/13037/chrome/test/chromedriver/...
File chrome/test/chromedriver/net/net_util.cc (right):

https://codereview.chromium.org/11415205/diff/13037/chrome/test/chromedriver/...
chrome/test/chromedriver/net/net_util.cc:35: success_ &=
source->GetResponseAsString(response_);
Why a bit operator here? This is a logical operation.

[commit-bot: I haz the power, 2012-12-04 17:09:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kkania@chromium.org/11415205/14056

[commit-bot: I haz the power, 2012-12-04 20:14:31]

Change committed as 171035

[kkania, 2013-02-28 21:51:58]

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

https://codereview.chromium.org/11415205/diff/13037/chrome/test/chromedriver/...
chrome/test/chromedriver/chrome_impl.cc:9: #include "base/logging.h"
On 2012/12/01 08:37:19, Shuotao Gao wrote:
> Remove logging?

I need this for the CHECK macro.

https://codereview.chromium.org/11415205/diff/13037/chrome/test/chromedriver/...
chrome/test/chromedriver/chrome_impl.cc:53: base::Time deadline =
base::Time::Now() + base::TimeDelta::FromSeconds(20);
On 2012/12/04 01:41:27, craigdh wrote:
> It might be nice if the timeout was configurable.

Yes, although I'd like to leave that till later.

https://codereview.chromium.org/11415205/diff/13037/chrome/test/chromedriver/...
File chrome/test/chromedriver/chrome_impl_unittest.cc (right):

https://codereview.chromium.org/11415205/diff/13037/chrome/test/chromedriver/...
chrome/test/chromedriver/chrome_impl_unittest.cc:31:
ASSERT_EQ("http://debugurl2", *(++urls.begin()));
On 2012/12/01 08:37:19, Shuotao Gao wrote:
> How about using urls.back() instead?

Done.

https://codereview.chromium.org/11415205/diff/13037/chrome/test/chromedriver/...
File chrome/test/chromedriver/chrome_launcher_impl.cc (right):

https://codereview.chromium.org/11415205/diff/13037/chrome/test/chromedriver/...
chrome/test/chromedriver/chrome_launcher_impl.cc:38:
command.AppendSwitch("disable-extensions");
On 2012/12/04 01:41:27, craigdh wrote:
> In Chrome OS we will certainly need to have extensions enabled as we intend to
> use ChromeDriver to test bundled extensions. Of course, ChromeOS will also
need
> to launch Chrome indirectly though the session manager, but I suspect this
could
> also be a use case for desktop Chrome (the extensions often work on both).

I removed this flag. I accidentally put it in there because I was tired of
external extensions messing things up while I was experimenting locally.

https://codereview.chromium.org/11415205/diff/13037/chrome/test/chromedriver/...
File chrome/test/chromedriver/chromedriver.cc (right):

https://codereview.chromium.org/11415205/diff/13037/chrome/test/chromedriver/...
chrome/test/chromedriver/chromedriver.cc:12: #include "base/message_loop.h"
On 2012/12/01 08:37:19, Shuotao Gao wrote:
> MessageLoop seems unused. The same for "base/threading/thread.h" below.

Done.

https://codereview.chromium.org/11415205/diff/13037/chrome/test/chromedriver/...
File chrome/test/chromedriver/command_executor_impl.h (right):

https://codereview.chromium.org/11415205/diff/13037/chrome/test/chromedriver/...
chrome/test/chromedriver/command_executor_impl.h:35: virtual void Init()
OVERRIDE;
On 2012/12/01 08:37:19, Shuotao Gao wrote:
> Also overridden.
> Move to after comment "// Overridden from CommandExecutor:".

Done.

https://codereview.chromium.org/11415205/diff/13037/chrome/test/chromedriver/...
File chrome/test/chromedriver/net/net_util.cc (right):

https://codereview.chromium.org/11415205/diff/13037/chrome/test/chromedriver/...
chrome/test/chromedriver/net/net_util.cc:35: success_ &=
source->GetResponseAsString(response_);
On 2012/12/04 01:41:27, craigdh wrote:
> Why a bit operator here? This is a logical operation.

Whoops, I don't know what I was thinking.

https://codereview.chromium.org/11415205/diff/13037/chrome/test/chromedriver/...
File chrome/test/chromedriver/net/net_util_unittest.cc (right):

https://codereview.chromium.org/11415205/diff/13037/chrome/test/chromedriver/...
chrome/test/chromedriver/net/net_util_unittest.cc:62: server_url_ =
GURL(base::StringPrintf("http://127.0.0.1:%d", address.port()));
On 2012/11/30 20:58:06, klundberg wrote:
> Seems longer than 80 characters.

Done.

https://codereview.chromium.org/11415205/diff/13037/chrome/test/chromedriver/...
File chrome/test/chromedriver/status.cc (right):

https://codereview.chromium.org/11415205/diff/13037/chrome/test/chromedriver/...
chrome/test/chromedriver/status.cc:9: #include "base/string_split.h"
On 2012/12/01 08:37:19, Shuotao Gao wrote:
> It seems unused.

Done.