Heuristics for grabber-continental.com.out (select-one)

Heuristics for grabber-continental.com.out (select-one)

Adds specific input types to the field_type.h bitfield to utilize the "select-one" signal in identifying the Country, State, and Credit Card date fields.  Other fields should not classify now if the input type is "select-one".

This is Phase 1 of fixes for the grabber-continental.com.html.  Phase 2 will add lookahead logic for the "BusinessPhone" and "Email Address" issues.

BUG=76299
TEST=FormStructureBrowserTest.DataDrivenHeuristics with test file grabber-continental.com.html

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

[honten.org, 2011-05-25 00:32:10]

So I don't have to change anything for grabber-continental.com.out anymore?

Should I close
http://codereview.chromium.org/7038003/
http://codereview.chromium.org/7014011/
?

Anyway, I'm a bit sad because I cannot commit anything but just analyzing.

On 2011/05/25 00:24:29, dhollowa wrote:

[dhollowa, 2011-05-25 00:42:10]

The analysis was extremely helpful.  But I needed to think this through fully
myself...  This patch is quite similar to yours.  But the look-ahead parts may
depart quite radically.

Once we get a solid foundation with the heuristics engine there will be a lot of
good bugs remaining with the other "grabber" files.  So you'll have plenty of
opportunity!  No worries.  :-)

On 2011/05/25 00:32:10, honten wrote:
> So I don't have to change anything for grabber-continental.com.out anymore?
> 
> Should I close
> http://codereview.chromium.org/7038003/
> http://codereview.chromium.org/7014011/
> ?
> 
> Anyway, I'm a bit sad because I cannot commit anything but just analyzing.
> 
> On 2011/05/25 00:24:29, dhollowa wrote:

[takano.naoki_gmail.com, 2011-05-25 00:47:04]

I can't wait to fix them!! ;-)

Anyway please let me know, once you finish it.

Right now, I'm mainly working on WebKit side while waiting you guys. I
already landed a couple of patches for WebKit.

Thanks,

On Tue, May 24, 2011 at 5:42 PM,  <dhollowa@chromium.org> wrote:
> The analysis was extremely helpful.  But I needed to think this through
> fully
> myself...  This patch is quite similar to yours.  But the look-ahead parts
> may
> depart quite radically.
>
> Once we get a solid foundation with the heuristics engine there will be a
> lot of
> good bugs remaining with the other "grabber" files.  So you'll have plenty
> of
> opportunity!  No worries.  :-)
>
> On 2011/05/25 00:32:10, honten wrote:
>>
>> So I don't have to change anything for grabber-continental.com.out
>> anymore?
>
>> Should I close
>> http://codereview.chromium.org/7038003/
>> http://codereview.chromium.org/7014011/
>> ?
>
>> Anyway, I'm a bit sad because I cannot commit anything but just analyzing.
>
>> On 2011/05/25 00:24:29, dhollowa wrote:
>
>
>
> http://codereview.chromium.org/7063031/
>

[Ilya Sherman, 2011-05-25 03:52:02]

http://codereview.chromium.org/7063031/diff/1/chrome/browser/autofill/form_fi...
File chrome/browser/autofill/form_field.cc (right):

http://codereview.chromium.org/7063031/diff/1/chrome/browser/autofill/form_fi...
chrome/browser/autofill/form_field.cc:152: return MatchAndAdvance(scanner,
pattern, match_type, match);
There don't seem to be any clients who ever call into this method with
MATCH_MONTH.  Presumably the credit card code should do that somewhere...

http://codereview.chromium.org/7063031/diff/1/chrome/browser/autofill/form_fi...
File chrome/browser/autofill/form_field.h (right):

http://codereview.chromium.org/7063031/diff/1/chrome/browser/autofill/form_fi...
chrome/browser/autofill/form_field.h:46: MATCH_TELEPHONE | MATCH_SELECT,
nit: This alignment looks off.

http://codereview.chromium.org/7063031/diff/1/chrome/browser/autofill/form_fi...
chrome/browser/autofill/form_field.h:97: const AutofillField** match);
nit: Can this be moved to the implementation file?

http://codereview.chromium.org/7063031/diff/1/chrome/browser/autofill/form_st...
File chrome/browser/autofill/form_structure_browsertest.cc (right):

http://codereview.chromium.org/7063031/diff/1/chrome/browser/autofill/form_st...
chrome/browser/autofill/form_structure_browsertest.cc:87: forms_string += "|";
Did you intentionally to leave these in, or was this for debugging?

