[signin] Reset DiceTabHelper to initial state when reusing signin tab

Bug: 791948
Change-Id: Ie8f9c43ace98082e86d1bfbe01bc3ffe58a01fd6
Reviewed-on: https://chromium-review.googlesource.com/808979
Commit-Queue: David Roger <droger@chromium.org>
Reviewed-by: Trent Apted <tapted@chromium.org>
Reviewed-by: Mihai Sardarescu <msarda@chromium.org>
Cr-Commit-Position: refs/heads/master@{#522397}

chrome/browser/signin/dice_tab_helper.cc

@@ -23,16 +23,12 @@ DiceTabHelper::DiceTabHelper(content::WebContents* web_contents)
 
 DiceTabHelper::~DiceTabHelper() {}
 
-void DiceTabHelper::SetSigninAccessPoint(
-    signin_metrics::AccessPoint access_point) {
-  DCHECK_EQ(signin_metrics::AccessPoint::ACCESS_POINT_UNKNOWN,
-            signin_access_point_);
+void DiceTabHelper::InitializeSigninFlow(
+    signin_metrics::AccessPoint access_point,
+    signin_metrics::Reason reason) {
   signin_access_point_ = access_point;
-}
-
-void DiceTabHelper::SetSigninReason(signin_metrics::Reason reason) {
-  DCHECK_EQ(signin_metrics::Reason::REASON_UNKNOWN_REASON, signin_reason_);
   signin_reason_ = reason;
+  should_start_sync_after_web_signin_ = true;
 }
 
 void DiceTabHelper::DidStartNavigation(

chrome/browser/signin/dice_tab_helper.h

@@ -28,11 +28,10 @@ class DiceTabHelper : public content::WebContentsUserData<DiceTabHelper>,
     return should_start_sync_after_web_signin_;
   }
 
-  // Sets the sign-in access point. Should be called only once.
-  void SetSigninAccessPoint(signin_metrics::AccessPoint access_point);
-
-  // Sets the sign-in reason. Should be called only once.
-  void SetSigninReason(signin_metrics::Reason reason);
+  // Initializes the DiceTabHelper for a new signin flow. Must be called once
+  // per signin flow happening in the tab.
+  void InitializeSigninFlow(signin_metrics::AccessPoint access_point,
+                            signin_metrics::Reason reason);
 
   // content::WebContentsObserver:
   void DidStartNavigation(

chrome/browser/signin/dice_tab_helper_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/dice_tab_helper.h"
+
+#include "chrome/test/base/testing_profile.h"
+#include "components/signin/core/browser/signin_metrics.h"
+#include "content/public/test/test_browser_thread_bundle.h"
+#include "content/public/test/test_web_contents_factory.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+// Tests DiceTabHelper intialization.
+TEST(DiceTabHelperTest, Initialization) {
+  content::TestBrowserThreadBundle thread_bundle;
+  TestingProfile profile;
+  content::TestWebContentsFactory factory;
+  content::WebContents* web_contents = factory.CreateWebContents(&profile);
+  DiceTabHelper::CreateForWebContents(web_contents);
+  DiceTabHelper* dice_tab_helper = DiceTabHelper::FromWebContents(web_contents);
+
+  // Check default state.
+  EXPECT_EQ(signin_metrics::AccessPoint::ACCESS_POINT_UNKNOWN,
+            dice_tab_helper->signin_access_point());
+  EXPECT_EQ(signin_metrics::Reason::REASON_UNKNOWN_REASON,
+            dice_tab_helper->signin_reason());
+  EXPECT_TRUE(dice_tab_helper->should_start_sync_after_web_signin());
+
+  // Initialize the signin flow.
+  signin_metrics::AccessPoint access_point =
+      signin_metrics::AccessPoint::ACCESS_POINT_BOOKMARK_BUBBLE;
+  signin_metrics::Reason reason =
+      signin_metrics::Reason::REASON_SIGNIN_PRIMARY_ACCOUNT;
+  dice_tab_helper->InitializeSigninFlow(access_point, reason);
+  EXPECT_EQ(access_point, dice_tab_helper->signin_access_point());
+  EXPECT_EQ(reason, dice_tab_helper->signin_reason());
+}

chrome/browser/ui/signin_view_controller.cc

@@ -154,8 +154,7 @@ void SigninViewController::ShowDiceSigninTab(
   DCHECK_EQ(signin_url, active_contents->GetVisibleURL());
   DiceTabHelper::CreateForWebContents(active_contents);
   DiceTabHelper* tab_helper = DiceTabHelper::FromWebContents(active_contents);
-  tab_helper->SetSigninAccessPoint(access_point);
-  tab_helper->SetSigninReason(signin_reason);
+  tab_helper->InitializeSigninFlow(access_point, signin_reason);
 
   if (signin_reason == signin_metrics::Reason::REASON_SIGNIN_PRIMARY_ACCOUNT) {
     signin_metrics::LogSigninAccessPointStarted(access_point);

chrome/test/BUILD.gn

@@ -2660,7 +2660,10 @@ test("unit_tests") {
   }
 
   if (enable_dice_support) {
-    sources += [ "../browser/signin/dice_response_handler_unittest.cc" ]
+    sources += [
+      "../browser/signin/dice_response_handler_unittest.cc",
+      "../browser/signin/dice_tab_helper_unittest.cc",
+    ]
   }
 
   if (enable_offline_pages) {
@@ -2932,8 +2935,6 @@ test("unit_tests") {
       "../browser/media/router/discovery/mdns/dns_sd_registry_unittest.cc",
       "../browser/media/router/discovery/media_sink_discovery_metrics_unittest.cc",
       "../browser/media/router/event_page_request_manager_unittest.cc",
-      "../common/media_router/discovery/media_sink_internal_unittest.cc",
-      "../common/media_router/discovery/media_sink_service_base_unittest.cc",
       "../browser/media/router/mojo/extension_media_route_provider_proxy_unittest.cc",
       "../browser/media/router/mojo/media_route_controller_unittest.cc",
       "../browser/media/router/mojo/media_router_desktop_unittest.cc",
@@ -2961,6 +2962,8 @@ test("unit_tests") {
       "../browser/ui/webui/media_router/media_router_web_ui_test.h",
       "../browser/ui/webui/media_router/media_router_webui_message_handler_unittest.cc",
       "../browser/ui/webui/media_router/query_result_manager_unittest.cc",
+      "../common/media_router/discovery/media_sink_internal_unittest.cc",
+      "../common/media_router/discovery/media_sink_service_base_unittest.cc",
       "../common/media_router/mojo/media_router_struct_traits_unittest.cc",
     ]
     deps += [ "//components/bubble:test_support" ]