[chromedriver] Send all OnConnected before any OnEvent when connected.

[chromedriver] Send all OnConnected before any OnEvent when connected.

It is to make sure that all listeners (dom trackers)  could clear their internal state before receiving events which might update the state.

NOTRY=true

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

[chrisgao (Use stgao instead), 2013-02-13 19:04:17]

ptal

[craigdh, 2013-02-13 19:19:39]

https://chromiumcodereview.appspot.com/12230021/diff/1/chrome/test/chromedriv...
File chrome/test/chromedriver/devtools_client_impl.cc (right):

https://chromiumcodereview.appspot.com/12230021/diff/1/chrome/test/chromedriv...
chrome/test/chromedriver/devtools_client_impl.cc:131:
listeners_for_on_connected_ = listeners_;
Instead of copying the list, why not just have a boolean flag indicating that
OnConnected has been triggered and needs to be sent to all listeners?

https://chromiumcodereview.appspot.com/12230021/diff/1/chrome/test/chromedriv...
chrome/test/chromedriver/devtools_client_impl.cc:179: if
(cmd_response_map_.find(expected_id) != cmd_response_map_.end()
Consider making this a private function, something like
HasReceivedCommandResponse(int cmd_id). It'd be a little more clear what you're
checking for and make the code more readable.

[kkania, 2013-02-13 20:57:34]

let's see some unit tests too

https://chromiumcodereview.appspot.com/12230021/diff/1/chrome/test/chromedriv...
File chrome/test/chromedriver/devtools_client_impl.cc (right):

https://chromiumcodereview.appspot.com/12230021/diff/1/chrome/test/chromedriv...
chrome/test/chromedriver/devtools_client_impl.cc:131:
listeners_for_on_connected_ = listeners_;
On 2013/02/13 19:19:39, craigdh wrote:
> Instead of copying the list, why not just have a boolean flag indicating that
> OnConnected has been triggered and needs to be sent to all listeners?

then you'd also have to keep track of what listeners you've already notified,
prolly via a int; i think list copy might be simpler

[craigdh, 2013-02-13 21:45:45]

https://chromiumcodereview.appspot.com/12230021/diff/1/chrome/test/chromedriv...
File chrome/test/chromedriver/devtools_client_impl.cc (right):

https://chromiumcodereview.appspot.com/12230021/diff/1/chrome/test/chromedriv...
chrome/test/chromedriver/devtools_client_impl.cc:131:
listeners_for_on_connected_ = listeners_;
On 2013/02/13 20:57:34, kkania wrote:
> On 2013/02/13 19:19:39, craigdh wrote:
> > Instead of copying the list, why not just have a boolean flag indicating
that
> > OnConnected has been triggered and needs to be sent to all listeners?
> 
> then you'd also have to keep track of what listeners you've already notified,
> prolly via a int; i think list copy might be simpler

Ah, I see the issue. Agreed.

[chrisgao (Use stgao instead), 2013-02-14 03:49:45]

unittest is added.
ptal

https://chromiumcodereview.appspot.com/12230021/diff/1/chrome/test/chromedriv...
File chrome/test/chromedriver/devtools_client_impl.cc (right):

https://chromiumcodereview.appspot.com/12230021/diff/1/chrome/test/chromedriv...
chrome/test/chromedriver/devtools_client_impl.cc:131:
listeners_for_on_connected_ = listeners_;
On 2013/02/13 21:45:45, craigdh wrote:
> On 2013/02/13 20:57:34, kkania wrote:
> > On 2013/02/13 19:19:39, craigdh wrote:
> > > Instead of copying the list, why not just have a boolean flag indicating
> that
> > > OnConnected has been triggered and needs to be sent to all listeners?
> > 
> > then you'd also have to keep track of what listeners you've already
notified,
> > prolly via a int; i think list copy might be simpler
> 
> Ah, I see the issue. Agreed.

I also think list copy is easier and simpler.

https://chromiumcodereview.appspot.com/12230021/diff/1/chrome/test/chromedriv...
chrome/test/chromedriver/devtools_client_impl.cc:179: if
(cmd_response_map_.find(expected_id) != cmd_response_map_.end()
On 2013/02/13 19:19:39, craigdh wrote:
> Consider making this a private function, something like
> HasReceivedCommandResponse(int cmd_id). It'd be a little more clear what
you're
> checking for and make the code more readable.

Good idea.

[chrisgao (Use stgao instead), 2013-02-14 03:50:46]

ptal

[kkania, 2013-02-14 17:04:59]

lgtm

[craigdh, 2013-02-14 17:51:55]

lgtm.

[commit-bot: I haz the power, 2013-02-14 18:44:07]

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

[commit-bot: I haz the power, 2013-02-14 18:47:08]

Change committed as 182501