Change ConfirmMessageBoxDialog to just be a native view (it was only windows ...

Change ConfirmMessageBoxDialog to just be a native view (it was only windows before, too, but now at least it doesn't
cause zombie processes) with an observer that is notified when the user clicks OK/closes the dialog instead of running
a nested message loop and blocking.  This fixes bug 20451, where nested message loops were running a confirm dialog and
could cause zombie processes at browser shutdown.

BUG=20451
TEST=None (manual: verify that the confirmation dialog is shown when you try to remove all passwords, and that no zombie is left behind if the browser is closed while the confirmation dialog is showing)

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=27961

[tfarina (gmail-do not use), 2009-10-01 00:53:03]

http://codereview.chromium.org/243054/diff/1/6
File chrome/browser/views/confirm_message_box_dialog.h (right):

http://codereview.chromium.org/243054/diff/1/6#newcode13
Line 13: #include "views/controls/label.h"
Tim could you move this to line 12 instead?

http://codereview.chromium.org/243054/diff/1/6#newcode36
Line 36: const std::wstring& window_text);
Either |window_title| or change the others |window_title| to |window_text|?

I prefer window_title because it matchs with GetWindowTitle() function.

http://codereview.chromium.org/243054/diff/1/6#newcode51
Line 51: // views::View overrides.
May be 'implementation.' to match with all others above?

[tim (not reviewing), 2009-10-01 00:56:35]

On 2009/10/01 00:53:03, tfarina wrote:
> http://codereview.chromium.org/243054/diff/1/6
> File chrome/browser/views/confirm_message_box_dialog.h (right):
> 
> http://codereview.chromium.org/243054/diff/1/6#newcode13
> Line 13: #include "views/controls/label.h"
> Tim could you move this to line 12 instead?
> 
> http://codereview.chromium.org/243054/diff/1/6#newcode36
> Line 36: const std::wstring& window_text);
> Either |window_title| or change the others |window_title| to |window_text|?
> 
> I prefer window_title because it matchs with GetWindowTitle() function.
> 
> http://codereview.chromium.org/243054/diff/1/6#newcode51
> Line 51: // views::View overrides.
> May be 'implementation.' to match with all others above?

Hi Thiago, 

Thanks! I'll make those changes. I'm still working on this actually :) I'll
probably delete this issue and create a new one when I'm happy with it to send
it out for review. I admit I'm lamely using Rietveld as a diff tool at the
moment (and hitting the try servers :) )

[tfarina (gmail-do not use), 2009-10-01 01:19:10]

On 2009/10/01 00:56:35, timsteele wrote:
> On 2009/10/01 00:53:03, tfarina wrote:
> > http://codereview.chromium.org/243054/diff/1/6
> > File chrome/browser/views/confirm_message_box_dialog.h (right):
> > 
> > http://codereview.chromium.org/243054/diff/1/6#newcode13
> > Line 13: #include "views/controls/label.h"
> > Tim could you move this to line 12 instead?
> > 
> > http://codereview.chromium.org/243054/diff/1/6#newcode36
> > Line 36: const std::wstring& window_text);
> > Either |window_title| or change the others |window_title| to |window_text|?
> > 
> > I prefer window_title because it matchs with GetWindowTitle() function.
> > 
> > http://codereview.chromium.org/243054/diff/1/6#newcode51
> > Line 51: // views::View overrides.
> > May be 'implementation.' to match with all others above?
> 
> Hi Thiago, 
> 
> Thanks! I'll make those changes. I'm still working on this actually :) I'll
> probably delete this issue and create a new one when I'm happy with it to send
> it out for review. I admit I'm lamely using Rietveld as a diff tool at the
> moment (and hitting the try servers :) )
Yeah, sometimes I do this too. I like to upload and see the diffs here. It's
very nice and easy to read it through codereview, but I can't send to try
servers :/.

http://codereview.chromium.org/243054/diff/4007/4011
File chrome/browser/views/confirm_message_box_dialog.cc (right):

