Do not set autofill property for datalist options

Do not set autofill property when choose option from datalist.

BUG=404818
Committed: https://crrev.com/ee0fd44359e1b292ab8a903c0329d315caea0204
Cr-Commit-Position: refs/heads/master@{#292131}

[ziran.sun, 2014-08-22 12:51:13]

Please review. Thanks!

[Ilya Sherman, 2014-08-22 23:49:17]

Thanks!

https://codereview.chromium.org/498653003/diff/20001/chrome/browser/autofill/...
File chrome/browser/autofill/autofill_interactive_uitest.cc (right):

https://codereview.chromium.org/498653003/diff/20001/chrome/browser/autofill/...
chrome/browser/autofill/autofill_interactive_uitest.cc:298: std::string& color)
{
nit: Please pass a pointer.  We pretty much never pass by non-const-reference in
the Chromium repository.

https://codereview.chromium.org/498653003/diff/20001/chrome/browser/autofill/...
chrome/browser/autofill/autofill_interactive_uitest.cc:303: field_name +
"')).backgroundColor);",
nit: The current indentation looks really odd.  I'd either de-indent this line,
or indent the line above by four spaces.

https://codereview.chromium.org/498653003/diff/20001/chrome/browser/autofill/...
chrome/browser/autofill/autofill_interactive_uitest.cc:436: // There is no need
to start message loop for Datalist.
Hmm, why not?  Doesn't it use the same UI?

Even if it's not *necessary* to start the message loop, does it hurt anything to
do so?  If not, I'd prefer that we reuse the SendKeyToPopupAndWait() method, for
the sake of having less (and therefore simpler) code.

https://codereview.chromium.org/498653003/diff/20001/chrome/browser/autofill/...
chrome/browser/autofill/autofill_interactive_uitest.cc:661: // its datalist
option list.
nit: Perhaps something like "Test that an input field is not rendered with the
yellow autofilled background color when choosing an option from the datalist
suggestion list."? (As always, feel free to refine further :)

https://codereview.chromium.org/498653003/diff/20001/chrome/browser/autofill/...
chrome/browser/autofill/autofill_interactive_uitest.cc:668: " <input list=\"dl\"
type=\"search\" id=\"firstname\""
nit: Please indent this line, and the ones below, by one additional space
(within the quotation marks).  That is, the start of the line should be

           "  <input ...

https://codereview.chromium.org/498653003/diff/20001/chrome/browser/autofill/...
chrome/browser/autofill/autofill_interactive_uitest.cc:670: " <datalist
id=\"dl\">"
nit: Why does the datalist need an id?  Would the test work fine if you omitted
this?

[Ilya Sherman, 2014-08-22 23:50:14]

By the way, do we show previews for datalist suggestions?  If so, do those
render with or without the yellow background color?

[ziran.sun, 2014-08-26 13:32:15]

Update code as per review comments. Please review. Thanks!

https://codereview.chromium.org/498653003/diff/20001/chrome/browser/autofill/...
File chrome/browser/autofill/autofill_interactive_uitest.cc (right):

https://codereview.chromium.org/498653003/diff/20001/chrome/browser/autofill/...
chrome/browser/autofill/autofill_interactive_uitest.cc:298: std::string& color)
{
On 2014/08/22 23:49:17, Ilya Sherman wrote:
> nit: Please pass a pointer.  We pretty much never pass by non-const-reference
in
> the Chromium repository.

Done.

https://codereview.chromium.org/498653003/diff/20001/chrome/browser/autofill/...
chrome/browser/autofill/autofill_interactive_uitest.cc:303: field_name +
"')).backgroundColor);",
On 2014/08/22 23:49:17, Ilya Sherman wrote:
> nit: The current indentation looks really odd.  I'd either de-indent this
line,
> or indent the line above by four spaces.

I tried both suggestions, "git cl format" doesn't seem liking them :(. It
returns the original format we have here.

https://codereview.chromium.org/498653003/diff/20001/chrome/browser/autofill/...
chrome/browser/autofill/autofill_interactive_uitest.cc:436: // There is no need
to start message loop for Datalist.
On 2014/08/22 23:49:17, Ilya Sherman wrote:
> Hmm, why not?  Doesn't it use the same UI?
> 
> Even if it's not *necessary* to start the message loop, does it hurt anything
to
> do so?  If not, I'd prefer that we reuse the SendKeyToPopupAndWait() method,
for
> the sake of having less (and therefore simpler) code. 

Existing test cases in this test file assume that once select an option in the
suggestion popup box, the selection gets previewed. So after
"DidSelectSuggestion()" is called, "DidPreviewFormData()" should be called.
"DidPreviewFormData()" defined in this file will stop the message loop.

We don't have preview support for datalist. If we starts message loop for
datalist, after "DidSelectSuggestion()" it will wait for autofill preview that
will never happen. This will cause the message loop hang and the test will end
up with timeout.

https://codereview.chromium.org/498653003/diff/20001/chrome/browser/autofill/...
chrome/browser/autofill/autofill_interactive_uitest.cc:661: // its datalist
option list.
On 2014/08/22 23:49:17, Ilya Sherman wrote:
> nit: Perhaps something like "Test that an input field is not rendered with the
> yellow autofilled background color when choosing an option from the datalist
> suggestion list."? (As always, feel free to refine further :)

Done.

https://codereview.chromium.org/498653003/diff/20001/chrome/browser/autofill/...
chrome/browser/autofill/autofill_interactive_uitest.cc:668: " <input list=\"dl\"
type=\"search\" id=\"firstname\""
On 2014/08/22 23:49:17, Ilya Sherman wrote:
> nit: Please indent this line, and the ones below, by one additional space
> (within the quotation marks).  That is, the start of the line should be
> 
>            "  <input ...

Done.

https://codereview.chromium.org/498653003/diff/20001/chrome/browser/autofill/...
chrome/browser/autofill/autofill_interactive_uitest.cc:670: " <datalist
id=\"dl\">"
On 2014/08/22 23:49:17, Ilya Sherman wrote:
> nit: Why does the datalist need an id?  Would the test work fine if you
omitted
> this?

I think the minimum argument for datalist tag is to have an id. And we refer
this id in input "list". 

The test doesn't work without id specified here.

[ziran.sun, 2014-08-26 13:33:03]

On 2014/08/22 23:50:14, Ilya Sherman wrote:
> By the way, do we show previews for datalist suggestions?  If so, do those
> render with or without the yellow background color?

We don't show previews for datalist.

[Ilya Sherman, 2014-08-27 00:37:47]

LGTM % the formatting issue.  Thanks :)

