Massive refactor of the Android invalidation code.

Massive refactor of the Android invalidation code.

We want the invalidations code to exist as a component
independent of sync (see bug). This CL seeks to move toward
that for the Android invalidations codebase.

Generally, things moved from //chrome/browser/invalidation
to //components/invalidation, even if git didn't mark them
as moves.

To accomplish this, I had to unify where the JNI occurred.
In Java, InvalidationService was renamed to
InvalidationClientService as the class that communicates
with GCM.

A new InvalidationService class was introduced to handle JNI
and other Java entry points to the component. Several
methods from ProfileSyncService and InvalidationController
moved to it.

InvalidationController is now the bridge between sync and
the invalidation component, and has no JNI nor C++
counterpart.

InvalidationServiceFactory was added to assist in the
creation and management of InvalidationService objects
across the JNI boundary.

BUG=259559
Committed: https://crrev.com/b8d6efcb29ede4dd881700526930b75a077f9a4e
Cr-Commit-Position: refs/heads/master@{#297202}

[maxbogue, 2014-08-27 17:28:58]

maxbogue@chromium.org changed reviewers:
+ nyquist@chromium.org

[maxbogue, 2014-08-27 17:28:58]

I think this is ready for review; PTAL.

[nyquist, 2014-09-04 09:01:57]

I'm a bit concerned about the new dependency on //sync from
//components/invalidation/.

Also, I think a CL of this size deserves a bit more thorough CL description.

Could you add things like:
- how did this work before?
- what was the issue with that?
- what does this CL do to remedy that?

https://codereview.chromium.org/459513002/diff/140001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/invalidation/InvalidationServiceFactory.java
(right):

https://codereview.chromium.org/459513002/diff/140001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/invalidation/InvalidationServiceFactory.java:23:
public class InvalidationServiceFactory {
utility class (only statics), so add private empty constructor and set class as
final

https://codereview.chromium.org/459513002/diff/140001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/invalidation/InvalidationServiceFactory.java:25:
private static final HashMap<Profile, InvalidationService> sServiceMap =
Just use Map for left hand side (Map<K, V> foo = new HashMap<K, V>())

https://codereview.chromium.org/459513002/diff/140001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/invalidation/InvalidationServiceFactory.java:35:
service = (InvalidationService) nativeGetForProfile(profile);
unnecessary cast

https://codereview.chromium.org/459513002/diff/140001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/invalidation/InvalidationServiceFactory.java:42:
return (InvalidationService) nativeGetForTest(context);
unnecessary cast

https://codereview.chromium.org/459513002/diff/140001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/sync/ProfileSyncService.java
(right):

https://codereview.chromium.org/459513002/diff/140001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/sync/ProfileSyncService.java:147:
// TODO(maxbogue): Remove once upstream use is removed.
s/upstream/downstream/
Also, add @Deprecated annotation and refer to the bug (just using
http://crbug.com/259559) and also say what the callers should call instead.

https://codereview.chromium.org/459513002/diff/140001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/invalidation/InvalidationServiceTest.java
(right):

https://codereview.chromium.org/459513002/diff/140001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/invalidation/InvalidationServiceTest.java:84:
private static class IntentSavingContext extends AdvancedMockContext {
Could this be extracted to a new class and re-used between this and
InvalidationControllerTest?

https://codereview.chromium.org/459513002/diff/140001/chrome/browser/invalida...
File chrome/browser/invalidation/invalidation_service_factory_android.cc
(right):

https://codereview.chromium.org/459513002/diff/140001/chrome/browser/invalida...
chrome/browser/invalidation/invalidation_service_factory_android.cc:24:
ProfileInvalidationProviderFactory::GetForProfile(profile);
Can this in any way return NULL? Or is this always successfully created?

https://codereview.chromium.org/459513002/diff/140001/chrome/browser/invalida...
chrome/browser/invalidation/invalidation_service_factory_android.cc:39: jobject
GetForProfile(JNIEnv* env, jclass clazz, jobject j_profile) {
If you want to do community work, feel free to add support for calling static
class-methods in our JNI generator! It would basically just do something similar
to what you are doing here, but automatically.

https://codereview.chromium.org/459513002/diff/140001/chrome/browser/invalida...
File chrome/browser/invalidation/profile_invalidation_provider_factory.cc
(right):

https://codereview.chromium.org/459513002/diff/140001/chrome/browser/invalida...
chrome/browser/invalidation/profile_invalidation_provider_factory.cc:97: 
Nit: remove empty line

https://codereview.chromium.org/459513002/diff/140001/chrome/browser/sync/pro...
File chrome/browser/sync/profile_sync_service_android.cc (right):

https://codereview.chromium.org/459513002/diff/140001/chrome/browser/sync/pro...
chrome/browser/sync/profile_sync_service_android.cc:26: #include
"components/invalidation/object_id_invalidation_map.h"
remove?
could there also be other related includes you could remove?

https://codereview.chromium.org/459513002/diff/140001/components/invalidation...
File components/invalidation.gypi (right):

https://codereview.chromium.org/459513002/diff/140001/components/invalidation...
components/invalidation.gypi:174: 'dependencies': [
Doesn't this also depend on the cacheinvalidation_javalib target?

https://codereview.chromium.org/459513002/diff/140001/components/invalidation...
components/invalidation.gypi:176: '../sync/sync.gyp:sync_java',
In the future, would this go away? I think we should aim towards that going
forward.
There will always be the class ModelType, so somewhere there needs to be some
kind of conversion, but preferably, this component should not depend on //sync.

https://codereview.chromium.org/459513002/diff/140001/components/invalidation...
File components/invalidation/BUILD.gn (right):

https://codereview.chromium.org/459513002/diff/140001/components/invalidation...
components/invalidation/BUILD.gn:82: "invalidation_service_android.cc",
duplicate. remove

https://codereview.chromium.org/459513002/diff/140001/components/invalidation...
File components/invalidation/android/DEPS (right):

https://codereview.chromium.org/459513002/diff/140001/components/invalidation...
components/invalidation/android/DEPS:2: "+sync",
again: is this OK?

https://codereview.chromium.org/459513002/diff/140001/components/invalidation...
File components/invalidation/android/component_jni_registrar.h (right):

https://codereview.chromium.org/459513002/diff/140001/components/invalidation...
components/invalidation/android/component_jni_registrar.h:14: // Register all
JNI bindings necessary for the variations component.
s/variations/invalidation/

https://codereview.chromium.org/459513002/diff/140001/components/invalidation...
File
components/invalidation/android/java/src/org/chromium/components/invalidation/InvalidationService.java
(right):

https://codereview.chromium.org/459513002/diff/140001/components/invalidation...
components/invalidation/android/java/src/org/chromium/components/invalidation/InvalidationService.java:1:
// Copyright 2013 The Chromium Authors. All rights reserved.
2014 for non-move-detected files?

https://codereview.chromium.org/459513002/diff/140001/components/invalidation...
components/invalidation/android/java/src/org/chromium/components/invalidation/InvalidationService.java:55:
public void requestSyncCycleForTest() {
Why is this needed? Couldn't you just call
requestSyncFromNativeChromeForAllTypes directly? It's public.

https://codereview.chromium.org/459513002/diff/140001/components/invalidation...
components/invalidation/android/java/src/org/chromium/components/invalidation/InvalidationService.java:74:
public void setRegisteredObjectIds(int[] objectSources, String[] objectNames) {
Does this method and the one below need to be public? Or should this only ever
be called by native? If so, make this and the method below private?

https://codereview.chromium.org/459513002/diff/140001/components/invalidation...
File components/invalidation/invalidation_service_android.cc (right):

https://codereview.chromium.org/459513002/diff/140001/components/invalidation...
components/invalidation/invalidation_service_android.cc:1: // Copyright (c) 2013
The Chromium Authors. All rights reserved.
2014 and no (c) now, since the move detection did not hit?

https://codereview.chromium.org/459513002/diff/140001/components/invalidation...
components/invalidation/invalidation_service_android.cc:112: void
InvalidationServiceAndroid::TriggerStateChangeForTest(
Missing newline before this function.

https://codereview.chromium.org/459513002/diff/140001/components/invalidation...
components/invalidation/invalidation_service_android.cc:175: } //  namespace
invalidation
extra space before //, not after.

https://codereview.chromium.org/459513002/diff/140001/components/invalidation...
File components/invalidation/invalidation_service_android.h (right):

https://codereview.chromium.org/459513002/diff/140001/components/invalidation...
components/invalidation/invalidation_service_android.h:55: void
RequestSync(JNIEnv* env,
Is RequestSync really the correct name now? Shouldn't this be more in line with
DispatchInvalidations?
Remember: invalidations are not only used for Chrome Sync.

https://codereview.chromium.org/459513002/diff/140001/components/invalidation...
File components/invalidation/invalidation_service_android_unittest.cc (right):

https://codereview.chromium.org/459513002/diff/140001/components/invalidation...
components/invalidation/invalidation_service_android_unittest.cc:30:
TEST_F(InvalidationServiceAndroidTest, FetchClientId) {
Seems like this test is crashing.

[nyquist, 2014-09-04 18:16:32]

https://codereview.chromium.org/459513002/diff/140001/chrome/android/shell/ja...
File chrome/android/shell/java/AndroidManifest.xml (right):

https://codereview.chromium.org/459513002/diff/140001/chrome/android/shell/ja...
chrome/android/shell/java/AndroidManifest.xml:179:
android:value="org.chromium.components.invalidation.InvalidationClientService"/>
FYI: Since you have to do this here, you will have to do something similar
downstream. So this will break downstream builds (runtime?) whenever this rolls
in.

[maxbogue, 2014-09-05 16:42:46]

buh

https://codereview.chromium.org/459513002/diff/140001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/invalidation/InvalidationServiceFactory.java
(right):

https://codereview.chromium.org/459513002/diff/140001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/invalidation/InvalidationServiceFactory.java:23:
public class InvalidationServiceFactory {
On 2014/09/04 09:01:55, nyquist wrote:
> utility class (only statics), so add private empty constructor and set class
as
> final

Done.

https://codereview.chromium.org/459513002/diff/140001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/invalidation/InvalidationServiceFactory.java:25:
private static final HashMap<Profile, InvalidationService> sServiceMap =
On 2014/09/04 09:01:55, nyquist wrote:
> Just use Map for left hand side (Map<K, V> foo = new HashMap<K, V>())

Done.

https://codereview.chromium.org/459513002/diff/140001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/invalidation/InvalidationServiceFactory.java:35:
service = (InvalidationService) nativeGetForProfile(profile);
On 2014/09/04 09:01:55, nyquist wrote:
> unnecessary cast

Done.

https://codereview.chromium.org/459513002/diff/140001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/invalidation/InvalidationServiceFactory.java:42:
return (InvalidationService) nativeGetForTest(context);
On 2014/09/04 09:01:55, nyquist wrote:
> unnecessary cast

Done.

https://codereview.chromium.org/459513002/diff/140001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/sync/ProfileSyncService.java
(right):

https://codereview.chromium.org/459513002/diff/140001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/sync/ProfileSyncService.java:147:
// TODO(maxbogue): Remove once upstream use is removed.
On 2014/09/04 09:01:55, nyquist wrote:
> s/upstream/downstream/
> Also, add @Deprecated annotation and refer to the bug (just using
> http://crbug.com/259559) and also say what the callers should call instead.

Done.

https://codereview.chromium.org/459513002/diff/140001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/invalidation/InvalidationServiceTest.java
(right):

https://codereview.chromium.org/459513002/diff/140001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/invalidation/InvalidationServiceTest.java:84:
private static class IntentSavingContext extends AdvancedMockContext {
On 2014/09/04 09:01:55, nyquist wrote:
> Could this be extracted to a new class and re-used between this and
> InvalidationControllerTest?

Done.

https://codereview.chromium.org/459513002/diff/140001/chrome/android/shell/ja...
File chrome/android/shell/java/AndroidManifest.xml (right):

https://codereview.chromium.org/459513002/diff/140001/chrome/android/shell/ja...
chrome/android/shell/java/AndroidManifest.xml:179:
android:value="org.chromium.components.invalidation.InvalidationClientService"/>
On 2014/09/04 18:16:32, nyquist wrote:
> FYI: Since you have to do this here, you will have to do something similar
> downstream. So this will break downstream builds (runtime?) whenever this
rolls
> in.

Acknowledged.

https://codereview.chromium.org/459513002/diff/140001/chrome/browser/invalida...
File chrome/browser/invalidation/invalidation_service_factory_android.cc
(right):

https://codereview.chromium.org/459513002/diff/140001/chrome/browser/invalida...
chrome/browser/invalidation/invalidation_service_factory_android.cc:24:
ProfileInvalidationProviderFactory::GetForProfile(profile);
On 2014/09/04 09:01:55, nyquist wrote:
> Can this in any way return NULL? Or is this always successfully created?

Looks like it can only be NULL if your platform doesn't support invalidations
(which is just Chrome OS).

https://cs.corp.google.com/#clankium/src/chrome/browser/invalidation/profile_...

https://codereview.chromium.org/459513002/diff/140001/chrome/browser/invalida...
chrome/browser/invalidation/invalidation_service_factory_android.cc:39: jobject
GetForProfile(JNIEnv* env, jclass clazz, jobject j_profile) {
On 2014/09/04 09:01:56, nyquist wrote:
> If you want to do community work, feel free to add support for calling static
> class-methods in our JNI generator! It would basically just do something
similar
> to what you are doing here, but automatically.

Acknowledged.

https://codereview.chromium.org/459513002/diff/140001/chrome/browser/invalida...
File chrome/browser/invalidation/profile_invalidation_provider_factory.cc
(right):

https://codereview.chromium.org/459513002/diff/140001/chrome/browser/invalida...
chrome/browser/invalidation/profile_invalidation_provider_factory.cc:97: 
On 2014/09/04 09:01:56, nyquist wrote:
> Nit: remove empty line

Done.

https://codereview.chromium.org/459513002/diff/140001/chrome/browser/sync/pro...
File chrome/browser/sync/profile_sync_service_android.cc (right):

https://codereview.chromium.org/459513002/diff/140001/chrome/browser/sync/pro...
chrome/browser/sync/profile_sync_service_android.cc:26: #include
"components/invalidation/object_id_invalidation_map.h"
On 2014/09/04 09:01:56, nyquist wrote:
> remove?
> could there also be other related includes you could remove?

Done.

https://codereview.chromium.org/459513002/diff/140001/components/invalidation...
File components/invalidation.gypi (right):

https://codereview.chromium.org/459513002/diff/140001/components/invalidation...
components/invalidation.gypi:174: 'dependencies': [
On 2014/09/04 09:01:56, nyquist wrote:
> Doesn't this also depend on the cacheinvalidation_javalib target?

Done.

https://codereview.chromium.org/459513002/diff/140001/components/invalidation...
components/invalidation.gypi:176: '../sync/sync.gyp:sync_java',
On 2014/09/04 09:01:56, nyquist wrote:
> In the future, would this go away? I think we should aim towards that going
> forward.
> There will always be the class ModelType, so somewhere there needs to be some
> kind of conversion, but preferably, this component should not depend on
//sync.

I agree that it's preferable to not depend on //sync, but for now it's fine.

https://codereview.chromium.org/459513002/diff/140001/components/invalidation...
File components/invalidation/BUILD.gn (right):

https://codereview.chromium.org/459513002/diff/140001/components/invalidation...
components/invalidation/BUILD.gn:82: "invalidation_service_android.cc",
On 2014/09/04 09:01:56, nyquist wrote:
> duplicate. remove

Done.

https://codereview.chromium.org/459513002/diff/140001/components/invalidation...
File components/invalidation/android/DEPS (right):

https://codereview.chromium.org/459513002/diff/140001/components/invalidation...
components/invalidation/android/DEPS:2: "+sync",
On 2014/09/04 09:01:56, nyquist wrote:
> again: is this OK?

zea@ says yes, so we're gonna go with it for now.

https://codereview.chromium.org/459513002/diff/140001/components/invalidation...
File components/invalidation/android/component_jni_registrar.h (right):

https://codereview.chromium.org/459513002/diff/140001/components/invalidation...
components/invalidation/android/component_jni_registrar.h:14: // Register all
JNI bindings necessary for the variations component.
On 2014/09/04 09:01:56, nyquist wrote:
> s/variations/invalidation/

Done.

https://codereview.chromium.org/459513002/diff/140001/components/invalidation...
File
components/invalidation/android/java/src/org/chromium/components/invalidation/InvalidationService.java
(right):

https://codereview.chromium.org/459513002/diff/140001/components/invalidation...
components/invalidation/android/java/src/org/chromium/components/invalidation/InvalidationService.java:1:
// Copyright 2013 The Chromium Authors. All rights reserved.
On 2014/09/04 09:01:56, nyquist wrote:
> 2014 for non-move-detected files?

Done.

https://codereview.chromium.org/459513002/diff/140001/components/invalidation...
components/invalidation/android/java/src/org/chromium/components/invalidation/InvalidationService.java:55:
public void requestSyncCycleForTest() {
On 2014/09/04 09:01:56, nyquist wrote:
> Why is this needed? Couldn't you just call
> requestSyncFromNativeChromeForAllTypes directly? It's public.

Done.

https://codereview.chromium.org/459513002/diff/140001/components/invalidation...
components/invalidation/android/java/src/org/chromium/components/invalidation/InvalidationService.java:74:
public void setRegisteredObjectIds(int[] objectSources, String[] objectNames) {
On 2014/09/04 09:01:56, nyquist wrote:
> Does this method and the one below need to be public? Or should this only ever
> be called by native? If so, make this and the method below private?

Done.

https://codereview.chromium.org/459513002/diff/140001/components/invalidation...
File components/invalidation/invalidation_service_android.cc (right):

https://codereview.chromium.org/459513002/diff/140001/components/invalidation...
components/invalidation/invalidation_service_android.cc:1: // Copyright (c) 2013
The Chromium Authors. All rights reserved.
On 2014/09/04 09:01:56, nyquist wrote:
> 2014 and no (c) now, since the move detection did not hit?

Done.

https://codereview.chromium.org/459513002/diff/140001/components/invalidation...
components/invalidation/invalidation_service_android.cc:112: void
InvalidationServiceAndroid::TriggerStateChangeForTest(
On 2014/09/04 09:01:56, nyquist wrote:
> Missing newline before this function.

Done.

https://codereview.chromium.org/459513002/diff/140001/components/invalidation...
components/invalidation/invalidation_service_android.cc:175: } //  namespace
invalidation
On 2014/09/04 09:01:56, nyquist wrote:
> extra space before //, not after.

Done.

https://codereview.chromium.org/459513002/diff/140001/components/invalidation...
File components/invalidation/invalidation_service_android.h (right):

https://codereview.chromium.org/459513002/diff/140001/components/invalidation...
components/invalidation/invalidation_service_android.h:55: void
RequestSync(JNIEnv* env,
On 2014/09/04 09:01:56, nyquist wrote:
> Is RequestSync really the correct name now? Shouldn't this be more in line
with
> DispatchInvalidations?
> Remember: invalidations are not only used for Chrome Sync.

Since this would require changing a lot of things, is it ok if I do it in a
follow-up CL?

https://codereview.chromium.org/459513002/diff/140001/components/invalidation...
File components/invalidation/invalidation_service_android_unittest.cc (right):

https://codereview.chromium.org/459513002/diff/140001/components/invalidation...
components/invalidation/invalidation_service_android_unittest.cc:30:
TEST_F(InvalidationServiceAndroidTest, FetchClientId) {
On 2014/09/04 09:01:56, nyquist wrote:
> Seems like this test is crashing.

Passes for me. Elaborate?

[maxbogue, 2014-09-09 23:43:27]

Did I miss anything in the description?

[nyquist, 2014-09-10 18:25:04]

https://codereview.chromium.org/459513002/diff/160001/components/invalidation...
File components/invalidation.gypi (right):

https://codereview.chromium.org/459513002/diff/160001/components/invalidation...
components/invalidation.gypi:168: 'conditions': [
add invalidation_javatests. see for example
//sync/sync_tests.gypi:sync_javatests

https://codereview.chromium.org/459513002/diff/160001/components/invalidation...
File
components/invalidation/android/java/src/org/chromium/components/invalidation/InvalidationService.java
(right):

https://codereview.chromium.org/459513002/diff/160001/components/invalidation...
components/invalidation/android/java/src/org/chromium/components/invalidation/InvalidationService.java:27:
private static final String TAG = "InvalidationService";
I don't think this field is ever used.

https://codereview.chromium.org/459513002/diff/160001/components/invalidation...
components/invalidation/android/java/src/org/chromium/components/invalidation/InvalidationService.java:65:
private void setRegisteredObjectIds(int[] objectSources, String[] objectNames) {
I now realize that this is used in the InvalidationServiceTest, so I it needs to
be public.Then also add @VisibleForTesting annotation so readers can see why it
is not private.

https://codereview.chromium.org/459513002/diff/160001/components/invalidation...
File
components/invalidation/android/javatests/src/org/chromium/components/invalidation/InvalidationClientServiceTest.java
(right):

https://codereview.chromium.org/459513002/diff/160001/components/invalidation...
components/invalidation/android/javatests/src/org/chromium/components/invalidation/InvalidationClientServiceTest.java:24:
import org.chromium.sync.notifier.InvalidationPreferences.EditContext;
add org.chromium.sync.notifier.InvalidationPreferences and
org.chromium.sync.notifier.InvalidationIntentProtocol

https://codereview.chromium.org/459513002/diff/160001/components/invalidation...
File
components/invalidation/android/javatests/src/org/chromium/components/invalidation/TestableInvalidationService.java
(right):

https://codereview.chromium.org/459513002/diff/160001/components/invalidation...
components/invalidation/android/javatests/src/org/chromium/components/invalidation/TestableInvalidationService.java:5:
package org.chromium.sync.notifier;
org.chromium.components.invalidation

https://codereview.chromium.org/459513002/diff/160001/components/invalidation...
components/invalidation/android/javatests/src/org/chromium/components/invalidation/TestableInvalidationService.java:27:
public class TestableInvalidationClientService extends InvalidationClientService
{
rename file to this new name.

[maxbogue, 2014-09-11 20:08:09]

Fixed the tests. Let me know if I added invalidation_javatests to the wrong
place in //chrome/chrome.gyp...

https://codereview.chromium.org/459513002/diff/160001/components/invalidation...
File components/invalidation.gypi (right):

https://codereview.chromium.org/459513002/diff/160001/components/invalidation...
components/invalidation.gypi:168: 'conditions': [
On 2014/09/10 18:25:04, nyquist wrote:
> add invalidation_javatests. see for example
> //sync/sync_tests.gypi:sync_javatests

Done. I added it in chrome/chrome.gyp too; hopefully I found the right place for
it but I'm not sure.

https://codereview.chromium.org/459513002/diff/160001/components/invalidation...
File
components/invalidation/android/java/src/org/chromium/components/invalidation/InvalidationService.java
(right):

https://codereview.chromium.org/459513002/diff/160001/components/invalidation...
components/invalidation/android/java/src/org/chromium/components/invalidation/InvalidationService.java:27:
private static final String TAG = "InvalidationService";
On 2014/09/10 18:25:04, nyquist wrote:
> I don't think this field is ever used.

Done.

https://codereview.chromium.org/459513002/diff/160001/components/invalidation...
components/invalidation/android/java/src/org/chromium/components/invalidation/InvalidationService.java:65:
private void setRegisteredObjectIds(int[] objectSources, String[] objectNames) {
On 2014/09/10 18:25:04, nyquist wrote:
> I now realize that this is used in the InvalidationServiceTest, so I it needs
to
> be public.Then also add @VisibleForTesting annotation so readers can see why
it
> is not private.

Done.

https://codereview.chromium.org/459513002/diff/160001/components/invalidation...
File
components/invalidation/android/javatests/src/org/chromium/components/invalidation/InvalidationClientServiceTest.java
(right):

https://codereview.chromium.org/459513002/diff/160001/components/invalidation...
components/invalidation/android/javatests/src/org/chromium/components/invalidation/InvalidationClientServiceTest.java:24:
import org.chromium.sync.notifier.InvalidationPreferences.EditContext;
On 2014/09/10 18:25:04, nyquist wrote:
> add org.chromium.sync.notifier.InvalidationPreferences and
> org.chromium.sync.notifier.InvalidationIntentProtocol

Done.

https://codereview.chromium.org/459513002/diff/160001/components/invalidation...
File
components/invalidation/android/javatests/src/org/chromium/components/invalidation/TestableInvalidationService.java
(right):

https://codereview.chromium.org/459513002/diff/160001/components/invalidation...
components/invalidation/android/javatests/src/org/chromium/components/invalidation/TestableInvalidationService.java:5:
package org.chromium.sync.notifier;
On 2014/09/10 18:25:04, nyquist wrote:
> org.chromium.components.invalidation

Done.

https://codereview.chromium.org/459513002/diff/160001/components/invalidation...
components/invalidation/android/javatests/src/org/chromium/components/invalidation/TestableInvalidationService.java:27:
public class TestableInvalidationClientService extends InvalidationClientService
{
On 2014/09/10 18:25:04, nyquist wrote:
> rename file to this new name.

Done.

[maxbogue, 2014-09-11 20:10:08]

Er, sorry, I meant chrome/chrome_tests.gypi.

[nyquist, 2014-09-16 17:33:29]

lgtm!

https://codereview.chromium.org/459513002/diff/200001/components/invalidation...
File
components/invalidation/android/java/src/org/chromium/components/invalidation/InvalidationService.java
(right):

https://codereview.chromium.org/459513002/diff/200001/components/invalidation...
components/invalidation/android/java/src/org/chromium/components/invalidation/InvalidationService.java:11:
import com.google.common.annotations.VisibleForTesting;
Please use the one in org.chromium.base instead. I think it might just need
rebasing?

Also, see: https://codereview.chromium.org/573733004/

https://codereview.chromium.org/459513002/diff/200001/components/invalidation...
File
components/invalidation/android/javatests/src/org/chromium/components/invalidation/TestableInvalidationClientService.java
(right):

https://codereview.chromium.org/459513002/diff/200001/components/invalidation...
components/invalidation/android/javatests/src/org/chromium/components/invalidation/TestableInvalidationClientService.java:54:
public TestableInvalidationClientService() {
You can just remove this constructor alltogether as it doesn't add anything.

[maxbogue, 2014-09-16 21:07:38]

https://codereview.chromium.org/459513002/diff/200001/components/invalidation...
File
components/invalidation/android/java/src/org/chromium/components/invalidation/InvalidationService.java
(right):

https://codereview.chromium.org/459513002/diff/200001/components/invalidation...
components/invalidation/android/java/src/org/chromium/components/invalidation/InvalidationService.java:11:
import com.google.common.annotations.VisibleForTesting;
On 2014/09/16 17:33:29, nyquist wrote:
> Please use the one in org.chromium.base instead. I think it might just need
> rebasing?
> 
> Also, see: https://codereview.chromium.org/573733004/

Done.

https://codereview.chromium.org/459513002/diff/200001/components/invalidation...
File
components/invalidation/android/javatests/src/org/chromium/components/invalidation/TestableInvalidationClientService.java
(right):

https://codereview.chromium.org/459513002/diff/200001/components/invalidation...
components/invalidation/android/javatests/src/org/chromium/components/invalidation/TestableInvalidationClientService.java:54:
public TestableInvalidationClientService() {
On 2014/09/16 17:33:29, nyquist wrote:
> You can just remove this constructor alltogether as it doesn't add anything.

Done.

[maxbogue, 2014-09-16 23:04:33]

The CQ bit was checked by maxbogue@chromium.org

[commit-bot: I haz the power, 2014-09-16 23:07:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/459513002/220001

[commit-bot: I haz the power, 2014-09-16 23:22:36]

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

[commit-bot: I haz the power, 2014-09-16 23:22:37]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[maxbogue, 2014-09-16 23:39:56]

maxbogue@chromium.org changed reviewers:
+ thestig@chromium.org, zea@chromium.org

[maxbogue, 2014-09-16 23:39:57]

Adding missing OWNERS.

+zea for:
    chrome/browser/sync/profile_sync_service_android.cc
    chrome/browser/sync/profile_sync_service_android.h

+thestig for:
    chrome/chrome.gyp

[Lei Zhang, 2014-09-17 00:26:20]

chrome/chrome.gyp lgtm, but it looks like you have a sad android try bot.

https://codereview.chromium.org/459513002/diff/220001/chrome/browser/invalida...
File chrome/browser/invalidation/profile_invalidation_provider_factory.cc
(right):

https://codereview.chromium.org/459513002/diff/220001/chrome/browser/invalida...
chrome/browser/invalidation/profile_invalidation_provider_factory.cc:117:
Profile* profile = static_cast<Profile*>(context);
nit: (I know this is just cut+paste) you can use Profile::FromBrowserContext()

[maxbogue, 2014-09-17 00:57:20]

The trybot error didn't look related to me. Trying a rebase and running them
again.

https://codereview.chromium.org/459513002/diff/220001/chrome/browser/invalida...
File chrome/browser/invalidation/profile_invalidation_provider_factory.cc
(right):

https://codereview.chromium.org/459513002/diff/220001/chrome/browser/invalida...
chrome/browser/invalidation/profile_invalidation_provider_factory.cc:117:
Profile* profile = static_cast<Profile*>(context);
On 2014/09/17 00:26:20, Lei Zhang wrote:
> nit: (I know this is just cut+paste) you can use Profile::FromBrowserContext()

Done.

[nyquist, 2014-09-17 02:23:45]

Seems to me like InvalidationServiceAndroidTest.FetchClientId crashed?

[Nicolas Zea, 2014-09-18 17:39:35]

sync LGTM

[maxbogue, 2014-09-19 19:00:23]

maxbogue@chromium.org changed reviewers:
+ caitkp@chromium.org

[maxbogue, 2014-09-19 19:00:25]

+caitkp for:
    components/test/DEPS
    components/test/run_all_unittests.cc

[Cait (Slow), 2014-09-19 19:11:06]

lgtm

[maxbogue, 2014-09-19 23:24:20]

maxbogue@chromium.org changed reviewers:
+ jam@chromium.org

[maxbogue, 2014-09-19 23:24:22]

+jam for:
    content/content.gyp
    content/content_browser.gypi

(The invalidation_unittests target revealed some missing DEPS in //content.)

[jam, 2014-09-23 05:14:41]

https://codereview.chromium.org/459513002/diff/380001/content/content.gyp
File content/content.gyp (right):

https://codereview.chromium.org/459513002/diff/380001/content/content.gyp#new...
content/content.gyp:156: 'content_renderer',
huh?

[maxbogue, 2014-09-25 18:59:22]

maxbogue@chromium.org changed reviewers:
- jam@chromium.org

[maxbogue, 2014-09-25 18:59:23]

-jam because I removed the //content changes. Sorry about that.

[nyquist, 2014-09-26 20:26:15]

https://codereview.chromium.org/459513002/diff/420001/components/signin.gypi
File components/signin.gypi (right):

https://codereview.chromium.org/459513002/diff/420001/components/signin.gypi#...
components/signin.gypi:29: '../base/base.gyp:base_i18n',
You didn't change anything in this component, why these new dependencies?

[maxbogue, 2014-09-26 21:06:57]

https://codereview.chromium.org/459513002/diff/420001/components/signin.gypi
File components/signin.gypi (right):

https://codereview.chromium.org/459513002/diff/420001/components/signin.gypi#...
components/signin.gypi:29: '../base/base.gyp:base_i18n',
On 2014/09/26 20:26:15, nyquist wrote:
> You didn't change anything in this component, why these new dependencies?

Leftover from the dep fixing I was doing when I had invalidation_unittests.
These are actual missing deps; should I leave them in or take them out?

[maxbogue, 2014-09-29 17:39:11]

The CQ bit was checked by maxbogue@chromium.org

[commit-bot: I haz the power, 2014-09-29 17:40:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/459513002/440001

[commit-bot: I haz the power, 2014-09-29 17:45:29]

Committed patchset #23 (id:440001) as 7a5485838c3d0c677194fcb17ef9e04e0ab0377f

[commit-bot: I haz the power, 2014-09-29 17:46:07]

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