Tweaking WebPageSerializerImpl to emit a BOM for UTF16/32.

chrome/browser/download/save_page_browsertest.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/command_line.h"
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
 #include "base/files/scoped_temp_dir.h"
@@ skipping 529 matching lines @@
 IN_PROC_BROWSER_TEST_F(SavePageBrowserTest, DISABLED_SaveCompleteHTML) {
   GURL url = NavigateToMockURL("b");
 
   base::FilePath full_file_name, dir;
   SaveCurrentTab(url, content::SAVE_PAGE_TYPE_AS_COMPLETE_HTML, "b", 3, &dir,
                  &full_file_name);
   ASSERT_FALSE(HasFailure());
 
   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(
       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")));
 }
 
+// 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
 #if defined(OS_WIN)
 #define MAYBE_SaveDuringInitialNavigationIncognito DISABLED_SaveDuringInitialNavigationIncognito
 #else
 #define MAYBE_SaveDuringInitialNavigationIncognito SaveDuringInitialNavigationIncognito
 #endif
 IN_PROC_BROWSER_TEST_F(SavePageBrowserTest,
                        MAYBE_SaveDuringInitialNavigationIncognito) {
@@ skipping 60 matching lines @@
       loop_runner->QuitClosure());
   ASSERT_TRUE(GetCurrentTab(browser())->SavePage(
       full_file_name, dir, content::SAVE_PAGE_TYPE_AS_COMPLETE_HTML));
 
   loop_runner->Run();
   ASSERT_TRUE(VerifySavePackageExpectations(browser(), url));
   persisted.WaitForPersisted();
 
   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(
       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")));
 }
 
@@ skipping 214 matching lines @@
   ASSERT_TRUE(base::ReadFileToString(full_file_name, &main_contents));
   EXPECT_THAT(main_contents,
               HasSubstr("<iframe src=\"./iframes_files/a.html\"></iframe>"));
   EXPECT_THAT(main_contents,
               HasSubstr("<iframe src=\"./iframes_files/b.html\"></iframe>"));
   EXPECT_THAT(main_contents,
               HasSubstr("<img src=\"./iframes_files/1.png\">"));
 }
 
 }  // namespace