Add a triggered profile reset mechanism.

chrome/browser/ui/startup/startup_browser_creator_triggered_reset_browsertest_win.cc

+// Copyright (c) 2015 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 "base/callback_list.h"
+#include "base/command_line.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/win/windows_version.h"
+#include "chrome/browser/browser_process.h"
+#include "chrome/browser/profile_resetter/triggered_profile_resetter.h"
+#include "chrome/browser/profile_resetter/triggered_profile_resetter_factory.h"
+#include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/browser_finder.h"
+#include "chrome/browser/ui/browser_iterator.h"
+#include "chrome/browser/ui/startup/startup_browser_creator.h"
+#include "chrome/browser/ui/startup/startup_browser_creator_impl.h"
+#include "chrome/browser/ui/tabs/tab_strip_model.h"
+#include "chrome/common/url_constants.h"
+#include "chrome/test/base/in_process_browser_test.h"
+#include "components/keyed_service/content/browser_context_dependency_manager.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace {
+
+// Check that there are two browsers. Find the one that is not |browser|.
+Browser* FindOneOtherBrowser(Browser* browser) {
+  // There should only be one other browser.
+  EXPECT_EQ(2u, chrome::GetBrowserCount(browser->profile(),
+                                        browser->host_desktop_type()));
+
+  // Find the new browser.
+  Browser* other_browser = NULL;

[msw, 2015/09/24 17:15:32]

nit: nullptr (okay as-is if you'd rather straight dup this...)

[robertshield, 2015/09/25 00:39:19]

Done.

+  for (chrome::BrowserIterator it; !it.done() && !other_browser; it.Next()) {

[msw, 2015/09/24 17:15:32]

nit: no curlies needed, afaik (ditto ignore if duping)

[robertshield, 2015/09/25 00:39:19]

I could drop them if I make the if statement single-line, but of the two
options:

for (chrome::BrowserIterator it; !it.done(); it.Next())
    if (*it != browser) return *it;

for (chrome::BrowserIterator it; !it.done(); it.Next()) {
  if (*it != browser)
    return *it;
}

I find the second one infinitesimally more readable and both are explicitly
allowed by the style guide. 

[msw, 2015/09/25 01:55:42]

Second is fine, I thought that simply without curlies would be ok, but maybe
not. git cl format has thrown me for a loop :)

+    if (*it != browser)
+      other_browser = *it;

[msw, 2015/09/24 17:15:31]

nit: just return *it here and remove |other_browser| (ditto ignore if duping).

[robertshield, 2015/09/25 00:39:19]

Done.

+  }
+  return other_browser;
+}
+
+class MockTriggeredProfileResetter : public TriggeredProfileResetter {
+ public:
+  MockTriggeredProfileResetter() : TriggeredProfileResetter(nullptr) {}
+
+  void Activate() override {}
+  bool HasResetTrigger() override { return true; }
+};

[msw, 2015/09/24 17:15:32]

nit: DISALLOW_COPY_AND_ASSIGN?

[robertshield, 2015/09/25 00:39:19]

Done.

+
+scoped_ptr<KeyedService> BuildMockTriggeredProfileResetter(
+    content::BrowserContext* context) {
+  return make_scoped_ptr(new MockTriggeredProfileResetter);
+}
+
+}  // namespace
+
+class StartupBrowserCreatorTriggeredResetTest : public InProcessBrowserTest {
+ protected:
+  void SetUpCommandLine(base::CommandLine* command_line) override {
+    command_line->AppendArg("http://www.chromium.org");

[msw, 2015/09/24 17:15:31]

q: Where does this come into play? Is the SetUpCommandLine override needed?

[robertshield, 2015/09/25 00:39:19]

Hrm, actually no, all the tab setup happens in the test. Removed.

+  };

[msw, 2015/09/24 17:15:32]

nit: no semicolon to close function

[robertshield, 2015/09/25 00:39:19]

ah, copy/paste, you fail me again!

+
+  void SetUpInProcessBrowserTestFixture() override {
+    will_create_browser_context_services_subscription_ =
+        BrowserContextDependencyManager::GetInstance()
+            ->RegisterWillCreateBrowserContextServicesCallbackForTesting(
+                base::Bind(&StartupBrowserCreatorTriggeredResetTest::
+                               OnWillCreateBrowserContextServices,
+                           base::Unretained(this)))
+            .Pass();
+  };

[msw, 2015/09/24 17:15:31]

ditto nit: no semicolon to close function

[robertshield, 2015/09/25 00:39:19]

Done.

+
+ private:
+  void OnWillCreateBrowserContextServices(content::BrowserContext* context) {
+    TriggeredProfileResetterFactory::GetInstance()->SetTestingFactory(
+        context, &BuildMockTriggeredProfileResetter);
+  }
+
+  scoped_ptr<base::CallbackList<void(content::BrowserContext*)>::Subscription>
+      will_create_browser_context_services_subscription_;
+};

[msw, 2015/09/24 17:15:32]

nit: DISALLOW_COPY_AND_ASSIGN?

[robertshield, 2015/09/25 00:39:19]

Done.

