[autofill] Fill in values on a successful run of interactive autocomplete.

[autofill] Fill in values on a successful run of interactive autocomplete.

R=estade@chromium.org,isherman@chromium.org
TBR=jschuh@chromium.org
BUG=157661
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=170581

[Ilya Sherman, 2012-11-28 06:16:39]

Looks good at a high level, but a bunch of comments on the details:

https://codereview.chromium.org/11348273/diff/1/chrome/browser/autofill/form_...
File chrome/browser/autofill/form_structure.cc (right):

https://codereview.chromium.org/11348273/diff/1/chrome/browser/autofill/form_...
chrome/browser/autofill/form_structure.cc:818: // TODO(dbeam): Store
user_submitted in ctor if it's important.
It's not important.  Just include a comment that it will always be false.

https://codereview.chromium.org/11348273/diff/1/chrome/browser/autofill/form_...
chrome/browser/autofill/form_structure.cc:819: FormData* data = new FormData();
If you keep the heap allocation, please use a scoped_ptr here.

https://codereview.chromium.org/11348273/diff/1/chrome/browser/autofill/form_...
chrome/browser/autofill/form_structure.cc:827: new AutofillField(*fields_[i],
fields_[i]->unique_name());
This sure looks like it will leak.  Why isn't this just stack allocated?

https://codereview.chromium.org/11348273/diff/1/chrome/browser/autofill/form_...
chrome/browser/autofill/form_structure.cc:828: data->fields.push_back(*copy);
Methinks the body of this for loop should be:

data->fields.push_back(FormFieldData(*fields_[i]));

https://codereview.chromium.org/11348273/diff/1/chrome/browser/autofill/form_...
File chrome/browser/autofill/form_structure.h (right):

https://codereview.chromium.org/11348273/diff/1/chrome/browser/autofill/form_...
chrome/browser/autofill/form_structure.h:148: FormData* ToFormData() const;
nit: Please change this signature to: "void ToFormData(FormData* form) const;". 
Returning raw memory is generally a bad idea as it's easy to leak the memory. 
We could return a scoped_ptr<>, but I think filling an out-param is more
Chrome-y.

Also, please add a comment describing this method.

https://codereview.chromium.org/11348273/diff/1/chrome/common/autofill_messag...
File chrome/common/autofill_messages.h (right):

https://codereview.chromium.org/11348273/diff/1/chrome/common/autofill_messag...
chrome/common/autofill_messages.h:137: // Sent when interactive autocomplete
succeeds. Changes the original form.
nit: I'm not sure I follow what "Changes the original form" means...

https://codereview.chromium.org/11348273/diff/1/chrome/common/autofill_messag...
chrome/common/autofill_messages.h:142:
IPC_MESSAGE_ROUTED0(AutofillMsg_RequestAutocompleteError);
nit: The comment makes me want to see the failure reason passed back as part of
this IPC.  How come it isn't?

https://codereview.chromium.org/11348273/diff/1/chrome/renderer/autofill/auto...
File chrome/renderer/autofill/autofill_agent.cc (right):

https://codereview.chromium.org/11348273/diff/1/chrome/renderer/autofill/auto...
chrome/renderer/autofill/autofill_agent.cc:621: }
Any reason not to just re-use FillForm() from form_autofill_util?

https://codereview.chromium.org/11348273/diff/1/chrome/renderer/autofill/auto...
File chrome/renderer/autofill/autofill_agent.h (right):

https://codereview.chromium.org/11348273/diff/1/chrome/renderer/autofill/auto...
chrome/renderer/autofill/autofill_agent.h:120: const
WebKit::WebFormElement::AutocompleteResult& result);
Hmm, I'm not seeing where this gets called.  Is it still being used / am I just
missing it?

[Dan Beam, 2012-11-28 19:59:10]

https://codereview.chromium.org/11348273/diff/1/chrome/browser/autofill/form_...
File chrome/browser/autofill/form_structure.cc (right):

https://codereview.chromium.org/11348273/diff/1/chrome/browser/autofill/form_...
chrome/browser/autofill/form_structure.cc:818: // TODO(dbeam): Store
user_submitted in ctor if it's important.
On 2012/11/28 06:16:39, Ilya Sherman wrote:
> It's not important.  Just include a comment that it will always be false.

Done.

https://codereview.chromium.org/11348273/diff/1/chrome/browser/autofill/form_...
chrome/browser/autofill/form_structure.cc:819: FormData* data = new FormData();
On 2012/11/28 06:16:39, Ilya Sherman wrote:
> If you keep the heap allocation, please use a scoped_ptr here.

Done.  How would I do this alternatively?  I'm a C++ noob.

https://codereview.chromium.org/11348273/diff/1/chrome/browser/autofill/form_...
chrome/browser/autofill/form_structure.cc:827: new AutofillField(*fields_[i],
fields_[i]->unique_name());
On 2012/11/28 06:16:39, Ilya Sherman wrote:
> This sure looks like it will leak.  Why isn't this just stack allocated?

Done.

https://codereview.chromium.org/11348273/diff/1/chrome/browser/autofill/form_...
chrome/browser/autofill/form_structure.cc:828: data->fields.push_back(*copy);
On 2012/11/28 06:16:39, Ilya Sherman wrote:
> Methinks the body of this for loop should be:
> 
> data->fields.push_back(FormFieldData(*fields_[i]));

Done.

https://codereview.chromium.org/11348273/diff/1/chrome/browser/autofill/form_...
File chrome/browser/autofill/form_structure.h (right):

