Autofill i18n: Develop a system for validating Autofill heuristics from HTML input (issue5280001)

dhol...@chromium.org

unread,

Nov 22, 2010, 10:13:26 PM11/22/10

to viv...@chromium.org, ishe...@chromium.org, chromium...@chromium.org, phajd...@chromium.org, ben...@chromium.org, jhaw...@chromium.org

Reviewers: vivianz, Ilya Sherman,

Description:
Autofill i18n: Develop a system for validating Autofill heuristics from HTML
input

Adds form_structure_browsertest.cc, an InProcessBrowserTest that serializes
the
parsed FormStructure objects and compares the results in string form. This
is
the first step towards a system that reads HTML forms from disk and compares
against known results.

BUG=60022
TEST=FormStructureBrowserTest.BasicFormStructure

Please review this at http://codereview.chromium.org/5280001/

SVN Base: svn://svn.chromium.org/chrome/trunk/src/chrome

Affected files:
M chrome/browser/autofill/autofill_manager.h
A chrome/browser/autofill/form_structure_browsertest.cc
M chrome/chrome_tests.gypi

viv...@chromium.org

unread,

Nov 23, 2010, 12:02:58 AM11/23/10

to dhol...@chromium.org, ishe...@chromium.org, chromium...@chromium.org, phajd...@chromium.org, ben...@chromium.org, jhaw...@chromium.org, dhol...@chromium.org

LGTM

thanks! David

http://codereview.chromium.org/5280001/

ishe...@chromium.org

unread,

Nov 23, 2010, 12:45:24 AM11/23/10

to dhol...@chromium.org, viv...@chromium.org, chromium...@chromium.org, phajd...@chromium.org, ben...@chromium.org, jhaw...@chromium.org, dhol...@chromium.org

LGTM, thanks

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

http://codereview.chromium.org/5280001/diff/1/chrome/browser/autofill/form_structure_browsertest.cc#newcode61
chrome/browser/autofill/form_structure_browsertest.cc:61: if
(!*field_iter)
nit: Would be nice to add a comment explaining why we need this if-stmt
-- it's because the AutoFillField vector is NULL-terminated, right?

http://codereview.chromium.org/5280001/diff/1/chrome/browser/autofill/form_structure_browsertest.cc#newcode195
chrome/browser/autofill/form_structure_browsertest.cc:195: "<form
action=\"http://www.google.com/\" method=\"POST\">"
nit: 80-col

http://codereview.chromium.org/5280001/diff/1/chrome/browser/autofill/form_structure_browsertest.cc#newcode214
chrome/browser/autofill/form_structure_browsertest.cc:214:
EXPECT_EQ(1UL, forms.size());
nit: this seems unnecessary given the string comparison above

http://codereview.chromium.org/5280001/

dhol...@chromium.org

unread,

Nov 23, 2010, 1:10:09 AM11/23/10

to viv...@chromium.org, ishe...@chromium.org, chromium...@chromium.org, phajd...@chromium.org, ben...@chromium.org, jhaw...@chromium.org

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

http://codereview.chromium.org/5280001/diff/1/chrome/browser/autofill/form_structure_browsertest.cc#newcode61
chrome/browser/autofill/form_structure_browsertest.cc:61: if
(!*field_iter)

On 2010/11/23 00:45:24, Ilya Sherman wrote:
> nit: Would be nice to add a comment explaining why we need this
if-stmt -- it's
> because the AutoFillField vector is NULL-terminated, right?

Done.

http://codereview.chromium.org/5280001/diff/1/chrome/browser/autofill/form_structure_browsertest.cc#newcode195
chrome/browser/autofill/form_structure_browsertest.cc:195: "<form
action=\"http://www.google.com/\" method=\"POST\">"

On 2010/11/23 00:45:24, Ilya Sherman wrote:
> nit: 80-col

Done.

http://codereview.chromium.org/5280001/diff/1/chrome/browser/autofill/form_structure_browsertest.cc#newcode214
chrome/browser/autofill/form_structure_browsertest.cc:214:
EXPECT_EQ(1UL, forms.size());

On 2010/11/23 00:45:24, Ilya Sherman wrote:
> nit: this seems unnecessary given the string comparison above