Add support for GCM subtypes to desktop Instance ID implementation

Add support for GCM subtypes to desktop Instance ID implementation

Adds support for subtypes to the desktop Instance ID
implementation. A follow-on patch will switch Web Push
to use subtypes on desktop. Chrome Apps/Extensions will
be unchanged.

The main benefit will be consistency between desktop
and Android - both will be able to use subtypes for the
app_id, rather than sending app_id to GCM as the
category field (package name) on desktop only. This will
avoid spamming GCM's category column with many unique
values (since each app_id that Push generates contains
a random GUID).

Part of a series of patches:
1. https://codereview.chromium.org/1832833002 adds InstanceIDWithSubtype
2. https://codereview.chromium.org/1830983002 adds JNI bindings
3. https://codereview.chromium.org/1829023002 adds fake and test
4. https://codereview.chromium.org/1899753002 fixes strict mode violations
5. https://codereview.chromium.org/1854093002 enables InstanceID by default
6. https://codereview.chromium.org/1953273002 extends the GCMKeyStore
7. https://codereview.chromium.org/1923953002 integrates IIDs with crypto
8. this patch
9. https://codereview.chromium.org/1851423003 switches Push to InstanceIDs

Also depends on
- https://codereview.chromium.org/2148163003 fix test asserts
- https://codereview.chromium.org/2217803003 EXPECT_DCHECK

BUG=533498, 589461
Committed: https://crrev.com/627dc8c76a66a8c6251c6d0423e8782b101bb050
Cr-Commit-Position: refs/heads/master@{#413259}

[johnme, 2016-07-01 13:51:31]

Description was changed from

==========
Switch Web Push to use GCM subtypes on desktop

Adds support for subtypes to the desktop Instance ID implementation,
and switches Web Push to use subtypes on desktop. Chrome Apps/Extensions
are unchanged.

The main benefit is consistency between desktop and Android - now both
use subtypes for the app_id, rather than sending app_id to GCM as the
category field (package name) on desktop only. This will avoid spamming
GCM's category column with many unique values (since each app_id that
Push generates contains a random GUIDs).

BUG=533498,589461
==========

to

==========
Add support for GCM subtypes to desktop Instance ID implementation

Adds support for subtypes to the desktop Instance ID
implementation. A follow-on patch will switch Web Push
to use subtypes on desktop. Chrome Apps/Extensions will
be unchanged.

The main benefit will be consistency between desktop
and Android - both will be able to use subtypes for the
app_id, rather than sending app_id to GCM as the
category field (package name) on desktop only. This will
avoid spamming GCM's category column with many unique
values (since each app_id that Push generates contains
a random GUID).

Part of a series of patches:
1. https://codereview.chromium.org/1832833002 adds InstanceIDWithSubtype
2. https://codereview.chromium.org/1830983002 adds JNI bindings
3. https://codereview.chromium.org/1829023002 adds fake and test
4. https://codereview.chromium.org/1899753002 fixes strict mode violations
5. https://codereview.chromium.org/1854093002 enables InstanceID by default
6. https://codereview.chromium.org/1953273002 extends the GCMKeyStore
7. https://codereview.chromium.org/1923953002 integrates IIDs with crypto
8. this patch
9. https://codereview.chromium.org/1851423003 switches Push to InstanceIDs

BUG=533498,589461
==========

[johnme, 2016-07-05 15:18:55]

johnme@chromium.org changed reviewers:
+ peter@chromium.org

[johnme, 2016-07-05 15:18:56]

+peter for first pass

[Peter Beverloo, 2016-07-08 13:21:06]

Thank you for the CL, John!

First things first- this CL introduces a very significant amount of
complexity, complicated comments and conditionals throughout the code,
including a dependency on a regular expressions library, which we should
absolutely try to find alternatives to.

Let's start with defining today's users of GCM and their app ids:
    - Extensions using the extension ID (consecutive characters),
    - Web Push using "wp:" prefixed identifiers,
    - Various Google-controlled features (invalidation, proximity auth,
      Chrome OS' heartbeat, copresence, and so on...) using package names.

None of these allow package name-like identifiers to be provided by third-
party content. Note that I deliberately exclude ARC++, as their use of
the GCM Driver is purely hypothetical at this point, so we do not have to
preemptively cater for them.

Secondly, we also know that subscriptions created for Windows will never
reach users on other platforms— in effect, I believe that the category
that will be verified by GCMClient::CategoryHasSubtype should always be
identical to |GCMDriverDesktop::IOWorker::category_for_subtypes_|.

Given this, I see two paths for very significant simplification. Both
involve creating a _normal_ package name-like identifier for Chrome, i.e.
getting rid of the colon.

    (1) We trust the subtype when provided, and the category is set to
        a package name. This is fine because we know only Chrome's code
        can create subscriptions with package name-like identifiers.

    (2) Slightly more conservative, we trust the subtype when the category
        is identical to |category_for_subtypes_|.

I'm fine starting with (2), because it's easier to implement and widening
it to option (1) is easy to do when necessary. I further suspect that this
would allow us to remove the knowledge from the GCMDriver altogether, and
make this a concern of the GCMClientImpl?

-----

The second high-level comment I have are the |use_subtype_for_app_id|
booleans used for InstanceIDs, and the |force_subtype_for_app_id| boolean
when creating an InstanceID. I find it very hard to reason about them.

Since this is up to the consumer of the API in either case, why don't we:
    (1) Expose the installation's "master" category from the GCM Driver.
    (2) Have customers, i.e. Web Push, use this as the |app_id| passed
        to these APIs, and have the APIs take a separate string for the
        |subtype| to consider?

Depending on which path we go with for the default category name, it
wouldn't be hard to add a DCHECK for these cases. (Especially if the
GCMDriver exposes a getter for it.)

-----

Finally, I see that you're using ScopedDisableExitOnDFatal again. Would
you please consider _not_ using this, and instead extracting the
verification code to a separate method that the DCHECK uses as well? I
got very confused at test output yesterday because it does output the
stack trace, despite succeeding.

[johnme, 2016-07-13 15:39:59]

On 2016/07/08 13:21:06, Peter Beverloo wrote:
> Thank you for the CL, John!
> 
> First things first- this CL introduces a very significant amount of
> complexity, complicated comments and conditionals throughout the code,
> including a dependency on a regular expressions library, which we should
> absolutely try to find alternatives to.

Let's discuss that offline.

> -----
> 
> The second high-level comment I have are the |use_subtype_for_app_id|
> booleans used for InstanceIDs, and the |force_subtype_for_app_id| boolean
> when creating an InstanceID. I find it very hard to reason about them.
> 
> Since this is up to the consumer of the API in either case, why don't we:
>     (1) Expose the installation's "master" category from the GCM Driver.
>     (2) Have customers, i.e. Web Push, use this as the |app_id| passed
>         to these APIs, and have the APIs take a separate string for the
>         |subtype| to consider?

It would be confusing for consumers to have to pass in a category and
subtype, since all other interactions with GCM/InstanceID deal with
app_ids (so it would require consumers to understand 3 different kinds
of id).

Instead I've removed the boolean parameter from InstanceIDDriver, and
made subtypes the default. To support Chrome Apps/Extensions (the only
expected consumer of non-subtypes), there is now a dedicated
GetInstanceIDForExtensions method. This method is not supported on
Android (hence is simpler than the previous approach, which used to
fallback to subtypes on Android).

> -----
> 
> Finally, I see that you're using ScopedDisableExitOnDFatal again. Would
> you please consider _not_ using this, and instead extracting the
> verification code to a separate method that the DCHECK uses as well? I
> got very confused at test output yesterday because it does output the
> stack trace, despite succeeding.

The CHECKs and LOG(DFATAL) being tested are part of core workflows, so
they can't be extracted, and there's no clean way to toggle them off.
I've added LOG statements to the tests that use EXPECT_(D)FATAL, which
should make things much clearer when looking at the test output.

[johnme, 2016-07-14 11:44:12]

Description was changed from

==========
Add support for GCM subtypes to desktop Instance ID implementation

