Change heuristic regex and order to match grabber-continental.

Change heuristic regex and order to match grabber-continental.

To support grabber-continental form, change the following part.
1, Delete "department" from company regex.
2, Insert email and phone check in address field loop.
3, Add |select_one_is_one| in FormField::Pattern and change |ParseText| to pass Pattern from string16.
4, Add |RewindTo()| function to rewind to specific cursor position.

BUG=76299
TEST=1, put grabber-continental.html in heuristic/input. 2, Run browser_tests --gtest_filter=*.DataDrivenHeuristics and see heuristic/output.

[honten.org, 2011-05-12 05:36:33]

Please review.

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

http://codereview.chromium.org/7014011/diff/1/chrome/browser/autofill/form_fi...
chrome/browser/autofill/form_field.h:86: Pattern(const string16& pattern)
I know we need to add explicit.
But before adding explicit, I want to make sure the Pattern struct usage is
suitable or not.

[honten.org, 2011-05-12 22:39:31]

Please review.

On 2011/05/12 05:36:33, honten wrote:
> Please review.
> 
>
http://codereview.chromium.org/7014011/diff/1/chrome/browser/autofill/form_fi...
> File chrome/browser/autofill/form_field.h (right):
> 
>
http://codereview.chromium.org/7014011/diff/1/chrome/browser/autofill/form_fi...
> chrome/browser/autofill/form_field.h:86: Pattern(const string16& pattern)
> I know we need to add explicit.
> But before adding explicit, I want to make sure the Pattern struct usage is
> suitable or not.

[Ilya Sherman, 2011-05-13 00:54:53]

If you prefer, I can make the suggested changes to AutofillScanner in a separate
CL.  Let me know if you'd like me to do that.

http://codereview.chromium.org/7014011/diff/1/chrome/browser/autofill/autofil...
File chrome/browser/autofill/autofill_scanner.cc (right):

http://codereview.chromium.org/7014011/diff/1/chrome/browser/autofill/autofil...
chrome/browser/autofill/autofill_scanner.cc:45: saved_cursors_.pop_back();
(See also the comment below)

Let's instead have this be

cursor_ = start_ + position;

http://codereview.chromium.org/7014011/diff/1/chrome/browser/autofill/autofil...
chrome/browser/autofill/autofill_scanner.cc:50: size_t index =
saved_cursors_.size();
Let's instead have this be

size_t index = cursor_ - start_;

where |start_| is captured in the constructor as the value of |fields.begin()|

http://codereview.chromium.org/7014011/diff/1/chrome/browser/autofill/autofil...
File chrome/browser/autofill/autofill_scanner.h (right):

http://codereview.chromium.org/7014011/diff/1/chrome/browser/autofill/autofil...
chrome/browser/autofill/autofill_scanner.h:48:
std::vector<std::vector<AutofillField*>::const_iterator> saved_cursors_;
Since we need to expose RewindTo() functionality anyway, let's not also provide
stack semantics as we have here.  So, let's change this to be 

// The most recently saved position in the stream.
std::vector<AutofillField*>::const_iterator saved_cursor_;

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

http://codereview.chromium.org/7014011/diff/1/chrome/browser/autofill/form_fi...
chrome/browser/autofill/form_field.h:86: Pattern(const string16& pattern)
On 2011/05/12 05:36:33, honten wrote:
> I know we need to add explicit.
> But before adding explicit, I want to make sure the Pattern struct usage is
> suitable or not.

Let's instead create an enum along the lines of

enum MatchFormControlType {
  MATCH_ONLY_TEXT = 0,
  MATCH_TEXT_AND_SELECT
};

and pass a value of this type as an additional parameter to ParseText().

[honten.org, 2011-05-13 01:39:48]

> If you prefer, I can make the suggested changes to AutofillScanner in a
separate
> CL.  Let me know if you'd like me to do that.
Yes please, after this patch landing;-)

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

http://codereview.chromium.org/7014011/diff/1/chrome/browser/autofill/form_fi...
chrome/browser/autofill/form_field.h:86: Pattern(const string16& pattern)
After this change, we might need to implement more pattern parameter (e.g.
only_check_label or do_not_check_inferred_label) in the future, as I and David
discussed.

