Label scraping for AutoFill....

Label scraping for AutoFill.

Adds label scraping to AutoFill.  Infers labels from surrounding context of
input fields in the WebKit DOM.  Specific cases added are:
  - Text element immediately preceeding INPUT element.
    Eg. First name:<INPUT type="text" id="firstname" value="John"/>
  - Paragraph element containing text immediately preceeding INPUT element.
    Eg. <P>First name:</P><INPUT type="text" id="firstname" value="John"/>

BUG=33031
TEST=FormManagerTest.LabelsFromInferredParagraph,
     FormManagerTest.LabelsFromInferredText,
     FormStructureTest.HeuristicsLabelsOnly


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

[James Hawkins, 2010-03-26 00:29:06]

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

http://codereview.chromium.org/1380002/diff/1/2#newcode391
chrome/browser/autofill/form_structure_unittest.cc:391: values.method =
ASCIIToUTF16("post");
Can you add a comment that says why this test works? e.g. "The name attribute of
each element is not set, so we only have the label value to determine the
heuristics."

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

http://codereview.chromium.org/1380002/diff/1/5#newcode261
chrome/renderer/form_manager.cc:261: TrimWhitespace(inferred_label, TRIM_ALL,
&inferred_label);
Is it possible to move all of these TrimWhitespace calls to the very end?  I'm
assuming you're worried that inferred_label.empty() will be false in the case
that it's just whitespace.  I'm not sure it's valid to assume that a text node
may only have whitespace.  You should investigate that.  If it is, we should
still move the TrimWhitespace call to the bottom and add '&&
!ContainsOnlyWhitespace(inferred_label)) {' to each inferred_label.empty()
check.

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

http://codereview.chromium.org/1380002/diff/1/4#newcode471
chrome/renderer/form_manager_unittest.cc:471: "  <P>First name:</P><INPUT
type=\"text\" id=\"firstname\" value=\"John\"/>"
Line >80 cols.

[dhollowa, 2010-03-26 00:54:22]

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

http://codereview.chromium.org/1380002/diff/1/2#newcode391
chrome/browser/autofill/form_structure_unittest.cc:391: values.method =
ASCIIToUTF16("post");
On 2010/03/26 00:29:07, James Hawkins wrote:
> Can you add a comment that says why this test works? e.g. "The name attribute
of
> each element is not set, so we only have the label value to determine the
> heuristics."

Done.

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

http://codereview.chromium.org/1380002/diff/1/5#newcode261
chrome/renderer/form_manager.cc:261: TrimWhitespace(inferred_label, TRIM_ALL,
&inferred_label);
No, it is a common case that text elements exist but have no content.  The
logic, as is, ensures that we treat empty text elements as a non-match.  Prefer
to leave as is.

On 2010/03/26 00:29:07, James Hawkins wrote:
> Is it possible to move all of these TrimWhitespace calls to the very end?  I'm
> assuming you're worried that inferred_label.empty() will be false in the case
> that it's just whitespace.  I'm not sure it's valid to assume that a text node
> may only have whitespace.  You should investigate that.  If it is, we should
> still move the TrimWhitespace call to the bottom and add '&&
> !ContainsOnlyWhitespace(inferred_label)) {' to each inferred_label.empty()
> check.

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

http://codereview.chromium.org/1380002/diff/1/4#newcode471
chrome/renderer/form_manager_unittest.cc:471: "  <P>First name:</P><INPUT
type=\"text\" id=\"firstname\" value=\"John\"/>"
On 2010/03/26 00:29:07, James Hawkins wrote:
> Line >80 cols.

Done.

[James Hawkins, 2010-03-26 01:00:30]

LGTM after final comments are addressed.

http://codereview.chromium.org/1380002/diff/7002/8001
File chrome/browser/autofill/form_structure_unittest.cc (right):

http://codereview.chromium.org/1380002/diff/7002/8001#newcode387
chrome/browser/autofill/form_structure_unittest.cc:387: // Tests a sequence of
FormField's where only labels are supplied to heuristics
nit: no apostrophe

http://codereview.chromium.org/1380002/diff/7002/8003
File chrome/renderer/form_manager_unittest.cc (right):

http://codereview.chromium.org/1380002/diff/7002/8003#newcode24
chrome/renderer/form_manager_unittest.cc:24: class FormManagerTest : public
RenderViewTest {
Please change this to 'typedef RenderViewTest FormManagerTest;'

[dhollowa, 2010-03-26 01:57:29]

http://codereview.chromium.org/1380002/diff/7002/8001
File chrome/browser/autofill/form_structure_unittest.cc (right):

http://codereview.chromium.org/1380002/diff/7002/8001#newcode387
chrome/browser/autofill/form_structure_unittest.cc:387: // Tests a sequence of
FormField's where only labels are supplied to heuristics
On 2010/03/26 01:00:30, James Hawkins wrote:
> nit: no apostrophe

Done.

http://codereview.chromium.org/1380002/diff/7002/8003
File chrome/renderer/form_manager_unittest.cc (right):

http://codereview.chromium.org/1380002/diff/7002/8003#newcode24
chrome/renderer/form_manager_unittest.cc:24: class FormManagerTest : public
RenderViewTest {
On 2010/03/26 01:00:30, James Hawkins wrote:
> Please change this to 'typedef RenderViewTest FormManagerTest;'

Done.