Adds support for subtypes to the desktop Instance ID
implementation. A follow-on patch will switch Web Push
to use subtypes on desktop. Chrome Apps/Extensions will
be unchanged.

The main benefit will be consistency between desktop
and Android - both will be able to use subtypes for the
app_id, rather than sending app_id to GCM as the
category field (package name) on desktop only. This will
avoid spamming GCM's category column with many unique
values (since each app_id that Push generates contains
a random GUID).

Part of a series of patches:
1. https://codereview.chromium.org/1832833002 adds InstanceIDWithSubtype
2. https://codereview.chromium.org/1830983002 adds JNI bindings
3. https://codereview.chromium.org/1829023002 adds fake and test
4. https://codereview.chromium.org/1899753002 fixes strict mode violations
5. https://codereview.chromium.org/1854093002 enables InstanceID by default
6. https://codereview.chromium.org/1953273002 extends the GCMKeyStore
7. https://codereview.chromium.org/1923953002 integrates IIDs with crypto
8. this patch
9. https://codereview.chromium.org/1851423003 switches Push to InstanceIDs

BUG=533498,589461
==========

to

==========
Add support for GCM subtypes to desktop Instance ID implementation

Adds support for subtypes to the desktop Instance ID
implementation. A follow-on patch will switch Web Push
to use subtypes on desktop. Chrome Apps/Extensions will
be unchanged.

The main benefit will be consistency between desktop
and Android - both will be able to use subtypes for the
app_id, rather than sending app_id to GCM as the
category field (package name) on desktop only. This will
avoid spamming GCM's category column with many unique
values (since each app_id that Push generates contains
a random GUID).

Part of a series of patches:
1. https://codereview.chromium.org/1832833002 adds InstanceIDWithSubtype
2. https://codereview.chromium.org/1830983002 adds JNI bindings
3. https://codereview.chromium.org/1829023002 adds fake and test
4. https://codereview.chromium.org/1899753002 fixes strict mode violations
5. https://codereview.chromium.org/1854093002 enables InstanceID by default
6. https://codereview.chromium.org/1953273002 extends the GCMKeyStore
7. https://codereview.chromium.org/1923953002 integrates IIDs with crypto
8. this patch
9. https://codereview.chromium.org/1851423003 switches Push to InstanceIDs

Also depends on
- https://codereview.chromium.org/2148163003 fix test asserts

BUG=533498,589461
==========

[johnme, 2016-07-19 18:39:01]

Made a bunch of modifications to simplify this CL:
- Now generates the default category (used when app_id is sent as a subtype)
once only, and stores it in prefs.
- Uses a valid package name for the default category (s/:/\./).
- Replaces the check in RegistrationInfoComparer with checks in
GCMClientImpl::Register, GCMClientImpl::Unregister and
GCMClientImpl::HandleIncomingDataMessage.
- Removed two uses of EXPECT_FATAL, and the addition of EXPECT_FATAL to net/;
unfortunately I needed instead to add an EXPECT_DCHECK to net/ instead (since I
didn't want to add the overhead of evaluating the condition and doing a
LOG(DFATAL) in release builds). If you're ok with this single remaining
EXPECT_DCHECK, then I'll go ahead and factor the net/ changes out to a separate
patch.

[Peter Beverloo, 2016-07-22 12:17:04]

This looks much better already, thanks! A few comments;

https://codereview.chromium.org/2111973002/diff/80001/components/gcm_driver.gypi
File components/gcm_driver.gypi (right):

https://codereview.chromium.org/2111973002/diff/80001/components/gcm_driver.g...
components/gcm_driver.gypi:37: '../third_party/re2/re2.gyp:re2',
drop

https://codereview.chromium.org/2111973002/diff/80001/components/gcm_driver/B...
File components/gcm_driver/BUILD.gn (right):

https://codereview.chromium.org/2111973002/diff/80001/components/gcm_driver/B...
components/gcm_driver/BUILD.gn:79: "//third_party/re2",
drop

https://codereview.chromium.org/2111973002/diff/80001/components/gcm_driver/g...
File components/gcm_driver/gcm_client.h (right):

https://codereview.chromium.org/2111973002/diff/80001/components/gcm_driver/g...
components/gcm_driver/gcm_client.h:324: virtual bool CategoryHasSubtype(const
std::string& category) = 0;
What's the benefit of keeping the ambiguously named "CategoryHasSubtype"
mentioning a "special category" if introducing the concept of a "product
subtype" would catch them all?

https://codereview.chromium.org/2111973002/diff/80001/components/gcm_driver/g...
File components/gcm_driver/gcm_client_impl.cc (right):

https://codereview.chromium.org/2111973002/diff/80001/components/gcm_driver/g...
components/gcm_driver/gcm_client_impl.cc:965: instance_id_token_info &&
instance_id_token_info->use_subtype,
nit: extract to a local bool w/ an appropriate name

https://codereview.chromium.org/2111973002/diff/80001/components/gcm_driver/g...
components/gcm_driver/gcm_client_impl.cc:1110: instance_id_token_info &&
instance_id_token_info->use_subtype,
nit: extract to a local bool w/ an appropriate name

https://codereview.chromium.org/2111973002/diff/80001/components/gcm_driver/g...
components/gcm_driver/gcm_client_impl.cc:1320: std::string subtype = "";
nit: no init

https://codereview.chromium.org/2111973002/diff/80001/components/gcm_driver/g...
components/gcm_driver/gcm_client_impl.cc:1321: auto subtype_iter =
message_data.find(kSubtypeKey);
This is using "subtype", which is potentially untrusted, while we agreed to
start using "gcm.subtype", which is trusted. This is not yet supported by the
server, though, so we should probably keep this code, but include a TODO for
simply deferring to the "gcm.subtype" value when present?

https://codereview.chromium.org/2111973002/diff/80001/components/gcm_driver/g...
components/gcm_driver/gcm_client_impl.cc:1403: DLOG(ERROR) << "Incoming GCM
message for " << app_id
Is this DLOG necessary? It would only happen when the server sends an invalid
message to a debug build of Chrome, so (a) how would anyone ever see this
message, and (b) what are they supposed to do when it happens?

https://codereview.chromium.org/2111973002/diff/80001/components/gcm_driver/g...
File components/gcm_driver/gcm_client_impl_unittest.cc (right):

https://codereview.chromium.org/2111973002/diff/80001/components/gcm_driver/g...
components/gcm_driver/gcm_client_impl_unittest.cc:1532:
EXPECT_EQ(GCMClientImpl::READY, gcm_client_state());
Sounds like an ASSERT_EQ?

https://codereview.chromium.org/2111973002/diff/80001/components/gcm_driver/g...
File components/gcm_driver/gcm_desktop_utils.cc (right):

