Dismiss action in tab modal dialogs.

Dismiss action in tab modal dialogs.

See design document: https://docs.google.com/a/google.com/document/d/1i7V7rIcZiWRd9RqWDBC7bdtXeoT3ievSwgUOqrS1148/edit?usp=sharing

TEST=
1. Sign in to Chromium. (Hot-dog menu > Sign in to Chromium...) 
2. Sign out from Chromium. (Hot-dog menu > Settings, then click the "Disconnect your Google Account..." button) 
3. Sign in to Chromium with a different account than the one used at step 1. 
4. Do one of the following action once the email confirmation dialog appears:
- Click the X on the top-left corner.
- Close the parent tab.
- Navigate to a different URL in the parent tab.
Expected result: The dialog disappears and no action is performed (no Settings page is opened and the user is not signed in).

BUG=175647
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=214965

[fdoray, 2013-06-28 15:32:59]

https://codereview.chromium.org/18179004/diff/1/chrome/browser/download/downl...
File chrome/browser/download/download_danger_prompt.cc (right):

https://codereview.chromium.org/18179004/diff/1/chrome/browser/download/downl...
chrome/browser/download/download_danger_prompt.cc:151: RunCallback(canceled_);
Should I call OnCanceled here to avoid code duplication? It would break the
guarantee that exactly one |On...| method is called.

https://codereview.chromium.org/18179004/diff/1/chrome/browser/ui/tab_modal_c...
File chrome/browser/ui/tab_modal_confirm_dialog_delegate.h (right):

https://codereview.chromium.org/18179004/diff/1/chrome/browser/ui/tab_modal_c...
chrome/browser/ui/tab_modal_confirm_dialog_delegate.h:50: bool Accept();
Changing the return value is helpful to avoid multiple calls to
DialogDelegate::OnClose (see DialogClientView::CanClose).

I think it's cleaner to avoid multiple calls to DialogDelegate::OnClose, but it
doesn't really change anything since the implementation of this method is always
empty for tab modal dialogs.

https://codereview.chromium.org/18179004/diff/1/ui/views/window/dialog_client...
File ui/views/window/dialog_client_view.cc (right):

https://codereview.chromium.org/18179004/diff/1/ui/views/window/dialog_client...
ui/views/window/dialog_client_view.cc:116: GetDialogDelegate()->OnClose();
Before this CL, DialogDelegate::OnClose was not called when a dialog was closed
by clicking the "X" button.

[benjhayden, 2013-06-28 16:05:46]

lgtm

https://codereview.chromium.org/18179004/diff/1/chrome/browser/download/downl...
File chrome/browser/download/download_danger_prompt.cc (right):

https://codereview.chromium.org/18179004/diff/1/chrome/browser/download/downl...
chrome/browser/download/download_danger_prompt.cc:151: RunCallback(canceled_);
On 2013/06/28 15:32:59, fdoray wrote:
> Should I call OnCanceled here to avoid code duplication? It would break the
> guarantee that exactly one |On...| method is called.

I have a slight preference for calling RunCallback over calling OnCanceled for
consistency, but it doesn't matter much to me.
Alternatively, you could make the base class implementation of OnDismissed()
call OnCanceled() so that only those prompts that want to do something different
need to override it.

[Roger Tawa OOO till Jul 10th, 2013-07-02 15:09:11]

lgtm

Personally, I think its easier to understand the base class if OnDismissed()
does not call OnCanceled().

[fdoray, 2013-07-02 15:15:53]

+ben@chromium.org
Owner

[benjhayden, 2013-07-09 20:01:35]

Ping?
It would be nice if this were committed so that
https://codereview.chromium.org/16924017/ could use it.
You may need to ping owners via IM in order to get them to actually review.

[fdoray, 2013-07-12 18:58:33]

Rebase.

My CL makes changes in the same spot of tab_modal_confirm_dialog_delegate.cc
than https://chromiumcodereview.appspot.com/17500003 (which was committed and
then reverted). The conflict will be easy to resolve. My dismiss action works
fine even when NOTIFICATION_LOAD_START is moved out of
tab_modal_confirm_dialog_delegate.cc.

[fdoray, 2013-07-12 19:09:00]

-ben@
+sky@
Could you review this CL?

[fdoray, 2013-07-12 19:57:59]

+jam@
Reviewer for chrome/browser/repost_form_warning_controller.*?

[jam, 2013-07-12 21:31:31]

lgtm

[noms (inactive), 2013-07-15 14:20:44]

lgtm

[sky, 2013-07-15 19:48:59]

What swap Ben for me?

[fdoray, 2013-07-15 20:14:04]

On 2013/07/15 19:48:59, sky wrote:
> What swap Ben for me?

