Implement 'invalid' AutocompleteErrorEvent#reason

Implement 'invalid' AutocompleteErrorEvent#reason

R=isherman@chromium.org
BUG=168967
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=207791

[Dan Beam, 2013-06-19 00:21:48]

isherman@: where would I test this?

[Dan Beam, 2013-06-19 00:22:01]

depends on https://codereview.chromium.org/17076013/, btw

[Dan Beam, 2013-06-19 08:38:53]

so i wrote a test and it *almost* works, but i need to figure out which event
loop to run or something.  see question below:

https://codereview.chromium.org/17391012/diff/11001/chrome/browser/ui/autofil...
File chrome/browser/ui/autofill/autofill_dialog_controller_browsertest.cc
(right):

https://codereview.chromium.org/17391012/diff/11001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_controller_browsertest.cc:665: //
TODO(dbeam): do some magic here to make `onautocompleteerror` fire.
estade@/isherman@: if you know what this magic is, it'd really really help...

[Dan Beam, 2013-06-19 22:33:19]

okie dokie, tests fixed, ptal

[Ilya Sherman, 2013-06-19 23:56:14]

https://chromiumcodereview.appspot.com/17391012/diff/24001/chrome/browser/ui/...
File chrome/browser/ui/autofill/autofill_dialog_controller_browsertest.cc
(right):