https://codereview.chromium.org/2111973002/diff/80001/components/gcm_driver/g...
components/gcm_driver/gcm_desktop_utils.cc:96: std::string ns = product ==
"chromium" ? "org" : "com";
This really feels like the wrong layer to make this decision, notably the fact
that it forces ".com" for non-Chromium builds :-(.

Have you considered moving this logic to
//chrome/browser/services/gcm/gcm_product_util.{cc,h} or similar, and just
passing in the |product_category| whenever constructing the GCM Driver?

https://codereview.chromium.org/2111973002/diff/80001/components/gcm_driver/g...
File components/gcm_driver/gcm_driver.h (right):

https://codereview.chromium.org/2111973002/diff/80001/components/gcm_driver/g...
components/gcm_driver/gcm_driver.h:58: // Delete all tokens assoicated with
|app_id|.
nit: Delete comment, the name is clear enough.

https://codereview.chromium.org/2111973002/diff/80001/components/gcm_driver/r...
File components/gcm_driver/registration_info.h (right):

https://codereview.chromium.org/2111973002/diff/80001/components/gcm_driver/r...
components/gcm_driver/registration_info.h:114: // are not
serialized/deserialized!
s/!/\./

[johnme, 2016-07-26 17:11:56]

Addressed review comments - thanks!

https://codereview.chromium.org/2111973002/diff/80001/components/gcm_driver.gypi
File components/gcm_driver.gypi (right):

https://codereview.chromium.org/2111973002/diff/80001/components/gcm_driver.g...
components/gcm_driver.gypi:37: '../third_party/re2/re2.gyp:re2',
On 2016/07/22 12:17:03, Peter Beverloo wrote:
> drop

Oops done.

https://codereview.chromium.org/2111973002/diff/80001/components/gcm_driver/B...
File components/gcm_driver/BUILD.gn (right):

https://codereview.chromium.org/2111973002/diff/80001/components/gcm_driver/B...
components/gcm_driver/BUILD.gn:79: "//third_party/re2",
On 2016/07/22 12:17:03, Peter Beverloo wrote:
> drop

Oops done.

https://codereview.chromium.org/2111973002/diff/80001/components/gcm_driver/g...
File components/gcm_driver/gcm_client.h (right):

https://codereview.chromium.org/2111973002/diff/80001/components/gcm_driver/g...
components/gcm_driver/gcm_client.h:324: virtual bool CategoryHasSubtype(const
std::string& category) = 0;
On 2016/07/22 12:17:03, Peter Beverloo wrote:
> What's the benefit of keeping the ambiguously named "CategoryHasSubtype"
> mentioning a "special category" if introducing the concept of a "product
> subtype" would catch them all?

Fair. I've inlined this into GCMClientImpl::HandleIncomingMessage.

https://codereview.chromium.org/2111973002/diff/80001/components/gcm_driver/g...
File components/gcm_driver/gcm_client_impl.cc (right):

https://codereview.chromium.org/2111973002/diff/80001/components/gcm_driver/g...
components/gcm_driver/gcm_client_impl.cc:965: instance_id_token_info &&
instance_id_token_info->use_subtype,
On 2016/07/22 12:17:03, Peter Beverloo wrote:
> nit: extract to a local bool w/ an appropriate name

Done.

https://codereview.chromium.org/2111973002/diff/80001/components/gcm_driver/g...
components/gcm_driver/gcm_client_impl.cc:1110: instance_id_token_info &&
instance_id_token_info->use_subtype,
On 2016/07/22 12:17:03, Peter Beverloo wrote:
> nit: extract to a local bool w/ an appropriate name

Done.

https://codereview.chromium.org/2111973002/diff/80001/components/gcm_driver/g...
components/gcm_driver/gcm_client_impl.cc:1320: std::string subtype = "";
On 2016/07/22 12:17:03, Peter Beverloo wrote:
> nit: no init

Done.

https://codereview.chromium.org/2111973002/diff/80001/components/gcm_driver/g...
components/gcm_driver/gcm_client_impl.cc:1321: auto subtype_iter =
message_data.find(kSubtypeKey);
On 2016/07/22 12:17:03, Peter Beverloo wrote:
> This is using "subtype", which is potentially untrusted, while we agreed to
> start using "gcm.subtype", which is trusted. This is not yet supported by the
> server, though, so we should probably keep this code, but include a TODO for
> simply deferring to the "gcm.subtype" value when present?

Added TODO.

https://codereview.chromium.org/2111973002/diff/80001/components/gcm_driver/g...
components/gcm_driver/gcm_client_impl.cc:1403: DLOG(ERROR) << "Incoming GCM
message for " << app_id
On 2016/07/22 12:17:03, Peter Beverloo wrote:
> Is this DLOG necessary? It would only happen when the server sends an invalid
> message to a debug build of Chrome, so (a) how would anyone ever see this
> message, and (b) what are they supposed to do when it happens?

a) it's most likely to happen if the value of use_subtype is incorrect when we
register (or if the GCM server swaps the fields round, but that's very
unlikely).

b) hopefully file a bug?

https://codereview.chromium.org/2111973002/diff/80001/components/gcm_driver/g...
File components/gcm_driver/gcm_client_impl_unittest.cc (right):

https://codereview.chromium.org/2111973002/diff/80001/components/gcm_driver/g...
components/gcm_driver/gcm_client_impl_unittest.cc:1532:
EXPECT_EQ(GCMClientImpl::READY, gcm_client_state());
On 2016/07/22 12:17:03, Peter Beverloo wrote:
> Sounds like an ASSERT_EQ?

Done.

https://codereview.chromium.org/2111973002/diff/80001/components/gcm_driver/g...
File components/gcm_driver/gcm_desktop_utils.cc (right):

https://codereview.chromium.org/2111973002/diff/80001/components/gcm_driver/g...
components/gcm_driver/gcm_desktop_utils.cc:96: std::string ns = product ==
"chromium" ? "org" : "com";
On 2016/07/22 12:17:03, Peter Beverloo wrote:
> This really feels like the wrong layer to make this decision, notably the fact
> that it forces ".com" for non-Chromium builds :-(.
> 
> Have you considered moving this logic to
> //chrome/browser/services/gcm/gcm_product_util.{cc,h} or similar, and just
> passing in the |product_category| whenever constructing the GCM Driver?

Done.

https://codereview.chromium.org/2111973002/diff/80001/components/gcm_driver/g...
File components/gcm_driver/gcm_driver.h (right):

https://codereview.chromium.org/2111973002/diff/80001/components/gcm_driver/g...
components/gcm_driver/gcm_driver.h:58: // Delete all tokens assoicated with
|app_id|.
On 2016/07/22 12:17:03, Peter Beverloo wrote:
> nit: Delete comment, the name is clear enough.

Done.

https://codereview.chromium.org/2111973002/diff/80001/components/gcm_driver/r...
File components/gcm_driver/registration_info.h (right):

https://codereview.chromium.org/2111973002/diff/80001/components/gcm_driver/r...
components/gcm_driver/registration_info.h:114: // are not
serialized/deserialized!
On 2016/07/22 12:17:04, Peter Beverloo wrote:
> s/!/\./

Done.

[Peter Beverloo, 2016-07-28 12:34:10]

lgtm % nits

https://codereview.chromium.org/2111973002/diff/80001/components/gcm_driver/r...
File components/gcm_driver/registration_info.h (right):

https://codereview.chromium.org/2111973002/diff/80001/components/gcm_driver/r...
components/gcm_driver/registration_info.h:114: // are not
serialized/deserialized!
On 2016/07/26 17:11:56, johnme wrote:
> On 2016/07/22 12:17:04, Peter Beverloo wrote:
> > s/!/\./
> 
> Done.

No you didn't?

https://codereview.chromium.org/2111973002/diff/100001/chrome/browser/browser...
File chrome/browser/browser_process_impl.cc (right):

https://codereview.chromium.org/2111973002/diff/100001/chrome/browser/browser...
chrome/browser/browser_process_impl.cc:1185:
gcm::CategoryForSubtypes(local_state()),
nit: as discussed, name this method GetProductCategoryForSubtypes and the
property itself product_category_for_subtypes.

https://codereview.chromium.org/2111973002/diff/100001/chrome/browser/service...
File chrome/browser/services/gcm/gcm_product_util.cc (right):

https://codereview.chromium.org/2111973002/diff/100001/chrome/browser/service...
chrome/browser/services/gcm/gcm_product_util.cc:23: for (size_t i = 0; i <
in.size(); i++) {
nit: can modernize and reduce indexed array access by using a c++11 for.

https://codereview.chromium.org/2111973002/diff/100001/chrome/browser/service...
chrome/browser/services/gcm/gcm_product_util.cc:43: std::string ns = product ==
"chromium" ? "org" : "com";
This still feels wrong to me, but I don't know what else to suggest :-(. I guess
it'll work in the 80% case..

https://codereview.chromium.org/2111973002/diff/100001/chrome/browser/service...
File chrome/browser/services/gcm/gcm_product_util.h (right):

https://codereview.chromium.org/2111973002/diff/100001/chrome/browser/service...
chrome/browser/services/gcm/gcm_product_util.h:18: std::string
CategoryForSubtypes(PrefService* prefs);
nit: document the method here, not in the implementation file

https://codereview.chromium.org/2111973002/diff/100001/components/gcm_driver/...
File components/gcm_driver/gcm_client_impl.cc (right):

https://codereview.chromium.org/2111973002/diff/100001/components/gcm_driver/...
components/gcm_driver/gcm_client_impl.cc:254: token_a->scope == token_b->scope;
nit: Could we logically separate these checks for readability?

====
  if (!token_a || !token_b)
    return false;

  if (token_a->app_id != token_b->app_id ||
      token_a->authorized_entity != token_b->authorized_entity ||
      token_a->scope != token_b->scope) {
    return false;
  }

  return token_a->use_subtype != token_b->use_subtype;
====

https://codereview.chromium.org/2111973002/diff/100001/components/gcm_driver/...
components/gcm_driver/gcm_client_impl.cc:1319: // TODO(johnme): Get FCM to send
the subtype as a trusted google.subtype key.
This is rather passive-aggressive :-) What about:

  // TODO(johnme): Remove this limitation when GCM sends us the subtype in a
  // field that's guaranteed to be trusted. crbug.com/123456

https://codereview.chromium.org/2111973002/diff/100001/components/gcm_driver/...
components/gcm_driver/gcm_client_impl.cc:1328: // check, indeed we cannot, since
category is fixed to the true package name).
nit: avoid use of "we" (3x) in comments... All of these can easily be rephrased,
making the comments shorter too.