[dhollowa, 2011-05-25 15:29:47]

http://codereview.chromium.org/7063031/diff/1/chrome/browser/autofill/form_fi...
File chrome/browser/autofill/form_field.cc (right):

http://codereview.chromium.org/7063031/diff/1/chrome/browser/autofill/form_fi...
chrome/browser/autofill/form_field.cc:152: return MatchAndAdvance(scanner,
pattern, match_type, match);
The credit card code currently explicitly looks for "month" type and matches
based on that context.  It is a bit of a special case.  I'll remove this for
now, but I'm expecting we'll want to bring it back when we rework that code.

On 2011/05/25 03:52:02, Ilya Sherman wrote:
> There don't seem to be any clients who ever call into this method with
> MATCH_MONTH.  Presumably the credit card code should do that somewhere...

http://codereview.chromium.org/7063031/diff/1/chrome/browser/autofill/form_fi...
File chrome/browser/autofill/form_field.h (right):

http://codereview.chromium.org/7063031/diff/1/chrome/browser/autofill/form_fi...
chrome/browser/autofill/form_field.h:46: MATCH_TELEPHONE | MATCH_SELECT,
On 2011/05/25 03:52:02, Ilya Sherman wrote:
> nit: This alignment looks off.

Done.

http://codereview.chromium.org/7063031/diff/1/chrome/browser/autofill/form_fi...
chrome/browser/autofill/form_field.h:97: const AutofillField** match);
This needs to call private method |Match| (which is used in tests) so I needed
to put this here.

On 2011/05/25 03:52:02, Ilya Sherman wrote:
> nit: Can this be moved to the implementation file?

http://codereview.chromium.org/7063031/diff/1/chrome/browser/autofill/form_st...
File chrome/browser/autofill/form_structure_browsertest.cc (right):

http://codereview.chromium.org/7063031/diff/1/chrome/browser/autofill/form_st...
chrome/browser/autofill/form_structure_browsertest.cc:87: forms_string += "|";
Debugging.

On 2011/05/25 03:52:02, Ilya Sherman wrote:
> Did you intentionally to leave these in, or was this for debugging?

[Ilya Sherman, 2011-05-25 19:17:28]

LGTM, thanks

http://codereview.chromium.org/7063031/diff/1/chrome/browser/autofill/form_fi...
File chrome/browser/autofill/form_field.h (right):

http://codereview.chromium.org/7063031/diff/1/chrome/browser/autofill/form_fi...
chrome/browser/autofill/form_field.h:97: const AutofillField** match);
On 2011/05/25 15:29:47, dhollowa wrote:
> This needs to call private method |Match| (which is used in tests) so I needed
> to put this here.

Ah, ok.

http://codereview.chromium.org/7063031/diff/6001/chrome/browser/autofill/form...
File chrome/browser/autofill/form_field.cc (right):

http://codereview.chromium.org/7063031/diff/6001/chrome/browser/autofill/form...
chrome/browser/autofill/form_field.cc:152: &&
IsTelephoneField(field->form_control_type)) {
nit: The "&&" should be on the previous line.

http://codereview.chromium.org/7063031/diff/6001/chrome/browser/autofill/form...
File chrome/browser/autofill/form_field.h (right):

http://codereview.chromium.org/7063031/diff/6001/chrome/browser/autofill/form...
chrome/browser/autofill/form_field.h:47: // By default match label, name, for
input/text types.
nit: "label, name, for" -> "label and name for"

[dhollowa, 2011-05-25 21:55:42]

http://codereview.chromium.org/7063031/diff/6001/chrome/browser/autofill/form...
File chrome/browser/autofill/form_field.cc (right):

http://codereview.chromium.org/7063031/diff/6001/chrome/browser/autofill/form...
chrome/browser/autofill/form_field.cc:152: &&
IsTelephoneField(field->form_control_type)) {
On 2011/05/25 19:17:28, Ilya Sherman wrote:
> nit: The "&&" should be on the previous line.

Done.

http://codereview.chromium.org/7063031/diff/6001/chrome/browser/autofill/form...
File chrome/browser/autofill/form_field.h (right):

http://codereview.chromium.org/7063031/diff/6001/chrome/browser/autofill/form...
chrome/browser/autofill/form_field.h:47: // By default match label, name, for
input/text types.
On 2011/05/25 19:17:28, Ilya Sherman wrote:
> nit: "label, name, for" -> "label and name for"

Done.