https://codereview.chromium.org/11348273/diff/1/chrome/browser/autofill/form_...
chrome/browser/autofill/form_structure.h:148: FormData* ToFormData() const;
On 2012/11/28 06:16:39, Ilya Sherman wrote:
> nit: Please change this signature to: "void ToFormData(FormData* form)
const;". 
> Returning raw memory is generally a bad idea as it's easy to leak the memory. 
> We could return a scoped_ptr<>, but I think filling an out-param is more
> Chrome-y.
> 
> Also, please add a comment describing this method.

Done.

https://codereview.chromium.org/11348273/diff/1/chrome/common/autofill_messag...
File chrome/common/autofill_messages.h (right):

https://codereview.chromium.org/11348273/diff/1/chrome/common/autofill_messag...
chrome/common/autofill_messages.h:137: // Sent when interactive autocomplete
succeeds. Changes the original form.
On 2012/11/28 06:16:39, Ilya Sherman wrote:
> nit: I'm not sure I follow what "Changes the original form" means...

Done. (how about now?)

https://codereview.chromium.org/11348273/diff/1/chrome/common/autofill_messag...
chrome/common/autofill_messages.h:142:
IPC_MESSAGE_ROUTED0(AutofillMsg_RequestAutocompleteError);
On 2012/11/28 06:16:39, Ilya Sherman wrote:
> nit: The comment makes me want to see the failure reason passed back as part
of
> this IPC.  How come it isn't?

We currently don't have a failure reason enum or anything like that.  Do you
want me to make?  It'll be useful if a) we end up passing this the whole way
back to the DOM, b) for testing.

https://codereview.chromium.org/11348273/diff/1/chrome/renderer/autofill/auto...
File chrome/renderer/autofill/autofill_agent.cc (right):

https://codereview.chromium.org/11348273/diff/1/chrome/renderer/autofill/auto...
chrome/renderer/autofill/autofill_agent.cc:621: }
On 2012/11/28 06:16:39, Ilya Sherman wrote:
> Any reason not to just re-use FillForm() from form_autofill_util?

That's really similar, but
a) how does this handle multiple elements with the same [autocomplete]
attribute? estade@ and I envisioned only filling the first one with the same
type.
b) it seems this won't work on non-focusable elements (i.e. [hidden] ones)?

Also, just using FillFormField() might be a middle ground.  I've added that
method to the form_autofill_util.h just for now until we decide.

https://codereview.chromium.org/11348273/diff/1/chrome/renderer/autofill/auto...
File chrome/renderer/autofill/autofill_agent.h (right):

https://codereview.chromium.org/11348273/diff/1/chrome/renderer/autofill/auto...
chrome/renderer/autofill/autofill_agent.h:120: const
WebKit::WebFormElement::AutocompleteResult& result);
On 2012/11/28 06:16:39, Ilya Sherman wrote:
> Hmm, I'm not seeing where this gets called.  Is it still being used / am I
just
> missing it?

Nope, it's not any more.

[Evan Stade, 2012-11-28 22:45:51]

https://codereview.chromium.org/11348273/diff/5002/chrome/browser/autofill/fo...
File chrome/browser/autofill/form_structure.cc (right):

https://codereview.chromium.org/11348273/diff/5002/chrome/browser/autofill/fo...
chrome/browser/autofill/form_structure.cc:829: out_data = data.release();
this doesn't do anything

https://codereview.chromium.org/11348273/diff/5002/chrome/browser/autofill/fo...
File chrome/browser/autofill/form_structure.h (right):

https://codereview.chromium.org/11348273/diff/5002/chrome/browser/autofill/fo...
chrome/browser/autofill/form_structure.h:150: void ToFormData(FormData*
out_data) const;
return FormData*

[Ilya Sherman, 2012-11-29 00:08:59]

https://chromiumcodereview.appspot.com/11348273/diff/1/chrome/common/autofill...
File chrome/common/autofill_messages.h (right):

https://chromiumcodereview.appspot.com/11348273/diff/1/chrome/common/autofill...
chrome/common/autofill_messages.h:137: // Sent when interactive autocomplete
succeeds. Changes the original form.
On 2012/11/28 19:59:10, Dan Beam wrote:
> On 2012/11/28 06:16:39, Ilya Sherman wrote:
> > nit: I'm not sure I follow what "Changes the original form" means...
> 
> Done. (how about now?)

This is clearer, thanks.  It's still a little weird to say that the IPC message
fills a form though -- the IPC message just passes data to the renderer. 
Perhaps phrase the second sentence as "Tells the renderer to Autofill the form
that requested autocomplete with the |form_data| values input by the user."?

https://chromiumcodereview.appspot.com/11348273/diff/1/chrome/common/autofill...
chrome/common/autofill_messages.h:142:
IPC_MESSAGE_ROUTED0(AutofillMsg_RequestAutocompleteError);
On 2012/11/28 19:59:10, Dan Beam wrote:
> On 2012/11/28 06:16:39, Ilya Sherman wrote:
> > nit: The comment makes me want to see the failure reason passed back as part
> of
> > this IPC.  How come it isn't?
> 
> We currently don't have a failure reason enum or anything like that.  Do you
> want me to make?  It'll be useful if a) we end up passing this the whole way
> back to the DOM, b) for testing.

IMO this would be clearer either if you pass an enum value; or if there's no
need for one yet, drop the "for any number of reasons" phrase from the comment.

