Add a flag to ignore autocomplete="off" for Autofill.

Add a flag to ignore autocomplete="off" for Autofill.

This should help us experiment with making this the default behavior. Note that autocomplete="off" still works for autocomplete history.

new flag is: --ignore-autocomplete-off-autofill

BUG=406083, 425672
Committed: https://crrev.com/0ee9126c15e2c0ec18a9cc9b6522133e45946db2
Cr-Commit-Position: refs/heads/master@{#300929}

[Evan Stade, 2014-10-20 21:24:31]

estade@chromium.org changed reviewers:
+ groby@chromium.org, isherman@chromium.org

[groby-ooo-7-16, 2014-10-20 22:52:31]

LGTM, with the caveat mentioned below

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

https://codereview.chromium.org/667043002/diff/40001/components/autofill/core...
components/autofill/core/browser/autofill_manager.cc:537:
autocomplete_history_manager_->OnGetAutocompleteSuggestions(
Question: OnGetAutocompleteSuggestions also cancels pending queries - is that
important for the "ignore autocomplete" case?

[Evan Stade, 2014-10-20 23:01:54]

Thanks Rachel. I'd also like to wait for Ilya's input.

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

https://codereview.chromium.org/667043002/diff/40001/components/autofill/core...
components/autofill/core/browser/autofill_manager.cc:537:
autocomplete_history_manager_->OnGetAutocompleteSuggestions(
On 2014/10/20 22:52:31, groby wrote:
> Question: OnGetAutocompleteSuggestions also cancels pending queries - is that
> important for the "ignore autocomplete" case?

good call, we should probably do
autocomplete_history_manager_->CancelPendingQuery() in the !should_autocomplete
case.

[Ilya Sherman, 2014-10-21 01:19:12]

Thanks, Evan.  This makes sense to me behind a flag; I definitely still think
we'd want to get the web platform team's explicit approval prior to turning this
flag on for any users, since it could break sites.

https://codereview.chromium.org/667043002/diff/40001/chrome/browser/chrome_co...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/667043002/diff/40001/chrome/browser/chrome_co...
chrome/browser/chrome_content_browser_client.cc:1345:
autofill::switches::kIgnoreAutocompleteOffForAutofill,
nit: Please alphabetize.

https://codereview.chromium.org/667043002/diff/40001/components/autofill/cont...
File components/autofill/content/renderer/form_autofill_util.cc (right):

https://codereview.chromium.org/667043002/diff/40001/components/autofill/cont...
components/autofill/content/renderer/form_autofill_util.cc:10: #include
"base/command_line.h"
nit: Duplicate #include.

https://codereview.chromium.org/667043002/diff/40001/components/autofill/cont...
components/autofill/content/renderer/form_autofill_util.cc:18: #include
"components/autofill/core/common/autofill_switches.h"
Here too.

https://codereview.chromium.org/667043002/diff/40001/components/autofill/cont...
components/autofill/content/renderer/form_autofill_util.cc:495: if
(control_elements.size() != data.fields.size()) {
Hmm, did you also change which fields are extracted when passing fields to the
browser?  If not, how has this check not been consistently failing for ages?

It seems safer to me to limit the smarts of the renderer somewhat: Always
extract all the fields, and just be judicious about which fields to fill when
getting back data from the browser.

https://codereview.chromium.org/667043002/diff/40001/components/autofill/cont...
components/autofill/content/renderer/form_autofill_util.cc:1067: form_element,
ExtractionRequirements(), &control_elements);
We could probably just always pass REQUIRE_NONE here, given that we skip any
non-autofilled fields.

https://codereview.chromium.org/667043002/diff/40001/components/autofill/cont...
components/autofill/content/renderer/form_autofill_util.cc:1127: form_element,
ExtractionRequirements(), &control_elements);
We could probably just always pass REQUIRE_NONE here, too.

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

https://codereview.chromium.org/667043002/diff/40001/components/autofill/core...
components/autofill/core/browser/autofill_external_delegate.cc:50: return
form_field.should_autocomplete ||
nit: Please add parens to support this text alignment, i.e. having the line
below be indented more than four spaces.

https://codereview.chromium.org/667043002/diff/40001/components/autofill/core...
File components/autofill/core/common/autofill_switches.cc (right):

https://codereview.chromium.org/667043002/diff/40001/components/autofill/core...
components/autofill/core/common/autofill_switches.cc:46: }  // namespace
autofill
Not for this CL, but it might be nice to see if we can clear out some obsolete
flags from this file.

[Evan Stade, 2014-10-21 19:12:47]

https://codereview.chromium.org/667043002/diff/40001/chrome/browser/chrome_co...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/667043002/diff/40001/chrome/browser/chrome_co...
chrome/browser/chrome_content_browser_client.cc:1345:
autofill::switches::kIgnoreAutocompleteOffForAutofill,
On 2014/10/21 01:19:11, Ilya Sherman wrote:
> nit: Please alphabetize.

Done.

https://codereview.chromium.org/667043002/diff/40001/components/autofill/cont...
File components/autofill/content/renderer/form_autofill_util.cc (right):

https://codereview.chromium.org/667043002/diff/40001/components/autofill/cont...
components/autofill/content/renderer/form_autofill_util.cc:10: #include
"base/command_line.h"
On 2014/10/21 01:19:11, Ilya Sherman wrote:
> nit: Duplicate #include.

Done.

https://codereview.chromium.org/667043002/diff/40001/components/autofill/cont...
components/autofill/content/renderer/form_autofill_util.cc:18: #include
"components/autofill/core/common/autofill_switches.h"
On 2014/10/21 01:19:11, Ilya Sherman wrote:
> Here too.

Done.

https://codereview.chromium.org/667043002/diff/40001/components/autofill/cont...
components/autofill/content/renderer/form_autofill_util.cc:495: if
(control_elements.size() != data.fields.size()) {
On 2014/10/21 01:19:11, Ilya Sherman wrote:
> Hmm, did you also change which fields are extracted when passing fields to the
> browser?  If not, how has this check not been consistently failing for ages?

good question. I think if there was a mismatch, it only occurred on fields where
autocomplete was off.  See this code:
https://code.google.com/p/chromium/codesearch#chromium/src/components/autofil...

If autocomplete was off, there might have been a mismatch, but we didn't do any
filling anyway.

That said, I don't quite know why I don't have to change that line in some way
now, but leaving it alone, I still can't get Autofill not to work.

> 
> It seems safer to me to limit the smarts of the renderer somewhat: Always
> extract all the fields, and just be judicious about which fields to fill when
> getting back data from the browser.

yea, that's the path I would take if I were going to remove this flag (make it
the default behavior). But since it's still just a flag, I wanted to leave the
current code as much alone as possible. I think the RequirementsMask will just
go away if we make this default behavior.

https://codereview.chromium.org/667043002/diff/40001/components/autofill/cont...
components/autofill/content/renderer/form_autofill_util.cc:1067: form_element,
ExtractionRequirements(), &control_elements);
On 2014/10/21 01:19:11, Ilya Sherman wrote:
> We could probably just always pass REQUIRE_NONE here, given that we skip any
> non-autofilled fields.

