[Sync] Add auto-generated ModelType in Java.

sync/android/java/src/org/chromium/sync/ModelTypeHelper.java

Index: sync/android/java/src/org/chromium/sync/ModelTypeHelper.java
diff --git a/sync/android/java/src/org/chromium/sync/ModelTypeHelper.java b/sync/android/java/src/org/chromium/sync/ModelTypeHelper.java
new file mode 100644
index 0000000000000000000000000000000000000000..7ae906de34262992031444f4fd87118b5aaa0061
--- /dev/null
+++ b/sync/android/java/src/org/chromium/sync/ModelTypeHelper.java
@@ -0,0 +1,96 @@
+// Copyright 2015 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+package org.chromium.sync;
+
+import com.google.ipc.invalidation.external.client.types.ObjectId;
+import com.google.protos.ipc.invalidation.Types;
+
+import org.chromium.base.FieldTrialList;
+import org.chromium.base.JNINamespace;
+import org.chromium.base.VisibleForTesting;
+import org.chromium.base.library_loader.LibraryLoader;
+
+import java.util.Collection;
+import java.util.HashSet;
+import java.util.Set;
+
+/**
+ * Helper methods for dealing with ModelTypes.
+ */
+@JNINamespace("syncer")
+public class ModelTypeHelper {
+    private static final String TAG = "ModelTypeHelper";
+
+    private static final int[] NON_INVALIDATION_TYPES_ARRAY = new int[] {
+        ModelType.PROXY_TABS
+    };
+
+    private static Set<String> sNonInvalidationTypes = null;

[Nicolas Zea, 2015/07/30 22:20:13]

Would be good to comment what these fields are for (e.g. why is PROXY_TABS
treated differently).

[maxbogue, 2015/07/31 15:49:56]

Done.

+    private static Set<String> sTypesDisabledForExperiment = null;

[Nicolas Zea, 2015/07/30 22:20:13]

nit: they're not being disabled per se, just not being registered for
invalidations. Perhaps name this sNonInvalidationTypesExperiment?

[maxbogue, 2015/07/31 15:49:56]

Done.

+
+    private static void initNonInvalidationTypes() {

[Nicolas Zea, 2015/07/30 22:20:13]

In general I prefer being generous with commenting methods for anything that's
not ultra trivial. As such, I think it would be good too comment
initNonInvalidationTypes and isInvalidationType.

[maxbogue, 2015/07/31 15:49:56]

Done.

+        sNonInvalidationTypes = new HashSet<String>();
+        for (int i = 0; i < NON_INVALIDATION_TYPES_ARRAY.length; i++) {
+            sNonInvalidationTypes.add(toString(NON_INVALIDATION_TYPES_ARRAY[i]));
+        }
+        sTypesDisabledForExperiment = new HashSet<String>();
+        sTypesDisabledForExperiment.add(toString(ModelType.SESSIONS));
+        sTypesDisabledForExperiment.add(toString(ModelType.FAVICON_TRACKING));
+        sTypesDisabledForExperiment.add(toString(ModelType.FAVICON_IMAGES));
+    }
+
+    private static boolean isInvalidationType(String modelType) {
+        if (sNonInvalidationTypes == null) {
+            initNonInvalidationTypes();
+        }
+        if (sTypesDisabledForExperiment.contains(modelType) && LibraryLoader.isInitialized()) {
+            return !FieldTrialList.findFullName("AndroidSessionNotifications").equals("Disabled");

[Nicolas Zea, 2015/07/30 22:20:13]

Seems like this is a bit of a layering invalidation. Either the notion of
Sessions/Favicons being part of an experiment should be tightly coupled to the
field trial here, or the field trial should be coupled to those types within
sTypesDisabledForExperiment. I wonder if we should just get rid of
sTypesDisabledForExperiment altogether.

[maxbogue, 2015/07/31 00:49:39]

I wanted to avoid calling toString on the relevant types and constructing a set
for them every time isInvalidationType was called. If experiment values can't
change except on a Chrome restart, I could do the experiment check in init. Is
that the case?

[Nicolas Zea, 2015/08/04 17:04:53]

Experiments get polled every couple of hours and can therefore change on the fly
as I understand it (you can look up the finch experiments documentation to find
out the details).

I don't feel too strongly about this though, I guess it's okay like this.

[maxbogue, 2015/08/04 20:06:12]

Yeah, in that case I don't see a much better way to do this.

+        }
+        return !sNonInvalidationTypes.contains(modelType);
+    }
+
+    private static ObjectId toObjectId(String modelType) {
+        return ObjectId.newInstance(Types.ObjectSource.CHROME_SYNC, modelType.getBytes());
+    }
+
+    @VisibleForTesting
+    public static ObjectId toObjectId(int modelType) {
+        return toObjectId(toString(modelType));
+    }
+
+    /**
+     * Converts a model type to its string representation using JNI.
+     * This is the value that is stored in the invalidation preferences.
+     *
+     * @param modelType the model type to convert to a string.
+     * @return the string representation of the model type constant.
+     */
+    public static String toString(Integer modelType) {

[Nicolas Zea, 2015/07/30 22:20:13]

Given the other comment about ModelTypeToString being confusing, I think this
should probably be renamed as well, possibly toNotificationtype?

[maxbogue, 2015/07/31 00:49:40]

NotificationType is a more confusing name to me since it's actually a string
key, but I can change it.

[maxbogue, 2015/07/31 15:49:56]

I would actually prefer to not have the concept of notification type in the Java
code, especially since with the PROXY_TABS thing that's not even really what
this is.

[Nicolas Zea, 2015/08/04 17:04:53]

What would you suggest in place of toString then, given ModelTypeToString is
already defined to mean something else in the native code?

I'm not sure what you mean by notification type not being what this is.
Eventually this is just returning the native RealModelTypeToNotificationType,
right? What do you consider this value to be?

Notification type as far as I recall is just a string that can be used to create
an object id.

[maxbogue, 2015/08/04 20:06:12]

Ah, I wasn't thinking the native ModelTypeToString would conflict with this. Re:
notification type, I was simply referring to the PROXY_TABS exception, but I
guess that doesn't matter so much. I'll change this to toNotificationType.

+        // Because PROXY_TABS isn't an invalidation type, it doesn't have a string from native,
+        // but for backwards compatibility we need to keep its pref value the same as the old
+        // ModelType enum name value.
+        if (modelType == ModelType.PROXY_TABS) {
+            return "PROXY_TABS";

[Nicolas Zea, 2015/07/30 22:20:13]

Didn't PROXY_TABS just previously have the string "NULL"?

[maxbogue, 2015/07/31 00:49:40]

That's what I thought at first, but no... Previously all the ModelTypes had two
string representations. Their .name(), which is just the text of the actual enum
text, and the string that was passed into their constructor (stored as
mModelType). The name seems to be what was stored in invalidation preferences,
and the mModelType value was only used when they were converted to ObjectIds.
PROXY_TABS should be filtered out before being converted to an ObjectId, and my
understanding is that the pref value has to match. I could add a check in
toObjectId that returns "NULL" if !isInvalidationType just in case someone
doesn't use modelTypeStringsToObjectIds properly; would that be better?

[Nicolas Zea, 2015/08/04 17:04:53]

I see. What information did the preference store related to PROXY_TABS? What
would happen if this wasn't handled?

[maxbogue, 2015/08/04 20:06:12]

My understanding is that it would store "PROXY_TABS", but then when it was going
to register the prefs values, PROXY_TABS would get stripped out in the
conversion to object IDs. This is how it still works in this CL; PROXY_TABS
should be removed in modelTypeStringsToObjectIds and never be registered.
However, previously if somehow that filter was bypassed, PROXY_TABS would be
registered using the string "NULL". In the current state of this CL, it would be
registered with "PROXY_TABS". If we'd like to fix this, I'd propose just adding
a isInvalidationType() check to toObjectId() that uses the string "NULL" if it
fails. Alternatively, we could just assert isInvalidationType.

+        }
+        return nativeModelTypeToString(modelType);
+    }
+
+    /**
+     * Converts a set of {@link String} model types to a set of {@link ObjectId}.
+     *
+     * This strips out any {@link ModelType} that is not an invalidation type.
+     */
+    public static Set<ObjectId> modelTypeStringsToObjectIds(Collection<String> modelTypes) {
+        Set<ObjectId> objectIds = new HashSet<ObjectId>();
+        for (String modelType : modelTypes) {
+            if (isInvalidationType(modelType)) {
+                objectIds.add(toObjectId(modelType));
+            }
+        }
+        return objectIds;
+    }
+
+    private static native String nativeModelTypeToString(int modelType);
+}