Make JavaScript dialogs auto-dismiss on tab switch.

chrome/browser/ui/javascript_dialogs/javascript_dialog_views.cc

Index: chrome/browser/ui/javascript_dialogs/javascript_dialog_views.cc
diff --git a/chrome/browser/ui/javascript_dialogs/javascript_dialog_views.cc b/chrome/browser/ui/javascript_dialogs/javascript_dialog_views.cc
new file mode 100644
index 0000000000000000000000000000000000000000..be8b1c4824a758a801917e9bfbf49b45bbd758b3
--- /dev/null
+++ b/chrome/browser/ui/javascript_dialogs/javascript_dialog_views.cc
@@ -0,0 +1,131 @@
+// 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_views.h"
+
+#include "base/memory/ptr_util.h"
+#include "components/constrained_window/constrained_window_views.h"
+#include "ui/base/l10n/l10n_util.h"
+#include "ui/views/controls/message_box_view.h"
+#include "ui/views/controls/textfield/textfield.h"
+#include "ui/views/layout/layout_constants.h"
+
+// static
+base::WeakPtr<JavaScriptDialogViews> JavaScriptDialogViews::Create(
+    content::WebContents* parent_web_contents,
+    content::WebContents* alerting_web_contents,
+    const base::string16& title,
+    content::JavaScriptMessageType message_type,
+    const base::string16& message_text,
+    const base::string16& default_prompt_text,
+    const content::JavaScriptDialogManager::DialogClosedCallback&
+        dialog_callback) {
+  return (new JavaScriptDialogViews(parent_web_contents, alerting_web_contents,
+                                    title, message_type, message_text,
+                                    default_prompt_text, dialog_callback))
+      ->weak_factory_.GetWeakPtr();
+}
+
+//////////////////////////////////////////////////////////////////////////////
+// JavaScriptDialogViews, constructor & destructor:

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

Nit: Personally I'm not a fan of these ///// comment blocks on individual pieces
of a single class' definition, because they tend to get out-of-date later as we
add and move around functions, and they don't seem that helpful in the meantime.
 Also, they can hinder addressing my next comment.

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

Stolen from other Views code. Removed.

