Add supports that allow Autofill to be initiated from textarea field

Add supports that allow Autofill to be initiated from textarea field

Handling both keyboard and mouse events in textarea that initiate autofill preview and fill form actions. Add test cases from browser tests aspect to cover page click and findformfieldfortextarea cases.

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

[ziran.sun, 2014-03-19 17:29:36]

Textarea should not support autocomplete preview and fill. This is an attempt to
fix Null pointer crash on early submit. Please review. Thanks!

[Ilya Sherman, 2014-03-19 23:47:42]

Please add some test coverage for the changes since your last CL.  Otherwise,
this looks great.  Thanks!

https://codereview.chromium.org/204343004/diff/20001/components/autofill/core...
File components/autofill/core/browser/autocomplete_history_manager.cc (right):

https://codereview.chromium.org/204343004/diff/20001/components/autofill/core...
components/autofill/core/browser/autocomplete_history_manager.cc:96: // Don't
return autocomplete suggestions for textarea.
nit: This comment is redundant with the code; please omit it.

[ziran.sun, 2014-03-20 19:03:41]

Test case added and updated code as per review comments. Please review. Thanks!

https://codereview.chromium.org/204343004/diff/20001/components/autofill/core...
File components/autofill/core/browser/autocomplete_history_manager.cc (right):

https://codereview.chromium.org/204343004/diff/20001/components/autofill/core...
components/autofill/core/browser/autocomplete_history_manager.cc:96: // Don't
return autocomplete suggestions for textarea.
On 2014/03/19 23:47:42, Ilya Sherman wrote:
> nit: This comment is redundant with the code; please omit it.

Done.

[Ilya Sherman, 2014-03-20 22:22:03]

https://codereview.chromium.org/204343004/diff/40001/components/autofill/core...
File components/autofill/core/browser/autocomplete_history_manager_unittest.cc
(right):

https://codereview.chromium.org/204343004/diff/40001/components/autofill/core...
components/autofill/core/browser/autocomplete_history_manager_unittest.cc:228:
// Verify that no autocomplete suggestion is returned for textarea.
Hmm, where does the test verify that no suggestions were returned?  Am I just
overlooking something?

As a general thought: One good way to verify that a test is testing what you
intend for it to test is to temporarily revert the changes that are meant to fix
the test, and then run the test.  If it fails without the changes, that's good. 
If it still passes without the changes, then it's not really testing what you
mean for it to be testing.

