chrome: Add TabModalConfirmDialog interface.

chrome: Add TabModalConfirmDialog interface.

BUG=125846
R=pkasting@chromium.org
TBR=ben@chromium.org

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

[tfarina, 2012-09-24 23:29:47]

Peter, it seems Ben is out sick. Please, could you review this to me instead?

[tfarina, 2012-09-24 23:31:22]

http://codereview.chromium.org/10972002/diff/13014/chrome/browser/ui/tab_moda...
File chrome/browser/ui/tab_modal_confirm_dialog_browsertest.h (right):

http://codereview.chromium.org/10972002/diff/13014/chrome/browser/ui/tab_moda...
chrome/browser/ui/tab_modal_confirm_dialog_browsertest.h:10: #include
"chrome/browser/ui/tab_modal_confirm_dialog.h"
Just noticed, I can forward declare this. I'll do right way.

[Peter Kasting, 2012-09-24 23:33:16]

I don't have the slightest idea what this buys us.

[tfarina, 2012-09-25 00:23:08]

On 2012/09/24 23:33:16, Peter Kasting wrote:
> I don't have the slightest idea what this buys us.

OK. I'll wait for Ben so ;)

But the idea is that nobody outside of chrome/browser/ui/views/, should include
from chrome/browser/ui/views/.

By adding an interface we are able to remove the platform-specific files from
chrome/browser/ui/tab_modal_confirm_dialog_browsertest.h, so it doesn't need to
know about the ui implementation details.

Hope that is clearer now.

[Peter Kasting, 2012-09-25 00:39:20]

http://codereview.chromium.org/10972002/diff/13014/chrome/browser/ui/cocoa/ta...
File chrome/browser/ui/cocoa/tab_modal_confirm_dialog_mac.h (right):

http://codereview.chromium.org/10972002/diff/13014/chrome/browser/ui/cocoa/ta...
chrome/browser/ui/cocoa/tab_modal_confirm_dialog_mac.h:48: // the new
constrained window look and feel.
Shouldn't this implement your interface too?

http://codereview.chromium.org/10972002/diff/13014/chrome/browser/ui/cocoa/ta...
File chrome/browser/ui/cocoa/tab_modal_confirm_dialog_mac.mm (right):

http://codereview.chromium.org/10972002/diff/13014/chrome/browser/ui/cocoa/ta...
chrome/browser/ui/cocoa/tab_modal_confirm_dialog_mac.mm:93: NSWindow* window =
[(NSAlert*)sheet() window];
Nit: This can call CancelTabModalDialog() instead

http://codereview.chromium.org/10972002/diff/13014/chrome/browser/ui/cocoa/ta...
chrome/browser/ui/cocoa/tab_modal_confirm_dialog_mac.mm:122: // static
Nit: Place this atop this .cc file

http://codereview.chromium.org/10972002/diff/13014/chrome/browser/ui/gtk/tab_...
File chrome/browser/ui/gtk/tab_modal_confirm_dialog_gtk.cc (right):

