Add GCMChannelStatusRequest to talk with server for all users

Add GCMChannelStatusRequest to talk with server for all users

This is to support querying GCM channel status for all users. This
is needed to drop the sign-in enforcement.

BUG=384041
TEST=new tests added

Committed: https://crrev.com/1235e51cc9427558d07f12acd80b6fa96b312748
Cr-Commit-Position: refs/heads/master@{#293764}

[jianli, 2014-09-02 23:27:46]

jianli@chromium.org changed reviewers:
+ fgorski@chromium.org, zea@chromium.org

[jianli, 2014-09-02 23:44:00]

Patchset #2 (id:20001) has been deleted

[jianli, 2014-09-02 23:45:21]

Patchset #2 (id:40001) has been deleted

[jianli, 2014-09-02 23:46:25]

Patchset #2 (id:30011) has been deleted

[jianli, 2014-09-02 23:48:45]

Patchset #2 (id:70001) has been deleted

[jianli, 2014-09-03 00:04:52]

Patchset #2 (id:90001) has been deleted

[jianli, 2014-09-03 00:05:02]

Patchset #2 (id:20002) has been deleted

[jianli, 2014-09-03 17:45:01]

Patchset #3 (id:140001) has been deleted

[fgorski, 2014-09-03 20:42:16]

General comment: please consider which of the files can be moved to
google_Apis/gcm

https://codereview.chromium.org/530253002/diff/160001/components/gcm_driver/g...
File components/gcm_driver/gcm_backoff_policy.h (right):

https://codereview.chromium.org/530253002/diff/160001/components/gcm_driver/g...
components/gcm_driver/gcm_backoff_policy.h:4: 
missing #ifndef COMPONENTS_GCM_DRIKVER_GCM_BACKOFF_ENTRY_H_ ...

https://codereview.chromium.org/530253002/diff/160001/components/gcm_driver/g...
File components/gcm_driver/gcm_channel_status_request.cc (right):

https://codereview.chromium.org/530253002/diff/160001/components/gcm_driver/g...
components/gcm_driver/gcm_channel_status_request.cc:82:
url_fetcher_->SetLoadFlags(net::LOAD_DO_NOT_SEND_COOKIES |
Did you check with sync that we are not supposed to send cookies? This is
removed from registration/unregistration request because GCM uses different
means of authentication. Perhaps those are required here, perhaps not.

https://codereview.chromium.org/530253002/diff/160001/components/gcm_driver/g...
File components/gcm_driver/gcm_channel_status_request.h (right):

https://codereview.chromium.org/530253002/diff/160001/components/gcm_driver/g...
components/gcm_driver/gcm_channel_status_request.h:25: class
GCMChannelStatusRequest : public net::URLFetcherDelegate {
document

https://codereview.chromium.org/530253002/diff/160001/components/gcm_driver/g...
components/gcm_driver/gcm_channel_status_request.h:27: // Callback completing
the unregistration request.
fix doc

https://codereview.chromium.org/530253002/diff/160001/components/gcm_driver/g...
components/gcm_driver/gcm_channel_status_request.h:51: std::string client_id_;
comment on the client id

https://codereview.chromium.org/530253002/diff/160001/components/gcm_driver/g...
File components/gcm_driver/gcm_channel_status_request_unittest.cc (right):

https://codereview.chromium.org/530253002/diff/160001/components/gcm_driver/g...
components/gcm_driver/gcm_channel_status_request_unittest.cc:67: bool
enabled_set, bool enabled, int poll_interval_seconds) {
you could use a "test scenario" enum instead:
{
  NOT_SPECIFIED,
  GCM_ENABLED,
  GCM_DISABLED
}

https://codereview.chromium.org/530253002/diff/160001/components/gcm_driver/g...
components/gcm_driver/gcm_channel_status_request_unittest.cc:166: // enforce.
There is an assumption here that min poll interval is > than 15 minutes. Please
make that statement in the comment.

https://codereview.chromium.org/530253002/diff/160001/components/gcm_driver/p...
File components/gcm_driver/proto/gcm_channel_status.proto (right):

https://codereview.chromium.org/530253002/diff/160001/components/gcm_driver/p...
components/gcm_driver/proto/gcm_channel_status.proto:16: // This request allows
an unauthenticated client to check the status of the
OK that kind off answers the cookies question.

[jianli, 2014-09-03 21:17:55]

Per offline discussions, we still want to keep new files under
components/gcm_driver.

https://codereview.chromium.org/530253002/diff/160001/components/gcm_driver/g...
File components/gcm_driver/gcm_backoff_policy.h (right):

https://codereview.chromium.org/530253002/diff/160001/components/gcm_driver/g...
components/gcm_driver/gcm_backoff_policy.h:4: 
On 2014/09/03 20:42:16, fgorski wrote:
> missing #ifndef COMPONENTS_GCM_DRIKVER_GCM_BACKOFF_ENTRY_H_ ...

Done.

https://codereview.chromium.org/530253002/diff/160001/components/gcm_driver/g...
File components/gcm_driver/gcm_channel_status_request.cc (right):

https://codereview.chromium.org/530253002/diff/160001/components/gcm_driver/g...
components/gcm_driver/gcm_channel_status_request.cc:82:
url_fetcher_->SetLoadFlags(net::LOAD_DO_NOT_SEND_COOKIES |
On 2014/09/03 20:42:16, fgorski wrote:
> Did you check with sync that we are not supposed to send cookies? This is
> removed from registration/unregistration request because GCM uses different
> means of authentication. Perhaps those are required here, perhaps not.

Anyone can talk with the server without authentication.

https://codereview.chromium.org/530253002/diff/160001/components/gcm_driver/g...
File components/gcm_driver/gcm_channel_status_request.h (right):

https://codereview.chromium.org/530253002/diff/160001/components/gcm_driver/g...
components/gcm_driver/gcm_channel_status_request.h:25: class
GCMChannelStatusRequest : public net::URLFetcherDelegate {
On 2014/09/03 20:42:16, fgorski wrote:
> document

Done.

https://codereview.chromium.org/530253002/diff/160001/components/gcm_driver/g...
components/gcm_driver/gcm_channel_status_request.h:27: // Callback completing
the unregistration request.
On 2014/09/03 20:42:16, fgorski wrote:
> fix doc

Done.

https://codereview.chromium.org/530253002/diff/160001/components/gcm_driver/g...
components/gcm_driver/gcm_channel_status_request.h:51: std::string client_id_;
On 2014/09/03 20:42:16, fgorski wrote:
> comment on the client id

Done.

https://codereview.chromium.org/530253002/diff/160001/components/gcm_driver/g...
File components/gcm_driver/gcm_channel_status_request_unittest.cc (right):

https://codereview.chromium.org/530253002/diff/160001/components/gcm_driver/g...
components/gcm_driver/gcm_channel_status_request_unittest.cc:67: bool
enabled_set, bool enabled, int poll_interval_seconds) {
On 2014/09/03 20:42:16, fgorski wrote:
> you could use a "test scenario" enum instead:
> {
>   NOT_SPECIFIED,
>   GCM_ENABLED,
>   GCM_DISABLED
> }

Done.

https://codereview.chromium.org/530253002/diff/160001/components/gcm_driver/g...
components/gcm_driver/gcm_channel_status_request_unittest.cc:166: // enforce.
On 2014/09/03 20:42:16, fgorski wrote:
> There is an assumption here that min poll interval is > than 15 minutes.
Please
> make that statement in the comment.

I think this still works if the min poll interval is smaller than 15 minutes.

https://codereview.chromium.org/530253002/diff/160001/components/gcm_driver/p...
File components/gcm_driver/proto/gcm_channel_status.proto (right):

https://codereview.chromium.org/530253002/diff/160001/components/gcm_driver/p...
components/gcm_driver/proto/gcm_channel_status.proto:16: // This request allows
an unauthenticated client to check the status of the
On 2014/09/03 20:42:16, fgorski wrote:
> OK that kind off answers the cookies question.

Acknowledged.

[fgorski, 2014-09-03 22:38:58]

lgtm

[Nicolas Zea, 2014-09-04 23:57:51]

https://codereview.chromium.org/530253002/diff/200001/components/gcm_driver/g...
File components/gcm_driver/gcm_channel_status_request.cc (right):

https://codereview.chromium.org/530253002/diff/200001/components/gcm_driver/g...
components/gcm_driver/gcm_channel_status_request.cc:28: const base::TimeDelta
kDefaultPollInterval = base::TimeDelta::FromMinutes(60);
const values should be POD, not classes like TimeDelta

https://codereview.chromium.org/530253002/diff/200001/components/gcm_driver/g...
components/gcm_driver/gcm_channel_status_request.cc:32: const char kClientName[]
= "Google Chrome";
Why do we differentiate?

https://codereview.chromium.org/530253002/diff/200001/components/gcm_driver/g...
components/gcm_driver/gcm_channel_status_request.cc:76:
CHECK(proto_data.SerializeToString(&upload_data));
Why is this a CHECK?

https://codereview.chromium.org/530253002/diff/200001/components/gcm_driver/g...
File components/gcm_driver/gcm_channel_status_request.h (right):

https://codereview.chromium.org/530253002/diff/200001/components/gcm_driver/g...
components/gcm_driver/gcm_channel_status_request.h:35: const std::string&
client_id,
What is client id going to be?

https://codereview.chromium.org/530253002/diff/200001/components/gcm_driver/g...
components/gcm_driver/gcm_channel_status_request.h:36:
scoped_refptr<net::URLRequestContextGetter> request_context_getter,
nit: scoped_refptrs should be passed by const ref (const scoped_refptr<...>&)

https://codereview.chromium.org/530253002/diff/200001/components/gcm_driver/g...
components/gcm_driver/gcm_channel_status_request.h:53: std::string client_id_; 
// Our unique ID.
nit: make const (and any other of these members than can be const)

https://codereview.chromium.org/530253002/diff/200001/components/gcm_driver/g...
File components/gcm_driver/gcm_channel_status_request_unittest.cc (right):

https://codereview.chromium.org/530253002/diff/200001/components/gcm_driver/g...
components/gcm_driver/gcm_channel_status_request_unittest.cc:46: enabled_(false)
{
Presumably we'll normally be enabled by default right? Initialize this to true?

https://codereview.chromium.org/530253002/diff/200001/components/gcm_driver/g...
components/gcm_driver/gcm_channel_status_request_unittest.cc:158:
base::TimeDelta::FromMinutes(15);
Isn't the minimum 30 minutes?

https://codereview.chromium.org/530253002/diff/200001/components/gcm_driver/p...
File components/gcm_driver/proto/gcm_channel_status.proto (right):

https://codereview.chromium.org/530253002/diff/200001/components/gcm_driver/p...
components/gcm_driver/proto/gcm_channel_status.proto:33: optional
GcmChannelFlags gcm_channel = 2;
why is this a separate message?

[Nicolas Zea, 2014-09-04 23:58:13]

https://codereview.chromium.org/530253002/diff/200001/components/gcm_driver/g...
File components/gcm_driver/gcm_channel_status_request.cc (right):

https://codereview.chromium.org/530253002/diff/200001/components/gcm_driver/g...
components/gcm_driver/gcm_channel_status_request.cc:28: const base::TimeDelta
kDefaultPollInterval = base::TimeDelta::FromMinutes(60);
On 2014/09/04 23:57:50, Nicolas Zea wrote:
> const values should be POD, not classes like TimeDelta

Rather, global values should be POD

[jianli, 2014-09-05 18:41:56]

https://codereview.chromium.org/530253002/diff/200001/components/gcm_driver/g...
File components/gcm_driver/gcm_channel_status_request.cc (right):

https://codereview.chromium.org/530253002/diff/200001/components/gcm_driver/g...
components/gcm_driver/gcm_channel_status_request.cc:28: const base::TimeDelta
kDefaultPollInterval = base::TimeDelta::FromMinutes(60);
On 2014/09/04 23:57:50, Nicolas Zea wrote:
> const values should be POD, not classes like TimeDelta

Done.

https://codereview.chromium.org/530253002/diff/200001/components/gcm_driver/g...
components/gcm_driver/gcm_channel_status_request.cc:28: const base::TimeDelta
kDefaultPollInterval = base::TimeDelta::FromMinutes(60);
On 2014/09/04 23:58:12, Nicolas Zea wrote:
> On 2014/09/04 23:57:50, Nicolas Zea wrote:
> > const values should be POD, not classes like TimeDelta
> 
> Rather, global values should be POD

Done.

https://codereview.chromium.org/530253002/diff/200001/components/gcm_driver/g...
components/gcm_driver/gcm_channel_status_request.cc:32: const char kClientName[]
= "Google Chrome";
On 2014/09/04 23:57:50, Nicolas Zea wrote:
> Why do we differentiate?

This is to differentiate between official build and non-official build. This
seems to be the one we usually passed to the server (see
autofill_download_manager).

https://codereview.chromium.org/530253002/diff/200001/components/gcm_driver/g...
components/gcm_driver/gcm_channel_status_request.cc:76:
CHECK(proto_data.SerializeToString(&upload_data));
On 2014/09/04 23:57:50, Nicolas Zea wrote:
> Why is this a CHECK?

Changed to DCHECK.

https://codereview.chromium.org/530253002/diff/200001/components/gcm_driver/g...
File components/gcm_driver/gcm_channel_status_request.h (right):

https://codereview.chromium.org/530253002/diff/200001/components/gcm_driver/g...
components/gcm_driver/gcm_channel_status_request.h:35: const std::string&
client_id,
On 2014/09/04 23:57:50, Nicolas Zea wrote:
> What is client id going to be?

This is similar to GenerateCacheGUID or GenerateInvalidatorClientId. We can't
use the one that is created only for sync users. So here we just generate our
own one.

https://codereview.chromium.org/530253002/diff/200001/components/gcm_driver/g...
components/gcm_driver/gcm_channel_status_request.h:36:
scoped_refptr<net::URLRequestContextGetter> request_context_getter,
On 2014/09/04 23:57:50, Nicolas Zea wrote:
> nit: scoped_refptrs should be passed by const ref (const scoped_refptr<...>&)

Done.

https://codereview.chromium.org/530253002/diff/200001/components/gcm_driver/g...
components/gcm_driver/gcm_channel_status_request.h:53: std::string client_id_; 
// Our unique ID.
On 2014/09/04 23:57:50, Nicolas Zea wrote:
> nit: make const (and any other of these members than can be const)

Done.

https://codereview.chromium.org/530253002/diff/200001/components/gcm_driver/g...
File components/gcm_driver/gcm_channel_status_request_unittest.cc (right):

https://codereview.chromium.org/530253002/diff/200001/components/gcm_driver/g...
components/gcm_driver/gcm_channel_status_request_unittest.cc:46: enabled_(false)
{
On 2014/09/04 23:57:50, Nicolas Zea wrote:
> Presumably we'll normally be enabled by default right? Initialize this to
true?

Done.

https://codereview.chromium.org/530253002/diff/200001/components/gcm_driver/g...
components/gcm_driver/gcm_channel_status_request_unittest.cc:158:
base::TimeDelta::FromMinutes(15);
On 2014/09/04 23:57:50, Nicolas Zea wrote:
> Isn't the minimum 30 minutes?

Yes, the minimum is 30 minutes. Here we're setting the poll interval to min
value plus 15 minutes. This makes us keep the test intact in case that the
minimum value gets changed later.

https://codereview.chromium.org/530253002/diff/200001/components/gcm_driver/p...
File components/gcm_driver/proto/gcm_channel_status.proto (right):

https://codereview.chromium.org/530253002/diff/200001/components/gcm_driver/p...
components/gcm_driver/proto/gcm_channel_status.proto:33: optional
GcmChannelFlags gcm_channel = 2;
On 2014/09/04 23:57:50, Nicolas Zea wrote:
> why is this a separate message?

This is what I am given from the server guy. Will check with him on this.

[Nicolas Zea, 2014-09-05 18:53:03]

https://codereview.chromium.org/530253002/diff/200001/components/gcm_driver/g...
File components/gcm_driver/gcm_channel_status_request.cc (right):

https://codereview.chromium.org/530253002/diff/200001/components/gcm_driver/g...
components/gcm_driver/gcm_channel_status_request.cc:32: const char kClientName[]
= "Google Chrome";
On 2014/09/05 18:41:55, jianli wrote:
> On 2014/09/04 23:57:50, Nicolas Zea wrote:
> > Why do we differentiate?
> 
> This is to differentiate between official build and non-official build. This
> seems to be the one we usually passed to the server (see
> autofill_download_manager).

Why do we care in this case?

https://codereview.chromium.org/530253002/diff/200001/components/gcm_driver/g...
File components/gcm_driver/gcm_channel_status_request.h (right):

https://codereview.chromium.org/530253002/diff/200001/components/gcm_driver/g...
components/gcm_driver/gcm_channel_status_request.h:35: const std::string&
client_id,
On 2014/09/05 18:41:55, jianli wrote:
> On 2014/09/04 23:57:50, Nicolas Zea wrote:
> > What is client id going to be?
> 
> This is similar to GenerateCacheGUID or GenerateInvalidatorClientId. We can't
> use the one that is created only for sync users. So here we just generate our
> own one.

Do we need a client id at all? If this is a kill switch, why do we need to
differentiate by client? Presumably UserAgent is enough?

[jianli, 2014-09-05 21:26:17]

https://codereview.chromium.org/530253002/diff/200001/components/gcm_driver/g...
File components/gcm_driver/gcm_channel_status_request.cc (right):

https://codereview.chromium.org/530253002/diff/200001/components/gcm_driver/g...
components/gcm_driver/gcm_channel_status_request.cc:32: const char kClientName[]
= "Google Chrome";
On 2014/09/05 18:53:03, Nicolas Zea wrote:
> On 2014/09/05 18:41:55, jianli wrote:
> > On 2014/09/04 23:57:50, Nicolas Zea wrote:
> > > Why do we differentiate?
> > 
> > This is to differentiate between official build and non-official build. This
> > seems to be the one we usually passed to the server (see
> > autofill_download_manager).
> 
> Why do we care in this case?

Removed. We don't need this any more.

https://codereview.chromium.org/530253002/diff/200001/components/gcm_driver/g...
File components/gcm_driver/gcm_channel_status_request.h (right):

https://codereview.chromium.org/530253002/diff/200001/components/gcm_driver/g...
components/gcm_driver/gcm_channel_status_request.h:35: const std::string&
client_id,
On 2014/09/05 18:53:03, Nicolas Zea wrote:
> On 2014/09/05 18:41:55, jianli wrote:
> > On 2014/09/04 23:57:50, Nicolas Zea wrote:
> > > What is client id going to be?
> > 
> > This is similar to GenerateCacheGUID or GenerateInvalidatorClientId. We
can't
> > use the one that is created only for sync users. So here we just generate
our
> > own one.
> 
> Do we need a client id at all? If this is a kill switch, why do we need to
> differentiate by client? Presumably UserAgent is enough?

After talking w/ Kevin, we don't need this.

[Nicolas Zea, 2014-09-05 21:49:19]

lgtm

[jianli, 2014-09-05 21:50:49]

The CQ bit was checked by jianli@chromium.org

[commit-bot: I haz the power, 2014-09-05 21:51:39]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jianli@chromium.org/530253002/260001

[jianli, 2014-09-05 22:04:19]

The CQ bit was unchecked by jianli@chromium.org

[jianli, 2014-09-05 22:05:30]

The CQ bit was checked by jianli@chromium.org

[commit-bot: I haz the power, 2014-09-05 22:06:25]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jianli@chromium.org/530253002/280001

[commit-bot: I haz the power, 2014-09-06 00:28:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-06 00:28:58]

Exceeded time limit waiting for builds to trigger.

[jianli, 2014-09-08 16:57:28]

The CQ bit was checked by jianli@chromium.org

[commit-bot: I haz the power, 2014-09-08 17:19:19]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jianli@chromium.org/530253002/280001

[commit-bot: I haz the power, 2014-09-08 19:08:42]

Committed patchset #9 (id:280001) as 6d5f9f281a9f2a3dd3e6113e50dbbb86b0019b56

[commit-bot: I haz the power, 2014-09-10 03:47:30]

Patchset 9 (id:??) landed as
https://crrev.com/1235e51cc9427558d07f12acd80b6fa96b312748
Cr-Commit-Position: refs/heads/master@{#293764}