In that case, we need to add those parameters again and again every time. That's
the reason why I declared the sturct.

But, even if it is OK, I will change the parameters as an additional parameter.

What do you think?

On 2011/05/13 00:54:53, Ilya Sherman wrote:
> On 2011/05/12 05:36:33, honten wrote:
> > I know we need to add explicit.
> > But before adding explicit, I want to make sure the Pattern struct usage is
> > suitable or not.
> 
> Let's instead create an enum along the lines of
> 
> enum MatchFormControlType {
>   MATCH_ONLY_TEXT = 0,
>   MATCH_TEXT_AND_SELECT
> };
> 
> and pass a value of this type as an additional parameter to ParseText().

[takano.naoki_gmail.com, 2011-05-13 20:22:34]

Ilya or David,

Could you give me your comment or thought?

On Thu, May 12, 2011 at 6:39 PM,  <Takano.Naoki@gmail.com> wrote:
>> If you prefer, I can make the suggested changes to AutofillScanner in a
>
> separate
>>
>> CL.  Let me know if you'd like me to do that.
>
> Yes please, after this patch landing;-)
>
>
>
http://codereview.chromium.org/7014011/diff/1/chrome/browser/autofill/form_fi...
> File chrome/browser/autofill/form_field.h (right):
>
>
http://codereview.chromium.org/7014011/diff/1/chrome/browser/autofill/form_fi...
> chrome/browser/autofill/form_field.h:86: Pattern(const string16&
> pattern)
> After this change, we might need to implement more pattern parameter
> (e.g. only_check_label or do_not_check_inferred_label) in the future, as
> I and David discussed.
>
> In that case, we need to add those parameters again and again every
> time. That's the reason why I declared the sturct.
>
> But, even if it is OK, I will change the parameters as an additional
> parameter.
>
> What do you think?
>
> On 2011/05/13 00:54:53, Ilya Sherman wrote:
>>
>> On 2011/05/12 05:36:33, honten wrote:
>> > I know we need to add explicit.
>> > But before adding explicit, I want to make sure the Pattern struct
>
> usage is
>>
>> > suitable or not.
>
>> Let's instead create an enum along the lines of
>
>> enum MatchFormControlType {
>>   MATCH_ONLY_TEXT = 0,
>>   MATCH_TEXT_AND_SELECT
>> };
>
>> and pass a value of this type as an additional parameter to
>
> ParseText().
>
> http://codereview.chromium.org/7014011/
>

[dhollowa, 2011-05-14 01:23:26]

Looking...  The look-ahead with the scanner reset seems like it could be
simpler.  But I'm looking into how we might approach this.

On 2011/05/13 20:22:34, takano.naoki_gmail.com wrote:
> Ilya or David,
> 
> Could you give me your comment or thought?
> 
> On Thu, May 12, 2011 at 6:39 PM,  <mailto:Takano.Naoki@gmail.com> wrote:
> >> If you prefer, I can make the suggested changes to AutofillScanner in a
> >
> > separate
> >>
> >> CL. &nbsp;Let me know if you'd like me to do that.
> >
> > Yes please, after this patch landing;-)
> >
> >
> >
>
http://codereview.chromium.org/7014011/diff/1/chrome/browser/autofill/form_fi...
> > File chrome/browser/autofill/form_field.h (right):
> >
> >
>
http://codereview.chromium.org/7014011/diff/1/chrome/browser/autofill/form_fi...
> > chrome/browser/autofill/form_field.h:86: Pattern(const string16&
> > pattern)
> > After this change, we might need to implement more pattern parameter
> > (e.g. only_check_label or do_not_check_inferred_label) in the future, as
> > I and David discussed.
> >
> > In that case, we need to add those parameters again and again every
> > time. That's the reason why I declared the sturct.
> >
> > But, even if it is OK, I will change the parameters as an additional
> > parameter.
> >
> > What do you think?
> >
> > On 2011/05/13 00:54:53, Ilya Sherman wrote:
> >>
> >> On 2011/05/12 05:36:33, honten wrote:
> >> > I know we need to add explicit.
> >> > But before adding explicit, I want to make sure the Pattern struct
> >
> > usage is
> >>
> >> > suitable or not.
> >
> >> Let's instead create an enum along the lines of
> >
> >> enum MatchFormControlType {
> >> &nbsp; MATCH_ONLY_TEXT = 0,
> >> &nbsp; MATCH_TEXT_AND_SELECT
> >> };
> >
> >> and pass a value of this type as an additional parameter to
> >
> > ParseText().
> >
> > http://codereview.chromium.org/7014011/
> >

