Ensure we never try to register for the object id "NULL"

Ensure we never try to register for the object id "NULL"

Currently if we choose to register for all model types for cache
invalidation, we also register for the ObjectId "NULL", which does not
exist. This causes errors with syncing the registration state between
client and server, since the server does not accept this ObjectId.

BUG=240535
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=216591

[Yaron, 2013-08-08 01:52:16]

https://codereview.chromium.org/22642004/diff/1/sync/android/java/src/org/chr...
File
sync/android/java/src/org/chromium/sync/internal_api/pub/base/ModelType.java
(right):

https://codereview.chromium.org/22642004/diff/1/sync/android/java/src/org/chr...
sync/android/java/src/org/chromium/sync/internal_api/pub/base/ModelType.java:152:
public static Set<ModelType> stripNonInvalidationTypes(Set<ModelType>
modelTypes) {
Pleaes add Javadoc.

https://codereview.chromium.org/22642004/diff/1/sync/android/java/src/org/chr...
sync/android/java/src/org/chromium/sync/internal_api/pub/base/ModelType.java:153:
Set<ModelType> result = new HashSet<ModelType>(modelTypes);
Which a name of "strip..." it's not clear why you're performing a copy. I also
think it's unnecessary.

https://codereview.chromium.org/22642004/diff/1/sync/android/java/src/org/chr...
File sync/android/java/src/org/chromium/sync/notifier/InvalidationService.java
(right):

https://codereview.chromium.org/22642004/diff/1/sync/android/java/src/org/chr...
sync/android/java/src/org/chromium/sync/notifier/InvalidationService.java:300:
Set<ObjectId> objectIds = Sets.newHashSetWithExpectedSize(modelTypes.size());
These 4 lines can be replaced with ModelType.modelTypesToObjectIds.
That said we're now iterating through all types 3 times, creating copies each
time (or maybe twice depending on other comment). Seems suboptimal.

Also, is it too weird to just do this in modelTypesToObjectIds? You'd never want
the null type there, so you could strip it at conversion time and leave sync
types alone.

[nyquist, 2013-08-08 17:59:28]

Addressed all comments. PTAL.

https://codereview.chromium.org/22642004/diff/1/sync/android/java/src/org/chr...
File
sync/android/java/src/org/chromium/sync/internal_api/pub/base/ModelType.java
(right):

https://codereview.chromium.org/22642004/diff/1/sync/android/java/src/org/chr...
sync/android/java/src/org/chromium/sync/internal_api/pub/base/ModelType.java:152:
public static Set<ModelType> stripNonInvalidationTypes(Set<ModelType>
modelTypes) {
On 2013/08/08 01:52:16, Yaron wrote:
> Pleaes add Javadoc.

Removed.

https://codereview.chromium.org/22642004/diff/1/sync/android/java/src/org/chr...
sync/android/java/src/org/chromium/sync/internal_api/pub/base/ModelType.java:153:
Set<ModelType> result = new HashSet<ModelType>(modelTypes);
On 2013/08/08 01:52:16, Yaron wrote:
> Which a name of "strip..." it's not clear why you're performing a copy. I also
> think it's unnecessary.

Removed.

https://codereview.chromium.org/22642004/diff/1/sync/android/java/src/org/chr...
File sync/android/java/src/org/chromium/sync/notifier/InvalidationService.java
(right):

https://codereview.chromium.org/22642004/diff/1/sync/android/java/src/org/chr...
sync/android/java/src/org/chromium/sync/notifier/InvalidationService.java:300:
Set<ObjectId> objectIds = Sets.newHashSetWithExpectedSize(modelTypes.size());
On 2013/08/08 01:52:16, Yaron wrote:
> These 4 lines can be replaced with ModelType.modelTypesToObjectIds.
> That said we're now iterating through all types 3 times, creating copies each
> time (or maybe twice depending on other comment). Seems suboptimal.
> 
> Also, is it too weird to just do this in modelTypesToObjectIds? You'd never
want
> the null type there, so you could strip it at conversion time and leave sync
> types alone.

Simplified this, and moved logic to modelTypesToObjectIds.
Still iterates twice though, but code is still fairly easy to read.

The manual call to toObjectId seems to only be used in tests, so added an
explanatory comment that it should be used with caution.

[Yaron, 2013-08-08 18:36:44]

lgtm

[commit-bot: I haz the power, 2013-08-08 22:07:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nyquist@chromium.org/22642004/9001

[commit-bot: I haz the power, 2013-08-09 05:01:23]

Change committed as 216591