Add InstanceIDWithSubtype.java to allow an Instance ID per website

Add InstanceIDWithSubtype.java to allow an Instance ID per website

Uses the protected constructor to create a subtype-specific InstanceID.

Part of a series of patches:
1. this patch
2. https://codereview.chromium.org/1830983002 adds JNI bindings
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

I kept this initial patch separate to make it easier to review the core
InstanceIDWithSubtype behavior without distractions.

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

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

Description was changed from

==========
Add InstanceIDWithSubtype.java to allow an Instance ID per website

Uses the protected constructor to create a subtype-specific InstanceID.

Part of a series of patches:
1. this patch
2. TODO (adds JNI bindings)
3. TODO (adds fake and test)

I kept this initial patch separate to make it easier to review the core
InstanceIDWithSubtype behavior without distractions.

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

to

==========
Add InstanceIDWithSubtype.java to allow an Instance ID per website

Uses the protected constructor to create a subtype-specific InstanceID.

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

I kept this initial patch separate to make it easier to review the core
InstanceIDWithSubtype behavior without distractions.

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

[dgiorgini, 2016-03-24 15:08:09]

dgiorgini@google.com changed reviewers:
+ dgiorgini@google.com

[dgiorgini, 2016-03-24 15:08:10]

lgtm

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

Description was changed from

==========
Add InstanceIDWithSubtype.java to allow an Instance ID per website

Uses the protected constructor to create a subtype-specific InstanceID.

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

I kept this initial patch separate to make it easier to review the core
InstanceIDWithSubtype behavior without distractions.

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

to

==========
Add InstanceIDWithSubtype.java to allow an Instance ID per website

Uses the protected constructor to create a subtype-specific InstanceID.

Part of a series of patches:
1. this patch
2. https://codereview.chromium.org/1830983002 adds JNI bindings
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

I kept this initial patch separate to make it easier to review the core
InstanceIDWithSubtype behavior without distractions.

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

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

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

[Peter Beverloo, 2016-04-11 13:31:57]