[takano.naoki_gmail.com, 2011-05-14 01:59:56]

Thanks, David.

How about parameter things for ParseText()?

On Fri, May 13, 2011 at 6:23 PM,  <dhollowa@chromium.org> wrote:
> Looking...  The look-ahead with the scanner reset seems like it could be
> simpler.  But I'm looking into how we might approach this.
>
> On 2011/05/13 20:22:34, takano.naoki_gmail.com wrote:
>>
>> Ilya or David,
>
>> Could you give me your comment or thought?
>
>> On Thu, May 12, 2011 at 6:39 PM,  <mailto:Takano.Naoki@gmail.com> wrote:
>> >> If you prefer, I can make the suggested changes to AutofillScanner in a
>> >
>> > separate
>> >>
>> >> CL. &nbsp;Let me know if you'd like me to do that.
>> >
>> > Yes please, after this patch landing;-)
>> >
>> >
>> >
>
>
http://codereview.chromium.org/7014011/diff/1/chrome/browser/autofill/form_fi...
>>
>> > File chrome/browser/autofill/form_field.h (right):
>> >
>> >
>
>
http://codereview.chromium.org/7014011/diff/1/chrome/browser/autofill/form_fi...
>>
>> > chrome/browser/autofill/form_field.h:86: Pattern(const string16&
>> > pattern)
>> > After this change, we might need to implement more pattern parameter
>> > (e.g. only_check_label or do_not_check_inferred_label) in the future, as
>> > I and David discussed.
>> >
>> > In that case, we need to add those parameters again and again every
>> > time. That's the reason why I declared the sturct.
>> >
>> > But, even if it is OK, I will change the parameters as an additional
>> > parameter.
>> >
>> > What do you think?
>> >
>> > On 2011/05/13 00:54:53, Ilya Sherman wrote:
>> >>
>> >> On 2011/05/12 05:36:33, honten wrote:
>> >> > I know we need to add explicit.
>> >> > But before adding explicit, I want to make sure the Pattern struct
>> >
>> > usage is
>> >>
>> >> > suitable or not.
>> >
>> >> Let's instead create an enum along the lines of
>> >
>> >> enum MatchFormControlType {
>> >> &nbsp; MATCH_ONLY_TEXT = 0,
>> >> &nbsp; MATCH_TEXT_AND_SELECT
>> >> };
>> >
>> >> and pass a value of this type as an additional parameter to
>> >
>> > ParseText().
>> >
>> > http://codereview.chromium.org/7014011/
>> >
>
>
>
> http://codereview.chromium.org/7014011/
>

[dhollowa, 2011-05-14 02:15:50]

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

http://codereview.chromium.org/7014011/diff/1/chrome/browser/autofill/form_fi...
chrome/browser/autofill/form_field.h:86: Pattern(const string16& pattern)
I think this may be a good opportunity to use a bit-field.  Something like:

enum MatchType {
  MATCH_LABEL = 1 << 0,
  MATCH_NAME = 1 << 1,
  MATCH_REQUIRES_SELECT = 1 << 2
  MATCH_EMPTY = 1 << 3
};

Then we could try to consolidate FormField::ParseLabelAndName,
FormField::ParseLabelText, and all that mess.

