Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
5781005:
Upgrade autofill interactive_ui_test from BasicFormStructure test to I18NForm... (Closed)
|
Created:
10 years ago by vivianz1 Modified:
9 years, 7 months ago CC:
chromium-reviews, ben+cc_chromium.org, dhollowa, James Hawkins, Paweł Hajdan Jr. Base URL:
http://src.chromium.org/svn/trunk/src/ Visibility:
Public. |
DescriptionUpgrade autofill interactive_ui_test from BasicFormStructure test to I18NFormStructure test.
Added inputting i18n form file and write out and/or compare test result against expected test
result in output directory. Also add the initial English version of test form html and expected
test result file.
Contributed by vivianz@chromium.org
BUG=60022
TEST=none
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=69503
Patch Set 1 #Patch Set 2 : '' #
Total comments: 11
Patch Set 3 : '' #Patch Set 4 : '' #Patch Set 5 : '' #Patch Set 6 : '' #
Total comments: 26
Patch Set 7 : '' #Patch Set 8 : '' #
Total comments: 13
Patch Set 9 : '' #
Total comments: 6
Patch Set 10 : '' #Patch Set 11 : '' #
Messages
Total messages: 16 (0 generated)
here is the autofill i18n form structure test. the rest of the i18n form input test files are temparaily located at : http://www.corp.google.com/~vivianz/chrome/test_data/autofill_heuristics/input since currently autofill manager does not yet render these i18n version of labels/attributes correctly.
The CL description should include lines: BUG=60022 TEST=FormStructureBrowserTest.HTMLFiles http://codereview.chromium.org/5781005/diff/3001/chrome/browser/autofill/form... File chrome/browser/autofill/form_structure_browsertest.cc (right): http://codereview.chromium.org/5781005/diff/3001/chrome/browser/autofill/form... chrome/browser/autofill/form_structure_browsertest.cc:61: forms_string += (*iter)->source_url().spec(); Let's remove these two lines. We don't need to copy the html through to output. http://codereview.chromium.org/5781005/diff/3001/chrome/browser/autofill/form... chrome/browser/autofill/form_structure_browsertest.cc:175: FilePath GetInputFileDirectory() { Please move this to top of the file and wrap it in an unnamed namespace. Same with |GetOutputFileDirectory()| and |WriteFile()|. Also, there should be no indentation of the function definitions. I.e.: namespace { FilePath GetInputFileDirectory() { ... } FilePath GetOutputFileDirectory() { ... } bool WriteFile() { ... } } // namespace http://codereview.chromium.org/5781005/diff/3001/chrome/browser/autofill/form... chrome/browser/autofill/form_structure_browsertest.cc:199: IN_PROC_BROWSER_TEST_F(FormStructureBrowserTest, I18NFormStructure) { Let's rename from I18NFormStructure to HTMLFiles. http://codereview.chromium.org/5781005/diff/3001/chrome/browser/autofill/form... chrome/browser/autofill/form_structure_browsertest.cc:227: .BaseName().StripTrailingSeparators().ReplaceExtension(L".out")); The "L" prefix is not cross-platform. You can use the FILE_PATH_LITERAL macro here. http://codereview.chromium.org/5781005/diff/3001/chrome/test/data/autofill_he... File chrome/test/data/autofill_heuristics/input/form_en.html (right): http://codereview.chromium.org/5781005/diff/3001/chrome/test/data/autofill_he... chrome/test/data/autofill_heuristics/input/form_en.html:1: data:text/html;charset=utf-8,<form action="http://www.google.com/" method="POST"><label for="firstname">First name:</label> <input type="text" id="firstname"/><br /><label for="lastname">Last name:</label> <input type="text" id="lastname" /><br /><label for="company">Company Name:</label> <input type="text" id="company" /><br /><label for="address1">Address line 1:</label> <input type="text" id="address1" /><br /><label for="address2">Address line 2:</label> <input type="text" id="address2" /><br /><label for="city">City:</label> <input type="text" id="city" /><br /><label for="zip">Zip Code:</label> <input type="text" id="zip" /><br /><label for="state">State:</label> <input type="text" id="state" /><br /><label for="email">Email:</label> <input type="text" id="email" /><br /><label for="phone">Phone Number:</label> <input type="text" id="phone" /><br /><label for="fax">Fax Number:</label> <input type="text" id="fax" /><br /></form> Let's tidy this up to be readable HTML. I'll email you a tidied version. This may require that we load the HTML in a different way however, possibly using a file:// protocol. http://codereview.chromium.org/5781005/diff/3001/chrome/test/data/autofill_he... File chrome/test/data/autofill_heuristics/output/form_en.out (right): http://codereview.chromium.org/5781005/diff/3001/chrome/test/data/autofill_he... chrome/test/data/autofill_heuristics/output/form_en.out:1: data:text/html;charset=utf-8,<form action="http://www.google.com/" method="POST"><label for="firstname">First name:</label> <input type="text" id="firstname"/><br /><label for="lastname">Last name:</label> <input type="text" id="lastname" /><br /><label for="company">Company Name:</label> <input type="text" id="company" /><br /><label for="address1">Address line 1:</label> <input type="text" id="address1" /><br /><label for="address2">Address line 2:</label> <input type="text" id="address2" /><br /><label for="city">City:</label> <input type="text" id="city" /><br /><label for="zip">Zip Code:</label> <input type="text" id="zip" /><br /><label for="state">State:</label> <input type="text" id="state" /><br /><label for="email">Email:</label> <input type="text" id="email" /><br /><label for="phone">Phone Number:</label> <input type="text" id="phone" /><br /><label for="fax">Fax Number:</label> <input type="text" id="fax" /><br /></form> This first line should be removed. (Along with the code that generates it).
fixed code according to comments, in addtional, added the ConvertToURLFormat(input_str), to convert the html header to URL format. I will change all the international version input files to used the new html format. http://codereview.chromium.org/5781005/diff/3001/chrome/browser/autofill/form... File chrome/browser/autofill/form_structure_browsertest.cc (right): http://codereview.chromium.org/5781005/diff/3001/chrome/browser/autofill/form... chrome/browser/autofill/form_structure_browsertest.cc:61: forms_string += (*iter)->source_url().spec(); On 2010/12/13 22:11:30, dhollowa wrote: make sense, removing it. http://codereview.chromium.org/5781005/diff/3001/chrome/browser/autofill/form... chrome/browser/autofill/form_structure_browsertest.cc:175: FilePath GetInputFileDirectory() { On 2010/12/13 22:11:30, dhollowa wrote: Fixed http://codereview.chromium.org/5781005/diff/3001/chrome/browser/autofill/form... chrome/browser/autofill/form_structure_browsertest.cc:199: IN_PROC_BROWSER_TEST_F(FormStructureBrowserTest, I18NFormStructure) { On 2010/12/13 22:11:30, dhollowa wrote: Done. http://codereview.chromium.org/5781005/diff/3001/chrome/browser/autofill/form... chrome/browser/autofill/form_structure_browsertest.cc:227: .BaseName().StripTrailingSeparators().ReplaceExtension(L".out")); On 2010/12/13 22:11:30, dhollowa wrote: Done. http://codereview.chromium.org/5781005/diff/3001/chrome/test/data/autofill_he... File chrome/test/data/autofill_heuristics/output/form_en.out (right): http://codereview.chromium.org/5781005/diff/3001/chrome/test/data/autofill_he... chrome/test/data/autofill_heuristics/output/form_en.out:1: data:text/html;charset=utf-8,<form action="http://www.google.com/" method="POST"><label for="firstname">First name:</label> <input type="text" id="firstname"/><br /><label for="lastname">Last name:</label> <input type="text" id="lastname" /><br /><label for="company">Company Name:</label> <input type="text" id="company" /><br /><label for="address1">Address line 1:</label> <input type="text" id="address1" /><br /><label for="address2">Address line 2:</label> <input type="text" id="address2" /><br /><label for="city">City:</label> <input type="text" id="city" /><br /><label for="zip">Zip Code:</label> <input type="text" id="zip" /><br /><label for="state">State:</label> <input type="text" id="state" /><br /><label for="email">Email:</label> <input type="text" id="email" /><br /><label for="phone">Phone Number:</label> <input type="text" id="phone" /><br /><label for="fax">Fax Number:</label> <input type="text" id="fax" /><br /></form> On 2010/12/13 22:11:30, dhollowa wrote: Done.
http://codereview.chromium.org/5781005/diff/19001/chrome/browser/autofill/for... File chrome/browser/autofill/form_structure_browsertest.cc (right): http://codereview.chromium.org/5781005/diff/19001/chrome/browser/autofill/for... chrome/browser/autofill/form_structure_browsertest.cc:22: FilePath GetInputFileDirectory() { These functions should not be indented. See http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Namespaces http://codereview.chromium.org/5781005/diff/19001/chrome/browser/autofill/for... chrome/browser/autofill/form_structure_browsertest.cc:39: bool WriteFile(const FilePath& outputfile, Let's rename |outputfile| to |file|, and |outputcontent| to |content|. http://codereview.chromium.org/5781005/diff/19001/chrome/browser/autofill/for... chrome/browser/autofill/form_structure_browsertest.cc:47: const std::string ConvertToURLFormat(std::string& input_str) { Let's change the input parameter to: const std::string& html We can simply return the concatenation of the "data:..." string with the html string. I.e. return std::string("data:text/html;charset=utf-8,") + html; http://codereview.chromium.org/5781005/diff/19001/chrome/browser/autofill/for... chrome/browser/autofill/form_structure_browsertest.cc:60: // Test class for verifying proper international form structure as determined This code is not i18n-specific. Let's change this back to the original comment. http://codereview.chromium.org/5781005/diff/19001/chrome/browser/autofill/for... chrome/browser/autofill/form_structure_browsertest.cc:217: false, file_util::FileEnumerator::FILES); Let's add the filter "*.html" to limit processing to html files only.
Just have a few nits. Nearly there =) http://codereview.chromium.org/5781005/diff/19001/chrome/browser/autofill/for... File chrome/browser/autofill/form_structure_browsertest.cc (right): http://codereview.chromium.org/5781005/diff/19001/chrome/browser/autofill/for... chrome/browser/autofill/form_structure_browsertest.cc:230: browser(), GURL(UTF8ToUTF16(input_file_source)))); nit: I don't believe we need UTF8ToUTF16 here. http://codereview.chromium.org/5781005/diff/19001/chrome/browser/autofill/for... chrome/browser/autofill/form_structure_browsertest.cc:233: VIEW_ID_TAB_CONTAINER)); nit: Please preserve the indentation here. The second line should line up with the "b" from "browser()". Same applies below. http://codereview.chromium.org/5781005/diff/19001/chrome/browser/autofill/for... chrome/browser/autofill/form_structure_browsertest.cc:245: FILE_PATH_LITERAL(".out"))); nit: I would prefer the following formatting (each indent is four spaces -- even if it doesn't look like it with this non-monospaced font :) FilePath output_file_path = output_file_directory.Append( input_file_path.BaseName().StripTrailingSeparators().ReplaceExtension( FILE_PATH_LITERAL(".out"))); http://codereview.chromium.org/5781005/diff/19001/chrome/browser/autofill/for... chrome/browser/autofill/form_structure_browsertest.cc:254: FormStructureBrowserTest::FormStructuresToString(forms))); nit: Arguments to a function should be aligned. In this case, since the second argument cannot be indented to line up with the first, the first should be moved down onto a separate line so that it can align with the second. http://codereview.chromium.org/5781005/diff/19001/chrome/test/data/autofill_h... File chrome/test/data/autofill_heuristics/input/form_en.html (right): http://codereview.chromium.org/5781005/diff/19001/chrome/test/data/autofill_h... chrome/test/data/autofill_heuristics/input/form_en.html:8: <label for="firstname">First name:</label> <input type="text" id="firstname"><br> nit: It would be great to keep these files to the 80-col width as well. One way would be to have each <input> appear on the line below the corresponding <label> (perhaps indented by a couple of spaces).
http://codereview.chromium.org/5781005/diff/19001/chrome/browser/autofill/for... File chrome/browser/autofill/form_structure_browsertest.cc (right): http://codereview.chromium.org/5781005/diff/19001/chrome/browser/autofill/for... chrome/browser/autofill/form_structure_browsertest.cc:249: EXPECT_EQ(output_file_source, I ran this through the trybots and we're getting failures on Windows. Windows is reading newlines as "\r\n", whereas Mac and Linux are reading them as "\n". To fix this we should change line 111 to use platform specific line breaks when generating the expected string. [ RUN ] FormStructureBrowserTest.HTMLFiles e:\b\build\slave\win\build\src\chrome\browser\autofill\form_structure_browsertest.cc(242): error: Value of: FormStructureBrowserTest::FormStructuresToString(forms) Actual: "NAME_FIRST\nNAME_LAST\nCOMPANY_NAME\nADDRESS_HOME_LINE1\nADDRESS_HOME_LINE2\nADDRESS_HOME_CITY\nADDRESS_HOME_ZIP\nADDRESS_HOME_STATE\nEMAIL_ADDRESS\nPHONE_HOME_WHOLE_NUMBER\nPHONE_FAX_WHOLE_NUMBER\n" Expected: output_file_source Which is: "NAME_FIRST\r\nNAME_LAST\r\nCOMPANY_NAME\r\nADDRESS_HOME_LINE1\r\nADDRESS_HOME_LINE2\r\nADDRESS_HOME_CITY\r\nADDRESS_HOME_ZIP\r\nADDRESS_HOME_STATE\r\nEMAIL_ADDRESS\r\nPHONE_HOME_WHOLE_NUMBER\r\nPHONE_FAX_WHOLE_NUMBER\r\n" http://codereview.chromium.org/5781005/diff/19001/chrome/test/data/autofill_h... File chrome/test/data/autofill_heuristics/input/form_en.html (right): http://codereview.chromium.org/5781005/diff/19001/chrome/test/data/autofill_h... chrome/test/data/autofill_heuristics/input/form_en.html:8: <label for="firstname">First name:</label> <input type="text" id="firstname"><br> No, I disagree. This is data. We want to be able to simply drop new HTML files in. Formatting be damned. On 2010/12/15 10:58:32, Ilya Sherman wrote: > nit: It would be great to keep these files to the 80-col width as well. One way > would be to have each <input> appear on the line below the corresponding <label> > (perhaps indented by a couple of spaces).
http://codereview.chromium.org/5781005/diff/19001/chrome/browser/autofill/for... File chrome/browser/autofill/form_structure_browsertest.cc (right): http://codereview.chromium.org/5781005/diff/19001/chrome/browser/autofill/for... chrome/browser/autofill/form_structure_browsertest.cc:22: FilePath GetInputFileDirectory() { On 2010/12/15 03:20:45, dhollowa wrote: Done. http://codereview.chromium.org/5781005/diff/19001/chrome/browser/autofill/for... chrome/browser/autofill/form_structure_browsertest.cc:39: bool WriteFile(const FilePath& outputfile, On 2010/12/15 03:20:45, dhollowa wrote: Done. http://codereview.chromium.org/5781005/diff/19001/chrome/browser/autofill/for... chrome/browser/autofill/form_structure_browsertest.cc:47: const std::string ConvertToURLFormat(std::string& input_str) { On 2010/12/15 03:20:45, dhollowa wrote: Done. http://codereview.chromium.org/5781005/diff/19001/chrome/browser/autofill/for... chrome/browser/autofill/form_structure_browsertest.cc:60: // Test class for verifying proper international form structure as determined On 2010/12/15 03:20:45, dhollowa wrote: remove all the i18n related comment, let me know if this looks ok now? http://codereview.chromium.org/5781005/diff/19001/chrome/browser/autofill/for... chrome/browser/autofill/form_structure_browsertest.cc:217: false, file_util::FileEnumerator::FILES); On 2010/12/15 03:20:45, dhollowa wrote: Done. http://codereview.chromium.org/5781005/diff/19001/chrome/browser/autofill/for... chrome/browser/autofill/form_structure_browsertest.cc:230: browser(), GURL(UTF8ToUTF16(input_file_source)))); On 2010/12/15 10:58:32, Ilya Sherman wrote: I think we do need this here, otherwise, the first non-ASCII character appear in label, GURL will fail to recognize it. http://codereview.chromium.org/5781005/diff/19001/chrome/browser/autofill/for... chrome/browser/autofill/form_structure_browsertest.cc:233: VIEW_ID_TAB_CONTAINER)); On 2010/12/15 10:58:32, Ilya Sherman wrote: Done. http://codereview.chromium.org/5781005/diff/19001/chrome/browser/autofill/for... chrome/browser/autofill/form_structure_browsertest.cc:245: FILE_PATH_LITERAL(".out"))); On 2010/12/15 10:58:32, Ilya Sherman wrote: Done. http://codereview.chromium.org/5781005/diff/19001/chrome/browser/autofill/for... chrome/browser/autofill/form_structure_browsertest.cc:249: EXPECT_EQ(output_file_source, On 2010/12/15 15:24:01, dhollowa wrote: my bad, I over-complicated the situation here. the only reason this happen is because we use "\n" as sepearator for the autofill labels in output file, it is nothing to do with the input html file, so I believe just replace the seperator with a space should fix the problem, am I wrong? http://codereview.chromium.org/5781005/diff/19001/chrome/browser/autofill/for... chrome/browser/autofill/form_structure_browsertest.cc:249: EXPECT_EQ(output_file_source, On 2010/12/15 15:24:01, dhollowa wrote: my bad, I over-complicated the situation here. the reason this test failed is cause we use "\n" as line sepearator in our test result file. it has nothing to do with the input html file. so as long as we use platform unanimous character " ", this error should be fixed. am I wrong? so I replaced line 111 "\n" with " ". let me know if this is ok? http://codereview.chromium.org/5781005/diff/19001/chrome/browser/autofill/for... chrome/browser/autofill/form_structure_browsertest.cc:254: FormStructureBrowserTest::FormStructuresToString(forms))); On 2010/12/15 10:58:32, Ilya Sherman wrote: Done. http://codereview.chromium.org/5781005/diff/19001/chrome/test/data/autofill_h... File chrome/test/data/autofill_heuristics/input/form_en.html (right): http://codereview.chromium.org/5781005/diff/19001/chrome/test/data/autofill_h... chrome/test/data/autofill_heuristics/input/form_en.html:8: <label for="firstname">First name:</label> <input type="text" id="firstname"><br> On 2010/12/15 10:58:32, Ilya Sherman wrote: i will leave this as-is for now.
http://codereview.chromium.org/5781005/diff/19001/chrome/browser/autofill/for... File chrome/browser/autofill/form_structure_browsertest.cc (right): http://codereview.chromium.org/5781005/diff/19001/chrome/browser/autofill/for... chrome/browser/autofill/form_structure_browsertest.cc:230: browser(), GURL(UTF8ToUTF16(input_file_source)))); On 2010/12/15 23:31:37, vivianz wrote: > On 2010/12/15 10:58:32, Ilya Sherman wrote: > > I think we do need this here, otherwise, the first non-ASCII character appear in > label, GURL will fail to recognize it. The GURL documentation [1] indicates std::string input to the constructor will be parsed as UTF8 input. Are you finding that this is not the case? If so, we should let brettw know. [1] http://codesearch.google.com/codesearch/p?vert=chromium#OAMlx_jo-ck/src/googl... http://codereview.chromium.org/5781005/diff/30001/chrome/browser/autofill/for... chrome/browser/autofill/form_structure_browsertest.cc:34: .AppendASCII("output"); nit: this line is still indented two extra spaces http://codereview.chromium.org/5781005/diff/30001/chrome/browser/autofill/for... chrome/browser/autofill/form_structure_browsertest.cc:41: int write_size = file_util::WriteFile(outputfile, outputcontent.c_str(), nit: this line is still indented two extra spaces http://codereview.chromium.org/5781005/diff/30001/chrome/browser/autofill/for... chrome/browser/autofill/form_structure_browsertest.cc:227: nit: please either indent VIEW_ID_TAB... to align with the "b" from "browser()" (if it fits), or else move "browser()" down onto a separate line.
http://codereview.chromium.org/5781005/diff/30001/chrome/browser/autofill/for... File chrome/browser/autofill/form_structure_browsertest.cc (right): http://codereview.chromium.org/5781005/diff/30001/chrome/browser/autofill/for... chrome/browser/autofill/form_structure_browsertest.cc:102: forms_string += " "; Isn't it easier to do svn pset svn:eol-style LF chrome/test/data/autofill_heuristics/output/form_en.out Then leave the code here as it is and use the new line separation in form_en.out (this will be more readable for sure :))?
http://codereview.chromium.org/5781005/diff/30001/chrome/browser/autofill/for... File chrome/browser/autofill/form_structure_browsertest.cc (right): http://codereview.chromium.org/5781005/diff/30001/chrome/browser/autofill/for... chrome/browser/autofill/form_structure_browsertest.cc:102: forms_string += " "; Yes, this should return to "\n". And we should add a |CompareText(const std::string& a, const::string& b)| helper function that replaces "\r\n" with " " (space), and then replaces any remaining "\n" with " ". This |CompareText| helper should then be called at line 241. On 2010/12/16 00:46:42, GeorgeY wrote: > Isn't it easier to do > svn pset svn:eol-style LF > chrome/test/data/autofill_heuristics/output/form_en.out > Then leave the code here as it is and use the new line separation in form_en.out > (this will be more readable for sure :))? http://codereview.chromium.org/5781005/diff/30001/chrome/browser/autofill/for... chrome/browser/autofill/form_structure_browsertest.cc:241: EXPECT_EQ(output_file_source, This will change to: EXPECT_TRUE(CompareText(...)); http://codereview.chromium.org/5781005/diff/30001/chrome/test/data/autofill_h... File chrome/test/data/autofill_heuristics/output/form_en.out (right): http://codereview.chromium.org/5781005/diff/30001/chrome/test/data/autofill_h... chrome/test/data/autofill_heuristics/output/form_en.out:1: NAME_FIRST NAME_LAST COMPANY_NAME ADDRESS_HOME_LINE1 ADDRESS_HOME_LINE2 ADDRESS_HOME_CITY ADDRESS_HOME_ZIP ADDRESS_HOME_STATE EMAIL_ADDRESS PHONE_HOME_WHOLE_NUMBER PHONE_FAX_WHOLE_NUMBER No, this should go back to how it was.
http://codereview.chromium.org/5781005/diff/19001/chrome/browser/autofill/for... File chrome/browser/autofill/form_structure_browsertest.cc (right): http://codereview.chromium.org/5781005/diff/19001/chrome/browser/autofill/for... chrome/browser/autofill/form_structure_browsertest.cc:230: browser(), GURL(UTF8ToUTF16(input_file_source)))); On 2010/12/16 00:24:03, Ilya Sherman wrote: actaully you are right, i jump to conclusion with my previous testing, after remove the unicode convertion, the previous failure of GURL is no longer appear now with non-ASCII label name in html. the failure we got before is likely still cause by the BOM character in html not utf8 GURL. fixed http://codereview.chromium.org/5781005/diff/30001/chrome/browser/autofill/for... chrome/browser/autofill/form_structure_browsertest.cc:34: .AppendASCII("output"); On 2010/12/16 00:24:03, Ilya Sherman wrote: Done. http://codereview.chromium.org/5781005/diff/30001/chrome/browser/autofill/for... chrome/browser/autofill/form_structure_browsertest.cc:41: int write_size = file_util::WriteFile(outputfile, outputcontent.c_str(), On 2010/12/16 00:24:03, Ilya Sherman wrote: Done. http://codereview.chromium.org/5781005/diff/30001/chrome/browser/autofill/for... chrome/browser/autofill/form_structure_browsertest.cc:102: On 2010/12/16 00:46:42, GeorgeY wrote: ok, sounds good:), let's try the svn way first. http://codereview.chromium.org/5781005/diff/30001/chrome/browser/autofill/for... chrome/browser/autofill/form_structure_browsertest.cc:102: On 2010/12/16 01:10:22, dhollowa wrote: let's try the svn first on trybot first, will add the CompareText helper later. http://codereview.chromium.org/5781005/diff/30001/chrome/browser/autofill/for... chrome/browser/autofill/form_structure_browsertest.cc:227: On 2010/12/16 00:24:03, Ilya Sherman wrote: Done.
LGTM. Running past the trybots to see if the svn property change does the trick. http://codereview.chromium.org/5781005/diff/39001/chrome/browser/autofill/for... File chrome/browser/autofill/form_structure_browsertest.cc (right): http://codereview.chromium.org/5781005/diff/39001/chrome/browser/autofill/for... chrome/browser/autofill/form_structure_browsertest.cc:58: // in output directory, else the form structures are compared again the nit: again => against
This still failed on the Windows trybot due to the "\n" vs "\r\n" issue :(
http://codereview.chromium.org/5781005/diff/30001/chrome/browser/autofill/for... File chrome/browser/autofill/form_structure_browsertest.cc (right): http://codereview.chromium.org/5781005/diff/30001/chrome/browser/autofill/for... chrome/browser/autofill/form_structure_browsertest.cc:102: forms_string += " "; On 2010/12/16 02:00:14, vivianz wrote: > On 2010/12/16 01:10:22, dhollowa wrote: > > let's try the svn first on trybot first, will add the CompareText helper later. Looks like the svn props didn't work. I'm still seeing the same failure on the Windows try bot: FormStructureBrowserTest.HTMLFiles: e:\b\build\slave\win\build\src\chrome\browser\autofill\form_structure_browsertest.cc(243): error: Value of: FormStructureBrowserTest::FormStructuresToString(forms) Actual: "NAME_FIRST\nNAME_LAST\nCOMPANY_NAME\nADDRESS_HOME_LINE1\nADDRESS_HOME_LINE2\nADDRESS_HOME_CITY\nADDRESS_HOME_ZIP\nADDRESS_HOME_STATE\nEMAIL_ADDRESS\nPHONE_HOME_WHOLE_NUMBER\nPHONE_FAX_WHOLE_NUMBER\n" Expected: output_file_source Which is: "NAME_FIRST\r\nNAME_LAST\r\nCOMPANY_NAME\r\nADDRESS_HOME_LINE1\r\nADDRESS_HOME_LINE2\r\nADDRESS_HOME_CITY\r\nADDRESS_HOME_ZIP\r\nADDRESS_HOME_STATE\r\nEMAIL_ADDRESS\r\nPHONE_HOME_WHOLE_NUMBER\r\nPHONE_FAX_WHOLE_NUMBER\r\n" Let's proceed with the |CompareText| function. http://codereview.chromium.org/5781005/diff/39001/chrome/browser/autofill/for... chrome/browser/autofill/form_structure_browsertest.cc:54: // loads its HTMLcontent with a call to |NavigateToURL|, the |AutoFillManager| nit: s/HTMLcontent/HTML content/ http://codereview.chromium.org/5781005/diff/39001/chrome/browser/autofill/for... chrome/browser/autofill/form_structure_browsertest.cc:58: // in output directory, else the form structures are compared again the nit: s/again/against/
Added the CompareText() function to trim off all EOL char in both form string and output file source, then verify if the two strings are identical. it should works for all os platform now, let me know if this is ok? thanks! http://codereview.chromium.org/5781005/diff/39001/chrome/browser/autofill/for... File chrome/browser/autofill/form_structure_browsertest.cc (right): http://codereview.chromium.org/5781005/diff/39001/chrome/browser/autofill/for... chrome/browser/autofill/form_structure_browsertest.cc:54: // loads its HTMLcontent with a call to |NavigateToURL|, the |AutoFillManager| On 2010/12/16 16:35:53, dhollowa wrote: Done. http://codereview.chromium.org/5781005/diff/39001/chrome/browser/autofill/for... chrome/browser/autofill/form_structure_browsertest.cc:58: // in output directory, else the form structures are compared again the On 2010/12/16 05:07:14, Ilya Sherman wrote: Done. http://codereview.chromium.org/5781005/diff/39001/chrome/browser/autofill/for... chrome/browser/autofill/form_structure_browsertest.cc:58: // in output directory, else the form structures are compared again the On 2010/12/16 16:35:53, dhollowa wrote: Done.
LGTM! I'll run it through the trybots and then land this sucker. Thanks Vivian! |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