I wasn't sure he was available since I didn't get any reply from him in the last
days.

[sky, 2013-07-15 21:01:27]

He's around.

On Mon, Jul 15, 2013 at 1:14 PM,  <fdoray@chromium.org> wrote:
> On 2013/07/15 19:48:59, sky wrote:
>>
>> What swap Ben for me?
>
>
> I wasn't sure he was available since I didn't get any reply from him in the
> last
> days.
>
> https://codereview.chromium.org/18179004/

[fdoray, 2013-07-15 21:08:16]

+ben@
Could you review this CL?
Thanks.

[fdoray, 2013-07-17 17:43:36]

Ben, could you review this CL?
Thanks.

On 2013/07/15 21:08:16, fdoray wrote:
> +ben@
> Could you review this CL?
> Thanks.

[fdoray, 2013-07-22 13:40:06]

+sky@
Scott, could you review this CL? Ben seems to be too busy to review it.
Thanks!

[Ben Goodger (Google), 2013-07-23 17:52:09]

https://codereview.chromium.org/18179004/diff/9001/ui/views/window/dialog_del...
File ui/views/window/dialog_delegate.h (right):

https://codereview.chromium.org/18179004/diff/9001/ui/views/window/dialog_del...
ui/views/window/dialog_delegate.h:78: virtual bool Dismiss();
Can we call this "Close()"? That is more in keeping with platform conventions
(e.g. WM_CLOSE). I would then rephrase this comment to:

// Called when the user closes the window without selecting an option, e.g. by
pressing the close button on the window or using a window manager gesture. By
default this calls the Cancel() function.

[fdoray, 2013-07-24 15:09:16]

Ben, could you review this again?
Thanks.

https://codereview.chromium.org/18179004/diff/9001/ui/views/window/dialog_del...
File ui/views/window/dialog_delegate.h (right):

https://codereview.chromium.org/18179004/diff/9001/ui/views/window/dialog_del...
ui/views/window/dialog_delegate.h:78: virtual bool Dismiss();
Yes... but there is a |OnClose| method in DialogDelegate that is called for
every action (Accept, Cancel, Dismiss). Will that be confusing?

Also, some subclasses have a |Close| method that actually closes the dialog
(ExtensionDialog, BookmarkEditorView). Here, Dismiss doesn't actually close the
dialog: it just returns a boolean (and do some extra stuff when it's
overridden).

Can I call the method |Closed|?

On 2013/07/23 17:52:09, Ben Goodger (Google) wrote:
> Can we call this "Close()"? That is more in keeping with platform conventions
> (e.g. WM_CLOSE). I would then rephrase this comment to:
> 
> // Called when the user closes the window without selecting an option, e.g. by
> pressing the close button on the window or using a window manager gesture. By
> default this calls the Cancel() function.

[Ben Goodger (Google), 2013-07-24 17:56:21]

https://codereview.chromium.org/18179004/diff/9001/ui/views/window/dialog_del...
File ui/views/window/dialog_delegate.h (right):

https://codereview.chromium.org/18179004/diff/9001/ui/views/window/dialog_del...
ui/views/window/dialog_delegate.h:78: virtual bool Dismiss();
I'd change the notification below to OnClosed() (past tense, to match its
behavior) and change this to Close() (verb).

Bigger picture, there should probably be a WidgetObserver method that tracks
widget closing that people observer, rather than having a
DialogDelegate-specific method.

[fdoray, 2013-07-24 20:21:06]

Ben: I renamed the methods as you suggested. CL is ready to be reviewed again.

https://codereview.chromium.org/18179004/diff/9001/ui/views/window/dialog_del...
File ui/views/window/dialog_delegate.h (right):

https://codereview.chromium.org/18179004/diff/9001/ui/views/window/dialog_del...
ui/views/window/dialog_delegate.h:78: virtual bool Dismiss();
Done.

Little concern about using a verb: as I said in my last comment, this doesn't
close the dialog. It's called from DialogClientView::CanClose, when the widget
is already trying to close itself.

On 2013/07/24 17:56:21, Ben Goodger (Google) wrote:
> I'd change the notification below to OnClosed() (past tense, to match its
> behavior) and change this to Close() (verb).
> 
> Bigger picture, there should probably be a WidgetObserver method that tracks
> widget closing that people observer, rather than having a
> DialogDelegate-specific method.

https://codereview.chromium.org/18179004/diff/53001/chrome/browser/ui/cocoa/t...
File chrome/browser/ui/cocoa/tab_modal_confirm_dialog_mac.mm (right):

https://codereview.chromium.org/18179004/diff/53001/chrome/browser/ui/cocoa/t...
chrome/browser/ui/cocoa/tab_modal_confirm_dialog_mac.mm:117: delegate_->Close();
Cannot be done in a separate CL because the tests would be broken.

