Increase visibility into HatsService failures

DevTools is intending to share Chrome's HaTS implementation, but we have
a slightly different use-case as we are presenting 'persistent' surveys,
which are triggered by user interaction like clicking a link to take a
survey. These surveys are always triggered on the server-side, and don't
have a client-side rate limit.

DevTools' surveys will be triggered by the user clicking on a link. If
we know that a survey won't succeed, we want to avoid showing the link
in the first place. We've extended the HatsService API with
CanShowSurvey() to indicate whether we already know a survey will fail.

We also want to know whether a survey has failed to show to the user,
even after the initial CanShowSurvey check, which is possible. To get a
signal for this we've added success and failure callbacks to
LaunchSurvey().

As DevTools will use persistent, user-prompted surveys, we've added
the notion of user-prompted to the SurveyConfig struct. This changes
some survey triggering behavior:
 - We remove the minimum profile age check.
 - We remove the last started time for this survey check.
 - We remove the last started time for any survey check.

We still trigger the UMA stats in CanShowSurvey(), even though they will
be duplicated in LaunchSurvey. The reasoning is that the UMA stats are
only recorded for 'failed' cases where CanShowSurvey() returns false -
the expectation is that the caller will not then call LaunchSurvey and
trigger a 2nd UMA stat which would potentially skew results.

Doc: https://docs.google.com/document/d/1wMRb1hI2zJ1mzOSJjjF46C2hToGksQf2AxurorxaiTg

Bug: 1112738
Change-Id: Id48fb9296fbce898fc6428cd562990e25e96457b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2485940
Commit-Queue: Peter Marshall <petermarshall@chromium.org>
Reviewed-by: Theodore Olsauskas-Warren <sauski@google.com>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#822079}

chrome/browser/ui/browser_window.h

