Add support for autofilling radio buttons and checkboxes.

Add support for autofilling radio buttons and checkboxes.

Autofill server is equiped with provision to say a field type as
FIELD_WITH_DEFAULT_VALUE and specify what value to use by default it
client wants to fill it. For radio buttons, if the default value
specified by the autofill server is same as its value, Chrome checks that
input element.

all changes are behind switch.

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

[Raman Kakilate, 2012-11-30 02:19:57]

Updated description with the bug number.

[Albert Bodenhamer, 2012-11-30 19:01:08]

This looks straightforward but Ilya knows this code much better than I do.  I'd
like him to take at least a quick look before I approve.

"Chrome" is misspelled as "chome" in description.

https://codereview.chromium.org/11415221/diff/1/chrome/browser/autofill/autof...
File chrome/browser/autofill/autofill_xml_parser.cc (right):

https://codereview.chromium.org/11415221/diff/1/chrome/browser/autofill/autof...
chrome/browser/autofill/autofill_xml_parser.cc:92: if (field_type ==
FIELD_WITH_DEFAULT_VALUE && attrs[2]) {
As the list of attributes gets longer the magic numbers for indexing |attrs|
become less clear.

Consider creating a enum for this?

https://codereview.chromium.org/11415221/diff/1/chrome/browser/autofill/autof...
File chrome/browser/autofill/autofill_xml_parser.h (right):

https://codereview.chromium.org/11415221/diff/1/chrome/browser/autofill/autof...
chrome/browser/autofill/autofill_xml_parser.h:70:
std::vector<AutofillFieldInfo>* field_infos,
Does the indent need to change here?

[Raman Kakilate, 2012-11-30 22:36:52]

Fixed the description.

https://codereview.chromium.org/11415221/diff/1/chrome/browser/autofill/autof...
File chrome/browser/autofill/autofill_xml_parser.cc (right):

https://codereview.chromium.org/11415221/diff/1/chrome/browser/autofill/autof...
chrome/browser/autofill/autofill_xml_parser.cc:92: if (field_type ==
FIELD_WITH_DEFAULT_VALUE && attrs[2]) {
On 2012/11/30 19:01:08, Albert Bodenhamer wrote:
> As the list of attributes gets longer the magic numbers for indexing |attrs|
> become less clear.
> 
> Consider creating a enum for this?

ATTRIBUTE_1_NAME, ATTRIBUTE_1_VALUE, ATTRIBUTE_2_NAME, ATTRIBUTE_2_VALUE are the
names I could come up with and I am not liking them.

https://codereview.chromium.org/11415221/diff/1/chrome/browser/autofill/autof...
File chrome/browser/autofill/autofill_xml_parser.h (right):

https://codereview.chromium.org/11415221/diff/1/chrome/browser/autofill/autof...
chrome/browser/autofill/autofill_xml_parser.h:70:
std::vector<AutofillFieldInfo>* field_infos,
On 2012/11/30 19:01:08, Albert Bodenhamer wrote:
> Does the indent need to change here?

Done.

[Ilya Sherman, 2012-12-01 00:54:12]

Thanks, looks pretty on track at a high level :)

https://codereview.chromium.org/11415221/diff/1/chrome/browser/autofill/autof...
File chrome/browser/autofill/autofill_xml_parser.cc (right):

https://codereview.chromium.org/11415221/diff/1/chrome/browser/autofill/autof...
chrome/browser/autofill/autofill_xml_parser.cc:92: if (field_type ==
FIELD_WITH_DEFAULT_VALUE && attrs[2]) {
On 2012/11/30 22:36:53, Raman Kakilate wrote:
> On 2012/11/30 19:01:08, Albert Bodenhamer wrote:
> > As the list of attributes gets longer the magic numbers for indexing |attrs|
> > become less clear.
> > 
> > Consider creating a enum for this?
> 
> ATTRIBUTE_1_NAME, ATTRIBUTE_1_VALUE, ATTRIBUTE_2_NAME, ATTRIBUTE_2_VALUE are
the
> names I could come up with and I am not liking them.

How about names like ATTRIBUTE_NAME_DEFAULT_FIELD_VALUE,
ATTRIBUTE_VALUE_DEFAULT_FIELD_VALUE?

https://codereview.chromium.org/11415221/diff/4001/chrome/browser/autofill/au...
File chrome/browser/autofill/autofill_field.h (right):

https://codereview.chromium.org/11415221/diff/4001/chrome/browser/autofill/au...
chrome/browser/autofill/autofill_field.h:82: // Used to park the default value
returned by the autofill server.
nit: "autofill" -> "Autofill"

https://codereview.chromium.org/11415221/diff/4001/chrome/browser/autofill/au...
File chrome/browser/autofill/autofill_manager.cc (right):

https://codereview.chromium.org/11415221/diff/4001/chrome/browser/autofill/au...
chrome/browser/autofill/autofill_manager.cc:653: if (cached_field->type() ==
FIELD_WITH_DEFAULT_VALUE &&
nit: Combine this with the else on line 651, as in "else if (...) {"

https://codereview.chromium.org/11415221/diff/4001/chrome/browser/autofill/au...
chrome/browser/autofill/autofill_manager.cc:655: string16 default_value =
ASCIIToUTF16(cached_field->default_value());
How do you know that the cached_field's default value is ASCII?

https://codereview.chromium.org/11415221/diff/4001/chrome/browser/autofill/au...
chrome/browser/autofill/autofill_manager.cc:656: result.fields[i].is_checked =
(default_value == result.fields[i].value);
I'm confused by this line.  Why isn't it just "result.fields[i].value =
default_value"?

https://codereview.chromium.org/11415221/diff/4001/chrome/browser/autofill/au...
chrome/browser/autofill/autofill_manager.cc:657:
form_structure->field(i)->is_autofilled = true;
nit: Please reproduce the comment from line 648-649 here, assuming that's why we
need this line.

https://codereview.chromium.org/11415221/diff/4001/chrome/browser/autofill/au...
File chrome/browser/autofill/autofill_manager_unittest.cc (right):

https://codereview.chromium.org/11415221/diff/4001/chrome/browser/autofill/au...
chrome/browser/autofill/autofill_manager_unittest.cc:1858: }
Please also add a test where the field should end up un-checked.

https://codereview.chromium.org/11415221/diff/4001/chrome/browser/autofill/au...
File chrome/browser/autofill/autofill_xml_parser.cc (right):

https://codereview.chromium.org/11415221/diff/4001/chrome/browser/autofill/au...
chrome/browser/autofill/autofill_xml_parser.cc:97: }
nit: No need for curly braces

https://codereview.chromium.org/11415221/diff/4001/chrome/browser/autofill/au...
chrome/browser/autofill/autofill_xml_parser.cc:101:
field_infos_->push_back(AutofillFieldInfo(field_type, default_value));
Please create a struct named AutofillServerFieldInfo instead of typedef'ing
AutofillFieldInfo as an std::pair<>.  A struct allows for meaningful names for
the members, rather than .first and .second.

https://codereview.chromium.org/11415221/diff/4001/chrome/browser/autofill/fi...
File chrome/browser/autofill/field_types.h (right):

https://codereview.chromium.org/11415221/diff/4001/chrome/browser/autofill/fi...
chrome/browser/autofill/field_types.h:82: FIELD_WITH_DEFAULT_VALUE = 61,
Has the server-side change adding this field type already landed?

https://codereview.chromium.org/11415221/diff/4001/chrome/browser/autofill/fi...
chrome/browser/autofill/field_types.h:82: FIELD_WITH_DEFAULT_VALUE = 61,
You should update the if-stmt in AutofillField::set_heuristic_type() to disallow
setting this field type as a heuristic type.

https://codereview.chromium.org/11415221/diff/4001/chrome/browser/autofill/fi...
chrome/browser/autofill/field_types.h:90: typedef std::pair<AutofillFieldType,
std::string> AutofillFieldInfo;
Please don't add this typedef here.  Spell out this type in the files that need
it.

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

https://codereview.chromium.org/11415221/diff/4001/chrome/browser/autofill/fo...
chrome/browser/autofill/form_structure.cc:236: // interfere with existing field
signatures with autofill servers.
nit: "autofill" -> "Autofill"

https://codereview.chromium.org/11415221/diff/4001/chrome/browser/autofill/fo...
chrome/browser/autofill/form_structure.cc:893: // Skip putting checkable fields
in the request flag is not set.
nit: "in the request flag is not set" -> "in the request if the flag is not set"

https://codereview.chromium.org/11415221/diff/4001/chrome/browser/autofill/fo...
File chrome/browser/autofill/form_structure_unittest.cc (right):

https://codereview.chromium.org/11415221/diff/4001/chrome/browser/autofill/fo...
chrome/browser/autofill/form_structure_unittest.cc:1549: EXPECT_EQ(kResponse3,
encoded_xml);
Please also add a similar test within EncodeUploadRequest

https://codereview.chromium.org/11415221/diff/4001/chrome/common/form_field_d...
File chrome/common/form_field_data.h (right):

https://codereview.chromium.org/11415221/diff/4001/chrome/common/form_field_d...
chrome/common/form_field_data.h:33: bool is_checked;
Can you just re-use value for this, or do checkboxes not have a value()
attribute?

https://codereview.chromium.org/11415221/diff/4001/chrome/renderer/autofill/f...
File chrome/renderer/autofill/form_autofill_util.cc (right):

https://codereview.chromium.org/11415221/diff/4001/chrome/renderer/autofill/f...
chrome/renderer/autofill/form_autofill_util.cc:468:
!input_element->value().isEmpty())
nit: Let's just move this if-stmt to replace the outer one -- I'm not sure why
this was previously structured as an if-stmt nested in another one.

https://codereview.chromium.org/11415221/diff/4001/chrome/renderer/autofill/f...
chrome/renderer/autofill/form_autofill_util.cc:527: // setSuggestedCheckedValue
in WebInputElement.
nit: Please combine this comment with the one on line 524.

https://codereview.chromium.org/11415221/diff/4001/chrome/renderer/autofill/f...
chrome/renderer/autofill/form_autofill_util.cc:883: if
(!IsAutofillableInputElement(input_element))
Given the comment on line 881, I would expect this to remain IsTextInput()

https://codereview.chromium.org/11415221/diff/4001/chrome/renderer/autofill/f...
File chrome/renderer/autofill/form_cache.cc (right):

https://codereview.chromium.org/11415221/diff/4001/chrome/renderer/autofill/f...
chrome/renderer/autofill/form_cache.cc:167: }
nit: It seems like this code should be decomposed into a (templatized?) helper
function.

https://codereview.chromium.org/11415221/diff/4001/chrome/renderer/autofill/f...
chrome/renderer/autofill/form_cache.cc:205: } else {
nit: Please DCHECK that the type is a radio or checkbox element.

https://codereview.chromium.org/11415221/diff/4001/chrome/renderer/autofill/f...
chrome/renderer/autofill/form_cache.cc:208: std::make_pair(input_element,
input_element.isChecked()));
Lines 207-208 don't look like they belong.  Am I misunderstanding the code?

https://codereview.chromium.org/11415221/diff/4001/chrome/renderer/autofill/f...
chrome/renderer/autofill/form_cache.cc:210: it =
initial_checked_state_.find(input_element);
nit: "it =" belongs on the previous line, since it will fit.

[Raman Kakilate, 2012-12-06 01:54:05]

https://codereview.chromium.org/11415221/diff/1/chrome/browser/autofill/autof...
File chrome/browser/autofill/autofill_xml_parser.cc (right):