ditto

https://codereview.chromium.org/667043002/diff/40001/components/autofill/cont...
components/autofill/content/renderer/form_autofill_util.cc:1127: form_element,
ExtractionRequirements(), &control_elements);
On 2014/10/21 01:19:11, Ilya Sherman wrote:
> We could probably just always pass REQUIRE_NONE here, too.

ditto

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

https://codereview.chromium.org/667043002/diff/40001/components/autofill/core...
components/autofill/core/browser/autofill_external_delegate.cc:50: return
form_field.should_autocomplete ||
On 2014/10/21 01:19:11, Ilya Sherman wrote:
> nit: Please add parens to support this text alignment, i.e. having the line
> below be indented more than four spaces.

I'm not sure where you want me to add parens. I'm pretty sure this formatting
was a result of git cl format. I probably would have indented the second line
just 4 spaces.

https://codereview.chromium.org/667043002/diff/40001/components/autofill/core...
File components/autofill/core/common/autofill_switches.cc (right):

https://codereview.chromium.org/667043002/diff/40001/components/autofill/core...
components/autofill/core/common/autofill_switches.cc:46: }  // namespace
autofill
On 2014/10/21 01:19:11, Ilya Sherman wrote:
> Not for this CL, but it might be nice to see if we can clear out some obsolete
> flags from this file.

which ones are obsolete?

[Evan Stade, 2014-10-21 19:32:13]

https://codereview.chromium.org/667043002/diff/100001/chrome/browser/about_fl...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/667043002/diff/100001/chrome/browser/about_fl...
chrome/browser/about_flags.cc:1658: "ignore-autocomplete-off-autofill",
Also added an entry here. It's limited to desktop for now because I haven't had
a chance to test elsewhere (I'm fairly certain iOS will need additional changes
but Android is probably covered).

[Ilya Sherman, 2014-10-21 23:58:19]

https://codereview.chromium.org/667043002/diff/40001/components/autofill/cont...
File components/autofill/content/renderer/form_autofill_util.cc (right):

https://codereview.chromium.org/667043002/diff/40001/components/autofill/cont...
components/autofill/content/renderer/form_autofill_util.cc:495: if
(control_elements.size() != data.fields.size()) {
On 2014/10/21 19:12:47, Evan Stade wrote:
> On 2014/10/21 01:19:11, Ilya Sherman wrote:
> > Hmm, did you also change which fields are extracted when passing fields to
the
> > browser?  If not, how has this check not been consistently failing for ages?
> 
> good question. I think if there was a mismatch, it only occurred on fields
where
> autocomplete was off.  See this code:
>
https://code.google.com/p/chromium/codesearch#chromium/src/components/autofil...
> 
> If autocomplete was off, there might have been a mismatch, but we didn't do
any
> filling anyway.
> 
> That said, I don't quite know why I don't have to change that line in some way
> now, but leaving it alone, I still can't get Autofill not to work.

Hmm, that's odd.  Could you please do a bit of debugging to convince yourself
that the code is working as you'd expect?  What is data.fields.size() if you try
to autofill from the top element in the following form?  With the flag enabled,
does Autofilling this form from the top field work, and fill the confirm email
field as well?

<form>
  <input name="full_name">
  <input name="email">
  <input name="confirm_email" autocomplete="off">
  <input name="phone">
</form>

> > It seems safer to me to limit the smarts of the renderer somewhat: Always
> > extract all the fields, and just be judicious about which fields to fill
when
> > getting back data from the browser.
> 
> yea, that's the path I would take if I were going to remove this flag (make it
> the default behavior). But since it's still just a flag, I wanted to leave the
> current code as much alone as possible. I think the RequirementsMask will just
> go away if we make this default behavior.

