Add a UI test for "Encoding" menu. Please see crbug.com/5515 for more details...

Add a UI test for "Encoding" menu. Please see crbug.com/5515 for more details.This change list is based on http://codereview.chromium.org/18417 which was written by xlyuan@chromiumBug=5515
( http://crbug.com/5515 )

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

[jungshik at Google, 2009-08-20 17:58:39]

Sorry for the delay. Overall looks good. Let's go one more round. Thank you for
working on this.

http://codereview.chromium.org/165393/diff/78/152
File chrome/browser/browser_encoding_uitest.cc (right):

http://codereview.chromium.org/165393/diff/78/152#newcode1
Line 1: // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
nit: should be just 2009, I guess because this is a new file.

http://codereview.chromium.org/165393/diff/78/152#newcode26
Line 26: // Make sure the content of the page are expected
nit: the page are as expected.

http://codereview.chromium.org/165393/diff/78/152#newcode52
Line 52: // for more details.
nit: Please, replace the above comment with the following:

TODO: 1. Some encodings are missing here. It'll be added later. See
http://crbug.com/13306.
2. Add more files with multiple encoding name variants for each canonical
encoding name). Webkit layout tests cover some, but testing in the UI test is
also necessary.

http://codereview.chromium.org/165393/diff/78/152#newcode66
Line 66: { L"encoding-windows-949.htm", L"windows-949" },
we don't want to expose 'windows-949', but I guess we do at the moment. I'll
take care of that in another bug. Don't worry about it here.

http://codereview.chromium.org/165393/diff/78/152#newcode67
Line 67: { L"encoding-Shift-JIS.htm", L"Shift_JIS" },
nit: All these html files are in encoding_alias_mapping directory. Please, get
rid  'encoding-' prefix of all html files. And, as a long time Unix/Linux users,
I'd prefer 'html' suffix to  'htm' suffix.

http://codereview.chromium.org/165393/diff/78/152#newcode113
Line 113: L"gb18030_content_with_encoding_iso88591.htm";
nit: How about gb18030_with_iso88591_meta.html?  

Your current name seems contradictory (gb18030 content but encoding is
iso-8859-1? I know what you meant, but still a bit strange).

http://codereview.chromium.org/165393/diff/78/152#newcode115
Line 115: L"expect_gb18030_content_save_from_iso88591.htm";
nit: this one is tough....
how about 
gb18030_expected_from_iso88591_meta.html or

expected_gb18030_saved_from_iso88591_meta.html ?

http://codereview.chromium.org/165393/diff/78/152#newcode127
Line 127: // Get the original defined encoding in the page.
nit: Get the encoding declared in the page.

http://codereview.chromium.org/165393/diff/78/152#newcode142
Line 142: // Dump the page, the content of dump page should be equal with our
expected
nit: should be identical to the expected result file, |expect...html|

http://codereview.chromium.org/165393/diff/78/152#newcode146
Line 146: L"gb18030_content_with_encoding_iso88591_files";
I was a bit confused initially. Can you add a comment about this directory (why
we need to specify it)?

http://codereview.chromium.org/165393/diff/78/152#newcode148
Line 148: SavePackage::SAVE_AS_COMPLETE_HTML));
Can we just use SAVE_AS_ONLY_HTML and get rid of |dir| above? 

Later, if we add test files with external resources (frames, javascript, css),
we should use SAVE_AS_COMPLETE_HTML, but we don't do that now, do we? 

So, why don't you add a TODO comment and just use SAVE_AS_ONLY_HTML?

http://codereview.chromium.org/165393/diff/78/152#newcode160
Line 160: // auto detect doesn't return right value for them
nit: Change the comment as below:

The following encodings are excluded from the auto-detection test because it's a
known issue that the current encoding detector does not detect them: ISO-8859-4,
ISO-8859-13, .... windows-874, windows-1258, windows-1253

