[Sync] Store only latest versions of invalidations in DelayedInvalidationsController

[Sync] Store only latest versions of invalidations in DelayedInvalidationsController

DelayedInvalidationsController stores all invalidations that come while Chrome
is backgrounded. The list of invalidations can grow arbitrarily long even though
only highest versions of invalidations for each object are needed.

In this change I'm making DelayedInvalidationsController discard lower versions
of invalidations when receiving new invalidation.

BUG=705765
R=nyquist@chromium.org

Review-Url: https://codereview.chromium.org/2798613002
Cr-Commit-Position: refs/heads/master@{#463052}
Committed: https://chromium.googlesource.com/chromium/src/+/e3e4a4e437e837f6696861741dcde15693529b50

[pavely, 2017-04-04 15:58:07]

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

[commit-bot: I haz the power, 2017-04-04 15:58:48]

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

[pavely, 2017-04-04 17:06:59]

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

[commit-bot: I haz the power, 2017-04-04 17:07:35]

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

[pavely, 2017-04-04 17:09:27]

Tommy, PTAL. I don't regularly write in Java, there could be some horrible
blunders here.

[commit-bot: I haz the power, 2017-04-04 17:45:49]

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

[commit-bot: I haz the power, 2017-04-04 17:45:50]

Dry run: This issue passed the CQ dry run.

[nyquist, 2017-04-07 00:01:08]

https://codereview.chromium.org/2798613002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/invalidation/DelayedInvalidationsController.java
(right):

https://codereview.chromium.org/2798613002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/invalidation/DelayedInvalidationsController.java:116:
editor.putStringSet(DELAYED_INVALIDATIONS, invalidateAllTypes ? null : invals);
If we write null here, does that mean that we will not get the default value?
(see line 98). If so, invals could be null.

Regardless, could you write a test that ensures that for at least one of the
cases where we want to invalidate all, we never later add any invalidations?

https://codereview.chromium.org/2798613002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/invalidation/DelayedInvalidationsController.java:124:
* Returns true if update is successful, false when decoding invalidation from
string fails.
Nit: @return true if ...

[pavely, 2017-04-07 17:58:32]

https://codereview.chromium.org/2798613002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/invalidation/DelayedInvalidationsController.java
(right):

https://codereview.chromium.org/2798613002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/invalidation/DelayedInvalidationsController.java:116:
editor.putStringSet(DELAYED_INVALIDATIONS, invalidateAllTypes ? null : invals);
On 2017/04/07 00:01:08, nyquist wrote:
> If we write null here, does that mean that we will not get the default value?
> (see line 98). If so, invals could be null.
> 
> Regardless, could you write a test that ensures that for at least one of the
> cases where we want to invalidate all, we never later add any invalidations?

Documentation says that editor.putStringSet(null) is equivalent to editor.remove
so the next call to prefs.getStringSet will return default value. But I agree
this behaviour is subtle, it is better to explicitly call remove, will make code
more readable. I made corresponding change.

testAllInvalidationsTriggeredOnResume already tests sequence when single type
invalidation is added after "all invalidations" one. It calls
addPendingInvalidation for first, then all and then second and asserts that only
allInvalidations get delivered.

https://codereview.chromium.org/2798613002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/invalidation/DelayedInvalidationsController.java:124:
* Returns true if update is successful, false when decoding invalidation from
string fails.
On 2017/04/07 00:01:08, nyquist wrote:
> Nit: @return true if ...

Done.

[pavely, 2017-04-07 17:58:35]

The CQ bit was checked by pavely@chromium.org

[commit-bot: I haz the power, 2017-04-07 17:59:19]

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

[commit-bot: I haz the power, 2017-04-07 17:59:20]

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

[commit-bot: I haz the power, 2017-04-07 17:59:20]

No L-G-T-M from a valid reviewer yet.
CQ run can only be started once the patch has received an L-G-T-M from a full
committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a
full super star committer.
Committers are members of the group "project-chromium-committers".
Note that this has nothing to do with OWNERS files.

[nyquist, 2017-04-07 18:51:32]

awesome! lgtm!

[pavely, 2017-04-07 19:48:58]

The CQ bit was checked by pavely@chromium.org

[commit-bot: I haz the power, 2017-04-07 19:49:36]

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

[commit-bot: I haz the power, 2017-04-07 19:58:38]

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

[commit-bot: I haz the power, 2017-04-07 19:58:39]

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

[pavely, 2017-04-07 20:15:16]

The CQ bit was checked by pavely@chromium.org

[commit-bot: I haz the power, 2017-04-07 20:16:12]

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

[commit-bot: I haz the power, 2017-04-07 20:26:10]

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

[commit-bot: I haz the power, 2017-04-07 20:26:11]

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

[pavely, 2017-04-07 22:06:11]

The CQ bit was checked by pavely@chromium.org

[commit-bot: I haz the power, 2017-04-07 22:06:46]

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

[commit-bot: I haz the power, 2017-04-07 22:18:31]

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

[commit-bot: I haz the power, 2017-04-07 22:18:32]

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

[pavely, 2017-04-07 23:34:16]

The CQ bit was checked by pavely@chromium.org

[commit-bot: I haz the power, 2017-04-07 23:34:41]

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

[commit-bot: I haz the power, 2017-04-07 23:41:01]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1491608056239260,
"parent_rev": "00daa6ffb9302d37c20324ab4f36485969b63a5a", "commit_rev":
"e3e4a4e437e837f6696861741dcde15693529b50"}

[commit-bot: I haz the power, 2017-04-07 23:41:51]

Description was changed from

==========
[Sync] Store only latest versions of invalidations in
DelayedInvalidationsController

DelayedInvalidationsController stores all invalidations that come while Chrome
is backgrounded. The list of invalidations can grow arbitrarily long even though
only highest versions of invalidations for each object are needed.

In this change I'm making DelayedInvalidationsController discard lower versions
of invalidations when receiving new invalidation.

BUG=705765
R=nyquist@chromium.org
==========

to

==========
[Sync] Store only latest versions of invalidations in
DelayedInvalidationsController

DelayedInvalidationsController stores all invalidations that come while Chrome
is backgrounded. The list of invalidations can grow arbitrarily long even though
only highest versions of invalidations for each object are needed.

In this change I'm making DelayedInvalidationsController discard lower versions
of invalidations when receiving new invalidation.

BUG=705765
R=nyquist@chromium.org

Review-Url: https://codereview.chromium.org/2798613002
Cr-Commit-Position: refs/heads/master@{#463052}
Committed:
https://chromium.googlesource.com/chromium/src/+/e3e4a4e437e837f6696861741dcd...
==========

[commit-bot: I haz the power, 2017-04-07 23:41:52]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/e3e4a4e437e837f6696861741dcd...