Refactor signin_poromo.h/cc and add unittest for it.

Using build-in util URL function to add path and query to the url
instead of using StringAppendF.

Bug: 
Change-Id: Ie62bff86da96e62bf41ed6097adbe43956efe99f
Reviewed-on: https://chromium-review.googlesource.com/775934Reviewed-by: Mihai Sardarescu <msarda@chromium.org>
Commit-Queue: Owen Min <zmin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#517446}

chrome/browser/signin/signin_promo.cc

@@ -7,9 +7,6 @@
 #include <limits.h>
 
 #include "base/strings/string_number_conversions.h"
-#include "base/strings/string_util.h"
-#include "base/strings/stringprintf.h"
-#include "base/strings/utf_string_conversions.h"
 #include "build/build_config.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/extensions/signin/gaia_auth_extension_loader.h"
@@ -35,8 +32,6 @@
 #include "content/public/browser/web_contents.h"
 #include "content/public/browser/web_ui.h"
 #include "content/public/browser/web_ui_data_source.h"
-#include "google_apis/gaia/gaia_urls.h"
-#include "net/base/escape.h"
 #include "net/base/url_util.h"
 #include "url/gurl.h"
 
@@ -98,16 +93,22 @@ GURL GetPromoURL(signin_metrics::AccessPoint access_point,
   CHECK_NE(static_cast<int>(reason),
            static_cast<int>(signin_metrics::Reason::REASON_UNKNOWN_REASON));
 
-  std::string url(chrome::kChromeUIChromeSigninURL);
-  base::StringAppendF(&url, "?%s=%d", signin::kSignInPromoQueryKeyAccessPoint,
-                      static_cast<int>(access_point));
-  base::StringAppendF(&url, "&%s=%d", signin::kSignInPromoQueryKeyReason,
-                      static_cast<int>(reason));
-  if (auto_close)
-    base::StringAppendF(&url, "&%s=1", signin::kSignInPromoQueryKeyAutoClose);
-  if (is_constrained)
-    base::StringAppendF(&url, "&%s=1", signin::kSignInPromoQueryKeyConstrained);
-  return GURL(url);
+  GURL url(chrome::kChromeUIChromeSigninURL);
+  url = net::AppendQueryParameter(
+      url, signin::kSignInPromoQueryKeyAccessPoint,
+      base::IntToString(static_cast<int>(access_point)));
+  url = net::AppendQueryParameter(url, signin::kSignInPromoQueryKeyReason,
+                                  base::IntToString(static_cast<int>(reason)));
+  if (auto_close) {
+    url = net::AppendQueryParameter(url, signin::kSignInPromoQueryKeyAutoClose,
+                                    "1");
+  }
+  if (is_constrained) {
+    url = net::AppendQueryParameter(
+        url, signin::kSignInPromoQueryKeyConstrained, "1");
+  }
+
+  return url;
 }
 
 GURL GetReauthURL(signin_metrics::AccessPoint access_point,
@@ -187,26 +188,28 @@ void SetUserSkippedPromo(Profile* profile) {
 }
 
 GURL GetLandingURL(signin_metrics::AccessPoint access_point) {
-  std::string url = base::StringPrintf(
-      "%s/success.html?%s=%d", extensions::kGaiaAuthExtensionOrigin,
-      kSignInPromoQueryKeyAccessPoint, static_cast<int>(access_point));
+  GURL url(extensions::kGaiaAuthExtensionOrigin);
+  GURL::Replacements replacements;
+  replacements.SetPathStr(kSigninPromoLandingURLSuccessPage);
+  url = url.ReplaceComponents(replacements);
+
+  url = net::AppendQueryParameter(
+      url, kSignInPromoQueryKeyAccessPoint,
+      base::IntToString(static_cast<int>(access_point)));
 
   // TODO(gogerald): right now, gaia server needs to distinguish the source from
   // signin_metrics::SOURCE_START_PAGE, signin_metrics::SOURCE_SETTINGS and
   // the others to show advanced sync settings, remove them after
   // switching to Minute Maid sign in flow.
+  signin_metrics::Source source = signin_metrics::SOURCE_OTHERS;
   if (access_point == signin_metrics::AccessPoint::ACCESS_POINT_START_PAGE) {
-    base::StringAppendF(&url, "&%s=%d", kSignInPromoQueryKeySource,
-                        signin_metrics::SOURCE_START_PAGE);
+    source = signin_metrics::SOURCE_START_PAGE;
   } else if (access_point ==
              signin_metrics::AccessPoint::ACCESS_POINT_SETTINGS) {
-    base::StringAppendF(&url, "&%s=%d", kSignInPromoQueryKeySource,
-                        signin_metrics::SOURCE_SETTINGS);
-  } else {
-    base::StringAppendF(&url, "&%s=%d", kSignInPromoQueryKeySource,
-                        signin_metrics::SOURCE_OTHERS);
+    source = signin_metrics::SOURCE_SETTINGS;
   }
-
+  url = net::AppendQueryParameter(url, signin::kSignInPromoQueryKeySource,
+                                  base::IntToString(static_cast<int>(source)));
   return GURL(url);
 }
 

chrome/browser/signin/signin_promo.h

@@ -29,6 +29,7 @@ const char kSignInPromoQueryKeySource[] = "source";
 const char kSignInPromoQueryKeyConstrained[] = "constrained";
 const char kSignInPromoQueryKeyShowAccountManagement[] =
     "showAccountManagement";
+const char kSigninPromoLandingURLSuccessPage[] = "success.html";
 
 // Returns true if we should show the sign in promo at startup.
 bool ShouldShowPromoAtStartup(Profile* profile, bool is_new_profile);

chrome/browser/signin/signin_promo_unittest.cc

+// 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.
+
+#include "chrome/browser/signin/signin_promo.h"
+
+#include "testing/gtest/include/gtest/gtest.h"
+#include "url/gurl.h"
+
+namespace signin {
+
+class SigninPromoTest : public ::testing::Test {};
+
+TEST_F(SigninPromoTest, TestPromoURL) {
+  GURL expected_url_1(
+      "chrome://chrome-signin/"
+      "?access_point=0&reason=0&auto_close=1&constrained=1");
+  EXPECT_EQ(expected_url_1,
+            GetPromoURLForDialog(
+                signin_metrics::AccessPoint::ACCESS_POINT_START_PAGE,
+                signin_metrics::Reason::REASON_SIGNIN_PRIMARY_ACCOUNT, true));
+  GURL expected_url_2(
+      "chrome://chrome-signin/?access_point=15&reason=3&constrained=1");
+  EXPECT_EQ(expected_url_2,
+            GetPromoURLForDialog(
+                signin_metrics::AccessPoint::ACCESS_POINT_SIGNIN_PROMO,
+                signin_metrics::Reason::REASON_UNLOCK, false));
+}
+
+TEST_F(SigninPromoTest, TestReauthURL) {
+  GURL expected_url_1(
+      "chrome://chrome-signin/"
+      "?access_point=0&reason=0&auto_close=1&constrained=1&email=example%"
+      "40domain.com&validateEmail=1&readOnlyEmail=1");
+  EXPECT_EQ(expected_url_1,
+            GetReauthURLWithEmailForDialog(
+                signin_metrics::AccessPoint::ACCESS_POINT_START_PAGE,
+                signin_metrics::Reason::REASON_SIGNIN_PRIMARY_ACCOUNT,
+                "example@domain.com"));
+}
+
+TEST_F(SigninPromoTest, TestLandingURL) {
+  GURL expected_url_1(
+      "chrome-extension://mfffpogegjflfpflabcdkioaeobkgjik/"
+      "success.html?access_point=1&source=13");
+  EXPECT_EQ(expected_url_1,
+            GetLandingURL(signin_metrics::AccessPoint::ACCESS_POINT_NTP_LINK));
+  GURL expected_url_2(
+      "chrome-extension://mfffpogegjflfpflabcdkioaeobkgjik/"
+      "success.html?access_point=0&source=0");
+  EXPECT_EQ(
+      expected_url_2,
+      GetLandingURL(signin_metrics::AccessPoint::ACCESS_POINT_START_PAGE));
+  GURL expected_url_3(
+      "chrome-extension://mfffpogegjflfpflabcdkioaeobkgjik/"
+      "success.html?access_point=3&source=3");
+  EXPECT_EQ(expected_url_3,
+            GetLandingURL(signin_metrics::AccessPoint::ACCESS_POINT_SETTINGS));
+}
+
+}  // namespace signin

chrome/test/BUILD.gn

@@ -2745,6 +2745,7 @@ test("unit_tests") {
       "../browser/search/search_unittest.cc",
       "../browser/sessions/persistent_tab_restore_service_unittest.cc",
       "../browser/signin/mutable_profile_oauth2_token_service_delegate_unittest.cc",
+      "../browser/signin/signin_promo_unittest.cc",
       "../browser/speech/extension_api/extension_manifests_tts_unittest.cc",
       "../browser/speech/tts_controller_unittest.cc",
       "../browser/sync/sessions/browser_list_router_helper_unittest.cc",