https://codereview.chromium.org/11415221/diff/1/chrome/browser/autofill/autof...
chrome/browser/autofill/autofill_xml_parser.cc:92: if (field_type ==
FIELD_WITH_DEFAULT_VALUE && attrs[2]) {
On 2012/12/01 00:54:12, Ilya Sherman wrote:
> On 2012/11/30 22:36:53, Raman Kakilate wrote:
> > On 2012/11/30 19:01:08, Albert Bodenhamer wrote:
> > > As the list of attributes gets longer the magic numbers for indexing
|attrs|
> > > become less clear.
> > > 
> > > Consider creating a enum for this?
> > 
> > ATTRIBUTE_1_NAME, ATTRIBUTE_1_VALUE, ATTRIBUTE_2_NAME, ATTRIBUTE_2_VALUE are
> the
> > names I could come up with and I am not liking them.
> 
> How about names like ATTRIBUTE_NAME_DEFAULT_FIELD_VALUE,
> ATTRIBUTE_VALUE_DEFAULT_FIELD_VALUE?

changed the implementation to loop over attribute and value pairs. Similar to
how we do for upload element.

https://codereview.chromium.org/11415221/diff/4001/chrome/browser/autofill/au...
File chrome/browser/autofill/autofill_field.h (right):

https://codereview.chromium.org/11415221/diff/4001/chrome/browser/autofill/au...
chrome/browser/autofill/autofill_field.h:82: // Used to park the default value
returned by the autofill server.
On 2012/12/01 00:54:12, Ilya Sherman wrote:
> nit: "autofill" -> "Autofill"

Done.

https://codereview.chromium.org/11415221/diff/4001/chrome/browser/autofill/au...
File chrome/browser/autofill/autofill_manager.cc (right):

https://codereview.chromium.org/11415221/diff/4001/chrome/browser/autofill/au...
chrome/browser/autofill/autofill_manager.cc:653: if (cached_field->type() ==
FIELD_WITH_DEFAULT_VALUE &&
On 2012/12/01 00:54:12, Ilya Sherman wrote:
> nit: Combine this with the else on line 651, as in "else if (...) {"

Done.

https://codereview.chromium.org/11415221/diff/4001/chrome/browser/autofill/au...
chrome/browser/autofill/autofill_manager.cc:655: string16 default_value =
ASCIIToUTF16(cached_field->default_value());
On 2012/12/01 00:54:12, Ilya Sherman wrote:
> How do you know that the cached_field's default value is ASCII?

Changed it to UTF8ToUTF16. I am not entirely sure if this is good enough. Do you
know if the XML response from autofill server is UTF8 encoded ?

https://codereview.chromium.org/11415221/diff/4001/chrome/browser/autofill/au...
chrome/browser/autofill/autofill_manager.cc:656: result.fields[i].is_checked =
(default_value == result.fields[i].value);
On 2012/12/01 00:54:12, Ilya Sherman wrote:
> I'm confused by this line.  Why isn't it just "result.fields[i].value =
> default_value"?

as I understand, for checkboxes and radio buttons - a user cannot change the
values of them, only thing user controls is its checked state.

Idea is maintain default values to mark checked in the autofill server. When the
client sees the values to be checked, client checks if the values of the field
is same as default value, if yes it marks as checked. else, un-checks it.

https://codereview.chromium.org/11415221/diff/4001/chrome/browser/autofill/au...
chrome/browser/autofill/autofill_manager.cc:657:
form_structure->field(i)->is_autofilled = true;
On 2012/12/01 00:54:12, Ilya Sherman wrote:
> nit: Please reproduce the comment from line 648-649 here, assuming that's why
we
> need this line.

Done.

https://codereview.chromium.org/11415221/diff/4001/chrome/browser/autofill/au...
File chrome/browser/autofill/autofill_manager_unittest.cc (right):

https://codereview.chromium.org/11415221/diff/4001/chrome/browser/autofill/au...
chrome/browser/autofill/autofill_manager_unittest.cc:1858: }
On 2012/12/01 00:54:12, Ilya Sherman wrote:
> Please also add a test where the field should end up un-checked.

Done.

https://codereview.chromium.org/11415221/diff/4001/chrome/browser/autofill/au...
File chrome/browser/autofill/autofill_xml_parser.cc (right):

https://codereview.chromium.org/11415221/diff/4001/chrome/browser/autofill/au...
chrome/browser/autofill/autofill_xml_parser.cc:97: }
On 2012/12/01 00:54:12, Ilya Sherman wrote:
> nit: No need for curly braces

Done.

https://codereview.chromium.org/11415221/diff/4001/chrome/browser/autofill/au...
chrome/browser/autofill/autofill_xml_parser.cc:101:
field_infos_->push_back(AutofillFieldInfo(field_type, default_value));
On 2012/12/01 00:54:12, Ilya Sherman wrote:
> Please create a struct named AutofillServerFieldInfo instead of typedef'ing
> AutofillFieldInfo as an std::pair<>.  A struct allows for meaningful names for
> the members, rather than .first and .second.

Done. Added struct for AutofillServerFieldInfo

https://codereview.chromium.org/11415221/diff/4001/chrome/browser/autofill/fi...
File chrome/browser/autofill/field_types.h (right):

https://codereview.chromium.org/11415221/diff/4001/chrome/browser/autofill/fi...
chrome/browser/autofill/field_types.h:82: FIELD_WITH_DEFAULT_VALUE = 61,
On 2012/12/01 00:54:12, Ilya Sherman wrote:
> Has the server-side change adding this field type already landed?

Yes, they are in.

https://codereview.chromium.org/11415221/diff/4001/chrome/browser/autofill/fi...
chrome/browser/autofill/field_types.h:82: FIELD_WITH_DEFAULT_VALUE = 61,
On 2012/12/01 00:54:12, Ilya Sherman wrote:
> You should update the if-stmt in AutofillField::set_heuristic_type() to
disallow
> setting this field type as a heuristic type.

Done.

https://codereview.chromium.org/11415221/diff/4001/chrome/browser/autofill/fi...
chrome/browser/autofill/field_types.h:90: typedef std::pair<AutofillFieldType,
std::string> AutofillFieldInfo;
On 2012/12/01 00:54:12, Ilya Sherman wrote:
> Please don't add this typedef here.  Spell out this type in the files that
need
> it.
Removed and created a struct for AutofillServerFieldType.

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

https://codereview.chromium.org/11415221/diff/4001/chrome/browser/autofill/fo...
chrome/browser/autofill/form_structure.cc:236: // interfere with existing field
signatures with autofill servers.
On 2012/12/01 00:54:12, Ilya Sherman wrote:
> nit: "autofill" -> "Autofill"

Done.

https://codereview.chromium.org/11415221/diff/4001/chrome/browser/autofill/fo...
chrome/browser/autofill/form_structure.cc:893: // Skip putting checkable fields
in the request flag is not set.
On 2012/12/01 00:54:12, Ilya Sherman wrote:
> nit: "in the request flag is not set" -> "in the request if the flag is not
set"

Done.

https://codereview.chromium.org/11415221/diff/4001/chrome/browser/autofill/fo...
File chrome/browser/autofill/form_structure_unittest.cc (right):

https://codereview.chromium.org/11415221/diff/4001/chrome/browser/autofill/fo...
chrome/browser/autofill/form_structure_unittest.cc:1549: EXPECT_EQ(kResponse3,
encoded_xml);
On 2012/12/01 00:54:12, Ilya Sherman wrote:
> Please also add a similar test within EncodeUploadRequest

Done. and fixed the code to not consider checkable elements in upload request.

https://codereview.chromium.org/11415221/diff/4001/chrome/common/form_field_d...
File chrome/common/form_field_data.h (right):

https://codereview.chromium.org/11415221/diff/4001/chrome/common/form_field_d...
chrome/common/form_field_data.h:33: bool is_checked;
On 2012/12/01 00:54:12, Ilya Sherman wrote:
> Can you just re-use value for this, or do checkboxes not have a value()
> attribute?

"value" is not controlled by the user, while "checked" state is.

https://codereview.chromium.org/11415221/diff/4001/chrome/renderer/autofill/f...
File chrome/renderer/autofill/form_autofill_util.cc (right):

https://codereview.chromium.org/11415221/diff/4001/chrome/renderer/autofill/f...
chrome/renderer/autofill/form_autofill_util.cc:468:
!input_element->value().isEmpty())
On 2012/12/01 00:54:12, Ilya Sherman wrote:
> nit: Let's just move this if-stmt to replace the outer one -- I'm not sure why
> this was previously structured as an if-stmt nested in another one.

Done.

https://codereview.chromium.org/11415221/diff/4001/chrome/renderer/autofill/f...
chrome/renderer/autofill/form_autofill_util.cc:527: // setSuggestedCheckedValue
in WebInputElement.
On 2012/12/01 00:54:12, Ilya Sherman wrote:
> nit: Please combine this comment with the one on line 524.

Done.

https://codereview.chromium.org/11415221/diff/4001/chrome/renderer/autofill/f...
chrome/renderer/autofill/form_autofill_util.cc:883: if
(!IsAutofillableInputElement(input_element))
On 2012/12/01 00:54:12, Ilya Sherman wrote:
> Given the comment on line 881, I would expect this to remain IsTextInput()

Done.

https://codereview.chromium.org/11415221/diff/4001/chrome/renderer/autofill/f...
File chrome/renderer/autofill/form_cache.cc (right):

https://codereview.chromium.org/11415221/diff/4001/chrome/renderer/autofill/f...
chrome/renderer/autofill/form_cache.cc:167: }
On 2012/12/01 00:54:12, Ilya Sherman wrote:
> nit: It seems like this code should be decomposed into a (templatized?) helper
> function.

code looks repeated 3 times, but the container objects in these 3 cases are
different.
set<WebDocument>, map<WebSelectElement, string16>, map<WebInputElement, bool>

Present code looks simpler to maintain.

https://codereview.chromium.org/11415221/diff/4001/chrome/renderer/autofill/f...
chrome/renderer/autofill/form_cache.cc:205: } else {
On 2012/12/01 00:54:12, Ilya Sherman wrote:
> nit: Please DCHECK that the type is a radio or checkbox element.

Done.

https://codereview.chromium.org/11415221/diff/4001/chrome/renderer/autofill/f...
chrome/renderer/autofill/form_cache.cc:208: std::make_pair(input_element,
input_element.isChecked()));
On 2012/12/01 00:54:12, Ilya Sherman wrote:
> Lines 207-208 don't look like they belong.  Am I misunderstanding the code?

my-bad. removed them. thanks for catching it.

https://codereview.chromium.org/11415221/diff/4001/chrome/renderer/autofill/f...
chrome/renderer/autofill/form_cache.cc:210: it =
initial_checked_state_.find(input_element);
On 2012/12/01 00:54:12, Ilya Sherman wrote:
> nit: "it =" belongs on the previous line, since it will fit.

Done.

[Albert Bodenhamer, 2012-12-06 17:39:04]

https://codereview.chromium.org/11415221/diff/16001/chrome/browser/autofill/a...
File chrome/browser/autofill/autofill_xml_parser.cc (right):

https://codereview.chromium.org/11415221/diff/16001/chrome/browser/autofill/a...
chrome/browser/autofill/autofill_xml_parser.cc:96: attrs +=2;
This is awkward.

If you're going to do C-style pointer math you should use it consistently and
not mix with array-dereference (attrs[1]) syntax.

Generally, I prefer array syntax for readability.  I'd be ok with either, but
not both.

The cleanest approach would be to look into replacing char** with something more
structured, but that's probably overkill for this CL.

[Raman Kakilate, 2012-12-06 18:06:35]

https://codereview.chromium.org/11415221/diff/16001/chrome/browser/autofill/a...
File chrome/browser/autofill/autofill_xml_parser.cc (right):

https://codereview.chromium.org/11415221/diff/16001/chrome/browser/autofill/a...
chrome/browser/autofill/autofill_xml_parser.cc:96: attrs +=2;
On 2012/12/06 17:39:05, Albert Bodenhamer wrote:
> This is awkward.
> 
> If you're going to do C-style pointer math you should use it consistently and
> not mix with array-dereference (attrs[1]) syntax.
> 
> Generally, I prefer array syntax for readability.  I'd be ok with either, but
> not both.
> 
> The cleanest approach would be to look into replacing char** with something
more
> structured, but that's probably overkill for this CL.