@@ -468,7 +468,11 @@ class BrowserWindow : public ui::BaseWindow {
   // Shows User Happiness Tracking Survey's invitation bubble when possible
   // (such as having the proper anchor view).
   // |site_id| is the site identification of the survey the bubble leads to.
-  virtual void ShowHatsBubble(const std::string& site_id) = 0;
+  // Note: |success_callback| and |failure_callback| are discarded for HaTS v1
+  // surveys, which are deprecated (crbug.com/1143176).
+  virtual void ShowHatsBubble(const std::string& site_id,
+                              base::OnceClosure success_callback,
+                              base::OnceClosure failure_callback) = 0;
 
   // Returns object implementing ExclusiveAccessContext interface.
   virtual ExclusiveAccessContext* GetExclusiveAccessContext() = 0;

chrome/browser/ui/hats/hats_service.h

@@ -9,6 +9,7 @@
 #include <set>
 #include <string>
 
+#include "base/callback.h"
 #include "base/containers/flat_map.h"
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
@@ -46,8 +47,10 @@ extern const char kHatsShouldShowSurveyReasonHistogram[];
 class HatsService : public KeyedService {
  public:
   struct SurveyConfig {
-    SurveyConfig(const double probability, const std::string en_site_id)
-        : probability_(probability), en_site_id_(en_site_id) {}
+    SurveyConfig(double probability, std::string en_site_id, bool user_prompted)
+        : probability_(probability),
+          en_site_id_(std::move(en_site_id)),
+          user_prompted_(user_prompted) {}
 
     SurveyConfig() = default;
 
@@ -56,6 +59,10 @@ class HatsService : public KeyedService {
 
     // Site ID for the survey.
     std::string en_site_id_;
+
+    // The survey will prompt every time because the user has explicitly decided
+    // to take the survey e.g. clicking a link.
+    bool user_prompted_;
   };
 
   struct SurveyMetadata {
@@ -135,7 +142,12 @@ class HatsService : public KeyedService {
   static void RegisterProfilePrefs(PrefRegistrySimple* registry);
 
   // Launches survey with identifier |trigger| if appropriate.
-  virtual void LaunchSurvey(const std::string& trigger);
+  // |success_callback| is called when the survey is shown to the user.
+  // |failure_callback| is called if the survey does not launch for any reason.
+  virtual void LaunchSurvey(
+      const std::string& trigger,
+      base::OnceClosure success_callback = base::DoNothing(),
+      base::OnceClosure failure_callback = base::DoNothing());
 
   // Launches survey (with id |trigger|) with a timeout |timeout_ms| if
   // appropriate. Survey will be shown at the active window/tab by the
@@ -171,6 +183,12 @@ class HatsService : public KeyedService {
       std::unique_ptr<HatsSurveyStatusChecker> checker);
   bool HasPendingTasks();
 
+  // Whether the survey specified by |trigger| can be shown to the user. This
+  // is a pre-check that calculates as many conditions as possible, but could
+  // still return a false positive due to client-side rate limiting, a change
+  // in network conditions, or intervening calls to this API.
+  bool CanShowSurvey(const std::string& trigger) const;
+
  private:
   friend class DelayedSurveyTask;
   FRIEND_TEST_ALL_PREFIXES(HatsServiceHatsNext, SingleHatsNextDialog);
@@ -178,14 +196,21 @@ class HatsService : public KeyedService {
   void LaunchSurveyForWebContents(const std::string& trigger,
                                   content::WebContents* web_contents);
 
-  void LaunchSurveyForBrowser(const std::string& trigger, Browser* browser);
+  void LaunchSurveyForBrowser(Browser* browser,
+                              const std::string& trigger,
+                              base::OnceClosure success_callback,
+                              base::OnceClosure failure_callback);
 
   // Returns true is the survey trigger specified should be shown.
   bool ShouldShowSurvey(const std::string& trigger) const;
 
-  // Check whether the survey is reachable and under capacity.
+  // Check whether the survey is reachable and under capacity and show it.
+  // |success_callback| is called when the survey is shown to the user.
+  // |failure_callback| is called if the survey does not launch for any reason.
   void CheckSurveyStatusAndMaybeShow(Browser* browser,
-                                     const std::string& trigger);
+                                     const std::string& trigger,
+                                     base::OnceClosure success_callback,
+                                     base::OnceClosure failure_callback);
 
   // Callbacks for survey capacity checking.
   void ShowSurvey(Browser* browser, const std::string& trigger);

chrome/browser/ui/hats/mock_hats_service.h

@@ -22,7 +22,12 @@ class MockHatsService : public HatsService {
   explicit MockHatsService(Profile* profile);
   ~MockHatsService() override;
 
-  MOCK_METHOD(void, LaunchSurvey, (const std::string& trigger), (override));
+  MOCK_METHOD(void,
+              LaunchSurvey,
+              (const std::string& trigger,
+               base::OnceClosure success_callback,
+               base::OnceClosure failure_callback),
+              (override));
   MOCK_METHOD(bool,
               LaunchDelayedSurvey,
               (const std::string& trigger, int timeout_ms),

chrome/browser/ui/views/frame/browser_view.cc

@@ -3440,8 +3440,12 @@ void BrowserView::ShowAvatarBubbleFromAvatarButton(
                                   focus_first_profile_button);
 }
 
-void BrowserView::ShowHatsBubble(const std::string& site_id) {
-  HatsBubbleView::ShowOnContentReady(browser(), site_id);
+void BrowserView::ShowHatsBubble(const std::string& site_id,
+                                 base::OnceClosure success_callback,
+                                 base::OnceClosure failure_callback) {
+  HatsBubbleView::ShowOnContentReady(browser(), site_id,
+                                     std::move(success_callback),
+                                     std::move(failure_callback));
 }
 
 ExclusiveAccessContext* BrowserView::GetExclusiveAccessContext() {

chrome/browser/ui/views/frame/browser_view.h

@@ -468,7 +468,9 @@ class BrowserView : public BrowserWindow,
       AvatarBubbleMode mode,
       signin_metrics::AccessPoint access_point,
       bool is_source_keyboard) override;
-  void ShowHatsBubble(const std::string& site_id) override;
+  void ShowHatsBubble(const std::string& site_id,
+                      base::OnceClosure success_callback,
+                      base::OnceClosure failure_callback) override;
   ExclusiveAccessContext* GetExclusiveAccessContext() override;
   std::string GetWorkspace() const override;
   bool IsVisibleOnAllWorkspaces() const override;

chrome/browser/ui/views/hats/hats_browsertest.cc

@@ -9,6 +9,7 @@
 #include "base/files/file_util.h"
 #include "base/path_service.h"
 #include "base/run_loop.h"
+#include "base/test/bind_test_util.h"
 #include "base/test/metrics/histogram_tester.h"
 #include "base/test/scoped_feature_list.h"
 #include "base/time/time.h"
@@ -40,7 +41,7 @@ class HatsBubbleTest : public DialogBrowserTest {
   void ShowUi(const std::string& name) override {
     ASSERT_TRUE(browser()->is_type_normal());
     BrowserView::GetBrowserViewForBrowser(InProcessBrowserTest::browser())
-        ->ShowHatsBubble("test_site_id");
+        ->ShowHatsBubble("test_site_id", base::DoNothing(), base::DoNothing());
   }
 
  private:
@@ -195,8 +196,15 @@ class MockHatsNextWebDialog : public HatsNextWebDialog {
   MockHatsNextWebDialog(Browser* browser,
                         const std::string& trigger_id,
                         const GURL& hats_survey_url,
-                        const base::TimeDelta& timeout)
-      : HatsNextWebDialog(browser, trigger_id, hats_survey_url, timeout) {}
+                        const base::TimeDelta& timeout,
+                        base::OnceClosure success_callback,
+                        base::OnceClosure failure_callback)
+      : HatsNextWebDialog(browser,
+                          trigger_id,
+                          hats_survey_url,
+                          timeout,
+                          std::move(success_callback),
+                          std::move(failure_callback)) {}
 
   MOCK_METHOD0(ShowWidget, void());
   MOCK_METHOD0(CloseWidget, void());
@@ -235,6 +243,17 @@ class HatsNextWebDialogBrowserTest : public InProcessBrowserTest {
 
   MockHatsService* hats_service() { return hats_service_; }
 
+  base::OnceClosure GetSuccessClosure() {
+    return base::BindLambdaForTesting([&]() { ++success_count; });
+  }
+
+  base::OnceClosure GetFailureClosure() {
+    return base::BindLambdaForTesting([&]() { ++failure_count; });
+  }
+
+  int success_count = 0;
+  int failure_count = 0;
+
  private:
   base::test::ScopedFeatureList feature_list_;
   MockHatsService* hats_service_;
@@ -254,7 +273,8 @@ IN_PROC_BROWSER_TEST_F(HatsNextWebDialogBrowserTest, SurveyLoaded) {
   auto* dialog = new MockHatsNextWebDialog(
       browser(), kHatsNextSurveyTriggerIDTesting,
       embedded_test_server()->GetURL("/hats/hats_next_mock.html"),
-      base::TimeDelta::FromSeconds(100));
+      base::TimeDelta::FromSeconds(100), GetSuccessClosure(),
+      GetFailureClosure());
 
   // Check that no record of a survey being shown is present.
   const base::DictionaryValue* pref_data =
@@ -277,6 +297,9 @@ IN_PROC_BROWSER_TEST_F(HatsNextWebDialogBrowserTest, SurveyLoaded) {
       }));
   run_loop.Run();
 
+  EXPECT_EQ(1, success_count);
+  EXPECT_EQ(0, failure_count);
+
   // Check that a record of the survey being shown has been recorded.
   pref_data = browser()->profile()->GetPrefs()->GetDictionary(
       prefs::kHatsSurveyMetadata);
@@ -299,12 +322,16 @@ IN_PROC_BROWSER_TEST_F(HatsNextWebDialogBrowserTest, SurveyClosed) {
   auto* dialog = new MockHatsNextWebDialog(
       browser(), "close_for_testing",
       embedded_test_server()->GetURL("/hats/hats_next_mock.html"),
-      base::TimeDelta::FromSeconds(100));
+      base::TimeDelta::FromSeconds(100), GetSuccessClosure(),
+      GetFailureClosure());
 
   // The hats_next_mock.html will provide a state update to the dialog to
   // indicate that the survey window should be closed.
   dialog->WaitForClose();
 
+  EXPECT_EQ(0, success_count);
+  EXPECT_EQ(1, failure_count);
+
   // Because no loaded state was provided, only a rejection should be recorded.
   histogram_tester.ExpectUniqueSample(
       kHatsShouldShowSurveyReasonHistogram,
@@ -321,9 +348,13 @@ IN_PROC_BROWSER_TEST_F(HatsNextWebDialogBrowserTest, SurveyLoadedThenClosed) {
   auto* dialog = new MockHatsNextWebDialog(
       browser(), kHatsNextSurveyTriggerIDTesting,
       embedded_test_server()->GetURL("/hats/hats_next_mock.html"),
-      base::TimeDelta::FromSeconds(100));
+      base::TimeDelta::FromSeconds(100), GetSuccessClosure(),
+      GetFailureClosure());
   dialog->WaitForClose();
 
+  EXPECT_EQ(1, success_count);
+  EXPECT_EQ(0, failure_count);
+
   // The only recorded sample should indicate that the survey was shown.
   histogram_tester.ExpectUniqueSample(
       kHatsShouldShowSurveyReasonHistogram,
@@ -340,9 +371,13 @@ IN_PROC_BROWSER_TEST_F(HatsNextWebDialogBrowserTest, SurveyTimeout) {
   auto* dialog = new MockHatsNextWebDialog(
       browser(), "invalid_test",
       embedded_test_server()->GetURL("/hats/non_existent.html"),
-      base::TimeDelta::FromMilliseconds(1));
+      base::TimeDelta::FromMilliseconds(1), GetSuccessClosure(),
+      GetFailureClosure());
 
   dialog->WaitForClose();
+
+  EXPECT_EQ(0, success_count);
+  EXPECT_EQ(1, failure_count);
   histogram_tester.ExpectUniqueSample(
       kHatsShouldShowSurveyReasonHistogram,
       HatsService::ShouldShowSurveyReasons::kNoSurveyUnreachable, 1);
@@ -357,9 +392,12 @@ IN_PROC_BROWSER_TEST_F(HatsNextWebDialogBrowserTest, UnknownURLFragment) {
   auto* dialog = new MockHatsNextWebDialog(
       browser(), "invalid_url_fragment_for_testing",
       embedded_test_server()->GetURL("/hats/hats_next_mock.html"),
-      base::TimeDelta::FromSeconds(100));
+      base::TimeDelta::FromSeconds(100), GetSuccessClosure(),
+      GetFailureClosure());
 
   dialog->WaitForClose();
+  EXPECT_EQ(0, success_count);
+  EXPECT_EQ(1, failure_count);
 }
 
 IN_PROC_BROWSER_TEST_F(HatsNextWebDialogBrowserTest, NewWebContents) {
@@ -368,7 +406,7 @@ IN_PROC_BROWSER_TEST_F(HatsNextWebDialogBrowserTest, NewWebContents) {
   auto* dialog = new MockHatsNextWebDialog(
       browser(), "open_new_web_contents_for_testing",
       embedded_test_server()->GetURL("/hats/hats_next_mock.html"),
-      base::TimeDelta::FromSeconds(100));
+      base::TimeDelta::FromSeconds(100), base::DoNothing(), base::DoNothing());
 
   // The mock hats dialog will push a close state after it has attempted to
   // open another web contents.
@@ -388,7 +426,7 @@ IN_PROC_BROWSER_TEST_F(HatsNextWebDialogBrowserTest, DialogResize) {
   auto* dialog = new MockHatsNextWebDialog(
       browser(), "resize_for_testing",
       embedded_test_server()->GetURL("/hats/hats_next_mock.html"),
-      base::TimeDelta::FromSeconds(100));
+      base::TimeDelta::FromSeconds(100), base::DoNothing(), base::DoNothing());
 
   // Check that the dialog reports a preferred size the same as the size defined
   // in hats_next_mock.html.
@@ -412,7 +450,7 @@ IN_PROC_BROWSER_TEST_F(HatsNextWebDialogBrowserTest, MaximumSize) {
   auto* dialog = new MockHatsNextWebDialog(
       browser(), "resize_to_large_for_testing",
       embedded_test_server()->GetURL("/hats/hats_next_mock.html"),
-      base::TimeDelta::FromSeconds(100));
+      base::TimeDelta::FromSeconds(100), base::DoNothing(), base::DoNothing());
 
   // Check that the maximum size of the dialog is bounded appropriately by the
   // dialogs maximum size. Depending on renderer warm-up, an initial empty size

chrome/browser/ui/views/hats/hats_bubble_view.cc

@@ -61,7 +61,9 @@ views::BubbleDialogDelegateView* HatsBubbleView::GetHatsBubble() {
 
 // static
 void HatsBubbleView::ShowOnContentReady(Browser* browser,
-                                        const std::string& site_id) {
+                                        const std::string& site_id,
+                                        base::OnceClosure success_callback,
+                                        base::OnceClosure failure_callback) {
   if (site_id == "test_site_id") {
     // Directly show the bubble during tests.
     HatsBubbleView::Show(browser, base::DoNothing());
@@ -75,7 +77,8 @@ void HatsBubbleView::ShowOnContentReady(Browser* browser,
   if (base::FeatureList::IsEnabled(
           features::kHappinessTrackingSurveysForDesktopMigration)) {
     // Self deleting on close.
-    new HatsNextWebDialog(browser, site_id);
+    new HatsNextWebDialog(browser, site_id, std::move(success_callback),
+                          std::move(failure_callback));
   } else {
     HatsWebDialog::Create(browser, site_id);
   }

chrome/browser/ui/views/hats/hats_bubble_view.h

@@ -40,7 +40,10 @@ class HatsBubbleView : public views::BubbleDialogDelegateView {
   static views::BubbleDialogDelegateView* GetHatsBubble();
 
   // Shows the bubble when the survey content identified by |site_id| is ready.
-  static void ShowOnContentReady(Browser* browser, const std::string& site_id);
+  static void ShowOnContentReady(Browser* browser,
+                                 const std::string& site_id,
+                                 base::OnceClosure success_callback,
+                                 base::OnceClosure failure_callback);
 
   // Shows the bubble now with supplied callback |consent_callback|.
   static void Show(Browser* browser, HatsConsentCallback consent_callback);

chrome/browser/ui/views/hats/hats_next_web_dialog.cc

@@ -101,12 +101,16 @@ class HatsNextWebDialog::HatsWebView : public views::WebView {
 };
 
 HatsNextWebDialog::HatsNextWebDialog(Browser* browser,
-                                     const std::string& trigger_id)
+                                     const std::string& trigger_id,
+                                     base::OnceClosure success_callback,
+                                     base::OnceClosure failure_callback)
     : HatsNextWebDialog(
           browser,
           trigger_id,
           GURL("https://storage.googleapis.com/chrome_hats_staging/index.html"),
-          base::TimeDelta::FromSeconds(10)) {}
+          base::TimeDelta::FromSeconds(10),
+          std::move(success_callback),
+          std::move(failure_callback)) {}
 
 gfx::Size HatsNextWebDialog::CalculatePreferredSize() const {
   gfx::Size preferred_size = views::View::CalculatePreferredSize();
@@ -123,7 +127,9 @@ void HatsNextWebDialog::OnProfileWillBeDestroyed(Profile* profile) {
 HatsNextWebDialog::HatsNextWebDialog(Browser* browser,
                                      const std::string& trigger_id,
                                      const GURL& hats_survey_url,
-                                     const base::TimeDelta& timeout)
+                                     const base::TimeDelta& timeout,
+                                     base::OnceClosure success_callback,
+                                     base::OnceClosure failure_callback)
     : BubbleDialogDelegateView(BrowserView::GetBrowserViewForBrowser(browser)
                                    ->toolbar_button_provider()
                                    ->GetAppMenuButton(),
@@ -133,7 +139,9 @@ HatsNextWebDialog::HatsNextWebDialog(Browser* browser,
       browser_(browser),
       trigger_id_(trigger_id),
       hats_survey_url_(hats_survey_url),
-      timeout_(timeout) {
+      timeout_(timeout),
+      success_callback_(std::move(success_callback)),
+      failure_callback_(std::move(failure_callback)) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   otr_profile_->AddObserver(this);
   set_close_on_deactivate(false);
@@ -184,6 +192,7 @@ void HatsNextWebDialog::LoadTimedOut() {
       kHatsShouldShowSurveyReasonHistogram,
       HatsService::ShouldShowSurveyReasons::kNoSurveyUnreachable);
   CloseWidget();
+  std::move(failure_callback_).Run();
 }
 
 void HatsNextWebDialog::OnSurveyStateUpdateReceived(std::string state) {
@@ -197,6 +206,7 @@ void HatsNextWebDialog::OnSurveyStateUpdateReceived(std::string state) {
     service->RecordSurveyAsShown(trigger_id_);
     received_survey_loaded_ = true;
     ShowWidget();
+    std::move(success_callback_).Run();
   } else if (state == "close") {
     if (!received_survey_loaded_) {
       // Receiving a close state prior to a loaded state indicates that contact
@@ -206,12 +216,14 @@ void HatsNextWebDialog::OnSurveyStateUpdateReceived(std::string state) {
       base::UmaHistogramEnumeration(
           kHatsShouldShowSurveyReasonHistogram,
           HatsService::ShouldShowSurveyReasons::kNoRejectedByHatsService);
+      std::move(failure_callback_).Run();
     }
     CloseWidget();
   } else {
     LOG(ERROR) << "Unknown state provided in URL fragment by HaTS survey:"
                << state;
     CloseWidget();
+    std::move(failure_callback_).Run();
   }
 }
 

chrome/browser/ui/views/hats/hats_next_web_dialog.h

@@ -29,7 +29,10 @@ class HatsNextWebDialog : public views::BubbleDialogDelegateView,
                           public content::WebContentsDelegate,
                           public ProfileObserver {
  public:
-  HatsNextWebDialog(Browser* browser, const std::string& trigger_id);
+  HatsNextWebDialog(Browser* browser,
+                    const std::string& trigger_id,
+                    base::OnceClosure success_callback,
+                    base::OnceClosure failure_callback);
   ~HatsNextWebDialog() override;
   HatsNextWebDialog(const HatsNextWebDialog&) = delete;
   HatsNextWebDialog& operator=(const HatsNextWebDialog&) = delete;
@@ -49,7 +52,9 @@ class HatsNextWebDialog : public views::BubbleDialogDelegateView,
   HatsNextWebDialog(Browser* browser,
                     const std::string& trigger_id,
                     const GURL& hats_survey_url_,
-                    const base::TimeDelta& timeout);
+                    const base::TimeDelta& timeout,
+                    base::OnceClosure success_callback,
+                    base::OnceClosure failure_callback);
 
   class HatsWebView;
 
@@ -114,6 +119,9 @@ class HatsNextWebDialog : public views::BubbleDialogDelegateView,
 
   base::TimeDelta timeout_;
 
+  base::OnceClosure success_callback_;
+  base::OnceClosure failure_callback_;
+
   base::WeakPtrFactory<HatsNextWebDialog> weak_factory_{this};
 };
 

chrome/test/base/test_browser_window.h

@@ -177,7 +177,9 @@ class TestBrowserWindow : public BrowserWindow {
 
 #if defined(OS_CHROMEOS) || defined(OS_MAC) || defined(OS_WIN) || \
     defined(OS_LINUX)
-  void ShowHatsBubble(const std::string& site_id) override {}
+  void ShowHatsBubble(const std::string& site_id,
+                      base::OnceClosure success_callback,
+                      base::OnceClosure failure_callback) override {}
 #endif
 
   ExclusiveAccessContext* GetExclusiveAccessContext() override;