Tweaking WebPageSerializerImpl to emit a BOM for UTF16/32.

chrome/browser/download/save_page_browsertest.cc

Index: chrome/browser/download/save_page_browsertest.cc
diff --git a/chrome/browser/download/save_page_browsertest.cc b/chrome/browser/download/save_page_browsertest.cc
index 67ba540264c7f25e972b1f065888092133f07788..2672ff8c3e947d59380f61ddf8f086a5a29a0acc 100644
--- a/chrome/browser/download/save_page_browsertest.cc
+++ b/chrome/browser/download/save_page_browsertest.cc
@@ -547,7 +547,7 @@ IN_PROC_BROWSER_TEST_F(SavePageBrowserTest, DISABLED_SaveCompleteHTML) {
 
   EXPECT_TRUE(base::PathExists(full_file_name));
   EXPECT_TRUE(base::PathExists(dir));
-  EXPECT_TRUE(base::TextContentsEqual(
+  EXPECT_TRUE(base::ContentsEqual(
       test_dir_.Append(base::FilePath(kTestDir)).AppendASCII("b.saved1.htm"),
       full_file_name));
   EXPECT_TRUE(base::ContentsEqual(
@@ -558,6 +558,34 @@ IN_PROC_BROWSER_TEST_F(SavePageBrowserTest, DISABLED_SaveCompleteHTML) {
       dir.AppendASCII("1.css")));
 }
 
+// Disabled on Windows due to flakiness. http://crbug.com/162323

[Randy Smith (Not in Mondays), 2015/10/15 18:57:06]

This is a new test; have you confirmed that it's flaky?  Any possibility of
fixing it?  I wince at landing tests in pre-flaky/disabled fashion, though if
it's in some test infrastructure you have nothing to do with it's quite
reasonable to not fix it.

[Łukasz Anforowicz, 2015/10/20 21:08:43]

Done.

Also following up with the following CLs:
- https://codereview.chromium.org/1408993003/ (landed)
- https://codereview.chromium.org/1408393004/ (landed)
- https://codereview.chromium.org/1413763008/ (in CR)

+#if defined(OS_WIN)
+#define MAYBE_SaveUTF32CompleteHTML DISABLED_SaveUTF32CompleteHTML
+#else
+#define MAYBE_SaveUTF32CompleteHTML SaveUTF32CompleteHTML
+#endif
+IN_PROC_BROWSER_TEST_F(SavePageBrowserTest, MAYBE_SaveUTF32CompleteHTML) {
+  GURL url = NavigateToMockURL("utf32");
+  ui_test_utils::NavigateToURL(browser(), url);
+
+  base::FilePath full_file_name, dir;
+  SaveCurrentTab(url, content::SAVE_PAGE_TYPE_AS_COMPLETE_HTML, "utf32", 3,
+                 &dir, &full_file_name);
+  ASSERT_FALSE(HasFailure());
+
+  EXPECT_TRUE(base::PathExists(full_file_name));
+  EXPECT_TRUE(base::PathExists(dir));
+  EXPECT_TRUE(base::ContentsEqual(

[Randy Smith (Not in Mondays), 2015/10/15 18:57:06]

I wince a bit at this, since it tests the implementation rather than the
behavior we care about (I could imagine many character level changes that
wouldn't effect the web page viewed by the user after saving)--is there any way
you can put something in the original file that would be messed up if the
encoding was messed up, but would always be saved correctly if it wasn't?

[Łukasz Anforowicz, 2015/10/20 21:08:43]

I still think that comparing the whole file contents is desirable here:

- This captures encoding of the whole page (which is important if in the future
we decide to split encoding across 2 separate pieces of code as the tentative
crrev.com/1373573002 is doing).  Alternative here would be to foresee which
pieces might be encoded separately and add verification for each such piece
flavor.

- This captures BOM behavior.

- Avoiding whole-file-comparison in the new test doesn't help that much without
also avoiding whole-file-comparison in the 2 old tests.  And if somebody is
replacing the other 2 expected contents, then in the same go he/she can retain
and replace the file generated in the new test.

- There is a trade-off between robustness of verification and effort required to
update the tests.  I think the effort is not that big, and I think there is a
significant value in making sure that all unexpected changes are detected by the
test (i.e. I don't worry about overtesting at browser-test level as much as I
would at unit-test level).


If you feel strongly about this, then maybe we can track this as a separate bug,
that also covers the 2 old tests that currently use whole file comparison. 
WDYT?

+      test_dir_.Append(base::FilePath(kTestDir)).AppendASCII("utf32.saved.htm"),
+      full_file_name));
+  EXPECT_TRUE(base::ContentsEqual(
+      test_dir_.Append(base::FilePath(kTestDir)).AppendASCII("1.png"),
+      dir.AppendASCII("1.png")));
+  EXPECT_TRUE(base::ContentsEqual(
+      test_dir_.Append(base::FilePath(kTestDir)).AppendASCII("1.css"),
+      dir.AppendASCII("1.css")));
+}
+
 // Invoke a save page during the initial navigation.
 // (Regression test for http://crbug.com/156538).
 // Disabled on Windows due to flakiness. http://crbug.com/162323
@@ -638,7 +666,7 @@ IN_PROC_BROWSER_TEST_F(SavePageBrowserTest, DISABLED_FileNameFromPageTitle) {
 
   EXPECT_TRUE(base::PathExists(full_file_name));
   EXPECT_TRUE(base::PathExists(dir));
-  EXPECT_TRUE(base::TextContentsEqual(
+  EXPECT_TRUE(base::ContentsEqual(
       test_dir_.Append(base::FilePath(kTestDir)).AppendASCII("b.saved2.htm"),
       full_file_name));
   EXPECT_TRUE(base::ContentsEqual(