Revert 212329 "Reland "Close web contents modal dialogs on conte..."

Causes several problems related to the print preview dialog:
  http://crbug.com/262023
  http://crbug.com/262916
  http://crbug.com/265427

> Reland "Close web contents modal dialogs on content load start"
> 
> Address the failure of some web contents modal dialogs to close when
> interstitial WebUI is displayed by closing the dialogs on content load start.
> Remove existing ad hoc support in dialogs for closing on page loading in favor
> of a uniform approach.
> 
> I'm using load start as the trigger for dialog close as this seems to be the
> point in time at which the user first perceives the page to be changing.
> 
> Notes on the print preview changes: with this change the dialog is closed when
> load starts and the initiator tab gets removed via RemovePreviewDialog, so there
> is no longer a need to handle NOTIFICATION_NAV_ENTRY_COMITTED for the initiator
> tab.  The PrintViewManager::PrintPreviewDone() DCHECK is removed since now
> PrintViewManager::RenderViewGone() may be invoked and print_preview_state_ reset
> due to WebContents closure before the dialog is destroyed and PrintPreviewDone()
> is invoked.
> 
> BUG=240575
> 
> Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=211058
> 
> Review URL: https://chromiumcodereview.appspot.com/17500003

TBR=wittman@chromium.org

Review URL: https://codereview.chromium.org/21372006

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@214775 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/printing/background_printing_manager.cc

@@ -63,15 +63,16 @@ void BackgroundPrintingManager::OwnPrintPreviewDialog(
                    rph_source);
   }
 
-  // Activate the initiator.
+  // Activate the initiator tab.
   PrintPreviewDialogController* dialog_controller =
       PrintPreviewDialogController::GetInstance();
   if (!dialog_controller)
     return;
-  WebContents* initiator = dialog_controller->GetInitiator(preview_dialog);
-  if (!initiator)
+  WebContents* initiator_tab =
+      dialog_controller->GetInitiatorTab(preview_dialog);
+  if (!initiator_tab)
     return;
