Disable submit button briefly when launching rAc dialog

Disable submit button briefly when launching rAc dialog

A malicious site could create content that could pop up the rAc dialog and get
the user to accidentally click submit. This CL disables the submit for a brief
time (1s), so such clicks wouldn't trigger the submit.

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

[please use gerrit instead, 2013-09-04 00:03:34]

Evan: PTAL.

[Evan Stade, 2013-09-04 02:36:28]

https://codereview.chromium.org/23717029/diff/19001/chrome/browser/ui/autofil...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://codereview.chromium.org/23717029/diff/19001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:2243:
is_submit_button_delayed_ = true;
I think this will be slightly cleaner if you use a Timer

[please use gerrit instead, 2013-09-04 02:51:46]

https://codereview.chromium.org/23717029/diff/19001/chrome/browser/ui/autofil...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://codereview.chromium.org/23717029/diff/19001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:2243:
is_submit_button_delayed_ = true;
On 2013/09/04 02:36:28, Evan Stade wrote:
> I think this will be slightly cleaner if you use a Timer

I can see using a Timer instead of PostDelayedTask, but you seem to have
commented on the is_submit_button_delayed variable. Was that intentional?

[Evan Stade, 2013-09-04 18:24:29]

https://codereview.chromium.org/23717029/diff/19001/chrome/browser/ui/autofil...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://codereview.chromium.org/23717029/diff/19001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:2243:
is_submit_button_delayed_ = true;
On 2013/09/04 02:51:46, Rouslan Solomakhin wrote:
> On 2013/09/04 02:36:28, Evan Stade wrote:
> > I think this will be slightly cleaner if you use a Timer
> 
> I can see using a Timer instead of PostDelayedTask, but you seem to have
> commented on the is_submit_button_delayed variable. Was that intentional?

just because it was the first line of this fn (also you wouldn't need to have
this variable any more)

[please use gerrit instead, 2013-09-04 21:01:13]

Evan: PTAL.

[Evan Stade, 2013-09-04 21:42:52]

https://codereview.chromium.org/23717029/diff/27001/chrome/browser/ui/autofil...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://codereview.chromium.org/23717029/diff/27001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:660:
SubmitButtonDelayBegin(kSubmitButtonDelayMs);
I think you should do this before view_->Show()

https://codereview.chromium.org/23717029/diff/27001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:2262: if
(submit_buton_delay_timer_.IsRunning())
I dislike this because it shouldn't happen outside of testing, and that's not
obvious here.

https://codereview.chromium.org/23717029/diff/27001/chrome/browser/ui/autofil...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.h (right):

https://codereview.chromium.org/23717029/diff/27001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.h:292: virtual void
SubmitButtonDelayBegin(int delay_ms);
nit: this should be TimeDelta rather than int

https://codereview.chromium.org/23717029/diff/27001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.h:713:
base::OneShotTimer<AutofillDialogControllerImpl> submit_buton_delay_timer_;
nit: s/button/button

https://codereview.chromium.org/23717029/diff/27001/chrome/browser/ui/autofil...
File chrome/browser/ui/autofill/autofill_dialog_controller_unittest.cc (right):

https://codereview.chromium.org/23717029/diff/27001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_controller_unittest.cc:334: virtual
void SubmitButtonDelayBegin(int delay_ms) OVERRIDE {
You don't verify that production code ever calls this function. I think you
should have a way to make this pass through to the super's impl (just in the
case of the new test), then can you verify that the submit button is disabled
when the view gets Show().

https://codereview.chromium.org/23717029/diff/27001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_controller_unittest.cc:2417:
SwitchToAutofill();
do you need this?

[please use gerrit instead, 2013-09-05 02:30:58]

Evan: PTAL.

https://codereview.chromium.org/23717029/diff/27001/chrome/browser/ui/autofil...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://codereview.chromium.org/23717029/diff/27001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:660:
SubmitButtonDelayBegin(kSubmitButtonDelayMs);
On 2013/09/04 21:42:52, Evan Stade wrote:
> I think you should do this before view_->Show()

Moved up before view_->Show().

https://codereview.chromium.org/23717029/diff/27001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:2262: if
(submit_buton_delay_timer_.IsRunning())
On 2013/09/04 21:42:52, Evan Stade wrote:
> I dislike this because it shouldn't happen outside of testing, and that's not
> obvious here.

