Add an experiment for the default browser infobar on Windows 10

chrome/browser/ui/startup/default_browser_infobar_delegate_unittest.cc

+// Copyright 2016 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/ui/startup/default_browser_infobar_delegate.h"
+
+#include <memory>
+#include <string>
+
+#include "base/feature_list.h"
+#include "base/macros.h"
+#include "base/memory/ptr_util.h"
+#include "base/run_loop.h"
+#include "chrome/browser/ui/views/infobars/confirm_infobar.h"
+#include "components/infobars/core/confirm_infobar_delegate.h"
+#include "components/infobars/core/infobar.h"
+#include "components/infobars/core/infobar_manager.h"
+#include "content/public/test/test_browser_thread_bundle.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace chrome {
+
+class FakeDefaultBrowserWorker
+    : public shell_integration::DefaultBrowserWorker {
+ public:
+  FakeDefaultBrowserWorker(
+      const shell_integration::DefaultWebClientWorkerCallback& callback)
+      : shell_integration::DefaultBrowserWorker(callback) {}
+
+ private:
+  ~FakeDefaultBrowserWorker() override = default;

[Peter Kasting, 2016/06/01 00:21:12]

Nit: Is this declaration necessary, or the similar one just below?

[Patrick Monette, 2016/06/01 18:43:48]

Removed.

+
+  // Check if Chrome is the default browser.

[Peter Kasting, 2016/06/01 00:21:12]

Nit: These two comments are misleading because they describe what the functions
would do in the real executable, but what they explicitly don't do in this test.

I would remove these comments, and if you want instead add a comment above this
class like "An implementation of DefaultBrowserWorker that will simply invoke
the 'finished' callback immediately."

[Patrick Monette, 2016/06/01 18:43:48]

Done.

[Peter Kasting, 2016/06/01 19:32:10]

New comment added, but these comments not yet removed.

[Patrick Monette, 2016/06/01 22:07:39]

My bad.

+  shell_integration::DefaultWebClientState CheckIsDefaultImpl() override {
+    return shell_integration::NOT_DEFAULT;
+  }
+
+  // Set Chrome as the default browser.
+  void SetAsDefaultImpl(const base::Closure& on_finished_callback) override {
+    on_finished_callback.Run();
+  }
+
+  DISALLOW_COPY_AND_ASSIGN(FakeDefaultBrowserWorker);
+};
+
+class FakeDefaultBrowserInfoBarDelegate : public DefaultBrowserInfoBarDelegate {
+ public:
+  FakeDefaultBrowserInfoBarDelegate()
+      : DefaultBrowserInfoBarDelegate(nullptr) {}
+
+  ~FakeDefaultBrowserInfoBarDelegate() override = default;
+
+ private:
+  scoped_refptr<shell_integration::DefaultBrowserWorker>
+  CreateDefaultBrowserWorker(
+      const shell_integration::DefaultWebClientWorkerCallback& callback) {

[Peter Kasting, 2016/06/01 00:21:12]

Nit: override

[Patrick Monette, 2016/06/01 18:43:48]

Done.

+    return new FakeDefaultBrowserWorker(callback);
+  }
+
+  DISALLOW_COPY_AND_ASSIGN(FakeDefaultBrowserInfoBarDelegate);
+};
+
+class FakeInfoBarManager : public infobars::InfoBarManager {
+ public:
+  FakeInfoBarManager() = default;
+
+  std::unique_ptr<infobars::InfoBar> CreateConfirmInfoBar(

[Peter Kasting, 2016/06/01 00:21:12]

Instead of re-implementing this here, which is what leads to your DEPS issues,
why not just subclass InfoBarService instead of InfoBarManager?  InfoBarService
is meant to be the chrome-side functional implementation of the abstract base
InfoBarManager, and since you need the functionality of the real implementation
(in this case, to add an instance of a concrete infobar implementation), it
seems like that's the class you should be using.

Then you should get CreateConfirmInfoBar for free.  This will also give you an
implementation of this method on non-views platforms (e.g. Mac) as well, which
seems important since this test file is not views-specific.

[Patrick Monette, 2016/06/01 18:43:48]

Okay this works! I initially didn't want to do that because it require a Profile
and a WebContents but it was simpler than I thought

+      std::unique_ptr<ConfirmInfoBarDelegate> delegate) override {
+    return base::WrapUnique(new ConfirmInfoBar(std::move(delegate)));
+  }
+
+ private:
+  // infobars::InfoBarManager:
+  int GetActiveEntryID() override { return 0; }
+  void OpenURL(const GURL& url, WindowOpenDisposition disposition) override {}
+
+  DISALLOW_COPY_AND_ASSIGN(FakeInfoBarManager);
+};
+
+class DefaultBrowserInfoBarDelegateTest : public ::testing::Test {
+ public:
+  DefaultBrowserInfoBarDelegateTest() = default;
+
+ protected:
+  void EnableStickDefaultBrowserPrompt() {

[Peter Kasting, 2016/06/01 00:21:12]

Nit: Sticky

[Patrick Monette, 2016/06/01 18:43:48]

Done.

+    base::FeatureList::ClearInstanceForTesting();
+    std::unique_ptr<base::FeatureList> feature_list(new base::FeatureList);
+    feature_list->InitializeFromCommandLine(kStickyDefaultBrowserPrompt.name,
+                                            std::string());
+    base::FeatureList::SetInstance(std::move(feature_list));
+  }
+
+  void AddDefaultBrowserInfoBar() {
+    infobar_manager_.AddInfoBar(infobar_manager_.CreateConfirmInfoBar(
+        base::WrapUnique(new FakeDefaultBrowserInfoBarDelegate())));
+  }
+
+  infobars::InfoBarManager& infobar_manager() { return infobar_manager_; }

[Peter Kasting, 2016/06/01 00:21:12]

Nit: We normally avoid APIs that return non-const refs; returning this by
pointer seems fine

[Patrick Monette, 2016/06/01 18:43:48]

Done.

+
+ private:
+  FakeInfoBarManager infobar_manager_;
+
+  // For UI and File thread.

[Peter Kasting, 2016/06/01 00:21:12]

Nit: If this test needs these, expand briefly on what uses them, since nothing
refers to them directly.

[Patrick Monette, 2016/06/01 18:43:48]

Done.

+  content::TestBrowserThreadBundle thread_bundle;
+
+  DISALLOW_COPY_AND_ASSIGN(DefaultBrowserInfoBarDelegateTest);
+};
+
+TEST_F(DefaultBrowserInfoBarDelegateTest, DefaultBehavior) {
+  AddDefaultBrowserInfoBar();
+  ASSERT_EQ(1U, infobar_manager().infobar_count());
+
+  infobars::InfoBar* default_browser_infobar = infobar_manager().infobar_at(0);
+  ConfirmInfoBarDelegate* default_browser_infobar_delegate =
+      default_browser_infobar->delegate()->AsConfirmInfoBarDelegate();
+
+  // Clicking the button should returns true, which means the infobar should
+  // immediatly be dismissed. Unfortunately, this does not simulate a real click
+  // on the button so the infobar still exist after calling Accept().

[Peter Kasting, 2016/06/01 00:21:12]

This comment is a bit misleading.  Accept() knows nothing about buttons and
clicks.  Its return value simply tells the caller whether the caller should
destroy the infobar after asking the infobar to execute its default action
(however that execution is triggered).

Perhaps this comment instead:

When the sticky default browser prompt experiment is not enabled, the infobar
delegate should allow itself to be destroyed immediately if the user activates
the default action.  Accept() should return true to indicate this.

[Patrick Monette, 2016/06/01 18:43:48]

Done. I removed the tidbit about the experiment since I felt it was out of place
for the DefaultBehavior test.

[Peter Kasting, 2016/06/01 19:32:10]

The main reason I liked that was that, until I realized the whole point here was
to contrast with what happens in that experiment, I was writing a comment to
tell you to just remove this test because you're not really testing anything
meaningful.  When I realized that this test ensures the experimental behavior is
not only "don't dismiss" but that it's explicitly different than the default I
figured it was worth keeping with a note about why.

It's not all that unclear, or a big deal, so up to you.

[Patrick Monette, 2016/06/01 22:07:40]

Alright I'll add it then.

+  EXPECT_TRUE(default_browser_infobar_delegate->Accept());
+}
+
+#if defined(OS_WIN)
+TEST_F(DefaultBrowserInfoBarDelegateTest, StickyDefaultBrowserPrompt) {
+  EnableStickDefaultBrowserPrompt();
+
+  // For most Windows versions, this experiment is disabled.
+  if (!IsStickyDefaultBrowserPromptEnabled())
+    return;
+
+  AddDefaultBrowserInfoBar();
+  ASSERT_EQ(1U, infobar_manager().infobar_count());
+
+  infobars::InfoBar* default_browser_infobar = infobar_manager().infobar_at(0);
+  ConfirmInfoBarDelegate* default_browser_infobar_delegate =
+      default_browser_infobar->delegate()->AsConfirmInfoBarDelegate();
+
+  // Clicking the button should returns false, which means the infobar should
+  // not be dismissed immediatly. It will be dismissed after the
+  // DefaultBrowserWorker is done (i.e. after RunUntilIdle()).

[Peter Kasting, 2016/06/01 00:21:12]

Nit: For parallel structure with my above suggestion:

The sticky default browser prompt experiment should mean activating the
infobar's default action does not result in the infobar's destruction.  Instead,
the infobar will ultimately be destroyed when the DefaultBrowserWorker is done. 
To indicate this Accept() should return false.

Then above the RunUntilIdle() call below:

Spin the message lop to allow the FakeDefaultBrowserWorker to complete, which
should destroy the infobar.

[Patrick Monette, 2016/06/01 18:43:48]

Thanks for the suggestion!

+  EXPECT_FALSE(default_browser_infobar_delegate->Accept());
+  ASSERT_EQ(1U, infobar_manager().infobar_count());

[Peter Kasting, 2016/06/01 00:21:12]

Nit: EXPECT

[Patrick Monette, 2016/06/01 18:43:49]

Done.

+
+  base::RunLoop().RunUntilIdle();
+  ASSERT_EQ(0U, infobar_manager().infobar_count());
+}
+#endif  // defined(OS_WIN)
+
+}  // namespace chrome