Made sure NativeMessagingHostTest.All can handle out of order responses.

Made sure NativeMessagingHostTest.All can handle out of order responses.

This CL tags every request NativeMessagingHostTest.All sends and process the responses out of order since the native messaging host does not guarantee the order. It also removes the call log because of the same reason - the order is not reliable unless the client wait for the resonse before sending another request.

Also with this CL NativeMessagingHost delays shutdown until all replies have been sent.

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

[alexeypa (please no reviews), 2013-09-05 21:39:52]

PTAL.

[Lambros, 2013-09-06 03:03:12]

lgtm with comments

https://codereview.chromium.org/23461030/diff/11001/remoting/host/setup/nativ...
File remoting/host/setup/native_messaging_host.cc (right):

https://codereview.chromium.org/23461030/diff/11001/remoting/host/setup/nativ...
remoting/host/setup/native_messaging_host.cc:138: pending_requests_ += 1;
pending_requests++ ?

https://codereview.chromium.org/23461030/diff/11001/remoting/host/setup/nativ...
remoting/host/setup/native_messaging_host.cc:177: pending_requests_ -= 1;
pending_requests_-- ?

https://codereview.chromium.org/23461030/diff/11001/remoting/host/setup/nativ...
remoting/host/setup/native_messaging_host.cc:420: pending_requests_ -= 1;
pending_requests_-- ?

https://codereview.chromium.org/23461030/diff/11001/remoting/host/setup/nativ...
File remoting/host/setup/native_messaging_host_unittest.cc (right):

https://codereview.chromium.org/23461030/diff/11001/remoting/host/setup/nativ...
remoting/host/setup/native_messaging_host_unittest.cc:6: 
#include "base/basictypes.h" for arraysize().

