Display a warning when autofill is disabled for a website.

Display a warning when autofill is disabled for a website.

This depends on a WebKit change being tracked at https://bugs.webkit.org/show_bug.cgi?id=49291

BUG=58509
TEST=unit_tests --gtest_filter=AutoFillManagerTest.*:AutoFillHelperTest.*

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=66214

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=66237

[Ilya Sherman, 2010-11-10 00:59:56]

This has a WebKit dependency -- I'll add a link to the corresponding WebKit bug
shortly.

I haven't looked at tests yet -- there's a good chance they don't currently
compile.

[dhollowa, 2010-11-10 01:58:38]

As we discussed offline, we should use the existing ids and labels for this
"disabled" messaging as opposed to extending the params and IPC messages.

[Ilya Sherman, 2010-11-10 10:16:20]

On 2010/11/10 01:58:38, dhollowa wrote:
> As we discussed offline, we should use the existing ids and labels for this
> "disabled" messaging as opposed to extending the params and IPC messages.

Done.

[dhollowa, 2010-11-12 01:52:03]

http://codereview.chromium.org/4591001/diff/10001/base/scoped_vector.h
File base/scoped_vector.h (right):

http://codereview.chromium.org/4591001/diff/10001/base/scoped_vector.h#newcode58
base/scoped_vector.h:58: void clear() { v.clear(); }
|weak_erase| should be sufficient, no?

http://codereview.chromium.org/4591001/diff/10001/chrome/renderer/autofill_he...
File chrome/renderer/autofill_helper.cc (right):

http://codereview.chromium.org/4591001/diff/10001/chrome/renderer/autofill_he...
chrome/renderer/autofill_helper.cc:269: QueryAutoFillSuggestions(element,
autofill_disabled);
Could we avoid the IPC call here in the case that |autofill_disabled| is true? 
It seems strange that we'd call over to the browser just to fetch the disabled
string.

[Ilya Sherman, 2010-11-12 05:15:38]

The previous patch set had a bug where the warning would be shown if there were
no autofill suggestions but there were autocomplete suggestions -- that's now
fixed, in the code and in the unit tests.

http://codereview.chromium.org/4591001/diff/10001/base/scoped_vector.h
File base/scoped_vector.h (right):

http://codereview.chromium.org/4591001/diff/10001/base/scoped_vector.h#newcode58
base/scoped_vector.h:58: void clear() { v.clear(); }
On 2010/11/12 01:52:03, dhollowa wrote:
> |weak_erase| should be sufficient, no?

Oops, I should be using reset().  Thanks for pointing this out :)

http://codereview.chromium.org/4591001/diff/10001/chrome/renderer/autofill_he...
File chrome/renderer/autofill_helper.cc (right):

http://codereview.chromium.org/4591001/diff/10001/chrome/renderer/autofill_he...
chrome/renderer/autofill_helper.cc:269: QueryAutoFillSuggestions(element,
autofill_disabled);
On 2010/11/12 01:52:03, dhollowa wrote:
> Could we avoid the IPC call here in the case that |autofill_disabled| is true?

> It seems strange that we'd call over to the browser just to fetch the disabled
> string.

The thing is, we don't want to show a warning if there are no matching profiles
or credit cards; and only the browser knows that.  We also don't want to show a
warning if Autofill is disabled; again, only the browser knows that.

[dhollowa, 2010-11-12 23:44:19]

http://codereview.chromium.org/4591001/diff/21001/chrome/app/generated_resour...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/4591001/diff/21001/chrome/app/generated_resour...
chrome/app/generated_resources.grd:6660: <message
name="IDS_AUTOFILL_WARNING_AUTOFILL_DISABLED" desc="Warning text to show when
autofill is disabled by the website for a given form.">
How about IDS_AUTOFILL_WARNING_FORM_DISABLED?  When reading the code this seems
more descriptive.

http://codereview.chromium.org/4591001/diff/21001/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/render_view_host.cc (right):