https://codereview.chromium.org/498653003/diff/20001/chrome/browser/autofill/...
File chrome/browser/autofill/autofill_interactive_uitest.cc (right):

https://codereview.chromium.org/498653003/diff/20001/chrome/browser/autofill/...
chrome/browser/autofill/autofill_interactive_uitest.cc:303: field_name +
"')).backgroundColor);",
On 2014/08/26 13:32:15, ziran.sun wrote:
> On 2014/08/22 23:49:17, Ilya Sherman wrote:
> > nit: The current indentation looks really odd.  I'd either de-indent this
> line,
> > or indent the line above by four spaces.
> 
> I tried both suggestions, "git cl format" doesn't seem liking them :(. It
> returns the original format we have here.

I understand that this is the format that "git cl format" returns, but "git cl
format" is not entirely canonical -- it's a good starting point, but it
sometimes gets the formatting wrong.  This is such a case.

https://codereview.chromium.org/498653003/diff/20001/chrome/browser/autofill/...
chrome/browser/autofill/autofill_interactive_uitest.cc:436: // There is no need
to start message loop for Datalist.
On 2014/08/26 13:32:15, ziran.sun wrote:
> On 2014/08/22 23:49:17, Ilya Sherman wrote:
> > Hmm, why not?  Doesn't it use the same UI?
> > 
> > Even if it's not *necessary* to start the message loop, does it hurt
anything
> to
> > do so?  If not, I'd prefer that we reuse the SendKeyToPopupAndWait() method,
> for
> > the sake of having less (and therefore simpler) code. 
> 
> Existing test cases in this test file assume that once select an option in the
> suggestion popup box, the selection gets previewed. So after
> "DidSelectSuggestion()" is called, "DidPreviewFormData()" should be called.
> "DidPreviewFormData()" defined in this file will stop the message loop.
> 
> We don't have preview support for datalist. If we starts message loop for
> datalist, after "DidSelectSuggestion()" it will wait for autofill preview that
> will never happen. This will cause the message loop hang and the test will end
> up with timeout.

I see.  That makes sense; thanks for the explanation.

[ziran.sun, 2014-08-27 09:16:05]

The CQ bit was checked by ziran.sun@samsung.com

[commit-bot: I haz the power, 2014-08-27 09:17:05]

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

[commit-bot: I haz the power, 2014-08-27 10:11:13]

Committed patchset #4 (50001) as cd4373b75a68f35e75c9e956bf2ac08025108db1

[commit-bot: I haz the power, 2014-09-10 02:50:59]

Patchset 4 (id:??) landed as
https://crrev.com/ee0fd44359e1b292ab8a903c0329d315caea0204
Cr-Commit-Position: refs/heads/master@{#292131}