https://chromiumcodereview.appspot.com/11348273/diff/1/chrome/renderer/autofi...
File chrome/renderer/autofill/autofill_agent.cc (right):

https://chromiumcodereview.appspot.com/11348273/diff/1/chrome/renderer/autofi...
chrome/renderer/autofill/autofill_agent.cc:621: }
On 2012/11/28 19:59:10, Dan Beam wrote:
> On 2012/11/28 06:16:39, Ilya Sherman wrote:
> > Any reason not to just re-use FillForm() from form_autofill_util?
> 
> That's really similar, but
> a) how does this handle multiple elements with the same [autocomplete]
> attribute? estade@ and I envisioned only filling the first one with the same
> type.

FillForm() would fill all of them.  What's the reason to only fill the first one
if the website asks for the info multiple times?

> b) it seems this won't work on non-focusable elements (i.e. [hidden] ones)?

That's true, but we could add a parameter to control that.  We presumably still
don't want to fill disabled or read-only fields, though.

> Also, just using FillFormField() might be a middle ground.  I've added that
> method to the form_autofill_util.h just for now until we decide.

I think it would be best to re-use as much of the logic from the
ForEachMatchingFormField() method as possible; but if that proves to be too much
of a PITA for whatever reason, using FillFormField() is definitely better than
doing everything from scratch :)

https://chromiumcodereview.appspot.com/11348273/diff/5002/chrome/browser/auto...
File chrome/browser/autofill/form_structure.cc (right):

https://chromiumcodereview.appspot.com/11348273/diff/5002/chrome/browser/auto...
chrome/browser/autofill/form_structure.cc:823: data->method = method_;
Why not just directly set the members of |out_data|, rather than allocating a
new form within this method?

https://chromiumcodereview.appspot.com/11348273/diff/5002/chrome/browser/auto...
File chrome/browser/autofill/form_structure.h (right):

https://chromiumcodereview.appspot.com/11348273/diff/5002/chrome/browser/auto...
chrome/browser/autofill/form_structure.h:149: // |out_data->user_initiated| will
always be false.
nit: "user_initiated" -> "user_submitted"

https://chromiumcodereview.appspot.com/11348273/diff/5002/chrome/browser/auto...
chrome/browser/autofill/form_structure.h:150: void ToFormData(FormData*
out_data) const;
On 2012/11/28 22:45:51, Evan Stade wrote:
> return FormData*

Conceptually this function signature should be "FormData ToFormData() const". 
However, for efficiency's sake, Chrome code usually instead uses out-params,
i.e. "void ToFormData(FormData* result) const".

Returning an un-owned heap allocated pointer is definitely wrong, so the
signature "FormData* ToFormData() const" doesn't make sense.  The signature
"scoped_ptr<FormData> ToFormData()" makes sense, but requires an extra heap
allocation, which is otherwise unnecessary.  That's why I recommended using the
current method signature.

https://chromiumcodereview.appspot.com/11348273/diff/5002/chrome/renderer/aut...
File chrome/renderer/autofill/autofill_agent.cc (right):

https://chromiumcodereview.appspot.com/11348273/diff/5002/chrome/renderer/aut...
chrome/renderer/autofill/autofill_agent.cc:619: }
If we end up keeping this logic, I would recommend moving it into the
form_autofill_util files, which are responsible for most of the other logic that
actually deals with reading and writing form data.

https://chromiumcodereview.appspot.com/11348273/diff/5002/chrome/renderer/aut...
File chrome/renderer/autofill/form_autofill_util.cc (right):

https://chromiumcodereview.appspot.com/11348273/diff/5002/chrome/renderer/aut...
chrome/renderer/autofill/form_autofill_util.cc:824: }
I'm assuming this just got moved, with no changes to the code -- oui?  (I wish
our code review tool would just show that...)

[Evan Stade, 2012-11-29 00:20:59]

https://chromiumcodereview.appspot.com/11348273/diff/5002/chrome/browser/auto...
File chrome/browser/autofill/form_structure.h (right):

https://chromiumcodereview.appspot.com/11348273/diff/5002/chrome/browser/auto...
chrome/browser/autofill/form_structure.h:150: void ToFormData(FormData*
out_data) const;
On 2012/11/29 00:08:59, Ilya Sherman wrote:
> On 2012/11/28 22:45:51, Evan Stade wrote:
> > return FormData*
> 
> Conceptually this function signature should be "FormData ToFormData() const". 
> However, for efficiency's sake, Chrome code usually instead uses out-params,
> i.e. "void ToFormData(FormData* result) const".

yes, FormGroup is correct rather than FormGroup*. I don't think efficiency plays
a role here because the size of the structure is quite small. Do you have any
evidence that Chrome usually uses out params? I think out params are easier to
misuse and harder to read. They are acceptable under certain circumstances, for
example if failure is possible, if multiple values need to be output, etc. None
of those apply here though.

> 
> Returning an un-owned heap allocated pointer is definitely wrong, so the
> signature "FormData* ToFormData() const" doesn't make sense.  The signature
> "scoped_ptr<FormData> ToFormData()" makes sense, but requires an extra heap
> allocation, which is otherwise unnecessary.  That's why I recommended using
the
> current method signature.

[Evan Stade, 2012-11-29 00:21:17]

s/FormGroup/FormData

[Ilya Sherman, 2012-11-29 00:26:42]

https://chromiumcodereview.appspot.com/11348273/diff/5002/chrome/browser/auto...
File chrome/browser/autofill/form_structure.h (right):