[Ilya Sherman, 2014-10-22 00:00:39]

https://codereview.chromium.org/667043002/diff/120001/components/autofill/con...
File components/autofill/content/renderer/form_autofill_util.cc (right):

https://codereview.chromium.org/667043002/diff/120001/components/autofill/con...
components/autofill/content/renderer/form_autofill_util.cc:63:
FILTER_ALL_NON_EDITABLE_ELEMENTS = FILTER_DISABLED_ELEMENTS |
nit: Please keep the equals signs aligned, and update the alignment in general.

[Evan Stade, 2014-10-22 21:52:33]

https://codereview.chromium.org/667043002/diff/120001/components/autofill/con...
File components/autofill/content/renderer/form_autofill_util.cc (right):

https://codereview.chromium.org/667043002/diff/120001/components/autofill/con...
components/autofill/content/renderer/form_autofill_util.cc:63:
FILTER_ALL_NON_EDITABLE_ELEMENTS = FILTER_DISABLED_ELEMENTS |
On 2014/10/22 00:00:39, Ilya Sherman wrote:
> nit: Please keep the equals signs aligned, and update the alignment in
general.

Done.

https://codereview.chromium.org/667043002/diff/140001/components/autofill/con...
File components/autofill/content/renderer/autofill_agent.cc (right):

https://codereview.chromium.org/667043002/diff/140001/components/autofill/con...
components/autofill/content/renderer/autofill_agent.cc:638: // If we're ignoring
autocomplete="off", always extract everything.
you were right about needing to make this change

https://codereview.chromium.org/667043002/diff/140001/components/autofill/cor...
File components/autofill/core/browser/autofill_manager.cc (right):

https://codereview.chromium.org/667043002/diff/140001/components/autofill/cor...
components/autofill/core/browser/autofill_manager.cc:498:
"reduce-security-for-testing")) {
this is a string literal because the flag is defined in content/ (i.e. no
accessible from this file)

[Ilya Sherman, 2014-10-23 00:35:27]

Thanks.  The logic LG now -- could you please add some test coverage?  (Sorry
for not calling that out sooner.)

https://codereview.chromium.org/667043002/diff/140001/components/autofill/cor...
File components/autofill/core/browser/autofill_manager.cc (right):

https://codereview.chromium.org/667043002/diff/140001/components/autofill/cor...
components/autofill/core/browser/autofill_manager.cc:498:
"reduce-security-for-testing")) {
On 2014/10/22 21:52:33, Evan Stade wrote:
> this is a string literal because the flag is defined in content/ (i.e. no
> accessible from this file)

This seems unrelated to this CL.  I'd honestly prefer not to make this change at
all, but I don't feel strongly about that.  However, I do think that if we want
to make this change, we should (a) split it off into a separate CL, and (b)
access the flag via the embedder interface, rather than using a literal string
here.

[Evan Stade, 2014-10-23 02:02:20]

I don't think it makes sense to write tests for functionality that's behind a
flag. Nevertheless, tests added.

https://codereview.chromium.org/667043002/diff/140001/components/autofill/cor...
File components/autofill/core/browser/autofill_manager.cc (right):

https://codereview.chromium.org/667043002/diff/140001/components/autofill/cor...
components/autofill/core/browser/autofill_manager.cc:498:
"reduce-security-for-testing")) {
On 2014/10/23 00:35:27, Ilya Sherman wrote:
> On 2014/10/22 21:52:33, Evan Stade wrote:
> > this is a string literal because the flag is defined in content/ (i.e. no
> > accessible from this file)
> 
> This seems unrelated to this CL.  I'd honestly prefer not to make this change
at
> all, but I don't feel strongly about that.  However, I do think that if we
want
> to make this change, we should (a) split it off into a separate CL, and (b)
> access the flag via the embedder interface, rather than using a literal string
> here.

split this into a separate CL.

[Ilya Sherman, 2014-10-23 04:24:31]

LGTM, thanks.

[Evan Stade, 2014-10-23 18:18:35]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2014-10-23 18:19:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/667043002/180001

[commit-bot: I haz the power, 2014-10-23 19:29:31]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2014-10-23 19:30:06]

Patchset 10 (id:??) landed as
https://crrev.com/0ee9126c15e2c0ec18a9cc9b6522133e45946db2
Cr-Commit-Position: refs/heads/master@{#300929}