Implement InstanceIDAndroid using InstanceIDWithSubtype.java

Implement InstanceIDAndroid using InstanceIDWithSubtype.java

Replaces the previous stub implementation with a fully functional
implementation backed by InstanceIDWithSubtype.java.

Part of a series of patches:
1. https://codereview.chromium.org/1832833002 adds InstanceIDWithSubtype
2. this patch
3. https://codereview.chromium.org/1829023002 adds fake and test
4. https://codereview.chromium.org/1854093002 enables InstanceID by default
5. https://codereview.chromium.org/1851423003 switches Push to InstanceIDs

BUG=589461
Committed: https://crrev.com/2f8daf9d61b0bd6260dda858f852b258e03341f4
Cr-Commit-Position: refs/heads/master@{#387646}

[johnme, 2016-03-24 14:14:43]

Description was changed from

==========
Implement InstanceIDAndroid using InstanceIDWithSubtype.java

Replaces the previous stub implementation with a fully functional
implementation backed by InstanceIDWithSubtype.java.

Part of a series of patches:
1. https://codereview.chromium.org/1832833002 adds InstanceIDWithSubtype
2. this patch
3. TODO (adds fake and test)

BUG=589461
==========

to

==========
Implement InstanceIDAndroid using InstanceIDWithSubtype.java

Replaces the previous stub implementation with a fully functional
implementation backed by InstanceIDWithSubtype.java.

Part of a series of patches:
1. https://codereview.chromium.org/1832833002 adds InstanceIDWithSubtype
2. this patch
3. https://codereview.chromium.org/1829023002 adds fake and test

BUG=589461
==========

[johnme, 2016-04-04 12:19:21]

Description was changed from

==========
Implement InstanceIDAndroid using InstanceIDWithSubtype.java

Replaces the previous stub implementation with a fully functional
implementation backed by InstanceIDWithSubtype.java.

Part of a series of patches:
1. https://codereview.chromium.org/1832833002 adds InstanceIDWithSubtype
2. this patch
3. https://codereview.chromium.org/1829023002 adds fake and test

BUG=589461
==========

to

==========
Implement InstanceIDAndroid using InstanceIDWithSubtype.java

Replaces the previous stub implementation with a fully functional
implementation backed by InstanceIDWithSubtype.java.

Part of a series of patches:
1. https://codereview.chromium.org/1832833002 adds InstanceIDWithSubtype
2. this patch
3. https://codereview.chromium.org/1829023002 adds fake and test
4. https://codereview.chromium.org/1854093002 enables InstanceID by default
5. https://codereview.chromium.org/1851423003 switches Push to InstanceIDs

BUG=589461
==========

[johnme, 2016-04-05 19:10:54]

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

[jianli, 2016-04-08 23:20:00]

lgtm

https://codereview.chromium.org/1830983002/diff/40001/components/gcm_driver/i...
File components/gcm_driver/instance_id/instance_id.h (right):

https://codereview.chromium.org/1830983002/diff/40001/components/gcm_driver/i...
components/gcm_driver/instance_id/instance_id.h:60: //            Must outlive
this class. On Android, this can be null instead.
Probably it is better to refactor this in order to avoid having different value
for a parameter. I am thinking:
  ... Create(const std::string& app_id, gcm::GCMDriver* gcm_driver)

InstanceIDAndroid could ignore gcm_driver parameter while InstanceIDImpl could
get InstanceIDHandler out of it.

[Peter Beverloo, 2016-04-11 14:05:30]

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

[Peter Beverloo, 2016-04-11 14:05:32]

https://codereview.chromium.org/1830983002/diff/40001/chrome/browser/BUILD.gn
File chrome/browser/BUILD.gn (right):

https://codereview.chromium.org/1830983002/diff/40001/chrome/browser/BUILD.gn...
chrome/browser/BUILD.gn:766: "//components/gcm_driver/instance_id",
Why is this necessary? The deps array defined on line 98 already declares this
dependency for all platforms, and I don't think we remove it anywhere for
Android.

https://codereview.chromium.org/1830983002/diff/40001/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/1830983002/diff/40001/chrome/chrome_browser.g...
chrome/chrome_browser.gypi:3753: '../components/components.gyp:instance_id',
dito

https://codereview.chromium.org/1830983002/diff/40001/components/gcm_driver.gypi
File components/gcm_driver.gypi (right):

https://codereview.chromium.org/1830983002/diff/40001/components/gcm_driver.g...
components/gcm_driver.gypi:335: '../base/base.gyp:base',
Can this be :base_java?

(If it can, mind updating :308 as well and removing the void comment on :309?)

https://codereview.chromium.org/1830983002/diff/40001/components/gcm_driver/i...
File components/gcm_driver/instance_id/android/component_jni_registrar.h
(right):

https://codereview.chromium.org/1830983002/diff/40001/components/gcm_driver/i...
components/gcm_driver/instance_id/android/component_jni_registrar.h:12:
namespace android {
nit: no blank line between the namespace declarations (nor when closing them).
In the .cc too.

https://google.github.io/styleguide/cppguide.html#Namespace_Formatting

https://codereview.chromium.org/1830983002/diff/40001/components/gcm_driver/i...
File
components/gcm_driver/instance_id/android/java/src/org/chromium/components/gcm_driver/instance_id/InstanceIDWithSubtype.java
(right):

https://codereview.chromium.org/1830983002/diff/40001/components/gcm_driver/i...
components/gcm_driver/instance_id/android/java/src/org/chromium/components/gcm_driver/instance_id/InstanceIDWithSubtype.java:24:
@JNINamespace("instance_id")
I would be very much in favor to split this up in two files. One having the
InstanceIDWithSubtype class that extends InstanceID, and one that adds the JNI
and does any asynchronous operation we may have to do ourselves.

It's rather convoluted this way, and the InstanceID base class has lots of
similar methods that, in some cases, behave *slightly* differently. It would
simplify the code a lot.

https://codereview.chromium.org/1830983002/diff/40001/components/gcm_driver/i...
components/gcm_driver/instance_id/android/java/src/org/chromium/components/gcm_driver/instance_id/InstanceIDWithSubtype.java:89:
private void getToken(final int requestId, final String authorizedEntity, final
String scope,
These methods need documentation, specifically because you're doing asynchronous
operations and you're not mentioning why. It may be clear to us now, but it
won't be for the next reader.

https://codereview.chromium.org/1830983002/diff/40001/components/gcm_driver/i...
components/gcm_driver/instance_id/android/java/src/org/chromium/components/gcm_driver/instance_id/InstanceIDWithSubtype.java:93:
extras.putString(extrasStrings[i - 1], extrasStrings[i]);
nit: why start at 1 and then use [i - 1 / i] while you could start at 0 and use
[i / i + 1]? Seems clearer.

https://codereview.chromium.org/1830983002/diff/40001/components/gcm_driver/i...
components/gcm_driver/instance_id/android/java/src/org/chromium/components/gcm_driver/instance_id/InstanceIDWithSubtype.java:101:
return "";
Why is it OK to silently drop exceptions? Shouldn't we at least write them to a
log?

https://codereview.chromium.org/1830983002/diff/40001/components/gcm_driver/i...
components/gcm_driver/instance_id/android/java/src/org/chromium/components/gcm_driver/instance_id/InstanceIDWithSubtype.java:106:
nativeDidGetToken(mNativeInstanceIDAndroid, requestId, token);
What guarantees that mNativeInstanceIDAndroid is alive when calling this method?

https://codereview.chromium.org/1830983002/diff/40001/components/gcm_driver/i...
File components/gcm_driver/instance_id/instance_id_android.cc (right):

https://codereview.chromium.org/1830983002/diff/40001/components/gcm_driver/i...
components/gcm_driver/instance_id/instance_id_android.cc:40: // TODO(johnme):
Passing app_id as subtype isn't always correct.
nit: Either remove or clarify the TODO, this is very unactionable.

https://codereview.chromium.org/1830983002/diff/40001/components/gcm_driver/i...
components/gcm_driver/instance_id/instance_id_android.cc:59: callback.Run(id);
nit: for this one and the callbacks on lines :70 and :72, consider posting a
task to avoid people unintentionally relying on synchronous behaviour.

https://codereview.chromium.org/1830983002/diff/40001/components/gcm_driver/i...
components/gcm_driver/instance_id/instance_id_android.cc:69: if
(!creation_time_unix_ms) {
Please document when this would happen.

https://codereview.chromium.org/1830983002/diff/40001/components/gcm_driver/i...
components/gcm_driver/instance_id/instance_id_android.cc:135: token,
token.empty() ? InstanceID::UNKNOWN_ERROR : InstanceID::SUCCESS);
What happens when we get a token whilst the device is offline?

https://codereview.chromium.org/1830983002/diff/40001/components/gcm_driver/i...
File components/gcm_driver/instance_id/instance_id_android.h (right):

https://codereview.chromium.org/1830983002/diff/40001/components/gcm_driver/i...
components/gcm_driver/instance_id/instance_id_android.h:28: static bool
RegisterBindings(JNIEnv* env);
nit: RegisterJni for consistency. Should probably upload the GCMDriverAndroid
too.

[Peter Beverloo, 2016-04-11 14:28:05]

https://codereview.chromium.org/1830983002/diff/40001/components/gcm_driver/i...
File
components/gcm_driver/instance_id/android/java/src/org/chromium/components/gcm_driver/instance_id/InstanceIDWithSubtype.java
(right):

https://codereview.chromium.org/1830983002/diff/40001/components/gcm_driver/i...
components/gcm_driver/instance_id/android/java/src/org/chromium/components/gcm_driver/instance_id/InstanceIDWithSubtype.java:79:
return super.getId();
Both getId() and getCreationTime() may hit SharedPreferences on first use. (They
are lazily initialized.) I think that could trigger a StrictMode violation as
well— why don't these methods have to be asynchronous?

[johnme, 2016-04-13 11:42:12]

Addressed most review comments, except splitting off the JNI parts of
InstanceIDWithSubtype to a separate class for clarity, and documenting them.
I'll do those next.

https://codereview.chromium.org/1830983002/diff/40001/chrome/browser/BUILD.gn
File chrome/browser/BUILD.gn (right):

https://codereview.chromium.org/1830983002/diff/40001/chrome/browser/BUILD.gn...
chrome/browser/BUILD.gn:766: "//components/gcm_driver/instance_id",
On 2016/04/11 14:05:31, Peter Beverloo wrote:
> Why is this necessary? The deps array defined on line 98 already declares this
> dependency for all platforms, and I don't think we remove it anywhere for
> Android.

Removed

https://codereview.chromium.org/1830983002/diff/40001/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/1830983002/diff/40001/chrome/chrome_browser.g...
chrome/chrome_browser.gypi:3753: '../components/components.gyp:instance_id',
On 2016/04/11 14:05:31, Peter Beverloo wrote:
> dito

Removed

https://codereview.chromium.org/1830983002/diff/40001/components/gcm_driver.gypi
File components/gcm_driver.gypi (right):

https://codereview.chromium.org/1830983002/diff/40001/components/gcm_driver.g...
components/gcm_driver.gypi:335: '../base/base.gyp:base',
On 2016/04/11 14:05:31, Peter Beverloo wrote:
> Can this be :base_java?
> 
> (If it can, mind updating :308 as well and removing the void comment on :309?)

Done.

https://codereview.chromium.org/1830983002/diff/40001/components/gcm_driver/i...
File components/gcm_driver/instance_id/android/component_jni_registrar.h
(right):

https://codereview.chromium.org/1830983002/diff/40001/components/gcm_driver/i...
components/gcm_driver/instance_id/android/component_jni_registrar.h:12:
namespace android {
On 2016/04/11 14:05:31, Peter Beverloo wrote:
> nit: no blank line between the namespace declarations (nor when closing them).
> In the .cc too.
> 
> https://google.github.io/styleguide/cppguide.html#Namespace_Formatting

Done.

https://codereview.chromium.org/1830983002/diff/40001/components/gcm_driver/i...
File
components/gcm_driver/instance_id/android/java/src/org/chromium/components/gcm_driver/instance_id/InstanceIDWithSubtype.java
(right):

https://codereview.chromium.org/1830983002/diff/40001/components/gcm_driver/i...
components/gcm_driver/instance_id/android/java/src/org/chromium/components/gcm_driver/instance_id/InstanceIDWithSubtype.java:79:
return super.getId();
On 2016/04/11 14:28:05, Peter Beverloo wrote:
> Both getId() and getCreationTime() may hit SharedPreferences on first use.
(They
> are lazily initialized.) I think that could trigger a StrictMode violation as
> well— why don't these methods have to be asynchronous?

They can indeed trigger StrictMode violations; I have a follow-on patch that
makes them async as well.

https://codereview.chromium.org/1830983002/diff/40001/components/gcm_driver/i...
components/gcm_driver/instance_id/android/java/src/org/chromium/components/gcm_driver/instance_id/InstanceIDWithSubtype.java:93:
extras.putString(extrasStrings[i - 1], extrasStrings[i]);
On 2016/04/11 14:05:31, Peter Beverloo wrote:
> nit: why start at 1 and then use [i - 1 / i] while you could start at 0 and
use
> [i / i + 1]? Seems clearer.

Done (it was so the loop condition can just be i < extraStrings.length, and in
the case of an odd number of strings it'd avoid triggering
ArrayIndexOutOfBoundsException. But on second thought it's probably better to
throw if the number of strings is odd which is an invalid input; I've added an
assert as documentation too).

https://codereview.chromium.org/1830983002/diff/40001/components/gcm_driver/i...
components/gcm_driver/instance_id/android/java/src/org/chromium/components/gcm_driver/instance_id/InstanceIDWithSubtype.java:101:
return "";
On 2016/04/11 14:05:31, Peter Beverloo wrote:
> Why is it OK to silently drop exceptions? Shouldn't we at least write them to
a
> log?

IOException usually just means the device was offline. Doesn't seem worth
spamming a log?

https://codereview.chromium.org/1830983002/diff/40001/components/gcm_driver/i...
components/gcm_driver/instance_id/android/java/src/org/chromium/components/gcm_driver/instance_id/InstanceIDWithSubtype.java:106:
nativeDidGetToken(mNativeInstanceIDAndroid, requestId, token);
On 2016/04/11 14:05:31, Peter Beverloo wrote:
> What guarantees that mNativeInstanceIDAndroid is alive when calling this
method?

Fixed.

https://codereview.chromium.org/1830983002/diff/40001/components/gcm_driver/i...
File components/gcm_driver/instance_id/instance_id.h (right):

https://codereview.chromium.org/1830983002/diff/40001/components/gcm_driver/i...
components/gcm_driver/instance_id/instance_id.h:60: //            Must outlive
this class. On Android, this can be null instead.
On 2016/04/08 23:20:00, jianli wrote:
> Probably it is better to refactor this in order to avoid having different
value
> for a parameter. I am thinking:
>   ... Create(const std::string& app_id, gcm::GCMDriver* gcm_driver)
> 
> InstanceIDAndroid could ignore gcm_driver parameter while InstanceIDImpl could
> get InstanceIDHandler out of it.

Hmm, Peter suggested we pass in only the InstanceIDHandler in
https://codereview.chromium.org/1785613006, to insulate InstanceID from the
implementation detail that InstanceIDHandler happens to be obtained from &
implemented by GCMDriver (which is a different component). That still seems a
good reason.

This patch doesn't change this behaviour anyway (it was already null on Android,
I'm just documenting that fact).

https://codereview.chromium.org/1830983002/diff/40001/components/gcm_driver/i...
File components/gcm_driver/instance_id/instance_id_android.cc (right):

https://codereview.chromium.org/1830983002/diff/40001/components/gcm_driver/i...
components/gcm_driver/instance_id/instance_id_android.cc:40: // TODO(johnme):
Passing app_id as subtype isn't always correct.
On 2016/04/11 14:05:31, Peter Beverloo wrote:
> nit: Either remove or clarify the TODO, this is very unactionable.

Done.

https://codereview.chromium.org/1830983002/diff/40001/components/gcm_driver/i...
components/gcm_driver/instance_id/instance_id_android.cc:59: callback.Run(id);
On 2016/04/11 14:05:31, Peter Beverloo wrote:
> nit: for this one and the callbacks on lines :70 and :72, consider posting a
> task to avoid people unintentionally relying on synchronous behaviour.

Done.

https://codereview.chromium.org/1830983002/diff/40001/components/gcm_driver/i...
components/gcm_driver/instance_id/instance_id_android.cc:69: if
(!creation_time_unix_ms) {
On 2016/04/11 14:05:31, Peter Beverloo wrote:
> Please document when this would happen.

Added comment.

https://codereview.chromium.org/1830983002/diff/40001/components/gcm_driver/i...
components/gcm_driver/instance_id/instance_id_android.cc:135: token,
token.empty() ? InstanceID::UNKNOWN_ERROR : InstanceID::SUCCESS);
On 2016/04/11 14:05:31, Peter Beverloo wrote:
> What happens when we get a token whilst the device is offline?

The token is the empty string, and the status is UNKNOWN_ERROR

https://codereview.chromium.org/1830983002/diff/40001/components/gcm_driver/i...
File components/gcm_driver/instance_id/instance_id_android.h (right):

https://codereview.chromium.org/1830983002/diff/40001/components/gcm_driver/i...
components/gcm_driver/instance_id/instance_id_android.h:28: static bool
RegisterBindings(JNIEnv* env);
On 2016/04/11 14:05:31, Peter Beverloo wrote:
> nit: RegisterJni for consistency. Should probably upload the GCMDriverAndroid
> too.

Done (assuming you mean s/upload/rename/)

[Peter Beverloo, 2016-04-13 13:21:59]

https://codereview.chromium.org/1830983002/diff/40001/components/gcm_driver/i...
File
components/gcm_driver/instance_id/android/java/src/org/chromium/components/gcm_driver/instance_id/InstanceIDWithSubtype.java
(right):

https://codereview.chromium.org/1830983002/diff/40001/components/gcm_driver/i...
components/gcm_driver/instance_id/android/java/src/org/chromium/components/gcm_driver/instance_id/InstanceIDWithSubtype.java:101:
return "";
On 2016/04/13 11:42:12, johnme wrote:
> On 2016/04/11 14:05:31, Peter Beverloo wrote:
> > Why is it OK to silently drop exceptions? Shouldn't we at least write them
to
> a
> > log?
> 
> IOException usually just means the device was offline. Doesn't seem worth
> spamming a log?

The user engaged in an action (subscribing for push through a website) that now
cannot complete. I'm sure that we'll get a bug about "why doesn't it work" at
some point...

https://codereview.chromium.org/1830983002/diff/60001/components/gcm_driver/i...
File
components/gcm_driver/instance_id/android/java/src/org/chromium/components/gcm_driver/instance_id/InstanceIDWithSubtype.java
(right):

https://codereview.chromium.org/1830983002/diff/60001/components/gcm_driver/i...
components/gcm_driver/instance_id/android/java/src/org/chromium/components/gcm_driver/instance_id/InstanceIDWithSubtype.java:62:
existing.mNativeInstanceIDAndroid = nativeInstanceIDAndroid;
The following code would now trigger the DCHECK on line 144 of
instance_id_android.cc:

  std::unique_ptr<InstanceID> instance_id(InstanceID::Create("MyID", nullptr));
  instance_id->GetToken(...);
  instance_id.reset(InstanceID::Create("MyID", nullptr));

I guess we'd always want to create a new Java instance (i.e. hit line 59 when
mNativeInstanceIDAndroid is 0)?

https://codereview.chromium.org/1830983002/diff/60001/components/gcm_driver/i...
components/gcm_driver/instance_id/android/java/src/org/chromium/components/gcm_driver/instance_id/InstanceIDWithSubtype.java:108:
assert extrasStrings.length % 2 == 0;
`assert` doesn't do anything in our Java, they're decorative only... Throw an
exception instead?

[johnme, 2016-04-13 18:46:51]

Addressed remaining review comments - thanks!

https://codereview.chromium.org/1830983002/diff/40001/components/gcm_driver/i...
File
components/gcm_driver/instance_id/android/java/src/org/chromium/components/gcm_driver/instance_id/InstanceIDWithSubtype.java
(right):

https://codereview.chromium.org/1830983002/diff/40001/components/gcm_driver/i...
components/gcm_driver/instance_id/android/java/src/org/chromium/components/gcm_driver/instance_id/InstanceIDWithSubtype.java:24:
@JNINamespace("instance_id")
On 2016/04/11 14:05:31, Peter Beverloo wrote:
> I would be very much in favor to split this up in two files. One having the
> InstanceIDWithSubtype class that extends InstanceID, and one that adds the JNI
> and does any asynchronous operation we may have to do ourselves.
> 
> It's rather convoluted this way, and the InstanceID base class has lots of
> similar methods that, in some cases, behave *slightly* differently. It would
> simplify the code a lot.

Done.

https://codereview.chromium.org/1830983002/diff/40001/components/gcm_driver/i...
components/gcm_driver/instance_id/android/java/src/org/chromium/components/gcm_driver/instance_id/InstanceIDWithSubtype.java:89:
private void getToken(final int requestId, final String authorizedEntity, final
String scope,
On 2016/04/11 14:05:31, Peter Beverloo wrote:
> These methods need documentation, specifically because you're doing
asynchronous
> operations and you're not mentioning why. It may be clear to us now, but it
> won't be for the next reader.

Done.

https://codereview.chromium.org/1830983002/diff/40001/components/gcm_driver/i...
components/gcm_driver/instance_id/android/java/src/org/chromium/components/gcm_driver/instance_id/InstanceIDWithSubtype.java:101:
return "";
On 2016/04/13 13:21:59, Peter Beverloo wrote:
> On 2016/04/13 11:42:12, johnme wrote:
> > On 2016/04/11 14:05:31, Peter Beverloo wrote:
> > > Why is it OK to silently drop exceptions? Shouldn't we at least write them
> to
> > a
> > > log?
> > 
> > IOException usually just means the device was offline. Doesn't seem worth
> > spamming a log?
> 
> The user engaged in an action (subscribing for push through a website) that
now
> cannot complete. I'm sure that we'll get a bug about "why doesn't it work" at
> some point...

I don't understand what you want. Spamming logs whenever we're offline doesn't
seem a good idea. InstanceIDAndroid::DidGetToken will return an
InstanceID::UNKNOWN_ERROR which Push will eventually handle, though perhaps a
more specific error would be useful.

https://codereview.chromium.org/1830983002/diff/60001/components/gcm_driver/i...
File
components/gcm_driver/instance_id/android/java/src/org/chromium/components/gcm_driver/instance_id/InstanceIDWithSubtype.java
(right):

https://codereview.chromium.org/1830983002/diff/60001/components/gcm_driver/i...
components/gcm_driver/instance_id/android/java/src/org/chromium/components/gcm_driver/instance_id/InstanceIDWithSubtype.java:62:
existing.mNativeInstanceIDAndroid = nativeInstanceIDAndroid;
On 2016/04/13 13:21:59, Peter Beverloo wrote:
> The following code would now trigger the DCHECK on line 144 of
> instance_id_android.cc:
> 
>   std::unique_ptr<InstanceID> instance_id(InstanceID::Create("MyID",
nullptr));
>   instance_id->GetToken(...);
>   instance_id.reset(InstanceID::Create("MyID", nullptr));
> 
> I guess we'd always want to create a new Java instance (i.e. hit line 59 when
> mNativeInstanceIDAndroid is 0)?

This is no longer a problem with the new InstanceIDBridge, which has a
many-to-one relation to InstanceID instances.

https://codereview.chromium.org/1830983002/diff/60001/components/gcm_driver/i...
components/gcm_driver/instance_id/android/java/src/org/chromium/components/gcm_driver/instance_id/InstanceIDWithSubtype.java:108:
assert extrasStrings.length % 2 == 0;
On 2016/04/13 13:21:59, Peter Beverloo wrote:
> `assert` doesn't do anything in our Java, they're decorative only... Throw an
> exception instead?

The assert is just documentation. Java will throw ArrayIndexOutOfBoundsException
for us.

[Peter Beverloo, 2016-04-14 14:30:48]

lgtm % removing getInstance() from the bridge

https://codereview.chromium.org/1830983002/diff/100001/components/gcm_driver/...
File
components/gcm_driver/instance_id/android/java/src/org/chromium/components/gcm_driver/instance_id/InstanceIDBridge.java
(right):

https://codereview.chromium.org/1830983002/diff/100001/components/gcm_driver/...
components/gcm_driver/instance_id/android/java/src/org/chromium/components/gcm_driver/instance_id/InstanceIDBridge.java:41:
InstanceID instanceID = TextUtils.isEmpty(subtype)
There's a DCHECK in the C++ code to make sure the subtype is not empty, why do
you special case for it here?

Could we simplify this class, get rid of the getInstance() method completely and
just instantiate it directly from C++? Throw an exception when |subtype| is
empty, and assign InstanceIDWithSubtype.getInstance() to mInstanceID :-).

https://codereview.chromium.org/1830983002/diff/100001/components/gcm_driver/...
components/gcm_driver/instance_id/android/java/src/org/chromium/components/gcm_driver/instance_id/InstanceIDBridge.java:59:
return mInstanceID.getId();
Any reason for not making this asynchronous immediately? If it's convenience,
I'm OK with postponing but then please add a TODO since these methods violate
the class-level documentation on line 21.

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

https://codereview.chromium.org/1830983002/diff/100001/components/gcm_driver/...
components/gcm_driver/instance_id/instance_id_android.cc:44: // Android we
cannot modify that, so we send it as the subtype field instead.
Please avoid "we" in comments. What about:

  // The |app_id| is stored in GCM's category field by the desktop InstanceID
  // implementation, but because the category is reserved for the app's package
  // name on Android the subtype field is used instead.

https://codereview.chromium.org/1830983002/diff/100001/components/gcm_driver/...
components/gcm_driver/instance_id/instance_id_android.cc:80: // called, or
deleteInstanceID has cleared it since, creation time will be 0.
Is this something we could fix on the Java side (in the bridge if you like)
instead? For example, we could implicitly call getId() if the creation time is
zero, to make sure it is initialized when returning the value.

[johnme, 2016-04-14 18:00:15]

Addressed nits - thanks!

https://codereview.chromium.org/1830983002/diff/100001/components/gcm_driver/...
File
components/gcm_driver/instance_id/android/java/src/org/chromium/components/gcm_driver/instance_id/InstanceIDBridge.java
(right):

https://codereview.chromium.org/1830983002/diff/100001/components/gcm_driver/...
components/gcm_driver/instance_id/android/java/src/org/chromium/components/gcm_driver/instance_id/InstanceIDBridge.java:41:
InstanceID instanceID = TextUtils.isEmpty(subtype)
On 2016/04/14 14:30:47, Peter Beverloo wrote:
> There's a DCHECK in the C++ code to make sure the subtype is not empty, why do
> you special case for it here?
> 
> Could we simplify this class, get rid of the getInstance() method completely
and
> just instantiate it directly from C++? Throw an exception when |subtype| is
> empty, and assign InstanceIDWithSubtype.getInstance() to mInstanceID :-).

It would be valid for Chrome components to use an empty subtype (and hence
InstanceID.getInstance) to access Chrome's default InstanceID. But since I don't
currently support that in C++, there's not much point supporting it in Java
either, so done. Our JNI generator doesn't support calling constructors from
C++, so getInstance remains (renamed to create), though simplified.

https://codereview.chromium.org/1830983002/diff/100001/components/gcm_driver/...
components/gcm_driver/instance_id/android/java/src/org/chromium/components/gcm_driver/instance_id/InstanceIDBridge.java:59:
return mInstanceID.getId();
On 2016/04/14 14:30:47, Peter Beverloo wrote:
> Any reason for not making this asynchronous immediately? If it's convenience,
> I'm OK with postponing but then please add a TODO since these methods violate
> the class-level documentation on line 21. 

Convenience (note that create will also need to be async, so it's not trivial).
Added TODOs.

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

https://codereview.chromium.org/1830983002/diff/100001/components/gcm_driver/...
components/gcm_driver/instance_id/instance_id_android.cc:44: // Android we
cannot modify that, so we send it as the subtype field instead.
On 2016/04/14 14:30:47, Peter Beverloo wrote:
> Please avoid "we" in comments. What about:
> 
>   // The |app_id| is stored in GCM's category field by the desktop InstanceID
>   // implementation, but because the category is reserved for the app's
package
>   // name on Android the subtype field is used instead.

Done.

https://codereview.chromium.org/1830983002/diff/100001/components/gcm_driver/...
components/gcm_driver/instance_id/instance_id_android.cc:80: // called, or
deleteInstanceID has cleared it since, creation time will be 0.
On 2016/04/14 14:30:47, Peter Beverloo wrote:
> Is this something we could fix on the Java side (in the bridge if you like)
> instead? For example, we could implicitly call getId() if the creation time is
> zero, to make sure it is initialized when returning the value.

This is expected behavior, that is consistent with the existing desktop
implementation. For example InstanceIDDriverTest.NewID starts with:

  // Creation time should not be set when the ID is not created.
  InstanceID* instance_id1 = driver()->GetInstanceID(kTestAppID1);
  EXPECT_TRUE(GetCreationTime(instance_id1).is_null());

[johnme, 2016-04-14 18:10:09]

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

[johnme, 2016-04-14 18:10:09]

thestig@chromium.org: Please review changes in chrome/
Thanks!

[Lei Zhang, 2016-04-14 19:58:51]

On 2016/04/14 18:10:09, johnme wrote:
> mailto:thestig@chromium.org: Please review changes in chrome/
> Thanks!

chrome/ lgtm

[Peter Beverloo, 2016-04-14 23:31:44]

still lgtm

One thought regarding making the constructor asynchronous: we can avoid a whole
bunch of hassle in C++ if we instead just initialize the InstanceID Java class
lazily, since all other operations will be asynchronous.

[johnme, 2016-04-15 11:56:30]

The CQ bit was checked by johnme@chromium.org

[johnme, 2016-04-15 11:56:31]

The patchset sent to the CQ was uploaded after l-g-t-m from jianli@chromium.org
Link to the patchset: https://codereview.chromium.org/1830983002/#ps120001
(title: "Address peter's nits")

[commit-bot: I haz the power, 2016-04-15 11:56:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1830983002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1830983002/120001

[commit-bot: I haz the power, 2016-04-15 12:12:50]

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

[commit-bot: I haz the power, 2016-04-15 12:12:51]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[johnme, 2016-04-15 17:03:39]

The CQ bit was checked by johnme@chromium.org

[johnme, 2016-04-15 17:03:39]

The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org,
thestig@chromium.org, jianli@chromium.org
Link to the patchset: https://codereview.chromium.org/1830983002/#ps140001
(title: "Add gyp dep on google_play_services_javalib")

[commit-bot: I haz the power, 2016-04-15 17:04:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1830983002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1830983002/140001

[johnme, 2016-04-15 17:11:49]

The CQ bit was checked by johnme@chromium.org

[johnme, 2016-04-15 17:11:49]

The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org,
thestig@chromium.org, jianli@chromium.org
Link to the patchset: https://codereview.chromium.org/1830983002/#ps160001
(title: "Make mNativeInstanceIDAndroid non-final")

[commit-bot: I haz the power, 2016-04-15 17:12:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1830983002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1830983002/160001

[commit-bot: I haz the power, 2016-04-15 18:17:57]

Description was changed from

==========
Implement InstanceIDAndroid using InstanceIDWithSubtype.java

Replaces the previous stub implementation with a fully functional
implementation backed by InstanceIDWithSubtype.java.

Part of a series of patches:
1. https://codereview.chromium.org/1832833002 adds InstanceIDWithSubtype
2. this patch
3. https://codereview.chromium.org/1829023002 adds fake and test
4. https://codereview.chromium.org/1854093002 enables InstanceID by default
5. https://codereview.chromium.org/1851423003 switches Push to InstanceIDs

BUG=589461
==========

to

==========
Implement InstanceIDAndroid using InstanceIDWithSubtype.java

Replaces the previous stub implementation with a fully functional
implementation backed by InstanceIDWithSubtype.java.

Part of a series of patches:
1. https://codereview.chromium.org/1832833002 adds InstanceIDWithSubtype
2. this patch
3. https://codereview.chromium.org/1829023002 adds fake and test
4. https://codereview.chromium.org/1854093002 enables InstanceID by default
5. https://codereview.chromium.org/1851423003 switches Push to InstanceIDs

BUG=589461
==========

[commit-bot: I haz the power, 2016-04-15 18:17:58]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-04-15 18:19:18]

Description was changed from

==========
Implement InstanceIDAndroid using InstanceIDWithSubtype.java

Replaces the previous stub implementation with a fully functional
implementation backed by InstanceIDWithSubtype.java.

Part of a series of patches:
1. https://codereview.chromium.org/1832833002 adds InstanceIDWithSubtype
2. this patch
3. https://codereview.chromium.org/1829023002 adds fake and test
4. https://codereview.chromium.org/1854093002 enables InstanceID by default
5. https://codereview.chromium.org/1851423003 switches Push to InstanceIDs

BUG=589461
==========

to

==========
Implement InstanceIDAndroid using InstanceIDWithSubtype.java

Replaces the previous stub implementation with a fully functional
implementation backed by InstanceIDWithSubtype.java.

Part of a series of patches:
1. https://codereview.chromium.org/1832833002 adds InstanceIDWithSubtype
2. this patch
3. https://codereview.chromium.org/1829023002 adds fake and test
4. https://codereview.chromium.org/1854093002 enables InstanceID by default
5. https://codereview.chromium.org/1851423003 switches Push to InstanceIDs

BUG=589461

Committed: https://crrev.com/2f8daf9d61b0bd6260dda858f852b258e03341f4
Cr-Commit-Position: refs/heads/master@{#387646}
==========

[commit-bot: I haz the power, 2016-04-15 18:19:19]

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