Add AwVariationsSeedFetchService and refactory VariationsSeedFetcher

android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java

+// Copyright 2017 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.android_webview;
+
+import android.annotation.SuppressLint;
+import android.app.job.JobParameters;
+import android.app.job.JobService;
+import android.os.AsyncTask;
+
+import org.chromium.base.ContextUtils;
+import org.chromium.base.Log;
+import org.chromium.base.ThreadUtils;
+import org.chromium.components.variations.firstrun.VariationsSeedFetcher;
+import org.chromium.components.variations.firstrun.VariationsSeedFetcher.SeedInfo;
+
+import java.io.Closeable;
+import java.io.File;
+import java.io.FileNotFoundException;
+import java.io.FileOutputStream;
+import java.io.IOException;
+import java.io.ObjectOutputStream;
+import java.io.Serializable;
+import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReentrantLock;
+
+/**
+ * AwVariationsSeedFetchService is a Job Service to fetch test seed data which is used by Finch
+ * to enable AB testing experiments in the native code. The fetched data is stored in the local
+ * directory which belongs to the Service process.
+ */
+@SuppressLint("NewApi") // JobService requires API level 21.
+public class AwVariationsSeedFetchService extends JobService {
+    private static final String TAG = "AwVartnsSeedFetchSvc";
+
+    public static final String WEBVIEW_VARIATIONS_DIR = "WebView_Variations/";
+    public static final String SEED_DATA_FILENAME = "variations_seed_data";
+    public static final String SEED_PREF_FILENAME = "variations_seed_pref";
+
+    // Synchronization lock to prevent simultaneous local seed file writing.
+    private static final Lock sLock = new ReentrantLock();
+
+    @Override
+    public boolean onStartJob(JobParameters params) {
+        new FetchFinchSeedDataTask(params).execute();
+        return true;
+    }
+
+    @Override
+    public boolean onStopJob(JobParameters params) {
+        // When job parameter is not satisfied any more, the job should be rescheduled.
+        return true;
+    }
+
+    private class FetchFinchSeedDataTask extends AsyncTask<Void, Void, Void> {
+        private JobParameters mJobParams;
+
+        FetchFinchSeedDataTask(JobParameters params) {
+            mJobParams = params;
+        }
+
+        @Override
+        protected Void doInBackground(Void... params) {
+            AwVariationsSeedFetchService.fetchSeed();
+            return null;
+        }
+
+        @Override
+        protected void onPostExecute(Void success) {
+            jobFinished(mJobParams, false);
+        }
+    }
+
+    private static void fetchSeed() {
+        assert !ThreadUtils.runningOnUiThread();
+        // TryLock will drop calls from other threads when one threads are executing the function.
+        // TODO(yiyuny): Add explicitly control to to ensure there's only one threading fetching at
+        // a time and that the seed doesn't get fetched too frequently
+        if (sLock.tryLock()) {
+            try {
+                SeedInfo seedInfo = VariationsSeedFetcher.get().downloadContent(
+                        VariationsSeedFetcher.VariationsPlatform.ANDROID_WEBVIEW, "");
+                storeVariationsSeed(seedInfo);
+            } catch (IOException e) {
+                // Exceptions are handled in the downloadContent function and rethrowing the
+                // exception is to stop the normal logic flow after it, so no error-handling here.
+                // Not explicitly handing SocketTimeoutException and UnknownHostException for they
+                // are both subclasses of IOException.
+            } finally {
+                sLock.unlock();
+            }
+        }
+    }
+
+    /**
+     * SeedPreference serialize related fields of seed data and is used when reading or writing

[Alexei Svitkine (slow), 2017/07/12 19:01:21]

Nit: "serializes" or "is used to serialize"

[yiyuny, 2017/07/12 22:22:05]

Done.

+     * the fields to the internal storage. serialVersionUID can let the program deserialize the data
+     * when the fields are changed.

[Alexei Svitkine (slow), 2017/07/12 19:01:21]

Nit: Move the last sentence to be about serialVersionUID. Mention that
InvalidClassException is thrown when it doesn't match.

[yiyuny, 2017/07/12 22:22:06]

Done.

+     */
+    public static class SeedPreference implements Serializable {
+        private static final long serialVersionUID = 0L;

[Alexei Svitkine (slow), 2017/07/12 19:01:21]

Nit: Add an empty line after this.

[yiyuny, 2017/07/12 22:22:05]

Done.

+        public final String signature;
+        public final String country;
+        public final String date;
+        public final boolean isGzipCompressed;
+
+        public SeedPreference(SeedInfo seedInfo) {
+            signature = seedInfo.signature;
+            country = seedInfo.country;
+            date = seedInfo.date;
+            isGzipCompressed = seedInfo.isGzipCompressed;
+        }
+    }
+
+    private static File getOrCreateWebViewVariationsDir() {
+        File webViewFileDir = ContextUtils.getApplicationContext().getFilesDir();
+        File dir = new File(webViewFileDir, WEBVIEW_VARIATIONS_DIR);
+        if (dir.mkdir() || dir.isDirectory()) {
+            return dir;
+        }
+        return null;
+    }
+
+    private static void storeVariationsSeed(SeedInfo seedInfo) {
+        File webViewVariationsDir = getOrCreateWebViewVariationsDir();
+        if (webViewVariationsDir == null) {
+            return;
+        }
+
+        FileOutputStream fosSeed = null;
+        ObjectOutputStream oosSeedPref = null;
+        try {
+            fosSeed = new FileOutputStream(new File(webViewVariationsDir, SEED_DATA_FILENAME));
+            fosSeed.write(seedInfo.seedData, 0, seedInfo.seedData.length);

[Alexei Svitkine (slow), 2017/07/12 19:01:21]

How will you read this?

I think you need to also write the length as a field, so you know how much data
to read. For example, the length can be in the SeedPreference struct.

Or, I think you can just have the byte[] be part of that struct and the
ObjectOutputStream can handle both?

Please add a function to read the data and add a unit test that verifies that
the data you've written can be read successfully.

[yiyuny, 2017/07/12 22:22:06]

I choose to store the seed data and preference separately because I don't want
to read the whole seed when I need to check if it is expired. So based on this,
if there is only seed data in one file, I don't need to store the length because
I can read it until EOF. And there is a function convertInputStreamToByteArray
in VariationsSeedFetcher will help.

[Alexei Svitkine (slow), 2017/07/13 20:36:25]

Ah, sorry I had missed that you're writing to different files. OK. Still, please
add a unit test that reads the data to ensure both parts work correctly with
each other.

+            // Store separately so that reading large seed data (which is expensive) can be
+            // prevented when checking the last seed fetch time.
+            oosSeedPref = new ObjectOutputStream(
+                    new FileOutputStream(new File(webViewVariationsDir, SEED_PREF_FILENAME)));
+            oosSeedPref.writeObject(new SeedPreference(seedInfo));
+        } catch (FileNotFoundException e) {
+            Log.e(TAG,
+                    "FileNotFoundException failed to open file to write seed data or preference.");

[Alexei Svitkine (slow), 2017/07/12 19:01:21]

Nit: If you pass e as a last param, you don't need to have the name of the
exception in the message. Just started with "Failed to". Same below.

[paulmiller, 2017/07/12 20:21:59]

I recommended not passing e, because I thought a stack trace would be
unnecessarily verbose. It wouldn't be very informative, because there's only 1
code path to this function, and logcat is already pretty noisy with stack
traces.

[Alexei Svitkine (slow), 2017/07/12 20:32:31]

Ah, fair enough. Maybe log e.getMessage() then, at least?

[yiyuny, 2017/07/12 22:22:06]

I don't know the default value of the Exception's message, so I don't know what
it will print out. 
But I think it is fine to use the current one for it just explains the
situation.

Do you mean that I should use getMessage() to replace the whole string here?

+        } catch (IOException e) {
+            Log.e(TAG,
+                    "IOException failed to write variations seed data or preference to the file.");
+        } finally {
+            closeStream(fosSeed, SEED_DATA_FILENAME);
+            closeStream(oosSeedPref, SEED_PREF_FILENAME);
+        }
+    }
+
+    private static void closeStream(Closeable stream, String filename) {
+        if (stream != null) {
+            try {
+                stream.close();
+            } catch (IOException e) {
+                Log.e(TAG, "IOException failed to close " + filename + " file.");
+            }
+        }
+    }
+}