http://codereview.chromium.org/4591001/diff/21001/chrome/browser/renderer_hos...
chrome/browser/renderer_host/render_view_host.cc:1831: if (autofill_disabled_ &&
suggestions.size() > 0) {
As we discussed, an alternate approach that might simplify things would be to
have the AutoFillHelper, the AutoFillManager, and the Autocomplete code add to
the list.  Then, once returned, have Autofill helper cull the list in one place.
 It will also add the "Clear" and "Options" menu items conditionally as well.

[Ilya Sherman, 2010-11-13 05:24:43]

http://codereview.chromium.org/4591001/diff/21001/chrome/app/generated_resour...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/4591001/diff/21001/chrome/app/generated_resour...
chrome/app/generated_resources.grd:6660: <message
name="IDS_AUTOFILL_WARNING_AUTOFILL_DISABLED" desc="Warning text to show when
autofill is disabled by the website for a given form.">
On 2010/11/12 23:44:20, dhollowa wrote:
> How about IDS_AUTOFILL_WARNING_FORM_DISABLED?  When reading the code this
seems
> more descriptive.

Done.

http://codereview.chromium.org/4591001/diff/21001/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/render_view_host.cc (right):

http://codereview.chromium.org/4591001/diff/21001/chrome/browser/renderer_hos...
chrome/browser/renderer_host/render_view_host.cc:1831: if (autofill_disabled_ &&
suggestions.size() > 0) {
On 2010/11/12 23:44:20, dhollowa wrote:
> As we discussed, an alternate approach that might simplify things would be to
> have the AutoFillHelper, the AutoFillManager, and the Autocomplete code add to
> the list.  Then, once returned, have Autofill helper cull the list in one
place.
>  It will also add the "Clear" and "Options" menu items conditionally as well.

This patch set mostly does this.  I've moved all the warning logic out of
RenderViewHost, so that part is nice and clean.  However, I didn't quite take
the approach of "dump everything in, then cull at the end".  In particular,
AutoFillManager doesn't add any autofill entries if it adds a warning; and it
adds at most one warning.  So, the autofill results consist of either a single
warning, or some number (possibly 0) of actual autofill results.  I think the
code comes out a little cleaner this way.

This also doesn't have unit tests for the AutoFillHelper logic, because both
RenderView and WebView were particularly painful to mock out.  In the spirit of
getting this landed and on the canaries, I think it's best to punt the
AutoFillHelper tests to a separate cl.

[dhollowa, 2010-11-15 18:47:57]

http://codereview.chromium.org/4591001/diff/26001/chrome/browser/autofill/aut...
File chrome/browser/autofill/autofill_manager.cc (right):

http://codereview.chromium.org/4591001/diff/26001/chrome/browser/autofill/aut...
chrome/browser/autofill/autofill_manager.cc:137: if
(!upload_form_structure_->IsAutoFillable(false))
These changes should not alter the types of form data that get uploaded to the
server.  I'd expect |IsAutoFillable(true)| here.

http://codereview.chromium.org/4591001/diff/26001/chrome/browser/autofill/aut...
chrome/browser/autofill/autofill_manager.cc:176: if
(!form->IsAutoFillable(false))
Instead of changing the |IsAutoFillable| and |ShouldBeParsed| semantics, we
should send back the IDS_AUTOFILL_WARNING_FORM_DISABLED warning when we don't
find a valid |autofill_field|.

http://codereview.chromium.org/4591001/diff/26001/chrome/browser/autofill/aut...
chrome/browser/autofill/autofill_manager.cc:218: // Don't provide autofill
suggestions when autofill is disabled, but provide a
nit: The convention in comments is to use "AutoFill" when referencing the
feature.

http://codereview.chromium.org/4591001/diff/26001/chrome/browser/autofill/aut...
chrome/browser/autofill/autofill_manager.cc:627: // TODO(isherman): Might want
to set aside forms that have method != POST,
As mentioned above, we can keep |ShouldBeParsed| as it was.

http://codereview.chromium.org/4591001/diff/26001/chrome/browser/autofill/for...
File chrome/browser/autofill/form_structure.cc (right):

http://codereview.chromium.org/4591001/diff/26001/chrome/browser/autofill/for...
chrome/browser/autofill/form_structure.cc:110: bool auto_fillable =
IsAutoFillable(false);
These changes should not alter the types of form data that get uploaded to the
server.  I'd expect |IsAutoFillable(true)| here.

http://codereview.chromium.org/4591001/diff/26001/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/render_view_host.cc (right):

http://codereview.chromium.org/4591001/diff/26001/chrome/browser/renderer_hos...
chrome/browser/renderer_host/render_view_host.cc:1793: // TODO(isherman): Why
just when the label is empty?
An non-empty label is sufficient to distinguish an Autofill entry.  With the
addition of warnings, this still holds.

http://codereview.chromium.org/4591001/diff/26001/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/render_view_host_delegate.h (right):

http://codereview.chromium.org/4591001/diff/26001/chrome/browser/renderer_hos...
chrome/browser/renderer_host/render_view_host_delegate.h:549: // given the name
of the field, whether it is autofilled, and what the user
nit: s/autofilled/auto-filled/

http://codereview.chromium.org/4591001/diff/26001/chrome/renderer/autofill_he...
File chrome/renderer/autofill_helper.h (right):

http://codereview.chromium.org/4591001/diff/26001/chrome/renderer/autofill_he...
chrome/renderer/autofill_helper.h:113: // displayed to the user if autofill has
suggestions available, but cannot
nit: s/autofill/AutoFill/

[Ilya Sherman, 2010-11-15 22:53:32]

http://codereview.chromium.org/4591001/diff/26001/chrome/browser/autofill/aut...
File chrome/browser/autofill/autofill_manager.cc (right):

http://codereview.chromium.org/4591001/diff/26001/chrome/browser/autofill/aut...
chrome/browser/autofill/autofill_manager.cc:137: if
(!upload_form_structure_->IsAutoFillable(false))
On 2010/11/15 18:47:57, dhollowa wrote:
> These changes should not alter the types of form data that get uploaded to the
> server.  I'd expect |IsAutoFillable(true)| here.

Done.

http://codereview.chromium.org/4591001/diff/26001/chrome/browser/autofill/aut...
chrome/browser/autofill/autofill_manager.cc:176: if
(!form->IsAutoFillable(false))
On 2010/11/15 18:47:57, dhollowa wrote:
> Instead of changing the |IsAutoFillable| and |ShouldBeParsed| semantics, we
> should send back the IDS_AUTOFILL_WARNING_FORM_DISABLED warning when we don't
> find a valid |autofill_field|.

As discussed offline, we only want to show the warning for relevant forms (i.e.
ones that have address or credit card fields); and we have to parse the forms to
determine their relevance.

http://codereview.chromium.org/4591001/diff/26001/chrome/browser/autofill/aut...
chrome/browser/autofill/autofill_manager.cc:218: // Don't provide autofill
suggestions when autofill is disabled, but provide a
On 2010/11/15 18:47:57, dhollowa wrote:
> nit: The convention in comments is to use "AutoFill" when referencing the
> feature.

Done.

http://codereview.chromium.org/4591001/diff/26001/chrome/browser/autofill/aut...
chrome/browser/autofill/autofill_manager.cc:627: // TODO(isherman): Might want
to set aside forms that have method != POST,
On 2010/11/15 18:47:57, dhollowa wrote:
> As mentioned above, we can keep |ShouldBeParsed| as it was.

As mentioned in the reply above, keeping the semantics change to
|ShouldBeParsed|.  However, these forms are now set aside, so that there is no
impact on the server from additional queries.

http://codereview.chromium.org/4591001/diff/26001/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/render_view_host.cc (right):

http://codereview.chromium.org/4591001/diff/26001/chrome/browser/renderer_hos...
chrome/browser/renderer_host/render_view_host.cc:1793: // TODO(isherman): Why
just when the label is empty?
On 2010/11/15 18:47:57, dhollowa wrote:
> An non-empty label is sufficient to distinguish an Autofill entry.  With the
> addition of warnings, this still holds.

This TODO is not related to warnings, but I think it looks very confusing to
have both autofill suggestions and autocomplete suggestions for the same value;
especially if we expect that users don't need to be aware of the difference
between autofill and autocomplete.  Even when I know why we show both entries,
the duplication bugs me every time.

http://codereview.chromium.org/4591001/diff/26001/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/render_view_host_delegate.h (right):

http://codereview.chromium.org/4591001/diff/26001/chrome/browser/renderer_hos...
chrome/browser/renderer_host/render_view_host_delegate.h:549: // given the name
of the field, whether it is autofilled, and what the user
On 2010/11/15 18:47:57, dhollowa wrote:
> nit: s/autofilled/auto-filled/

Done.

http://codereview.chromium.org/4591001/diff/26001/chrome/renderer/autofill_he...
File chrome/renderer/autofill_helper.h (right):

http://codereview.chromium.org/4591001/diff/26001/chrome/renderer/autofill_he...
chrome/renderer/autofill_helper.h:113: // displayed to the user if autofill has
suggestions available, but cannot
On 2010/11/15 18:47:57, dhollowa wrote:
> nit: s/autofill/AutoFill/

Done.

[dhollowa, 2010-11-16 00:02:25]

LGTM (Pending verification against pending Webkit changes). Thanks!