On 2011/05/13 01:39:48, honten wrote:
> After this change, we might need to implement more pattern parameter (e.g.
> only_check_label or do_not_check_inferred_label) in the future, as I and David
> discussed.
> 
> In that case, we need to add those parameters again and again every time.
That's
> the reason why I declared the sturct.
> 
> But, even if it is OK, I will change the parameters as an additional
parameter.
> 
> What do you think?
> 
> On 2011/05/13 00:54:53, Ilya Sherman wrote:
> > On 2011/05/12 05:36:33, honten wrote:
> > > I know we need to add explicit.
> > > But before adding explicit, I want to make sure the Pattern struct usage
is
> > > suitable or not.
> > 
> > Let's instead create an enum along the lines of
> > 
> > enum MatchFormControlType {
> >   MATCH_ONLY_TEXT = 0,
> >   MATCH_TEXT_AND_SELECT
> > };
> > 
> > and pass a value of this type as an additional parameter to ParseText().
>

[takano.naoki_gmail.com, 2011-05-14 02:21:38]

That makes sense;-)

Ok, I'll do that.

On Fri, May 13, 2011 at 7:15 PM,  <dhollowa@chromium.org> wrote:
>
>
http://codereview.chromium.org/7014011/diff/1/chrome/browser/autofill/form_fi...
> File chrome/browser/autofill/form_field.h (right):
>
>
http://codereview.chromium.org/7014011/diff/1/chrome/browser/autofill/form_fi...
> chrome/browser/autofill/form_field.h:86: Pattern(const string16&
> pattern)
> I think this may be a good opportunity to use a bit-field.  Something
> like:
>
> enum MatchType {
>  MATCH_LABEL = 1 << 0,
>  MATCH_NAME = 1 << 1,
>  MATCH_REQUIRES_SELECT = 1 << 2
>  MATCH_EMPTY = 1 << 3
> };
>
> Then we could try to consolidate FormField::ParseLabelAndName,
> FormField::ParseLabelText, and all that mess.
>
> On 2011/05/13 01:39:48, honten wrote:
>>
>> After this change, we might need to implement more pattern parameter
>
> (e.g.
>>
>> only_check_label or do_not_check_inferred_label) in the future, as I
>
> and David
>>
>> discussed.
>
>> In that case, we need to add those parameters again and again every
>
> time. That's
>>
>> the reason why I declared the sturct.
>
>> But, even if it is OK, I will change the parameters as an additional
>
> parameter.
>
>> What do you think?
>
>> On 2011/05/13 00:54:53, Ilya Sherman wrote:
>> > On 2011/05/12 05:36:33, honten wrote:
>> > > I know we need to add explicit.
>> > > But before adding explicit, I want to make sure the Pattern struct
>
> usage is
>>
>> > > suitable or not.
>> >
>> > Let's instead create an enum along the lines of
>> >
>> > enum MatchFormControlType {
>> >   MATCH_ONLY_TEXT = 0,
>> >   MATCH_TEXT_AND_SELECT
>> > };
>> >
>> > and pass a value of this type as an additional parameter to
>
> ParseText().
>
>
> http://codereview.chromium.org/7014011/
>

[honten.org, 2011-05-14 06:35:23]

Please review again.

[Ilya Sherman, 2011-05-14 06:59:12]

http://codereview.chromium.org/7014011/diff/1/chrome/browser/autofill/address...
File chrome/browser/autofill/address_field.cc (right):

http://codereview.chromium.org/7014011/diff/1/chrome/browser/autofill/address...
chrome/browser/autofill/address_field.cc:97: break;
David and I discussed this a bit offline today -- I'll try to rehash/summarize
that here:  Rather than changing the parsing for address fields, let's try
making multiple passes over the form in FormField::ParseFormFields().  In the
first pass, we can check for any email fields; in the second pass, we can check
for any phone fields; in the third pass, for any address fields; and so on. 
This should both add a bit more clarity to the code; and also give us a bit more
flexibility for parsing.

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

http://codereview.chromium.org/7014011/diff/8001/chrome/browser/autofill/form...
chrome/browser/autofill/form_field.cc:275: field->form_control_type ==
ASCIIToUTF16("text"))
What about 'email', 'tel', and so on?  Are there any clients for which
MATCH_TEXT is not set?

http://codereview.chromium.org/7014011/diff/8001/chrome/browser/autofill/form...
chrome/browser/autofill/form_field.cc:279: !(match_type & MATCH_NAME);
This computation doesn't seem right to me -- if MATCH_LABEL isn't set, then we
end up matching against both the name and the label?  We should refactor Match()
to also take a match type -- or perhaps refactor the code that calls into the
Match() method to call the submethods directly.

