[Mac] Implement form resubmission warning dialog

[Mac] Implement form resubmission warning dialog

Surprisingly, this is not a tab-modal sheet on linux and windows, so it's window-modal on os x as well for now.

BUG=23526
TEST=Go to http://www.cs.unc.edu/~jbs/resources/perl/perl-cgi/programs/form1-POST.html , click "Do it!", hit reload. Window sheet should come up. Hitting cancel should cancel the navigation (and hitting reload again should bring up sheet again). Hitting "Resend" should trigger reload.

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

[Nico, 2009-10-23 06:04:16]

I don't get what the observer is good for, as the window can't be used anyway
while the sheet is up? Looks like the dialog is window-modal on windows and gtk
too. I added it since linux and windows have it too, but what is it for?

[brettw, 2009-10-23 17:00:38]

Cross-platformy bits look good to me.

I don't remember the specifics, but I think this needs to be a window-modal
dialog due to some nasty edge cases.

The dialog will run a nested message loop, so it may be possible for the page to
request the tab to be closed or to navigate somewhere else, which I think is
what the observer is for.

[Scott Hess - ex-Googler, 2009-10-23 17:16:44]

http://codereview.chromium.org/327009/diff/2001/2003
File chrome/browser/cocoa/repost_form_warning_mac.h (right):

http://codereview.chromium.org/327009/diff/2001/2003#newcode50
Line 50: NSAlert* alert_;
I don't understand why this isn't scoped_nsobject.  I can see the manual
clearing in Destroy(), but that could as easily do reset().

http://codereview.chromium.org/327009/diff/2001/2004
File chrome/browser/cocoa/repost_form_warning_mac.mm (right):

http://codereview.chromium.org/327009/diff/2001/2004#newcode82
Line 82: Destroy();
My understanding is that this code path is the one that requires Destroy() to
call -orderOut:?  Does it also need to end the sheet started in the constructor?

I think you could replace the Destroy() with:
  [NSApp endSheet:[alert_ window] returnCode:NSAlertSecondButtonReturn];
which would send control through Cancel().

http://codereview.chromium.org/327009/diff/2001/2004#newcode91
Line 91: MessageLoop::current()->DeleteSoon(FROM_HERE, this);
I do not like fixing ownership-versus-delete issues using delayed deletes.  They
often seem to bite us later.  With the suggested change to Observe(), and if you
had your destructor unregister notifications added in the constructor, I think
that you could call "delete this" from Conform() and Cancel().

[Nico, 2009-10-27 20:18:49]

Thanks for the thoughtful comments. Took me a while to get to them, sorry.

http://codereview.chromium.org/327009/diff/2001/2003
File chrome/browser/cocoa/repost_form_warning_mac.h (right):

http://codereview.chromium.org/327009/diff/2001/2003#newcode50
Line 50: NSAlert* alert_;
On 2009/10/23 17:16:44, shess wrote:
> I don't understand why this isn't scoped_nsobject.  I can see the manual
> clearing in Destroy(), but that could as easily do reset().

Done.

http://codereview.chromium.org/327009/diff/2001/2004
File chrome/browser/cocoa/repost_form_warning_mac.mm (right):

http://codereview.chromium.org/327009/diff/2001/2004#newcode82
Line 82: Destroy();
On 2009/10/23 17:16:44, shess wrote:
> My understanding is that this code path is the one that requires Destroy() to
> call -orderOut:?  Does it also need to end the sheet started in the
constructor?
> 
> I think you could replace the Destroy() with:
>   [NSApp endSheet:[alert_ window] returnCode:NSAlertSecondButtonReturn];
> which would send control through Cancel().
> 

Good idea. Done.

http://codereview.chromium.org/327009/diff/2001/2004#newcode91
Line 91: MessageLoop::current()->DeleteSoon(FROM_HERE, this);
On 2009/10/23 17:16:44, shess wrote:
> I do not like fixing ownership-versus-delete issues using delayed deletes. 
They
> often seem to bite us later.  With the suggested change to Observe(), and if
you
> had your destructor unregister notifications added in the constructor, I think
> that you could call "delete this" from Conform() and Cancel().

I agree in theory, but I'd prefer to keep this similar to the windows and linux
versions. I can say from experience that doing changes on all 3 platforms is
much easier when the code on all platforms is similar.

[Nico, 2009-10-29 22:22:50]

Ping.

[Scott Hess - ex-Googler, 2009-10-30 00:51:45]

lgtm

[pink (ping after 24hrs), 2009-11-02 17:36:18]

Please fix and comment these things in a subsequent CL.

http://codereview.chromium.org/327009/diff/10001/11002
File chrome/browser/cocoa/repost_form_warning_mac.h (right):

http://codereview.chromium.org/327009/diff/10001/11002#newcode16
Line 16: @interface RepostDelegate : NSObject {
class needs a comment. Does this really need to be public? Can't you just say
@class RepostDelegate and do the declaration in the .mm file?

http://codereview.chromium.org/327009/diff/10001/11002#newcode25
Line 25: class RepostFormWarningMac : public NotificationObserver {
class needs a comment. Especially needs to mention how it cleans itself up since
it appears from the use that it will leak itself and handle its own cleanup.

[Nico, 2009-11-02 18:05:16]

Follow-up in http://codereview.chromium.org/354009

http://codereview.chromium.org/327009/diff/10001/11002
File chrome/browser/cocoa/repost_form_warning_mac.h (right):

http://codereview.chromium.org/327009/diff/10001/11002#newcode16
Line 16: @interface RepostDelegate : NSObject {
On 2009/11/02 17:36:19, pink wrote:
> class needs a comment. Does this really need to be public? Can't you just say
> @class RepostDelegate and do the declaration in the .mm file?

Done.

http://codereview.chromium.org/327009/diff/10001/11002#newcode25
Line 25: class RepostFormWarningMac : public NotificationObserver {
On 2009/11/02 17:36:19, pink wrote:
> class needs a comment. Especially needs to mention how it cleans itself up
since
> it appears from the use that it will leak itself and handle its own cleanup.

Done.