[Android] Only invalidate objects that were received from Tango on resume.

[Android] Only invalidate objects that were received from Tango on resume.

On Android, invalidations are deferred if they are received when Chrome is in
the background.
Presently we invalidate all objectIds if any invalidation was received when
Chrome was in the background.
This cl changes that by storing the specific invalidations received and only
falling back to invalidating all if the stored invalidations cannot be parsed.

Further references to sync in places that deal with generic invalidations have
been updated to reflect that.

Branched from http://crrev.com/1131733003/

BUG=489722
Committed: https://crrev.com/d6ed555991e93a5a18b40d9846dad00b624ec899
Cr-Commit-Position: refs/heads/master@{#333513}

[knn, 2015-05-19 18:10:59]

knn@chromium.org changed reviewers:
+ bauerb@chromium.org, nyquist@chromium.org, treib@chromium.org

[knn, 2015-05-19 18:11:00]

nyquist@chromium.org: I have branched it from the previous cl as it was getting
too big. Hope that is fine.

I will upload the changes there as well once, for you to verify previous
comments. Please add additional comments here.

@bauerb, @treib: Mainly FYI but please have look as well.

[knn, 2015-05-20 11:42:02]

Transcribed non trivial comments from old patch. PTAL.

https://codereview.chromium.org/1144543009/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/sync/ChromiumSyncAdapter.java
(right):

https://codereview.chromium.org/1144543009/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/sync/ChromiumSyncAdapter.java:144:
final SyncResult syncResult, final Semaphore semaphore) {
On 2015/05/19 08:51:19, nyquist wrote:
> Could you add a test to ensure this works as expected?

Isn't this already covered by various tests? I have only pre-processed the data
from the bundle here.

https://codereview.chromium.org/1144543009/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/sync/DelayedSyncController.java
(right):

https://codereview.chromium.org/1144543009/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/sync/DelayedSyncController.java:91:
void addPendingInvalidation(Context context, String account, PendingInvalidation
invalidation) {
On 2015/05/19 08:51:20, nyquist wrote:
> This class seems like it should have a few updated tests now.

PTAL.

https://codereview.chromium.org/1144543009/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/sync/DelayedSyncController.java:109:
private List<Bundle> popPendingInvalidations(final Context context) {
On 2015/05/19 19:15:15, Bernhard Bauer wrote:
> If you return a list here, it's a bit easier to construct (pity we don't have
> map()...).

Done.

https://codereview.chromium.org/1144543009/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/sync/DelayedSyncController.java:115:
if (invals == null) return Arrays.asList(new Bundle());
On 2015/05/19 19:15:15, Bernhard Bauer wrote:
> Is there a reason you return a list consisting of one empty bundle?

Added a comment to that effect. No specific invalidation means invalidate all
types.

https://codereview.chromium.org/1144543009/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/sync/DelayedSyncController.java:122:
} catch (ClassNotFoundException | IOException e) {
On 2015/05/19 19:15:15, Bernhard Bauer wrote:
> Which exception do you expect here? Can you catch a more specific type?

ClassNotFound + IO. Done.

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

https://codereview.chromium.org/1144543009/diff/20001/components/invalidation...
components/invalidation/android/java/src/org/chromium/components/invalidation/InvalidationService.java:48:
notifyInvalidationToNativeChrome(Types.ObjectSource.CHROME_SYNC, null, 0L,
null);
On 2015/05/19 08:51:20, nyquist wrote:
> Instead of keeping this method, could we instead infer this from the object
> source in native code?
I made a first pass through simplifying these methods.
This method is also being called from SigninHelper.java and SyncTestUtil.java.
Should I move the logic there?

https://codereview.chromium.org/1144543009/diff/20001/components/invalidation...
File
components/invalidation/android/java/src/org/chromium/components/invalidation/PendingInvalidation.java
(right):

https://codereview.chromium.org/1144543009/diff/20001/components/invalidation...
components/invalidation/android/java/src/org/chromium/components/invalidation/PendingInvalidation.java:66:
ObjectOutputStream oos = null;
On 2015/05/19 19:15:16, Bernhard Bauer wrote:
> TBH, I would just roll your own string serialization/deserialization
mechanism.
> It would probably still be easier than jumping through all these hoops.

I can't think of any that does not make assumptions about the data. And this
seems better than escaping data. I could make assumptions and mark them
explicitly (through an assert), if there likely aren't too many use cases of
this in the future.
WDYT?

https://codereview.chromium.org/1144543009/diff/20001/components/invalidation...
components/invalidation/android/java/src/org/chromium/components/invalidation/PendingInvalidation.java:72:
} finally {
On 2015/05/19 08:51:19, nyquist wrote:
> It seems to me that we should have some of protection for this. Reading and
> writing prefs to disk on background threads seems to be unsafe on Android.
Could
> you ensure all these parse-statements, etc. are protected and return a
> best-effort PendingInvalidation, or worst case an invalidateAll-invalidation?

Doing this while handling exceptions.

[Bernhard Bauer, 2015-05-20 16:05:51]

https://codereview.chromium.org/1144543009/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/sync/ChromiumSyncAdapter.java
(right):

https://codereview.chromium.org/1144543009/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/sync/ChromiumSyncAdapter.java:34:
* A invalidation adapter for Chromium.
"Sync adapter" is an expression with a defined meaning on Android (refering to
Android Sync, not Chrome Sync), so I think the old comment expresses the
situation better.

https://codereview.chromium.org/1144543009/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/sync/ChromiumSyncAdapter.java:37:
private static final String TAG = "cr.invalidation";
Use Log.makeTag?

https://codereview.chromium.org/1144543009/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/sync/DelayedSyncController.java
(right):

https://codereview.chromium.org/1144543009/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/sync/DelayedSyncController.java:111:
assert prefs.getString(DELAYED_ACCOUNT_NAME, null) != null;
Use contains()?

https://codereview.chromium.org/1144543009/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/sync/DelayedSyncController.java:123:
e.printStackTrace();
Use Log.e()?

https://codereview.chromium.org/1144543009/diff/20001/components/invalidation...
File
components/invalidation/android/java/src/org/chromium/components/invalidation/PendingInvalidation.java
(right):

https://codereview.chromium.org/1144543009/diff/20001/components/invalidation...
components/invalidation/android/java/src/org/chromium/components/invalidation/PendingInvalidation.java:66:
ObjectOutputStream oos = null;
On 2015/05/20 11:42:01, knn wrote:
> On 2015/05/19 19:15:16, Bernhard Bauer wrote:
> > TBH, I would just roll your own string serialization/deserialization
> mechanism.
> > It would probably still be easier than jumping through all these hoops.
> 
> I can't think of any that does not make assumptions about the data. And this
> seems better than escaping data. I could make assumptions and mark them
> explicitly (through an assert), if there likely aren't too many use cases of
> this in the future.
> WDYT?

Eh, I'm okay with assumptions (maybe with an always-on check, to avoid data
corruption), but I don't have a problem with escaping either. You don't even
have to come up with your own serialization format: There are for example JSON
classes in the Android framework that seem to be pretty straight forward
(http://developer.android.com/reference/android/util/JsonReader.html). Part of
it is that I'm simply not a fan of the Java serialization mechanism, but you
can't tell me that this right here is pretty ;-)

[knn, 2015-05-21 14:14:06]

PTAL.

https://codereview.chromium.org/1144543009/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/sync/ChromiumSyncAdapter.java
(right):

https://codereview.chromium.org/1144543009/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/sync/ChromiumSyncAdapter.java:34:
* A invalidation adapter for Chromium.
On 2015/05/20 16:05:51, Bernhard Bauer wrote:
> "Sync adapter" is an expression with a defined meaning on Android (refering to
> Android Sync, not Chrome Sync), so I think the old comment expresses the
> situation better.

Thanks. I knew I was going to interchange this somewhere :(

https://codereview.chromium.org/1144543009/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/sync/ChromiumSyncAdapter.java:37:
private static final String TAG = "cr.invalidation";
On 2015/05/20 16:05:51, Bernhard Bauer wrote:
> Use Log.makeTag?

Done.

https://codereview.chromium.org/1144543009/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/sync/DelayedSyncController.java
(right):

https://codereview.chromium.org/1144543009/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/sync/DelayedSyncController.java:111:
assert prefs.getString(DELAYED_ACCOUNT_NAME, null) != null;
On 2015/05/20 16:05:51, Bernhard Bauer wrote:
> Use contains()?

Done.

https://codereview.chromium.org/1144543009/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/sync/DelayedSyncController.java:123:
e.printStackTrace();
On 2015/05/20 16:05:51, Bernhard Bauer wrote:
> Use Log.e()?

Done.

https://codereview.chromium.org/1144543009/diff/20001/components/invalidation...
File
components/invalidation/android/java/src/org/chromium/components/invalidation/PendingInvalidation.java
(right):

https://codereview.chromium.org/1144543009/diff/20001/components/invalidation...
components/invalidation/android/java/src/org/chromium/components/invalidation/PendingInvalidation.java:66:
ObjectOutputStream oos = null;
On 2015/05/20 16:05:51, Bernhard Bauer wrote:
> On 2015/05/20 11:42:01, knn wrote:
> > On 2015/05/19 19:15:16, Bernhard Bauer wrote:
> > > TBH, I would just roll your own string serialization/deserialization
> > mechanism.
> > > It would probably still be easier than jumping through all these hoops.
> > 
> > I can't think of any that does not make assumptions about the data. And this
> > seems better than escaping data. I could make assumptions and mark them
> > explicitly (through an assert), if there likely aren't too many use cases of
> > this in the future.
> > WDYT?
> 
> Eh, I'm okay with assumptions (maybe with an always-on check, to avoid data
> corruption), but I don't have a problem with escaping either. You don't even
> have to come up with your own serialization format: There are for example JSON
> classes in the Android framework that seem to be pretty straight forward
> (http://developer.android.com/reference/android/util/JsonReader.html). Part of
> it is that I'm simply not a fan of the Java serialization mechanism, but you
> can't tell me that this right here is pretty ;-)

Done.

[knn, 2015-05-21 14:17:18]

Sent too early. PowerBroadcastReceiver and ChromeBrowserSyncAdapterTest has been
added from downstream cl.

[Bernhard Bauer, 2015-05-21 16:21:25]

Nice! LGTM with an optional thingy below:

https://codereview.chromium.org/1144543009/diff/40001/components/invalidation...
File
components/invalidation/android/java/src/org/chromium/components/invalidation/PendingInvalidation.java
(right):

https://codereview.chromium.org/1144543009/diff/40001/components/invalidation...
components/invalidation/android/java/src/org/chromium/components/invalidation/PendingInvalidation.java:84:
// Handled below.
We shouldn't really get an IO exception from a StringReader anyway, right?

If we did, we probably should close the reader in a finally block, but we don't,
so... yeah. :)

[nyquist, 2015-05-22 20:21:07]

this is great!

after changing to protobufs, I think this should be more or less good to go.

https://codereview.chromium.org/1144543009/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/sync/ChromiumSyncAdapter.java
(right):

https://codereview.chromium.org/1144543009/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/sync/ChromiumSyncAdapter.java:38:
private static final String TAG = Log.makeTag("invalidation");
Nit: "cr.invalidation"? static initializers hurt startup time.

https://codereview.chromium.org/1144543009/diff/40001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/sync/ChromeBrowserSyncAdapterTest.java
(right):

https://codereview.chromium.org/1144543009/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/sync/ChromeBrowserSyncAdapterTest.java:119:
final String objectId = "objectid_value";
Nit: unnecessary final

https://codereview.chromium.org/1144543009/diff/40001/components/invalidation...
File
components/invalidation/android/java/src/org/chromium/components/invalidation/PendingInvalidation.java
(right):

https://codereview.chromium.org/1144543009/diff/40001/components/invalidation...
components/invalidation/android/java/src/org/chromium/components/invalidation/PendingInvalidation.java:22:
public class PendingInvalidation {
Should this class have a simple test for serialization/deserialization?

https://codereview.chromium.org/1144543009/diff/40001/components/invalidation...
components/invalidation/android/java/src/org/chromium/components/invalidation/PendingInvalidation.java:96:
StringWriter stringWriter = new StringWriter();
I would much rather us storing Protocol Buffers than rolling our own here. I
like this better than ObjectStream though.

We already have MessageNano easily available. For an example of reading and
writing, see
//chrome/android/java/src/org/chromium/chrome/browser/tabmodel/document/DocumentTabModelImpl.java

[knn, 2015-06-04 18:22:18]

Patchset #4 (id:60001) has been deleted

[knn, 2015-06-04 18:23:29]

PTAL. I have added some tests. Hope the coverage is better now.

https://codereview.chromium.org/1144543009/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/sync/ChromiumSyncAdapter.java
(right):

https://codereview.chromium.org/1144543009/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/sync/ChromiumSyncAdapter.java:38:
private static final String TAG = Log.makeTag("invalidation");
On 2015/05/22 20:21:07, nyquist wrote:
> Nit: "cr.invalidation"? static initializers hurt startup time.

Done.

https://codereview.chromium.org/1144543009/diff/40001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/sync/ChromeBrowserSyncAdapterTest.java
(right):

https://codereview.chromium.org/1144543009/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/sync/ChromeBrowserSyncAdapterTest.java:119:
final String objectId = "objectid_value";
On 2015/05/22 20:21:07, nyquist wrote:
> Nit: unnecessary final

Done.

https://codereview.chromium.org/1144543009/diff/40001/components/invalidation...
File
components/invalidation/android/java/src/org/chromium/components/invalidation/PendingInvalidation.java
(right):

https://codereview.chromium.org/1144543009/diff/40001/components/invalidation...
components/invalidation/android/java/src/org/chromium/components/invalidation/PendingInvalidation.java:22:
public class PendingInvalidation {
On 2015/05/22 20:21:07, nyquist wrote:
> Should this class have a simple test for serialization/deserialization?

Done.

https://codereview.chromium.org/1144543009/diff/40001/components/invalidation...
components/invalidation/android/java/src/org/chromium/components/invalidation/PendingInvalidation.java:96:
StringWriter stringWriter = new StringWriter();
On 2015/05/22 20:21:07, nyquist wrote:
> I would much rather us storing Protocol Buffers than rolling our own here. I
> like this better than ObjectStream though.
> 
> We already have MessageNano easily available. For an example of reading and
> writing, see
>
//chrome/android/java/src/org/chromium/chrome/browser/tabmodel/document/DocumentTabModelImpl.java

Serializing to file to avoid the byte -> string ugliness.

[Bernhard Bauer, 2015-06-05 08:20:49]

https://codereview.chromium.org/1144543009/diff/40001/components/invalidation...
File
components/invalidation/android/java/src/org/chromium/components/invalidation/PendingInvalidation.java
(right):

https://codereview.chromium.org/1144543009/diff/40001/components/invalidation...
components/invalidation/android/java/src/org/chromium/components/invalidation/PendingInvalidation.java:96:
StringWriter stringWriter = new StringWriter();
On 2015/06/04 18:23:29, knn wrote:
> On 2015/05/22 20:21:07, nyquist wrote:
> > I would much rather us storing Protocol Buffers than rolling our own here. I
> > like this better than ObjectStream though.
> > 
> > We already have MessageNano easily available. For an example of reading and
> > writing, see
> >
>
//chrome/android/java/src/org/chromium/chrome/browser/tabmodel/document/DocumentTabModelImpl.java
> 
> Serializing to file to avoid the byte -> string ugliness.

You could Base64-encode the byte array to get a string.

https://codereview.chromium.org/1144543009/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/sync/DelayedSyncController.java
(right):

https://codereview.chromium.org/1144543009/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/sync/DelayedSyncController.java:148:
inputStream = new FileInputStream(file);
You can directly get this stream from the context.

https://codereview.chromium.org/1144543009/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/sync/DelayedSyncController.java:159:
// TODO(knn): Use StreamUtil.closeQuietly instead.
Yes, that might be a good idea.

https://codereview.chromium.org/1144543009/diff/80001/components/invalidation...
File
components/invalidation/android/java/src/org/chromium/components/invalidation/PendingInvalidation.java
(right):

https://codereview.chromium.org/1144543009/diff/80001/components/invalidation...
components/invalidation/android/java/src/org/chromium/components/invalidation/PendingInvalidation.java:59:
public byte[] appendEncoded(byte[] bytes) {
Add Javadoc? It's really not clear what this method does, and more specifically
what gets appended to what.

[knn, 2015-06-05 15:09:53]

I misunderstood what is ugly, reverted to using preferences instead of a file
now. PTAL.

https://codereview.chromium.org/1144543009/diff/40001/components/invalidation...
File
components/invalidation/android/java/src/org/chromium/components/invalidation/PendingInvalidation.java
(right):

https://codereview.chromium.org/1144543009/diff/40001/components/invalidation...
components/invalidation/android/java/src/org/chromium/components/invalidation/PendingInvalidation.java:96:
StringWriter stringWriter = new StringWriter();
On 2015/06/05 08:20:48, Bernhard Bauer wrote:
> On 2015/06/04 18:23:29, knn wrote:
> > On 2015/05/22 20:21:07, nyquist wrote:
> > > I would much rather us storing Protocol Buffers than rolling our own here.
I
> > > like this better than ObjectStream though.
> > > 
> > > We already have MessageNano easily available. For an example of reading
and
> > > writing, see
> > >
> >
>
//chrome/android/java/src/org/chromium/chrome/browser/tabmodel/document/DocumentTabModelImpl.java
> > 
> > Serializing to file to avoid the byte -> string ugliness.
> 
> You could Base64-encode the byte array to get a string.

Done.

https://codereview.chromium.org/1144543009/diff/80001/components/invalidation...
File
components/invalidation/android/java/src/org/chromium/components/invalidation/PendingInvalidation.java
(right):

https://codereview.chromium.org/1144543009/diff/80001/components/invalidation...
components/invalidation/android/java/src/org/chromium/components/invalidation/PendingInvalidation.java:59:
public byte[] appendEncoded(byte[] bytes) {
On 2015/06/05 08:20:49, Bernhard Bauer wrote:
> Add Javadoc? It's really not clear what this method does, and more
specifically
> what gets appended to what.

Done.

[Bernhard Bauer, 2015-06-05 16:17:49]

Nice! Just two nits:

https://codereview.chromium.org/1144543009/diff/100001/components/invalidatio...
File components/invalidation.gypi (right):

https://codereview.chromium.org/1144543009/diff/100001/components/invalidatio...
components/invalidation.gypi:211: 'target_name': 'invalidation_java_unit_tests',
Nit: These kinds of targets are usually named "X_junit_tests".

https://codereview.chromium.org/1144543009/diff/100001/components/invalidatio...
File
components/invalidation/android/java/src/org/chromium/components/invalidation/PendingInvalidation.java
(right):

https://codereview.chromium.org/1144543009/diff/100001/components/invalidatio...
components/invalidation/android/java/src/org/chromium/components/invalidation/PendingInvalidation.java:51:
* bundle.get* methods helpfully give compatible fallback values so we don't
handle that.
Super-nit: This sentence is an implementation detail, so you could move it
inside the method.

[knn, 2015-06-05 16:31:43]

Done. Thanks for the quick review!

https://codereview.chromium.org/1144543009/diff/100001/components/invalidatio...
File components/invalidation.gypi (right):

https://codereview.chromium.org/1144543009/diff/100001/components/invalidatio...
components/invalidation.gypi:211: 'target_name': 'invalidation_java_unit_tests',
On 2015/06/05 16:17:49, Bernhard Bauer wrote:
> Nit: These kinds of targets are usually named "X_junit_tests".

Done.

https://codereview.chromium.org/1144543009/diff/100001/components/invalidatio...
File
components/invalidation/android/java/src/org/chromium/components/invalidation/PendingInvalidation.java
(right):

https://codereview.chromium.org/1144543009/diff/100001/components/invalidatio...
components/invalidation/android/java/src/org/chromium/components/invalidation/PendingInvalidation.java:51:
* bundle.get* methods helpfully give compatible fallback values so we don't
handle that.
On 2015/06/05 16:17:49, Bernhard Bauer wrote:
> Super-nit: This sentence is an implementation detail, so you could move it
> inside the method.

Done.

[nyquist, 2015-06-05 19:49:40]

https://codereview.chromium.org/1144543009/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/sync/DelayedSyncController.java
(right):

https://codereview.chromium.org/1144543009/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/sync/DelayedSyncController.java:32:
public class DelayedSyncController {
Now with your new awesomeness this class name seems off.
DelayedInvalidationController?
And also; maybe move to //c/a/j/s/o/c/c/b/invalidation ?

https://codereview.chromium.org/1144543009/diff/120001/components/invalidatio...
File components/invalidation.gypi (right):

https://codereview.chromium.org/1144543009/diff/120001/components/invalidatio...
components/invalidation.gypi:183: 'invalidation_proto_java',
reorder: local deps first in list.

https://codereview.chromium.org/1144543009/diff/120001/components/invalidatio...
components/invalidation.gypi:191: 'target_name': 'invalidation_proto_java',
Could you also update GN?

https://codereview.chromium.org/1144543009/diff/120001/components/invalidatio...
File
components/invalidation/android/java/src/org/chromium/components/invalidation/InvalidationClientService.java
(right):

https://codereview.chromium.org/1144543009/diff/120001/components/invalidatio...
components/invalidation/android/java/src/org/chromium/components/invalidation/InvalidationClientService.java:429:
private void requestSync(@Nullable ObjectId objectId, Long version, @Nullable
String payload) {
Could the type of the |version| param become long instead of Long now?

https://codereview.chromium.org/1144543009/diff/120001/components/invalidatio...
File
components/invalidation/android/java/src/org/chromium/components/invalidation/PendingInvalidation.java
(right):

https://codereview.chromium.org/1144543009/diff/120001/components/invalidatio...
components/invalidation/android/java/src/org/chromium/components/invalidation/PendingInvalidation.java:97:
Invalidation invalidation = null;
Nit: unnecessary init to null

https://codereview.chromium.org/1144543009/diff/120001/components/invalidatio...
components/invalidation/android/java/src/org/chromium/components/invalidation/PendingInvalidation.java:101:
Log.e(TAG, "Could not parse the serialized invalidations.");
Optional nit: Add e.getMessage() to log message?
I mean, this isn't supposed to happen a lot, so I'd be OK with even logging the
whole exception (  ...invalidations.", e); )

https://codereview.chromium.org/1144543009/diff/120001/components/invalidatio...
components/invalidation/android/java/src/org/chromium/components/invalidation/PendingInvalidation.java:112:
public boolean equals(Object other) {
When foo.equals(bar), then foo.hashCode() must be the same as bar.hashCode().
Could you please override hashCode() as well?

https://codereview.chromium.org/1144543009/diff/120001/components/invalidatio...
File
components/invalidation/android/java/src/org/chromium/components/invalidation/serialized_invalidation.proto
(right):

https://codereview.chromium.org/1144543009/diff/120001/components/invalidatio...
components/invalidation/android/java/src/org/chromium/components/invalidation/serialized_invalidation.proto:1:
// Copyright 2015 The Chromium Authors. All rights reserved.
Could you move this file to //components/invalidation/android/proto ?

https://codereview.chromium.org/1144543009/diff/120001/components/invalidatio...
File
components/invalidation/android/junit/src/org/chromium/components/invalidation/PendingInvalidationTest.java
(right):

https://codereview.chromium.org/1144543009/diff/120001/components/invalidatio...
components/invalidation/android/junit/src/org/chromium/components/invalidation/PendingInvalidationTest.java:32:
assertEquals(invalidation, parsedInvalidation);
If any of the asserts below would've failed, wouldn't this line fail first?
Did you mean to put this assert in the end?
Same for the test below.

[knn, 2015-06-08 11:09:01]

PTAL. Moved files to appropriate places now, so has some more files with trivial
package rename changes.

https://codereview.chromium.org/1144543009/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/sync/DelayedSyncController.java
(right):

https://codereview.chromium.org/1144543009/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/sync/DelayedSyncController.java:32:
public class DelayedSyncController {
On 2015/06/05 19:49:39, nyquist wrote:
> Now with your new awesomeness this class name seems off.
> DelayedInvalidationController?
> And also; maybe move to //c/a/j/s/o/c/c/b/invalidation ?

I wanted to defer any churn until the cl is reviewed. Done.

https://codereview.chromium.org/1144543009/diff/120001/components/invalidatio...
File components/invalidation.gypi (right):

https://codereview.chromium.org/1144543009/diff/120001/components/invalidatio...
components/invalidation.gypi:183: 'invalidation_proto_java',
On 2015/06/05 19:49:39, nyquist wrote:
> reorder: local deps first in list.

Done.

https://codereview.chromium.org/1144543009/diff/120001/components/invalidatio...
components/invalidation.gypi:191: 'target_name': 'invalidation_proto_java',
On 2015/06/05 19:49:39, nyquist wrote:
> Could you also update GN?

Thanks for the catch!

https://codereview.chromium.org/1144543009/diff/120001/components/invalidatio...
File
components/invalidation/android/java/src/org/chromium/components/invalidation/InvalidationClientService.java
(right):

https://codereview.chromium.org/1144543009/diff/120001/components/invalidatio...
components/invalidation/android/java/src/org/chromium/components/invalidation/InvalidationClientService.java:429:
private void requestSync(@Nullable ObjectId objectId, Long version, @Nullable
String payload) {
On 2015/06/05 19:49:40, nyquist wrote:
> Could the type of the |version| param become long instead of Long now?

Indeed! my bad.

https://codereview.chromium.org/1144543009/diff/120001/components/invalidatio...
File
components/invalidation/android/java/src/org/chromium/components/invalidation/PendingInvalidation.java
(right):

https://codereview.chromium.org/1144543009/diff/120001/components/invalidatio...
components/invalidation/android/java/src/org/chromium/components/invalidation/PendingInvalidation.java:97:
Invalidation invalidation = null;
On 2015/06/05 19:49:40, nyquist wrote:
> Nit: unnecessary init to null

Done.

https://codereview.chromium.org/1144543009/diff/120001/components/invalidatio...
components/invalidation/android/java/src/org/chromium/components/invalidation/PendingInvalidation.java:101:
Log.e(TAG, "Could not parse the serialized invalidations.");
On 2015/06/05 19:49:40, nyquist wrote:
> Optional nit: Add e.getMessage() to log message?
> I mean, this isn't supposed to happen a lot, so I'd be OK with even logging
the
> whole exception (  ...invalidations.", e); )

Done.

https://codereview.chromium.org/1144543009/diff/120001/components/invalidatio...
components/invalidation/android/java/src/org/chromium/components/invalidation/PendingInvalidation.java:112:
public boolean equals(Object other) {
On 2015/06/05 19:49:40, nyquist wrote:
> When foo.equals(bar), then foo.hashCode() must be the same as bar.hashCode().
> Could you please override hashCode() as well?

Seems unnecessary as this is not used in any hash based collection, but Done.

https://codereview.chromium.org/1144543009/diff/120001/components/invalidatio...
File
components/invalidation/android/java/src/org/chromium/components/invalidation/serialized_invalidation.proto
(right):

https://codereview.chromium.org/1144543009/diff/120001/components/invalidatio...
components/invalidation/android/java/src/org/chromium/components/invalidation/serialized_invalidation.proto:1:
// Copyright 2015 The Chromium Authors. All rights reserved.
On 2015/06/05 19:49:40, nyquist wrote:
> Could you move this file to //components/invalidation/android/proto ?

Done.

https://codereview.chromium.org/1144543009/diff/120001/components/invalidatio...
File
components/invalidation/android/junit/src/org/chromium/components/invalidation/PendingInvalidationTest.java
(right):

https://codereview.chromium.org/1144543009/diff/120001/components/invalidatio...
components/invalidation/android/junit/src/org/chromium/components/invalidation/PendingInvalidationTest.java:32:
assertEquals(invalidation, parsedInvalidation);
On 2015/06/05 19:49:40, nyquist wrote:
> If any of the asserts below would've failed, wouldn't this line fail first?
> Did you mean to put this assert in the end?
> Same for the test below.

Right, moved it below!

[knn, 2015-06-08 11:18:17]

Also few things I missed,

Bernhard, you are the owner of the churn area. PTAL. I haven't done anything
beyond package renames + file moves though.

Tommy, I have left the OWNERS file in
chrome/android/java/src/org/chromium/chrome/browser/sync/ as is.
Perhaps some/all of it needs to be copied/moved to invalidation? Let me know.

[Bernhard Bauer, 2015-06-08 11:30:35]

Moves/renames LGTM

[nyquist, 2015-06-08 23:42:03]

lgtm

https://codereview.chromium.org/1144543009/diff/140001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/invalidation/ChromeBrowserSyncAdapter.java
(right):

https://codereview.chromium.org/1144543009/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/invalidation/ChromeBrowserSyncAdapter.java:13:
public class ChromeBrowserSyncAdapter extends ChromiumSyncAdapter {
This requires a change to the downstream manifest.
Suggestion: Keep the old file as @Deprecated, and make it extend this new file
as an empty class with no methods except the constructor.

Then, land a downstream change to update the package name. Then, land a third CL
to remove the deprecated class.

https://codereview.chromium.org/1144543009/diff/140001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/invalidation/DelayedInvalidationsController.java
(right):

https://codereview.chromium.org/1144543009/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/invalidation/DelayedInvalidationsController.java:1:
// Copyright 2013 The Chromium Authors. All rights reserved.
Optional nit: This is so changed now that the move-detector might not pick it
up, so possibly change year?

https://codereview.chromium.org/1144543009/diff/140001/components/invalidatio...
File
components/invalidation/android/java/src/org/chromium/components/invalidation/PendingInvalidation.java
(right):

https://codereview.chromium.org/1144543009/diff/140001/components/invalidatio...
components/invalidation/android/java/src/org/chromium/components/invalidation/PendingInvalidation.java:126:
// Update this if using in an hash based collection.
Okay. I don't like this, but sure, it does follow the contract.
Something a little bit better could possibly be
return mObjectId == null ? mObjectSource : mObjectSource + mObjectId.hashCode()
+ mVersion;

[nyquist, 2015-06-08 23:45:52]

https://codereview.chromium.org/1144543009/diff/140001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/invalidation/ChromeBrowserSyncAdapterService.java
(right):

https://codereview.chromium.org/1144543009/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/invalidation/ChromeBrowserSyncAdapterService.java:13:
public class ChromeBrowserSyncAdapterService extends ChromiumSyncAdapterService
{
Requires manifest change. See chrome/android/java_staging/AndroidManifest.xml
and also downstream.

https://codereview.chromium.org/1144543009/diff/140001/chrome/android/shell/j...
File
chrome/android/shell/java/src/org/chromium/chrome/shell/invalidation/ChromeShellSyncAdapterService.java
(right):

https://codereview.chromium.org/1144543009/diff/140001/chrome/android/shell/j...
chrome/android/shell/java/src/org/chromium/chrome/shell/invalidation/ChromeShellSyncAdapterService.java:13:
public class ChromeShellSyncAdapterService extends ChromiumSyncAdapterService {
Requires manifest change. See:
chrome/android/sync_shell/java/AndroidManifest.xml.jinja2
chrome/android/shell/java/AndroidManifest.xml.jinja2

[nyquist, 2015-06-09 05:58:10]

Regarding OWNERS, please copy:
chrome/android/java/src/org/chromium/chrome/browser/sync/OWNERS
to
chrome/android/java/src/org/chromium/chrome/browser/invalidation/OWNERS

[knn, 2015-06-09 10:18:30]

https://codereview.chromium.org/1144543009/diff/140001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/invalidation/ChromeBrowserSyncAdapter.java
(right):

https://codereview.chromium.org/1144543009/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/invalidation/ChromeBrowserSyncAdapter.java:13:
public class ChromeBrowserSyncAdapter extends ChromiumSyncAdapter {
On 2015/06/08 23:42:03, nyquist wrote:
> This requires a change to the downstream manifest.
> Suggestion: Keep the old file as @Deprecated, and make it extend this new file
> as an empty class with no methods except the constructor.
> 
> Then, land a downstream change to update the package name. Then, land a third
CL
> to remove the deprecated class.

I could not find any references downstream for any SyncAdapter or
SyncAdapterService. They are all derived from
chrome/android/java_staging/AndroidManifest.xml since jinja supports extending
an xml now. :)

https://codereview.chromium.org/1144543009/diff/140001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/invalidation/ChromeBrowserSyncAdapterService.java
(right):

https://codereview.chromium.org/1144543009/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/invalidation/ChromeBrowserSyncAdapterService.java:13:
public class ChromeBrowserSyncAdapterService extends ChromiumSyncAdapterService
{
On 2015/06/08 23:45:51, nyquist wrote:
> Requires manifest change. See chrome/android/java_staging/AndroidManifest.xml
> and also downstream.

Done.

https://codereview.chromium.org/1144543009/diff/140001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/invalidation/DelayedInvalidationsController.java
(right):

https://codereview.chromium.org/1144543009/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/invalidation/DelayedInvalidationsController.java:1:
// Copyright 2013 The Chromium Authors. All rights reserved.
On 2015/06/08 23:42:03, nyquist wrote:
> Optional nit: This is so changed now that the move-detector might not pick it
> up, so possibly change year?

Done.

https://codereview.chromium.org/1144543009/diff/140001/chrome/android/shell/j...
File
chrome/android/shell/java/src/org/chromium/chrome/shell/invalidation/ChromeShellSyncAdapterService.java
(right):

https://codereview.chromium.org/1144543009/diff/140001/chrome/android/shell/j...
chrome/android/shell/java/src/org/chromium/chrome/shell/invalidation/ChromeShellSyncAdapterService.java:13:
public class ChromeShellSyncAdapterService extends ChromiumSyncAdapterService {
On 2015/06/08 23:45:51, nyquist wrote:
> Requires manifest change. See:
> chrome/android/sync_shell/java/AndroidManifest.xml.jinja2
> chrome/android/shell/java/AndroidManifest.xml.jinja2

Done.

https://codereview.chromium.org/1144543009/diff/140001/components/invalidatio...
File
components/invalidation/android/java/src/org/chromium/components/invalidation/PendingInvalidation.java
(right):

https://codereview.chromium.org/1144543009/diff/140001/components/invalidatio...
components/invalidation/android/java/src/org/chromium/components/invalidation/PendingInvalidation.java:126:
// Update this if using in an hash based collection.
On 2015/06/08 23:42:03, nyquist wrote:
> Okay. I don't like this, but sure, it does follow the contract.
> Something a little bit better could possibly be
> return mObjectId == null ? mObjectSource : mObjectSource +
mObjectId.hashCode()
> + mVersion;

Done.

[knn, 2015-06-09 10:19:02]

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

[knn, 2015-06-09 10:19:02]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
nyquist@chromium.org
Link to the patchset: https://codereview.chromium.org/1144543009/#ps160001
(title: " ")

[commit-bot: I haz the power, 2015-06-09 10:19:16]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1144543009/160001

[commit-bot: I haz the power, 2015-06-09 12:34:52]

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

[commit-bot: I haz the power, 2015-06-09 12:34:53]

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

[nyquist, 2015-06-09 14:20:30]

https://codereview.chromium.org/1144543009/diff/180001/components/invalidatio...
File
components/invalidation/android/java/src/org/chromium/components/invalidation/PendingInvalidation.java
(right):

https://codereview.chromium.org/1144543009/diff/180001/components/invalidatio...
components/invalidation/android/java/src/org/chromium/components/invalidation/PendingInvalidation.java:25:
public class PendingInvalidation {
Given the test error; would it make sense to have a toString for this method of
mObjectId, mObjectSource and mVersion? I think that might in fact help a bit.

[knn, 2015-06-09 15:13:54]

https://codereview.chromium.org/1144543009/diff/180001/components/invalidatio...
File
components/invalidation/android/java/src/org/chromium/components/invalidation/PendingInvalidation.java
(right):

https://codereview.chromium.org/1144543009/diff/180001/components/invalidatio...
components/invalidation/android/java/src/org/chromium/components/invalidation/PendingInvalidation.java:25:
public class PendingInvalidation {
On 2015/06/09 14:20:29, nyquist wrote:
> Given the test error; would it make sense to have a toString for this method
of
> mObjectId, mObjectSource and mVersion? I think that might in fact help a bit.

Done. The order of iteration through the string set is different locally vs the
test server. Fixed that.

[nyquist, 2015-06-09 16:46:39]

thanks. still lgtm.

[knn, 2015-06-09 17:03:36]

The CQ bit was checked by knn@chromium.org

[knn, 2015-06-09 17:03:37]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org
Link to the patchset: https://codereview.chromium.org/1144543009/#ps220001
(title: "+toString")

[commit-bot: I haz the power, 2015-06-09 17:04:23]

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

[commit-bot: I haz the power, 2015-06-09 17:26:39]

Committed patchset #11 (id:220001)

[commit-bot: I haz the power, 2015-06-09 17:27:50]

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

[Bernhard Bauer, 2015-06-11 10:17:48]

https://codereview.chromium.org/1144543009/diff/220001/components/invalidatio...
File
components/invalidation/android/java/src/org/chromium/components/invalidation/PendingInvalidation.java
(right):

https://codereview.chromium.org/1144543009/diff/220001/components/invalidatio...
components/invalidation/android/java/src/org/chromium/components/invalidation/PendingInvalidation.java:132:
return String.format("objectSrc:%d,objectId:%s,version:%d,payload:%s",
mObjectSource,
BTW, I'm getting a Lint error during build about implicitly using the default
locale.