Adding milestone and channel to variations seed request URL

Bug: 776071
Change-Id: I14e4aac7e366d9344f4c25e1f752fce193226784
Reviewed-on: https://chromium-review.googlesource.com/737558
Commit-Queue: Gayane Petrosyan <gayane@chromium.org>
Reviewed-by: Yaron Friedman <yfriedman@chromium.org>
Reviewed-by: Tao Bai <michaelbai@chromium.org>
Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
Cr-Commit-Position: refs/heads/master@{#517113}

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

@@ -87,8 +87,10 @@ public class AwVariationsSeedFetchService extends JobService {
     private void fetchVariationsSeed() {
         assert !ThreadUtils.runningOnUiThread();
         try {
+            // TODO(paulmiller): Addd milestone and channel information to seed request.
+            // crbug.com/784905
             SeedInfo seedInfo = VariationsSeedFetcher.get().downloadContent(
-                    VariationsSeedFetcher.VariationsPlatform.ANDROID_WEBVIEW, "");
+                    VariationsSeedFetcher.VariationsPlatform.ANDROID_WEBVIEW, "", "", "");
             transferFetchedSeed(seedInfo);
         } catch (IOException e) {
             // Exceptions are handled and logged in the downloadContent method, so we don't

chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitTaskRunner.java

@@ -70,14 +70,18 @@ public abstract class AsyncInitTaskRunner {
 
     private class FetchSeedTask extends AsyncTask<Void, Void, Void> {
         private final String mRestrictMode;
+        private final String mMilestone;
+        private final String mChannel;
 
         public FetchSeedTask(String restrictMode) {
             mRestrictMode = restrictMode;
+            mMilestone = Integer.toString(ChromeVersionInfo.getProductMajorVersion());
+            mChannel = getChannelString();
         }
 
         @Override
         protected Void doInBackground(Void... params) {
-            VariationsSeedFetcher.get().fetchSeed(mRestrictMode);
+            VariationsSeedFetcher.get().fetchSeed(mRestrictMode, mMilestone, mChannel);
             return null;
         }
 
@@ -86,6 +90,22 @@ public abstract class AsyncInitTaskRunner {
             mFetchingVariations = false;
             tasksPossiblyComplete(true);
         }
+
+        private String getChannelString() {
+            if (ChromeVersionInfo.isCanaryBuild()) {
+                return "canary";
+            }
+            if (ChromeVersionInfo.isDevBuild()) {
+                return "dev";
+            }
+            if (ChromeVersionInfo.isBetaBuild()) {
+                return "beta";
+            }
+            if (ChromeVersionInfo.isStableBuild()) {
+                return "stable";
+            }
+            return "";
+        }
     }
 
     /**

chrome/android/junit/src/org/chromium/chrome/browser/init/AsyncInitTaskRunnerTest.java

@@ -5,6 +5,7 @@
 package org.chromium.chrome.browser.init;
 
 import static org.junit.Assert.assertTrue;
+import static org.mockito.Matchers.anyString;
 import static org.mockito.Mockito.doNothing;
 import static org.mockito.Mockito.doThrow;
 import static org.mockito.Mockito.mock;
@@ -86,7 +87,7 @@ public class AsyncInitTaskRunnerTest {
         verify(mLoader).ensureInitialized();
         verify(mLoader).asyncPrefetchLibrariesToMemory();
         verify(mRunner).onSuccess();
-        verify(mVariationsSeedFetcher, never()).fetchSeed("");
+        verify(mVariationsSeedFetcher, never()).fetchSeed(anyString(), anyString(), anyString());
     }
 
     @Test
@@ -100,7 +101,7 @@ public class AsyncInitTaskRunnerTest {
         Robolectric.flushForegroundThreadScheduler();
         assertTrue(mLatch.await(0, TimeUnit.SECONDS));
         verify(mRunner).onFailure();
-        verify(mVariationsSeedFetcher, never()).fetchSeed("");
+        verify(mVariationsSeedFetcher, never()).fetchSeed(anyString(), anyString(), anyString());
     }
 
     @Test
@@ -113,7 +114,7 @@ public class AsyncInitTaskRunnerTest {
         verify(mLoader).ensureInitialized();
         verify(mLoader).asyncPrefetchLibrariesToMemory();
         verify(mRunner).onSuccess();
-        verify(mVariationsSeedFetcher).fetchSeed("");
+        verify(mVariationsSeedFetcher).fetchSeed(anyString(), anyString(), anyString());
     }
 
     // TODO(aberent) Test for allocateChildConnection. Needs refactoring of ChildProcessLauncher to

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

@@ -79,8 +79,17 @@ public class VariationsSeedFetcher {
     }
 
     @VisibleForTesting
-    protected HttpURLConnection getServerConnection(VariationsPlatform platform,
-            String restrictMode) throws MalformedURLException, IOException {
+    protected HttpURLConnection getServerConnection(
+            VariationsPlatform platform, String restrictMode, String milestone, String channel)
+            throws MalformedURLException, IOException {
+        String urlString = getConnectionString(platform, restrictMode, milestone, channel);
+        URL url = new URL(urlString);
+        return (HttpURLConnection) url.openConnection();
+    }
+
+    @VisibleForTesting
+    protected String getConnectionString(
+            VariationsPlatform platform, String restrictMode, String milestone, String channel) {
         String urlString = VARIATIONS_SERVER_URL;
         switch (platform) {
             case ANDROID:
@@ -95,8 +104,14 @@ public class VariationsSeedFetcher {
         if (restrictMode != null && !restrictMode.isEmpty()) {
             urlString += "&restrict=" + restrictMode;
         }
-        URL url = new URL(urlString);
-        return (HttpURLConnection) url.openConnection();
+        if (milestone != null && !milestone.isEmpty()) {
+            urlString += "&milestone=" + milestone;
+        }
+        if (channel != null && !channel.isEmpty()) {
+            urlString += "&channel=" + channel;
+        }
+
+        return urlString;
     }
 
     /**
@@ -113,8 +128,10 @@ public class VariationsSeedFetcher {
     /**
      * Fetch the first run variations seed.
      * @param restrictMode The restrict mode parameter to pass to the server via a URL param.
+     * @param milestone The milestone parameter to pass to the server via a URL param.
+     * @param channel The channel parameter to pass to the server via a URL param.
      */
-    public void fetchSeed(String restrictMode) {
+    public void fetchSeed(String restrictMode, String milestone, String channel) {
         assert !ThreadUtils.runningOnUiThread();
         // Prevent multiple simultaneous fetches
         synchronized (sLock) {
@@ -130,10 +147,12 @@ public class VariationsSeedFetcher {
             }
 
             try {
-                SeedInfo info = downloadContent(VariationsPlatform.ANDROID, restrictMode);
+                SeedInfo info = downloadContent(
+                        VariationsPlatform.ANDROID, restrictMode, milestone, channel);
                 VariationsSeedBridge.setVariationsFirstRunSeed(info.seedData, info.signature,
                         info.country, info.date, info.isGzipCompressed);
             } catch (IOException e) {
+                Log.e(TAG, "IOException when fetching variations seed.", e);
                 // Exceptions are handled and logged in the downloadContent method, so we don't
                 // need any exception handling here. The only reason we need a catch-statement here
                 // is because those exceptions are re-thrown from downloadContent to skip the
@@ -168,18 +187,21 @@ public class VariationsSeedFetcher {
      * @param platform the platform parameter to let server only return experiments which can be
      * run on that platform.
      * @param restrictMode the restrict mode parameter to pass to the server via a URL param.
+     * @param milestone the milestone parameter to pass to the server via a URL param.
+     * @param channel the channel parameter to pass to the server via a URL param.
      * @return the object holds the seed data and its related header fields.
      * @throws SocketTimeoutException when fetching seed connection times out.
      * @throws UnknownHostException when fetching seed connection has an unknown host.
      * @throws IOException when response code is not HTTP_OK or transmission fails on the open
      * connection.
      */
-    public SeedInfo downloadContent(VariationsPlatform platform, String restrictMode)
+    public SeedInfo downloadContent(
+            VariationsPlatform platform, String restrictMode, String milestone, String channel)
             throws SocketTimeoutException, UnknownHostException, IOException {
         HttpURLConnection connection = null;
         try {
             long startTimeMillis = SystemClock.elapsedRealtime();
-            connection = getServerConnection(platform, restrictMode);
+            connection = getServerConnection(platform, restrictMode, milestone, channel);
             connection.setReadTimeout(READ_TIMEOUT);
             connection.setConnectTimeout(REQUEST_TIMEOUT);
             connection.setDoInput(true);

components/variations/android/junit/src/org/chromium/components/variations/firstrun/VariationsSeedFetcherTest.java

@@ -45,6 +45,10 @@ public class VariationsSeedFetcherTest {
     private VariationsSeedFetcher mFetcher;
     private SharedPreferences mPrefs;
 
+    private static final String sRestrict = "restricted";
+    private static final String sMilestone = "64";
+    private static final String sChannel = "dev";
+
     @Before
     public void setUp() throws IOException {
         ContextUtils.initApplicationContextForTests(RuntimeEnvironment.application);
@@ -55,7 +59,8 @@ public class VariationsSeedFetcherTest {
         mConnection = mock(HttpURLConnection.class);
         doReturn(mConnection)
                 .when(mFetcher)
-                .getServerConnection(VariationsSeedFetcher.VariationsPlatform.ANDROID, "");
+                .getServerConnection(VariationsSeedFetcher.VariationsPlatform.ANDROID, sRestrict,
+                        sMilestone, sChannel);
         mPrefs = ContextUtils.getAppSharedPreferences();
         mPrefs.edit().clear().apply();
     }
@@ -82,7 +87,7 @@ public class VariationsSeedFetcherTest {
         when(mConnection.getHeaderField("IM")).thenReturn("gzip");
         when(mConnection.getInputStream()).thenReturn(new ByteArrayInputStream("1234".getBytes()));
 
-        mFetcher.fetchSeed("");
+        mFetcher.fetchSeed(sRestrict, sMilestone, sChannel);
 
         assertThat(mPrefs.getString(VariationsSeedBridge.VARIATIONS_FIRST_RUN_SEED_SIGNATURE, ""),
                 equalTo("signature"));
@@ -97,40 +102,81 @@ public class VariationsSeedFetcherTest {
     }
 
     /**
-    * Test method for {@link VariationsSeedFetcher#fetchSeed()} when no fetch is needed
-    */
+     * Test method for {@link VariationsSeedFetcher#fetchSeed()} when no fetch is needed
+     */
     @Test
     public void testFetchSeed_noFetchNeeded() throws IOException {
         mPrefs.edit().putBoolean(VariationsSeedFetcher.VARIATIONS_INITIALIZED_PREF, true).apply();
 
-        mFetcher.fetchSeed("");
+        mFetcher.fetchSeed(sRestrict, sMilestone, sChannel);
 
         verify(mConnection, never()).connect();
     }
 
     /**
-    * Test method for {@link VariationsSeedFetcher#fetchSeed()} with a bad response
-    */
+     * Test method for {@link VariationsSeedFetcher#fetchSeed()} with a bad response
+     */
     @Test
     public void testFetchSeed_badResponse() throws IOException {
         when(mConnection.getResponseCode()).thenReturn(HttpURLConnection.HTTP_NOT_FOUND);
 
-        mFetcher.fetchSeed("");
+        mFetcher.fetchSeed(sRestrict, sMilestone, sChannel);
 
         assertTrue(mPrefs.getBoolean(VariationsSeedFetcher.VARIATIONS_INITIALIZED_PREF, false));
         assertFalse(VariationsSeedBridge.hasJavaPref());
     }
 
     /**
-    * Test method for {@link VariationsSeedFetcher#fetchSeed()} with an exception when connecting
-    */
+     * Test method for {@link VariationsSeedFetcher#fetchSeed()} with an exception when connecting
+     */
     @Test
     public void testFetchSeed_IOException() throws IOException {
         doThrow(new IOException()).when(mConnection).connect();
 
-        mFetcher.fetchSeed("");
+        mFetcher.fetchSeed(sRestrict, sMilestone, sChannel);
 
         assertTrue(mPrefs.getBoolean(VariationsSeedFetcher.VARIATIONS_INITIALIZED_PREF, false));
         assertFalse(VariationsSeedBridge.hasJavaPref());
     }
+
+    /**
+     * Test method for {@link VariationsSeedFetcher#getConnectionString()} has URl params.
+     */
+    @Test
+    public void testGetConnectionString_HasParams() throws IOException {
+        String urlString = mFetcher.getConnectionString(
+                VariationsSeedFetcher.VariationsPlatform.ANDROID, sRestrict, sMilestone, sChannel);
+
+        // Has the params.
+        assertTrue(urlString, urlString.contains("restrict"));
+        assertTrue(urlString, urlString.contains("osname"));
+        assertTrue(urlString, urlString.contains("milestone"));
+        assertTrue(urlString, urlString.contains("channel"));
+
+        // Has the param values.
+        assertTrue(urlString, urlString.contains(sRestrict));
+        assertTrue(urlString, urlString.contains(sMilestone));
+        assertTrue(urlString, urlString.contains(sChannel));
+
+        urlString = mFetcher.getConnectionString(
+                VariationsSeedFetcher.VariationsPlatform.ANDROID, "", sMilestone, sChannel);
+        assertFalse(urlString.contains("restrict"));
+        assertTrue(urlString.contains("osname"));
+        assertTrue(urlString.contains("milestone"));
+        assertTrue(urlString.contains("channel"));
+
+        urlString = mFetcher.getConnectionString(
+                VariationsSeedFetcher.VariationsPlatform.ANDROID, sRestrict, "", sChannel);
+        assertTrue(urlString.contains("restrict"));
+        assertTrue(urlString.contains("osname"));
+        assertFalse(urlString.contains("milestone"));
+        assertTrue(urlString.contains("channel"));
+
+        urlString = mFetcher.getConnectionString(
+                VariationsSeedFetcher.VariationsPlatform.ANDROID, sRestrict, sMilestone, "");
+        assertTrue(urlString.contains("restrict"));
+        assertTrue(urlString.contains("osname"));
+        assertTrue(urlString.contains("milestone"));
+        assertFalse(urlString.contains("channel"));
+    }
 }

components/variations/service/variations_service.cc

@@ -16,6 +16,7 @@
 #include "base/memory/ptr_util.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/metrics/sparse_histogram.h"
+#include "base/strings/string_split.h"
 #include "base/strings/string_util.h"
 #include "base/sys_info.h"
 #include "base/task_runner_util.h"
@@ -38,6 +39,7 @@
 #include "components/variations/variations_seed_simulator.h"
 #include "components/variations/variations_switches.h"
 #include "components/variations/variations_url_constants.h"
+#include "components/version_info/version_info.h"
 #include "net/base/load_flags.h"
 #include "net/base/net_errors.h"
 #include "net/base/network_change_notifier.h"
@@ -344,6 +346,21 @@ GURL VariationsService::GetVariationsServerURL(
   server_url = net::AppendOrReplaceQueryParameter(server_url, "osname",
                                                   GetPlatformString());
 
+  // Add channel to the request URL.
+  version_info::Channel channel = client_->GetChannel();
+  if (channel != version_info::Channel::UNKNOWN) {
+    server_url = net::AppendOrReplaceQueryParameter(
+        server_url, "channel", version_info::GetChannelString(channel));
+  }
+
+  // Add milestone to the request URL.
+  std::string version = version_info::GetVersionNumber();
+  std::vector<std::string> version_parts = base::SplitString(
+      version, ".", base::TRIM_WHITESPACE, base::SPLIT_WANT_NONEMPTY);
+  if (version_parts.size() > 0) {
+    server_url = net::AppendOrReplaceQueryParameter(server_url, "milestone",
+                                                    version_parts[0]);
+  }
   DCHECK(server_url.is_valid());
   return server_url;
 }

components/variations/service/variations_service.h

@@ -214,6 +214,7 @@ class VariationsService
                            LoadPermanentConsistencyCountry);
   FRIEND_TEST_ALL_PREFIXES(VariationsServiceTest, CountryHeader);
   FRIEND_TEST_ALL_PREFIXES(VariationsServiceTest, GetVariationsServerURL);
+  FRIEND_TEST_ALL_PREFIXES(VariationsServiceTest, VariationsURLHasParams);
   FRIEND_TEST_ALL_PREFIXES(VariationsServiceTest, RequestsInitiallyAllowed);
   FRIEND_TEST_ALL_PREFIXES(VariationsServiceTest, RequestsInitiallyNotAllowed);
   FRIEND_TEST_ALL_PREFIXES(VariationsServiceTest,

components/variations/service/variations_service_client.h

@@ -33,7 +33,7 @@ class VariationsServiceClient {
 
   // Returns a callback that when run returns the base::Version to use for
   // variations seed simulation. VariationsService guarantees that the callback
-  // will be run on a background threadt that permits blocking.
+  // will be run on a background thread that permits blocking.
   virtual base::Callback<base::Version(void)>
   GetVersionForSimulationCallback() = 0;
 

components/variations/service/variations_service_unittest.cc

@@ -72,9 +72,7 @@ class TestVariationsServiceClient : public VariationsServiceClient {
   network_time::NetworkTimeTracker* GetNetworkTimeTracker() override {
     return nullptr;
   }
-  version_info::Channel GetChannel() override {
-    return version_info::Channel::UNKNOWN;
-  }
+  version_info::Channel GetChannel() override { return channel_; }
   bool OverridesRestrictParameter(std::string* parameter) override {
     if (restrict_parameter_.empty())
       return false;
@@ -86,8 +84,11 @@ class TestVariationsServiceClient : public VariationsServiceClient {
     restrict_parameter_ = value;
   }
 
+  void set_channel(version_info::Channel channel) { channel_ = channel; }
+
  private:
   std::string restrict_parameter_;
+  version_info::Channel channel_ = version_info::Channel::UNKNOWN;
 
   DISALLOW_COPY_AND_ASSIGN(TestVariationsServiceClient);
 };
@@ -342,15 +343,34 @@ TEST_F(VariationsServiceTest, GetVariationsServerURL) {
   EXPECT_EQ("override", value);
 }
 
-TEST_F(VariationsServiceTest, VariationsURLHasOSNameParam) {
-  TestVariationsService service(
+TEST_F(VariationsServiceTest, VariationsURLHasParams) {
+  std::unique_ptr<TestVariationsServiceClient> client =
+      base::MakeUnique<TestVariationsServiceClient>();
+  TestVariationsServiceClient* raw_client = client.get();
+  VariationsService service(
+      std::move(client),
       base::MakeUnique<web_resource::TestRequestAllowedNotifier>(&prefs_),
-      &prefs_, GetMetricsStateManager());
-  const GURL url = service.GetVariationsServerURL(&prefs_, std::string());
+      &prefs_, GetMetricsStateManager(), UIStringOverrider());
+  raw_client->set_channel(version_info::Channel::UNKNOWN);
+  GURL url = service.GetVariationsServerURL(&prefs_, std::string());
 
   std::string value;
   EXPECT_TRUE(net::GetValueForKeyInQuery(url, "osname", &value));
   EXPECT_FALSE(value.empty());
+
+  std::string milestone;
+  EXPECT_TRUE(net::GetValueForKeyInQuery(url, "milestone", &milestone));
+  EXPECT_FALSE(milestone.empty());
+
+  // Channel param should not be present for UNKNOWN channel.
+  std::string channel;
+  EXPECT_FALSE(net::GetValueForKeyInQuery(url, "channel", &channel));
+  EXPECT_TRUE(channel.empty());
+
+  raw_client->set_channel(version_info::Channel::STABLE);
+  url = service.GetVariationsServerURL(&prefs_, std::string());
+  EXPECT_TRUE(net::GetValueForKeyInQuery(url, "channel", &channel));
+  EXPECT_FALSE(channel.empty());
 }
 
 TEST_F(VariationsServiceTest, RequestsInitiallyNotAllowed) {