Add a UI test for "Encoding" menu

Bug = 5515

[xlyuan, 2009-02-23 04:54:09]

Hi, Johnny and Jungshik:

    I fixed several style issues, can you please take a look?

Regards,
Xiaolu

[Johnny(Jianning) Ding, 2009-02-24 13:01:58]

I have a few comments for the first two .cc files. Will check the rest files
after you finish my comments.

Thanks!

http://codereview.chromium.org/18417/diff/1482/1557
File chrome/browser/browser_encoding_uitest.cc (right):

http://codereview.chromium.org/18417/diff/1482/1557#newcode22
Line 22: protected:
only one space needed for public:, protected:, and private: keywords.

http://codereview.chromium.org/18417/diff/1482/1557#newcode40
Line 40: int func(T(&p)[n]) {
why not directly use arraysize defined in basictypes.h. Also  the name "func"
makes me confused.

http://codereview.chromium.org/18417/diff/1482/1557#newcode51
Line 51: std::wstring encoding;
data member should be always ended with a trailing underscore

http://codereview.chromium.org/18417/diff/1482/1557#newcode55
Line 55: // Below encodings are ignored because I don't have en editor to
"en" -> "an"?

http://codereview.chromium.org/18417/diff/1482/1557#newcode56
Line 56: // create corresponding encoded test file for now :
I think we can directly use notepad++ to create test file for UTF-16LE

http://codereview.chromium.org/18417/diff/1482/1557#newcode68
Line 68: std::wstring file_name[] = {L"encoding-UTF-8.htm",
please use const wchar_t* instead of using std::wstring to avoid constructing
lots of std::wstring instances.
The array looks like
const wchar_t* file_name[] = {
  L"encoding-UTF-8.htm",
  L"encoding-iso-8859-1.htm",
...
};

http://codereview.chromium.org/18417/diff/1482/1557#newcode97
Line 97: std::wstring encoding_name[] = {L"UTF-8",
same problem with file_name[]
Actually you could bind the encoding test file and corresponding encoding
together. Just like
struct {
  const wchar_t* encoding_testfile_name;
  const wchar_t* encoding_name;
} EncodingTestData encoding_test_data[] = {
  { L"encoding-UTF-8.htm", L"UTF-8" },
  { L"encoding-iso-8859-1.htm", L"ISO-8859-1" },
  ...
}

http://codereview.chromium.org/18417/diff/1482/1557#newcode126
Line 126: for(int i = 0;i < func (file_name);i++) {
need a space between "for" and "(".
need a space between ";" and "i++".

http://codereview.chromium.org/18417/diff/1482/1557#newcode128
Line 128: L"/encoding_alias_mapping/" + file_name[i]);
please take a look
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Calls
for the format of function call.
Same problems happened on line 149, 326

http://codereview.chromium.org/18417/diff/1482/1557#newcode142
Line 142:
L"encoding_user_override\\expect_gb18030_content_save_from_encoding_iso88591.htm";
exceeds 80 characters limitation,
same problems happened on line 190

http://codereview.chromium.org/18417/diff/1482/1557#newcode171
Line 171: EXPECT_TRUE(tab->SavePage(full_file_name, dir,
move the line 144-145 to the position before this line.

http://codereview.chromium.org/18417/diff/1482/1557#newcode175
Line 175: }
For test of encoding oevrride, testing overriding from "8859-1" to "gb18030" is
good enough?

http://codereview.chromium.org/18417/diff/1482/1557#newcode178
Line 178: // Below encodings are ignored because I don't have en editor to
create
please this redundant comments

http://codereview.chromium.org/18417/diff/1482/1557#newcode209
Line 209: std::wstring file_name[] = {
same problem with line 68

http://codereview.chromium.org/18417/diff/1482/1557#newcode220
Line 220: // L"windows-874_content_without_encoding_declaration.htm",
please remove useless code instead of commenting them

http://codereview.chromium.org/18417/diff/1482/1557#newcode233
Line 233: std::wstring expect_folder_name =
L"encoding_auto_detect\\expect_results\\";
same problem with line 68

http://codereview.chromium.org/18417/diff/1482/1557#newcode234
Line 234: std::wstring expect_file_name[] = {
same problem with line 68

http://codereview.chromium.org/18417/diff/1482/1557#newcode245
Line 245: //
L"expect_windows-874_content_save_from_without_encoding_declaration.htm",
same problem with line 220

http://codereview.chromium.org/18417/diff/1482/1557#newcode261
Line 261: std::wstring expect_encoding[] = {L"Big5",
same problem with line 68

http://codereview.chromium.org/18417/diff/1482/1557#newcode271
Line 271: // L"windows-874",
same problem with line 220

http://codereview.chromium.org/18417/diff/1482/1557#newcode284
Line 284: std::wstring save_dir_sub[] = {
same problem with line 68

http://codereview.chromium.org/18417/diff/1482/1557#newcode295
Line 295: // L"windows-874_content_without_encoding_declaration_files",
same problem with line 220

http://codereview.chromium.org/18417/diff/1482/1557#newcode349
Line 349: // Re-get the encoding of page. It should be gb18030.
should be "gb18030"? it looks the comments need to be changed.

http://codereview.chromium.org/18417/diff/1482/1558
File chrome/browser/download/save_page_uitest.cc (right):

http://codereview.chromium.org/18417/diff/1482/1558#newcode26
Line 26: std::wstring expect_result_filepath =
You might want to move this function to UITest with adding one additional
parameter "test_dir_path" to remove the duplicate code both encoding_uitest and
save_page_uitest.
The new function looks like
void CheckFile(const std::wstring& generated_file,
               const std::wstring& expect_result_file,
               const std::wstring& test_dir_path,
               bool check_equal)

[xlyuan, 2009-03-05 03:22:13]

Hi, Johnny:

    For add more encoding for user override, I thought one encoding is enough
because we just want to make sure override works, how do you think? If you think
we need more, I will create more test files and add them; for the rest, all
done, can you please take another look?

Regards,
Xiaolu

On 2009/02/24 13:01:58, Johnny(Jianning) Ding wrote:
> I have a few comments for the first two .cc files. Will check the rest files
> after you finish my comments.
> 
> Thanks!
> 
> http://codereview.chromium.org/18417/diff/1482/1557
> File chrome/browser/browser_encoding_uitest.cc (right):
> 
> http://codereview.chromium.org/18417/diff/1482/1557#newcode22
> Line 22: protected:
> only one space needed for public:, protected:, and private: keywords.
> Done.

> http://codereview.chromium.org/18417/diff/1482/1557#newcode40
> Line 40: int func(T(&p)[n]) {
> why not directly use arraysize defined in basictypes.h. Also  the name "func"
> makes me confused.
> Done.

> http://codereview.chromium.org/18417/diff/1482/1557#newcode51
> Line 51: std::wstring encoding;
> data member should be always ended with a trailing underscore
> Done.

> http://codereview.chromium.org/18417/diff/1482/1557#newcode55
> Line 55: // Below encodings are ignored because I don't have en editor to
> "en" -> "an"?
> Done.

> http://codereview.chromium.org/18417/diff/1482/1557#newcode56
> Line 56: // create corresponding encoded test file for now :
> I think we can directly use notepad++ to create test file for UTF-16LE
> Done.

> http://codereview.chromium.org/18417/diff/1482/1557#newcode68
> Line 68: std::wstring file_name[] = {L"encoding-UTF-8.htm",
> please use const wchar_t* instead of using std::wstring to avoid constructing
> lots of std::wstring instances.
> The array looks like
> const wchar_t* file_name[] = {
>   L"encoding-UTF-8.htm",
>   L"encoding-iso-8859-1.htm",
> ...
> };
> Done.

> http://codereview.chromium.org/18417/diff/1482/1557#newcode97
> Line 97: std::wstring encoding_name[] = {L"UTF-8",
> same problem with file_name[]
> Actually you could bind the encoding test file and corresponding encoding
> together. Just like
> struct {
>   const wchar_t* encoding_testfile_name;
>   const wchar_t* encoding_name;
> } EncodingTestData encoding_test_data[] = {
>   { L"encoding-UTF-8.htm", L"UTF-8" },
>   { L"encoding-iso-8859-1.htm", L"ISO-8859-1" },
>   ...
> }
> Done.

> http://codereview.chromium.org/18417/diff/1482/1557#newcode126
> Line 126: for(int i = 0;i < func (file_name);i++) {
> need a space between "for" and "(".
> need a space between ";" and "i++".
> Done.

> http://codereview.chromium.org/18417/diff/1482/1557#newcode128
> Line 128: L"/encoding_alias_mapping/" + file_name[i]);
> please take a look
> http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Calls
> for the format of function call.
> Same problems happened on line 149, 326
> Done.

> http://codereview.chromium.org/18417/diff/1482/1557#newcode142
> Line 142:
>
L"encoding_user_override\\expect_gb18030_content_save_from_encoding_iso88591.htm";
> exceeds 80 characters limitation,
> same problems happened on line 190
> Done.

> http://codereview.chromium.org/18417/diff/1482/1557#newcode171
> Line 171: EXPECT_TRUE(tab->SavePage(full_file_name, dir,
> move the line 144-145 to the position before this line.
> Done.

> http://codereview.chromium.org/18417/diff/1482/1557#newcode175
> Line 175: }
> For test of encoding oevrride, testing overriding from "8859-1" to "gb18030"
is
> good enough?
> I thought override one encoding is enough, could you please give me your
prefer?
I will also check with Jungshik.

> http://codereview.chromium.org/18417/diff/1482/1557#newcode178
> Line 178: // Below encodings are ignored because I don't have en editor to
> create
> please this redundant comments
> Done.

> http://codereview.chromium.org/18417/diff/1482/1557#newcode209
> Line 209: std::wstring file_name[] = {
> same problem with line 68
> Done.

> http://codereview.chromium.org/18417/diff/1482/1557#newcode220
> Line 220: // L"windows-874_content_without_encoding_declaration.htm",
> please remove useless code instead of commenting them
> Done.

> http://codereview.chromium.org/18417/diff/1482/1557#newcode233
> Line 233: std::wstring expect_folder_name =
> L"encoding_auto_detect\\expect_results\\";
> same problem with line 68
> Done.

> http://codereview.chromium.org/18417/diff/1482/1557#newcode234
> Line 234: std::wstring expect_file_name[] = {
> same problem with line 68
> Done.

> http://codereview.chromium.org/18417/diff/1482/1557#newcode245
> Line 245: //
> L"expect_windows-874_content_save_from_without_encoding_declaration.htm",
> same problem with line 220
> Done.

> http://codereview.chromium.org/18417/diff/1482/1557#newcode261
> Line 261: std::wstring expect_encoding[] = {L"Big5",
> same problem with line 68
> Done.

> http://codereview.chromium.org/18417/diff/1482/1557#newcode271
> Line 271: // L"windows-874",
> same problem with line 220
> Done.

> http://codereview.chromium.org/18417/diff/1482/1557#newcode284
> Line 284: std::wstring save_dir_sub[] = {
> same problem with line 68
> Done.

> http://codereview.chromium.org/18417/diff/1482/1557#newcode295
> Line 295: // L"windows-874_content_without_encoding_declaration_files",
> same problem with line 220
> Done.

> http://codereview.chromium.org/18417/diff/1482/1557#newcode349
> Line 349: // Re-get the encoding of page. It should be gb18030.
> should be "gb18030"? it looks the comments need to be changed.
> Done.

> http://codereview.chromium.org/18417/diff/1482/1558
> File chrome/browser/download/save_page_uitest.cc (right):
> 
> http://codereview.chromium.org/18417/diff/1482/1558#newcode26
> Line 26: std::wstring expect_result_filepath =
> You might want to move this function to UITest with adding one additional
> parameter "test_dir_path" to remove the duplicate code both encoding_uitest
and
> save_page_uitest.
> The new function looks like
> void CheckFile(const std::wstring& generated_file,
>                const std::wstring& expect_result_file,
>                const std::wstring& test_dir_path,
>                bool check_equal)

[Johnny(Jianning) Ding, 2009-03-09 12:31:46]

The following are some comments. Thanks!

http://codereview.chromium.org/18417/diff/3483/2477
File chrome/browser/browser_encoding_uitest.cc (right):

http://codereview.chromium.org/18417/diff/3483/2477#newcode27
Line 27: void CheckFile(const std::wstring& generated_file,
would please move this function to UITest with adding one additional parameter
"test_dir_path". Then we can remove the  duplicated duplicate CheckFile both
encoding_uitest and savepage_uitest

http://codereview.chromium.org/18417/diff/3483/2477#newcode59
Line 59: // ISO-8859-16
would you please file a bug for those encoding, the once you  have practicable
editor, you can add those tests.

http://codereview.chromium.org/18417/diff/3483/2477#newcode61
Line 61: struct EncodingTestData {
you can move the definition to class BrowserEncodingTest, then we use can it in
those test functions.

http://codereview.chromium.org/18417/diff/3483/2477#newcode100
Line 100: kTestDir + L"/encoding_alias_mapping/" +
encoding_test_data[i].file_name);
needs 4 spaces indent, you can write it like
url = URLRequestMockHTTPJob::GetMockUrl(kTestDir + 
    L"/encoding_alias_mapping/" + encoding_test_data[i].file_name);
or
url = URLRequestMockHTTPJob::GetMockUrl(kTestDir + 
                                        L"/encoding_alias_mapping/" +
                                        encoding_test_data[i].file_name);

http://codereview.chromium.org/18417/diff/3483/2477#newcode117
Line 117: kTestDir + L"/encoding_user_override/" + file_name);
4 spaces indent

http://codereview.chromium.org/18417/diff/3483/2477#newcode156
Line 156: 
please remove this empty line.

http://codereview.chromium.org/18417/diff/3483/2477#newcode161
Line 161: 
please remove this empty line.

http://codereview.chromium.org/18417/diff/3483/2477#newcode167
Line 167: const wchar_t* file_name[] = {
please use the previous defined "EncodingTestData" structure instead of using
two separated string arrays.

http://codereview.chromium.org/18417/diff/3483/2477#newcode228
Line 228: L"Big5_content_without_encoding_declaration_files",
you can define the prefix name like EncodingTestData EncodingTestData
encoding_test_data[] = {
{ L"Big5_content_without_encoding_declaration", L"Big5" },
...
};
so 
test_file_name = encoding_test_data[i] + L".htm";
expact_file_name = L"expect_" + encoding_test_data[i] + L".htm";
sub_dir_name =  encoding_test_data[i] + L"_files";

it will make code more readable.

http://codereview.chromium.org/18417/diff/3483/2477#newcode264
Line 264: kTestDir + L"/encoding_auto_detect/" + file_name[i]);
4 spaces indent

http://codereview.chromium.org/18417/diff/3483/2479
File chrome/test/ui/ui_test.cc (right):

http://codereview.chromium.org/18417/diff/3483/2479#newcode759
Line 759: const std::wstring &test_case) {
make the parameter of this line aligned with previous parameter

http://codereview.chromium.org/18417/diff/3483/2480
File chrome/test/ui/ui_tests.vcproj (right):

http://codereview.chromium.org/18417/diff/3483/2480#newcode541
Line 541: >
only needs to add browser_encoding_uitest.cc in this project file. Make sure no
other changes in this file