[Ilya Sherman, 2011-05-14 07:01:32]

http://codereview.chromium.org/7014011/diff/1/chrome/browser/autofill/address...
File chrome/browser/autofill/address_field.cc (right):

http://codereview.chromium.org/7014011/diff/1/chrome/browser/autofill/address...
chrome/browser/autofill/address_field.cc:97: break;
On 2011/05/14 06:59:12, Ilya Sherman wrote:
> David and I discussed this a bit offline today -- I'll try to rehash/summarize
> that here:  Rather than changing the parsing for address fields, let's try
> making multiple passes over the form in FormField::ParseFormFields().  In the
> first pass, we can check for any email fields; in the second pass, we can
check
> for any phone fields; in the third pass, for any address fields; and so on. 
> This should both add a bit more clarity to the code; and also give us a bit
more
> flexibility for parsing.

Whoops, didn't mean to send this comment out -- wanted to let David think about
this some more and then add his thoughts.  So, I guess for now keep this in mind
as a possibility, but not necessarily the best one.

[honten.org, 2011-05-14 07:30:05]

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

http://codereview.chromium.org/7014011/diff/8001/chrome/browser/autofill/form...
chrome/browser/autofill/form_field.cc:275: field->form_control_type ==
ASCIIToUTF16("text"))
Yeah, you are right.

I completely forgot HTML5 tags;-)

Let me think about it.

On 2011/05/14 06:59:12, Ilya Sherman wrote:
> What about 'email', 'tel', and so on?  Are there any clients for which
> MATCH_TEXT is not set?

http://codereview.chromium.org/7014011/diff/8001/chrome/browser/autofill/form...
chrome/browser/autofill/form_field.cc:279: !(match_type & MATCH_NAME);
Actually, I wanted to refactor Match(), but it is used heavily in tests. And it
take a while to rewrite everything...

So can I prepare new function like MatchWithBitmap() to refactor?

On 2011/05/14 06:59:12, Ilya Sherman wrote:
> This computation doesn't seem right to me -- if MATCH_LABEL isn't set, then we
> end up matching against both the name and the label?  We should refactor
Match()
> to also take a match type -- or perhaps refactor the code that calls into the
> Match() method to call the submethods directly.

[honten.org, 2011-05-14 07:41:20]

http://codereview.chromium.org/7014011/diff/1/chrome/browser/autofill/address...
File chrome/browser/autofill/address_field.cc (right):

http://codereview.chromium.org/7014011/diff/1/chrome/browser/autofill/address...
chrome/browser/autofill/address_field.cc:97: break;
I see, it might be better idea.

But, at first, I want to refactor ParseText() with bitmap. I want to commit
gradually;-)

On 2011/05/14 07:01:33, Ilya Sherman wrote:
> On 2011/05/14 06:59:12, Ilya Sherman wrote:
> > David and I discussed this a bit offline today -- I'll try to
rehash/summarize
> > that here:  Rather than changing the parsing for address fields, let's try
> > making multiple passes over the form in FormField::ParseFormFields().  In
the
> > first pass, we can check for any email fields; in the second pass, we can
> check
> > for any phone fields; in the third pass, for any address fields; and so on. 
> > This should both add a bit more clarity to the code; and also give us a bit
> more
> > flexibility for parsing.
> 
> Whoops, didn't mean to send this comment out -- wanted to let David think
about
> this some more and then add his thoughts.  So, I guess for now keep this in
mind
> as a possibility, but not necessarily the best one.

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

http://codereview.chromium.org/7014011/diff/8001/chrome/browser/autofill/form...
chrome/browser/autofill/form_field.cc:275: field->form_control_type ==
ASCIIToUTF16("text"))
As I used to did in form_manager, maybe we should also add isTextField() in
FormField, down't we?

And we define
MATCH_ANY_TEXT,
MATCH_TEXT,
MATCH_EMAIL,
MATCH_DATE,
etc...

MATCH_ANY_TEXT checks isTextField flag and others check form_controy_type.

What do you think?