https://codereview.chromium.org/2111973002/diff/100001/components/gcm_driver/...
File components/gcm_driver/instance_id/fake_gcm_driver_for_instance_id.cc
(right):

https://codereview.chromium.org/2111973002/diff/100001/components/gcm_driver/...
components/gcm_driver/instance_id/fake_gcm_driver_for_instance_id.cc:22:
authorized_entity + ',' + scope;
nit: consider using:

  return base::StringPrintf(
      "%d,%s,%s,%s", use_subtype, app_id, authorized_entity, scope);

https://codereview.chromium.org/2111973002/diff/100001/ios/chrome/browser/app...
File ios/chrome/browser/application_context_impl.cc (right):

https://codereview.chromium.org/2111973002/diff/100001/ios/chrome/browser/app...
ios/chrome/browser/application_context_impl.cc:344: // category to FCM as we
originally passed when registering it.
nit: avoid "we" here too (2x)

[johnme, 2016-08-04 17:47:14]

Addressed nits - thanks!

https://codereview.chromium.org/2111973002/diff/80001/components/gcm_driver/r...
File components/gcm_driver/registration_info.h (right):

https://codereview.chromium.org/2111973002/diff/80001/components/gcm_driver/r...
components/gcm_driver/registration_info.h:114: // are not
serialized/deserialized!
On 2016/07/28 12:34:09, Peter Beverloo wrote:
> On 2016/07/26 17:11:56, johnme wrote:
> > On 2016/07/22 12:17:04, Peter Beverloo wrote:
> > > s/!/\./
> > 
> > Done.
> 
> No you didn't?

Blargh. Forgot to hit save :(

https://codereview.chromium.org/2111973002/diff/100001/chrome/browser/browser...
File chrome/browser/browser_process_impl.cc (right):

https://codereview.chromium.org/2111973002/diff/100001/chrome/browser/browser...
chrome/browser/browser_process_impl.cc:1185:
gcm::CategoryForSubtypes(local_state()),
On 2016/07/28 12:34:09, Peter Beverloo wrote:
> nit: as discussed, name this method GetProductCategoryForSubtypes and the
> property itself product_category_for_subtypes.

Done.

https://codereview.chromium.org/2111973002/diff/100001/chrome/browser/service...
File chrome/browser/services/gcm/gcm_product_util.cc (right):

https://codereview.chromium.org/2111973002/diff/100001/chrome/browser/service...
chrome/browser/services/gcm/gcm_product_util.cc:23: for (size_t i = 0; i <
in.size(); i++) {
On 2016/07/28 12:34:09, Peter Beverloo wrote:
> nit: can modernize and reduce indexed array access by using a c++11 for.

Done. Serve me right for copy-pasting from base/ :p

https://codereview.chromium.org/2111973002/diff/100001/chrome/browser/service...
chrome/browser/services/gcm/gcm_product_util.cc:43: std::string ns = product ==
"chromium" ? "org" : "com";
On 2016/07/28 12:34:09, Peter Beverloo wrote:
> This still feels wrong to me, but I don't know what else to suggest :-(. I
guess
> it'll work in the 80% case..

Acknowledged. Since this is now in the chrome/ layer, it's reasonable to expect
that non-Chrome embedders will fork this anyway.

https://codereview.chromium.org/2111973002/diff/100001/chrome/browser/service...
File chrome/browser/services/gcm/gcm_product_util.h (right):

https://codereview.chromium.org/2111973002/diff/100001/chrome/browser/service...
chrome/browser/services/gcm/gcm_product_util.h:18: std::string
CategoryForSubtypes(PrefService* prefs);
On 2016/07/28 12:34:09, Peter Beverloo wrote:
> nit: document the method here, not in the implementation file

Done.

https://codereview.chromium.org/2111973002/diff/100001/components/gcm_driver/...
File components/gcm_driver/gcm_client_impl.cc (right):

https://codereview.chromium.org/2111973002/diff/100001/components/gcm_driver/...
components/gcm_driver/gcm_client_impl.cc:254: token_a->scope == token_b->scope;
On 2016/07/28 12:34:10, Peter Beverloo wrote:
> nit: Could we logically separate these checks for readability?
> 
> ====
>   if (!token_a || !token_b)
>     return false;
> 
>   if (token_a->app_id != token_b->app_id ||
>       token_a->authorized_entity != token_b->authorized_entity ||
>       token_a->scope != token_b->scope) {
>     return false;
>   }
> 
>   return token_a->use_subtype != token_b->use_subtype;
> ====

Done.

https://codereview.chromium.org/2111973002/diff/100001/components/gcm_driver/...
components/gcm_driver/gcm_client_impl.cc:1319: // TODO(johnme): Get FCM to send
the subtype as a trusted google.subtype key.
On 2016/07/28 12:34:10, Peter Beverloo wrote:
> This is rather passive-aggressive :-) What about:
> 
>   // TODO(johnme): Remove this limitation when GCM sends us the subtype in a
>   // field that's guaranteed to be trusted. crbug.com/123456

I clicked on that crbug link! Done.

https://codereview.chromium.org/2111973002/diff/100001/components/gcm_driver/...
components/gcm_driver/gcm_client_impl.cc:1328: // check, indeed we cannot, since
category is fixed to the true package name).
On 2016/07/28 12:34:10, Peter Beverloo wrote:
> nit: avoid use of "we" (3x) in comments... All of these can easily be
rephrased,
> making the comments shorter too.

Removed we's. I can't find ways to shorten these comments without losing clarity
though.

https://codereview.chromium.org/2111973002/diff/100001/components/gcm_driver/...
File components/gcm_driver/instance_id/fake_gcm_driver_for_instance_id.cc
(right):

https://codereview.chromium.org/2111973002/diff/100001/components/gcm_driver/...
components/gcm_driver/instance_id/fake_gcm_driver_for_instance_id.cc:22:
authorized_entity + ',' + scope;
On 2016/07/28 12:34:10, Peter Beverloo wrote:
> nit: consider using:
> 
>   return base::StringPrintf(
>       "%d,%s,%s,%s", use_subtype, app_id, authorized_entity, scope);

Done, though I had to use .c_str() x3 since StringPrintf doesn't support
std::string.

https://codereview.chromium.org/2111973002/diff/100001/ios/chrome/browser/app...
File ios/chrome/browser/application_context_impl.cc (right):

https://codereview.chromium.org/2111973002/diff/100001/ios/chrome/browser/app...
ios/chrome/browser/application_context_impl.cc:344: // category to FCM as we
originally passed when registering it.
On 2016/07/28 12:34:10, Peter Beverloo wrote:
> nit: avoid "we" here too (2x)

Done.

[johnme, 2016-08-05 13:15:45]

The CQ bit was checked by johnme@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-05 13:16:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-05 13:26:16]

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

[commit-bot: I haz the power, 2016-08-05 13:26:17]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[johnme, 2016-08-05 13:31:12]

Description was changed from

==========
Add support for GCM subtypes to desktop Instance ID implementation

Adds support for subtypes to the desktop Instance ID
implementation. A follow-on patch will switch Web Push
to use subtypes on desktop. Chrome Apps/Extensions will
be unchanged.

