mash: Test ChromeLauncherControllerImpl more like production use.

Adjust the ownership of the instances used by most unit test fixtures.
The test Shell owns CLC as the ShelfDelegate, just like in production.
Fix up test helper functions and remove ProxyShelfDelegate.

Make BrowserWithTestWindowTest::TearDown more like production shutdown.
(first tear down the browser instance, then the Shell, then the profile)
Remove DestroyBrowserAndProfile, inline/recreate similar behavior.

Add a workaround for DefaultApps test teardown crash (crbug.com/709297).

BUG=557406
TEST=Automated tests still pass; more closely match production.
R=jamescook@chromium.org,sky@chromium.org

Review-Url: https://codereview.chromium.org/2804913002
Cr-Commit-Position: refs/heads/master@{#462880}

chrome/browser/ui/toolbar/app_menu_model_unittest.cc

@@ -78,7 +78,6 @@ class AppMenuModelTest : public BrowserWithTestWindowTest,
     BrowserWithTestWindowTest::TearDown();
     testing_io_thread_state_.reset();
     TestingBrowserProcess::GetGlobal()->SetLocalState(NULL);
-    DestroyBrowserAndProfile();
   }
 
  private:

chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_views_unittest.cc

@@ -196,13 +196,21 @@ TEST_F(AppInfoDialogViewsTest, UninstallingOtherAppDoesNotCloseDialog) {
 // Tests that the dialog closes when the current profile is destroyed.
 TEST_F(AppInfoDialogViewsTest, DestroyedProfileClosesDialog) {
   ShowAppInfo(kTestExtensionId);
+
   // First delete the test browser window. This ensures the test harness isn't
   // surprised by it being closed in response to the profile deletion below.
-  // Note the base class doesn't own the profile, so that part is skipped.
-  DestroyBrowserAndProfile();
+  std::unique_ptr<Browser> browser(release_browser());
+  browser->tab_strip_model()->CloseAllTabs();
+  browser.reset();
+  std::unique_ptr<BrowserWindow> browser_window(release_browser_window());
+  browser_window->Close();
+  browser_window.reset();
+
+  // This does not actually destroy the profile; see DestroyProfile above.
+  DestroyProfile(profile());
 
   // The following does nothing: it just ensures the Widget close is being
-  // triggered by the DeleteProfile() call rather than the line above.
+  // triggered by the DeleteProfile() call rather than the code above.
   base::RunLoop().RunUntilIdle();
 
   ASSERT_TRUE(widget_);

chrome/test/base/browser_with_test_window_test.cc

@@ -87,9 +87,11 @@ void BrowserWithTestWindowTest::TearDown() {
   // before the profile can be destroyed and the test safely shut down.
   base::RunLoop().RunUntilIdle();
 
-  // Reset the profile here because some profile keyed services (like the
-  // audio service) depend on test stubs that the helpers below will remove.
-  DestroyBrowserAndProfile();
+  // Close the browser tabs and destroy the browser and window instances.
+  if (browser_)
+    browser_->tab_strip_model()->CloseAllTabs();
+  browser_.reset();
+  window_.reset();
 
   if (content::IsBrowserSideNavigationEnabled())
     content::BrowserSideNavigationTearDown();
@@ -99,15 +101,23 @@ void BrowserWithTestWindowTest::TearDown() {
 #endif
 
 #if defined(OS_CHROMEOS)
+  // Destroy the shell before the profile to match production shutdown ordering.
   ash_test_helper_->TearDown();
 #elif defined(TOOLKIT_VIEWS)
   views_test_helper_.reset();
 #endif
 
+  // Destroy the profile here - otherwise, if the profile is freed in the
+  // destructor, and a test subclass owns a resource that the profile depends
+  // on (such as g_browser_process()->local_state()) there's no way for the
+  // subclass to free it after the profile.
+  if (profile_)
+    DestroyProfile(profile_);
+  profile_ = nullptr;
+
   testing::Test::TearDown();
 
-  // A Task is leaked if we don't destroy everything, then run the message
-  // loop.
+  // A Task is leaked if we don't destroy everything, then run the message loop.
   base::ThreadTaskRunnerHandle::Get()->PostTask(
       FROM_HERE, base::MessageLoop::QuitWhenIdleClosure());
   base::RunLoop().Run();
@@ -164,23 +174,6 @@ void BrowserWithTestWindowTest::NavigateAndCommitActiveTabWithTitle(
   contents->UpdateTitleForEntry(controller->GetActiveEntry(), title);
 }
 
-void BrowserWithTestWindowTest::DestroyBrowserAndProfile() {
-  if (browser_.get()) {
-    // Make sure we close all tabs, otherwise Browser isn't happy in its
-    // destructor.
-    browser()->tab_strip_model()->CloseAllTabs();
-    browser_.reset(NULL);
-  }
-  window_.reset(NULL);
-  // Destroy the profile here - otherwise, if the profile is freed in the
-  // destructor, and a test subclass owns a resource that the profile depends
-  // on (such as g_browser_process()->local_state()) there's no way for the
-  // subclass to free it after the profile.
-  if (profile_)
-    DestroyProfile(profile_);
-  profile_ = NULL;
-}
-
 TestingProfile* BrowserWithTestWindowTest::CreateProfile() {
   return new TestingProfile();
 }

chrome/test/base/browser_with_test_window_test.h

@@ -127,10 +127,6 @@ class BrowserWithTestWindowTest : public testing::Test {
                                            const GURL& url,
                                            const base::string16& title);
 
-  // Destroys the browser, window, and profile created by this class. This is
-  // invoked from the destructor.
-  void DestroyBrowserAndProfile();
-
   // Creates the profile used by this test. The caller owns the return value.
   virtual TestingProfile* CreateProfile();