Make JavaScript dialogs auto-dismiss on tab switch.

chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.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/javascript_dialogs/javascript_dialog_tab_helper.h"
 
+#include "base/bind.h"
 #include "base/feature_list.h"
 #include "base/metrics/histogram_macros.h"
 #include "chrome/browser/engagement/site_engagement_service.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/browser_list.h"
+#include "chrome/browser/ui/javascript_dialogs/javascript_dialog_views.h"
+#include "chrome/browser/ui/tab_modal_confirm_dialog.h"
+#include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "components/app_modal/javascript_dialog_manager.h"
+#include "content/public/browser/web_contents_delegate.h"
 
 DEFINE_WEB_CONTENTS_USER_DATA_KEY(JavaScriptDialogTabHelper);
 
 namespace {
 
 const base::Feature kAutoDismissingDialogsFeature{
     "AutoDismissingDialogs", base::FEATURE_DISABLED_BY_DEFAULT};
 
 bool IsEnabled() {
   return base::FeatureList::IsEnabled(kAutoDismissingDialogsFeature);
 }
 
-content::JavaScriptDialogManager* AppModalDialogManager() {
+app_modal::JavaScriptDialogManager* AppModalDialogManager() {
   return app_modal::JavaScriptDialogManager::GetInstance();
 }
 
+bool IsWebContentsForemost(content::WebContents* web_contents) {
+  Browser* browser = BrowserList::GetInstance()->GetLastActive();
+  if (!browser) {
+    DCHECK(false);

[Peter Kasting, 2016/10/17 20:16:04]

Do not handle DCHECK failure (banned by style guide).

Just DCHECK(browser); here and keep going.

[Avi (use Gerrit), 2016/10/18 02:46:41]

Done.

+    return false;
+  }
+
+  return browser->tab_strip_model()->GetActiveWebContents() == web_contents;
+}
+
 }  // namespace
 
 JavaScriptDialogTabHelper::JavaScriptDialogTabHelper(
     content::WebContents* web_contents)
     : content::WebContentsObserver(web_contents) {
 }
 
 JavaScriptDialogTabHelper::~JavaScriptDialogTabHelper() {
 }
 
-void JavaScriptDialogTabHelper::WebContentsDestroyed() {
-}
-
 void JavaScriptDialogTabHelper::RunJavaScriptDialog(
-    content::WebContents* web_contents,
+    content::WebContents* alerting_web_contents,

[Peter Kasting, 2016/10/17 20:16:04]

Nit: Param name in .cc and .h should match

[Avi (use Gerrit), 2016/10/18 02:46:41]

Hm.

The problem is that in this function, there are two (possibly different)
WebContentses in play: the alerting one, which is passed in, and the parent one,
which we get a pointer to later.

If we match the .cc and .h names, then we have |parent_web_contents| and
|web_contents| which is awkward. OTOH, if we explicitly call the alerting
WebContents |alerting_web_contents|, like I do in this CL, then that
significantly clarifies things inside this function, but we pay the price of
having the .cc and .h param names not match.

I think the clarity of the variable naming is worth the price of the param names
not matching.

[Peter Kasting, 2016/10/18 04:31:03]

I'm willing to be overridden on this.

Another option would simply be to declare a local:

  // Clearer variable name to avoid ambiguity below.
  content::WebContents* alerting_web_contents = web_contents;

Another option would be to try to rename some of the other variables; maybe call
|parent_web_contents| just |parent| (to me |parent| and |web_contents| are
pretty distinct when reading, so hard to mentally mix up).

     const GURL& origin_url,
     content::JavaScriptMessageType message_type,
     const base::string16& message_text,
     const base::string16& default_prompt_text,
     const DialogClosedCallback& callback,
     bool* did_suppress_message) {
   SiteEngagementService* site_engagement_service = SiteEngagementService::Get(
-      Profile::FromBrowserContext(web_contents->GetBrowserContext()));
+      Profile::FromBrowserContext(alerting_web_contents->GetBrowserContext()));
   double engagement_score = site_engagement_service->GetScore(origin_url);
   switch (message_type) {
     case content::JAVASCRIPT_MESSAGE_TYPE_ALERT:
       UMA_HISTOGRAM_PERCENTAGE("JSDialogs.SiteEngagementOfDialogs.Alert",
                                engagement_score);
       break;
     case content::JAVASCRIPT_MESSAGE_TYPE_CONFIRM:
       UMA_HISTOGRAM_PERCENTAGE("JSDialogs.SiteEngagementOfDialogs.Confirm",
                                engagement_score);
       break;
@@ skipping 11 matching lines @@
                          message_length);
   } else if (engagement_score < 5) {
     UMA_HISTOGRAM_COUNTS("JSDialogs.CharacterCount.EngagementOneToFive",
                          message_length);
   } else {
     UMA_HISTOGRAM_COUNTS("JSDialogs.CharacterCount.EngagementHigher",
                          message_length);
   }
 
   if (IsEnabled()) {
-    NOTREACHED() << "auto-dismissing dialog code does not yet exist";
+    content::WebContents* parent_web_contents =
+        WebContentsObserver::web_contents();
+
+    if (dialog_) {
+      // There's already a dialog up; clear it out.
+      CloseDialog(false, false, base::string16());
+    }
+
+    if (!IsWebContentsForemost(parent_web_contents) &&
+        message_type == content::JAVASCRIPT_MESSAGE_TYPE_PROMPT) {
+      // Don't allow "prompt" dialogs to steal the user's focus. TODO(avi):
+      // Eventually, for subsequent phases of http://bit.ly/project-oldspice,
+      // turn off focus stealing for other dialog types.
+      *did_suppress_message = true;
+      callback.Run(false, base::string16());
+      return;
+    } else {

[Peter Kasting, 2016/10/17 20:16:04]

Nit: No else after return

[Avi (use Gerrit), 2016/10/18 02:46:41]

Done.

+      parent_web_contents->GetDelegate()->ActivateContents(parent_web_contents);
+    }
+
+    base::string16 title =
+        AppModalDialogManager()->GetTitle(alerting_web_contents, origin_url);
+    dialog_callback_ = callback;
+    BrowserList::AddObserver(this);
+    // TODO(avi): abstract out so that we have a Mac version too...
+    dialog_ = JavaScriptDialogViews::Create(
+        parent_web_contents, alerting_web_contents, title, message_type,
+        message_text, default_prompt_text, callback);
+
+    // Message suppression is something that we don't give the user a checkbox
+    // for any more. It was useful back in the day when dialogs were app-modal
+    // and clicking the checkbox was the only way to escape a loop that the page
+    // was doing, but now the user can just close the page.

[Peter Kasting, 2016/10/17 20:16:04]

Nit: I sort of feel like this comment is really a TODO on the
message-suppression logic in general, like "rip all this out when auto-dismissal
is on all the time".

[Avi (use Gerrit), 2016/10/18 02:46:41]

Unfortunately, the old path without auto-dismissal will be with us forever, or
until JavaScript dialogs are entirely removed. We still need that old path,
complete with "suppress further dialogs", for onbeforeunload dialogs, for
extension dialogs, and for WebContentses that aren't browser tabs.

Given that the boolean |did_suppress_message| is going to stay forever in the
dialog API in which we're called here, I wanted to note why we return a constant
value.

[Peter Kasting, 2016/10/18 04:31:03]

Ah.  Maybe just update this comment to clarify this a little, e.g. "While we
still need to provide message suppression for onbeforeunload dialogs, extension
dialogs, and the like, we no longer need them for [phrase for the kind of
dialogs we're handling here] because ..."

+    *did_suppress_message = false;
   } else {
     AppModalDialogManager()->RunJavaScriptDialog(
-        web_contents, origin_url, message_type, message_text,
+        alerting_web_contents, origin_url, message_type, message_text,
         default_prompt_text, callback, did_suppress_message);
   }
 
   if (did_suppress_message) {
     UMA_HISTOGRAM_COUNTS("JSDialogs.CharacterCountUserSuppressed",
                          message_length);
   }
 }
 
 namespace {
@@ skipping 33 matching lines @@
 
   return AppModalDialogManager()->RunBeforeUnloadDialog(
       web_contents, is_reload,
       base::Bind(&SaveUnloadUmaStats, engagement_score, callback));
 }
 
 bool JavaScriptDialogTabHelper::HandleJavaScriptDialog(
     content::WebContents* web_contents,
     bool accept,
     const base::string16* prompt_override) {
-  if (!IsEnabled()) {
-    return AppModalDialogManager()->HandleJavaScriptDialog(web_contents, accept,
-                                                           prompt_override);
+  if (dialog_) {
+    CloseDialog(false /*suppress_callback*/, accept,
+                prompt_override ? *prompt_override : base::string16());
+    return true;
   }
 
-  NOTREACHED() << "auto-dismissing dialog code does not yet exist";
-  return false;
+  // Handle any app-modal dialogs that may be going.

[Peter Kasting, 2016/10/17 20:16:04]

Nit: "going"?  You mean "showing"?  (Sorry, but my brain is having a hard time
parsing "going" here.) (2 places)

[Avi (use Gerrit), 2016/10/18 02:46:41]

Done.

+  return AppModalDialogManager()->HandleJavaScriptDialog(web_contents, accept,
+                                                         prompt_override);
 }
 
 void JavaScriptDialogTabHelper::CancelDialogs(
     content::WebContents* web_contents,
     bool suppress_callbacks,
     bool reset_state) {
+  if (dialog_)
+    CloseDialog(suppress_callbacks, false, base::string16());
+
   // Cancel any app-modal dialogs that may be going.
-  if (!IsEnabled()) {
-    return AppModalDialogManager()->CancelDialogs(
-        web_contents, suppress_callbacks, reset_state);
-  }
+  return AppModalDialogManager()->CancelDialogs(
+      web_contents, suppress_callbacks, reset_state);
+}
 
-  // More work here for the auto-dismissing dialogs.
+void JavaScriptDialogTabHelper::WasHidden() {
+  if (dialog_)
+    CloseDialog(false, false, base::string16());
 }
+
+void JavaScriptDialogTabHelper::OnBrowserSetLastActive(Browser* browser) {
+  if (dialog_ && !IsWebContentsForemost(web_contents()))
+    CloseDialog(false, false, base::string16());
+}
+
+void JavaScriptDialogTabHelper::CloseDialog(bool suppress_callback,
+                                            bool success,
+                                            const base::string16& user_input) {
+  DCHECK(dialog_);
+
+  dialog_->CloseJavaScriptDialog();
+  if (!suppress_callback)
+    dialog_callback_.Run(success, user_input);
+
+  dialog_.reset();
+  dialog_callback_.Reset();
+  BrowserList::RemoveObserver(this);
+}