The main benefit will be consistency between desktop
and Android - both will be able to use subtypes for the
app_id, rather than sending app_id to GCM as the
category field (package name) on desktop only. This will
avoid spamming GCM's category column with many unique
values (since each app_id that Push generates contains
a random GUID).

Part of a series of patches:
1. https://codereview.chromium.org/1832833002 adds InstanceIDWithSubtype
2. https://codereview.chromium.org/1830983002 adds JNI bindings
3. https://codereview.chromium.org/1829023002 adds fake and test
4. https://codereview.chromium.org/1899753002 fixes strict mode violations
5. https://codereview.chromium.org/1854093002 enables InstanceID by default
6. https://codereview.chromium.org/1953273002 extends the GCMKeyStore
7. https://codereview.chromium.org/1923953002 integrates IIDs with crypto
8. this patch
9. https://codereview.chromium.org/1851423003 switches Push to InstanceIDs

Also depends on
- https://codereview.chromium.org/2148163003 fix test asserts

BUG=533498,589461
==========

to

==========
Add support for GCM subtypes to desktop Instance ID implementation

Adds support for subtypes to the desktop Instance ID
implementation. A follow-on patch will switch Web Push
to use subtypes on desktop. Chrome Apps/Extensions will
be unchanged.

The main benefit will be consistency between desktop
and Android - both will be able to use subtypes for the
app_id, rather than sending app_id to GCM as the
category field (package name) on desktop only. This will
avoid spamming GCM's category column with many unique
values (since each app_id that Push generates contains
a random GUID).

Part of a series of patches:
1. https://codereview.chromium.org/1832833002 adds InstanceIDWithSubtype
2. https://codereview.chromium.org/1830983002 adds JNI bindings
3. https://codereview.chromium.org/1829023002 adds fake and test
4. https://codereview.chromium.org/1899753002 fixes strict mode violations
5. https://codereview.chromium.org/1854093002 enables InstanceID by default
6. https://codereview.chromium.org/1953273002 extends the GCMKeyStore
7. https://codereview.chromium.org/1923953002 integrates IIDs with crypto
8. this patch
9. https://codereview.chromium.org/1851423003 switches Push to InstanceIDs

Also depends on
- https://codereview.chromium.org/2148163003 fix test asserts
- https://codereview.chromium.org/2217803003 EXPECT_DCHECK

BUG=533498,589461
==========

[johnme, 2016-08-05 13:57:33]

The CQ bit was checked by johnme@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-05 13:57:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[johnme, 2016-08-05 14:23:10]

johnme@chromium.org changed reviewers:
+ droger@chromium.org, thestig@chromium.org

[johnme, 2016-08-05 14:23:11]

droger, please review:
- ios/chrome/browser/application_context_impl.cc

thestig, please review:
- chrome/browser/browser_process_impl.cc
- chrome/browser/extensions/
- chrome/browser/prefs/browser_prefs.cc

Thanks!

[commit-bot: I haz the power, 2016-08-05 14:33:14]

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

[commit-bot: I haz the power, 2016-08-05 14:33:16]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Lei Zhang, 2016-08-05 15:37:08]

I see red bots.

https://codereview.chromium.org/2111973002/diff/140001/chrome/browser/extensi...
File chrome/browser/extensions/extension_gcm_app_handler_unittest.cc (right):

https://codereview.chromium.org/2111973002/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/extension_gcm_app_handler_unittest.cc:38: #include
"chrome/common/chrome_version.h"
ditto ditto

https://codereview.chromium.org/2111973002/diff/140001/chrome/browser/service...
File chrome/browser/services/gcm/gcm_profile_service_factory.cc (right):

https://codereview.chromium.org/2111973002/diff/140001/chrome/browser/service...
chrome/browser/services/gcm/gcm_profile_service_factory.cc:24: #include
"chrome/common/chrome_version.h"
not needed?

https://codereview.chromium.org/2111973002/diff/140001/chrome/browser/service...
File chrome/browser/services/gcm/gcm_profile_service_unittest.cc (right):

https://codereview.chromium.org/2111973002/diff/140001/chrome/browser/service...
chrome/browser/services/gcm/gcm_profile_service_unittest.cc:21: #include
"chrome/common/chrome_version.h"
ditto

[johnme, 2016-08-08 12:52:44]

The CQ bit was checked by johnme@chromium.org

[johnme, 2016-08-08 12:52:45]

The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org
Link to the patchset: https://codereview.chromium.org/2111973002/#ps160001
(title: "Fix thestig nits and Chrome OS compile")

[commit-bot: I haz the power, 2016-08-08 12:52:57]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[johnme, 2016-08-08 12:53:08]

The CQ bit was unchecked by johnme@chromium.org

[johnme, 2016-08-08 12:53:30]

The CQ bit was checked by johnme@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-08 12:53:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-08 14:07:35]

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

[commit-bot: I haz the power, 2016-08-08 14:07:36]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[johnme, 2016-08-08 18:22:05]

> I see red bots.

I fixed the Chrome OS bots that were red; now an Android one seems to be red,
but I'll fix that too before landing :)

https://codereview.chromium.org/2111973002/diff/140001/chrome/browser/extensi...
File chrome/browser/extensions/extension_gcm_app_handler_unittest.cc (right):

https://codereview.chromium.org/2111973002/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/extension_gcm_app_handler_unittest.cc:38: #include
"chrome/common/chrome_version.h"
On 2016/08/05 15:37:08, Lei Zhang wrote:
> ditto ditto

Done.

https://codereview.chromium.org/2111973002/diff/140001/chrome/browser/service...
File chrome/browser/services/gcm/gcm_profile_service_factory.cc (right):

https://codereview.chromium.org/2111973002/diff/140001/chrome/browser/service...
chrome/browser/services/gcm/gcm_profile_service_factory.cc:24: #include
"chrome/common/chrome_version.h"
On 2016/08/05 15:37:08, Lei Zhang wrote:
> not needed?

Done.

https://codereview.chromium.org/2111973002/diff/140001/chrome/browser/service...
File chrome/browser/services/gcm/gcm_profile_service_unittest.cc (right):

https://codereview.chromium.org/2111973002/diff/140001/chrome/browser/service...
chrome/browser/services/gcm/gcm_profile_service_unittest.cc:21: #include
"chrome/common/chrome_version.h"
On 2016/08/05 15:37:08, Lei Zhang wrote:
> ditto

Done.

[Lei Zhang, 2016-08-08 19:14:53]

chrome/ lgtm.

Not sure if the red Android bots are yours.

[jianli, 2016-08-08 22:11:49]

jianli@chromium.org changed reviewers:
+ jianli@chromium.org

[jianli, 2016-08-08 22:11:51]

I've not read all the codes yet. But based on my 1st read, it seems that passing
use_subtype make the whole code too complicated. Can we figure out some way to
simplify it?

Can we detect if an app_id is an extension id? Extension ID uses 32 chars from
'a' to 'p' only.

https://codereview.chromium.org/2111973002/diff/160001/chrome/browser/service...
File chrome/browser/services/gcm/gcm_product_util.cc (right):

https://codereview.chromium.org/2111973002/diff/160001/chrome/browser/service...
chrome/browser/services/gcm/gcm_product_util.cc:35:
prefs->GetString(prefs::kGCMProductCategoryForSubtypes);
Do we really need to save to prefs store? It seems that the generated category
value is fixed. Can we just construct it once and save to a static variable?

https://codereview.chromium.org/2111973002/diff/160001/components/gcm_driver/...
File components/gcm_driver/gcm_client.h (right):

https://codereview.chromium.org/2111973002/diff/160001/components/gcm_driver/...
components/gcm_driver/gcm_client.h:101: std::string
product_category_for_subtypes;
gcm_category

https://codereview.chromium.org/2111973002/diff/160001/components/gcm_driver/...
File components/gcm_driver/gcm_client_impl.cc (right):

https://codereview.chromium.org/2111973002/diff/160001/components/gcm_driver/...
components/gcm_driver/gcm_client_impl.cc:1321: // SECURITY NOTE: Subtypes
received from FCM *cannot* be trusted for
FCM => GCM