https://codereview.chromium.org/204343004/diff/40001/components/autofill/core...
components/autofill/core/browser/autocomplete_history_manager_unittest.cc:229:
TEST_F(AutocompleteHistoryManagerTest, TextArea) {
nit: Please name this test "NoAutocompleteSuggestionsForTextarea".

[ziran.sun, 2014-03-21 12:00:21]

Addressed comments. Any chance for a short discussion on IRC?

https://codereview.chromium.org/204343004/diff/40001/components/autofill/core...
File components/autofill/core/browser/autocomplete_history_manager_unittest.cc
(right):

https://codereview.chromium.org/204343004/diff/40001/components/autofill/core...
components/autofill/core/browser/autocomplete_history_manager_unittest.cc:228:
// Verify that no autocomplete suggestion is returned for textarea.
On 2014/03/20 22:22:03, Ilya Sherman wrote:
> Hmm, where does the test verify that no suggestions were returned?  Am I just
> overlooking something?
> 
> As a general thought: One good way to verify that a test is testing what you
> intend for it to test is to temporarily revert the changes that are meant to
fix
> the test, and then run the test.  If it fails without the changes, that's
good. 
> If it still passes without the changes, then it's not really testing what you
> mean for it to be testing.

It's actually testing if autocomplete_history_manager.SendSuggestions(NULL) is
called for textarea. This will result in a call on
OnSuggestionReturned(_,_,_,_,_) with empty results in external_delegate.

https://codereview.chromium.org/204343004/diff/40001/components/autofill/core...
components/autofill/core/browser/autocomplete_history_manager_unittest.cc:229:
TEST_F(AutocompleteHistoryManagerTest, TextArea) {
On 2014/03/20 22:22:03, Ilya Sherman wrote:
> nit: Please name this test "NoAutocompleteSuggestionsForTextarea".

Done.

[Ilya Sherman, 2014-03-21 23:01:11]

https://codereview.chromium.org/204343004/diff/40001/components/autofill/core...
File components/autofill/core/browser/autocomplete_history_manager_unittest.cc
(right):

https://codereview.chromium.org/204343004/diff/40001/components/autofill/core...
components/autofill/core/browser/autocomplete_history_manager_unittest.cc:228:
// Verify that no autocomplete suggestion is returned for textarea.
On 2014/03/21 12:00:22, ziran.sun wrote:
> On 2014/03/20 22:22:03, Ilya Sherman wrote:
> > Hmm, where does the test verify that no suggestions were returned?  Am I
just
> > overlooking something?
> > 
> > As a general thought: One good way to verify that a test is testing what you
> > intend for it to test is to temporarily revert the changes that are meant to
> fix
> > the test, and then run the test.  If it fails without the changes, that's
> good. 
> > If it still passes without the changes, then it's not really testing what
you
> > mean for it to be testing.
> 
> It's actually testing if autocomplete_history_manager.SendSuggestions(NULL) is
> called for textarea. This will result in a call on
> OnSuggestionReturned(_,_,_,_,_) with empty results in external_delegate.

IMO the test would make more sense if the mock expectation were set to

  EXPECT_CALL(external_delegate,
              OnSuggestionsReturned(0,
                                    std::vector<base::string16>(),
                                    std::vector<base::string16>(),
                                    std::vector<base::string16>(),
                                    std::vector<int>()));

That way, the test will clearly fail if the returned values are non-empty, which
is what you're trying to test.  Currently, the test is just verifying that
OnSuggestionsReturned() is called at all... but it could be called with
non-empty values just as easily as with empty ones.

[ziran.sun, 2014-03-24 15:24:48]

Test code updated. Please review. Thanks!

https://codereview.chromium.org/204343004/diff/40001/components/autofill/core...
File components/autofill/core/browser/autocomplete_history_manager_unittest.cc
(right):

https://codereview.chromium.org/204343004/diff/40001/components/autofill/core...
components/autofill/core/browser/autocomplete_history_manager_unittest.cc:228:
// Verify that no autocomplete suggestion is returned for textarea.
On 2014/03/21 23:01:11, Ilya Sherman wrote:
> On 2014/03/21 12:00:22, ziran.sun wrote:
> > On 2014/03/20 22:22:03, Ilya Sherman wrote:
> > > Hmm, where does the test verify that no suggestions were returned?  Am I
> just
> > > overlooking something?
> > > 
> > > As a general thought: One good way to verify that a test is testing what
you
> > > intend for it to test is to temporarily revert the changes that are meant
to
> > fix
> > > the test, and then run the test.  If it fails without the changes, that's
> > good. 
> > > If it still passes without the changes, then it's not really testing what
> you
> > > mean for it to be testing.
> > 
> > It's actually testing if autocomplete_history_manager.SendSuggestions(NULL)
is
> > called for textarea. This will result in a call on
> > OnSuggestionReturned(_,_,_,_,_) with empty results in external_delegate.
> 
> IMO the test would make more sense if the mock expectation were set to
> 
>   EXPECT_CALL(external_delegate,
>               OnSuggestionsReturned(0,
>                                     std::vector<base::string16>(),
>                                     std::vector<base::string16>(),
>                                     std::vector<base::string16>(),
>                                     std::vector<int>()));
> 
> That way, the test will clearly fail if the returned values are non-empty,
which
> is what you're trying to test.  Currently, the test is just verifying that
> OnSuggestionsReturned() is called at all... but it could be called with
> non-empty values just as easily as with empty ones.

Yes, this certainly makes sense. Just updated code as you suggested. Thanks very
much!

[Ilya Sherman, 2014-03-24 17:43:48]

LGTM.  Thanks, Ziran :)

[Ilya Sherman, 2014-03-24 17:43:53]

The CQ bit was checked by isherman@chromium.org

[commit-bot: I haz the power, 2014-03-24 17:44:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ziran.sun@samsung.com/204343004/80001

[commit-bot: I haz the power, 2014-03-24 19:19:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-24 19:19:11]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_rel

[Ilya Sherman, 2014-03-24 19:20:07]

The CQ bit was checked by isherman@chromium.org

[commit-bot: I haz the power, 2014-03-24 19:20:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ziran.sun@samsung.com/204343004/80001

[Ilya Sherman, 2014-03-24 19:24:23]

The CQ bit was unchecked by isherman@chromium.org

[Ilya Sherman, 2014-03-24 19:24:24]

The CQ bit was checked by isherman@chromium.org

[commit-bot: I haz the power, 2014-03-25 01:15:19]

Change committed as 259076