+
+IN_PROC_BROWSER_TEST_F(StartupBrowserCreatorTriggeredResetTest,
+                       TestTriggeredReset) {
+  // Use a couple same-site HTTP URLs.
+  ASSERT_TRUE(test_server()->Start());
+  std::vector<GURL> urls;
+  urls.push_back(test_server()->GetURL("files/title1.html"));
+  urls.push_back(test_server()->GetURL("files/title2.html"));
+
+  Profile* profile = browser()->profile();
+  chrome::HostDesktopType host_desktop_type = browser()->host_desktop_type();
+
+  // Set the startup preference to open these URLs.
+  SessionStartupPref pref(SessionStartupPref::URLS);
+  pref.urls = urls;
+  SessionStartupPref::SetStartupPref(profile, pref);
+
+  // Keep the browser process running while browsers are closed.
+  g_browser_process->AddRefModule();
+
+  // Close the browser.
+  CloseBrowserAsynchronously(browser());
+
+  // Do a simple non-process-startup browser launch.
+  base::CommandLine dummy(base::CommandLine::NO_PROGRAM);
+  StartupBrowserCreatorImpl launch(base::FilePath(), dummy,
+                                   chrome::startup::IS_NOT_FIRST_RUN);
+  ASSERT_TRUE(
+      launch.Launch(profile, std::vector<GURL>(), false, host_desktop_type));
+
+  // This should have created a new browser window.  |browser()| is still
+  // around at this point, even though we've closed its window.
+  Browser* new_browser = FindOneOtherBrowser(browser());
+  ASSERT_TRUE(new_browser);
+
+  std::vector<GURL> expected_urls(urls);
+  if (base::win::GetVersion() >= base::win::VERSION_WIN10)
+    expected_urls.insert(expected_urls.begin(), internals::GetWelcomePageURL());

[msw, 2015/09/24 17:15:31]

q: *should* we get the welcome page on non-first-run with a reset?

[robertshield, 2015/09/25 00:39:19]

Yes, the welcome page isn't displayed only at first run, as per
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...
it looks like the welcome page is intended to show up once for everybody, so for
tests it will appear unless it is squelched (which would take more code than
EXPECTing it). 

This test follows the patterns of the other tests in
startup_browser_creator_browsertest.cc.

[msw, 2015/09/25 01:55:42]

Acknowledged.

+  expected_urls.insert(expected_urls.begin(), internals::GetResetSettingsURL());
+
+  TabStripModel* tab_strip = new_browser->tab_strip_model();
+  ASSERT_EQ(static_cast<int>(expected_urls.size()), tab_strip->count());
+  for (size_t i = 0; i < expected_urls.size(); i++)
+    EXPECT_EQ(expected_urls[i], tab_strip->GetWebContentsAt(i)->GetURL());
+
+  g_browser_process->ReleaseModule();
+}
+
+IN_PROC_BROWSER_TEST_F(StartupBrowserCreatorTriggeredResetTest,
+                       TestTriggeredResetDoesNotShowWithFirstRunURLs) {
+  // The presence of First Run tabs (in production code, these commonly come
+  // from master_preferences) should suppress the reset UI. Check that this is
+  // the case.
+  StartupBrowserCreator browser_creator;
+  browser_creator.AddFirstRunTab(GURL("http://new_tab_page"));
+  browser_creator.AddFirstRunTab(test_server()->GetURL("files/title1.html"));
+
+  // Do a process-startup browser launch.
+  base::CommandLine dummy(base::CommandLine::NO_PROGRAM);
+  StartupBrowserCreatorImpl launch(base::FilePath(), dummy, &browser_creator,
+                                   chrome::startup::IS_NOT_FIRST_RUN);

[msw, 2015/09/24 17:15:31]

nit: it's odd that AddFirstRunTab is respected for IS_NOT_FIRST_RUN.

[robertshield, 2015/09/25 00:39:19]

Hrm, yes, I actually meant to make this IS_FIRST_RUN. Afaict, this is caused by
AddStartupURLs not checking is_first_run_, but only looking at first_run_tabs_.
While odd, it looks like this is Working As Implemented and I'm hesitant to
change the StartupBrowserCreator logic around this. I've updated the parameter
here to IS_FIRST_RUN to make the test less surprising. 

[msw, 2015/09/25 01:55:42]

Acknowledged.

+  ASSERT_TRUE(launch.Launch(browser()->profile(), std::vector<GURL>(), true,
+                            browser()->host_desktop_type()));
+
+  // This should have created a new browser window.
+  Browser* new_browser = FindOneOtherBrowser(browser());
+  ASSERT_TRUE(new_browser);
+
+  // Verify that only the first-run tabs are shown.

[msw, 2015/09/24 17:15:32]

q: *shouldn't* we show the profile reset even with first run URLs?

[robertshield, 2015/09/25 00:39:19]

I had intentionally made it so that profile reset does not show up with first
run URLs to follow the intent of the comment here:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

If a specific first run flow is described by master_preferences, I don't think
the reset should trump that.

[msw, 2015/09/25 01:55:42]

Fair enough, maybe run that by a PM interested in the profile reset feature, but
it's okay as-is for now.

+  TabStripModel* tab_strip = new_browser->tab_strip_model();
+  ASSERT_EQ(2, tab_strip->count());
+  EXPECT_EQ(GURL(chrome::kChromeUINewTabURL),
+            tab_strip->GetWebContentsAt(0)->GetURL());
+  EXPECT_EQ("title1.html",
+            tab_strip->GetWebContentsAt(1)->GetURL().ExtractFileName());
+}