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.FileNotFoundException;
+import java.io.FileOutputStream;
+import java.io.IOException;
+import java.io.ObjectOutputStream;
+import java.io.Serializable;
+import java.net.SocketTimeoutException;
+import java.net.UnknownHostException;
+
+/**
+ * Job Service to fetch seed in the background which guarantees to get the job done.

[paulmiller, 2017/07/10 18:23:12]

This comment is confusing. Explain what "seed" means, and remove "guarantees to
get the job done". (I don't think anything about this is guaranteed.)

[yiyuny, 2017/07/11 19:15:19]

Done.

+ */
+@SuppressLint("NewApi") // JobService requires API level 21
+public class AwVariationsSeedFetchService extends JobService {
+    private static final String TAG = "AwVartnsSeedFetchSvc";
+
+    public static final String SEED_DATA_FILENAME = "variations_seed";
+    public static final String SEED_DATA_PREF_FILENAME = "variations_seed_pref";

[paulmiller, 2017/07/10 18:23:12]

These files should go in the "app_webview" subdirectory, where WebView puts all
its other files like cookies and the UMA GUID. (E.g. `adb shell ls
/data/data/com.google.android.gm/app_webview`.)

[paulmiller, 2017/07/10 23:37:40]

Tony pointed out this is for the WebView/Monochrome directory, not the app's
directory, so there is no app_webview subdirectory. But we should still put them
in some subdirectory to avoid cluttering Chrome's data directory. Maybe
"webview_variations"? (Crash uploading seems to use "WebView_Crashes".)

[yiyuny, 2017/07/11 19:15:19]

Done.

+
+    // Synchronization lock to prevent simultaneous local seed file writing
+    private static final Object sLock = new Object();
+
+    @Override
+    public boolean onStartJob(final JobParameters params) {
+        new FetchFinchSeedDataTask(params, this).execute();
+        return true;
+    }
+
+    @Override
+    public boolean onStopJob(JobParameters params) {
+        return false;

[paulmiller, 2017/07/10 18:23:12]

Shouldn't this stop the FetchFinchSeedDataTask?

[paulmiller, 2017/07/10 23:37:40]

Tony pointed out that this is only called when the JobParameters are no longer
satisfied, so we don't need to do anything right now.

[yiyuny, 2017/07/11 19:15:18]

Done.

+    }
+
+    private static class FetchFinchSeedDataTask extends AsyncTask<Void, Void, Void> {
+        private JobParameters mJobParams;
+        private JobService mJobSvc;
+
+        FetchFinchSeedDataTask(JobParameters params, JobService svc) {

[paulmiller, 2017/07/10 18:23:12]

It seems strange to declare the subclass static, but then keep a "this"
reference anyway. Why not just make the class non-static?

[yiyuny, 2017/07/11 19:15:18]

Done.

+            mJobParams = params;
+            mJobSvc = svc;
+        }
+
+        @Override
+        protected Void doInBackground(Void... params) {
+            AwVariationsSeedFetchService.fetchSeed("");
+            return null;
+        }
+
+        @Override
+        protected void onPostExecute(Void success) {
+            mJobSvc.jobFinished(mJobParams, false);
+        }
+    }
+
+    private static void fetchSeed(String restrictMode) {

[paulmiller, 2017/07/10 18:23:12]

Remove the argument, since it's only ever called with "".

[yiyuny, 2017/07/11 19:15:19]

Done.

+        assert !ThreadUtils.runningOnUiThread();
+        // Prevent multiple simultaneous file writing
+        synchronized (sLock) {

[paulmiller, 2017/07/10 18:23:12]

If multiple jobs are scheduled at the same time, this will run all of them,
serially. But we only need one of them to run. What about using tryLock(), and
just not doing the fetch if we don't get the lock?

[yiyuny, 2017/07/11 19:15:19]

Done.

+            try {
+                VariationsSeedFetcher.SeedInfo si = VariationsSeedFetcher.get().downloadContent(
+                        VariationsSeedFetcher.VariationsPlatform.ANDROID_WEBVIEW, restrictMode);
+                if (si != null) {
+                    storeSeed(si);
+                }
+            } catch (SocketTimeoutException e) {
+            } catch (UnknownHostException e) {
+            } catch (IOException e) {

[paulmiller, 2017/07/10 18:23:12]

Group these exceptions together using "|" syntax:
https://docs.oracle.com/javase/tutorial/essential/exceptions/catch.html

Also, empty catch blocks are suspicious. Whenever you write an empty catch
block, it's best to put in a comment justifying the lack of error-handling.
Otherwise your reader feels nervous for the rest of the day :)

[yiyuny, 2017/07/11 19:15:19]

Done.

+            }
+        }
+    }
+
+    /**
+     * Store seed preference independently from Seed Info
+     */
+    public static class SeedPreference implements Serializable {
+        public String signature;
+        public String country;
+        public String date;
+        public boolean isGzipCompressed;
+
+        public SeedPreference(VariationsSeedFetcher.SeedInfo si) {
+            signature = si.signature;
+            country = si.country;
+            date = si.date;
+            isGzipCompressed = si.isGzipCompressed;
+        }
+    }
+
+    private static void storeSeed(final VariationsSeedFetcher.SeedInfo si) {
+        FileOutputStream fosSeed = null;
+        ObjectOutputStream oosSeedPref = null;
+        final Context appContext = ContextUtils.getApplicationContext();
+        try {
+            fosSeed = appContext.openFileOutput(SEED_DATA_FILENAME, Context.MODE_PRIVATE);
+            fosSeed.write(si.rawSeed, 0, si.rawSeed.length);
+            // store separately to prevent an expensive checking expiration

[paulmiller, 2017/07/10 18:23:12]

This comment is confusing. What is "checking expiration"?

[yiyuny, 2017/07/11 19:15:19]

Done.

+            SeedPreference sp = new SeedPreference(si);
+            oosSeedPref = new ObjectOutputStream(
+                    appContext.openFileOutput(SEED_DATA_PREF_FILENAME, Context.MODE_PRIVATE));
+            oosSeedPref.writeObject(sp);
+        } catch (FileNotFoundException e) {
+            Log.e(TAG, "FileNotFoundException store seed.");
+        } catch (IOException e) {
+            Log.e(TAG, "IOException store seed");
+        } finally {
+            if (fosSeed != null) {
+                try {
+                    fosSeed.close();
+                } catch (IOException e) {
+                    Log.e(TAG, "IOException file close");

[paulmiller, 2017/07/10 18:23:12]

Keep in mind that when people read your error messages, they'll read them in
logcat, mixed in with lots of other messages from all over the place, and they
won't be familiar with your code which produced the error. Be nice to these
people by making your error messages short but informative :) E.g. "Failed to
close variations seed file".

[yiyuny, 2017/07/11 19:15:19]

Done.

+                }
+            }
+            if (oosSeedPref != null) {
+                try {
+                    oosSeedPref.close();
+                } catch (IOException e) {
+                    Log.e(TAG, "IOException file close");
+                }
+            }
+        }
+    }
+}