[ChromeDriver] Send DOM.getDocument after each DOM.documentUpdated.

[ChromeDriver] Send DOM.getDocument after each DOM.documentUpdated.

This involved adding support for outstanding commands to return in any order.

BUG=172944
TEST=unittests


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

[craigdh, 2013-01-30 01:44:24]

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

https://codereview.chromium.org/12093057/diff/1/chrome/test/chromedriver/devt...
chrome/test/chromedriver/devtools_client_impl.cc:115: LOG(INFO) << "SEND: " <<
message;
Whoops. Will delete in the next patch.

https://codereview.chromium.org/12093057/diff/1/chrome/test/chromedriver/devt...
chrome/test/chromedriver/devtools_client_impl.cc:164: LOG(INFO) << "RECV: " <<
message;
ditto.

[kkania, 2013-01-30 02:17:55]

it looks like you forgot to add your new files too

[craigdh, 2013-01-30 17:36:55]

On 2013/01/30 02:17:55, kkania wrote:
> it looks like you forgot to add your new files too

Yep, I was in a hurry to catch my bus. Here they are.

[kkania, 2013-01-30 18:50:29]

https://codereview.chromium.org/12093057/diff/5001/chrome/test/chromedriver/d...
File chrome/test/chromedriver/devtools_state.cc (right):

https://codereview.chromium.org/12093057/diff/5001/chrome/test/chromedriver/d...
chrome/test/chromedriver/devtools_state.cc:29: status =
client_->SendCommand("Page.enable", params);
So actually, Page.enable and Runtime.enable only need to be done once per
devtools connection. So, we actually don't need to put these here.

I'm not sure it's best to put the DOM.getDocument in here, since it seems like
something the dom tracker should be responsible for.

https://codereview.chromium.org/12093057/diff/5001/chrome/test/chromedriver/d...
File chrome/test/chromedriver/devtools_state_unittest.cc (right):

https://codereview.chromium.org/12093057/diff/5001/chrome/test/chromedriver/d...
chrome/test/chromedriver/devtools_state_unittest.cc:46: private:
bad indent

https://codereview.chromium.org/12093057/diff/5001/chrome/test/chromedriver/d...
chrome/test/chromedriver/devtools_state_unittest.cc:57:
ASSERT_EQ(client.GetSendCount(), 3);
this is brittle and I'm not sure is very helpful. I think you should remove this
test or save the outgoing commands and make sure they are correct

[kkania, 2013-01-30 20:33:24]

https://codereview.chromium.org/12093057/diff/5001/chrome/test/chromedriver/d...
File chrome/test/chromedriver/devtools_state.cc (right):

https://codereview.chromium.org/12093057/diff/5001/chrome/test/chromedriver/d...
chrome/test/chromedriver/devtools_state.cc:29: status =
client_->SendCommand("Page.enable", params);
On 2013/01/30 18:50:29, kkania wrote:
> So actually, Page.enable and Runtime.enable only need to be done once per
> devtools connection. So, we actually don't need to put these here.
> 
> I'm not sure it's best to put the DOM.getDocument in here, since it seems like
> something the dom tracker should be responsible for.

Wait nevermind, this is only done once per connection. So this is ok. However, I
still think it makes more sense to do the DOM.getDocument as part of the DOM
tracker...

[craigdh, 2013-01-30 21:35:08]

https://codereview.chromium.org/12093057/diff/5001/chrome/test/chromedriver/d...
File chrome/test/chromedriver/devtools_state.cc (right):

https://codereview.chromium.org/12093057/diff/5001/chrome/test/chromedriver/d...
chrome/test/chromedriver/devtools_state.cc:29: status =
client_->SendCommand("Page.enable", params);
On 2013/01/30 20:33:24, kkania wrote:
> On 2013/01/30 18:50:29, kkania wrote:
> > So actually, Page.enable and Runtime.enable only need to be done once per
> > devtools connection. So, we actually don't need to put these here.
> > 
> > I'm not sure it's best to put the DOM.getDocument in here, since it seems
like
> > something the dom tracker should be responsible for.
> 
> Wait nevermind, this is only done once per connection. So this is ok. However,
I
> still think it makes more sense to do the DOM.getDocument as part of the DOM
> tracker...

Ok, I'll just fold this class's functionality into DomTracker.

[craigdh, 2013-01-30 22:20:04]

ptal.

https://codereview.chromium.org/12093057/diff/5001/chrome/test/chromedriver/d...
File chrome/test/chromedriver/devtools_state_unittest.cc (right):

https://codereview.chromium.org/12093057/diff/5001/chrome/test/chromedriver/d...
chrome/test/chromedriver/devtools_state_unittest.cc:46: private:
On 2013/01/30 18:50:29, kkania wrote:
> bad indent

Done.

