Java code for fetching variations first run seed after receiving chrome.TOS_ACKED broadcast.

components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java

Index: components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java
diff --git a/components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java b/components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java
new file mode 100644
index 0000000000000000000000000000000000000000..5656104a84a7aa7a968c6f3b1d094493b8a1c93f
--- /dev/null
+++ b/components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedService.java
@@ -0,0 +1,106 @@
+// 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.components.variations.firstrun;
+
+import android.app.IntentService;
+import android.content.Intent;
+import android.os.AsyncTask;
+import android.os.IBinder;
+import android.util.Log;

[newt (away), 2015/11/06 18:27:14]

Use org.chromium.base.Log in stead of android.util.Log. org.chromium.base.Log
has the same API but Debug and lower log messages are filtered out in release
builds. (Incidentally, this may address my comment below about minimizing
logging on production builds.)

[Alexander Agulenko, 2015/11/06 19:54:38]

Done.

+
+import java.io.ByteArrayOutputStream;
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.UnsupportedEncodingException;
+import java.net.HttpURLConnection;
+import java.net.MalformedURLException;
+import java.net.URL;
+
+/**
+ * Background service that fetches the variations seed before the actual first run of Chrome.
+ */
+public class VariationsSeedService extends IntentService {
+    private static final String TAG = "VariationsSeedService";
+    private static final String VARIATIONS_SERVER_URL =
+            "https://clients4.google.com/chrome-variations/seed?osname=android";
+    private static final int BUFFER_SIZE = 4096;
+    private static final int READ_TIMEOUT = 10000; // time in ms
+    private static final int REQUEST_TIMEOUT = 15000; // time in ms
+
+    public VariationsSeedService() {
+        super(TAG);
+    }
+
+    @Override
+    public void onHandleIntent(Intent intent) {
+        Log.d(TAG, "Variations service started!");

[newt (away), 2015/11/06 18:27:14]

Do we need to log this on production devices, or just test devices? i.e. will
its helpfulness in tracking down bugs outweigh its cost to every user? The log
buffer has a limited, small size and extraneous logs push important logs out of
the buffer and make debugging harder. Logging should be very limited for this
reason.

[Alexander Agulenko, 2015/11/06 19:54:38]

Done.

+        try {
+            new DownloadTask().execute(new URL(VARIATIONS_SERVER_URL));

[newt (away), 2015/11/06 18:27:15]

You don't need to use an AsyncTask here because this method is already running
on a background thread (that's the point of IntentService.onHandleIntent()).
This could just be

  downloadContent(new URL(VARIATIONS_SERVER_URL));

[Alexander Agulenko, 2015/11/06 19:54:38]

Done.

+        } catch (MalformedURLException e) {
+            Log.w(TAG, "Variations server URL is malformed.");
+        }
+    }
+
+    @Override
+    public IBinder onBind(Intent intent) {

[newt (away), 2015/11/06 18:27:14]

don't need this. IntentService provides this implementation already.

[Alexander Agulenko, 2015/11/06 19:54:38]

Done.

+        return null;
+    }
+
+    private class DownloadTask extends AsyncTask<URL, Void, Boolean> {

[Alexei Svitkine (slow), 2015/11/06 16:17:56]

Nit: Make this a separate top-level class with downloadContent a private method
of it. You can put the various public statics inside it.

Call it VariationsSeedDownloadTask.java

This way, when you add the second service, it can just re-use this code.

[Alexander Agulenko, 2015/11/06 19:54:37]

Removed this class according to comments by newt@.

+        @Override
+        protected Boolean doInBackground(URL... params) {
+            try {
+                return downloadContent(params[0]);
+            } catch (IOException e) {
+                return false;
+            }
+        }
+    }
+
+    private Boolean downloadContent(URL variationsServerUrl)

[Alexei Svitkine (slow), 2015/11/06 16:17:56]

Boolean -> boolean

The uppercase version is an object instead of a primitive which doesn't seem
useful here.

[Alexander Agulenko, 2015/11/06 19:54:38]

Done.

+            throws IOException, MalformedURLException {
+        InputStream inputStream = null;
+        try {
+            HttpURLConnection connection = (HttpURLConnection) variationsServerUrl.openConnection();
+            connection.setReadTimeout(READ_TIMEOUT);
+            connection.setConnectTimeout(REQUEST_TIMEOUT);
+            connection.setRequestMethod("GET");

[Alexei Svitkine (slow), 2015/11/06 16:17:56]

Nit: Per the docs, "The default method is GET."

So no need for this line.

[Alexander Agulenko, 2015/11/06 19:54:37]

Done.

+            connection.setDoInput(true);
+            // TODO(agulenko): add gzip compression support.
+            // connection.setRequestProperty("A-IM", "gzip");
+            connection.connect();
+            int responseCode = connection.getResponseCode();

[newt (away), 2015/11/06 18:27:14]

don't you need to check if the responseCode is 200 (HttpURLConnection.HTTP_OK)?

+
+            Log.d(TAG, "variationsTracker: Response code = " + Integer.toString(responseCode));

[Alexei Svitkine (slow), 2015/11/06 16:17:56]

Nit: What's variationsTracker?

Also, I don't think Integer.toString() is needed. Java will do this implicitly
when you use string + int.

[newt (away), 2015/11/06 18:27:14]

Use format strings instead of string concatenation in log messages. E.g.

  Log.d(TAG, "Response code = %d", responseCode);

This allows the string concatenation to be avoided in cases where the log
statement isn't going to be printed, and allows the Log statement to b
effectively removed from the code in release builds.

[Alexander Agulenko, 2015/11/06 19:54:37]

To Alexei: Integer.toString() was added according to bengr@ comments in previous
issue (https://codereview.chromium.org/1394133006/), but I totally agree with
you.

[Alexander Agulenko, 2015/11/06 19:54:38]

Done.

[newt (away), 2015/11/06 20:57:23]

Yeah. In Java, it's perfectly acceptable to count on the language to do the
string conversion for you.

+            inputStream = connection.getInputStream();
+
+            // Convert the InputStream into a byte array.
+            byte[] rawSeed = convertInputStreamToByteArray(inputStream);
+            String signature = connection.getHeaderField("X-Seed-Signature");
+            String country = connection.getHeaderField("X-Country");
+            VariationsSeedBridge.setVariationsFirstRunSeed(
+                    getApplicationContext(), rawSeed, signature, country);
+            return true;
+        } finally {

[newt (away), 2015/11/06 18:27:15]

Don't you also need to call connection.disconnect() here?

[Alexander Agulenko, 2015/11/06 19:54:38]

Done.

+            if (inputStream != null) {
+                inputStream.close();
+            }
+        }
+    }
+
+    public byte[] convertInputStreamToByteArray(InputStream inputStream)

[Alexei Svitkine (slow), 2015/11/06 16:17:56]

Nit: Should be private. Document params.

[Alexander Agulenko, 2015/11/06 19:54:37]

Done.

+            throws IOException, UnsupportedEncodingException {
+        ByteArrayOutputStream byteBuffer = new ByteArrayOutputStream();
+        byte[] buffer = new byte[BUFFER_SIZE];
+        int charactersReadCount = 0;
+        int totalCharactersCount = 0;

[newt (away), 2015/11/06 18:27:14]

You could just use byteBuffer.size() instead of explicitly tracking
totalCharactersCount.

[Alexander Agulenko, 2015/11/06 19:54:37]

Removed it.
Done.

+        while ((charactersReadCount = inputStream.read(buffer)) != -1) {
+            byteBuffer.write(buffer, 0, charactersReadCount);
+            totalCharactersCount += charactersReadCount;
+        }
+        Log.w(TAG, "variationsTracker: seed length = " + totalCharactersCount);

[newt (away), 2015/11/06 18:27:15]

Again, is this logging important?

[Alexander Agulenko, 2015/11/06 19:18:36]

No, this logging was added for debug purposes only. I will remove it in the
updated patch.

+        return byteBuffer.toByteArray();
+    }
+}