http://codereview.chromium.org/165393/diff/78/152#newcode182
Line 182: { L"Big5_content_without_encoding_declaration.htm",
nit: let's try to shorten filenames a little bit:

How about "Big5_with_no_encoding_specified.html"? We don't need 'content'.

http://codereview.chromium.org/165393/diff/78/152#newcode183
Line 183: L"expect_Big5_content_save_from_without_encoding_declaration.htm",
nit: how about expected_Big5_saved_from_no_encoding_specified.html ? 

and you can make the same changes to all other file names below.

http://codereview.chromium.org/165393/diff/78/152#newcode236
Line 236: tmp_save_dir += L"tmp_save_dir";
Again, at the moment, we don't do anything with sub-resources. Then, we can just
use SAVE_AS_ONLY_HTML and forget about sub-resources directory, can't we?

http://codereview.chromium.org/165393/diff/78/152#newcode243
Line 243: bool new_encoding_auto_detect = false;
I'm at loss as to why you need these two variables rather than just
encoding_auto_detection?

http://codereview.chromium.org/165393/diff/78/152#newcode248
Line 248: // incorrectly decode the page.
This comment does not hold. You're not just testing Simplified Chinese encodings
here. Perhaps, you have to set  the default to one of encodings not supported by
the current auto-detector (that are excluded from this test). Let's set it to
ISO-8859-4.

http://codereview.chromium.org/165393/diff/78/152#newcode262
Line 262: encoding_auto_detect));
you can call it with |false|, can't you?

http://codereview.chromium.org/165393/diff/78/152#newcode273
Line 273: prefs::kWebKitUsesUniversalDetector, !encoding_auto_detect));
here again, you can just call it with |true|, can't you?

http://codereview.chromium.org/165393/diff/78/152#newcode280
Line 280: EXPECT_EQ(new_encoding_auto_detect, !encoding_auto_detect);
Getter can be called with |&encoding_auto_detection| and its result can be
checked with EXPECT_TRUE, right?

http://codereview.chromium.org/165393/diff/78/153
File chrome/browser/download/save_page_uitest.cc (right):

http://codereview.chromium.org/165393/diff/78/153#newcode33
Line 33: const FilePath& expect_result_file) {
nit: expected_result_file and expected_result_filepath

http://codereview.chromium.org/165393/diff/78/151
File chrome/test/ui/ui_test.h (right):

http://codereview.chromium.org/165393/diff/78/151#newcode266
Line 266: // Waits for a generated file ready, then check it with the original
file
nit: Wait for |generated_file| to be ready and then compare it with 
|original_file| to see if they're identical or not if |compare_file| is true. If
need_equal is true, they need to be identical. Otherwise, they should be
different.

[Johnny(Jianning) Ding, 2009-08-21 18:17:18]

Done, please take another look. Thanks!

http://codereview.chromium.org/165393/diff/78/152
File chrome/browser/browser_encoding_uitest.cc (right):

http://codereview.chromium.org/165393/diff/78/152#newcode1
Line 1: // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
On 2009/08/20 17:58:39, Jungshik Shin wrote:
> nit: should be just 2009, I guess because this is a new file.

Done.

http://codereview.chromium.org/165393/diff/78/152#newcode26
Line 26: // Make sure the content of the page are expected
On 2009/08/20 17:58:39, Jungshik Shin wrote:
> nit: the page are as expected.

Done.

http://codereview.chromium.org/165393/diff/78/152#newcode52
Line 52: // for more details.
On 2009/08/20 17:58:39, Jungshik Shin wrote:
> nit: Please, replace the above comment with the following:
> 
> TODO: 1. Some encodings are missing here. It'll be added later. See
> http://crbug.com/13306.
> 2. Add more files with multiple encoding name variants for each canonical
> encoding name). Webkit layout tests cover some, but testing in the UI test is
> also necessary.

Done.

http://codereview.chromium.org/165393/diff/78/152#newcode66
Line 66: { L"encoding-windows-949.htm", L"windows-949" },
On 2009/08/20 17:58:39, Jungshik Shin wrote:
> we don't want to expose 'windows-949', but I guess we do at the moment. I'll
> take care of that in another bug. Don't worry about it here. 
> 

Thanks.

http://codereview.chromium.org/165393/diff/78/152#newcode67
Line 67: { L"encoding-Shift-JIS.htm", L"Shift_JIS" },
On 2009/08/20 17:58:39, Jungshik Shin wrote:
> nit: All these html files are in encoding_alias_mapping directory. Please, get
> rid  'encoding-' prefix of all html files. And, as a long time Unix/Linux
users,
> I'd prefer 'html' suffix to  'htm' suffix. 
> 

