Make repost form warning tab-modal on Mac.

Make repost form warning tab-modal on Mac.

Reloading a page for the second time dismisses the warning before showing it again.

CLs for other platforms forthcoming.

BUG=26271
TEST=RepostFormWarningTest.TestDoubleReload
Manual test: Go to http://www.cs.unc.edu/~jbs/resources/perl/perl-cgi/programs/form1-POST.html, hit Submit, then reload. The warning sheet should be tab-modal, not window-modal. Hit reload again. The warning sheet should close and immediately reopen.

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

[Evan Martin, 2010-03-16 15:26:16]

i skimmed, nico should look at the mac bits

http://codereview.chromium.org/969003/diff/5002/6007
File chrome/browser/repost_form_warning_uitest.cc (right):

http://codereview.chromium.org/969003/diff/5002/6007#newcode30
chrome/browser/repost_form_warning_uitest.cc:30:
HTTPTestServer::CreateServer(kDocRoot, NULL);
indent 4 more spaces here

http://codereview.chromium.org/969003/diff/5002/6009
File chrome/browser/tab_contents/navigation_controller.h (right):

http://codereview.chromium.org/969003/diff/5002/6009#newcode403
chrome/browser/tab_contents/navigation_controller.h:403: void
CancelPendingReload();
comments on these?

http://codereview.chromium.org/969003/diff/5002/6009#newcode563
chrome/browser/tab_contents/navigation_controller.h:563: ReloadType
pending_reload_;
comments on these?

http://codereview.chromium.org/969003/diff/5002/6012
File chrome/test/data/form.html (right):

http://codereview.chromium.org/969003/diff/5002/6012#newcode5
chrome/test/data/form.html:5: <form id="form" action="/moose" method="post">
perhaps put a comment in here (or some text) indicating what the test is for?  I
just wonder if someone found this file later if they'd know why it was here

[darin (slow to review), 2010-03-16 17:09:46]

http://codereview.chromium.org/969003/diff/5002/6005
File chrome/browser/cocoa/repost_form_warning_mac.mm (right):

http://codereview.chromium.org/969003/diff/5002/6005#newcode100
chrome/browser/cocoa/repost_form_warning_mac.mm:100: type ==
NotificationType::RELOADING)) {
is it really necessary to observe RELOADING in addition to LOAD_START? 
requesting a reload should trigger the start of loading.  or, is this to handle
the case where we could already be loading something when the user hits reload?

it would probably be better to observe FRAME_PROVISIONAL_LOAD_START.  if you hit
that, then you know that a load is starting in some frame.  that should be
enough to allow you to auto-dismiss the dialog.

it also seems like this code should be shared by all ports.  it is not good to
have to replicate this kind of logic N times.

[darin (slow to review), 2010-03-16 17:11:09]

related to my review comments above, please note that some reloads will occur as
a result of a script action.  so, it is not just the case of the user clicking
the reload button.  this is why observing FRAME_PROVISIONAL_LOAD_START should
give you a better result.

[Bernhard Bauer, 2010-03-16 17:58:07]

On 2010/03/16 17:09:46, darin wrote:
> http://codereview.chromium.org/969003/diff/5002/6005
> File chrome/browser/cocoa/repost_form_warning_mac.mm (right):
> 
> http://codereview.chromium.org/969003/diff/5002/6005#newcode100
> chrome/browser/cocoa/repost_form_warning_mac.mm:100: type ==
> NotificationType::RELOADING)) {
> is it really necessary to observe RELOADING in addition to LOAD_START? 
> requesting a reload should trigger the start of loading.  or, is this to
handle
> the case where we could already be loading something when the user hits
reload?
> 
> it would probably be better to observe FRAME_PROVISIONAL_LOAD_START.  if you
hit
> that, then you know that a load is starting in some frame.  that should be
> enough to allow you to auto-dismiss the dialog.

FRAME_PROVISIONAL_LOAD_START is only posted if the loading is actually starting.
The repost warning is shown before that.

> it also seems like this code should be shared by all ports.  it is not good to
> have to replicate this kind of logic N times.

I agree, but this could probably be done in a separate CL.

On 2010/03/16 17:11:09, darin wrote:
> related to my review comments above, please note that some reloads will occur
as
> a result of a script action.  so, it is not just the case of the user clicking
> the reload button.  this is why observing FRAME_PROVISIONAL_LOAD_START should
> give you a better result.

AFAICT, reloading from javascript doesn't trigger the warning at all
(intentionally or not).

[Nico, 2010-03-17 16:16:14]

LGTM from me.

Can you close the review of the other version of this patch?

http://codereview.chromium.org/969003/diff/13001/4008
File chrome/browser/repost_form_warning_uitest.cc (right):

http://codereview.chromium.org/969003/diff/13001/4008#newcode72
chrome/browser/repost_form_warning_uitest.cc:72: // 
ASSERT_TRUE(tab->CancelAuth());
Why is this commented out? If it's not needed, remove it completely.

[Bernhard Bauer, 2010-03-17 20:41:23]

On 2010/03/17 16:16:14, Nico wrote:
> LGTM from me.
> 
> Can you close the review of the other version of this patch?

I'm just waiting for a decision which version should be committed, then I'll
close the other one.

http://codereview.chromium.org/969003/diff/13001/4008
File chrome/browser/repost_form_warning_uitest.cc (right):

http://codereview.chromium.org/969003/diff/13001/4008#newcode72
chrome/browser/repost_form_warning_uitest.cc:72: // 
ASSERT_TRUE(tab->CancelAuth());
On 2010/03/17 16:16:14, Nico wrote:
> Why is this commented out? If it's not needed, remove it completely.

You're completely right, I just forgot to remove it.