http://codereview.chromium.org/243054/diff/4007/4011#newcode13
Line 13: #include "grit/locale_settings.h"
nit: please could you move this to line 10, too? :)

[tim (not reviewing), 2009-10-01 01:22:01]

I made the pre-review suggestions from Thiago and think this is ready for review
now.

I have the 40 locale_resource .xtb files ready to throw in to this patch, will
do that shortly.  Just thought I'd let you take a look without the intimidation
factor :)

[Finnur, 2009-10-01 22:56:43]

Who is the "you" here? Not sure why I got added here as an additional reviewer.
Doesn't seem like you need multiple people to review this...

On 2009/10/01 01:22:01, timsteele wrote:
> I made the pre-review suggestions from Thiago and think this is ready for
review
> now.
> 
> I have the 40 locale_resource .xtb files ready to throw in to this patch, will
> do that shortly.  Just thought I'd let you take a look without the
intimidation
> factor :)

[Finnur, 2009-10-02 17:51:28]

Just one question and a few nits.

http://codereview.chromium.org/243054/diff/5001/6005
File chrome/browser/views/confirm_message_box_dialog.h (right):

http://codereview.chromium.org/243054/diff/5001/6005#newcode30
Line 30: // The method causes a modal confirmation dialog to be presented to the
user
nit:
// Presents a modal confirmation dialog to the user...

http://codereview.chromium.org/243054/diff/5001/6005#newcode34
Line 34: // is the callers responsibility to ensure |observer| lives until one
of the
nit: caller's

http://codereview.chromium.org/243054/diff/5001/6005#newcode35
Line 35: // methods are invoked; it can be deleted thereafter from this class'
point
nit: are->is

http://codereview.chromium.org/243054/diff/5001/6003
File chrome/browser/views/options/passwords_page_view.h (right):

http://codereview.chromium.org/243054/diff/5001/6003#newcode165
Line 165: virtual void OnConfirmMessageCancel() {}
Why not just make OnConfirmMessageCencel non-pure virtual, so that derived
classes don't have to override it. I think most people just care about Accept,
some might care about Cancel and can override it if they want.

[tim (not reviewing), 2009-10-02 18:02:34]

thanks! snapshot up.

http://codereview.chromium.org/243054/diff/5001/6005
File chrome/browser/views/confirm_message_box_dialog.h (right):

http://codereview.chromium.org/243054/diff/5001/6005#newcode30
Line 30: // The method causes a modal confirmation dialog to be presented to the
user
On 2009/10/02 17:51:28, Finnur wrote:
> nit:
> // Presents a modal confirmation dialog to the user...

Done.

http://codereview.chromium.org/243054/diff/5001/6005#newcode34
Line 34: // is the callers responsibility to ensure |observer| lives until one
of the
On 2009/10/02 17:51:28, Finnur wrote:
> nit: caller's

Done. I always screw this up, IT'S hopeless.

http://codereview.chromium.org/243054/diff/5001/6005#newcode35
Line 35: // methods are invoked; it can be deleted thereafter from this class'
point
On 2009/10/02 17:51:28, Finnur wrote:
> nit: are->is

Done.

http://codereview.chromium.org/243054/diff/5001/6003
File chrome/browser/views/options/passwords_page_view.h (right):

http://codereview.chromium.org/243054/diff/5001/6003#newcode165
Line 165: virtual void OnConfirmMessageCancel() {}
On 2009/10/02 17:51:28, Finnur wrote:
> Why not just make OnConfirmMessageCencel non-pure virtual, so that derived
> classes don't have to override it. I think most people just care about Accept,
> some might care about Cancel and can override it if they want.

Good point.  Done.

[Finnur, 2009-10-02 18:22:58]

LG, one nit.

http://codereview.chromium.org/243054/diff/5002/8034
File chrome/app/resources/locale_settings_el.xtb (right):

http://codereview.chromium.org/243054/diff/5002/8034#newcode62
Line 62: 
Can you remove these redundant line breaks?