Set datapresent string to contain precisely those field types available in stored Autofill data.

Set datapresent string to contain precisely those field types available in stored Autofill data.

This is what the toolbar server expects of all of its clients.

BUG=none
TEST=unit_tests --gtest_filter=PersonalDataManagerTest.GetAvailableFieldTypes:FormStructureTest.*

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

[Ilya Sherman, 2011-05-05 07:47:32]

If there are any other cases that you think would be useful to verify in the
unit tests, please let me know.

[dhollowa, 2011-05-06 03:06:19]

http://codereview.chromium.org/6931029/diff/2001/chrome/browser/autofill/auto...
File chrome/browser/autofill/autofill_manager.cc (right):

http://codereview.chromium.org/6931029/diff/2001/chrome/browser/autofill/auto...
chrome/browser/autofill/autofill_manager.cc:566: if
(autofilled_form_signatures_.size() > 3)
It is not clear why this is here.  Could you explain?

http://codereview.chromium.org/6931029/diff/2001/chrome/browser/autofill/form...
File chrome/browser/autofill/form_structure.cc (right):

http://codereview.chromium.org/6931029/diff/2001/chrome/browser/autofill/form...
chrome/browser/autofill/form_structure.cc:152: for (FieldTypeSet::const_iterator
type = (*field)->possible_types().begin();
Is it just me or are the names "possible" and "available" deeply confounding? 
How about "matching" and "non_empty" or something?  Not essential for this patch
but would be really nice I think.

http://codereview.chromium.org/6931029/diff/2001/chrome/browser/autofill/form...
File chrome/browser/autofill/form_structure_unittest.cc (right):

http://codereview.chromium.org/6931029/diff/2001/chrome/browser/autofill/form...
chrome/browser/autofill/form_structure_unittest.cc:1495: // Check that we
computer the "datapresent" string correctly for the given
s/computer/compute/

http://codereview.chromium.org/6931029/diff/2001/chrome/browser/autofill/form...
chrome/browser/autofill/form_structure_unittest.cc:1547: // The set bits are:
Great comments, very useful.  It makes me wonder however if we should have a
datapresent decoder that outputs a human-readable string.  Maybe a good
candidate for a data-driven set of tests?

http://codereview.chromium.org/6931029/diff/1014/chrome/browser/autofill/form...
chrome/browser/autofill/form_structure_unittest.cc:1387: // TODO(isherman):
TODO?

http://codereview.chromium.org/6931029/diff/1014/chrome/browser/autofill/pers...
File chrome/browser/autofill/personal_data_manager.cc (right):

http://codereview.chromium.org/6931029/diff/1014/chrome/browser/autofill/pers...
chrome/browser/autofill/personal_data_manager.cc:618: 
nit: remove extra line.

[Ilya Sherman, 2011-05-06 03:33:07]

http://codereview.chromium.org/6931029/diff/2001/chrome/browser/autofill/auto...
File chrome/browser/autofill/autofill_manager.cc (right):

http://codereview.chromium.org/6931029/diff/2001/chrome/browser/autofill/auto...
chrome/browser/autofill/autofill_manager.cc:566: if
(autofilled_form_signatures_.size() > 3)
On 2011/05/06 03:06:20, dhollowa wrote:
> It is not clear why this is here.  Could you explain?

Made '3' a named constant, and added a brief comment.

http://codereview.chromium.org/6931029/diff/2001/chrome/browser/autofill/form...
File chrome/browser/autofill/form_structure.cc (right):

http://codereview.chromium.org/6931029/diff/2001/chrome/browser/autofill/form...
chrome/browser/autofill/form_structure.cc:152: for (FieldTypeSet::const_iterator
type = (*field)->possible_types().begin();
On 2011/05/06 03:06:20, dhollowa wrote:
> Is it just me or are the names "possible" and "available" deeply confounding? 
> How about "matching" and "non_empty" or something?  Not essential for this
patch
> but would be really nice I think.

I agree.  Filed as a follow-up bug:
http://code.google.com/p/chromium/issues/detail?id=81767

http://codereview.chromium.org/6931029/diff/2001/chrome/browser/autofill/form...
File chrome/browser/autofill/form_structure_unittest.cc (right):

http://codereview.chromium.org/6931029/diff/2001/chrome/browser/autofill/form...
chrome/browser/autofill/form_structure_unittest.cc:1495: // Check that we
computer the "datapresent" string correctly for the given
On 2011/05/06 03:06:20, dhollowa wrote:
> s/computer/compute/

Done.

http://codereview.chromium.org/6931029/diff/2001/chrome/browser/autofill/form...
chrome/browser/autofill/form_structure_unittest.cc:1547: // The set bits are:
On 2011/05/06 03:06:20, dhollowa wrote:
> Great comments, very useful.  It makes me wonder however if we should have a
> datapresent decoder that outputs a human-readable string.  Maybe a good
> candidate for a data-driven set of tests?

It's just a quick python script away.  I'm not convinced that we really need a
test that runs on each commit cycle though -- what code would it be
exercising/testing?

http://codereview.chromium.org/6931029/diff/1014/chrome/browser/autofill/form...
chrome/browser/autofill/form_structure_unittest.cc:1387: // TODO(isherman):
On 2011/05/06 03:06:20, dhollowa wrote:
> TODO?

Oops, to-done. =)

http://codereview.chromium.org/6931029/diff/1014/chrome/browser/autofill/pers...
File chrome/browser/autofill/personal_data_manager.cc (right):

http://codereview.chromium.org/6931029/diff/1014/chrome/browser/autofill/pers...
chrome/browser/autofill/personal_data_manager.cc:618: 
On 2011/05/06 03:06:20, dhollowa wrote:
> nit: remove extra line.

Done.

[dhollowa, 2011-05-06 04:12:49]

LGTM.  Thanks.

http://codereview.chromium.org/6931029/diff/2001/chrome/browser/autofill/form...
File chrome/browser/autofill/form_structure_unittest.cc (right):

http://codereview.chromium.org/6931029/diff/2001/chrome/browser/autofill/form...
chrome/browser/autofill/form_structure_unittest.cc:1547: // The set bits are:
Yes, it is a small finite set of combinations.  So data-driven tests are
overkill, I agree.

I do like the idea however of a small |DatapresentToString(...)| utility that
spits out essentially what you have in these comments here.  Then if these tests
ever fail we don't have to manually decode bit-fields.

This is just a suggestion though.  I'll leave it up to you.

On 2011/05/06 03:33:07, Ilya Sherman wrote:
> On 2011/05/06 03:06:20, dhollowa wrote:
> > Great comments, very useful.  It makes me wonder however if we should have a
> > datapresent decoder that outputs a human-readable string.  Maybe a good
> > candidate for a data-driven set of tests?
> 
> It's just a quick python script away.  I'm not convinced that we really need a
> test that runs on each commit cycle though -- what code would it be
> exercising/testing?