https://codereview.chromium.org/1832833002/diff/20001/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/1832833002/diff/20001/components/gcm_driver/i...
components/gcm_driver/instance_id/android/java/src/org/chromium/components/gcm_driver/instance_id/InstanceIDWithSubtype.java:30:
synchronized (InstanceID.class) {
I find these synchronized statements on the inheritance chain a bit icky— it's
hard to understand why they are necessary.

Could you:
  (a) Explain why they are necessary? We are in control of all the callers, so
we can just enforce usage from a single thread.
  (b) Assuming they are, capture this in the class-level comment.

https://codereview.chromium.org/1832833002/diff/20001/components/gcm_driver/i...
components/gcm_driver/instance_id/android/java/src/org/chromium/components/gcm_driver/instance_id/InstanceIDWithSubtype.java:31:
if (subtype == null || subtype == "") {
nit: use TextUtils.isEmpty(subtype)

https://codereview.chromium.org/1832833002/diff/20001/components/gcm_driver/i...
components/gcm_driver/instance_id/android/java/src/org/chromium/components/gcm_driver/instance_id/InstanceIDWithSubtype.java:38:
// This causes some important static fields to be initialized.
Could you swap out "some important static fields" with something higher-level?
What about:

  // InstanceID users are expected to obtain the singleton instance from the
InstanceID
  // class directly, which contains some one-time initialization logic that is
still
  // relevant for users of this sub-class.

https://codereview.chromium.org/1832833002/diff/20001/components/gcm_driver/i...
components/gcm_driver/instance_id/android/java/src/org/chromium/components/gcm_driver/instance_id/InstanceIDWithSubtype.java:54:
// Technically it's not currently necessary to remove the sSubtypeInstances
entry, as
I would remove this comment, it only adds ambiguity. Cleaning up after oneself
should be the default.

[johnme, 2016-04-12 15:26:30]

Addressed peter's review comments - thanks!

https://codereview.chromium.org/1832833002/diff/20001/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/1832833002/diff/20001/components/gcm_driver/i...
components/gcm_driver/instance_id/android/java/src/org/chromium/components/gcm_driver/instance_id/InstanceIDWithSubtype.java:30:
synchronized (InstanceID.class) {
On 2016/04/11 13:31:57, Peter Beverloo wrote:
> I find these synchronized statements on the inheritance chain a bit icky— it's
> hard to understand why they are necessary.
> 
> Could you:
>   (a) Explain why they are necessary? We are in control of all the callers, so
> we can just enforce usage from a single thread.
>   (b) Assuming they are, capture this in the class-level comment.

It's valid to interleave calls to this with calls to InstanceID.getInstance, and
it's valid (and indeed expected - see a later patch) to call this from a
background thread. So the synchronized is necessary, and it needs to lock on the
same object as the base class InstanceID.getInstance.

I've reduced the scope of the synchronized blocks and added comments:
// Synchronize on the base class, to match the synchronized statements in
// InstanceID.getInstance.
&
// Synchronize on the base class, to match getInstance.

https://codereview.chromium.org/1832833002/diff/20001/components/gcm_driver/i...
components/gcm_driver/instance_id/android/java/src/org/chromium/components/gcm_driver/instance_id/InstanceIDWithSubtype.java:31:
if (subtype == null || subtype == "") {
On 2016/04/11 13:31:57, Peter Beverloo wrote:
> nit: use TextUtils.isEmpty(subtype)

Done.

https://codereview.chromium.org/1832833002/diff/20001/components/gcm_driver/i...
components/gcm_driver/instance_id/android/java/src/org/chromium/components/gcm_driver/instance_id/InstanceIDWithSubtype.java:38:
// This causes some important static fields to be initialized.
On 2016/04/11 13:31:57, Peter Beverloo wrote:
> Could you swap out "some important static fields" with something higher-level?
> What about:
> 
>   // InstanceID users are expected to obtain the singleton instance from the
> InstanceID
>   // class directly, which contains some one-time initialization logic that is
> still
>   // relevant for users of this sub-class.

Reworded to:
// The static InstanceID.getInstance method performs some one-time
initialization
// logic that is also necessary for users of this sub-class. To work around
this,
// first get (but don't use) the default InstanceID.

https://codereview.chromium.org/1832833002/diff/20001/components/gcm_driver/i...
components/gcm_driver/instance_id/android/java/src/org/chromium/components/gcm_driver/instance_id/InstanceIDWithSubtype.java:54:
// Technically it's not currently necessary to remove the sSubtypeInstances
entry, as
On 2016/04/11 13:31:57, Peter Beverloo wrote:
> I would remove this comment, it only adds ambiguity. Cleaning up after oneself
> should be the default.

Done.

[Peter Beverloo, 2016-04-13 13:09:09]

lgtm % comment

https://codereview.chromium.org/1832833002/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/1832833002/diff/60001/components/gcm_driver/i...
components/gcm_driver/instance_id/android/java/src/org/chromium/components/gcm_driver/instance_id/InstanceIDWithSubtype.java:31:
public static InstanceIDWithSubtype getInstance(Context context, String subtype)
{
It's the de facto standard in Chromium code to document any public Java method
with a docblock comment— please do.

[johnme, 2016-04-13 18:09:21]

Addressed nit - thanks!

https://codereview.chromium.org/1832833002/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/1832833002/diff/60001/components/gcm_driver/i...
components/gcm_driver/instance_id/android/java/src/org/chromium/components/gcm_driver/instance_id/InstanceIDWithSubtype.java:31:
public static InstanceIDWithSubtype getInstance(Context context, String subtype)
{
On 2016/04/13 13:09:09, Peter Beverloo wrote:
> It's the de facto standard in Chromium code to document any public Java method
> with a docblock comment— please do.

Added comment for getInstance (skipped deleteInstanceID since it's just an
override with no significant added behavior, and it's purpose is clear from the
name anyway; and skipped getSubtype because
https://source.android.com/source/code-style.html#use-javadoc-standard-comments
says "you do not need to write Javadoc for trivial get and set methods").

[johnme, 2016-04-13 18:09:28]

The CQ bit was checked by johnme@chromium.org

[johnme, 2016-04-13 18:09:29]

The patchset sent to the CQ was uploaded after l-g-t-m from
dgiorgini@google.com, peter@chromium.org
Link to the patchset: https://codereview.chromium.org/1832833002/#ps80001
(title: "Improve comments")

[commit-bot: I haz the power, 2016-04-13 18:10:08]

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

[commit-bot: I haz the power, 2016-04-13 18:53:47]

Description was changed from

==========
Add InstanceIDWithSubtype.java to allow an Instance ID per website

Uses the protected constructor to create a subtype-specific InstanceID.

Part of a series of patches:
1. this patch
2. https://codereview.chromium.org/1830983002 adds JNI bindings
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

I kept this initial patch separate to make it easier to review the core
InstanceIDWithSubtype behavior without distractions.

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

to

==========
Add InstanceIDWithSubtype.java to allow an Instance ID per website

Uses the protected constructor to create a subtype-specific InstanceID.

Part of a series of patches:
1. this patch
2. https://codereview.chromium.org/1830983002 adds JNI bindings
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

I kept this initial patch separate to make it easier to review the core
InstanceIDWithSubtype behavior without distractions.

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

[commit-bot: I haz the power, 2016-04-13 18:53:49]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-04-13 18:56:29]

Description was changed from

==========
Add InstanceIDWithSubtype.java to allow an Instance ID per website

Uses the protected constructor to create a subtype-specific InstanceID.

Part of a series of patches:
1. this patch
2. https://codereview.chromium.org/1830983002 adds JNI bindings
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

I kept this initial patch separate to make it easier to review the core
InstanceIDWithSubtype behavior without distractions.

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

to

==========
Add InstanceIDWithSubtype.java to allow an Instance ID per website

Uses the protected constructor to create a subtype-specific InstanceID.

Part of a series of patches:
1. this patch
2. https://codereview.chromium.org/1830983002 adds JNI bindings
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

I kept this initial patch separate to make it easier to review the core
InstanceIDWithSubtype behavior without distractions.

BUG=589461

Committed: https://crrev.com/fd602ac8826848de48089ae8ef8b04297bb214aa
Cr-Commit-Position: refs/heads/master@{#387053}
==========

[commit-bot: I haz the power, 2016-04-13 18:56:30]

Patchset 5 (id:??) landed as
https://crrev.com/fd602ac8826848de48089ae8ef8b04297bb214aa
Cr-Commit-Position: refs/heads/master@{#387053}