https://chromiumcodereview.appspot.com/11348273/diff/5002/chrome/browser/auto...
chrome/browser/autofill/form_structure.h:150: void ToFormData(FormData*
out_data) const;
On 2012/11/29 00:20:59, Evan Stade wrote:
> On 2012/11/29 00:08:59, Ilya Sherman wrote:
> > On 2012/11/28 22:45:51, Evan Stade wrote:
> > > return FormData*
> > 
> > Conceptually this function signature should be "FormData ToFormData()
const". 
> > However, for efficiency's sake, Chrome code usually instead uses out-params,
> > i.e. "void ToFormData(FormData* result) const".
> 
> yes, FormGroup is correct rather than FormGroup*. I don't think efficiency
plays
> a role here because the size of the structure is quite small. Do you have any
> evidence that Chrome usually uses out params? I think out params are easier to
> misuse and harder to read. They are acceptable under certain circumstances,
for
> example if failure is possible, if multiple values need to be output, etc.
None
> of those apply here though.

Fair enough -- we're about to make another copy of this data when we send it
over IPC anyway, so a copy here isn't too bad.  I was under the impression that
Chrome code usually uses out-params based on this bit of the Chromium style
guide [1]: "After testing, it seems just as fast to return a std::[w]string by
value as by out param. So we encourage passing strings out as return values when
possible for the sake of readability."  To me that suggested that things bigger
than strings should normally be returned by out param.

[1] http://dev.chromium.org/developers/coding-style#Method_Signatures

> > Returning an un-owned heap allocated pointer is definitely wrong, so the
> > signature "FormData* ToFormData() const" doesn't make sense.  The signature
> > "scoped_ptr<FormData> ToFormData()" makes sense, but requires an extra heap
> > allocation, which is otherwise unnecessary.  That's why I recommended using
> the
> > current method signature.
>

[Evan Stade, 2012-11-29 00:38:56]

https://chromiumcodereview.appspot.com/11348273/diff/13001/chrome/renderer/au...
File chrome/renderer/autofill/autofill_agent.cc (right):

https://chromiumcodereview.appspot.com/11348273/diff/13001/chrome/renderer/au...
chrome/renderer/autofill/autofill_agent.cc:603: const FormFieldData field =
form_data.fields[i];
this should be const ref.

https://chromiumcodereview.appspot.com/11348273/diff/13001/chrome/renderer/au...
chrome/renderer/autofill/autofill_agent.cc:614: if (field_attr_lower ==
StringToLowerASCII(element_attr)) {
use LowerCaseEqualsASCII?

https://chromiumcodereview.appspot.com/11348273/diff/13001/chrome/renderer/au...
chrome/renderer/autofill/autofill_agent.cc:615:
autofill::FillFormField(&elements[j], &field, false);
FillFormField's second param should be a const ref. While you're fixing that,
you can also move the outparam to after the inparams.

[Dan Beam, 2012-11-29 00:51:13]

https://chromiumcodereview.appspot.com/11348273/diff/1/chrome/common/autofill...
File chrome/common/autofill_messages.h (right):

https://chromiumcodereview.appspot.com/11348273/diff/1/chrome/common/autofill...
chrome/common/autofill_messages.h:137: // Sent when interactive autocomplete
succeeds. Changes the original form.
On 2012/11/29 00:08:59, Ilya Sherman wrote:
> On 2012/11/28 19:59:10, Dan Beam wrote:
> > On 2012/11/28 06:16:39, Ilya Sherman wrote:
> > > nit: I'm not sure I follow what "Changes the original form" means...
> > 
> > Done. (how about now?)
> 
> This is clearer, thanks.  It's still a little weird to say that the IPC
message
> fills a form though -- the IPC message just passes data to the renderer. 
> Perhaps phrase the second sentence as "Tells the renderer to Autofill the form
> that requested autocomplete with the |form_data| values input by the user."?

Done.

https://chromiumcodereview.appspot.com/11348273/diff/1/chrome/common/autofill...
chrome/common/autofill_messages.h:142:
IPC_MESSAGE_ROUTED0(AutofillMsg_RequestAutocompleteError);
On 2012/11/29 00:08:59, Ilya Sherman wrote:
> On 2012/11/28 19:59:10, Dan Beam wrote:
> > On 2012/11/28 06:16:39, Ilya Sherman wrote:
> > > nit: The comment makes me want to see the failure reason passed back as
part
> > of
> > > this IPC.  How come it isn't?
> > 
> > We currently don't have a failure reason enum or anything like that.  Do you
> > want me to make?  It'll be useful if a) we end up passing this the whole way
> > back to the DOM, b) for testing.
> 
> IMO this would be clearer either if you pass an enum value; or if there's no
> need for one yet, drop the "for any number of reasons" phrase from the
comment.

This is only failing for 1 reason right now, so just updated the doc about this.

https://chromiumcodereview.appspot.com/11348273/diff/1/chrome/renderer/autofi...
File chrome/renderer/autofill/autofill_agent.cc (right):