https://codereview.chromium.org/2111973002/diff/160001/components/gcm_driver/...
File components/gcm_driver/gcm_profile_service.h (right):

https://codereview.chromium.org/2111973002/diff/160001/components/gcm_driver/...
components/gcm_driver/gcm_profile_service.h:59: const std::string&
product_category_for_subtypes,
The name |product_category_for_subtypes| is not easy to understand if you just
look at this particular line of code. How about naming it as |gcm_category|?

https://codereview.chromium.org/2111973002/diff/160001/google_apis/gcm/engine...
File google_apis/gcm/engine/registration_request.cc (right):

https://codereview.chromium.org/2111973002/diff/160001/google_apis/gcm/engine...
google_apis/gcm/engine/registration_request.cc:80: const std::string& app_id,
Can we refactor this to pass category and subtype?

[johnme, 2016-08-10 13:09:55]

Thanks Jian, addressed your comments.

https://codereview.chromium.org/2111973002/diff/160001/chrome/browser/service...
File chrome/browser/services/gcm/gcm_product_util.cc (right):

https://codereview.chromium.org/2111973002/diff/160001/chrome/browser/service...
chrome/browser/services/gcm/gcm_product_util.cc:35:
prefs->GetString(prefs::kGCMProductCategoryForSubtypes);
On 2016/08/08 22:11:51, jianli wrote:
> Do we really need to save to prefs store? It seems that the generated category
> value is fixed. Can we just construct it once and save to a static variable?

If the channel changes (e.g. because a user upgrades from stable to beta keeping
the same profile) or the PRODUCT_SHORTNAME_STRING changes (e.g. because a user
switches between chromium and chrome keeping the same profile), we must continue
to use the same string as the category sent to GCM for existing tokens.
Otherwise, things like deleteToken will fail because the category will not match
the one on the server.

https://codereview.chromium.org/2111973002/diff/160001/components/gcm_driver/...
File components/gcm_driver/gcm_client.h (right):

https://codereview.chromium.org/2111973002/diff/160001/components/gcm_driver/...
components/gcm_driver/gcm_client.h:101: std::string
product_category_for_subtypes;
On 2016/08/08 22:11:51, jianli wrote:
> gcm_category

It's not just any category, it's specifically the category that represents the
product (a "package name" for Chrome itself), and should only be used when
requesting tokens where the app_id is sent as the subtype. Peter and I discussed
this at length before settling on the name product_category_for_subtypes, and I
think that's clearer than gcm_category, though perhaps there exists something
even clearer?

https://codereview.chromium.org/2111973002/diff/160001/components/gcm_driver/...
File components/gcm_driver/gcm_client_impl.cc (right):

https://codereview.chromium.org/2111973002/diff/160001/components/gcm_driver/...
components/gcm_driver/gcm_client_impl.cc:1321: // SECURITY NOTE: Subtypes
received from FCM *cannot* be trusted for
On 2016/08/08 22:11:51, jianli wrote:
> FCM => GCM

You prefer that the codebase continues to consistently refer to Firebase Cloud
Messaging as the old name, GCM? Ok, done.

https://codereview.chromium.org/2111973002/diff/160001/components/gcm_driver/...
File components/gcm_driver/gcm_profile_service.h (right):

https://codereview.chromium.org/2111973002/diff/160001/components/gcm_driver/...
components/gcm_driver/gcm_profile_service.h:59: const std::string&
product_category_for_subtypes,
On 2016/08/08 22:11:51, jianli wrote:
> The name |product_category_for_subtypes| is not easy to understand if you just
> look at this particular line of code. How about naming it as |gcm_category|?

See reply above. gcm_category would be confusing, as neither GCM registrations
nor extension IID tokens use this category; it's only for IID tokens which use
subtypes.

https://codereview.chromium.org/2111973002/diff/160001/google_apis/gcm/engine...
File google_apis/gcm/engine/registration_request.cc (right):

https://codereview.chromium.org/2111973002/diff/160001/google_apis/gcm/engine...
google_apis/gcm/engine/registration_request.cc:80: const std::string& app_id,
On 2016/08/08 22:11:51, jianli wrote:
> Can we refactor this to pass category and subtype?

Done.

[johnme, 2016-08-10 13:11:38]

On 2016/08/08 22:11:51, jianli wrote:
> I've not read all the codes yet. But based on my 1st read, it seems that
passing
> use_subtype make the whole code too complicated. Can we figure out some way to
> simplify it?
> 
> Can we detect if an app_id is an extension id? Extension ID uses 32 chars from
> 'a' to 'p' only.

Done. I'd initially avoided that for layering reasons, but it does avoid a lot
of complexity and plumbing, so I agree that it's better that way.

[droger, 2016-08-16 08:05:23]

LGTM but please consider the comment below. In general, sharing code should be
better.

https://codereview.chromium.org/2111973002/diff/200001/ios/chrome/browser/ser...
File ios/chrome/browser/services/gcm/ios_chrome_gcm_profile_service_factory.cc
(right):

https://codereview.chromium.org/2111973002/diff/200001/ios/chrome/browser/ser...
ios/chrome/browser/services/gcm/ios_chrome_gcm_profile_service_factory.cc:44:
#endif
Why is this code different on iOS? Would it make sense to use the cross platform
code instead?

[Nicolas Zea, 2016-08-17 20:54:07]

zea@chromium.org changed reviewers:
+ zea@chromium.org

[Nicolas Zea, 2016-08-17 20:54:09]

LG with some nits and a question

https://codereview.chromium.org/2111973002/diff/200001/components/gcm_driver/...
File components/gcm_driver/DEPS (right):

https://codereview.chromium.org/2111973002/diff/200001/components/gcm_driver/...
components/gcm_driver/DEPS:12: "+content/public/android/java",
My understanding was that components should never depend on content/?

https://codereview.chromium.org/2111973002/diff/200001/components/gcm_driver/...
File components/gcm_driver/gcm_client_impl.h (right):

https://codereview.chromium.org/2111973002/diff/200001/components/gcm_driver/...
components/gcm_driver/gcm_client_impl.h:291: // Fires OnMessageSendError event
on the delegate of this calss, based on the
nit: calss -> class

https://codereview.chromium.org/2111973002/diff/200001/google_apis/gcm/engine...
File google_apis/gcm/engine/gcm_request_test_base.h (right):