Removed the timer check from OnSubmitButtonDelayEnd(). Added a method
SubmitButtonDelayEndForTesting() that stops the timer and then calls its
OnSubmitButtonDelayEnd() callback.

https://codereview.chromium.org/23717029/diff/27001/chrome/browser/ui/autofil...
File chrome/browser/ui/autofill/autofill_dialog_controller_unittest.cc (right):

https://codereview.chromium.org/23717029/diff/27001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_controller_unittest.cc:334: virtual
void SubmitButtonDelayBegin(int delay_ms) OVERRIDE {
On 2013/09/04 21:42:52, Evan Stade wrote:
> You don't verify that production code ever calls this function. I think you
> should have a way to make this pass through to the super's impl (just in the
> case of the new test), then can you verify that the submit button is disabled
> when the view gets Show().

I've overridden SubmitButtonDelayBegin() to increment a counter in testing and
added a test that verifies that this counter is equal to 1 after the dialog has
been shown.

https://codereview.chromium.org/23717029/diff/27001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_controller_unittest.cc:2417:
SwitchToAutofill();
On 2013/09/04 21:42:52, Evan Stade wrote:
> do you need this?

SwitchToAutofill() was to get rid of the wallet's loading spinner, which also
blocks the submit button. I've split this test case into three test cases for
clarity and increased coverage:

1) Wallet loading finishes before the delay timer.
2) Wallet loading finishes after the delay timer.
3) Wallet loading is ignored in autofill mode.

[Evan Stade, 2013-09-05 16:29:43]

https://codereview.chromium.org/23717029/diff/67001/chrome/browser/ui/autofil...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.h (right):

https://codereview.chromium.org/23717029/diff/67001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.h:291: void
OnSubmitButtonDelayEnd();
this can be private now right?

https://codereview.chromium.org/23717029/diff/67001/chrome/browser/ui/autofil...
File chrome/browser/ui/autofill/autofill_dialog_controller_unittest.cc (right):

https://codereview.chromium.org/23717029/diff/67001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_controller_unittest.cc:283: // 60
second delay to avoid test flakiness. The test would be flaky if this
I don't think this is necessary. The test executes synchronously so any delay
(even 0ms) would be enough to ensure that the callback doesn't run till after
the test is done.

https://codereview.chromium.org/23717029/diff/67001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_controller_unittest.cc:2412:
EXPECT_EQ(1, controller()->get_submit_button_delay_number());
nit: instead of having a separate test for this, add this line to the start of
each of the other 3 tests.

[please use gerrit instead, 2013-09-05 18:40:49]

Evan: PTAL.

https://codereview.chromium.org/23717029/diff/67001/chrome/browser/ui/autofil...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.h (right):

https://codereview.chromium.org/23717029/diff/67001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.h:291: void
OnSubmitButtonDelayEnd();
On 2013/09/05 16:29:44, Evan Stade wrote:
> this can be private now right?

Moved to private.

https://codereview.chromium.org/23717029/diff/67001/chrome/browser/ui/autofil...
File chrome/browser/ui/autofill/autofill_dialog_controller_unittest.cc (right):

https://codereview.chromium.org/23717029/diff/67001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_controller_unittest.cc:283: // 60
second delay to avoid test flakiness. The test would be flaky if this
On 2013/09/05 16:29:44, Evan Stade wrote:
> I don't think this is necessary. The test executes synchronously so any delay
> (even 0ms) would be enough to ensure that the callback doesn't run till after
> the test is done.

Using the same delay in production and in testing.

https://codereview.chromium.org/23717029/diff/67001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_controller_unittest.cc:2412:
EXPECT_EQ(1, controller()->get_submit_button_delay_number());
On 2013/09/05 16:29:44, Evan Stade wrote:
> nit: instead of having a separate test for this, add this line to the start of
> each of the other 3 tests.

Done.

[Evan Stade, 2013-09-06 01:06:41]

lgtm

https://codereview.chromium.org/23717029/diff/73001/chrome/browser/ui/autofil...
File chrome/browser/ui/autofill/autofill_dialog_controller_unittest.cc (right):

https://codereview.chromium.org/23717029/diff/73001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_controller_unittest.cc:345: int
submit_button_delay_number_;
nit: submit_button_delay_count_;

[commit-bot: I haz the power, 2013-09-06 01:19:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rouslan@chromium.org/23717029/91001

[commit-bot: I haz the power, 2013-09-06 06:11:54]

Change committed as 221598