Fix SavePageAsMHTMLBrowserTest

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/file_path.h"
 #include "base/file_util.h"
 #include "base/path_service.h"
@@ skipping 137 matching lines @@
 
     DownloadPersistentStoreInfoMatch(const GURL& url,
                                      const FilePath& path,
                                      int64 num_files)
       : url_(url),
         path_(path),
         num_files_(num_files) {
     }
 
     bool operator() (const DownloadPersistentStoreInfo& info) const {
-      return info.url == url_ &&
-        info.path == path_ &&
-        // For save packages, received bytes is actually the number of files.
-        info.received_bytes == num_files_ &&
-        info.total_bytes == 0 &&
-        info.state == DownloadItem::COMPLETE;
+      return ((info.url == url_) &&

[Randy Smith (Not in Mondays), 2012/05/14 17:55:19]

My personal feeling is that parens around the == is overkill, but it's up to
you.

[benjhayden, 2012/05/14 19:27:37]

My personal feeling is that ungrouped conditions are living dangerously, but
Done.

+              (info.path == path_) &&
+              // For non-MHTML save packages, received_bytes is actually the
+              // number of files.

[Randy Smith (Not in Mondays), 2012/05/14 17:55:19]

Instead of just putting in an off switch, can you replumb this path so that
info.received_bytes is called something like "progress_count" and then pass in
either the expected number of files or the expected number of bytes depending on
the test?  

[benjhayden, 2012/05/14 19:27:37]

Renaming the variable won't fix the problem.
The problem is that SavePageAsMHTML doesn't know the expected number of bytes.
It could be 2759 or it could be 2760. It probably won't be a different number
until the year 10,000, but I'm not sure.
I could make it calculate the expected number of bytes based on the date, but
that seems a little silly. Or perhaps it just highlights the existing silliness.
I'd like to be able to set a flag to disable the date header in the mhtml
generator (I did something like this in video), but that's probably an
"abstraction violation".
I'd like to parse the generated mhtml and expect some of the headers and a hash
of the body, but that seems too heavy. I did something like this in video
("video_digest"), but it was many-purposed, whereas such a parser here would be
for this one test.

+              ((num_files_ < 0) ||
+               (info.received_bytes == num_files_)) &&
+              (info.total_bytes == 0) &&
+              (info.state == DownloadItem::COMPLETE));
     }
 
     GURL url_;
     FilePath path_;
     int64 num_files_;
   };
 
   void CheckDownloadHistory(const GURL& url,
                             const FilePath& path,
                             int64 num_files) {
@@ skipping 207 matching lines @@
     command_line->AppendSwitch(switches::kSavePageAsMHTML);
   }
 
  private:
   DISALLOW_COPY_AND_ASSIGN(SavePageAsMHTMLBrowserTest);
 };
 
 SavePageAsMHTMLBrowserTest::~SavePageAsMHTMLBrowserTest() {
 }
 
-// Bug 127527: This test fails when the day of the month is >9.
-IN_PROC_BROWSER_TEST_F(SavePageAsMHTMLBrowserTest, DISABLED_SavePageAsMHTML) {
-  static const int64 kFileSize = 2759;
+IN_PROC_BROWSER_TEST_F(SavePageAsMHTMLBrowserTest, SavePageAsMHTML) {
   GURL url = NavigateToMockURL("b");
   FilePath download_dir = DownloadPrefs::FromDownloadManager(
       GetDownloadManager())->download_path();
   FilePath full_file_name = download_dir.AppendASCII(std::string(
       "Test page for saving page feature.mhtml"));
 #if defined(OS_CHROMEOS)
   SavePackageFilePickerChromeOS::SetShouldPromptUser(false);
 #else
   SavePackageFilePicker::SetShouldPromptUser(false);
 #endif
   ui_test_utils::WindowedNotificationObserver observer(
         content::NOTIFICATION_SAVE_PACKAGE_SUCCESSFULLY_FINISHED,
         content::NotificationService::AllSources());
   browser()->SavePage();
   observer.Wait();
-  CheckDownloadHistory(url, full_file_name, kFileSize);
+  CheckDownloadHistory(url, full_file_name, -1);
 
   EXPECT_TRUE(file_util::PathExists(full_file_name));
   int64 actual_file_size = -1;
   EXPECT_TRUE(file_util::GetFileSize(full_file_name, &actual_file_size));
-  EXPECT_EQ(kFileSize, actual_file_size);
+  EXPECT_LE(2758, actual_file_size);

[Randy Smith (Not in Mondays), 2012/05/14 17:55:19]

I dislike this type of naked constant.  Is there some way to derive this value
(probe the file system for the files we're actually saving)?  If not, I do think
we should keep it as a defined constant.

[benjhayden, 2012/05/14 19:27:37]

Done.

 }