https://chromiumcodereview.appspot.com/11348273/diff/1/chrome/renderer/autofi...
chrome/renderer/autofill/autofill_agent.cc:621: }
On 2012/11/29 00:08:59, Ilya Sherman wrote:
> On 2012/11/28 19:59:10, Dan Beam wrote:
> > On 2012/11/28 06:16:39, Ilya Sherman wrote:
> > > Any reason not to just re-use FillForm() from form_autofill_util?
> > 
> > That's really similar, but
> > a) how does this handle multiple elements with the same [autocomplete]
> > attribute? estade@ and I envisioned only filling the first one with the same
> > type.
> 
> FillForm() would fill all of them.  What's the reason to only fill the first
one
> if the website asks for the info multiple times?
> 
> > b) it seems this won't work on non-focusable elements (i.e. [hidden] ones)?
> 
> That's true, but we could add a parameter to control that.  We presumably
still
> don't want to fill disabled or read-only fields, though.
> 
> > Also, just using FillFormField() might be a middle ground.  I've added that
> > method to the form_autofill_util.h just for now until we decide.
> 
> I think it would be best to re-use as much of the logic from the
> ForEachMatchingFormField() method as possible; but if that proves to be too
much
> of a PITA for whatever reason, using FillFormField() is definitely better than
> doing everything from scratch :)

ForEachMatchingFormField also ignores select[autocomplete] at the moment -
should I remove this or is it still valid?
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/renderer/autofill/f...

https://chromiumcodereview.appspot.com/11348273/diff/5002/chrome/browser/auto...
File chrome/browser/autofill/form_structure.cc (right):

https://chromiumcodereview.appspot.com/11348273/diff/5002/chrome/browser/auto...
chrome/browser/autofill/form_structure.cc:823: data->method = method_;
On 2012/11/29 00:08:59, Ilya Sherman wrote:
> Why not just directly set the members of |out_data|, rather than allocating a
> new form within this method?

Changed to passing value rather than using out param.

https://chromiumcodereview.appspot.com/11348273/diff/5002/chrome/browser/auto...
chrome/browser/autofill/form_structure.cc:829: out_data = data.release();
On 2012/11/28 22:45:51, Evan Stade wrote:
> this doesn't do anything

Thanks for the explanation.

https://chromiumcodereview.appspot.com/11348273/diff/5002/chrome/renderer/aut...
File chrome/renderer/autofill/form_autofill_util.cc (right):

https://chromiumcodereview.appspot.com/11348273/diff/5002/chrome/renderer/aut...
chrome/renderer/autofill/form_autofill_util.cc:824: }
On 2012/11/29 00:08:59, Ilya Sherman wrote:
> I'm assuming this just got moved, with no changes to the code -- oui?  (I wish
> our code review tool would just show that...)

Yes, moved out of an anonymous namespace.

[Dan Beam, 2012-11-29 01:05:47]

https://chromiumcodereview.appspot.com/11348273/diff/13001/chrome/renderer/au...
File chrome/renderer/autofill/autofill_agent.cc (right):

https://chromiumcodereview.appspot.com/11348273/diff/13001/chrome/renderer/au...
chrome/renderer/autofill/autofill_agent.cc:613:
UTF16ToASCII(elements[j].getAttribute("autocomplete"));
double check that this is ASCII before converting

https://chromiumcodereview.appspot.com/11348273/diff/14001/chrome/renderer/au...
File chrome/renderer/autofill/autofill_agent.cc (right):