On 2011/05/14 07:30:05, honten wrote:
> Yeah, you are right.
> 
> I completely forgot HTML5 tags;-)
> 
> Let me think about it.
> 
> On 2011/05/14 06:59:12, Ilya Sherman wrote:
> > What about 'email', 'tel', and so on?  Are there any clients for which
> > MATCH_TEXT is not set?
>

[dhollowa, 2011-05-16 14:53:31]

This patch is getting too broad.  Let's split it into separate patches for the
different aspects of the refactorings.  I would propose separate patches for:

 1. .grd change (easy)
 2. the MatchType bit field refactoring
 3. two-pass solution for "address" versus "email address" matching

As a side note, Ilya and I are participating in a three day summit this week, so
may be less responsive until Thursday.


On 2011/05/14 07:41:20, honten wrote:
>
http://codereview.chromium.org/7014011/diff/1/chrome/browser/autofill/address...
> File chrome/browser/autofill/address_field.cc (right):
> 
>
http://codereview.chromium.org/7014011/diff/1/chrome/browser/autofill/address...
> chrome/browser/autofill/address_field.cc:97: break;
> I see, it might be better idea.
> 
> But, at first, I want to refactor ParseText() with bitmap. I want to commit
> gradually;-)
> 
> On 2011/05/14 07:01:33, Ilya Sherman wrote:
> > On 2011/05/14 06:59:12, Ilya Sherman wrote:
> > > David and I discussed this a bit offline today -- I'll try to
> rehash/summarize
> > > that here:  Rather than changing the parsing for address fields, let's try
> > > making multiple passes over the form in FormField::ParseFormFields().  In
> the
> > > first pass, we can check for any email fields; in the second pass, we can
> > check
> > > for any phone fields; in the third pass, for any address fields; and so
on. 
> > > This should both add a bit more clarity to the code; and also give us a
bit
> > more
> > > flexibility for parsing.
> > 
> > Whoops, didn't mean to send this comment out -- wanted to let David think
> about
> > this some more and then add his thoughts.  So, I guess for now keep this in
> mind
> > as a possibility, but not necessarily the best one.
> 
>
http://codereview.chromium.org/7014011/diff/8001/chrome/browser/autofill/form...
> File chrome/browser/autofill/form_field.cc (right):
> 
>
http://codereview.chromium.org/7014011/diff/8001/chrome/browser/autofill/form...
> chrome/browser/autofill/form_field.cc:275: field->form_control_type ==
> ASCIIToUTF16("text"))
> As I used to did in form_manager, maybe we should also add isTextField() in
> FormField, down't we?
> 
> And we define
> MATCH_ANY_TEXT,
> MATCH_TEXT,
> MATCH_EMAIL,
> MATCH_DATE,
> etc...
> 
> MATCH_ANY_TEXT checks isTextField flag and others check form_controy_type.
> 
> What do you think?
> 
> On 2011/05/14 07:30:05, honten wrote:
> > Yeah, you are right.
> > 
> > I completely forgot HTML5 tags;-)
> > 
> > Let me think about it.
> > 
> > On 2011/05/14 06:59:12, Ilya Sherman wrote:
> > > What about 'email', 'tel', and so on?  Are there any clients for which
> > > MATCH_TEXT is not set?
> >

[takano.naoki_gmail.com, 2011-05-16 16:52:39]

David,

Thank you for your comment.

On Mon, May 16, 2011 at 7:53 AM,  <dhollowa@chromium.org> wrote:
> This patch is getting too broad.  Let's split it into separate patches for
> the
> different aspects of the refactorings.  I would propose separate patches
> for:
>
>  1. .grd change (easy)
Ok, I'll do that first.

>  2. the MatchType bit field refactoring
About this, could you give me your comment?

The following, is my questions.

As I used to did in form_manager, maybe we should also add isTextField()
in FormField, down't we?

And we define
MATCH_ANY_TEXT,
MATCH_TEXT,
MATCH_EMAIL,
MATCH_DATE,
etc...

MATCH_ANY_TEXT checks isTextField flag and others check


Also Match() is already used broadly in test, so it's hard to find it at once.
Can I prepare the new function Match2() ?

>  3. two-pass solution for "address" versus "email address" matching
Ok,