https://chromiumcodereview.appspot.com/17391012/diff/24001/chrome/browser/ui/...
chrome/browser/ui/autofill/autofill_dialog_controller_browsertest.cc:212:
AutofillDialogControllerImpl* SetupHtmlAndInvoke(
nit: "Setup" -> "SetUp"

https://chromiumcodereview.appspot.com/17391012/diff/24001/chrome/browser/ui/...
chrome/browser/ui/autofill/autofill_dialog_controller_browsertest.cc:213: const
std::string& form_inner_html) {
nit: Docs.

https://chromiumcodereview.appspot.com/17391012/diff/24001/chrome/browser/ui/...
chrome/browser/ui/autofill/autofill_dialog_controller_browsertest.cc:224: "}"
nit: Does this not require a semicolon?

https://chromiumcodereview.appspot.com/17391012/diff/24001/chrome/browser/ui/...
chrome/browser/ui/autofill/autofill_dialog_controller_browsertest.cc:251: void
ExpectDomMessage(const std::string& expected) {
nit: Worth adding a SCOPED_TRACE(expected)?

https://chromiumcodereview.appspot.com/17391012/diff/24001/chrome/browser/ui/...
chrome/browser/ui/autofill/autofill_dialog_controller_browsertest.cc:251: void
ExpectDomMessage(const std::string& expected) {
nit: Docs.

https://chromiumcodereview.appspot.com/17391012/diff/24001/chrome/browser/ui/...
chrome/browser/ui/autofill/autofill_dialog_controller_browsertest.cc:255:
EXPECT_EQ(std::string("\"").append(expected).append("\""), message);
nit: Why not just "\"" + expected + "\""?

https://chromiumcodereview.appspot.com/17391012/diff/24001/chrome/browser/ui/...
chrome/browser/ui/autofill/autofill_dialog_controller_browsertest.cc:640:
content::RunAllPendingInMessageLoop(content::BrowserThread::DB);
nit: Would be nice to decompose lines 631-640 into a helper method.

https://chromiumcodereview.appspot.com/17391012/diff/24001/chrome/browser/ui/...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.h (right):

https://chromiumcodereview.appspot.com/17391012/diff/24001/chrome/browser/ui/...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.h:295:
AutocompleteErrorEventReasonCancel);
Why do we need to friend the tests?  IMO it's much better to expose subsets of
the private functionality to tests, rather than letting tests muck with
internals arbitrarily.

https://chromiumcodereview.appspot.com/17391012/diff/24001/chrome/browser/ui/...
File chrome/browser/ui/autofill/tab_autofill_manager_delegate.h (right):

https://chromiumcodereview.appspot.com/17391012/diff/24001/chrome/browser/ui/...
chrome/browser/ui/autofill/tab_autofill_manager_delegate.h:81: }
nit: If this is just for testing, please make this method have protected
visibility.

[Dan Beam, 2013-06-20 00:26:57]

https://chromiumcodereview.appspot.com/17391012/diff/24001/chrome/browser/ui/...
File chrome/browser/ui/autofill/autofill_dialog_controller_browsertest.cc
(right):

https://chromiumcodereview.appspot.com/17391012/diff/24001/chrome/browser/ui/...
chrome/browser/ui/autofill/autofill_dialog_controller_browsertest.cc:212:
AutofillDialogControllerImpl* SetupHtmlAndInvoke(
On 2013/06/19 23:56:14, Ilya Sherman wrote:
> nit: "Setup" -> "SetUp"

Done.

https://chromiumcodereview.appspot.com/17391012/diff/24001/chrome/browser/ui/...
chrome/browser/ui/autofill/autofill_dialog_controller_browsertest.cc:224: "}"
On 2013/06/19 23:56:14, Ilya Sherman wrote:
> nit: Does this not require a semicolon?

no, function declarations (function name() {}) do not whereas function
expressions (var a = function() {};) do.

https://chromiumcodereview.appspot.com/17391012/diff/24001/chrome/browser/ui/...
chrome/browser/ui/autofill/autofill_dialog_controller_browsertest.cc:251: void
ExpectDomMessage(const std::string& expected) {
On 2013/06/19 23:56:14, Ilya Sherman wrote:
> nit: Docs.

Done.

https://chromiumcodereview.appspot.com/17391012/diff/24001/chrome/browser/ui/...
chrome/browser/ui/autofill/autofill_dialog_controller_browsertest.cc:255:
EXPECT_EQ(std::string("\"").append(expected).append("\""), message);
On 2013/06/19 23:56:14, Ilya Sherman wrote:
> nit: Why not just "\"" + expected + "\""?

Done.

https://chromiumcodereview.appspot.com/17391012/diff/24001/chrome/browser/ui/...
chrome/browser/ui/autofill/autofill_dialog_controller_browsertest.cc:640:
content::RunAllPendingInMessageLoop(content::BrowserThread::DB);
On 2013/06/19 23:56:14, Ilya Sherman wrote:
> nit: Would be nice to decompose lines 631-640 into a helper method.

Done.

https://chromiumcodereview.appspot.com/17391012/diff/24001/chrome/browser/ui/...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.h (right):

https://chromiumcodereview.appspot.com/17391012/diff/24001/chrome/browser/ui/...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.h:295:
AutocompleteErrorEventReasonCancel);
On 2013/06/19 23:56:14, Ilya Sherman wrote:
> Why do we need to friend the tests?  IMO it's much better to expose subsets of
> the private functionality to tests, rather than letting tests muck with
> internals arbitrarily.

AutofillDialogControllerImpl::view() (not TestAutofillDialogController)

https://chromiumcodereview.appspot.com/17391012/diff/24001/chrome/browser/ui/...
File chrome/browser/ui/autofill/tab_autofill_manager_delegate.h (right):

https://chromiumcodereview.appspot.com/17391012/diff/24001/chrome/browser/ui/...
chrome/browser/ui/autofill/tab_autofill_manager_delegate.h:81: }
On 2013/06/19 23:56:14, Ilya Sherman wrote:
> nit: If this is just for testing, please make this method have protected
> visibility.

the test interacts with the public interface and doesn't derive this class as
it's operating solely on the real thing...

[Dan Beam, 2013-06-20 00:27:19]

https://chromiumcodereview.appspot.com/17391012/diff/24001/chrome/browser/ui/...
File chrome/browser/ui/autofill/autofill_dialog_controller_browsertest.cc
(right):

https://chromiumcodereview.appspot.com/17391012/diff/24001/chrome/browser/ui/...
chrome/browser/ui/autofill/autofill_dialog_controller_browsertest.cc:251: void
ExpectDomMessage(const std::string& expected) {
On 2013/06/19 23:56:14, Ilya Sherman wrote:
> nit: Worth adding a SCOPED_TRACE(expected)?

what does this do?

[Ilya Sherman, 2013-06-20 01:11:27]

https://chromiumcodereview.appspot.com/17391012/diff/24001/chrome/browser/ui/...
File chrome/browser/ui/autofill/autofill_dialog_controller_browsertest.cc
(right):

https://chromiumcodereview.appspot.com/17391012/diff/24001/chrome/browser/ui/...
chrome/browser/ui/autofill/autofill_dialog_controller_browsertest.cc:251: void
ExpectDomMessage(const std::string& expected) {
On 2013/06/20 00:27:19, Dan Beam wrote:
> On 2013/06/19 23:56:14, Ilya Sherman wrote:
> > nit: Worth adding a SCOPED_TRACE(expected)?
> 
> what does this do?

Nevermind, it's not helpful for this test.

https://chromiumcodereview.appspot.com/17391012/diff/24001/chrome/browser/ui/...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.h (right):

https://chromiumcodereview.appspot.com/17391012/diff/24001/chrome/browser/ui/...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.h:295:
AutocompleteErrorEventReasonCancel);
On 2013/06/20 00:26:57, Dan Beam wrote:
> On 2013/06/19 23:56:14, Ilya Sherman wrote:
> > Why do we need to friend the tests?  IMO it's much better to expose subsets
of
> > the private functionality to tests, rather than letting tests muck with
> > internals arbitrarily.
> 
> AutofillDialogControllerImpl::view() (not TestAutofillDialogController)

I'd much rather expose a GetTestableView() method publicly than add friend
tests.

https://chromiumcodereview.appspot.com/17391012/diff/24001/chrome/browser/ui/...
File chrome/browser/ui/autofill/tab_autofill_manager_delegate.h (right):

https://chromiumcodereview.appspot.com/17391012/diff/24001/chrome/browser/ui/...
chrome/browser/ui/autofill/tab_autofill_manager_delegate.h:81: }
On 2013/06/20 00:26:57, Dan Beam wrote:
> On 2013/06/19 23:56:14, Ilya Sherman wrote:
> > nit: If this is just for testing, please make this method have protected
> > visibility.
> 
> the test interacts with the public interface and doesn't derive this class as
> it's operating solely on the real thing...

Seems like you should be able to use an instance of a test-specific class rather
than the real thing.

https://chromiumcodereview.appspot.com/17391012/diff/26001/chrome/browser/ui/...
File chrome/browser/ui/autofill/autofill_dialog_controller_browsertest.cc
(right):

https://chromiumcodereview.appspot.com/17391012/diff/26001/chrome/browser/ui/...
chrome/browser/ui/autofill/autofill_dialog_controller_browsertest.cc:215: //
Returns the |AutofillDialogControllerImpl| that created by this invocation.
nit: "that created" -> "created" or "that was created"

[Dan Beam, 2013-06-20 01:47:09]

https://chromiumcodereview.appspot.com/17391012/diff/24001/chrome/browser/ui/...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.h (right):

https://chromiumcodereview.appspot.com/17391012/diff/24001/chrome/browser/ui/...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.h:295:
AutocompleteErrorEventReasonCancel);
On 2013/06/20 01:11:27, Ilya Sherman wrote:
> On 2013/06/20 00:26:57, Dan Beam wrote:
> > On 2013/06/19 23:56:14, Ilya Sherman wrote:
> > > Why do we need to friend the tests?  IMO it's much better to expose
subsets
> of
> > > the private functionality to tests, rather than letting tests muck with
> > > internals arbitrarily.
> > 
> > AutofillDialogControllerImpl::view() (not TestAutofillDialogController)
> 
> I'd much rather expose a GetTestableView() method publicly than add friend
> tests.