I was trying to be consistent with lines 55-69. I would have to change those
lines as well. 

array syntax is more rigid towards order of attributes specified in XML element 
(autofillqueryresponse element might have its attributes specified in different
order).

I can change them all to c-style pointer math if you agree.

[Albert Bodenhamer, 2012-12-06 18:58:31]

On Thu, Dec 6, 2012 at 10:06 AM, <ramankk@chromium.org> wrote:

>
> https://codereview.chromium.**org/11415221/diff/16001/**
>
chrome/browser/autofill/**autofill_xml_parser.cc<https://codereview.chromium.org/11415221/diff/16001/chrome/browser/autofill/autofill_xml_parser.cc>
> File chrome/browser/autofill/**autofill_xml_parser.cc (right):
>
> https://codereview.chromium.**org/11415221/diff/16001/**
>
chrome/browser/autofill/**autofill_xml_parser.cc#**newcode96<https://codereview.chromium.org/11415221/diff/16001/chrome/browser/autofill/autofill_xml_parser.cc#newcode96>
> chrome/browser/autofill/**autofill_xml_parser.cc:96: attrs +=2;
> On 2012/12/06 17:39:05, Albert Bodenhamer wrote:
>
>> This is awkward.
>>
>
>  If you're going to do C-style pointer math you should use it
>>
> consistently and
>
>> not mix with array-dereference (attrs[1]) syntax.
>>
>
>  Generally, I prefer array syntax for readability.  I'd be ok with
>>
> either, but
>
>> not both.
>>
>
>  The cleanest approach would be to look into replacing char** with
>>
> something more
>
>> structured, but that's probably overkill for this CL.
>>
>
> I was trying to be consistent with lines 55-69. I would have to change
> those lines as well.
>
> array syntax is more rigid towards order of attributes specified in XML
> element  (autofillqueryresponse element might have its attributes
> specified in different order).
>
> I can change them all to c-style pointer math if you agree.
>

I'm fine with either.


>
>
https://codereview.chromium.**org/11415221/<https://codereview.chromium.org/1...
>



-- 
Albert Bodenhamer | Software Engineer | abodenha@chromium.<abodenha@google.com>
org

[Raman Kakilate, 2012-12-06 22:51:47]

On 2012/12/06 18:58:31, Albert Bodenhamer wrote:
> On Thu, Dec 6, 2012 at 10:06 AM, <mailto:ramankk@chromium.org> wrote:
> 
> >
> > https://codereview.chromium.**org/11415221/diff/16001/**
> >
>
chrome/browser/autofill/**autofill_xml_parser.cc<https://codereview.chromium.org/11415221/diff/16001/chrome/browser/autofill/autofill_xml_parser.cc>
> > File chrome/browser/autofill/**autofill_xml_parser.cc (right):
> >
> > https://codereview.chromium.**org/11415221/diff/16001/**
> >
>
chrome/browser/autofill/**autofill_xml_parser.cc#**newcode96<https://codereview.chromium.org/11415221/diff/16001/chrome/browser/autofill/autofill_xml_parser.cc#newcode96>
> > chrome/browser/autofill/**autofill_xml_parser.cc:96: attrs +=2;
> > On 2012/12/06 17:39:05, Albert Bodenhamer wrote:
> >
> >> This is awkward.
> >>
> >
> >  If you're going to do C-style pointer math you should use it
> >>
> > consistently and
> >
> >> not mix with array-dereference (attrs[1]) syntax.
> >>
> >
> >  Generally, I prefer array syntax for readability.  I'd be ok with
> >>
> > either, but
> >
> >> not both.
> >>
> >
> >  The cleanest approach would be to look into replacing char** with
> >>
> > something more
> >
> >> structured, but that's probably overkill for this CL.
> >>
> >
> > I was trying to be consistent with lines 55-69. I would have to change
> > those lines as well.
> >
> > array syntax is more rigid towards order of attributes specified in XML
> > element  (autofillqueryresponse element might have its attributes
> > specified in different order).
> >
> > I can change them all to c-style pointer math if you agree.
> >
> 
> I'm fine with either.

Done.
> 
> 
> >
> >
>
https://codereview.chromium.**org/11415221/%3Chttps://codereview.chromium.org...>
> >
> 
> 
> 
> -- 
> Albert Bodenhamer | Software Engineer |
mailto:abodenha@chromium.<abodenha@google.com>
> org

[Albert Bodenhamer, 2012-12-06 23:15:44]

https://codereview.chromium.org/11415221/diff/16001/chrome/browser/autofill/a...
File chrome/browser/autofill/autofill_xml_parser.cc (right):

https://codereview.chromium.org/11415221/diff/16001/chrome/browser/autofill/a...
chrome/browser/autofill/autofill_xml_parser.cc:96: attrs +=2;
On 2012/12/06 18:06:35, Raman Kakilate wrote:
> On 2012/12/06 17:39:05, Albert Bodenhamer wrote:
> > This is awkward.
> > 
> > If you're going to do C-style pointer math you should use it consistently
and
> > not mix with array-dereference (attrs[1]) syntax.
> > 
> > Generally, I prefer array syntax for readability.  I'd be ok with either,
but
> > not both.
> > 
> > The cleanest approach would be to look into replacing char** with something
> more
> > structured, but that's probably overkill for this CL.
> 
> I was trying to be consistent with lines 55-69. I would have to change those
> lines as well. 
> 
> array syntax is more rigid towards order of attributes specified in XML
element 
> (autofillqueryresponse element might have its attributes specified in
different
> order).
> 
> I can change them all to c-style pointer math if you agree. 

I was thinking more like:
  while (*attrs) {
      buzz::QName attribute_qname = context->ResolveQName(*attrs, true);
      const std::string& attribute_name = attribute_qname.LocalPart();
      attrs++;
      if (attribute_name.compare("autofilltype") == 0) {
        int value = GetIntValue(context, *attrs);
        field_info.field_type = static_cast<AutofillFieldType>(value);
        if (field_info.field_type < 0 ||
            field_info.field_type > MAX_VALID_FIELD_TYPE)
          field_info.field_type = NO_SERVER_DATA;
      } else if (field_info.field_type == FIELD_WITH_DEFAULT_VALUE &&
                 attribute_name.compare("defaultvalue") == 0) {
        field_info.default_value.assign(*attrs);
      }

      attrs++;
    }

[Raman Kakilate, 2012-12-06 23:33:24]

On 2012/12/06 23:15:44, Albert Bodenhamer wrote:
>
https://codereview.chromium.org/11415221/diff/16001/chrome/browser/autofill/a...
> File chrome/browser/autofill/autofill_xml_parser.cc (right):
> 
>
https://codereview.chromium.org/11415221/diff/16001/chrome/browser/autofill/a...
> chrome/browser/autofill/autofill_xml_parser.cc:96: attrs +=2;
> On 2012/12/06 18:06:35, Raman Kakilate wrote:
> > On 2012/12/06 17:39:05, Albert Bodenhamer wrote:
> > > This is awkward.
> > > 
> > > If you're going to do C-style pointer math you should use it consistently
> and
> > > not mix with array-dereference (attrs[1]) syntax.
> > > 
> > > Generally, I prefer array syntax for readability.  I'd be ok with either,
> but
> > > not both.
> > > 
> > > The cleanest approach would be to look into replacing char** with
something
> > more
> > > structured, but that's probably overkill for this CL.
> > 
> > I was trying to be consistent with lines 55-69. I would have to change those
> > lines as well. 
> > 
> > array syntax is more rigid towards order of attributes specified in XML
> element 
> > (autofillqueryresponse element might have its attributes specified in
> different
> > order).
> > 
> > I can change them all to c-style pointer math if you agree. 
> 
> I was thinking more like:
>   while (*attrs) {
>       buzz::QName attribute_qname = context->ResolveQName(*attrs, true);
>       const std::string& attribute_name = attribute_qname.LocalPart();
>       attrs++;
>       if (attribute_name.compare("autofilltype") == 0) {
>         int value = GetIntValue(context, *attrs);
>         field_info.field_type = static_cast<AutofillFieldType>(value);
>         if (field_info.field_type < 0 ||
>             field_info.field_type > MAX_VALID_FIELD_TYPE)
>           field_info.field_type = NO_SERVER_DATA;
>       } else if (field_info.field_type == FIELD_WITH_DEFAULT_VALUE &&
>                  attribute_name.compare("defaultvalue") == 0) {
>         field_info.default_value.assign(*attrs);
>       }
> 
>       attrs++;
>     }

Sure. Done.

[Albert Bodenhamer, 2012-12-06 23:35:14]

lgtm

[Ilya Sherman, 2012-12-07 01:47:22]

https://codereview.chromium.org/11415221/diff/4001/chrome/browser/autofill/au...
File chrome/browser/autofill/autofill_manager.cc (right):

https://codereview.chromium.org/11415221/diff/4001/chrome/browser/autofill/au...
chrome/browser/autofill/autofill_manager.cc:655: string16 default_value =
ASCIIToUTF16(cached_field->default_value());
On 2012/12/06 01:54:05, Raman Kakilate wrote:
> On 2012/12/01 00:54:12, Ilya Sherman wrote:
> > How do you know that the cached_field's default value is ASCII?
> 
> Changed it to UTF8ToUTF16. I am not entirely sure if this is good enough. Do
you
> know if the XML response from autofill server is UTF8 encoded ?

I'm not certain, but it's likely.  Vadim Berman would be the best person to ask.

https://codereview.chromium.org/11415221/diff/4001/chrome/browser/autofill/au...
chrome/browser/autofill/autofill_manager.cc:656: result.fields[i].is_checked =
(default_value == result.fields[i].value);
On 2012/12/06 01:54:05, Raman Kakilate wrote:
> On 2012/12/01 00:54:12, Ilya Sherman wrote:
> > I'm confused by this line.  Why isn't it just "result.fields[i].value =
> > default_value"?
> 
> as I understand, for checkboxes and radio buttons - a user cannot change the
> values of them, only thing user controls is its checked state.
> 
> Idea is maintain default values to mark checked in the autofill server. When
the
> client sees the values to be checked, client checks if the values of the field
> is same as default value, if yes it marks as checked. else, un-checks it.

If the user can't set the value, why doesn't the server just return a boolean
for whether the field should be checked or not?  I find the current logic quite
confusing...

Moreover, if we need to keep the current logic, it seems like something that the
renderer should be responsible for, rather than having the browser need to get
involved, since the renderer is where the form actually gets filled.

https://codereview.chromium.org/11415221/diff/4001/chrome/renderer/autofill/f...
File chrome/renderer/autofill/form_cache.cc (right):

https://codereview.chromium.org/11415221/diff/4001/chrome/renderer/autofill/f...
chrome/renderer/autofill/form_cache.cc:167: }
On 2012/12/06 01:54:05, Raman Kakilate wrote:
> On 2012/12/01 00:54:12, Ilya Sherman wrote:
> > nit: It seems like this code should be decomposed into a (templatized?)
helper
> > function.
> 
> code looks repeated 3 times, but the container objects in these 3 cases are
> different.
> set<WebDocument>, map<WebSelectElement, string16>, map<WebInputElement, bool>
> 
> Present code looks simpler to maintain.

I agree that the set is different, but the two maps are pretty much identical. 
I don't see any reason why a templatized helper method would be harder to
maintain.  What am I missing?

https://codereview.chromium.org/11415221/diff/23022/chrome/browser/autofill/a...
File chrome/browser/autofill/autofill_manager_unittest.cc (right):

https://codereview.chromium.org/11415221/diff/23022/chrome/browser/autofill/a...
chrome/browser/autofill/autofill_manager_unittest.cc:1828: "Checkbox",
"checkbx", "dont-fill-me", "checkbox", &field);
nit: I think the test code would be clearer if this string were "fill-me", and
the one on line 1850 were "dont-fill-me"

https://codereview.chromium.org/11415221/diff/23022/chrome/browser/autofill/a...
chrome/browser/autofill/autofill_manager_unittest.cc:1861: // Last but one field
should be checked.
nit: "Last but one" -> "Second to last"

https://codereview.chromium.org/11415221/diff/23022/chrome/browser/autofill/a...
chrome/browser/autofill/autofill_manager_unittest.cc:1863: // Last field
shoutn't be checked as values are not same.
nit: "shoutn't" -> "shouldn't"

https://codereview.chromium.org/11415221/diff/23022/chrome/browser/autofill/a...
File chrome/browser/autofill/autofill_server_field_info.h (right):

https://codereview.chromium.org/11415221/diff/23022/chrome/browser/autofill/a...
chrome/browser/autofill/autofill_server_field_info.h:11: #include
"base/string16.h"
nit: It doesn't look like either of these headers is used.

https://codereview.chromium.org/11415221/diff/23022/chrome/browser/autofill/a...
chrome/browser/autofill/autofill_server_field_info.h:15: // Autofill Field Type.
nit: This comment is just repeating the type name.  Instead, how about something
like "The predicted type returned by the Autofill server for this field."

https://codereview.chromium.org/11415221/diff/23022/chrome/browser/autofill/a...
chrome/browser/autofill/autofill_server_field_info.h:17: // Default value for
the field
nit: Please mention that the default value only applies when the field_type is
FIELD_WITH_DEFAULT_VALUE".  Also, please end the sentence with a period.

https://codereview.chromium.org/11415221/diff/23022/chrome/browser/autofill/a...
File chrome/browser/autofill/autofill_xml_parser.cc (right):

https://codereview.chromium.org/11415221/diff/23022/chrome/browser/autofill/a...
chrome/browser/autofill/autofill_xml_parser.cc:54: // |attrs| is a
NULL-terminated list of (attribute, value) pairs.
nit: Please add a similar comment above line 81 as well.

https://codereview.chromium.org/11415221/diff/23022/chrome/browser/autofill/a...
chrome/browser/autofill/autofill_xml_parser.cc:57: attrs++;
nit: ++attrs

https://codereview.chromium.org/11415221/diff/23022/chrome/browser/autofill/a...
chrome/browser/autofill/autofill_xml_parser.cc:67: attrs ++;
nit: ++attrs

https://codereview.chromium.org/11415221/diff/23022/chrome/browser/autofill/a...
chrome/browser/autofill/autofill_xml_parser.cc:70: if (!attrs[0]) {
nit: *attrs

https://codereview.chromium.org/11415221/diff/23022/chrome/browser/autofill/a...
chrome/browser/autofill/autofill_xml_parser.cc:83: attrs++;
nit: ++attrs

https://codereview.chromium.org/11415221/diff/23022/chrome/browser/autofill/a...
chrome/browser/autofill/autofill_xml_parser.cc:85: if
(attribute_name.compare("autofilltype") == 0) {
nit: Can this be written as """if (attribute_name == "autofilltype")"""?

https://codereview.chromium.org/11415221/diff/23022/chrome/browser/autofill/a...
chrome/browser/autofill/autofill_xml_parser.cc:93:
field_info.default_value.assign(*attrs);
nit: Can this be written as "field_info.default_value = *attrs"?

https://codereview.chromium.org/11415221/diff/23022/chrome/browser/autofill/a...
chrome/browser/autofill/autofill_xml_parser.cc:95: attrs ++;
nit: ++attrs

[Evan Stade, 2012-12-07 19:21:14]

https://codereview.chromium.org/11415221/diff/23022/chrome/browser/autofill/a...
File chrome/browser/autofill/autofill_field.h (right):

https://codereview.chromium.org/11415221/diff/23022/chrome/browser/autofill/a...
chrome/browser/autofill/autofill_field.h:82: // Used to park the default value
returned by the Autofill server.
I don't understand this use of "park". How about just "the default value
returned..."

[Raman Kakilate, 2012-12-10 18:36:45]

https://codereview.chromium.org/11415221/diff/4001/chrome/browser/autofill/au...
File chrome/browser/autofill/autofill_manager.cc (right):

https://codereview.chromium.org/11415221/diff/4001/chrome/browser/autofill/au...
chrome/browser/autofill/autofill_manager.cc:655: string16 default_value =
ASCIIToUTF16(cached_field->default_value());
On 2012/12/07 01:47:22, Ilya Sherman wrote:
> On 2012/12/06 01:54:05, Raman Kakilate wrote:
> > On 2012/12/01 00:54:12, Ilya Sherman wrote:
> > > How do you know that the cached_field's default value is ASCII?
> > 
> > Changed it to UTF8ToUTF16. I am not entirely sure if this is good enough. Do
> you
> > know if the XML response from autofill server is UTF8 encoded ?
> 
> I'm not certain, but it's likely.  Vadim Berman would be the best person to
ask.

Vadim was also not sure. Code says its UTF8. 

In a different CL will be adding a test to server code to assert response is
UTF8 as well.

https://codereview.chromium.org/11415221/diff/4001/chrome/browser/autofill/au...
chrome/browser/autofill/autofill_manager.cc:656: result.fields[i].is_checked =
(default_value == result.fields[i].value);
On 2012/12/07 01:47:22, Ilya Sherman wrote:
> On 2012/12/06 01:54:05, Raman Kakilate wrote:
> > On 2012/12/01 00:54:12, Ilya Sherman wrote:
> > > I'm confused by this line.  Why isn't it just "result.fields[i].value =
> > > default_value"?
> > 
> > as I understand, for checkboxes and radio buttons - a user cannot change the
> > values of them, only thing user controls is its checked state.
> > 
> > Idea is maintain default values to mark checked in the autofill server. When
> the
> > client sees the values to be checked, client checks if the values of the
field
> > is same as default value, if yes it marks as checked. else, un-checks it.
> 
> If the user can't set the value, why doesn't the server just return a boolean
> for whether the field should be checked or not?  I find the current logic
quite
> confusing...
> 
> Moreover, if we need to keep the current logic, it seems like something that
the
> renderer should be responsible for, rather than having the browser need to get
> involved, since the renderer is where the form actually gets filled.

For a form with radio buttons, eg.

<form>
<input type="radio" name="sex" value="male">Male<br>
<input type="radio" name="sex" value="female">Female
</form>

field signature for both the radio type fields will be same (as the signature is
based on name; radio buttons share common name).

If field mapping uses a boolean checked value, and server wants it clients to
pick "Female" option in the above example, then it would return something like:

(fieldtype: 61, value: false)
(fieldtype: 61, value: true)

This will make the implementation sensitive to order of fields, and if webmaster
decides to change the order of her radio buttons, autofill would fail and would
have to regenerate field mappings. I am trying to avoid it by using value and
doing client side check.

Regarding renderer process or browser process: I was under the impression that
renderer process is mostly handling the view part and decisions are to be made
in the browser process. Please correct me if I am wrong.

https://codereview.chromium.org/11415221/diff/4001/chrome/renderer/autofill/f...
File chrome/renderer/autofill/form_cache.cc (right):

https://codereview.chromium.org/11415221/diff/4001/chrome/renderer/autofill/f...
chrome/renderer/autofill/form_cache.cc:167: }
On 2012/12/07 01:47:22, Ilya Sherman wrote:
> On 2012/12/06 01:54:05, Raman Kakilate wrote:
> > On 2012/12/01 00:54:12, Ilya Sherman wrote:
> > > nit: It seems like this code should be decomposed into a (templatized?)
> helper
> > > function.
> > 
> > code looks repeated 3 times, but the container objects in these 3 cases are
> > different.
> > set<WebDocument>, map<WebSelectElement, string16>, map<WebInputElement,
bool>
> > 
> > Present code looks simpler to maintain.
> 
> I agree that the set is different, but the two maps are pretty much identical.

> I don't see any reason why a templatized helper method would be harder to
> maintain.  What am I missing?

templatized helper method.

https://codereview.chromium.org/11415221/diff/23022/chrome/browser/autofill/a...
File chrome/browser/autofill/autofill_field.h (right):

https://codereview.chromium.org/11415221/diff/23022/chrome/browser/autofill/a...
chrome/browser/autofill/autofill_field.h:82: // Used to park the default value
returned by the Autofill server.
On 2012/12/07 19:21:14, Evan Stade wrote:
> I don't understand this use of "park". How about just "the default value
> returned..."

Done.

https://codereview.chromium.org/11415221/diff/23022/chrome/browser/autofill/a...
File chrome/browser/autofill/autofill_manager_unittest.cc (right):

https://codereview.chromium.org/11415221/diff/23022/chrome/browser/autofill/a...
chrome/browser/autofill/autofill_manager_unittest.cc:1828: "Checkbox",
"checkbx", "dont-fill-me", "checkbox", &field);
On 2012/12/07 01:47:22, Ilya Sherman wrote:
> nit: I think the test code would be clearer if this string were "fill-me", and
> the one on line 1850 were "dont-fill-me"

Done.

https://codereview.chromium.org/11415221/diff/23022/chrome/browser/autofill/a...
chrome/browser/autofill/autofill_manager_unittest.cc:1861: // Last but one field
should be checked.
On 2012/12/07 01:47:22, Ilya Sherman wrote:
> nit: "Last but one" -> "Second to last"

Done.

https://codereview.chromium.org/11415221/diff/23022/chrome/browser/autofill/a...
chrome/browser/autofill/autofill_manager_unittest.cc:1863: // Last field
shoutn't be checked as values are not same.
On 2012/12/07 01:47:22, Ilya Sherman wrote:
> nit: "shoutn't" -> "shouldn't"

Done.

https://codereview.chromium.org/11415221/diff/23022/chrome/browser/autofill/a...
File chrome/browser/autofill/autofill_server_field_info.h (right):

https://codereview.chromium.org/11415221/diff/23022/chrome/browser/autofill/a...
chrome/browser/autofill/autofill_server_field_info.h:11: #include
"base/string16.h"
On 2012/12/07 01:47:22, Ilya Sherman wrote:
> nit: It doesn't look like either of these headers is used.

Done.

https://codereview.chromium.org/11415221/diff/23022/chrome/browser/autofill/a...
chrome/browser/autofill/autofill_server_field_info.h:15: // Autofill Field Type.
On 2012/12/07 01:47:22, Ilya Sherman wrote:
> nit: This comment is just repeating the type name.  Instead, how about
something
> like "The predicted type returned by the Autofill server for this field."

Done.

https://codereview.chromium.org/11415221/diff/23022/chrome/browser/autofill/a...
chrome/browser/autofill/autofill_server_field_info.h:17: // Default value for
the field
On 2012/12/07 01:47:22, Ilya Sherman wrote:
> nit: Please mention that the default value only applies when the field_type is
> FIELD_WITH_DEFAULT_VALUE".  Also, please end the sentence with a period.

Done.

https://codereview.chromium.org/11415221/diff/23022/chrome/browser/autofill/a...
File chrome/browser/autofill/autofill_xml_parser.cc (right):

https://codereview.chromium.org/11415221/diff/23022/chrome/browser/autofill/a...
chrome/browser/autofill/autofill_xml_parser.cc:54: // |attrs| is a
NULL-terminated list of (attribute, value) pairs.
On 2012/12/07 01:47:22, Ilya Sherman wrote:
> nit: Please add a similar comment above line 81 as well.

Done.

https://codereview.chromium.org/11415221/diff/23022/chrome/browser/autofill/a...
chrome/browser/autofill/autofill_xml_parser.cc:57: attrs++;
On 2012/12/07 01:47:22, Ilya Sherman wrote:
> nit: ++attrs

Done.

https://codereview.chromium.org/11415221/diff/23022/chrome/browser/autofill/a...
chrome/browser/autofill/autofill_xml_parser.cc:67: attrs ++;
On 2012/12/07 01:47:22, Ilya Sherman wrote:
> nit: ++attrs

Done.

https://codereview.chromium.org/11415221/diff/23022/chrome/browser/autofill/a...
chrome/browser/autofill/autofill_xml_parser.cc:70: if (!attrs[0]) {
On 2012/12/07 01:47:22, Ilya Sherman wrote:
> nit: *attrs

Done.

https://codereview.chromium.org/11415221/diff/23022/chrome/browser/autofill/a...
chrome/browser/autofill/autofill_xml_parser.cc:83: attrs++;
On 2012/12/07 01:47:22, Ilya Sherman wrote:
> nit: ++attrs

Done.

https://codereview.chromium.org/11415221/diff/23022/chrome/browser/autofill/a...
chrome/browser/autofill/autofill_xml_parser.cc:85: if
(attribute_name.compare("autofilltype") == 0) {
On 2012/12/07 01:47:22, Ilya Sherman wrote:
> nit: Can this be written as """if (attribute_name == "autofilltype")"""?

There are more than few compare calls in this file. I can do it in a future CL.
For now, I will be consistent with rest of the file.

https://codereview.chromium.org/11415221/diff/23022/chrome/browser/autofill/a...
chrome/browser/autofill/autofill_xml_parser.cc:93:
field_info.default_value.assign(*attrs);
On 2012/12/07 01:47:22, Ilya Sherman wrote:
> nit: Can this be written as "field_info.default_value = *attrs"?

Done.

https://codereview.chromium.org/11415221/diff/23022/chrome/browser/autofill/a...
chrome/browser/autofill/autofill_xml_parser.cc:95: attrs ++;
On 2012/12/07 01:47:22, Ilya Sherman wrote:
> nit: ++attrs

Done.

[Ilya Sherman, 2012-12-11 05:56:18]

https://codereview.chromium.org/11415221/diff/4001/chrome/browser/autofill/au...
File chrome/browser/autofill/autofill_manager.cc (right):

https://codereview.chromium.org/11415221/diff/4001/chrome/browser/autofill/au...
chrome/browser/autofill/autofill_manager.cc:656: result.fields[i].is_checked =
(default_value == result.fields[i].value);
On 2012/12/10 18:36:45, Raman Kakilate wrote:
> On 2012/12/07 01:47:22, Ilya Sherman wrote:
> > On 2012/12/06 01:54:05, Raman Kakilate wrote:
> > > On 2012/12/01 00:54:12, Ilya Sherman wrote:
> > > > I'm confused by this line.  Why isn't it just "result.fields[i].value =
> > > > default_value"?
> > > 
> > > as I understand, for checkboxes and radio buttons - a user cannot change
the
> > > values of them, only thing user controls is its checked state.
> > > 
> > > Idea is maintain default values to mark checked in the autofill server.
When
> > the
> > > client sees the values to be checked, client checks if the values of the
> field
> > > is same as default value, if yes it marks as checked. else, un-checks it.
> > 
> > If the user can't set the value, why doesn't the server just return a
boolean
> > for whether the field should be checked or not?  I find the current logic
> quite
> > confusing...
> > 
> > Moreover, if we need to keep the current logic, it seems like something that
> the
> > renderer should be responsible for, rather than having the browser need to
get
> > involved, since the renderer is where the form actually gets filled.
> 
> For a form with radio buttons, eg.
> 
> <form>
> <input type="radio" name="sex" value="male">Male<br>
> <input type="radio" name="sex" value="female">Female
> </form>
> 
> field signature for both the radio type fields will be same (as the signature
is
> based on name; radio buttons share common name).

Perhaps this is a reason to include the value in the signature for radio
buttons?  I don't see any backwards compatibility issues that would be worse
than the ones we get from simply adding radio buttons in the first place (i.e.,
that the server will "forget" all that it has learned about any forms that
contain radio buttons).

> If field mapping uses a boolean checked value, and server wants it clients to
> pick "Female" option in the above example, then it would return something
like:
> 
> (fieldtype: 61, value: false)
> (fieldtype: 61, value: true)
> 
> This will make the implementation sensitive to order of fields, and if
webmaster
> decides to change the order of her radio buttons, autofill would fail and
would
> have to regenerate field mappings. I am trying to avoid it by using value and
> doing client side check.

I don't see how passing the value makes this any different -- the information
content is still essentially the same.  Either way, you're relying on the order
of the fields.  That is, if the server were to return

(fieldtype: 61, value: dont-fill-me)
(fieldtype: 61, value: female)

and the field order were to be reversed, Chrome still wouldn't be able to fill
the form correctly.  Or are you suggesting that the server should return

(fieldtype: 61, value: female)
(fieldtype: 61, value: female)

?  If so, that makes a bit of sense, but I think it would be helpful to add a
comment to this code explaining how this works, as it's not obvious (at least it
wasn't to me).

> Regarding renderer process or browser process: I was under the impression that
> renderer process is mostly handling the view part and decisions are to be made
> in the browser process. Please correct me if I am wrong.

It depends on what sort of decision is being made.  If the decision is pretty
much just about how to talk to WebKit, then the renderer is a more appropriate
place.  If the decision is more complex than that, then the browser is more
appropriate.  Now that I better understand what you're trying to achieve (i.e.
handling the fact that distinct radio buttons in a single group have the same
signature), I'm more ok with leaving this logic in the browser process.

https://codereview.chromium.org/11415221/diff/32001/chrome/renderer/autofill/...
File chrome/renderer/autofill/form_cache.cc (right):

https://codereview.chromium.org/11415221/diff/32001/chrome/renderer/autofill/...
chrome/renderer/autofill/form_cache.cc:47: // WebFormElement for K.form().
nit: Prior to describing this implementation detail, please also include a
sentence describing what this helper function does.

https://codereview.chromium.org/11415221/diff/32001/chrome/renderer/autofill/...
chrome/renderer/autofill/form_cache.cc:49: void RemoveOldElements(std::map<const
K, V> states, const WebFrame& frame) {
The |states| map is meant to be updated, so it should be passed as a pointer to
the map, and should be the second parameter to the function:

void RemoveOldElements(const WebFrame& frame, std::map<const K, V>* states)

https://codereview.chromium.org/11415221/diff/32001/chrome/renderer/autofill/...
chrome/renderer/autofill/form_cache.cc:49: void RemoveOldElements(std::map<const
K, V> states, const WebFrame& frame) {
nit: It doesn't seem necessary to include the "const" in front of "K" here,
assuming the code compiles without it.

https://codereview.chromium.org/11415221/diff/32001/chrome/renderer/autofill/...
File chrome/renderer/autofill/form_cache.h (right):

https://codereview.chromium.org/11415221/diff/32001/chrome/renderer/autofill/...
chrome/renderer/autofill/form_cache.h:22: class WebFormControlElement;
nit: This forward declaration doesn't seem to be necessary.

[Raman Kakilate, 2012-12-11 17:45:44]

https://codereview.chromium.org/11415221/diff/4001/chrome/browser/autofill/au...
File chrome/browser/autofill/autofill_manager.cc (right):

https://codereview.chromium.org/11415221/diff/4001/chrome/browser/autofill/au...
chrome/browser/autofill/autofill_manager.cc:656: result.fields[i].is_checked =
(default_value == result.fields[i].value);
On 2012/12/11 05:56:19, Ilya Sherman wrote:
> On 2012/12/10 18:36:45, Raman Kakilate wrote:
> > On 2012/12/07 01:47:22, Ilya Sherman wrote:
> > > On 2012/12/06 01:54:05, Raman Kakilate wrote:
> > > > On 2012/12/01 00:54:12, Ilya Sherman wrote:
> > > > > I'm confused by this line.  Why isn't it just "result.fields[i].value
=
> > > > > default_value"?
> > > > 
> > > > as I understand, for checkboxes and radio buttons - a user cannot change
> the
> > > > values of them, only thing user controls is its checked state.
> > > > 
> > > > Idea is maintain default values to mark checked in the autofill server.
> When
> > > the
> > > > client sees the values to be checked, client checks if the values of the
> > field
> > > > is same as default value, if yes it marks as checked. else, un-checks
it.
> > > 
> > > If the user can't set the value, why doesn't the server just return a
> boolean
> > > for whether the field should be checked or not?  I find the current logic
> > quite
> > > confusing...
> > > 
> > > Moreover, if we need to keep the current logic, it seems like something
that
> > the
> > > renderer should be responsible for, rather than having the browser need to
> get
> > > involved, since the renderer is where the form actually gets filled.
> > 
> > For a form with radio buttons, eg.
> > 
> > <form>
> > <input type="radio" name="sex" value="male">Male<br>
> > <input type="radio" name="sex" value="female">Female
> > </form>
> > 
> > field signature for both the radio type fields will be same (as the
signature
> is
> > based on name; radio buttons share common name).
> 
> Perhaps this is a reason to include the value in the signature for radio
> buttons?  I don't see any backwards compatibility issues that would be worse
> than the ones we get from simply adding radio buttons in the first place
(i.e.,
> that the server will "forget" all that it has learned about any forms that
> contain radio buttons).
> 

In this CL, I am counting radio-buttons/checkboxes towards form signature only
when experiment is on (in a longer run, only for whitelisted sites). When we
count checkable elements towards form signature, we also mark the request to
autofill server as experimental. Based on the marker in the autofill request,
autofill server looks at different SSTable to respond. (all this changes have
been submitted)

This way we wouldn't be breaking any of the existing forms with radio buttons.

> > If field mapping uses a boolean checked value, and server wants it clients
to
> > pick "Female" option in the above example, then it would return something
> like:
> > 
> > (fieldtype: 61, value: false)
> > (fieldtype: 61, value: true)
> > 
> > This will make the implementation sensitive to order of fields, and if
> webmaster
> > decides to change the order of her radio buttons, autofill would fail and
> would
> > have to regenerate field mappings. I am trying to avoid it by using value
and
> > doing client side check.
> 
> I don't see how passing the value makes this any different -- the information
> content is still essentially the same.  Either way, you're relying on the
order
> of the fields.  That is, if the server were to return
> 
> (fieldtype: 61, value: dont-fill-me)
> (fieldtype: 61, value: female)
> 
> and the field order were to be reversed, Chrome still wouldn't be able to fill
> the form correctly.  Or are you suggesting that the server should return
> 
> (fieldtype: 61, value: female)
> (fieldtype: 61, value: female)
> 
> ?  If so, that makes a bit of sense, but I think it would be helpful to add a
> comment to this code explaining how this works, as it's not obvious (at least
it
> wasn't to me).
> 

Sorry for the confusion, added more comments. Hopefully they address your point.

> > Regarding renderer process or browser process: I was under the impression
that
> > renderer process is mostly handling the view part and decisions are to be
made
> > in the browser process. Please correct me if I am wrong.
> 
> It depends on what sort of decision is being made.  If the decision is pretty
> much just about how to talk to WebKit, then the renderer is a more appropriate
> place.  If the decision is more complex than that, then the browser is more
> appropriate.  Now that I better understand what you're trying to achieve (i.e.
> handling the fact that distinct radio buttons in a single group have the same
> signature), I'm more ok with leaving this logic in the browser process.

Ack.

https://codereview.chromium.org/11415221/diff/32001/chrome/renderer/autofill/...
File chrome/renderer/autofill/form_cache.cc (right):

https://codereview.chromium.org/11415221/diff/32001/chrome/renderer/autofill/...
chrome/renderer/autofill/form_cache.cc:47: // WebFormElement for K.form().
On 2012/12/11 05:56:19, Ilya Sherman wrote:
> nit: Prior to describing this implementation detail, please also include a
> sentence describing what this helper function does.

Done.

https://codereview.chromium.org/11415221/diff/32001/chrome/renderer/autofill/...
chrome/renderer/autofill/form_cache.cc:49: void RemoveOldElements(std::map<const
K, V> states, const WebFrame& frame) {
On 2012/12/11 05:56:19, Ilya Sherman wrote:
> nit: It doesn't seem necessary to include the "const" in front of "K" here,
> assuming the code compiles without it.
code doesn't compile. Leaving it as it.

https://codereview.chromium.org/11415221/diff/32001/chrome/renderer/autofill/...
chrome/renderer/autofill/form_cache.cc:49: void RemoveOldElements(std::map<const
K, V> states, const WebFrame& frame) {
On 2012/12/11 05:56:19, Ilya Sherman wrote:
> The |states| map is meant to be updated, so it should be passed as a pointer
to
> the map, and should be the second parameter to the function:
> 
> void RemoveOldElements(const WebFrame& frame, std::map<const K, V>* states)

Done.

https://codereview.chromium.org/11415221/diff/32001/chrome/renderer/autofill/...
File chrome/renderer/autofill/form_cache.h (right):

https://codereview.chromium.org/11415221/diff/32001/chrome/renderer/autofill/...
chrome/renderer/autofill/form_cache.h:22: class WebFormControlElement;
On 2012/12/11 05:56:19, Ilya Sherman wrote:
> nit: This forward declaration doesn't seem to be necessary.

Done.

[Ilya Sherman, 2012-12-12 00:26:44]

Thanks.  LGTM with a last few nits:

https://codereview.chromium.org/11415221/diff/42001/chrome/browser/autofill/f...
File chrome/browser/autofill/form_structure.cc (left):

https://codereview.chromium.org/11415221/diff/42001/chrome/browser/autofill/f...
chrome/browser/autofill/form_structure.cc:451: 
nit: Please preserve this newline.

https://codereview.chromium.org/11415221/diff/42001/chrome/browser/autofill/f...
File chrome/browser/autofill/form_structure.cc (right):

https://codereview.chromium.org/11415221/diff/42001/chrome/browser/autofill/f...
chrome/browser/autofill/form_structure.cc:900: continue;
nit: Please leave a blank line after this one.

https://codereview.chromium.org/11415221/diff/42001/chrome/browser/autofill/f...
chrome/browser/autofill/form_structure.cc:919: continue;
nit: Please leave a blank line after this one.

https://codereview.chromium.org/11415221/diff/42001/chrome/renderer/autofill/...
File chrome/renderer/autofill/form_autofill_util.cc (right):

https://codereview.chromium.org/11415221/diff/42001/chrome/renderer/autofill/...
chrome/renderer/autofill/form_autofill_util.cc:464: const WebInputElement*
input_element = toWebInputElement(element);
nit: Please move this to be after the comment, i.e. between lines 466 and 467.

https://codereview.chromium.org/11415221/diff/42001/chrome/renderer/autofill/...
chrome/renderer/autofill/form_autofill_util.cc:647: }
nit: It's better to always explicitly set the value rather than relying on the
default value, so I'd suggest writing this as

field->is_checkable = IsRadioButtonElement(input_element) ||
IsCheckboxElement(input_element);

https://codereview.chromium.org/11415221/diff/42001/chrome/renderer/autofill/...
File chrome/renderer/autofill/form_autofill_util.h (right):

https://codereview.chromium.org/11415221/diff/42001/chrome/renderer/autofill/...
chrome/renderer/autofill/form_autofill_util.h:59: bool IsCheckboxElement(const
WebKit::WebInputElement* element);
Optional nit: Since you always check these two functions together, it might be
worth combining them into a single IsCheckableElement() functions.

https://codereview.chromium.org/11415221/diff/42001/chrome/renderer/autofill/...
File chrome/renderer/autofill/form_cache.cc (right):

https://codereview.chromium.org/11415221/diff/42001/chrome/renderer/autofill/...
chrome/renderer/autofill/form_cache.cc:47: // of web frame's scope.
nit: Perhaps mention that this is done so as to release memory that we no longer
need to hold on to.

[Raman Kakilate, 2012-12-12 01:22:04]

https://codereview.chromium.org/11415221/diff/42001/chrome/browser/autofill/f...
File chrome/browser/autofill/form_structure.cc (left):

https://codereview.chromium.org/11415221/diff/42001/chrome/browser/autofill/f...
chrome/browser/autofill/form_structure.cc:451: 
On 2012/12/12 00:26:44, Ilya Sherman wrote:
> nit: Please preserve this newline.

Done.

https://codereview.chromium.org/11415221/diff/42001/chrome/browser/autofill/f...
File chrome/browser/autofill/form_structure.cc (right):

https://codereview.chromium.org/11415221/diff/42001/chrome/browser/autofill/f...
chrome/browser/autofill/form_structure.cc:900: continue;
On 2012/12/12 00:26:44, Ilya Sherman wrote:
> nit: Please leave a blank line after this one.

Done.

https://codereview.chromium.org/11415221/diff/42001/chrome/browser/autofill/f...
chrome/browser/autofill/form_structure.cc:919: continue;
On 2012/12/12 00:26:44, Ilya Sherman wrote:
> nit: Please leave a blank line after this one.

Done.

https://codereview.chromium.org/11415221/diff/42001/chrome/renderer/autofill/...
File chrome/renderer/autofill/form_autofill_util.cc (right):

https://codereview.chromium.org/11415221/diff/42001/chrome/renderer/autofill/...
chrome/renderer/autofill/form_autofill_util.cc:464: const WebInputElement*
input_element = toWebInputElement(element);
On 2012/12/12 00:26:44, Ilya Sherman wrote:
> nit: Please move this to be after the comment, i.e. between lines 466 and 467.

Done.

https://codereview.chromium.org/11415221/diff/42001/chrome/renderer/autofill/...
chrome/renderer/autofill/form_autofill_util.cc:647: }
On 2012/12/12 00:26:44, Ilya Sherman wrote:
> nit: It's better to always explicitly set the value rather than relying on the
> default value, so I'd suggest writing this as
> 
> field->is_checkable = IsRadioButtonElement(input_element) ||
> IsCheckboxElement(input_element);

Done.

https://codereview.chromium.org/11415221/diff/42001/chrome/renderer/autofill/...
File chrome/renderer/autofill/form_autofill_util.h (right):

https://codereview.chromium.org/11415221/diff/42001/chrome/renderer/autofill/...
chrome/renderer/autofill/form_autofill_util.h:59: bool IsCheckboxElement(const
WebKit::WebInputElement* element);
On 2012/12/12 00:26:44, Ilya Sherman wrote:
> Optional nit: Since you always check these two functions together, it might be
> worth combining them into a single IsCheckableElement() functions.

Done.

https://codereview.chromium.org/11415221/diff/42001/chrome/renderer/autofill/...
File chrome/renderer/autofill/form_cache.cc (right):

https://codereview.chromium.org/11415221/diff/42001/chrome/renderer/autofill/...
chrome/renderer/autofill/form_cache.cc:47: // of web frame's scope.
On 2012/12/12 00:26:44, Ilya Sherman wrote:
> nit: Perhaps mention that this is done so as to release memory that we no
longer
> need to hold on to.

Done.

https://codereview.chromium.org/11415221/diff/42001/chrome/renderer/autofill/...
chrome/renderer/autofill/form_cache.cc:47: // of web frame's scope.
On 2012/12/12 00:26:44, Ilya Sherman wrote:
> nit: Perhaps mention that this is done so as to release memory that we no
longer
> need to hold on to.

Done.

[Raman Kakilate, 2012-12-12 16:55:33]

+sky for gypi and chrome/common changes.
+cdn for autofill_messages.h changes.

[sky, 2012-12-12 17:38:23]

LGTM

[Cris Neckar, 2012-12-12 18:42:57]

The IPC changes look OK but are we actually parsing xml in the browser process?
That seems bad. Any idea why this isn't pushed to the utility process?

[Raman Kakilate, 2012-12-12 18:56:52]

On 2012/12/12 18:42:57, Cris Neckar wrote:
> The IPC changes look OK but are we actually parsing xml in the browser
process?
> That seems bad. Any idea why this isn't pushed to the utility process?

Sorry - I have no idea. @isherman/@abodenha ?
anyways, its probably a good idea to not have those changes in this CL ?

[Cris Neckar, 2012-12-12 23:31:26]

Yep of course. I'll follow up with them. 

lgtm on ipc

[commit-bot: I haz the power, 2012-12-13 00:07:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ramankk@chromium.org/11415221/52001

[commit-bot: I haz the power, 2012-12-13 00:07:26]

Presubmit check for 11415221-52001 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit Warnings **
Found lines longer than 80 characters (first 5 shown).
  chrome/renderer/autofill/form_cache.cc, line 46, 86 chars

Presubmit checks took 4.1s to calculate.

[commit-bot: I haz the power, 2012-12-13 00:20:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ramankk@chromium.org/11415221/44003

[commit-bot: I haz the power, 2012-12-13 01:17:31]

Retried try job too often on linux_rel for step(s) unit_tests

[Albert Bodenhamer, 2012-12-13 01:21:59]

Looks like it might be a legit failure.
http://build.chromium.org/p/tryserver.chromium/builders/linux_rel/builds/8375...
details.

Raman, can you run those tests locally and make sure they pass?


On Wed, Dec 12, 2012 at 5:17 PM, <commit-bot@chromium.org> wrote:

> Retried try job too often on linux_rel for step(s) unit_tests
>
>
https://chromiumcodereview.**appspot.com/11415221/<https://chromiumcodereview...
>



-- 
Albert Bodenhamer | Software Engineer | abodenha@chromium.<abodenha@google.com>
org

[Raman Kakilate, 2012-12-13 02:03:36]

On 2012/12/13 01:21:59, Albert Bodenhamer wrote:
> Looks like it might be a legit failure.
>
http://build.chromium.org/p/tryserver.chromium/builders/linux_rel/builds/8375...
> details.
> 
> Raman, can you run those tests locally and make sure they pass?
> 

I am not sure whats happening. This test passes locally.
chrome/browser/autofill/autofill_xml_parser.cc: 89-91 handles it.

ramankk@ramankk:~/chrome2/src$ ./out/Release/unit_tests
--gtest_filter=AutofillQueryXmlParserTest.ParseErrors
Note: Google Test filter = AutofillQueryXmlParserTest.ParseErrors
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from AutofillQueryXmlParserTest
[ RUN      ] AutofillQueryXmlParserTest.ParseErrors
[       OK ] AutofillQueryXmlParserTest.ParseErrors (0 ms)
[----------] 1 test from AutofillQueryXmlParserTest (0 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test case ran. (0 ms total)
[  PASSED  ] 1 test.


> 
> On Wed, Dec 12, 2012 at 5:17 PM, <mailto:commit-bot@chromium.org> wrote:
> 
> > Retried try job too often on linux_rel for step(s) unit_tests
> >
> >
>
https://chromiumcodereview.**appspot.com/11415221/%3Chttps://chromiumcoderevi...>
> >
> 
> 
> 
> -- 
> Albert Bodenhamer | Software Engineer |
mailto:abodenha@chromium.<abodenha@google.com>
> org

[commit-bot: I haz the power, 2012-12-13 17:04:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ramankk@chromium.org/11415221/44003

[Albert Bodenhamer, 2012-12-13 17:05:09]

Let's try it again.  If it fails again we'll dig further.


On Wed, Dec 12, 2012 at 6:03 PM, <ramankk@chromium.org> wrote:

> On 2012/12/13 01:21:59, Albert Bodenhamer wrote:
>
>> Looks like it might be a legit failure.
>>
>
> http://build.chromium.org/p/**tryserver.chromium/builders/**
>
linux_rel/builds/83753/steps/**unit_tests/logs/stdio#**failure1has<http://build.chromium.org/p/tryserver.chromium/builders/linux_rel/builds/83753/steps/unit_tests/logs/stdio#failure1has>
>
>> details.
>>
>
>  Raman, can you run those tests locally and make sure they pass?
>>
>
>
> I am not sure whats happening. This test passes locally.
> chrome/browser/autofill/**autofill_xml_parser.cc: 89-91 handles it.
>
> ramankk@ramankk:~/chrome2/src$ ./out/Release/unit_tests
> --gtest_filter=**AutofillQueryXmlParserTest.**ParseErrors
> Note: Google Test filter = AutofillQueryXmlParserTest.**ParseErrors
> [==========] Running 1 test from 1 test case.
> [----------] Global test environment set-up.
> [----------] 1 test from AutofillQueryXmlParserTest
> [ RUN      ] AutofillQueryXmlParserTest.**ParseErrors
> [       OK ] AutofillQueryXmlParserTest.**ParseErrors (0 ms)
> [----------] 1 test from AutofillQueryXmlParserTest (0 ms total)
>
> [----------] Global test environment tear-down
> [==========] 1 test from 1 test case ran. (0 ms total)
> [  PASSED  ] 1 test.
>
>
>
>
>  On Wed, Dec 12, 2012 at 5:17 PM,
<mailto:commit-bot@chromium.**org<commit-bot@chromium.org>>
>> wrote:
>>
>
>  > Retried try job too often on linux_rel for step(s) unit_tests
>> >
>> >
>>
>
> https://chromiumcodereview.**a**ppspot.com/11415221/%3Chttps:/**
>
/chromiumcodereview.appspot.**com/11415221/<http://appspot.com/11415221/%3Chttps://chromiumcodereview.appspot.com/11415221/>
> >
>
>  >
>>
>
>
>
>  --
>> Albert Bodenhamer | Software Engineer |
>>
> mailto:abodenha@chromium.<abod**enha@google.com <abodenha@google.com>>
>
>> org
>>
>
>
>
https://codereview.chromium.**org/11415221/<https://codereview.chromium.org/1...
>



-- 
Albert Bodenhamer | Software Engineer | abodenha@chromium.<abodenha@google.com>
org

[commit-bot: I haz the power, 2012-12-13 17:54:25]

Retried try job too often on linux_rel for step(s) unit_tests

[Raman Kakilate, 2012-12-13 18:45:29]

On 2012/12/13 17:54:25, I haz the power (commit-bot) wrote:
> Retried try job too often on linux_rel for step(s) unit_tests

I am out of ideas. I tried to have same GYP_DEFINES as shows in the test run
logs.

ramankk@ramankk:~/chrome2/src$ echo $GYP_DEFINES
chromeos=1 fastbuild=1 dcheck_always_on=1 ffmpeg_branding=ChromeOS
component=shared_library proprietary_codecs=1

# rebuilt 
PATH=~/goma/goma/:$PATH ninja -C out/Release chrome unit_tests

and still I see test passing locally.. 

ramankk@ramankk:~/chrome2/src$ ./out/Release/unit_tests
--gtest_filter=AutofillQueryXmlParserTest.*
Note: Google Test filter = AutofillQueryXmlParserTest.*
[==========] Running 4 tests from 1 test case.
[----------] Global test environment set-up.
[----------] 4 tests from AutofillQueryXmlParserTest
[ RUN      ] AutofillQueryXmlParserTest.BasicQuery
[       OK ] AutofillQueryXmlParserTest.BasicQuery (0 ms)
[ RUN      ] AutofillQueryXmlParserTest.TestUploadRequired
[       OK ] AutofillQueryXmlParserTest.TestUploadRequired (0 ms)
[ RUN      ] AutofillQueryXmlParserTest.ParseExperimentId
[       OK ] AutofillQueryXmlParserTest.ParseExperimentId (0 ms)
[ RUN      ] AutofillQueryXmlParserTest.ParseErrors
[       OK ] AutofillQueryXmlParserTest.ParseErrors (0 ms)
[----------] 4 tests from AutofillQueryXmlParserTest (0 ms total)

[----------] Global test environment tear-down
[==========] 4 tests from 1 test case ran. (0 ms total)
[  PASSED  ] 4 tests.

[Albert Bodenhamer, 2012-12-13 23:17:37]

I'm able to repro the browser_test issue locally in a debug build on
Windows.:

ninja -C out/Debug browser_tests
out\Debug\browser_tests -gtest_filter="FormAutofillTest.*"


On Thu, Dec 13, 2012 at 10:45 AM, <ramankk@chromium.org> wrote:

> On 2012/12/13 17:54:25, I haz the power (commit-bot) wrote:
>
>> Retried try job too often on linux_rel for step(s) unit_tests
>>
>
> I am out of ideas. I tried to have same GYP_DEFINES as shows in the test
> run
> logs.
>
> ramankk@ramankk:~/chrome2/src$ echo $GYP_DEFINES
> chromeos=1 fastbuild=1 dcheck_always_on=1 ffmpeg_branding=ChromeOS
> component=shared_library proprietary_codecs=1
>
> # rebuilt
> PATH=~/goma/goma/:$PATH ninja -C out/Release chrome unit_tests
>
> and still I see test passing locally..
>
> ramankk@ramankk:~/chrome2/src$ ./out/Release/unit_tests
> --gtest_filter=**AutofillQueryXmlParserTest.*
> Note: Google Test filter = AutofillQueryXmlParserTest.*
> [==========] Running 4 tests from 1 test case.
>
> [----------] Global test environment set-up.
> [----------] 4 tests from AutofillQueryXmlParserTest
> [ RUN      ] AutofillQueryXmlParserTest.**BasicQuery
> [       OK ] AutofillQueryXmlParserTest.**BasicQuery (0 ms)
> [ RUN      ] AutofillQueryXmlParserTest.**TestUploadRequired
> [       OK ] AutofillQueryXmlParserTest.**TestUploadRequired (0 ms)
> [ RUN      ] AutofillQueryXmlParserTest.**ParseExperimentId
> [       OK ] AutofillQueryXmlParserTest.**ParseExperimentId (0 ms)
>
> [ RUN      ] AutofillQueryXmlParserTest.**ParseErrors
> [       OK ] AutofillQueryXmlParserTest.**ParseErrors (0 ms)
> [----------] 4 tests from AutofillQueryXmlParserTest (0 ms total)
>
>
> [----------] Global test environment tear-down
> [==========] 4 tests from 1 test case ran. (0 ms total)
> [  PASSED  ] 4 tests.
>
>
>
>
https://codereview.chromium.**org/11415221/<https://codereview.chromium.org/1...
>



-- 
Albert Bodenhamer | Software Engineer | abodenha@chromium.<abodenha@google.com>
org

[Raman Kakilate, 2012-12-14 22:38:24]

PTAL.

1. Updated loads of heuristics related .out files to reflect check-boxes and
radio-buttons as recognizable elements.
2. Updated autofill_xml_parser.cc to look at original value and hopefully will
bypass probable compiler optimization that breaks the test on CQ.
3. With recognition of radio-buttons and check-boxes as first class elements,
they are messing up with context when FormFieldParser try to parse the fields in
context. Eg., AddressLine2 is expected to be immediately after AddressLine1, but
if there is a "Is PO Box" checkbox after AddressLine1; AddressLine2 is not
recognized correctly.
To fix this, changed autofill_scanner to skip all clickable elements (probably a
sub-optimal solution, but would cover all the cases without much code changes).


On 2012/12/13 23:17:37, Albert Bodenhamer wrote:
> I'm able to repro the browser_test issue locally in a debug build on
> Windows.:
> 
> ninja -C out/Debug browser_tests
> out\Debug\browser_tests -gtest_filter="FormAutofillTest.*"
> 
> 
> On Thu, Dec 13, 2012 at 10:45 AM, <mailto:ramankk@chromium.org> wrote:
> 
> > On 2012/12/13 17:54:25, I haz the power (commit-bot) wrote:
> >
> >> Retried try job too often on linux_rel for step(s) unit_tests
> >>
> >
> > I am out of ideas. I tried to have same GYP_DEFINES as shows in the test
> > run
> > logs.
> >
> > ramankk@ramankk:~/chrome2/src$ echo $GYP_DEFINES
> > chromeos=1 fastbuild=1 dcheck_always_on=1 ffmpeg_branding=ChromeOS
> > component=shared_library proprietary_codecs=1
> >
> > # rebuilt
> > PATH=~/goma/goma/:$PATH ninja -C out/Release chrome unit_tests
> >
> > and still I see test passing locally..
> >
> > ramankk@ramankk:~/chrome2/src$ ./out/Release/unit_tests
> > --gtest_filter=**AutofillQueryXmlParserTest.*
> > Note: Google Test filter = AutofillQueryXmlParserTest.*
> > [==========] Running 4 tests from 1 test case.
> >
> > [----------] Global test environment set-up.
> > [----------] 4 tests from AutofillQueryXmlParserTest
> > [ RUN      ] AutofillQueryXmlParserTest.**BasicQuery
> > [       OK ] AutofillQueryXmlParserTest.**BasicQuery (0 ms)
> > [ RUN      ] AutofillQueryXmlParserTest.**TestUploadRequired
> > [       OK ] AutofillQueryXmlParserTest.**TestUploadRequired (0 ms)
> > [ RUN      ] AutofillQueryXmlParserTest.**ParseExperimentId
> > [       OK ] AutofillQueryXmlParserTest.**ParseExperimentId (0 ms)
> >
> > [ RUN      ] AutofillQueryXmlParserTest.**ParseErrors
> > [       OK ] AutofillQueryXmlParserTest.**ParseErrors (0 ms)
> > [----------] 4 tests from AutofillQueryXmlParserTest (0 ms total)
> >
> >
> > [----------] Global test environment tear-down
> > [==========] 4 tests from 1 test case ran. (0 ms total)
> > [  PASSED  ] 4 tests.
> >
> >
> >
> >
>
https://codereview.chromium.**org/11415221/%3Chttps://codereview.chromium.org...>
> >
> 
> 
> 
> -- 
> Albert Bodenhamer | Software Engineer |
mailto:abodenha@chromium.<abodenha@google.com>
> org

[Ilya Sherman, 2012-12-15 01:08:36]

https://chromiumcodereview.appspot.com/11415221/diff/70001/chrome/browser/aut...
File chrome/browser/autofill/autofill_scanner.h (right):

https://chromiumcodereview.appspot.com/11415221/diff/70001/chrome/browser/aut...
chrome/browser/autofill/autofill_scanner.h:18: // interferes with correctly
understanding ADDRESS_LINE2).
Rather than changing the scanner class, please change
FormField::ParseFormFields() to drop all checkboxes and radio buttons prior to
parsing.

https://chromiumcodereview.appspot.com/11415221/diff/70001/chrome/browser/aut...
File chrome/browser/autofill/autofill_xml_parser.cc (right):

https://chromiumcodereview.appspot.com/11415221/diff/70001/chrome/browser/aut...
chrome/browser/autofill/autofill_xml_parser.cc:90: field_info.field_type =
NO_SERVER_DATA;
nit: To further reduce the chances of this getting messed up in the future,
please write lines 88 through 90 as:

if (value >= 0 && value < MAX_VALID_FIELD_TYPE)
  field_info.field_type = static_cast<AutofillFieldType>(value);

Also, please add a test for the boundary cases of 0 and MAX_VALID_FIELD_TYPE --
it looks like the code currently incorrectly accepts MAX_VALID_FIELD_TYPE as a
possible server type.

https://chromiumcodereview.appspot.com/11415221/diff/70001/chrome/browser/aut...
File chrome/browser/autofill/form_structure.cc (right):

https://chromiumcodereview.appspot.com/11415221/diff/70001/chrome/browser/aut...
chrome/browser/autofill/form_structure.cc:588: return
!has_author_specified_types_ && ShouldBeParsed(true);
The test expectations changes made me realize that you haven't updated this code
path, which only queries the server for forms with 3 (a.k.a.
kRequiredFillableFields) or more fields.  Please update the ShouldBeParsed()
code so that checkboxes and radio buttons do not count toward that threshold.

[Raman Kakilate, 2012-12-17 20:11:00]

https://codereview.chromium.org/11415221/diff/70001/chrome/browser/autofill/a...
File chrome/browser/autofill/autofill_scanner.h (right):

https://codereview.chromium.org/11415221/diff/70001/chrome/browser/autofill/a...
chrome/browser/autofill/autofill_scanner.h:18: // interferes with correctly
understanding ADDRESS_LINE2).
On 2012/12/15 01:08:36, Ilya Sherman wrote:
> Rather than changing the scanner class, please change
> FormField::ParseFormFields() to drop all checkboxes and radio buttons prior to
> parsing.

Done.

https://codereview.chromium.org/11415221/diff/70001/chrome/browser/autofill/a...
File chrome/browser/autofill/autofill_xml_parser.cc (right):

https://codereview.chromium.org/11415221/diff/70001/chrome/browser/autofill/a...
chrome/browser/autofill/autofill_xml_parser.cc:90: field_info.field_type =
NO_SERVER_DATA;
On 2012/12/15 01:08:36, Ilya Sherman wrote:
> nit: To further reduce the chances of this getting messed up in the future,
> please write lines 88 through 90 as:
> 
> if (value >= 0 && value < MAX_VALID_FIELD_TYPE)
>   field_info.field_type = static_cast<AutofillFieldType>(value);
> 
> Also, please add a test for the boundary cases of 0 and MAX_VALID_FIELD_TYPE
--
> it looks like the code currently incorrectly accepts MAX_VALID_FIELD_TYPE as a
> possible server type.

Done.

https://codereview.chromium.org/11415221/diff/70001/chrome/browser/autofill/f...
File chrome/browser/autofill/form_structure.cc (right):

https://codereview.chromium.org/11415221/diff/70001/chrome/browser/autofill/f...
chrome/browser/autofill/form_structure.cc:588: return
!has_author_specified_types_ && ShouldBeParsed(true);
On 2012/12/15 01:08:36, Ilya Sherman wrote:
> The test expectations changes made me realize that you haven't updated this
code
> path, which only queries the server for forms with 3 (a.k.a.
> kRequiredFillableFields) or more fields.  Please update the ShouldBeParsed()
> code so that checkboxes and radio buttons do not count toward that threshold.

Thanks for catching this.
Done.

[Ilya Sherman, 2012-12-17 22:33:27]

https://chromiumcodereview.appspot.com/11415221/diff/84002/chrome/browser/aut...
File chrome/browser/autofill/autofill_xml_parser_unittest.cc (right):

https://chromiumcodereview.appspot.com/11415221/diff/84002/chrome/browser/aut...
chrome/browser/autofill/autofill_xml_parser_unittest.cc:200: // Update the
autofilltype to MAX_VALID_FIELD_TYPE when it changes.
nit: Please instead use MAX_VALID_FIELD_TYPE in the test directly, most likely
by using base::IntToString() from bas/string_number_conversions.h

https://chromiumcodereview.appspot.com/11415221/diff/84002/chrome/browser/aut...
File chrome/browser/autofill/form_field.cc (right):

https://chromiumcodereview.appspot.com/11415221/diff/84002/chrome/browser/aut...
chrome/browser/autofill/form_field.cc:66: remaining_fields.end());
Please add test coverage for this.

https://chromiumcodereview.appspot.com/11415221/diff/84002/chrome/browser/aut...
File chrome/browser/autofill/form_structure.cc (right):

https://chromiumcodereview.appspot.com/11415221/diff/84002/chrome/browser/aut...
chrome/browser/autofill/form_structure.cc:572: // or select elements.
Please add test coverage for this.

https://chromiumcodereview.appspot.com/11415221/diff/84002/chrome/browser/aut...
chrome/browser/autofill/form_structure.cc:573: if (field_count() -
checkable_field_count() < kRequiredFillableFields)
nit: Please add parentheses around the difference computation

[Raman Kakilate, 2012-12-18 01:54:04]

https://chromiumcodereview.appspot.com/11415221/diff/84002/chrome/browser/aut...
File chrome/browser/autofill/autofill_xml_parser_unittest.cc (right):

https://chromiumcodereview.appspot.com/11415221/diff/84002/chrome/browser/aut...
chrome/browser/autofill/autofill_xml_parser_unittest.cc:200: // Update the
autofilltype to MAX_VALID_FIELD_TYPE when it changes.
On 2012/12/17 22:33:27, Ilya Sherman wrote:
> nit: Please instead use MAX_VALID_FIELD_TYPE in the test directly, most likely
> by using base::IntToString() from bas/string_number_conversions.h

Done.

https://chromiumcodereview.appspot.com/11415221/diff/84002/chrome/browser/aut...
File chrome/browser/autofill/form_field.cc (right):

https://chromiumcodereview.appspot.com/11415221/diff/84002/chrome/browser/aut...
chrome/browser/autofill/form_field.cc:66: remaining_fields.end());
On 2012/12/17 22:33:27, Ilya Sherman wrote:
> Please add test coverage for this.

Done.

https://chromiumcodereview.appspot.com/11415221/diff/84002/chrome/browser/aut...
File chrome/browser/autofill/form_structure.cc (right):

https://chromiumcodereview.appspot.com/11415221/diff/84002/chrome/browser/aut...
chrome/browser/autofill/form_structure.cc:572: // or select elements.
On 2012/12/17 22:33:27, Ilya Sherman wrote:
> Please add test coverage for this.

Done.

https://chromiumcodereview.appspot.com/11415221/diff/84002/chrome/browser/aut...
chrome/browser/autofill/form_structure.cc:573: if (field_count() -
checkable_field_count() < kRequiredFillableFields)
On 2012/12/17 22:33:27, Ilya Sherman wrote:
> nit: Please add parentheses around the difference computation

Done.

[Ilya Sherman, 2012-12-18 02:05:10]

Last few nits; then this should be good to go.  Thanks!

https://chromiumcodereview.appspot.com/11415221/diff/87003/chrome/browser/aut...
File chrome/browser/autofill/form_field_unittest.cc (right):

https://chromiumcodereview.appspot.com/11415221/diff/87003/chrome/browser/aut...
chrome/browser/autofill/form_field_unittest.cc:119: AutofillField*
BuildTestField(const std::string& name) {
nit: Please rename the parameter to |label|, since that's how you're using it.

https://chromiumcodereview.appspot.com/11415221/diff/87003/chrome/browser/aut...
chrome/browser/autofill/form_field_unittest.cc:128: return new
AutofillField(field_data, field_data.label);
You should pretty much never return a raw pointer to heap-allocated memory, as
this is a recipe for leaks.  Instead, you can:

(a) Return a scoped_ptr<AutofillField>
(b) Pass in a pointer to an already allocated AutofillField, that this function
fills in.
(c) Return, by copy, a FormFieldData object, and construct the AutofillField
within the body of the test function.

https://chromiumcodereview.appspot.com/11415221/diff/87003/chrome/browser/aut...
chrome/browser/autofill/form_field_unittest.cc:129: }
nit: Please move this function definition into an anonymous namespace at the top
of this file.

[Raman Kakilate, 2012-12-18 03:26:14]

Inlined the function. PTAL.

On 2012/12/18 02:05:10, Ilya Sherman wrote:
> Last few nits; then this should be good to go.  Thanks!
> 
>
https://chromiumcodereview.appspot.com/11415221/diff/87003/chrome/browser/aut...
> File chrome/browser/autofill/form_field_unittest.cc (right):
> 
>
https://chromiumcodereview.appspot.com/11415221/diff/87003/chrome/browser/aut...
> chrome/browser/autofill/form_field_unittest.cc:119: AutofillField*
> BuildTestField(const std::string& name) {
> nit: Please rename the parameter to |label|, since that's how you're using it.
> 
>
https://chromiumcodereview.appspot.com/11415221/diff/87003/chrome/browser/aut...
> chrome/browser/autofill/form_field_unittest.cc:128: return new
> AutofillField(field_data, field_data.label);
> You should pretty much never return a raw pointer to heap-allocated memory, as
> this is a recipe for leaks.  Instead, you can:
> 
> (a) Return a scoped_ptr<AutofillField>
> (b) Pass in a pointer to an already allocated AutofillField, that this
function
> fills in.
> (c) Return, by copy, a FormFieldData object, and construct the AutofillField
> within the body of the test function.
> 
>
https://chromiumcodereview.appspot.com/11415221/diff/87003/chrome/browser/aut...
> chrome/browser/autofill/form_field_unittest.cc:129: }
> nit: Please move this function definition into an anonymous namespace at the
top
> of this file.

[Ilya Sherman, 2012-12-18 03:39:55]

LGTM, thanks :)

[commit-bot: I haz the power, 2012-12-18 03:43:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ramankk@chromium.org/11415221/82004

[commit-bot: I haz the power, 2012-12-18 04:26:49]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_clang_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[Ilya Sherman, 2012-12-18 04:32:40]

https://chromiumcodereview.appspot.com/11415221/diff/79005/chrome/browser/aut...
File chrome/browser/autofill/form_field.cc (right):

https://chromiumcodereview.appspot.com/11415221/diff/79005/chrome/browser/aut...
chrome/browser/autofill/form_field.cc:66: IsCheckable), remaining_fields.end());
nit: Please move the "remaining_fields.end()" argument to the next line.

[Raman Kakilate, 2012-12-18 04:37:32]

https://codereview.chromium.org/11415221/diff/79005/chrome/browser/autofill/f...
File chrome/browser/autofill/form_field.cc (right):

https://codereview.chromium.org/11415221/diff/79005/chrome/browser/autofill/f...
chrome/browser/autofill/form_field.cc:66: IsCheckable), remaining_fields.end());
On 2012/12/18 04:32:40, Ilya Sherman wrote:
> nit: Please move the "remaining_fields.end()" argument to the next line.

Done.

[commit-bot: I haz the power, 2012-12-18 06:39:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ramankk@chromium.org/11415221/91004

[commit-bot: I haz the power, 2012-12-19 01:47:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ramankk@chromium.org/11415221/91004

[commit-bot: I haz the power, 2012-12-19 10:08:47]

Change committed as 173889

[Albert Bodenhamer, 2013-01-08 17:12:54]

https://chromiumcodereview.appspot.com/11415221/diff/91004/chrome/browser/aut...
File chrome/browser/autofill/form_structure.cc (right):

https://chromiumcodereview.appspot.com/11415221/diff/91004/chrome/browser/aut...
chrome/browser/autofill/form_structure.cc:912: continue;
optional nit: I'm not fond of the use of continue in this way as I feel it makes
the code harder to follow.  Others feel differently.  Your call.

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

https://chromiumcodereview.appspot.com/11415221/diff/91004/chrome/renderer/au...
chrome/renderer/autofill/form_autofill_util.cc:562: return
element->formControlType() == ASCIIToUTF16("checkbox") ||
There was some evidence that this function was the cause of the slowdown. Right?
You should be able to create string16 constants for "checkbox" and "radio" in
order to avoid the overhead of constantly calling ASCIIToUTF16.

[Evan Stade, 2013-01-08 21:18:54]

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

https://chromiumcodereview.appspot.com/11415221/diff/91004/chrome/renderer/au...
chrome/renderer/autofill/form_autofill_util.cc:562: return
element->formControlType() == ASCIIToUTF16("checkbox") ||
On 2013/01/08 17:12:57, Albert Bodenhamer wrote:
> There was some evidence that this function was the cause of the slowdown.
Right?
> You should be able to create string16 constants for "checkbox" and "radio" in
> order to avoid the overhead of constantly calling ASCIIToUTF16.

ASCIIToUTF16 is cheap (it's just a copy, unlike UTF8ToUTF16).

That said, this code should use LowerCaseEqualsASCII()