Variations seed is pulled from the Java application on the first launch of Chrome

chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java

Index: chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java b/chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java
index c5c1564f934cc2c01e0cd420b0881b04861c3966..5d0e4f2f08863733cd050a5fcf8df6b759bef229 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java
@@ -219,6 +219,42 @@ public final class PrefServiceBridge {
         }
     }
 
+    public static final String VARIATIONS_FIRST_RUN_SEED_SIGNATURE = "variations_seed_signature";

[Alexei Svitkine (slow), 2015/10/29 15:04:12]

See Bernhard's suggestion about having a new class for this functionality.

I'm also OK with a TODO() here about that and doing that in a separate CL,
though. (But ultimately up to owners of this code.)

[newt (away), 2015/10/30 00:46:36]

Yes, please pull this out into a separate class. PrefServiceBridge is intended
for one-off methods to allow getting and setting native prefs from Java, and is
mainly used by code inside of Settings. This chunk is large enough and logically
cohesive that it should be in a separate file.

To add a new Java/C++ bridge, just follow the pattern used by all the classes
listed in chrome_jni_registrar.cc.

Also, this API is a bit wonky. Without yet seeing how this API is actually used,
I would recommend this design with a single public method:

  public static void setVariationsSeed(String signature, String country);

If you also need public getters, then you could add methods like
getVariationsSeedSignature().

Any method or constant that doesn't need to be public should be kept private.

[Alexander Agulenko, 2015/11/02 22:55:33]

Acknowledged.

[Bernhard Bauer, 2015/11/03 09:44:49]

^^^

[Bernhard Bauer, 2015/11/03 09:45:22]

(Or at the very least add a TODO.)

[Alexander Agulenko, 2015/11/04 08:00:58]

Acknowledged.

+    public static final String VARIATIONS_FIRST_RUN_SEED_COUNTRY = "variations_seed_country";
+
+    public static String getVariationsFirstRunSeedPref(Context context, String prefName) {
+        SharedPreferences prefs = PreferenceManager.getDefaultSharedPreferences(context);
+        return prefs.getString(prefName, new String());
+    }
+
+    public static void setVariationsFirstRunSeedPref(
+            Context context, String prefName, String data) {
+        SharedPreferences prefs = PreferenceManager.getDefaultSharedPreferences(context);
+        prefs.edit().remove(prefName).apply();

[newt (away), 2015/10/30 00:46:36]

There's no need to call remove(). The next line will overwrite the existing
value anyway.

[Alexander Agulenko, 2015/11/02 22:55:33]

Done.

+        prefs.edit().putString(prefName, data).apply();
+    }
+
+    private static byte[] sVariationsFirstRunSeedData;
+
+    public static void setVariationsFirstRunSeedData(Context context, byte[] rawSeed) {
+        sVariationsFirstRunSeedData = rawSeed;

[Steven Holte, 2015/10/29 21:26:56]

Actually, I'm a bit confused here about why we are storing this in a static
variable while storing the other values in prefs.  It seems like we should be
doing all one way or the other.  I guess using the static variable assumes
Chrome hasn't been evicted from memory by the time it is started.  We probably
need to check what conditions that will be true in.

[newt (away), 2015/10/30 00:46:36]

statics should rarely be used in Java. They're global and they don't ever get
garbage collected. It's possible that a static is appropriate here, but some
justification is warranted. Does this seed need to live for the entire lifetime
of the process? (How large is the seed, anyway?)

[Bernhard Bauer, 2015/10/30 09:56:08]

In addition, what if Chrome is killed _before_ this is read?

[Bernhard Bauer, 2015/11/03 09:44:49]

^^^

[Alexei Svitkine (slow), 2015/11/03 18:44:01]

I think we should store the three things in the same way, else there are weird
edge cases with different lifetimes between them. It sounds like using prefs is
better because of the process possibly being killed and all that stuff. In that
case, you'll likely need to serialize the pref before storing, such as via
base64 - and de-serialize when querying it.

[Alexander Agulenko, 2015/11/04 08:00:58]

Done.

+    }
+
+    @CalledByNative
+    public static byte[] getVariationsFirstRunSeedData(Context context) {

[newt (away), 2015/10/30 00:46:36]

These methods can be private since they're only called via JNI and they're an
implementation detail of this class

[Alexander Agulenko, 2015/11/02 22:55:33]

Done.

+        return sVariationsFirstRunSeedData;
+    }
+
+    @CalledByNative
+    public static String getVariationsFirstRunSeedSignature(Context context) {
+        return getVariationsFirstRunSeedPref(context, VARIATIONS_FIRST_RUN_SEED_SIGNATURE);
+    }
+
+    @CalledByNative
+    public static String getVariationsFirstRunSeedCountry(Context context) {
+        return getVariationsFirstRunSeedPref(context, VARIATIONS_FIRST_RUN_SEED_COUNTRY);
+    }
+
     public boolean isAcceptCookiesEnabled() {
         return nativeGetAcceptCookiesEnabled();
     }