https://codereview.chromium.org/23461030/diff/11001/remoting/host/setup/nativ...
remoting/host/setup/native_messaging_host_unittest.cc:30: void
VerifyHelloResponse(scoped_ptr<base::DictionaryValue> response) {
Changing to scoped_ptr means that these functions will delete the object passed
in - is that necessary?

https://codereview.chromium.org/23461030/diff/11001/remoting/host/setup/nativ...
remoting/host/setup/native_messaging_host_unittest.cc:354:
message.SetInteger("id", next_id++);
Optional: Maybe have WriteMessageWithIdToInputPipe() which automatically adds
the current id (protected member variable) and increments it? Or just fold it
into WriteMessageToInputPipe() ? I'd be happy with removing the test that sends
a message without an id, it doesn't seem important.

https://codereview.chromium.org/23461030/diff/11001/remoting/host/setup/nativ...
remoting/host/setup/native_messaging_host_unittest.cc:430: // Call the
verification routine corresponding to the message id.
ASSERT_TRUE(verify_routines[id] != NULL);
before trying to call the routine.

[alexeypa (please no reviews), 2013-09-06 16:14:56]

https://chromiumcodereview.appspot.com/23461030/diff/11001/remoting/host/setu...
File remoting/host/setup/native_messaging_host.cc (right):

https://chromiumcodereview.appspot.com/23461030/diff/11001/remoting/host/setu...
remoting/host/setup/native_messaging_host.cc:138: pending_requests_ += 1;
On 2013/09/06 03:03:13, Lambros wrote:
> pending_requests++ ?

Done.

https://chromiumcodereview.appspot.com/23461030/diff/11001/remoting/host/setu...
remoting/host/setup/native_messaging_host.cc:177: pending_requests_ -= 1;
On 2013/09/06 03:03:13, Lambros wrote:
> pending_requests_-- ?

Done.

https://chromiumcodereview.appspot.com/23461030/diff/11001/remoting/host/setu...
remoting/host/setup/native_messaging_host.cc:420: pending_requests_ -= 1;
On 2013/09/06 03:03:13, Lambros wrote:
> pending_requests_-- ?

Done.

https://chromiumcodereview.appspot.com/23461030/diff/11001/remoting/host/setu...
File remoting/host/setup/native_messaging_host_unittest.cc (right):

https://chromiumcodereview.appspot.com/23461030/diff/11001/remoting/host/setu...
remoting/host/setup/native_messaging_host_unittest.cc:6: 
On 2013/09/06 03:03:13, Lambros wrote:
> #include "base/basictypes.h" for arraysize().

Done.

https://chromiumcodereview.appspot.com/23461030/diff/11001/remoting/host/setu...
remoting/host/setup/native_messaging_host_unittest.cc:30: void
VerifyHelloResponse(scoped_ptr<base::DictionaryValue> response) {
On 2013/09/06 03:03:13, Lambros wrote:
> Changing to scoped_ptr means that these functions will delete the object
passed
> in - is that necessary?

Yes. It expresses the intent of these functions better. They are used to consume
the received messages.

https://chromiumcodereview.appspot.com/23461030/diff/11001/remoting/host/setu...
remoting/host/setup/native_messaging_host_unittest.cc:354:
message.SetInteger("id", next_id++);
On 2013/09/06 03:03:13, Lambros wrote:
> Optional: Maybe have WriteMessageWithIdToInputPipe() which 

I thought about it. The missing id test seems to be useful as long as NMH has
code to handle this case.

https://chromiumcodereview.appspot.com/23461030/diff/11001/remoting/host/setu...
remoting/host/setup/native_messaging_host_unittest.cc:430: // Call the
verification routine corresponding to the message id.
On 2013/09/06 03:03:13, Lambros wrote:
> ASSERT_TRUE(verify_routines[id] != NULL);
> before trying to call the routine.

Calling a routine using a NULL pointer will crash the process in equally
efficient way. There is no point to duplicate the check.

[commit-bot: I haz the power, 2013-09-06 16:15:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alexeypa@chromium.org/23461030/23001

[Lambros, 2013-09-06 18:55:42]

https://chromiumcodereview.appspot.com/23461030/diff/11001/remoting/host/setu...
File remoting/host/setup/native_messaging_host_unittest.cc (right):

https://chromiumcodereview.appspot.com/23461030/diff/11001/remoting/host/setu...
remoting/host/setup/native_messaging_host_unittest.cc:430: // Call the
verification routine corresponding to the message id.
On 2013/09/06 16:14:57, alexeypa wrote:
> On 2013/09/06 03:03:13, Lambros wrote:
> > ASSERT_TRUE(verify_routines[id] != NULL);
> > before trying to call the routine.
> 
> Calling a routine using a NULL pointer will crash the process in equally
> efficient way. There is no point to duplicate the check.
We shouldn't be crashing the test binary, as this means all subsequent tests
will fail to run. The ASSERT_TRUE macro takes care of only terminating the
current test.

[alexeypa (please no reviews), 2013-09-06 22:19:26]

https://chromiumcodereview.appspot.com/23461030/diff/11001/remoting/host/setu...
File remoting/host/setup/native_messaging_host_unittest.cc (right):

https://chromiumcodereview.appspot.com/23461030/diff/11001/remoting/host/setu...
remoting/host/setup/native_messaging_host_unittest.cc:430: // Call the
verification routine corresponding to the message id.
On 2013/09/06 18:55:42, Lambros wrote:
> On 2013/09/06 16:14:57, alexeypa wrote:
> > On 2013/09/06 03:03:13, Lambros wrote:
> > > ASSERT_TRUE(verify_routines[id] != NULL);
> > > before trying to call the routine.
> > 
> > Calling a routine using a NULL pointer will crash the process in equally
> > efficient way. There is no point to duplicate the check.
> We shouldn't be crashing the test binary, as this means all subsequent tests
> will fail to run. The ASSERT_TRUE macro takes care of only terminating the
> current test.

Done.

[commit-bot: I haz the power, 2013-09-06 22:27:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alexeypa@chromium.org/23461030/45001

[commit-bot: I haz the power, 2013-09-07 04:28:50]

Change committed as 221888