Done.

http://codereview.chromium.org/165393/diff/78/152#newcode113
Line 113: L"gb18030_content_with_encoding_iso88591.htm";
Your name is great. Thanks!
On 2009/08/20 17:58:39, Jungshik Shin wrote:
> nit: How about gb18030_with_iso88591_meta.html?  
> 
> Your current name seems contradictory (gb18030 content but encoding is
> iso-8859-1? I know what you meant, but still a bit strange). 
> 
> 
>

http://codereview.chromium.org/165393/diff/78/152#newcode115
Line 115: L"expect_gb18030_content_save_from_iso88591.htm";
On 2009/08/20 17:58:39, Jungshik Shin wrote:
> nit: this one is tough....
> how about 
> gb18030_expected_from_iso88591_meta.html or
> 
> expected_gb18030_saved_from_iso88591_meta.html ? 
> 
> 

Done.

http://codereview.chromium.org/165393/diff/78/152#newcode127
Line 127: // Get the original defined encoding in the page.
On 2009/08/20 17:58:39, Jungshik Shin wrote:
> nit: Get the encoding declared in the page.

Done.

http://codereview.chromium.org/165393/diff/78/152#newcode142
Line 142: // Dump the page, the content of dump page should be equal with our
expected
On 2009/08/20 17:58:39, Jungshik Shin wrote:
> nit: should be identical to the expected result file, |expect...html|
> 

Done.

http://codereview.chromium.org/165393/diff/78/152#newcode146
Line 146: L"gb18030_content_with_encoding_iso88591_files";
On 2009/08/20 17:58:39, Jungshik Shin wrote:
> I was a bit confused initially. Can you add a comment about this directory
(why
> we need to specify it)? 

Done.

http://codereview.chromium.org/165393/diff/78/152#newcode148
Line 148: SavePackage::SAVE_AS_COMPLETE_HTML));
We have to save it as complete HTML because Chrome re-writes the correct meta
tag for charset declaration only when using this way. In here we need to make
sure that the override encoding can be correctly serialized, so we use
SAVE_AS_COMPLETE_HTML.
On 2009/08/20 17:58:39, Jungshik Shin wrote:
> Can we just use SAVE_AS_ONLY_HTML and get rid of |dir| above? 
> 
> Later, if we add test files with external resources (frames, javascript, css),
> we should use SAVE_AS_COMPLETE_HTML, but we don't do that now, do we? 
> 
> So, why don't you add a TODO comment and just use SAVE_AS_ONLY_HTML?

http://codereview.chromium.org/165393/diff/78/152#newcode160
Line 160: // auto detect doesn't return right value for them
On 2009/08/20 17:58:39, Jungshik Shin wrote:
> nit: Change the comment as below:
> 
> The following encodings are excluded from the auto-detection test because it's
a
> known issue that the current encoding detector does not detect them:
ISO-8859-4,
> ISO-8859-13, .... windows-874, windows-1258, windows-1253
> 
> 
> 

Done.

http://codereview.chromium.org/165393/diff/78/152#newcode182
Line 182: { L"Big5_content_without_encoding_declaration.htm",
On 2009/08/20 17:58:39, Jungshik Shin wrote:
> nit: let's try to shorten filenames a little bit:
> 
> How about "Big5_with_no_encoding_specified.html"? We don't need 'content'.  

Done.

http://codereview.chromium.org/165393/diff/78/152#newcode183
Line 183: L"expect_Big5_content_save_from_without_encoding_declaration.htm",
On 2009/08/20 17:58:39, Jungshik Shin wrote:
> nit: how about expected_Big5_saved_from_no_encoding_specified.html ? 
> 
> and you can make the same changes to all other file names below.
> 

Done.

http://codereview.chromium.org/165393/diff/78/152#newcode243
Line 243: bool new_encoding_auto_detect = false;
On 2009/08/20 17:58:39, Jungshik Shin wrote:
> I'm at loss as to why you need these two variables rather than just
> encoding_auto_detection? 

Done.

http://codereview.chromium.org/165393/diff/78/152#newcode248
Line 248: // incorrectly decode the page.
On 2009/08/20 17:58:39, Jungshik Shin wrote:
> This comment does not hold. You're not just testing Simplified Chinese
encodings
> here. Perhaps, you have to set  the default to one of encodings not supported
by
> the current auto-detector (that are excluded from this test). Let's set it to
> ISO-8859-4.
> 
> 

Done.

http://codereview.chromium.org/165393/diff/78/152#newcode262
Line 262: encoding_auto_detect));
On 2009/08/20 17:58:39, Jungshik Shin wrote:
> you can call it with |false|, can't you?