Ilya already suggested me we might need to change the pass parsing,
how should I do?


> As a side note, Ilya and I are participating in a three day summit this
> week, so
> may be less responsive until Thursday.
Ok,

Thanks,

>
>
> On 2011/05/14 07:41:20, honten wrote:
>
>
http://codereview.chromium.org/7014011/diff/1/chrome/browser/autofill/address...
>>
>> File chrome/browser/autofill/address_field.cc (right):
>
>
>
http://codereview.chromium.org/7014011/diff/1/chrome/browser/autofill/address...
>>
>> chrome/browser/autofill/address_field.cc:97: break;
>> I see, it might be better idea.
>
>> But, at first, I want to refactor ParseText() with bitmap. I want to
>> commit
>> gradually;-)
>
>> On 2011/05/14 07:01:33, Ilya Sherman wrote:
>> > On 2011/05/14 06:59:12, Ilya Sherman wrote:
>> > > David and I discussed this a bit offline today -- I'll try to
>> rehash/summarize
>> > > that here:  Rather than changing the parsing for address fields, let's
>> > > try
>> > > making multiple passes over the form in FormField::ParseFormFields().
>> > >  In
>> the
>> > > first pass, we can check for any email fields; in the second pass, we
>> > > can
>> > check
>> > > for any phone fields; in the third pass, for any address fields; and
>> > > so
>
> on.
>>
>> > > This should both add a bit more clarity to the code; and also give us
>> > > a
>
> bit
>>
>> > more
>> > > flexibility for parsing.
>> >
>> > Whoops, didn't mean to send this comment out -- wanted to let David
>> > think
>> about
>> > this some more and then add his thoughts.  So, I guess for now keep this
>> > in
>> mind
>> > as a possibility, but not necessarily the best one.
>
>
>
http://codereview.chromium.org/7014011/diff/8001/chrome/browser/autofill/form...
>>
>> File chrome/browser/autofill/form_field.cc (right):
>
>
>
http://codereview.chromium.org/7014011/diff/8001/chrome/browser/autofill/form...
>>
>> chrome/browser/autofill/form_field.cc:275: field->form_control_type ==
>> ASCIIToUTF16("text"))
>> As I used to did in form_manager, maybe we should also add isTextField()
>> in
>> FormField, down't we?
>
>> And we define
>> MATCH_ANY_TEXT,
>> MATCH_TEXT,
>> MATCH_EMAIL,
>> MATCH_DATE,
>> etc...
>
>> MATCH_ANY_TEXT checks isTextField flag and others check form_controy_type.
>
>> What do you think?
>
>> On 2011/05/14 07:30:05, honten wrote:
>> > Yeah, you are right.
>> >
>> > I completely forgot HTML5 tags;-)
>> >
>> > Let me think about it.
>> >
>> > On 2011/05/14 06:59:12, Ilya Sherman wrote:
>> > > What about 'email', 'tel', and so on?  Are there any clients for which
>> > > MATCH_TEXT is not set?
>> >
>
>
>
> http://codereview.chromium.org/7014011/
>

[dhollowa, 2011-05-16 16:59:43]

On 2011/05/16 16:52:39, takano.naoki_gmail.com wrote:
> David,
> 
> Thank you for your comment.
> 
> On Mon, May 16, 2011 at 7:53 AM,  <mailto:dhollowa@chromium.org> wrote:
> > This patch is getting too broad. &nbsp;Let's split it into separate patches
for
> > the
> > different aspects of the refactorings. &nbsp;I would propose separate
patches
> > for:
> >
> > &nbsp;1. .grd change (easy)
> Ok, I'll do that first.
> 
> > &nbsp;2. the MatchType bit field refactoring
> About this, could you give me your comment?
> 
> The following, is my questions.
> 
> As I used to did in form_manager, maybe we should also add isTextField()
> in FormField, down't we?
> 

Yes, that seems fine.

> And we define
> MATCH_ANY_TEXT,
> MATCH_TEXT,
> MATCH_EMAIL,
> MATCH_DATE,
> etc...
> 
> MATCH_ANY_TEXT checks isTextField flag and others check
> 