https://codereview.chromium.org/2111973002/diff/200001/google_apis/gcm/engine...
google_apis/gcm/engine/gcm_request_test_base.h:37: void VerifyFetcherUploadData(
optional suggestion: it might make sense to return an AssertionResult here
instead of requiring the caller to wrap the method in an
ASSERT_NO_FATAL_FAILURE. In my mind having an AssertionResult makes it clearer
that the responsibility is on the caller to handle the result (and it also makes
it easier to trace where the failure happened).

https://codereview.chromium.org/2111973002/diff/200001/google_apis/gcm/engine...
File google_apis/gcm/engine/registration_request.cc (right):

https://codereview.chromium.org/2111973002/diff/200001/google_apis/gcm/engine...
google_apis/gcm/engine/registration_request.cc:158: if
(!request_info_.subtype.empty())
nit: make this an if/else?

https://codereview.chromium.org/2111973002/diff/200001/google_apis/gcm/engine...
File google_apis/gcm/engine/unregistration_request.cc (right):

https://codereview.chromium.org/2111973002/diff/200001/google_apis/gcm/engine...
google_apis/gcm/engine/unregistration_request.cc:123:
BuildFormEncoding(kCategoryKey, request_info_.category, body);
if/else here too?

[jianli, 2016-08-17 21:12:08]

lgtm

https://codereview.chromium.org/2111973002/diff/200001/chrome/browser/service...
File chrome/browser/services/gcm/gcm_product_util.h (right):

https://codereview.chromium.org/2111973002/diff/200001/chrome/browser/service...
chrome/browser/services/gcm/gcm_product_util.h:19: // Returns a string like
"com.chrome.stable.macosx" that should be used as the
Even we need to use same category after channel change, having the old channel
name in the category sounds very confusing when we examine the data usages. Can
we skip those flexible parts? We can still keep os.

https://codereview.chromium.org/2111973002/diff/200001/google_apis/gcm/engine...
File google_apis/gcm/engine/registration_request.h (right):

https://codereview.chromium.org/2111973002/diff/200001/google_apis/gcm/engine...
google_apis/gcm/engine/registration_request.h:80: const std::string& app_id() {
return subtype.empty() ? category : subtype; }
nit: add const modifier

https://codereview.chromium.org/2111973002/diff/200001/google_apis/gcm/engine...
google_apis/gcm/engine/registration_request.h:82: // GCM category field derived
from the |app_id|.
Having "derived from the |app_id|" in this comment and next comment are
confusing. Probably you can remove both.

[johnme, 2016-08-18 17:43:22]

Thanks all! Addressed review comments.

https://codereview.chromium.org/2111973002/diff/200001/chrome/browser/service...
File chrome/browser/services/gcm/gcm_product_util.h (right):

https://codereview.chromium.org/2111973002/diff/200001/chrome/browser/service...
chrome/browser/services/gcm/gcm_product_util.h:19: // Returns a string like
"com.chrome.stable.macosx" that should be used as the
On 2016/08/17 21:12:07, jianli wrote:
> Even we need to use same category after channel change, having the old channel
> name in the category sounds very confusing when we examine the data usages.
Can
> we skip those flexible parts? We can still keep os.

Done (The assumption was that most users wouldn't change channel, but thinking
about it further I'm not longer sure that's true - a significant fraction of
beta/dev users probably did switch channels from another channel. So I've
removed the channel as suggested. I kept product_shortname though, as that
shouldn't change often, and even if it does the old name will still be useful -
unlike channel, where a wrong channel can be misleading).

https://codereview.chromium.org/2111973002/diff/200001/components/gcm_driver/...
File components/gcm_driver/DEPS (right):

https://codereview.chromium.org/2111973002/diff/200001/components/gcm_driver/...
components/gcm_driver/DEPS:12: "+content/public/android/java",
On 2016/08/17 20:54:09, Nicolas Zea wrote:
> My understanding was that components should never depend on content/?

components/README says "Individual components may additionally allow
dependencies on the content API and IPC".

components/ code used by iOS can't depend on content/, but this DEPS rule is
Android-only so that's not a problem in this case.

https://codereview.chromium.org/2111973002/diff/200001/components/gcm_driver/...
File components/gcm_driver/gcm_client_impl.h (right):

https://codereview.chromium.org/2111973002/diff/200001/components/gcm_driver/...
components/gcm_driver/gcm_client_impl.h:291: // Fires OnMessageSendError event
on the delegate of this calss, based on the
On 2016/08/17 20:54:09, Nicolas Zea wrote:
> nit: calss -> class

Done.

https://codereview.chromium.org/2111973002/diff/200001/google_apis/gcm/engine...
File google_apis/gcm/engine/gcm_request_test_base.h (right):

https://codereview.chromium.org/2111973002/diff/200001/google_apis/gcm/engine...
google_apis/gcm/engine/gcm_request_test_base.h:37: void VerifyFetcherUploadData(
On 2016/08/17 20:54:09, Nicolas Zea wrote:
> optional suggestion: it might make sense to return an AssertionResult here
> instead of requiring the caller to wrap the method in an
> ASSERT_NO_FATAL_FAILURE. In my mind having an AssertionResult makes it clearer
> that the responsibility is on the caller to handle the result

Hmm, on the other hand it seems that would make the implementation less clear,
since each of the 5 ASSERT_ statements would go from one line to two, and the
ASSERT_EQs would have to manually construct error strings, which is currently
handled for them automatically.

> (and it also makes it easier to trace where the failure happened).

Actually, perhaps the opposite. With ASSERT_NO_FATAL_FAILURE you can nest them,
i.e. if MyTest calls ASSERT_NO_FATAL_FAILURE(Foo()) where Foo itself calls
ASSERT_NO_FATAL_FAILURE(Bar()), the error message will explain the full chain
(MyTest line -> Foo line -> Bar line -> actual assertion failure). Whereas with
ASSERT_TRUE(ReturnsAssertionResult()), you only see the outermost ASSERT_TRUE in
MyTest and the innermost error message.

https://codereview.chromium.org/2111973002/diff/200001/google_apis/gcm/engine...
File google_apis/gcm/engine/registration_request.cc (right):

https://codereview.chromium.org/2111973002/diff/200001/google_apis/gcm/engine...
google_apis/gcm/engine/registration_request.cc:158: if
(!request_info_.subtype.empty())
On 2016/08/17 20:54:09, Nicolas Zea wrote:
> nit: make this an if/else?

|subtype| is an additional field that is sent if non-empty (in addition to
|category|, which is always sent); there would be nothing to do in the else
clause.

https://codereview.chromium.org/2111973002/diff/200001/google_apis/gcm/engine...
File google_apis/gcm/engine/registration_request.h (right):

https://codereview.chromium.org/2111973002/diff/200001/google_apis/gcm/engine...
google_apis/gcm/engine/registration_request.h:80: const std::string& app_id() {
return subtype.empty() ? category : subtype; }
On 2016/08/17 21:12:08, jianli wrote:
> nit: add const modifier

Done.

https://codereview.chromium.org/2111973002/diff/200001/google_apis/gcm/engine...
google_apis/gcm/engine/registration_request.h:82: // GCM category field derived
from the |app_id|.
On 2016/08/17 21:12:08, jianli wrote:
> Having "derived from the |app_id|" in this comment and next comment are
> confusing. Probably you can remove both.

Done.

https://codereview.chromium.org/2111973002/diff/200001/google_apis/gcm/engine...
File google_apis/gcm/engine/unregistration_request.cc (right):

https://codereview.chromium.org/2111973002/diff/200001/google_apis/gcm/engine...
google_apis/gcm/engine/unregistration_request.cc:123:
BuildFormEncoding(kCategoryKey, request_info_.category, body);
On 2016/08/17 20:54:09, Nicolas Zea wrote:
> if/else here too?

As above, |subtype| is an additional field that is sent if non-empty (in
addition to |category|, which is always sent); there would be nothing to do in
the else clause.

https://codereview.chromium.org/2111973002/diff/200001/ios/chrome/browser/ser...
File ios/chrome/browser/services/gcm/ios_chrome_gcm_profile_service_factory.cc
(right):

https://codereview.chromium.org/2111973002/diff/200001/ios/chrome/browser/ser...
ios/chrome/browser/services/gcm/ios_chrome_gcm_profile_service_factory.cc:44:
#endif
On 2016/08/16 08:05:23, droger wrote:
> Why is this code different on iOS? Would it make sense to use the cross
platform
> code instead?

iOS can't access the cross platform version in
chrome/browser/services/gcm/gcm_product_util.h, and it doesn't make sense to
move that to components since it contains product-specific naming logic that
other embedders might wish to change (indeed a previous reviewer made me move it
from components/ to chrome/).

Rather than duplicating the complex logic (which would be especially tricky
since it depends on PRODUCT_SHORTNAME_STRING which doesn't seem to exist on
iOS), I was able to implement a simpler form for iOS that hardcodes the names
"chrome" or "chromium" depending on the GOOGLE_CHROME_BUILD (the same way iOS
defines kProductDirName).

[Nicolas Zea, 2016-08-18 20:00:20]

lgtm

[droger, 2016-08-19 09:10:28]

https://codereview.chromium.org/2111973002/diff/200001/ios/chrome/browser/ser...
File ios/chrome/browser/services/gcm/ios_chrome_gcm_profile_service_factory.cc
(right):

https://codereview.chromium.org/2111973002/diff/200001/ios/chrome/browser/ser...
ios/chrome/browser/services/gcm/ios_chrome_gcm_profile_service_factory.cc:44:
#endif
On 2016/08/18 17:43:22, johnme wrote:
> On 2016/08/16 08:05:23, droger wrote:
> > Why is this code different on iOS? Would it make sense to use the cross
> platform
> > code instead?
> 
> iOS can't access the cross platform version in
> chrome/browser/services/gcm/gcm_product_util.h, and it doesn't make sense to
> move that to components since it contains product-specific naming logic that
> other embedders might wish to change (indeed a previous reviewer made me move
it
> from components/ to chrome/).
> 
> Rather than duplicating the complex logic (which would be especially tricky
> since it depends on PRODUCT_SHORTNAME_STRING which doesn't seem to exist on
> iOS), I was able to implement a simpler form for iOS that hardcodes the names
> "chrome" or "chromium" depending on the GOOGLE_CHROME_BUILD (the same way iOS
> defines kProductDirName).

Ok thanks, this seems reasonable indeed.

FYI there is IDS_IOS_SHORT_PRODUCT_NAME on iOS which is probably equivalent to
PRODUCT_SHORTNAME_STRING.

[johnme, 2016-08-19 19:21:40]

johnme@chromium.org changed reviewers:
+ asargent@chromium.org, tedchoc@chromium.org

[johnme, 2016-08-19 19:21:42]

asargent: please give OWNERS approval for components/gcm_driver/DEPS dep on
components/crx_file (just to validate extension IDs)

tedchoc: please give OWNERS approval for components/gcm_driver/DEPS dep on
content/public/android/java (though that hasn't actually changed, I just swapped
the ! for a +).

[johnme, 2016-08-19 19:21:54]

The CQ bit was checked by johnme@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-19 19:22:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[asargent_no_longer_on_chrome, 2016-08-19 19:56:55]

adding dep on components/crx_file lgtm

[Ted C, 2016-08-19 21:00:43]

On 2016/08/19 19:56:55, Antony Sargent wrote:
> adding dep on components/crx_file lgtm

components/gcm_driver/DEPS - lgtm

[johnme, 2016-08-19 21:30:07]

The CQ bit was unchecked by johnme@chromium.org

[johnme, 2016-08-19 21:30:19]

The CQ bit was checked by johnme@chromium.org

[johnme, 2016-08-19 21:30:19]

The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org,
thestig@chromium.org, droger@chromium.org, jianli@chromium.org
Link to the patchset: https://codereview.chromium.org/2111973002/#ps220001
(title: "Remove channel from product_category_for_subtypes, and address nits")

[commit-bot: I haz the power, 2016-08-19 21:31:12]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-19 21:50:06]

Description was changed from

==========
Add support for GCM subtypes to desktop Instance ID implementation

Adds support for subtypes to the desktop Instance ID
implementation. A follow-on patch will switch Web Push
to use subtypes on desktop. Chrome Apps/Extensions will
be unchanged.

The main benefit will be consistency between desktop
and Android - both will be able to use subtypes for the
app_id, rather than sending app_id to GCM as the
category field (package name) on desktop only. This will
avoid spamming GCM's category column with many unique
values (since each app_id that Push generates contains
a random GUID).

Part of a series of patches:
1. https://codereview.chromium.org/1832833002 adds InstanceIDWithSubtype
2. https://codereview.chromium.org/1830983002 adds JNI bindings
3. https://codereview.chromium.org/1829023002 adds fake and test
4. https://codereview.chromium.org/1899753002 fixes strict mode violations
5. https://codereview.chromium.org/1854093002 enables InstanceID by default
6. https://codereview.chromium.org/1953273002 extends the GCMKeyStore
7. https://codereview.chromium.org/1923953002 integrates IIDs with crypto
8. this patch
9. https://codereview.chromium.org/1851423003 switches Push to InstanceIDs

Also depends on
- https://codereview.chromium.org/2148163003 fix test asserts
- https://codereview.chromium.org/2217803003 EXPECT_DCHECK

BUG=533498,589461
==========

to

==========
Add support for GCM subtypes to desktop Instance ID implementation

Adds support for subtypes to the desktop Instance ID
implementation. A follow-on patch will switch Web Push
to use subtypes on desktop. Chrome Apps/Extensions will
be unchanged.

The main benefit will be consistency between desktop
and Android - both will be able to use subtypes for the
app_id, rather than sending app_id to GCM as the
category field (package name) on desktop only. This will
avoid spamming GCM's category column with many unique
values (since each app_id that Push generates contains
a random GUID).

Part of a series of patches:
1. https://codereview.chromium.org/1832833002 adds InstanceIDWithSubtype
2. https://codereview.chromium.org/1830983002 adds JNI bindings
3. https://codereview.chromium.org/1829023002 adds fake and test
4. https://codereview.chromium.org/1899753002 fixes strict mode violations
5. https://codereview.chromium.org/1854093002 enables InstanceID by default
6. https://codereview.chromium.org/1953273002 extends the GCMKeyStore
7. https://codereview.chromium.org/1923953002 integrates IIDs with crypto
8. this patch
9. https://codereview.chromium.org/1851423003 switches Push to InstanceIDs

Also depends on
- https://codereview.chromium.org/2148163003 fix test asserts
- https://codereview.chromium.org/2217803003 EXPECT_DCHECK

BUG=533498,589461
==========

[commit-bot: I haz the power, 2016-08-19 21:50:08]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2016-08-19 21:52:47]