Done.

https://chromiumcodereview.appspot.com/17391012/diff/24001/chrome/browser/ui/...
File chrome/browser/ui/autofill/tab_autofill_manager_delegate.h (right):

https://chromiumcodereview.appspot.com/17391012/diff/24001/chrome/browser/ui/...
chrome/browser/ui/autofill/tab_autofill_manager_delegate.h:81: }
On 2013/06/20 01:11:27, Ilya Sherman wrote:
> On 2013/06/20 00:26:57, Dan Beam wrote:
> > On 2013/06/19 23:56:14, Ilya Sherman wrote:
> > > nit: If this is just for testing, please make this method have protected
> > > visibility.
> > 
> > the test interacts with the public interface and doesn't derive this class
as
> > it's operating solely on the real thing...
> 
> Seems like you should be able to use an instance of a test-specific class
rather
> than the real thing.

we talked about this and landed on GetDialogControllerForTesting() for now

https://chromiumcodereview.appspot.com/17391012/diff/26001/chrome/browser/ui/...
File chrome/browser/ui/autofill/autofill_dialog_controller_browsertest.cc
(right):

https://chromiumcodereview.appspot.com/17391012/diff/26001/chrome/browser/ui/...
chrome/browser/ui/autofill/autofill_dialog_controller_browsertest.cc:215: //
Returns the |AutofillDialogControllerImpl| that created by this invocation.
On 2013/06/20 01:11:27, Ilya Sherman wrote:
> nit: "that created" -> "created" or "that was created"

