[GCM] Support actual check-in in mcs_probe

[GCM] Support actual check-in in mcs_probe

BUG=284553
TEST=none due to that this is a tool
R=fgorski@chromium.org, zea@chromium.org

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

[fgorski, 2014-01-09 22:23:47]

A few comments to address.

https://codereview.chromium.org/132193003/diff/1/google_apis/gcm/tools/mcs_pr...
File google_apis/gcm/tools/mcs_probe.cc (right):

https://codereview.chromium.org/132193003/diff/1/google_apis/gcm/tools/mcs_pr...
google_apis/gcm/tools/mcs_probe.cc:55: const char kChromeVersion[] = "Test
Chrome Version";
Should we call it "Chrome MCS Probe"?
It would be clear what to look for if someone sees that in the logs and asks
questions.

https://codereview.chromium.org/132193003/diff/1/google_apis/gcm/tools/mcs_pr...
google_apis/gcm/tools/mcs_probe.cc:56: const int64 kUserSerialNumber = 1;
This should be unique within the device, so should probably take the
UserSerialNumber from the store.

Unless of course MCS Probe will never be used in a multi user scenario... (We
can make that kind of change later).

https://codereview.chromium.org/132193003/diff/1/google_apis/gcm/tools/mcs_pr...
google_apis/gcm/tools/mcs_probe.cc:391: secret_ = secret;
I think you should persist android_id and secret, and I am not clear if that
happens.

MCS Probe could have a switch to start fresh and wipe the db.

[jianli, 2014-01-09 23:37:05]

https://codereview.chromium.org/132193003/diff/1/google_apis/gcm/tools/mcs_pr...
File google_apis/gcm/tools/mcs_probe.cc (right):

https://codereview.chromium.org/132193003/diff/1/google_apis/gcm/tools/mcs_pr...
google_apis/gcm/tools/mcs_probe.cc:55: const char kChromeVersion[] = "Test
Chrome Version";
On 2014/01/09 22:23:47, Filip Gorski wrote:
> Should we call it "Chrome MCS Probe"?
> It would be clear what to look for if someone sees that in the logs and asks
> questions.

Done.

https://codereview.chromium.org/132193003/diff/1/google_apis/gcm/tools/mcs_pr...
google_apis/gcm/tools/mcs_probe.cc:56: const int64 kUserSerialNumber = 1;
On 2014/01/09 22:23:47, Filip Gorski wrote:
> This should be unique within the device, so should probably take the
> UserSerialNumber from the store.
> 
> Unless of course MCS Probe will never be used in a multi user scenario... (We
> can make that kind of change later).

For now, mcs_probe only supports single user scenario. We will add multi-user
support later.

https://codereview.chromium.org/132193003/diff/1/google_apis/gcm/tools/mcs_pr...
google_apis/gcm/tools/mcs_probe.cc:391: secret_ = secret;
On 2014/01/09 22:23:47, Filip Gorski wrote:
> I think you should persist android_id and secret, and I am not clear if that
> happens.

This has already been taken care of in MCSClient::Login.
> 
> MCS Probe could have a switch to start fresh and wipe the db.

I don't think we need a switch. You can just run "rm -rf gcm_store" to do it.

[fgorski, 2014-01-10 00:32:14]

LGTM.

Re: saving credentials, I completely missed the Login call.

[Nicolas Zea, 2014-01-10 00:57:57]

https://codereview.chromium.org/132193003/diff/90001/google_apis/gcm/tools/mc...
File google_apis/gcm/tools/mcs_probe.cc (right):

https://codereview.chromium.org/132193003/diff/90001/google_apis/gcm/tools/mc...
google_apis/gcm/tools/mcs_probe.cc:395: const int kDelayMS = 10000;
I'm not sure this is needed. The MCSClient keeps retrying if the propagation
delay hasn't been reached right? That reflects what a real client would do, so
I'd rather just let it retry.

[jianli, 2014-01-10 01:06:23]

https://codereview.chromium.org/132193003/diff/90001/google_apis/gcm/tools/mc...
File google_apis/gcm/tools/mcs_probe.cc (right):

https://codereview.chromium.org/132193003/diff/90001/google_apis/gcm/tools/mc...
google_apis/gcm/tools/mcs_probe.cc:395: const int kDelayMS = 10000;
On 2014/01/10 00:57:57, Nicolas Zea wrote:
> I'm not sure this is needed. The MCSClient keeps retrying if the propagation
> delay hasn't been reached right? That reflects what a real client would do, so
> I'd rather just let it retry.

Without the delaying logic, a CHECK failure is triggered:
 connection_factory_impl.cc(102)] Check failed: !IsEndpointReachable()

[Nicolas Zea, 2014-01-10 01:13:23]

https://codereview.chromium.org/132193003/diff/90001/google_apis/gcm/tools/mc...
File google_apis/gcm/tools/mcs_probe.cc (right):

https://codereview.chromium.org/132193003/diff/90001/google_apis/gcm/tools/mc...
google_apis/gcm/tools/mcs_probe.cc:395: const int kDelayMS = 10000;
On 2014/01/10 01:06:23, jianli wrote:
> On 2014/01/10 00:57:57, Nicolas Zea wrote:
> > I'm not sure this is needed. The MCSClient keeps retrying if the propagation
> > delay hasn't been reached right? That reflects what a real client would do,
so
> > I'd rather just let it retry.
> 
> Without the delaying logic, a CHECK failure is triggered:
>  connection_factory_impl.cc(102)] Check failed: !IsEndpointReachable()

I think that dcheck (and the one on line 169) may actually be unnecessary. Does
it work without them?

[jianli, 2014-01-10 21:42:52]

On 2014/01/10 01:13:23, Nicolas Zea wrote:
>
https://codereview.chromium.org/132193003/diff/90001/google_apis/gcm/tools/mc...
> File google_apis/gcm/tools/mcs_probe.cc (right):
> 
>
https://codereview.chromium.org/132193003/diff/90001/google_apis/gcm/tools/mc...
> google_apis/gcm/tools/mcs_probe.cc:395: const int kDelayMS = 10000;
> On 2014/01/10 01:06:23, jianli wrote:
> > On 2014/01/10 00:57:57, Nicolas Zea wrote:
> > > I'm not sure this is needed. The MCSClient keeps retrying if the
propagation
> > > delay hasn't been reached right? That reflects what a real client would
do,
> so
> > > I'd rather just let it retry.
> > 
> > Without the delaying logic, a CHECK failure is triggered:
> >  connection_factory_impl.cc(102)] Check failed: !IsEndpointReachable()
> 
> I think that dcheck (and the one on line 169) may actually be unnecessary.
Does
> it work without them?

Yes, it works after these 2 DCHECKS got removed. Please review again. Thanks.

[Nicolas Zea, 2014-01-10 21:46:07]

lgtm

[jianli, 2014-01-10 22:09:56]

Committed patchset #3 manually as r244259 (presubmit successful).