+
+JavaScriptDialogViews::JavaScriptDialogViews(

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

Nit: Function definition order should match .cc declaration order.

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

Done.

+    content::WebContents* parent_web_contents,
+    content::WebContents* alerting_web_contents,
+    const base::string16& title,
+    content::JavaScriptMessageType message_type,
+    const base::string16& message_text,
+    const base::string16& default_prompt_text,
+    const content::JavaScriptDialogManager::DialogClosedCallback&
+        dialog_callback)
+    : title_(title),
+      message_type_(message_type),
+      message_text_(message_text),
+      default_prompt_text_(default_prompt_text),
+      dialog_callback_(dialog_callback),
+      weak_factory_(this) {
+  int options = views::MessageBoxView::DETECT_DIRECTIONALITY;
+  if (message_type == content::JAVASCRIPT_MESSAGE_TYPE_PROMPT)
+    options |= views::MessageBoxView::HAS_PROMPT_FIELD;
+
+  views::MessageBoxView::InitParams params(message_text);
+  params.options = options;
+  params.default_prompt = default_prompt_text;
+  message_box_view_ = new views::MessageBoxView(params);
+  DCHECK(message_box_view_);
+
+  constrained_window::ShowWebModalDialogViews(this, parent_web_contents);
+}
+
+JavaScriptDialogViews::~JavaScriptDialogViews() {}
+
+void JavaScriptDialogViews::CloseJavaScriptDialog() {
+  dialog_callback_.Reset();

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

Why would we not call Close() instead?  It's not obvious to me what the
difference in intent of these two functions is, in part because the declaration
for this method in the header has no comment on it.

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

Close() is a Views method, and is an implementation detail due to
JavaScriptDialogViews being derived from views::DialogDelegate. When I write the
Mac UI version, the callbacks are going to be different, so I want to have a
single "hey, dialog, go away" method on both.

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

What I was more asking was, Close() runs the callback and passes false, but this
function doesn't run the callback.  I don't understand why this functional
difference exists.  Is it intentional?  If so, add comments about it, which
would presumably allude to the differing underlying purposes of each of these
functions, which would be explained ( :D ) in comments on their declarations.

If it's not intentional, then I'd call Close() here (and then
GetWidget()->Close()?  Or does that call Close() anyway?) directly to keep the
two in sync.

[Avi (use Gerrit), 2016/10/18 16:31:49]

Yes; JavaScriptDialogManager's call CancelDialogs has a "suppress_callbacks"
parameter. We need the ability to not do the callbacks. The other problem is
that JavaScriptDialogManager has HandleJavaScriptDialog which requires the
callback to be fired with custom values. If we want to have the view _always_ be
the one to fire callbacks, then we have to plumb three values (suppress
callback, custom result, custom user input) into the "close" method, and then we
end up polluting the view class with lots of code that is cross-platform.

This is the best solution I could find. Have the view class have an
externally-visible "make this go away silently with no callbacks" call, and have
the core tab helper do the work of figuring out whether or not to do callbacks
and with what values depending on what call it gets on the
JavaScriptDialogManager interface.
But this is a simple design. The view has the ability to go away silently, and
the tab helper handles the weirdness of the JavaScriptDialogManager interface.

[Peter Kasting, 2016/10/18 16:59:55]

Seems fine.
Right, I don't object to the design.  I just suggest clarifying the
understanding above for readers like me who do not necessarily know all this in
their heads.  For example, rename this function CloseDialogWithoutCallback() and
write a comment in the header giving some of the explanation you just gave me
about what this ends up being useful for, how it differs from the Close()
method, and why that method should, in fact, call the callback.

+  GetWidget()->Close();
+}
+
+//////////////////////////////////////////////////////////////////////////////
+// JavaScriptDialogViews, views::DialogDelegate implementation:
+
+int JavaScriptDialogViews::GetDefaultDialogButton() const {
+  return ui::DIALOG_BUTTON_OK;
+}
+
+int JavaScriptDialogViews::GetDialogButtons() const {
+  if (message_type_ == content::JAVASCRIPT_MESSAGE_TYPE_ALERT)
+    return ui::DIALOG_BUTTON_OK;
+
+  return ui::DIALOG_BUTTON_OK | ui::DIALOG_BUTTON_CANCEL;

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

Nit: Shorter:

  const bool is_alert = message_type_ == content::JAVASCRIPT_MESSAGE_TYPE_ALERT;
  return ui::DIALOG_BUTTON_OK | (is_alert ? 0 : ui::DIALOG_BUTTON_CANCEL);

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

OK.

(I don't know what Views idiom is, so I stole it from another file.)

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

Dunno that we have a consistent idiom for this.  Pretty much every way you could
phrase this is in use in some places.

+}
+
+base::string16 JavaScriptDialogViews::GetWindowTitle() const {
+  return title_;
+}
+
+bool JavaScriptDialogViews::Cancel() {
+  if (dialog_callback_)
+    dialog_callback_.Run(false, base::string16());
+  return true;
+}
+
+bool JavaScriptDialogViews::Accept() {
+  if (dialog_callback_)
+    dialog_callback_.Run(true, message_box_view_->GetInputText());
+  return true;
+}
+
+bool JavaScriptDialogViews::Close() {
+  if (dialog_callback_)
+    dialog_callback_.Run(false, base::string16());
+  return true;
+}
+
+void JavaScriptDialogViews::DeleteDelegate() {
+  delete this;
+}
+
+///////////////////////////////////////////////////////////////////////////////
+// JavaScriptDialogViews, views::WidgetDelegate implementation:
+
+views::View* JavaScriptDialogViews::GetContentsView() {
+  return message_box_view_;
+}
+
+views::View* JavaScriptDialogViews::GetInitiallyFocusedView() {
+  if (message_box_view_->text_box())
+    return message_box_view_->text_box();
+  return views::DialogDelegate::GetInitiallyFocusedView();

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

Nit: Shorter:

  auto* text_box = message_box_view_->text_box();
  return text_box ? text_box : views::DialogDelegate::GetInitiallyFocusedView();

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

ditto re stolen

+}
+
+views::Widget* JavaScriptDialogViews::GetWidget() {
+  return message_box_view_->GetWidget();
+}
+
+const views::Widget* JavaScriptDialogViews::GetWidget() const {
+  return message_box_view_->GetWidget();
+}
+
+ui::ModalType JavaScriptDialogViews::GetModalType() const {
+  return ui::MODAL_TYPE_CHILD;
+}