-  initiator->GetDelegate()->ActivateContents(initiator);
+  initiator_tab->GetDelegate()->ActivateContents(initiator_tab);
 }
 
 void BackgroundPrintingManager::Observe(

chrome/browser/printing/print_preview_dialog_controller.h

@@ -1,4 +1,4 @@
- // Copyright (c) 2012 The Chromium Authors. All rights reserved.
+// Copyright (c) 2012 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.
 
@@ -22,9 +22,9 @@ class WebContents;
 
 namespace printing {
 
-// For print preview, the WebContents that initiates the printing operation is
-// the initiator, and the constrained dialog that shows the print preview is the
-// print preview dialog.
+// For print preview, the tab that initiates the printing operation is the
+// initiator tab, and the constrained dialog that shows the print preview is
+// the print preview dialog.
 // This class manages print preview dialog creation and destruction, and keeps
 // track of the 1:1 relationship between initiatora tabs and print preview
 // dialogs.
@@ -36,24 +36,24 @@ class PrintPreviewDialogController
 
   static PrintPreviewDialogController* GetInstance();
 
-  // Initiate print preview for |initiator|.
+  // Initiate print preview for |initiator_tab|.
   // Call this instead of GetOrCreatePreviewDialog().
-  static void PrintPreview(content::WebContents* initiator);
+  static void PrintPreview(content::WebContents* initiator_tab);
 
-  // Get/Create the print preview dialog for |initiator|.
+  // Get/Create the print preview dialog for |initiator_tab|.
   // Exposed for unit tests.
   content::WebContents* GetOrCreatePreviewDialog(
-      content::WebContents* initiator);
+      content::WebContents* initiator_tab);
 
   // Returns the preview dialog for |contents|.
   // Returns |contents| if |contents| is a preview dialog.
   // Returns NULL if no preview dialog exists for |contents|.
   content::WebContents* GetPrintPreviewForContents(
       content::WebContents* contents) const;
 
-  // Returns the initiator for |preview_dialog|.
-  // Returns NULL if no initiator exists for |preview_dialog|.
-  content::WebContents* GetInitiator(content::WebContents* preview_dialog);
+  // Returns the initiator tab for |preview_dialog|.
+  // Returns NULL if no initiator tab exists for |preview_dialog|.
+  content::WebContents* GetInitiatorTab(content::WebContents* preview_dialog);
 
   // content::NotificationObserver implementation.
   virtual void Observe(int type,
@@ -66,8 +66,8 @@ class PrintPreviewDialogController
   // Returns true if |url| is a print preview url.
   static bool IsPrintPreviewURL(const GURL& url);
 
-  // Erase the initiator info associated with |preview_dialog|.
-  void EraseInitiatorInfo(content::WebContents* preview_dialog);
+  // Erase the initiator tab info associated with |preview_tab|.
+  void EraseInitiatorTabInfo(content::WebContents* preview_tab);
 
   bool is_creating_print_preview_dialog() const {
     return is_creating_print_preview_dialog_;
@@ -76,16 +76,9 @@ class PrintPreviewDialogController
  private:
   friend class base::RefCounted<PrintPreviewDialogController>;
 
-  // Used to distinguish between the two varieties of WebContents dealt with by
-  // this class.
-  enum ContentsType {
-    INITIATOR,
-    PREVIEW_DIALOG
-  };
-
-  // 1:1 relationship between a print preview dialog and its initiator.
+  // 1:1 relationship between a print preview dialog and its initiator tab.
   // Key: Print preview dialog.
-  // Value: Initiator.
+  // Value: Initiator tab.
   typedef std::map<content::WebContents*, content::WebContents*>
       PrintPreviewDialogMap;
 
@@ -106,22 +99,21 @@ class PrintPreviewDialogController
 
   // Creates a new print preview dialog.
   content::WebContents* CreatePrintPreviewDialog(
-      content::WebContents* initiator);
+      content::WebContents* initiator_tab);
 
-  // Helper function to store the title of the initiator associated with
+  // Helper function to store the title of the initiator tab associated with
   // |preview_dialog| in |preview_dialog|'s PrintPreviewUI.
-  void SaveInitiatorTitle(content::WebContents* preview_dialog);
+  void SaveInitiatorTabTitle(content::WebContents* preview_dialog);
 
   // Adds/Removes observers for notifications from |contents|.
-  void AddObservers(content::WebContents* contents, ContentsType contents_type);
-  void RemoveObservers(content::WebContents* contents,
-                       ContentsType contents_type);
+  void AddObservers(content::WebContents* contents);
+  void RemoveObservers(content::WebContents* contents);
 
   // Removes WebContents when they close/crash/navigate.
-  void RemoveInitiator(content::WebContents* initiator);
+  void RemoveInitiatorTab(content::WebContents* initiator_tab);
   void RemovePreviewDialog(content::WebContents* preview_dialog);
 
-  // Mapping between print preview dialog and the corresponding initiator.
+  // Mapping between print preview dialog and the corresponding initiator tab.
   PrintPreviewDialogMap preview_dialog_map_;
 
   // A registrar for listening notifications.

chrome/browser/printing/print_preview_dialog_controller_browsertest.cc

@@ -103,11 +103,11 @@ class PrintPreviewDialogDestroyedObserver : public WebContentsObserver {
 
 class PrintPreviewDialogControllerBrowserTest : public InProcessBrowserTest {
  public:
-  PrintPreviewDialogControllerBrowserTest() : initiator_(NULL) {}
+  PrintPreviewDialogControllerBrowserTest() : initiator_tab_(NULL) {}
   virtual ~PrintPreviewDialogControllerBrowserTest() {}
 
-  WebContents* initiator() {
-    return initiator_;
+  WebContents* initiator_tab() {
+    return initiator_tab_;
   }
 
   void PrintPreview() {
@@ -120,7 +120,7 @@ class PrintPreviewDialogControllerBrowserTest : public InProcessBrowserTest {
   WebContents* GetPrintPreviewDialog() {
     printing::PrintPreviewDialogController* dialog_controller =
         printing::PrintPreviewDialogController::GetInstance();
-    return dialog_controller->GetPrintPreviewForContents(initiator_);
+    return dialog_controller->GetPrintPreviewForContents(initiator_tab_);
   }
 
  private:
@@ -143,28 +143,28 @@ class PrintPreviewDialogControllerBrowserTest : public InProcessBrowserTest {
     cloned_tab_observer_.reset(new PrintPreviewDialogClonedObserver(first_tab));
     chrome::DuplicateTab(browser());
 
-    initiator_ = browser()->tab_strip_model()->GetActiveWebContents();
-    ASSERT_TRUE(initiator_);
-    ASSERT_NE(first_tab, initiator_);
+    initiator_tab_ = browser()->tab_strip_model()->GetActiveWebContents();
+    ASSERT_TRUE(initiator_tab_);
+    ASSERT_NE(first_tab, initiator_tab_);
   }
 
   virtual void CleanUpOnMainThread() OVERRIDE {
     cloned_tab_observer_.reset();
-    initiator_ = NULL;
+    initiator_tab_ = NULL;
   }
 
   RequestPrintPreviewObserver* request_preview_tab_observer() {
     return cloned_tab_observer_->request_preview_tab_observer();
   }
 
   scoped_ptr<PrintPreviewDialogClonedObserver> cloned_tab_observer_;
-  WebContents* initiator_;
+  WebContents* initiator_tab_;
 
   DISALLOW_COPY_AND_ASSIGN(PrintPreviewDialogControllerBrowserTest);
 };
 
-// Test to verify that when a initiator navigates, we can create a new preview
-// dialog for the new tab contents.
+// Test to verify that when a initiator tab navigates, we can create a new
+// preview dialog for the new tab contents.
 IN_PROC_BROWSER_TEST_F(PrintPreviewDialogControllerBrowserTest,
                        NavigateFromInitiatorTab) {
   // print for the first time.
@@ -175,7 +175,7 @@ IN_PROC_BROWSER_TEST_F(PrintPreviewDialogControllerBrowserTest,
 
   // Check a new print preview dialog got created.
   ASSERT_TRUE(preview_dialog);
-  ASSERT_NE(initiator(), preview_dialog);
+  ASSERT_NE(initiator_tab(), preview_dialog);
 
   // Navigate in the initiator tab. Make sure navigating destroys the print
   // preview dialog.
@@ -193,8 +193,8 @@ IN_PROC_BROWSER_TEST_F(PrintPreviewDialogControllerBrowserTest,
   EXPECT_TRUE(new_preview_dialog);
 }
 
-// Test to verify that after reloading the initiator, it creates a new print
-// preview dialog.
+// Test to verify that after reloading the initiator tab, it creates a new
+// print preview dialog.
 IN_PROC_BROWSER_TEST_F(PrintPreviewDialogControllerBrowserTest,
                        ReloadInitiatorTab) {
   // print for the first time.
@@ -204,9 +204,9 @@ IN_PROC_BROWSER_TEST_F(PrintPreviewDialogControllerBrowserTest,
 
   // Check a new print preview dialog got created.
   ASSERT_TRUE(preview_dialog);
-  ASSERT_NE(initiator(), preview_dialog);
+  ASSERT_NE(initiator_tab(), preview_dialog);
 
-  // Reload the initiator. Make sure reloading destroys the print preview
+  // Reload the initiator tab. Make sure reloading destroys the print preview
   // dialog.
   PrintPreviewDialogDestroyedObserver dialog_destroyed_observer(preview_dialog);
   content::WindowedNotificationObserver notification_observer(

chrome/browser/printing/print_preview_dialog_controller_unittest.cc

@@ -16,61 +16,62 @@
 
 using content::WebContents;
 
-// Test crashes on Aura due to initiator's native view having no parent.
+// Test crashes on Aura due to initiator tab's native view having no parent.
 // http://crbug.com/104284
 #if defined(USE_AURA)
 #define MAYBE_GetOrCreatePreviewDialog DISABLED_GetOrCreatePreviewDialog
 #define MAYBE_MultiplePreviewDialogs DISABLED_MultiplePreviewDialogs
-#define MAYBE_ClearInitiatorDetails DISABLED_ClearInitiatorDetails
+#define MAYBE_ClearInitiatorTabDetails DISABLED_ClearInitiatorTabDetails
 #else
 #define MAYBE_GetOrCreatePreviewDialog GetOrCreatePreviewDialog
 #define MAYBE_MultiplePreviewDialogs MultiplePreviewDialogs
-#define MAYBE_ClearInitiatorDetails ClearInitiatorDetails
+#define MAYBE_ClearInitiatorTabDetails ClearInitiatorTabDetails
 #endif
 
 namespace printing {
 
 typedef PrintPreviewTest PrintPreviewDialogControllerUnitTest;
 
-// Create/Get a preview dialog for initiator.
+// Create/Get a preview dialog for initiator tab.
 TEST_F(PrintPreviewDialogControllerUnitTest, MAYBE_GetOrCreatePreviewDialog) {
   // Lets start with one window with one tab.
   EXPECT_EQ(1u, chrome::GetTotalBrowserCount());
   EXPECT_EQ(0, browser()->tab_strip_model()->count());
   chrome::NewTab(browser());
   EXPECT_EQ(1, browser()->tab_strip_model()->count());
 
-  // Create a reference to initiator contents.
-  WebContents* initiator = browser()->tab_strip_model()->GetActiveWebContents();
+  // Create a reference to initiator tab contents.
+  WebContents* initiator_tab =
+      browser()->tab_strip_model()->GetActiveWebContents();
 
   PrintPreviewDialogController* dialog_controller =
       PrintPreviewDialogController::GetInstance();
   ASSERT_TRUE(dialog_controller);
 
-  // Get the preview dialog for initiator.
-  PrintViewManager::FromWebContents(initiator)->PrintPreviewNow(false);
+  // Get the preview dialog for initiator tab.
+  PrintViewManager::FromWebContents(initiator_tab)->PrintPreviewNow(false);
   WebContents* preview_dialog =
-      dialog_controller->GetOrCreatePreviewDialog(initiator);
+      dialog_controller->GetOrCreatePreviewDialog(initiator_tab);
 
   // New print preview dialog is a constrained window, so the number of tabs is
   // still 1.
   EXPECT_EQ(1, browser()->tab_strip_model()->count());
-  EXPECT_NE(initiator, preview_dialog);
+  EXPECT_NE(initiator_tab, preview_dialog);
 
-  // Get the print preview dialog for the same initiator.
+  // Get the print preview dialog for the same initiator tab.
   WebContents* new_preview_dialog =
-      dialog_controller->GetOrCreatePreviewDialog(initiator);
+      dialog_controller->GetOrCreatePreviewDialog(initiator_tab);
 
   // Preview dialog already exists. Tab count remains the same.
   EXPECT_EQ(1, browser()->tab_strip_model()->count());
 
-  // 1:1 relationship between initiator and preview dialog.
+  // 1:1 relationship between initiator tab and preview dialog.
   EXPECT_EQ(new_preview_dialog, preview_dialog);
 }
 
-// Tests multiple print preview dialogs exist in the same browser for different
-// initiators. If a preview dialog already exists for an initiator, that
-// initiator gets focused.
+// Tests multiple print preview dialogs exist in the same browser for
+// different initiator tabs. If a preview dialog already exists for an
+// initiator tab, that initiator tab gets focused.
 TEST_F(PrintPreviewDialogControllerUnitTest, MAYBE_MultiplePreviewDialogs) {
   // Lets start with one window and two tabs.
   EXPECT_EQ(1u, chrome::GetTotalBrowserCount());
@@ -79,7 +80,7 @@ TEST_F(PrintPreviewDialogControllerUnitTest, MAYBE_MultiplePreviewDialogs) {
 
   EXPECT_EQ(0, tab_strip_model->count());
 
-  // Create some new initiators.
+  // Create some new initiator tabs.
   chrome::NewTab(browser());
   WebContents* web_contents_1 = tab_strip_model->GetActiveWebContents();
   ASSERT_TRUE(web_contents_1);
@@ -108,8 +109,8 @@ TEST_F(PrintPreviewDialogControllerUnitTest, MAYBE_MultiplePreviewDialogs) {
 
   EXPECT_NE(web_contents_2, preview_dialog_2);
   EXPECT_NE(preview_dialog_1, preview_dialog_2);
-  // 2 initiators and 2 preview dialogs exist in the same browser.  The preview
-  // dialogs are constrained in their respective initiators.
+  // 2 initiator tabs and 2 preview dialogs exist in the same browser.
+  // The preview dialogs are constrained in their respective initiator tabs.
   EXPECT_EQ(2, tab_strip_model->count());
 
   int tab_1_index = tab_strip_model->GetIndexOfWebContents(web_contents_1);
@@ -124,7 +125,7 @@ TEST_F(PrintPreviewDialogControllerUnitTest, MAYBE_MultiplePreviewDialogs) {
   EXPECT_EQ(-1, preview_dialog_2_index);
 
   // Since |preview_dialog_2_index| was the most recently created dialog, its
-  // initiator should have focus.
+  // initiator tab should have focus.
   EXPECT_EQ(tab_2_index, tab_strip_model->active_index());
 
   // When we get the preview dialog for |web_contents_1|,
@@ -133,38 +134,39 @@ TEST_F(PrintPreviewDialogControllerUnitTest, MAYBE_MultiplePreviewDialogs) {
   EXPECT_EQ(tab_1_index, tab_strip_model->active_index());
 }
 
-// Check clearing the initiator details associated with a print preview dialog
-// allows the initiator to create another print preview dialog.
-TEST_F(PrintPreviewDialogControllerUnitTest, MAYBE_ClearInitiatorDetails) {
+// Check clearing the initiator tab details associated with a print preview
+// dialog allows the initiator tab to create another print preview dialog.
+TEST_F(PrintPreviewDialogControllerUnitTest, MAYBE_ClearInitiatorTabDetails) {
   // Lets start with one window with one tab.
   EXPECT_EQ(1u, chrome::GetTotalBrowserCount());
   EXPECT_EQ(0, browser()->tab_strip_model()->count());
   chrome::NewTab(browser());
   EXPECT_EQ(1, browser()->tab_strip_model()->count());
 
-  // Create a reference to initiator contents.
-  WebContents* initiator = browser()->tab_strip_model()->GetActiveWebContents();
+  // Create a reference to initiator tab contents.
+  WebContents* initiator_tab =
+      browser()->tab_strip_model()->GetActiveWebContents();
 
   PrintPreviewDialogController* dialog_controller =
       PrintPreviewDialogController::GetInstance();
   ASSERT_TRUE(dialog_controller);
 
-  // Get the preview dialog for the initiator.
-  PrintViewManager::FromWebContents(initiator)->PrintPreviewNow(false);
+  // Get the preview dialog for the initiator tab.
+  PrintViewManager::FromWebContents(initiator_tab)->PrintPreviewNow(false);
   WebContents* preview_dialog =
-      dialog_controller->GetOrCreatePreviewDialog(initiator);
+      dialog_controller->GetOrCreatePreviewDialog(initiator_tab);
 
   // New print preview dialog is a constrained window, so the number of tabs is
   // still 1.
   EXPECT_EQ(1, browser()->tab_strip_model()->count());
-  EXPECT_NE(initiator, preview_dialog);
+  EXPECT_NE(initiator_tab, preview_dialog);
 
-  // Clear the initiator details associated with the preview dialog.
-  dialog_controller->EraseInitiatorInfo(preview_dialog);
+  // Clear the initiator tab details associated with the preview dialog.
+  dialog_controller->EraseInitiatorTabInfo(preview_dialog);
 
-  // Get a new print preview dialog for the initiator.
+  // Get a new print preview dialog for the initiator tab.
   WebContents* new_preview_dialog =
-      dialog_controller->GetOrCreatePreviewDialog(initiator);
+      dialog_controller->GetOrCreatePreviewDialog(initiator_tab);
 
   // New print preview dialog is a constrained window, so the number of tabs is
   // still 1.

chrome/browser/printing/print_view_manager.h

@@ -46,7 +46,7 @@ class PrintViewManager : public content::NotificationObserver,
   bool PrintForSystemDialogNow();
 
   // Same as PrintNow(), but for the case where a user press "ctrl+shift+p" to
-  // show the native system dialog. This can happen from both initiator and
+  // show the native system dialog. This can happen from both initiator tab and
   // preview dialog.
   bool AdvancedPrintNow();