Description was changed from

==========
Add support for GCM subtypes to desktop Instance ID implementation

Adds support for subtypes to the desktop Instance ID
implementation. A follow-on patch will switch Web Push
to use subtypes on desktop. Chrome Apps/Extensions will
be unchanged.

The main benefit will be consistency between desktop
and Android - both will be able to use subtypes for the
app_id, rather than sending app_id to GCM as the
category field (package name) on desktop only. This will
avoid spamming GCM's category column with many unique
values (since each app_id that Push generates contains
a random GUID).

Part of a series of patches:
1. https://codereview.chromium.org/1832833002 adds InstanceIDWithSubtype
2. https://codereview.chromium.org/1830983002 adds JNI bindings
3. https://codereview.chromium.org/1829023002 adds fake and test
4. https://codereview.chromium.org/1899753002 fixes strict mode violations
5. https://codereview.chromium.org/1854093002 enables InstanceID by default
6. https://codereview.chromium.org/1953273002 extends the GCMKeyStore
7. https://codereview.chromium.org/1923953002 integrates IIDs with crypto
8. this patch
9. https://codereview.chromium.org/1851423003 switches Push to InstanceIDs

Also depends on
- https://codereview.chromium.org/2148163003 fix test asserts
- https://codereview.chromium.org/2217803003 EXPECT_DCHECK

BUG=533498,589461
==========

to

==========
Add support for GCM subtypes to desktop Instance ID implementation

Adds support for subtypes to the desktop Instance ID
implementation. A follow-on patch will switch Web Push
to use subtypes on desktop. Chrome Apps/Extensions will
be unchanged.

The main benefit will be consistency between desktop
and Android - both will be able to use subtypes for the
app_id, rather than sending app_id to GCM as the
category field (package name) on desktop only. This will
avoid spamming GCM's category column with many unique
values (since each app_id that Push generates contains
a random GUID).

Part of a series of patches:
1. https://codereview.chromium.org/1832833002 adds InstanceIDWithSubtype
2. https://codereview.chromium.org/1830983002 adds JNI bindings
3. https://codereview.chromium.org/1829023002 adds fake and test
4. https://codereview.chromium.org/1899753002 fixes strict mode violations
5. https://codereview.chromium.org/1854093002 enables InstanceID by default
6. https://codereview.chromium.org/1953273002 extends the GCMKeyStore
7. https://codereview.chromium.org/1923953002 integrates IIDs with crypto
8. this patch
9. https://codereview.chromium.org/1851423003 switches Push to InstanceIDs

Also depends on
- https://codereview.chromium.org/2148163003 fix test asserts
- https://codereview.chromium.org/2217803003 EXPECT_DCHECK

BUG=533498,589461

Committed: https://crrev.com/627dc8c76a66a8c6251c6d0423e8782b101bb050
Cr-Commit-Position: refs/heads/master@{#413259}
==========

[commit-bot: I haz the power, 2016-08-19 21:52:49]

Patchset 12 (id:??) landed as
https://crrev.com/627dc8c76a66a8c6251c6d0423e8782b101bb050
Cr-Commit-Position: refs/heads/master@{#413259}