http://codereview.chromium.org/10972002/diff/13014/chrome/browser/ui/gtk/tab_...
chrome/browser/ui/gtk/tab_modal_confirm_dialog_gtk.cc:113:
TabModalConfirmDialog* TabModalConfirmDialog::Create(
Nit: Place atop .cc file

http://codereview.chromium.org/10972002/diff/13014/chrome/browser/ui/gtk/tab_...
File chrome/browser/ui/gtk/tab_modal_confirm_dialog_gtk.h (right):

http://codereview.chromium.org/10972002/diff/13014/chrome/browser/ui/gtk/tab_...
chrome/browser/ui/gtk/tab_modal_confirm_dialog_gtk.h:30: // Overridden from
ConstrainedWindowGtkDelegate:
Nit: Be consistent; the the Mac header you used "XXX methods:".

(I just simplify these to "XXX:".)

http://codereview.chromium.org/10972002/diff/13014/chrome/browser/ui/tab_moda...
File chrome/browser/ui/tab_modal_confirm_dialog.h (right):

http://codereview.chromium.org/10972002/diff/13014/chrome/browser/ui/tab_moda...
chrome/browser/ui/tab_modal_confirm_dialog.h:13: static TabModalConfirmDialog*
Create(TabModalConfirmDialogDelegate* delegate,
This serves the same purpose as ShowTabModalConfirmDialog().  Pick one or the
other (personally I slightly prefer this Create() function), not both.

This will likely require you to also touch the TabModalConfirmDialogWebUI
implementation.

http://codereview.chromium.org/10972002/diff/13014/chrome/browser/ui/tab_moda...
chrome/browser/ui/tab_modal_confirm_dialog.h:16: virtual
~TabModalConfirmDialog() {}
Nit: Should probably be protected, I doubt we want people deleting through this
interface

http://codereview.chromium.org/10972002/diff/13014/chrome/browser/ui/views/ta...
File chrome/browser/ui/views/tab_modal_confirm_dialog_views.cc (right):

http://codereview.chromium.org/10972002/diff/13014/chrome/browser/ui/views/ta...
chrome/browser/ui/views/tab_modal_confirm_dialog_views.cc:97:
TabModalConfirmDialog* TabModalConfirmDialog::Create(
Nit: Put this atop the .cc file

[tfarina, 2012-09-25 02:03:38]

http://codereview.chromium.org/10972002/diff/13014/chrome/browser/ui/cocoa/ta...
File chrome/browser/ui/cocoa/tab_modal_confirm_dialog_mac.mm (right):

http://codereview.chromium.org/10972002/diff/13014/chrome/browser/ui/cocoa/ta...
chrome/browser/ui/cocoa/tab_modal_confirm_dialog_mac.mm:93: NSWindow* window =
[(NSAlert*)sheet() window];
On 2012/09/25 00:39:20, Peter Kasting wrote:
> Nit: This can call CancelTabModalDialog() instead

Done.

http://codereview.chromium.org/10972002/diff/13014/chrome/browser/ui/cocoa/ta...
chrome/browser/ui/cocoa/tab_modal_confirm_dialog_mac.mm:122: // static
On 2012/09/25 00:39:20, Peter Kasting wrote:
> Nit: Place this atop this .cc file

Done.

http://codereview.chromium.org/10972002/diff/13014/chrome/browser/ui/gtk/tab_...
File chrome/browser/ui/gtk/tab_modal_confirm_dialog_gtk.cc (right):

http://codereview.chromium.org/10972002/diff/13014/chrome/browser/ui/gtk/tab_...
chrome/browser/ui/gtk/tab_modal_confirm_dialog_gtk.cc:113:
TabModalConfirmDialog* TabModalConfirmDialog::Create(
On 2012/09/25 00:39:20, Peter Kasting wrote:
> Nit: Place atop .cc file

Done.

http://codereview.chromium.org/10972002/diff/13014/chrome/browser/ui/gtk/tab_...
File chrome/browser/ui/gtk/tab_modal_confirm_dialog_gtk.h (right):

http://codereview.chromium.org/10972002/diff/13014/chrome/browser/ui/gtk/tab_...
chrome/browser/ui/gtk/tab_modal_confirm_dialog_gtk.h:30: // Overridden from
ConstrainedWindowGtkDelegate:
On 2012/09/25 00:39:20, Peter Kasting wrote:
> Nit: Be consistent; the the Mac header you used "XXX methods:".
> 
> (I just simplify these to "XXX:".)

Done.

http://codereview.chromium.org/10972002/diff/13014/chrome/browser/ui/tab_moda...
File chrome/browser/ui/tab_modal_confirm_dialog.h (right):

http://codereview.chromium.org/10972002/diff/13014/chrome/browser/ui/tab_moda...
chrome/browser/ui/tab_modal_confirm_dialog.h:13: static TabModalConfirmDialog*
Create(TabModalConfirmDialogDelegate* delegate,
On 2012/09/25 00:39:20, Peter Kasting wrote:
> This serves the same purpose as ShowTabModalConfirmDialog().  Pick one or the
> other (personally I slightly prefer this Create() function), not both.
> 
> This will likely require you to also touch the TabModalConfirmDialogWebUI
> implementation.

Can I do this in a follow up, or this should be done within this patch?

http://codereview.chromium.org/10972002/diff/13014/chrome/browser/ui/tab_moda...
chrome/browser/ui/tab_modal_confirm_dialog.h:16: virtual
~TabModalConfirmDialog() {}
On 2012/09/25 00:39:20, Peter Kasting wrote:
> Nit: Should probably be protected, I doubt we want people deleting through
this
> interface

Done.

http://codereview.chromium.org/10972002/diff/13014/chrome/browser/ui/views/ta...
File chrome/browser/ui/views/tab_modal_confirm_dialog_views.cc (right):

http://codereview.chromium.org/10972002/diff/13014/chrome/browser/ui/views/ta...
chrome/browser/ui/views/tab_modal_confirm_dialog_views.cc:97:
TabModalConfirmDialog* TabModalConfirmDialog::Create(
On 2012/09/25 00:39:20, Peter Kasting wrote:
> Nit: Put this atop the .cc file

Done.

[Peter Kasting, 2012-09-25 02:10:31]

http://codereview.chromium.org/10972002/diff/13014/chrome/browser/ui/tab_moda...
File chrome/browser/ui/tab_modal_confirm_dialog.h (right):

http://codereview.chromium.org/10972002/diff/13014/chrome/browser/ui/tab_moda...
chrome/browser/ui/tab_modal_confirm_dialog.h:13: static TabModalConfirmDialog*
Create(TabModalConfirmDialogDelegate* delegate,
On 2012/09/25 02:03:38, tfarina wrote:
> On 2012/09/25 00:39:20, Peter Kasting wrote:
> > This serves the same purpose as ShowTabModalConfirmDialog().  Pick one or
the
> > other (personally I slightly prefer this Create() function), not both.
> > 
> > This will likely require you to also touch the TabModalConfirmDialogWebUI
> > implementation.
> 
> Can I do this in a follow up, or this should be done within this patch?

It probably makes sense to do it in this patch because it will mean making more
classes derive from TabModalConfirmDialog, and that in turn may influence the
rest of the design here.

[tfarina, 2012-09-25 02:10:42]

http://codereview.chromium.org/10972002/diff/13014/chrome/browser/ui/tab_moda...
File chrome/browser/ui/tab_modal_confirm_dialog.h (right):

http://codereview.chromium.org/10972002/diff/13014/chrome/browser/ui/tab_moda...
chrome/browser/ui/tab_modal_confirm_dialog.h:13: static TabModalConfirmDialog*
Create(TabModalConfirmDialogDelegate* delegate,
On 2012/09/25 00:39:20, Peter Kasting wrote:
> This serves the same purpose as ShowTabModalConfirmDialog().
>  Pick one or the
> other (personally I slightly prefer this Create() function), not both.
> 
> This will likely require you to also touch the TabModalConfirmDialogWebUI
> implementation.
Would you be fine if I add a Show() function here?

virtual void Show() = 0;

And then in the clients we do:

dialog = TabModalConfirmDialog::Create(...);
dialog->Show();

[Peter Kasting, 2012-09-25 02:11:37]

On 2012/09/25 02:10:42, tfarina wrote:
>
http://codereview.chromium.org/10972002/diff/13014/chrome/browser/ui/tab_moda...
> File chrome/browser/ui/tab_modal_confirm_dialog.h (right):
> 
>
http://codereview.chromium.org/10972002/diff/13014/chrome/browser/ui/tab_moda...
> chrome/browser/ui/tab_modal_confirm_dialog.h:13: static TabModalConfirmDialog*
> Create(TabModalConfirmDialogDelegate* delegate,
> On 2012/09/25 00:39:20, Peter Kasting wrote:
> > This serves the same purpose as ShowTabModalConfirmDialog().
> >  Pick one or the
> > other (personally I slightly prefer this Create() function), not both.
> > 
> > This will likely require you to also touch the TabModalConfirmDialogWebUI
> > implementation.
> Would you be fine if I add a Show() function here?
> 
> virtual void Show() = 0;
> 
> And then in the clients we do:
> 
> dialog = TabModalConfirmDialog::Create(...);
> dialog->Show();

When would we want Create() to not imply Show()?

[tfarina, 2012-09-25 02:18:07]

On 2012/09/25 02:11:37, Peter Kasting wrote:
> This serves the same purpose as ShowTabModalConfirmDialog().
>  Pick one or the
> other (personally I slightly prefer this Create() function), not both.
> 
> This will likely require you to also touch the TabModalConfirmDialogWebUI
> implementation.
> When would we want Create() to not imply Show()?

OK, I'll convert the calls from ShowTabModalConfirmDialog() to
TabModalConfirmDialog::Create() tomorrow.

[tfarina, 2012-09-25 17:47:48]

Done. PTAL.

[Peter Kasting, 2012-09-25 21:08:42]

LGTM

http://codereview.chromium.org/10972002/diff/29015/chrome/browser/ui/cocoa/ta...
File chrome/browser/ui/cocoa/tab_modal_confirm_dialog_mac.mm (right):

http://codereview.chromium.org/10972002/diff/29015/chrome/browser/ui/cocoa/ta...
chrome/browser/ui/cocoa/tab_modal_confirm_dialog_mac.mm:27: } else {
Nit: No else after return