https://chromiumcodereview.appspot.com/11348273/diff/14001/chrome/renderer/au...
chrome/renderer/autofill/autofill_agent.cc:598:
autofill::ExtractAutofillableElements(in_flight_request_form_,
oh, I guess I'm using this as well... (drops select tags)

[Evan Stade, 2012-11-29 01:06:28]

https://codereview.chromium.org/11348273/diff/14001/chrome/renderer/autofill/...
File chrome/renderer/autofill/autofill_agent.cc (right):

https://codereview.chromium.org/11348273/diff/14001/chrome/renderer/autofill/...
chrome/renderer/autofill/autofill_agent.cc:613:
UTF16ToASCII(elements[j].getAttribute("autocomplete"));
if the attribute can have non-ASCII characters, this will dcheck.

[Dan Beam, 2012-11-29 02:14:59]

https://codereview.chromium.org/11348273/diff/14001/chrome/renderer/autofill/...
File chrome/renderer/autofill/autofill_agent.cc (right):

https://codereview.chromium.org/11348273/diff/14001/chrome/renderer/autofill/...
chrome/renderer/autofill/autofill_agent.cc:613:
UTF16ToASCII(elements[j].getAttribute("autocomplete"));
On 2012/11/29 01:06:28, Evan Stade wrote:
> if the attribute can have non-ASCII characters, this will dcheck.

Fixed.

[Dan Beam, 2012-11-29 03:48:28]

https://chromiumcodereview.appspot.com/11348273/diff/13001/chrome/renderer/au...
File chrome/renderer/autofill/autofill_agent.cc (right):

https://chromiumcodereview.appspot.com/11348273/diff/13001/chrome/renderer/au...
chrome/renderer/autofill/autofill_agent.cc:603: const FormFieldData field =
form_data.fields[i];
On 2012/11/29 00:38:56, Evan Stade wrote:
> this should be const ref.

Done.

https://chromiumcodereview.appspot.com/11348273/diff/13001/chrome/renderer/au...
chrome/renderer/autofill/autofill_agent.cc:613:
UTF16ToASCII(elements[j].getAttribute("autocomplete"));
On 2012/11/29 01:05:48, Dan Beam wrote:
> double check that this is ASCII before converting

Done.

https://chromiumcodereview.appspot.com/11348273/diff/13001/chrome/renderer/au...
chrome/renderer/autofill/autofill_agent.cc:614: if (field_attr_lower ==
StringToLowerASCII(element_attr)) {
On 2012/11/29 00:38:56, Evan Stade wrote:
> use LowerCaseEqualsASCII?

Done.

https://chromiumcodereview.appspot.com/11348273/diff/13001/chrome/renderer/au...
chrome/renderer/autofill/autofill_agent.cc:615:
autofill::FillFormField(&elements[j], &field, false);
On 2012/11/29 00:38:56, Evan Stade wrote:
> FillFormField's second param should be a const ref. While you're fixing that,
> you can also move the outparam to after the inparams.

Done.

[Dan Beam, 2012-11-29 04:00:50]

removed element erase()s

[Evan Stade, 2012-11-29 04:01:25]

https://chromiumcodereview.appspot.com/11348273/diff/7013/chrome/renderer/aut...
File chrome/renderer/autofill/autofill_agent.cc (right):

https://chromiumcodereview.appspot.com/11348273/diff/7013/chrome/renderer/aut...
chrome/renderer/autofill/autofill_agent.cc:610: const string16 element_attr16 =
elements[j].getAttribute("autocomplete");
don't put type hints in variable names.

https://chromiumcodereview.appspot.com/11348273/diff/7013/chrome/renderer/aut...
chrome/renderer/autofill/autofill_agent.cc:612: elements.erase(elements.begin()
+ j--);
don't erase, it is error prone

https://chromiumcodereview.appspot.com/11348273/diff/7013/chrome/renderer/aut...
chrome/renderer/autofill/autofill_agent.cc:616: std::string element_attr =
UTF16ToASCII(element_attr16);
I don't think you need to do this conversion. field.autocomplete_attribute
should already be ASCII (at least I think it's ASCII and not utf8). 

LowerCaseEqualsASCII(element_attr, field.autocomplete_attribute)

https://chromiumcodereview.appspot.com/11348273/diff/7013/chrome/renderer/aut...
chrome/renderer/autofill/autofill_agent.cc:619: autofill::FillFormField(field,
false, &elements[j]);
it is confusing that what is called field here is not what is called field in
the parameter list for FillFormField. I don't think you can use a term as
generic as |field| in autofill code, unfortunately. Better make field
form_field_data.

[Dan Beam, 2012-11-29 04:15:19]

https://chromiumcodereview.appspot.com/11348273/diff/7013/chrome/renderer/aut...
File chrome/renderer/autofill/autofill_agent.cc (right):

https://chromiumcodereview.appspot.com/11348273/diff/7013/chrome/renderer/aut...
chrome/renderer/autofill/autofill_agent.cc:610: const string16 element_attr16 =
elements[j].getAttribute("autocomplete");
On 2012/11/29 04:01:25, Evan Stade wrote:
> don't put type hints in variable names.

Done.

https://chromiumcodereview.appspot.com/11348273/diff/7013/chrome/renderer/aut...
chrome/renderer/autofill/autofill_agent.cc:612: elements.erase(elements.begin()
+ j--);
On 2012/11/29 04:01:25, Evan Stade wrote:
> don't erase, it is error prone

Done.

https://chromiumcodereview.appspot.com/11348273/diff/7013/chrome/renderer/aut...
chrome/renderer/autofill/autofill_agent.cc:616: std::string element_attr =
UTF16ToASCII(element_attr16);
On 2012/11/29 04:01:25, Evan Stade wrote:
> I don't think you need to do this conversion. field.autocomplete_attribute
> should already be ASCII (at least I think it's ASCII and not utf8). 
> 
> LowerCaseEqualsASCII(element_attr, field.autocomplete_attribute)

Done.

https://chromiumcodereview.appspot.com/11348273/diff/7013/chrome/renderer/aut...
chrome/renderer/autofill/autofill_agent.cc:619: autofill::FillFormField(field,
false, &elements[j]);
On 2012/11/29 04:01:25, Evan Stade wrote:
> it is confusing that what is called field here is not what is called field in
> the parameter list for FillFormField. I don't think you can use a term as
> generic as |field| in autofill code, unfortunately. Better make field
> form_field_data.

Done.

[Ilya Sherman, 2012-11-29 08:24:01]

https://chromiumcodereview.appspot.com/11348273/diff/1/chrome/renderer/autofi...
File chrome/renderer/autofill/autofill_agent.cc (right):

https://chromiumcodereview.appspot.com/11348273/diff/1/chrome/renderer/autofi...
chrome/renderer/autofill/autofill_agent.cc:621: }
On 2012/11/29 00:51:13, Dan Beam wrote:
> On 2012/11/29 00:08:59, Ilya Sherman wrote:
> > On 2012/11/28 19:59:10, Dan Beam wrote:
> > > On 2012/11/28 06:16:39, Ilya Sherman wrote:
> > > > Any reason not to just re-use FillForm() from form_autofill_util?
> > > 
> > > That's really similar, but
> > > a) how does this handle multiple elements with the same [autocomplete]
> > > attribute? estade@ and I envisioned only filling the first one with the
same
> > > type.
> > 
> > FillForm() would fill all of them.  What's the reason to only fill the first
> one
> > if the website asks for the info multiple times?
> > 
> > > b) it seems this won't work on non-focusable elements (i.e. [hidden]
ones)?
> > 
> > That's true, but we could add a parameter to control that.  We presumably
> still
> > don't want to fill disabled or read-only fields, though.
> > 
> > > Also, just using FillFormField() might be a middle ground.  I've added
that
> > > method to the form_autofill_util.h just for now until we decide.
> > 
> > I think it would be best to re-use as much of the logic from the
> > ForEachMatchingFormField() method as possible; but if that proves to be too
> much
> > of a PITA for whatever reason, using FillFormField() is definitely better
than
> > doing everything from scratch :)
> 
> ForEachMatchingFormField also ignores select[autocomplete] at the moment -
> should I remove this or is it still valid?
>
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/renderer/autofill/f...