We don't need to expose EMAIL, DATE, in the enum/interface.  We can just encode
these in the |isTextField| implementation since no user external to the class
needs it.

> 
> Also Match() is already used broadly in test, so it's hard to find it at once.
> Can I prepare the new function Match2() ?
> 

Yes, but not named |Match2|. Please try to be as descriptive as possible with
the names.  Please refer to
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Overl...
for guidance here.

> > &nbsp;3. two-pass solution for "address" versus "email address" matching
> Ok,
> 
> Ilya already suggested me we might need to change the pass parsing,
> how should I do?
> 

Let me get back to you.  I'm thinking of some sort of two-pass approach where we
modify the scanner to have a "filter" interface so we can filter out previous
items on subsequent scans.


> 
> > As a side note, Ilya and I are participating in a three day summit this
> > week, so
> > may be less responsive until Thursday.
> Ok,
> 
> Thanks,
> 
> >
> >
> > On 2011/05/14 07:41:20, honten wrote:
> >
> >
>
http://codereview.chromium.org/7014011/diff/1/chrome/browser/autofill/address...
> >>
> >> File chrome/browser/autofill/address_field.cc (right):
> >
> >
> >
>
http://codereview.chromium.org/7014011/diff/1/chrome/browser/autofill/address...
> >>
> >> chrome/browser/autofill/address_field.cc:97: break;
> >> I see, it might be better idea.
> >
> >> But, at first, I want to refactor ParseText() with bitmap. I want to
> >> commit
> >> gradually;-)
> >
> >> On 2011/05/14 07:01:33, Ilya Sherman wrote:
> >> > On 2011/05/14 06:59:12, Ilya Sherman wrote:
> >> > > David and I discussed this a bit offline today -- I'll try to
> >> rehash/summarize
> >> > > that here: &nbsp;Rather than changing the parsing for address fields,
let's
> >> > > try
> >> > > making multiple passes over the form in FormField::ParseFormFields().
> >> > > &nbsp;In
> >> the
> >> > > first pass, we can check for any email fields; in the second pass, we
> >> > > can
> >> > check
> >> > > for any phone fields; in the third pass, for any address fields; and
> >> > > so
> >
> > on.
> >>
> >> > > This should both add a bit more clarity to the code; and also give us
> >> > > a
> >
> > bit
> >>
> >> > more
> >> > > flexibility for parsing.
> >> >
> >> > Whoops, didn't mean to send this comment out -- wanted to let David
> >> > think
> >> about
> >> > this some more and then add his thoughts. &nbsp;So, I guess for now keep
this
> >> > in
> >> mind
> >> > as a possibility, but not necessarily the best one.
> >
> >
> >
>
http://codereview.chromium.org/7014011/diff/8001/chrome/browser/autofill/form...
> >>
> >> File chrome/browser/autofill/form_field.cc (right):
> >
> >
> >
>
http://codereview.chromium.org/7014011/diff/8001/chrome/browser/autofill/form...
> >>
> >> chrome/browser/autofill/form_field.cc:275: field->form_control_type ==
> >> ASCIIToUTF16("text"))
> >> As I used to did in form_manager, maybe we should also add isTextField()
> >> in
> >> FormField, down't we?
> >
> >> And we define
> >> MATCH_ANY_TEXT,
> >> MATCH_TEXT,
> >> MATCH_EMAIL,
> >> MATCH_DATE,
> >> etc...
> >
> >> MATCH_ANY_TEXT checks isTextField flag and others check form_controy_type.
> >
> >> What do you think?
> >
> >> On 2011/05/14 07:30:05, honten wrote:
> >> > Yeah, you are right.
> >> >
> >> > I completely forgot HTML5 tags;-)
> >> >
> >> > Let me think about it.
> >> >
> >> > On 2011/05/14 06:59:12, Ilya Sherman wrote:
> >> > > What about 'email', 'tel', and so on? &nbsp;Are there any clients for
which
> >> > > MATCH_TEXT is not set?
> >> >
> >
> >
> >
> > http://codereview.chromium.org/7014011/
> >

[Ilya Sherman, 2011-06-04 01:56:54]

Closing, as I think these changes have all been incorporated into other CLs.