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.content.Context;
+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 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 tesing 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(final 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

[Alexei Svitkine (slow), 2017/07/11 19:37:23]

Comments should be full sentences. i.e. terminate with a .

Also do the same elsewhere.

[yiyuny, 2017/07/11 22:43:37]

Done.

+        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();
+        // Make sure there will only be one task fetching seed
+        if (sLock.tryLock()) {

[Alexei Svitkine (slow), 2017/07/11 19:37:23]

I don't think this does what you want it do. Unless I'm missing something subtle
- in which case you need to add more comments.

From the docs:

"tryLock()
Acquires the lock only if it is not held by another thread at the time of
invocation."

This means if another thread is holding the lock, we just ignore it and do
nothing here. Which doesn't seem correct to me.

I think you either you want:
  a) This operation cannot be done at the same time by two threads - in which
case you can use just the synchronized keyword.
  b) This operation should only be done once or - or have some rate-limiting on
how often it's done. If so, you probably want a variable that keeps track of
whether this has been started already or how long ago it's been done - or
perhaps a Future object indicating it was started but yet not complete, probably
in conjunction with synchronized keyword.

Of course, maybe I'm missing something and tryLock() is really desired. If so,
please provide the reasoning and it should also be made clear in the code
comments.

[paulmiller, 2017/07/11 20:51:36]

It was synchronized before. I don't think we want synchronized, because it's
pointless for threads to make the same request right after one another - we only
want this to happen once in a while. I recommended tryLock() for that reason.
Explicit rate limiting is was in the design doc, though I'm not sure where we
were planning to implement it. Here would be a good place if Tony isn't already
planning to put it somewhere else.

[Alexei Svitkine (slow), 2017/07/11 20:53:47]

Gotcha. It seems OK as a temporary solution with an explanation and TODO. But
otherwise I'd recommend something more robust if we expect multiple threads to
call this at once (i.e. so we don't make requests one after another if the first
one completes already.)

[yiyuny, 2017/07/11 22:43:37]

Done.

+            try {
+                VariationsSeedFetcher.SeedInfo si = VariationsSeedFetcher.get().downloadContent(
+                        VariationsSeedFetcher.VariationsPlatform.ANDROID_WEBVIEW, "");
+                storeVariationsSeed(si);
+            } 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();
+            }
+        }
+    }
+
+    /**
+     * Store seed preference independently from Seed Info
+     */
+    public static class SeedPreference implements Serializable {

[Alexei Svitkine (slow), 2017/07/11 19:37:23]

Can SeedInfo from the fetcher be re-used?

[yiyuny, 2017/07/11 22:43:37]

I didn't re-use the SeedInfo class is because

1. SeedPreference is to store seed preference and needs to implement
Serializable, but it is weird to implement it for SeedInfo which is a simple
class to get the return value(which is useless for clank).
2. There will be read operation on the seed preference, and I think store and
read operation had better not depend on the SeedInfo which is outside the
component. 

+        public String signature;
+        public String country;
+        public String date;
+        public boolean isGzipCompressed;
+
+        public SeedPreference(final VariationsSeedFetcher.SeedInfo si) {
+            signature = si.signature;
+            country = si.country;
+            date = si.date;
+            isGzipCompressed = si.isGzipCompressed;
+        }
+    }
+
+    public static File getOrCreateWebViewVariationsDir() {

[Alexei Svitkine (slow), 2017/07/11 19:37:23]

Does this need to be public?

[yiyuny, 2017/07/11 22:43:37]

The function will be used when reading the seed preference. I will make it
private in this CL.

+        File dir = new File(
+                ContextUtils.getApplicationContext().getFilesDir(), WEBVIEW_VARIATIONS_DIR);
+        if (dir.mkdir() || dir.isDirectory()) {
+            return dir;
+        }
+        return null;
+    }
+
+    private static void storeVariationsSeed(final VariationsSeedFetcher.SeedInfo si) {
+        FileOutputStream fosSeed = null;
+        ObjectOutputStream oosSeedPref = null;
+        final Context appContext = ContextUtils.getApplicationContext();
+        File webViewVariationsDir = getOrCreateWebViewVariationsDir();
+        if (webViewVariationsDir == null) {
+            return;
+        }
+        try {
+            fosSeed = new FileOutputStream(new File(webViewVariationsDir, SEED_DATA_FILENAME));
+            fosSeed.write(si.rawSeed, 0, si.rawSeed.length);
+            // 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(si));
+        } catch (FileNotFoundException e) {
+            Log.e(TAG,
+                    "FileNotFoundException failed to open file to write seed data or preference.");
+        } catch (IOException e) {
+            Log.e(TAG,
+                    "IOException failed to write variations seed data or preference to the file.");
+        } finally {
+            if (fosSeed != null) {
+                try {
+                    fosSeed.close();
+                } catch (IOException e) {
+                    Log.e(TAG, "IOException failed to close the variations seed data file.");
+                }
+            }

[Alexei Svitkine (slow), 2017/07/11 19:37:23]

This is pretty verbose. Perhaps add a helper function for this construct since
you're using it twice? Or perhaps there's a helper somewhere already for this
sort of thing?

[yiyuny, 2017/07/11 22:43:36]

Done.

+            if (oosSeedPref != null) {
+                try {
+                    oosSeedPref.close();
+                } catch (IOException e) {
+                    Log.e(TAG, "IOException failed to close the variations seed preference file.");
+                }
+            }
+        }
+    }
+}