[GCM] Add a SelectiveAck test for midstream acks

[GCM] Add a SelectiveAck test for midstream acks

SelectiveAck was over confirming due to unsigned in misuse. This adds a test
that exercises the confirm logic used by SelectiveAck.

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

[Nicolas Zea, 2014-07-11 22:14:43]

PTAL

[fgorski, 2014-07-14 20:59:36]

Overall ok, but the test is hard to follow,as it is not that trivial.

Could you update comments before I sign off?

https://codereview.chromium.org/383993006/diff/1/google_apis/gcm/engine/mcs_c...
File google_apis/gcm/engine/mcs_client.cc (right):

https://codereview.chromium.org/383993006/diff/1/google_apis/gcm/engine/mcs_c...
google_apis/gcm/engine/mcs_client.cc:842:
base::MessageLoop::current()->PostTask(
Add comment why this is added.

https://codereview.chromium.org/383993006/diff/1/google_apis/gcm/engine/mcs_c...
File google_apis/gcm/engine/mcs_client_unittest.cc (right):

https://codereview.chromium.org/383993006/diff/1/google_apis/gcm/engine/mcs_c...
google_apis/gcm/engine/mcs_client_unittest.cc:591: // the ack for the server
message 2, but not server message.
Please use the ids of messages consistently, and preferably in quotes, e.g. "2",
which will make it more readable.

Also perhaps making comments more structured could simplify the whole thing,
e.g.
Client sends "2"
Client acks "s1"

and then
Client receives "s2"
Client receives ack of "2"

And so on. I think it will help me and other what to expect after the restart of
the client.

https://codereview.chromium.org/383993006/diff/1/google_apis/gcm/engine/mcs_c...
google_apis/gcm/engine/mcs_client_unittest.cc:605: // Rebuild the client.
Message "20" should be acked as part of login,
what do you mean by "20"?

[Nicolas Zea, 2014-07-14 21:16:18]

PTAL

https://codereview.chromium.org/383993006/diff/1/google_apis/gcm/engine/mcs_c...
File google_apis/gcm/engine/mcs_client.cc (right):

https://codereview.chromium.org/383993006/diff/1/google_apis/gcm/engine/mcs_c...
google_apis/gcm/engine/mcs_client.cc:842:
base::MessageLoop::current()->PostTask(
On 2014/07/14 20:59:35, fgorski wrote:
> Add comment why this is added.

The above comment applies to this. We're resending any remaining outgoing
messages. This call should have been there before.

https://codereview.chromium.org/383993006/diff/1/google_apis/gcm/engine/mcs_c...
File google_apis/gcm/engine/mcs_client_unittest.cc (right):

https://codereview.chromium.org/383993006/diff/1/google_apis/gcm/engine/mcs_c...
google_apis/gcm/engine/mcs_client_unittest.cc:591: // the ack for the server
message 2, but not server message.
On 2014/07/14 20:59:35, fgorski wrote:
> Please use the ids of messages consistently, and preferably in quotes, e.g.
"2",
> which will make it more readable.
> 
> Also perhaps making comments more structured could simplify the whole thing,
> e.g.
> Client sends "2"
> Client acks "s1"
> 
> and then
> Client receives "s2"
> Client receives ack of "2"
> 
> And so on. I think it will help me and other what to expect after the restart
of
> the client.

Done.

https://codereview.chromium.org/383993006/diff/1/google_apis/gcm/engine/mcs_c...
google_apis/gcm/engine/mcs_client_unittest.cc:605: // Rebuild the client.
Message "20" should be acked as part of login,
On 2014/07/14 20:59:35, fgorski wrote:
> what do you mean by "20"?

Oops, typo. Fixed.

[fgorski, 2014-07-15 04:07:58]

lgtm with nit

https://codereview.chromium.org/383993006/diff/1/google_apis/gcm/engine/mcs_c...
File google_apis/gcm/engine/mcs_client.cc (right):

https://codereview.chromium.org/383993006/diff/1/google_apis/gcm/engine/mcs_c...
google_apis/gcm/engine/mcs_client.cc:842:
base::MessageLoop::current()->PostTask(
On 2014/07/14 21:16:17, Nicolas Zea wrote:
> On 2014/07/14 20:59:35, fgorski wrote:
> > Add comment why this is added.
> 
> The above comment applies to this. We're resending any remaining outgoing
> messages. This call should have been there before.

Fair enough, comment was cut by the diff view I was using.

https://codereview.chromium.org/383993006/diff/40001/google_apis/gcm/engine/m...
File google_apis/gcm/engine/mcs_client_unittest.cc (right):

https://codereview.chromium.org/383993006/diff/40001/google_apis/gcm/engine/m...
google_apis/gcm/engine/mcs_client_unittest.cc:610: // Rebuild the client without
any further acks from server.
nit: Does this reset the server stream id? If so, it's worth a comment.

[Nicolas Zea, 2014-07-16 17:58:32]

https://codereview.chromium.org/383993006/diff/40001/google_apis/gcm/engine/m...
File google_apis/gcm/engine/mcs_client_unittest.cc (right):

https://codereview.chromium.org/383993006/diff/40001/google_apis/gcm/engine/m...
google_apis/gcm/engine/mcs_client_unittest.cc:610: // Rebuild the client without
any further acks from server.
On 2014/07/15 04:07:58, fgorski wrote:
> nit: Does this reset the server stream id? If so, it's worth a comment.

Done.

[Nicolas Zea, 2014-07-16 17:58:36]

The CQ bit was checked by zea@chromium.org

[commit-bot: I haz the power, 2014-07-16 18:05:36]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/zea@chromium.org/383993006/60001

[commit-bot: I haz the power, 2014-07-17 09:48:14]

Change committed as 283729