I think you're misreading that code -- it ignores autocomplete="off" (which
might be specified on the form rather than on the field), so it simply always
treats <select> elements as potentially fillable.  It would be nice to fix that
bug, but it shouldn't affect your ability to fill the field here.  Did you test
it and find that it wasn't working?

https://chromiumcodereview.appspot.com/11348273/diff/3006/chrome/common/autof...
File chrome/common/autofill_messages.h (right):

https://chromiumcodereview.appspot.com/11348273/diff/3006/chrome/common/autof...
chrome/common/autofill_messages.h:137: // Sent when interactive autocomplete
succeeds. Tells the renderer to Autofill
nit: s/Autofill/fill (Autofill is the name of the feature; fill is the verb for
filling the form.  When "autofill" is used as a verb, it is lowercased, since in
that case it's not referring to the feature name.)

https://chromiumcodereview.appspot.com/11348273/diff/3006/chrome/common/autof...
chrome/common/autofill_messages.h:145: // autocomplete attributes.
nit: The second sentence in this comment is likely to grow stale quite quickly;
hence, I'd recommend simply omitting it here.

https://chromiumcodereview.appspot.com/11348273/diff/3006/chrome/renderer/aut...
File chrome/renderer/autofill/autofill_agent.cc (right):

https://chromiumcodereview.appspot.com/11348273/diff/3006/chrome/renderer/aut...
chrome/renderer/autofill/autofill_agent.cc:597: // TODO(dbeam): handle
<select>s?
What's part of the current code fails to handle <select> elements properly?

https://chromiumcodereview.appspot.com/11348273/diff/3006/chrome/renderer/aut...
chrome/renderer/autofill/autofill_agent.cc:612:
form_field_data.autocomplete_attribute.c_str())) {
Hmm, is form_field_data.autocomplete_attribute actually guaranteed to be in
lowercase?  In fact, why are we doing the comparison in lowercase?  Shouldn't
they just match exactly, case and all?

https://chromiumcodereview.appspot.com/11348273/diff/3006/chrome/renderer/aut...
chrome/renderer/autofill/autofill_agent.cc:614: break;
Why break out of the loop here?

https://chromiumcodereview.appspot.com/11348273/diff/3006/chrome/renderer/aut...
chrome/renderer/autofill/autofill_agent.cc:617: }
If you decide to keep this logic, please move it into form_autofill_util and add
test coverage for it.

[Evan Stade, 2012-11-29 18:54:07]

lgtm pending Ilya's review.

https://chromiumcodereview.appspot.com/11348273/diff/3006/chrome/renderer/aut...
File chrome/renderer/autofill/form_autofill_util.h (right):

https://chromiumcodereview.appspot.com/11348273/diff/3006/chrome/renderer/aut...
chrome/renderer/autofill/form_autofill_util.h:101: void FillFormField(const
FormFieldData& data,
move the docs in the .cc to here

[Dan Beam, 2012-11-30 00:23:15]

isherman@: I've made the changes we talked about wrt to removing my custom form
filling logic and just reusing autofill::FillForm, ptal.

[Ilya Sherman, 2012-11-30 01:26:54]

https://chromiumcodereview.appspot.com/11348273/diff/15003/chrome/renderer/au...
File chrome/renderer/autofill/autofill_agent.cc (right):

https://chromiumcodereview.appspot.com/11348273/diff/15003/chrome/renderer/au...
chrome/renderer/autofill/autofill_agent.cc:599: }
nit: Please move this function so that it has the same order in both the header
and the implementation w.r.t. the other functions.  (You can either move it here
or in the header, whichever you prefer.)

https://chromiumcodereview.appspot.com/11348273/diff/15003/chrome/renderer/au...
File chrome/renderer/autofill/form_autofill_util.cc (right):

https://chromiumcodereview.appspot.com/11348273/diff/15003/chrome/renderer/au...
chrome/renderer/autofill/form_autofill_util.cc:469: !element->isFocusable())
We'll need to add a parameter to this method to ignore isFocusable() for data
being filled from requestAutocomplete() -- right?

https://chromiumcodereview.appspot.com/11348273/diff/15003/chrome/renderer/au...
File chrome/renderer/autofill/form_autofill_util.h (right):

https://chromiumcodereview.appspot.com/11348273/diff/15003/chrome/renderer/au...
chrome/renderer/autofill/form_autofill_util.h:102: void FillForm(const FormData&
form_data,
nit: Overloaded functions are banned by the style guide, so you'll need to
change the name of either this function or the one on line 98.  I suggest
renaming this one to "FillFormForRequestAutocomplete" or something along those
lines.

[Dan Beam, 2012-11-30 03:01:15]

https://chromiumcodereview.appspot.com/11348273/diff/15003/chrome/renderer/au...
File chrome/renderer/autofill/form_autofill_util.cc (right):

https://chromiumcodereview.appspot.com/11348273/diff/15003/chrome/renderer/au...
chrome/renderer/autofill/form_autofill_util.cc:469: !element->isFocusable())
On 2012/11/30 01:26:54, Ilya Sherman wrote:
> We'll need to add a parameter to this method to ignore isFocusable() for data
> being filled from requestAutocomplete() -- right?