Done.

http://codereview.chromium.org/165393/diff/78/152#newcode273
Line 273: prefs::kWebKitUsesUniversalDetector, !encoding_auto_detect));
On 2009/08/20 17:58:39, Jungshik Shin wrote:
> here again, you can just call it with |true|, can't you?

Done.

http://codereview.chromium.org/165393/diff/78/152#newcode280
Line 280: EXPECT_EQ(new_encoding_auto_detect, !encoding_auto_detect);
On 2009/08/20 17:58:39, Jungshik Shin wrote:
> Getter can be called with |&encoding_auto_detection| and its result can be
> checked with EXPECT_TRUE, right? 
> 

Done.

http://codereview.chromium.org/165393/diff/78/153
File chrome/browser/download/save_page_uitest.cc (right):

http://codereview.chromium.org/165393/diff/78/153#newcode33
Line 33: const FilePath& expect_result_file) {
On 2009/08/20 17:58:39, Jungshik Shin wrote:
> nit: expected_result_file and expected_result_filepath

Done.

http://codereview.chromium.org/165393/diff/78/151
File chrome/test/ui/ui_test.h (right):

http://codereview.chromium.org/165393/diff/78/151#newcode266
Line 266: // Waits for a generated file ready, then check it with the original
file
On 2009/08/20 17:58:39, Jungshik Shin wrote:
> nit: Wait for |generated_file| to be ready and then compare it with 
> |original_file| to see if they're identical or not if |compare_file| is true.
If
> need_equal is true, they need to be identical. Otherwise, they should be
> different.

Done.

[jungshik at Google, 2009-08-21 20:18:53]

LGTM

Before checking this in, can you make these directory names shorter?

encoding_alias_mapping ==> alias_mapping
encoding_auto_detect ==> auto_detect

They're already under encoding_tests directory so that I think we don't need
'encoding_'.  

Thank you !

On 2009/08/21 18:17:18, Johnny(Jianning) Ding wrote:
> Done, please take another look. Thanks!
> 
> http://codereview.chromium.org/165393/diff/78/152
> File chrome/browser/browser_encoding_uitest.cc (right):
> 
> http://codereview.chromium.org/165393/diff/78/152#newcode1
> Line 1: // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
> On 2009/08/20 17:58:39, Jungshik Shin wrote:
> > nit: should be just 2009, I guess because this is a new file.
> 
> Done.
> 
> http://codereview.chromium.org/165393/diff/78/152#newcode26
> Line 26: // Make sure the content of the page are expected
> On 2009/08/20 17:58:39, Jungshik Shin wrote:
> > nit: the page are as expected.
> 
> Done.
> 
> http://codereview.chromium.org/165393/diff/78/152#newcode52
> Line 52: // for more details.
> On 2009/08/20 17:58:39, Jungshik Shin wrote:
> > nit: Please, replace the above comment with the following:
> > 
> > TODO: 1. Some encodings are missing here. It'll be added later. See
> > http://crbug.com/13306.
> > 2. Add more files with multiple encoding name variants for each canonical
> > encoding name). Webkit layout tests cover some, but testing in the UI test
is
> > also necessary.
> 
> Done.
> 
> http://codereview.chromium.org/165393/diff/78/152#newcode66
> Line 66: { L"encoding-windows-949.htm", L"windows-949" },
> On 2009/08/20 17:58:39, Jungshik Shin wrote:
> > we don't want to expose 'windows-949', but I guess we do at the moment. I'll
> > take care of that in another bug. Don't worry about it here. 
> > 
> 
> Thanks.
> 
> http://codereview.chromium.org/165393/diff/78/152#newcode67
> Line 67: { L"encoding-Shift-JIS.htm", L"Shift_JIS" },
> On 2009/08/20 17:58:39, Jungshik Shin wrote:
> > nit: All these html files are in encoding_alias_mapping directory. Please,
get
> > rid  'encoding-' prefix of all html files. And, as a long time Unix/Linux
> users,
> > I'd prefer 'html' suffix to  'htm' suffix. 
> > 
> 
> Done.
> 
> http://codereview.chromium.org/165393/diff/78/152#newcode113
> Line 113: L"gb18030_content_with_encoding_iso88591.htm";
> Your name is great. Thanks!
> On 2009/08/20 17:58:39, Jungshik Shin wrote:
> > nit: How about gb18030_with_iso88591_meta.html?  
> > 
> > Your current name seems contradictory (gb18030 content but encoding is
> > iso-8859-1? I know what you meant, but still a bit strange). 
> > 
> > 
> >
> 
> http://codereview.chromium.org/165393/diff/78/152#newcode115
> Line 115: L"expect_gb18030_content_save_from_iso88591.htm";
> On 2009/08/20 17:58:39, Jungshik Shin wrote:
> > nit: this one is tough....
> > how about 
> > gb18030_expected_from_iso88591_meta.html or
> > 
> > expected_gb18030_saved_from_iso88591_meta.html ? 
> > 
> > 
> 
> Done.
> 
> http://codereview.chromium.org/165393/diff/78/152#newcode127
> Line 127: // Get the original defined encoding in the page.
> On 2009/08/20 17:58:39, Jungshik Shin wrote:
> > nit: Get the encoding declared in the page.
> 
> Done.
> 
> http://codereview.chromium.org/165393/diff/78/152#newcode142
> Line 142: // Dump the page, the content of dump page should be equal with our
> expected
> On 2009/08/20 17:58:39, Jungshik Shin wrote:
> > nit: should be identical to the expected result file, |expect...html|
> > 
> 
> Done.
> 
> http://codereview.chromium.org/165393/diff/78/152#newcode146
> Line 146: L"gb18030_content_with_encoding_iso88591_files";
> On 2009/08/20 17:58:39, Jungshik Shin wrote:
> > I was a bit confused initially. Can you add a comment about this directory
> (why
> > we need to specify it)? 
> 
> Done.
> 
> http://codereview.chromium.org/165393/diff/78/152#newcode148
> Line 148: SavePackage::SAVE_AS_COMPLETE_HTML));
> We have to save it as complete HTML because Chrome re-writes the correct meta
> tag for charset declaration only when using this way. In here we need to make
> sure that the override encoding can be correctly serialized, so we use
> SAVE_AS_COMPLETE_HTML.
> On 2009/08/20 17:58:39, Jungshik Shin wrote:
> > Can we just use SAVE_AS_ONLY_HTML and get rid of |dir| above? 
> > 
> > Later, if we add test files with external resources (frames, javascript,
css),
> > we should use SAVE_AS_COMPLETE_HTML, but we don't do that now, do we? 
> > 
> > So, why don't you add a TODO comment and just use SAVE_AS_ONLY_HTML?
> 
> http://codereview.chromium.org/165393/diff/78/152#newcode160
> Line 160: // auto detect doesn't return right value for them
> On 2009/08/20 17:58:39, Jungshik Shin wrote:
> > nit: Change the comment as below:
> > 
> > The following encodings are excluded from the auto-detection test because
it's
> a
> > known issue that the current encoding detector does not detect them:
> ISO-8859-4,
> > ISO-8859-13, .... windows-874, windows-1258, windows-1253
> > 
> > 
> > 
> 
> Done.
> 
> http://codereview.chromium.org/165393/diff/78/152#newcode182
> Line 182: { L"Big5_content_without_encoding_declaration.htm",
> On 2009/08/20 17:58:39, Jungshik Shin wrote:
> > nit: let's try to shorten filenames a little bit:
> > 
> > How about "Big5_with_no_encoding_specified.html"? We don't need 'content'.  
> 
> Done.
> 
> http://codereview.chromium.org/165393/diff/78/152#newcode183
> Line 183: L"expect_Big5_content_save_from_without_encoding_declaration.htm",
> On 2009/08/20 17:58:39, Jungshik Shin wrote:
> > nit: how about expected_Big5_saved_from_no_encoding_specified.html ? 
> > 
> > and you can make the same changes to all other file names below.
> > 
> 
> Done.
> 
> http://codereview.chromium.org/165393/diff/78/152#newcode243
> Line 243: bool new_encoding_auto_detect = false;
> On 2009/08/20 17:58:39, Jungshik Shin wrote:
> > I'm at loss as to why you need these two variables rather than just
> > encoding_auto_detection? 
> 
> Done.
> 
> http://codereview.chromium.org/165393/diff/78/152#newcode248
> Line 248: // incorrectly decode the page.
> On 2009/08/20 17:58:39, Jungshik Shin wrote:
> > This comment does not hold. You're not just testing Simplified Chinese
> encodings
> > here. Perhaps, you have to set  the default to one of encodings not
supported
> by
> > the current auto-detector (that are excluded from this test). Let's set it
to
> > ISO-8859-4.
> > 
> > 
> 
> Done.
> 
> http://codereview.chromium.org/165393/diff/78/152#newcode262
> Line 262: encoding_auto_detect));
> On 2009/08/20 17:58:39, Jungshik Shin wrote:
> > you can call it with |false|, can't you?
> 
> Done.
> 
> http://codereview.chromium.org/165393/diff/78/152#newcode273
> Line 273: prefs::kWebKitUsesUniversalDetector, !encoding_auto_detect));
> On 2009/08/20 17:58:39, Jungshik Shin wrote:
> > here again, you can just call it with |true|, can't you?
> 
> Done.
> 
> http://codereview.chromium.org/165393/diff/78/152#newcode280
> Line 280: EXPECT_EQ(new_encoding_auto_detect, !encoding_auto_detect);
> On 2009/08/20 17:58:39, Jungshik Shin wrote:
> > Getter can be called with |&encoding_auto_detection| and its result can be
> > checked with EXPECT_TRUE, right? 
> > 
> 
> Done.
> 
> http://codereview.chromium.org/165393/diff/78/153
> File chrome/browser/download/save_page_uitest.cc (right):
> 
> http://codereview.chromium.org/165393/diff/78/153#newcode33
> Line 33: const FilePath& expect_result_file) {
> On 2009/08/20 17:58:39, Jungshik Shin wrote:
> > nit: expected_result_file and expected_result_filepath
> 
> Done.
> 
> http://codereview.chromium.org/165393/diff/78/151
> File chrome/test/ui/ui_test.h (right):
> 
> http://codereview.chromium.org/165393/diff/78/151#newcode266
> Line 266: // Waits for a generated file ready, then check it with the original
> file
> On 2009/08/20 17:58:39, Jungshik Shin wrote:
> > nit: Wait for |generated_file| to be ready and then compare it with 
> > |original_file| to see if they're identical or not if |compare_file| is
true.
> If
> > need_equal is true, they need to be identical. Otherwise, they should be
> > different.
> 
> Done.

[Johnny(Jianning) Ding, 2009-08-22 03:30:26]

Done.

On 2009/08/21 20:18:53, Jungshik Shin wrote:
> LGTM
> 
> Before checking this in, can you make these directory names shorter?
> 
> encoding_alias_mapping ==> alias_mapping
> encoding_auto_detect ==> auto_detect
> 
> They're already under encoding_tests directory so that I think we don't need
> 'encoding_'.  
> 
> Thank you !
> 
> On 2009/08/21 18:17:18, Johnny(Jianning) Ding wrote:
> > Done, please take another look. Thanks!
> > 
> > http://codereview.chromium.org/165393/diff/78/152
> > File chrome/browser/browser_encoding_uitest.cc (right):
> > 
> > http://codereview.chromium.org/165393/diff/78/152#newcode1
> > Line 1: // Copyright (c) 2006-2008 The Chromium Authors. All rights
reserved.
> > On 2009/08/20 17:58:39, Jungshik Shin wrote:
> > > nit: should be just 2009, I guess because this is a new file.
> > 
> > Done.
> > 
> > http://codereview.chromium.org/165393/diff/78/152#newcode26
> > Line 26: // Make sure the content of the page are expected
> > On 2009/08/20 17:58:39, Jungshik Shin wrote:
> > > nit: the page are as expected.
> > 
> > Done.
> > 
> > http://codereview.chromium.org/165393/diff/78/152#newcode52
> > Line 52: // for more details.
> > On 2009/08/20 17:58:39, Jungshik Shin wrote:
> > > nit: Please, replace the above comment with the following:
> > > 
> > > TODO: 1. Some encodings are missing here. It'll be added later. See
> > > http://crbug.com/13306.
> > > 2. Add more files with multiple encoding name variants for each canonical
> > > encoding name). Webkit layout tests cover some, but testing in the UI test
> is
> > > also necessary.
> > 
> > Done.
> > 
> > http://codereview.chromium.org/165393/diff/78/152#newcode66
> > Line 66: { L"encoding-windows-949.htm", L"windows-949" },
> > On 2009/08/20 17:58:39, Jungshik Shin wrote:
> > > we don't want to expose 'windows-949', but I guess we do at the moment.
I'll
> > > take care of that in another bug. Don't worry about it here. 
> > > 
> > 
> > Thanks.
> > 
> > http://codereview.chromium.org/165393/diff/78/152#newcode67
> > Line 67: { L"encoding-Shift-JIS.htm", L"Shift_JIS" },
> > On 2009/08/20 17:58:39, Jungshik Shin wrote:
> > > nit: All these html files are in encoding_alias_mapping directory. Please,
> get
> > > rid  'encoding-' prefix of all html files. And, as a long time Unix/Linux
> > users,
> > > I'd prefer 'html' suffix to  'htm' suffix. 
> > > 
> > 
> > Done.
> > 
> > http://codereview.chromium.org/165393/diff/78/152#newcode113
> > Line 113: L"gb18030_content_with_encoding_iso88591.htm";
> > Your name is great. Thanks!
> > On 2009/08/20 17:58:39, Jungshik Shin wrote:
> > > nit: How about gb18030_with_iso88591_meta.html?  
> > > 
> > > Your current name seems contradictory (gb18030 content but encoding is
> > > iso-8859-1? I know what you meant, but still a bit strange). 
> > > 
> > > 
> > >
> > 
> > http://codereview.chromium.org/165393/diff/78/152#newcode115
> > Line 115: L"expect_gb18030_content_save_from_iso88591.htm";
> > On 2009/08/20 17:58:39, Jungshik Shin wrote:
> > > nit: this one is tough....
> > > how about 
> > > gb18030_expected_from_iso88591_meta.html or
> > > 
> > > expected_gb18030_saved_from_iso88591_meta.html ? 
> > > 
> > > 
> > 
> > Done.
> > 
> > http://codereview.chromium.org/165393/diff/78/152#newcode127
> > Line 127: // Get the original defined encoding in the page.
> > On 2009/08/20 17:58:39, Jungshik Shin wrote:
> > > nit: Get the encoding declared in the page.
> > 
> > Done.
> > 
> > http://codereview.chromium.org/165393/diff/78/152#newcode142
> > Line 142: // Dump the page, the content of dump page should be equal with
our
> > expected
> > On 2009/08/20 17:58:39, Jungshik Shin wrote:
> > > nit: should be identical to the expected result file, |expect...html|
> > > 
> > 
> > Done.
> > 
> > http://codereview.chromium.org/165393/diff/78/152#newcode146
> > Line 146: L"gb18030_content_with_encoding_iso88591_files";
> > On 2009/08/20 17:58:39, Jungshik Shin wrote:
> > > I was a bit confused initially. Can you add a comment about this directory
> > (why
> > > we need to specify it)? 
> > 
> > Done.
> > 
> > http://codereview.chromium.org/165393/diff/78/152#newcode148
> > Line 148: SavePackage::SAVE_AS_COMPLETE_HTML));
> > We have to save it as complete HTML because Chrome re-writes the correct
meta
> > tag for charset declaration only when using this way. In here we need to
make
> > sure that the override encoding can be correctly serialized, so we use
> > SAVE_AS_COMPLETE_HTML.
> > On 2009/08/20 17:58:39, Jungshik Shin wrote:
> > > Can we just use SAVE_AS_ONLY_HTML and get rid of |dir| above? 
> > > 
> > > Later, if we add test files with external resources (frames, javascript,
> css),
> > > we should use SAVE_AS_COMPLETE_HTML, but we don't do that now, do we? 
> > > 
> > > So, why don't you add a TODO comment and just use SAVE_AS_ONLY_HTML?
> > 
> > http://codereview.chromium.org/165393/diff/78/152#newcode160
> > Line 160: // auto detect doesn't return right value for them
> > On 2009/08/20 17:58:39, Jungshik Shin wrote:
> > > nit: Change the comment as below:
> > > 
> > > The following encodings are excluded from the auto-detection test because
> it's
> > a
> > > known issue that the current encoding detector does not detect them:
> > ISO-8859-4,
> > > ISO-8859-13, .... windows-874, windows-1258, windows-1253
> > > 
> > > 
> > > 
> > 
> > Done.
> > 
> > http://codereview.chromium.org/165393/diff/78/152#newcode182
> > Line 182: { L"Big5_content_without_encoding_declaration.htm",
> > On 2009/08/20 17:58:39, Jungshik Shin wrote:
> > > nit: let's try to shorten filenames a little bit:
> > > 
> > > How about "Big5_with_no_encoding_specified.html"? We don't need 'content'.
 
> > 
> > Done.
> > 
> > http://codereview.chromium.org/165393/diff/78/152#newcode183
> > Line 183: L"expect_Big5_content_save_from_without_encoding_declaration.htm",
> > On 2009/08/20 17:58:39, Jungshik Shin wrote:
> > > nit: how about expected_Big5_saved_from_no_encoding_specified.html ? 
> > > 
> > > and you can make the same changes to all other file names below.
> > > 
> > 
> > Done.
> > 
> > http://codereview.chromium.org/165393/diff/78/152#newcode243
> > Line 243: bool new_encoding_auto_detect = false;
> > On 2009/08/20 17:58:39, Jungshik Shin wrote:
> > > I'm at loss as to why you need these two variables rather than just
> > > encoding_auto_detection? 
> > 
> > Done.
> > 
> > http://codereview.chromium.org/165393/diff/78/152#newcode248
> > Line 248: // incorrectly decode the page.
> > On 2009/08/20 17:58:39, Jungshik Shin wrote:
> > > This comment does not hold. You're not just testing Simplified Chinese
> > encodings
> > > here. Perhaps, you have to set  the default to one of encodings not
> supported
> > by
> > > the current auto-detector (that are excluded from this test). Let's set it
> to
> > > ISO-8859-4.
> > > 
> > > 
> > 
> > Done.
> > 
> > http://codereview.chromium.org/165393/diff/78/152#newcode262
> > Line 262: encoding_auto_detect));
> > On 2009/08/20 17:58:39, Jungshik Shin wrote:
> > > you can call it with |false|, can't you?
> > 
> > Done.
> > 
> > http://codereview.chromium.org/165393/diff/78/152#newcode273
> > Line 273: prefs::kWebKitUsesUniversalDetector, !encoding_auto_detect));
> > On 2009/08/20 17:58:39, Jungshik Shin wrote:
> > > here again, you can just call it with |true|, can't you?
> > 
> > Done.
> > 
> > http://codereview.chromium.org/165393/diff/78/152#newcode280
> > Line 280: EXPECT_EQ(new_encoding_auto_detect, !encoding_auto_detect);
> > On 2009/08/20 17:58:39, Jungshik Shin wrote:
> > > Getter can be called with |&encoding_auto_detection| and its result can be
> > > checked with EXPECT_TRUE, right? 
> > > 
> > 
> > Done.
> > 
> > http://codereview.chromium.org/165393/diff/78/153
> > File chrome/browser/download/save_page_uitest.cc (right):
> > 
> > http://codereview.chromium.org/165393/diff/78/153#newcode33
> > Line 33: const FilePath& expect_result_file) {
> > On 2009/08/20 17:58:39, Jungshik Shin wrote:
> > > nit: expected_result_file and expected_result_filepath
> > 
> > Done.
> > 
> > http://codereview.chromium.org/165393/diff/78/151
> > File chrome/test/ui/ui_test.h (right):
> > 
> > http://codereview.chromium.org/165393/diff/78/151#newcode266
> > Line 266: // Waits for a generated file ready, then check it with the
original
> > file
> > On 2009/08/20 17:58:39, Jungshik Shin wrote:
> > > nit: Wait for |generated_file| to be ready and then compare it with 
> > > |original_file| to see if they're identical or not if |compare_file| is
> true.
> > If
> > > need_equal is true, they need to be identical. Otherwise, they should be
> > > different.
> > 
> > Done.