https://codereview.chromium.org/18179004/diff/53001/chrome/browser/ui/tab_mod...
File chrome/browser/ui/tab_modal_confirm_dialog_delegate.cc (right):

https://codereview.chromium.org/18179004/diff/53001/chrome/browser/ui/tab_mod...
chrome/browser/ui/tab_modal_confirm_dialog_delegate.cc:62: CloseDialog();
For Views, calling CloseDialog() in Cancel(), Accept() and Close() is useless
because these methods are called when the widget is already trying to close
itself. The call is there for GTK and Cocoa.

Better to move the calls after the calls to Cancel, Accept and Close in the GTK
and Cocoa implementations?

[Ben Goodger (Google), 2013-07-25 18:06:47]

lgtm

[fdoray, 2013-07-25 18:23:44]

Owners, could you review this CL?
Thanks!

achuith@
chrome/browser/chromeos/enrollment_dialog_view.cc

asvitkine@
chrome/browser/ui/cocoa/tab_modal_confirm_dialog_mac.mm

erg@
chrome/browser/ui/gtk/tab_modal_confirm_dialog_gtk.cc

[Alexei Svitkine (slow), 2013-07-25 18:24:57]

cocoa lgtm

[Elliot Glaysher, 2013-07-25 18:26:21]

gtk lgtm

[achuithb, 2013-07-25 18:39:24]

chromeos/enrollment_dialog_view.cc rubberstamp lgtm