Done.

https://chromiumcodereview.appspot.com/11348273/diff/15003/chrome/renderer/au...
File chrome/renderer/autofill/form_autofill_util.h (right):

https://chromiumcodereview.appspot.com/11348273/diff/15003/chrome/renderer/au...
chrome/renderer/autofill/form_autofill_util.h:102: void FillForm(const FormData&
form_data,
On 2012/11/30 01:26:54, Ilya Sherman wrote:
> nit: Overloaded functions are banned by the style guide, so you'll need to
> change the name of either this function or the one on line 98.  I suggest
> renaming this one to "FillFormForRequestAutocomplete" or something along those
> lines.

Done.

[Ilya Sherman, 2012-11-30 04:04:54]

Hopefully last few nits:

https://chromiumcodereview.appspot.com/11348273/diff/5009/chrome/renderer/aut...
File chrome/renderer/autofill/form_autofill_util.cc (right):

https://chromiumcodereview.appspot.com/11348273/diff/5009/chrome/renderer/aut...
chrome/renderer/autofill/form_autofill_util.cc:49: using
autofill::RequirementsMask;
Do these lines actually compile?  I didn't think this was legal syntax...

https://chromiumcodereview.appspot.com/11348273/diff/5009/chrome/renderer/aut...
File chrome/renderer/autofill/form_autofill_util.h (right):

https://chromiumcodereview.appspot.com/11348273/diff/5009/chrome/renderer/aut...
chrome/renderer/autofill/form_autofill_util.h:27: REQUIRE_FOCUSABLE    = 1 << 1,
 // Require that a form element be focusable.
Please add this to a separate enum, as it doesn't make sense in quite the same
contexts where the RequirementsMask makes sense.  It's also an implementation
detail, so there's no need to expose it in the header.

https://chromiumcodereview.appspot.com/11348273/diff/5009/chrome/renderer/aut...
chrome/renderer/autofill/form_autofill_util.h:104: void
FillFormAndNonFocusableElements(
nit: Perhaps "including" rather than "and"?  The non-focusable elements are part
of the form, rather than separate from it.

[Dan Beam, 2012-11-30 04:13:33]

https://chromiumcodereview.appspot.com/11348273/diff/5009/chrome/renderer/aut...
File chrome/renderer/autofill/form_autofill_util.cc (right):

https://chromiumcodereview.appspot.com/11348273/diff/5009/chrome/renderer/aut...
chrome/renderer/autofill/form_autofill_util.cc:49: using
autofill::RequirementsMask;
On 2012/11/30 04:04:54, Ilya Sherman wrote:
> Do these lines actually compile?  I didn't think this was legal syntax...

Yes, at least with g++.

https://chromiumcodereview.appspot.com/11348273/diff/5009/chrome/renderer/aut...
File chrome/renderer/autofill/form_autofill_util.h (right):

https://chromiumcodereview.appspot.com/11348273/diff/5009/chrome/renderer/aut...
chrome/renderer/autofill/form_autofill_util.h:27: REQUIRE_FOCUSABLE    = 1 << 1,
 // Require that a form element be focusable.
On 2012/11/30 04:04:54, Ilya Sherman wrote:
> Please add this to a separate enum, as it doesn't make sense in quite the same
> contexts where the RequirementsMask makes sense.  It's also an implementation
> detail, so there's no need to expose it in the header.

changed to bool

https://chromiumcodereview.appspot.com/11348273/diff/5009/chrome/renderer/aut...
chrome/renderer/autofill/form_autofill_util.h:104: void
FillFormAndNonFocusableElements(
On 2012/11/30 04:04:54, Ilya Sherman wrote:
> nit: Perhaps "including" rather than "and"?  The non-focusable elements are
part
> of the form, rather than separate from it.

Done.

[Ilya Sherman, 2012-11-30 04:30:47]

LGTM, thanks :)

[Dan Beam, 2012-11-30 22:16:51]

+jhawkins@ for chrome/common/

[James Hawkins, 2012-11-30 22:17:24]

LGTM with optional nit.

https://chromiumcodereview.appspot.com/11348273/diff/12013/chrome/common/auto...
File chrome/common/autofill_messages.h (right):

https://chromiumcodereview.appspot.com/11348273/diff/12013/chrome/common/auto...
chrome/common/autofill_messages.h:140:
IPC_MESSAGE_ROUTED1(AutofillMsg_RequestAutocompleteSuccess,
Optional nit: Consider renaming the message to
AutofillMsg_InteractiveAutocompleteSuccess to match the comment (or update the
comment).  Same below.

[Dan Beam, 2012-11-30 22:21:04]

https://chromiumcodereview.appspot.com/11348273/diff/12013/chrome/common/auto...
File chrome/common/autofill_messages.h (right):

https://chromiumcodereview.appspot.com/11348273/diff/12013/chrome/common/auto...
chrome/common/autofill_messages.h:140:
IPC_MESSAGE_ROUTED1(AutofillMsg_RequestAutocompleteSuccess,
On 2012/11/30 22:17:25, James Hawkins wrote:
> Optional nit: Consider renaming the message to
> AutofillMsg_InteractiveAutocompleteSuccess to match the comment (or update the
> comment).  Same below.

Done. (changed comment)

[Dan Beam, 2012-11-30 22:22:54]

TBR'ing jschuh@ for chrome/common/autofill_message.h