https://codereview.chromium.org/12093057/diff/5001/chrome/test/chromedriver/d...
chrome/test/chromedriver/devtools_state_unittest.cc:57:
ASSERT_EQ(client.GetSendCount(), 3);
On 2013/01/30 18:50:29, kkania wrote:
> this is brittle and I'm not sure is very helpful. I think you should remove
this
> test or save the outgoing commands and make sure they are correct

Done.

[kkania, 2013-01-30 22:36:05]

https://codereview.chromium.org/12093057/diff/7/chrome/test/chromedriver/devt...
File chrome/test/chromedriver/devtools_client_impl.cc (right):

https://codereview.chromium.org/12093057/diff/7/chrome/test/chromedriver/devt...
chrome/test/chromedriver/devtools_client_impl.cc:129:
cmd_response_map_[command_id] = NULL;
what's this about?

https://codereview.chromium.org/12093057/diff/7/chrome/test/chromedriver/dom_...
File chrome/test/chromedriver/dom_tracker.cc (right):

https://codereview.chromium.org/12093057/diff/7/chrome/test/chromedriver/dom_...
chrome/test/chromedriver/dom_tracker.cc:15: : client_(client) {
indent

https://codereview.chromium.org/12093057/diff/7/chrome/test/chromedriver/dom_...
chrome/test/chromedriver/dom_tracker.cc:16: DCHECK(client_);
indent

https://codereview.chromium.org/12093057/diff/7/chrome/test/chromedriver/dom_...
chrome/test/chromedriver/dom_tracker.cc:30: status =
client_->SendCommand("Page.enable", params);
oops, i didn't mean to merge Page.enable and Runtime.enable here. they could go
in DevToolsClientImpl, or in navigation_tracker and frame_tracker
respectively...

[craigdh, 2013-01-30 23:05:21]

https://codereview.chromium.org/12093057/diff/7/chrome/test/chromedriver/devt...
File chrome/test/chromedriver/devtools_client_impl.cc (right):

https://codereview.chromium.org/12093057/diff/7/chrome/test/chromedriver/devt...
chrome/test/chromedriver/devtools_client_impl.cc:129:
cmd_response_map_[command_id] = NULL;
On 2013/01/30 22:36:05, kkania wrote:
> what's this about?

The overarching idea is that we support overlapping/nested commands. The logic
goes like this:

Before waiting for a response the map[command_id] is set to NULL. Then we
receive messages until the map[command_id] contains a response.

If we receive a response with a command_id not in the map then we have received
a response for an unknown command ID and it's an error. Otherwise store the
result in the map.

https://codereview.chromium.org/12093057/diff/7/chrome/test/chromedriver/dom_...
File chrome/test/chromedriver/dom_tracker.cc (right):

https://codereview.chromium.org/12093057/diff/7/chrome/test/chromedriver/dom_...
chrome/test/chromedriver/dom_tracker.cc:30: status =
client_->SendCommand("Page.enable", params);
On 2013/01/30 22:36:05, kkania wrote:
> oops, i didn't mean to merge Page.enable and Runtime.enable here. they could
go
> in DevToolsClientImpl, or in navigation_tracker and frame_tracker
> respectively...

Done.

[kkania, 2013-01-30 23:33:56]

i see

https://codereview.chromium.org/12093057/diff/4002/chrome/test/chromedriver/d...
File chrome/test/chromedriver/devtools_client_impl.cc (right):

https://codereview.chromium.org/12093057/diff/4002/chrome/test/chromedriver/d...
chrome/test/chromedriver/devtools_client_impl.cc:50: LOG(WARNING) << "Finished
with no response for command " << iter->first;
logging.h

https://codereview.chromium.org/12093057/diff/4002/chrome/test/chromedriver/d...
chrome/test/chromedriver/devtools_client_impl.cc:129:
cmd_response_map_[command_id] = NULL;
can we get some tests for the overlapping and nested cases

[craigdh, 2013-01-31 18:37:20]

On 2013/01/30 23:33:56, kkania wrote:
> i see
> 
>
https://codereview.chromium.org/12093057/diff/4002/chrome/test/chromedriver/d...
> File chrome/test/chromedriver/devtools_client_impl.cc (right):
> 
>
https://codereview.chromium.org/12093057/diff/4002/chrome/test/chromedriver/d...
> chrome/test/chromedriver/devtools_client_impl.cc:50: LOG(WARNING) << "Finished
> with no response for command " << iter->first;
> logging.h
> 
>
https://codereview.chromium.org/12093057/diff/4002/chrome/test/chromedriver/d...
> chrome/test/chromedriver/devtools_client_impl.cc:129:
> cmd_response_map_[command_id] = NULL;
> can we get some tests for the overlapping and nested cases

Added a unittest for the overlapping/nested case.

[kkania, 2013-01-31 19:15:11]

lgtm

[commit-bot: I haz the power, 2013-01-31 21:44:26]

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

[commit-bot: I haz the power, 2013-02-01 00:43:15]

Change committed as 180022