[commit-bot: I haz the power, 2013-07-25 19:23:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fdoray@chromium.org/18179004/61001

[commit-bot: I haz the power, 2013-07-25 22:35:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fdoray@chromium.org/18179004/61001

[commit-bot: I haz the power, 2013-07-25 22:36:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fdoray@chromium.org/18179004/61001

[commit-bot: I haz the power, 2013-07-25 23:06:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fdoray@chromium.org/18179004/61001

[commit-bot: I haz the power, 2013-07-26 03:37:58]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[fdoray, 2013-07-29 15:21:03]

achuith, Ben: The code changed since you reviewed it. Do you want to review it
again?

achuith: CrOS files added to the CL:
chrome/browser/chromeos/power/idle_action_warning*
chrome/browser/chromeos/ui/app_launch_view.*
chrome/browser/chromeos/ui/idle_logout_dialog_view*

Ben:
I removed the return value from
TabModalConfirmDialogViews::(Cancel|Accept|Close) because it broke tests that
called TabModalConfirmDialogDelegate(Cancel|Accept) directly. When
DialogClientView::CanClose was called, notified_delegate_ was false and
GetDialogDelegate()->Close() returned false, so the dialog was never closed.

With the new code, DialogDelegate::Close always return true for tab modal
dialogs. notified_delegate_ guarantees that DialogDelegate::OnClosed is still
called exactly 1 time.

[achuithb, 2013-07-29 17:56:38]

I didn't look over the entire CL, just the cros parts.

https://codereview.chromium.org/18179004/diff/159001/chrome/browser/chromeos/...
File chrome/browser/chromeos/power/idle_action_warning_dialog_view.h (right):

https://codereview.chromium.org/18179004/diff/159001/chrome/browser/chromeos/...
chrome/browser/chromeos/power/idle_action_warning_dialog_view.h:20: void
CloseDialog();
Why does this need to be renamed?

https://codereview.chromium.org/18179004/diff/159001/chrome/browser/chromeos/...
File chrome/browser/chromeos/ui/idle_logout_dialog_view.cc (right):

https://codereview.chromium.org/18179004/diff/159001/chrome/browser/chromeos/...
chrome/browser/chromeos/ui/idle_logout_dialog_view.cc:79:
g_instance->GetWidget()->Close();
Shouldn't you be calling g_instance->Close() here?

[fdoray, 2013-07-29 18:38:37]

achuith@ : I answered your questions. Can you review again?

https://codereview.chromium.org/18179004/diff/159001/chrome/browser/chromeos/...
File chrome/browser/chromeos/power/idle_action_warning_dialog_view.h (right):

https://codereview.chromium.org/18179004/diff/159001/chrome/browser/chromeos/...
chrome/browser/chromeos/power/idle_action_warning_dialog_view.h:20: void
CloseDialog();
There's a new DialogDelegate::Close method which name is conflicting with this
method (DialogDelegate is a base class for this class).

DialogDelegate::Close is called when the dialog is closed without selecting an
option (pressing X button, close Chrome, window manager gesture...). Unlike
IdleActionWarningDialogView::Close, it doesn't actually close the dialog. We
cannot override DialogDelegate::Close here because the 2 methods don't do the
same thing at all.

DialogDelegate::Close is the name suggested by Ben.

On 2013/07/29 17:56:38, achuith.bhandarkar wrote:
> Why does this need to be renamed?

https://codereview.chromium.org/18179004/diff/159001/chrome/browser/chromeos/...
File chrome/browser/chromeos/ui/idle_logout_dialog_view.cc (right):

https://codereview.chromium.org/18179004/diff/159001/chrome/browser/chromeos/...
chrome/browser/chromeos/ui/idle_logout_dialog_view.cc:79:
g_instance->GetWidget()->Close();
On 2013/07/29 17:56:38, achuith.bhandarkar wrote:
> Shouldn't you be calling g_instance->Close() here?

No. Calling g_instance->Close() wouldn't close the dialog. This line will
generate a call to DialogClientView::CanClose(), which will call Close() on
g_instance.

[achuithb, 2013-07-29 18:42:49]

owner lgtm for chrome/browser/chromeos/*

[Ben Goodger (Google), 2013-07-30 22:35:05]

lgtm

[commit-bot: I haz the power, 2013-07-31 17:26:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fdoray@chromium.org/18179004/220001

[commit-bot: I haz the power, 2013-07-31 18:10:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fdoray@chromium.org/18179004/220001

[commit-bot: I haz the power, 2013-07-31 18:10:43]

Failed to apply patch for chrome/browser/ui/gtk/tab_modal_confirm_dialog_gtk.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/ui/gtk/tab_modal_confirm_dialog_gtk.cc
  Hunk #1 FAILED at 33.
  Hunk #2 succeeded at 108 (offset -1 lines).
  Hunk #3 succeeded at 124 with fuzz 1 (offset -1 lines).
  1 out of 3 hunks FAILED -- saving rejects to file
chrome/browser/ui/gtk/tab_modal_confirm_dialog_gtk.cc.rej

Patch:       chrome/browser/ui/gtk/tab_modal_confirm_dialog_gtk.cc
Index: chrome/browser/ui/gtk/tab_modal_confirm_dialog_gtk.cc
diff --git a/chrome/browser/ui/gtk/tab_modal_confirm_dialog_gtk.cc
b/chrome/browser/ui/gtk/tab_modal_confirm_dialog_gtk.cc
index
625b7c8927a0ea0573e980015639a7f965d666e3..ff39efb22da580edd2f4c2ed71e3e3a65854a63c
100644
--- a/chrome/browser/ui/gtk/tab_modal_confirm_dialog_gtk.cc
+++ b/chrome/browser/ui/gtk/tab_modal_confirm_dialog_gtk.cc
@@ -33,7 +33,8 @@ TabModalConfirmDialogGtk::TabModalConfirmDialogGtk(
     content::WebContents* web_contents)
     : web_contents_(web_contents),
       delegate_(delegate),
-      window_(NULL) {
+      window_(NULL),
+      closing_(false) {
   dialog_ = gtk_vbox_new(FALSE, ui::kContentAreaSpacing);
   GtkWidget* label = gtk_label_new(
       UTF16ToUTF8(delegate->GetMessage()).c_str());
@@ -109,6 +110,10 @@ TabModalConfirmDialogGtk::TabModalConfirmDialogGtk(
 }
 
 TabModalConfirmDialogGtk::~TabModalConfirmDialogGtk() {
+  // Provide a disposition in case the dialog was closed without accepting or
+  // cancelling.
+  delegate_->Close();
+
   gtk_widget_destroy(dialog_);
 }
 
@@ -121,7 +126,10 @@ void TabModalConfirmDialogGtk::CancelTabModalDialog() {
 }
 
 void TabModalConfirmDialogGtk::CloseDialog() {
-  gtk_widget_destroy(window_);
+  if (!closing_) {
+    closing_ = true;
+    gtk_widget_destroy(window_);
+  }
 }
 
 void TabModalConfirmDialogGtk::SetPreventCloseOnLoadStart(bool prevent) {

[commit-bot: I haz the power, 2013-07-31 18:26:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fdoray@chromium.org/18179004/238001

[commit-bot: I haz the power, 2013-07-31 19:13:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fdoray@chromium.org/18179004/220002

[commit-bot: I haz the power, 2013-07-31 21:42:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fdoray@chromium.org/18179004/214002

[commit-bot: I haz the power, 2013-08-01 01:29:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fdoray@chromium.org/18179004/273001

[commit-bot: I haz the power, 2013-08-01 01:45:44]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-08-01 02:27:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fdoray@chromium.org/18179004/273001

[commit-bot: I haz the power, 2013-08-01 02:36:02]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-08-01 02:51:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fdoray@chromium.org/18179004/273001

[commit-bot: I haz the power, 2013-08-01 05:46:23]

Change committed as 214965