Done.

[Dan Beam, 2013-06-20 01:47:23]

+estade@ for chrome/browser/ui/views/autofill/autofill_dialog_views

[Dan Beam, 2013-06-20 02:14:00]

estade@: also see the interface change in
chrome/browser/ui/autofill/testable_autofill_dialog_view.h

[Evan Stade, 2013-06-20 03:15:52]

https://codereview.chromium.org/17391012/diff/33001/chrome/browser/ui/views/a...
File chrome/browser/ui/views/autofill/autofill_dialog_views.cc (right):

https://codereview.chromium.org/17391012/diff/33001/chrome/browser/ui/views/a...
chrome/browser/ui/views/autofill/autofill_dialog_views.cc:1155: void
AutofillDialogViews::SetCvc(const base::string16& cvc) {
this should be generic:

SetTextContentsOfSuggestionInput(DialogSection, string16)

(also please put it after the oter SetTextContents fns)

[Ilya Sherman, 2013-06-20 03:56:02]

LGTM, thanks.

[Dan Beam, 2013-06-20 16:22:53]

https://codereview.chromium.org/17391012/diff/33001/chrome/browser/ui/views/a...
File chrome/browser/ui/views/autofill/autofill_dialog_views.cc (right):

https://codereview.chromium.org/17391012/diff/33001/chrome/browser/ui/views/a...
chrome/browser/ui/views/autofill/autofill_dialog_views.cc:1155: void
AutofillDialogViews::SetCvc(const base::string16& cvc) {
On 2013/06/20 03:15:53, Evan Stade wrote:
> this should be generic:

does that mean we should eventually change GetCvc() to also be generic?

> 
> SetTextContentsOfSuggestionInput(DialogSection, string16)
> 
> (also please put it after the oter SetTextContents fns)

Done.

[Evan Stade, 2013-06-20 21:56:24]

lgtm

https://codereview.chromium.org/17391012/diff/33001/chrome/browser/ui/views/a...
File chrome/browser/ui/views/autofill/autofill_dialog_views.cc (right):

https://codereview.chromium.org/17391012/diff/33001/chrome/browser/ui/views/a...
chrome/browser/ui/views/autofill/autofill_dialog_views.cc:1155: void
AutofillDialogViews::SetCvc(const base::string16& cvc) {
On 2013/06/20 16:22:53, Dan Beam wrote:
> On 2013/06/20 03:15:53, Evan Stade wrote:
> > this should be generic:
> 
> does that mean we should eventually change GetCvc() to also be generic?

maybe


> 
> > 
> > SetTextContentsOfSuggestionInput(DialogSection, string16)
> > 
> > (also please put it after the oter SetTextContents fns)
> 
> Done.

https://codereview.chromium.org/17391012/diff/38001/chrome/browser/ui/views/a...
File chrome/browser/ui/views/autofill/autofill_dialog_views.cc (right):

https://codereview.chromium.org/17391012/diff/38001/chrome/browser/ui/views/a...
chrome/browser/ui/views/autofill/autofill_dialog_views.cc:1151:
textfield()->SetText(text);
(this will have a minor conflict with a recent patch I committed)

[commit-bot: I haz the power, 2013-06-21 02:23:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dbeam@chromium.org/17391012/62009

[commit-bot: I haz the power, 2013-06-21 10:25:37]

Change committed as 207791