AutoFill: Notify the renderer when the page has finished translating. Extract

AutoFill: Notify the renderer when the page has finished translating.  Extract
the forms once translation has occurred.  This change also includes another
variaton of the Name field, with tests.  In addition, this change fixes parsing
labels whose text element is not the first child of the label element.

BUG=41694
TEST=FormManagerTest.LabelsWithSpans,NameFieldTest.FirstLast

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

[dhollowa, 2010-04-27 00:02:48]

http://codereview.chromium.org/1801002/diff/1/2
File chrome/browser/autofill/name_field.cc (right):

http://codereview.chromium.org/1801002/diff/1/2#newcode78
chrome/browser/autofill/name_field.cc:78: ASCIIToUTF16("first
name||first_name|firstname|initials|fname|.*first$");
Two bars will match empty string.  I'm guessing you want only one.

http://codereview.chromium.org/1801002/diff/1/3
File chrome/browser/autofill/name_field_unittest.cc (right):

http://codereview.chromium.org/1801002/diff/1/3#newcode149
chrome/browser/autofill/name_field_unittest.cc:149: 
Might be worth adding a test of failure case for empty labels, since I believe
that would have matched.

http://codereview.chromium.org/1801002/diff/1/4
File chrome/renderer/form_manager.cc (right):

http://codereview.chromium.org/1801002/diff/1/4#newcode48
chrome/renderer/form_manager.cc:48: // Returns the node value of the first child
of |element| if the first child
Comment needs updating to match new behavior.

http://codereview.chromium.org/1801002/diff/1/4#newcode55
chrome/renderer/form_manager.cc:55: while (!child.isNull() &&
!child.isTextNode())
I don't think this will work if there is whitespace surrounding the child
elements.  For example, if you modify the unit test to add a space character
just before the<span> element, then this will match the space and not the
intended label string.

http://codereview.chromium.org/1801002/diff/1/5
File chrome/renderer/form_manager_unittest.cc (right):

http://codereview.chromium.org/1801002/diff/1/5#newcode471
chrome/renderer/form_manager_unittest.cc:471: "  <LABEL
for=\"firstname\"><span>First name: </span></LABEL>"
As mentioned, adding whitespace around the <span> elements will cause trouble I
believe.

[James Hawkins, 2010-04-27 00:10:33]

http://codereview.chromium.org/1801002/diff/1/2
File chrome/browser/autofill/name_field.cc (right):

http://codereview.chromium.org/1801002/diff/1/2#newcode78
chrome/browser/autofill/name_field.cc:78: ASCIIToUTF16("first
name||first_name|firstname|initials|fname|.*first$");
On 2010/04/27 00:02:48, dhollowa wrote:
> Two bars will match empty string.  I'm guessing you want only one.

Preemptively fixed in patch set 2.

http://codereview.chromium.org/1801002/diff/1/4
File chrome/renderer/form_manager.cc (right):

http://codereview.chromium.org/1801002/diff/1/4#newcode48
chrome/renderer/form_manager.cc:48: // Returns the node value of the first child
of |element| if the first child
On 2010/04/27 00:02:48, dhollowa wrote:
> Comment needs updating to match new behavior.

Done.

http://codereview.chromium.org/1801002/diff/1/4#newcode55
chrome/renderer/form_manager.cc:55: while (!child.isNull() &&
!child.isTextNode())
On 2010/04/27 00:02:48, dhollowa wrote:
> I don't think this will work if there is whitespace surrounding the child
> elements.  For example, if you modify the unit test to add a space character
> just before the<span> element, then this will match the space and not the
> intended label string.
> 

That would be an unfortunate case, but we can cross that bridge when we come to
it (if we ever see this in the wild.)  No need to over-engineer a corner case
that we're not sure exists.

[dhollowa, 2010-04-27 00:21:00]

Thoughts on the additional NameFieldTest unit test?

http://codereview.chromium.org/1801002/diff/1/4
File chrome/renderer/form_manager.cc (right):

http://codereview.chromium.org/1801002/diff/1/4#newcode55
chrome/renderer/form_manager.cc:55: while (!child.isNull() &&
!child.isTextNode())
On 2010/04/27 00:10:33, James Hawkins wrote:
> On 2010/04/27 00:02:48, dhollowa wrote:
> > I don't think this will work if there is whitespace surrounding the child
> > elements.  For example, if you modify the unit test to add a space character
> > just before the<span> element, then this will match the space and not the
> > intended label string.
> > 
> 
> That would be an unfortunate case, but we can cross that bridge when we come
to
> it (if we ever see this in the wild.)  No need to over-engineer a corner case
> that we're not sure exists.

IMO it is a corner case that element tags would match up with no intervening
whitespace.

[James Hawkins, 2010-04-27 00:25:49]

http://codereview.chromium.org/1801002/diff/1/3
File chrome/browser/autofill/name_field_unittest.cc (right):

http://codereview.chromium.org/1801002/diff/1/3#newcode149
chrome/browser/autofill/name_field_unittest.cc:149: 
On 2010/04/27 00:02:48, dhollowa wrote:
> Might be worth adding a test of failure case for empty labels, since I believe
> that would have matched.

I don't think it's worth it, and that type of test (empty fields) doesn't scale
well.

http://codereview.chromium.org/1801002/diff/1/4
File chrome/renderer/form_manager.cc (right):

http://codereview.chromium.org/1801002/diff/1/4#newcode55
chrome/renderer/form_manager.cc:55: while (!child.isNull() &&
!child.isTextNode())
On 2010/04/27 00:21:00, dhollowa wrote:
> On 2010/04/27 00:10:33, James Hawkins wrote:
> > On 2010/04/27 00:02:48, dhollowa wrote:
> > > I don't think this will work if there is whitespace surrounding the child
> > > elements.  For example, if you modify the unit test to add a space
character
> > > just before the<span> element, then this will match the space and not the
> > > intended label string.
> > > 
> > 
> > That would be an unfortunate case, but we can cross that bridge when we come
> to
> > it (if we ever see this in the wild.)  No need to over-engineer a corner
case
> > that we're not sure exists.
> 
> IMO it is a corner case that element tags would match up with no intervening
> whitespace.
> 

As I said, I'll add that handling when we find a form in the wild that matches
the hypothetical case.

[dhollowa, 2010-04-27 00:33:52]

LGTM.

[dhollowa, 2010-04-27 00:48:50]

On 2010/04/27 00:33:52, dhollowa wrote:
> LGTM.

Note: this change may impact performance.  LGTM pending no perf regression.