Remove "recursive" parameter from WebPageSerializer::serialize.

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 4a71ff5a825f030d7e34ca9438067d1f7eb055a6..4412c35d8526bc6621dd2785174d07275e877dd1 100644
--- a/chrome/browser/download/save_page_browsertest.cc
+++ b/chrome/browser/download/save_page_browsertest.cc
@@ -322,26 +322,24 @@ class SavePageBrowserTest : public InProcessBrowserTest {
 
   // Returns true if and when there was a single download created, and its url
   // is |expected_url|.
-  bool VerifySavePackageExpectations(
-      Browser* browser,
-      const GURL& expected_url) const {
-    // Generally, there should only be one download item created
-    // in all of these tests.  If it's already here, grab it; if not,
-    // wait for it to show up.
-    std::vector<DownloadItem*> items;
-    DownloadManager* manager(
-        BrowserContext::GetDownloadManager(browser->profile()));
-    manager->GetAllDownloads(&items);
-    if (items.size() == 0u) {
-      DownloadItemCreatedObserver(manager).WaitForDownloadItem(&items);
-    }
-
-    EXPECT_EQ(1u, items.size());
-    if (1u != items.size())
-      return false;
-    DownloadItem* download_item(items[0]);
-
-    return (expected_url == download_item->GetOriginalUrl());
+  void WaitForDownloadItemByUrl(Browser* browser,
+                                const GURL& expected_url) const {
+    bool found;
+    do {
+      DownloadManager* manager(
+          BrowserContext::GetDownloadManager(browser->profile()));
+
+      std::vector<DownloadItem*> items;
+      manager->GetAllDownloads(&items);
+
+      found = std::any_of(items.begin(), items.end(),

[dcheng, 2015/09/25 19:53:52]

C++11 library features are still banned, aren't they?

[Łukasz Anforowicz, 2015/09/25 20:35:57]

Thanks for catching this.  OTOH, based on the other piece of feedback, I will
revert VerifySavePackageExpectation changes.

+                          [&expected_url](DownloadItem* item) {
+                            return item->GetOriginalUrl() == expected_url;
+                          });
+      if (!found) {
+        DownloadItemCreatedObserver(manager).WaitForDownloadItem(&items);

[dcheng, 2015/09/25 19:53:52]

Hmm... maybe we should have a way to specify how many downloads we're expecting
/ what URLs? Otherwise, it seems like it could be non-deterministic how many
actual download items show up here, right?

[Łukasz Anforowicz, 2015/09/25 20:35:57]

Based on the other piece of feedback, I will revert VerifySavePackageExpectation
changes
and dismiss the extra download item before kicking off save-page-as (see the
changes [vs previous patchset] in the body of the new test).

+      }
+    } while (!found);
   }
 
   void SaveCurrentTab(const GURL& url,
@@ -362,7 +360,7 @@ class SavePageBrowserTest : public InProcessBrowserTest {
     ASSERT_TRUE(GetCurrentTab(browser())
                     ->SavePage(*main_file_name, *output_dir, save_page_type));
     run_loop.Run();
-    ASSERT_TRUE(VerifySavePackageExpectations(browser(), url));
+    WaitForDownloadItemByUrl(browser(), url);
     persisted.WaitForPersisted();
   }
 
@@ -593,7 +591,7 @@ IN_PROC_BROWSER_TEST_F(SavePageBrowserTest,
       full_file_name, dir, content::SAVE_PAGE_TYPE_AS_COMPLETE_HTML));
 
   loop_runner->Run();
-  ASSERT_TRUE(VerifySavePackageExpectations(incognito, url));
+  WaitForDownloadItemByUrl(incognito, url);
 
   // We can't check more than this because SavePackage is racing with
   // the page load.  If the page load won the race, then SavePackage
@@ -632,7 +630,7 @@ IN_PROC_BROWSER_TEST_F(SavePageBrowserTest, DISABLED_FileNameFromPageTitle) {
       full_file_name, dir, content::SAVE_PAGE_TYPE_AS_COMPLETE_HTML));
 
   loop_runner->Run();
-  ASSERT_TRUE(VerifySavePackageExpectations(browser(), url));
+  WaitForDownloadItemByUrl(browser(), url);
   persisted.WaitForPersisted();
 
   EXPECT_TRUE(base::PathExists(full_file_name));
@@ -745,7 +743,7 @@ IN_PROC_BROWSER_TEST_F(SavePageAsMHTMLBrowserTest, SavePageAsMHTML) {
       loop_runner->QuitClosure());
   chrome::SavePage(browser());
   loop_runner->Run();
-  ASSERT_TRUE(VerifySavePackageExpectations(browser(), url));
+  WaitForDownloadItemByUrl(browser(), url);
   persisted.WaitForPersisted();
 
   ASSERT_TRUE(base::PathExists(full_file_name));
@@ -774,6 +772,28 @@ IN_PROC_BROWSER_TEST_F(SavePageBrowserTest, SavePageBrowserTest_NonMHTML) {
   EXPECT_EQ("foo", contents);
 }
 
+// Disabled on Windows due to flakiness. http://crbug.com/162323
+#if defined(OS_WIN)
+#define MAYBE_SaveDownloadableIFrame DISABLED_SaveDownloadableIFrame
+#else
+#define MAYBE_SaveDownloadableIFrame SaveDownloadableIFrame
+#endif
+// Test that we don't crash when the page contains an iframe that
+// was handled as a download (http://crbug.com/42212).
+IN_PROC_BROWSER_TEST_F(SavePageBrowserTest, MAYBE_SaveDownloadableIFrame) {

[dcheng, 2015/09/25 19:53:52]

The original test wasn't marked as flaky, but this one is. What's the
difference?

[Łukasz Anforowicz, 2015/09/25 20:35:57]

This one interacts with the download manager (the other one was directly calling
WebPageSerializer).  I copied flakiness designation from another test in
save_page_browsertest.cc.

+  GURL url = URLRequestMockHTTPJob::GetMockUrl(
+      base::FilePath(FILE_PATH_LITERAL("downloads"))
+          .AppendASCII("iframe-src-is-a-download.htm"));
+  ui_test_utils::NavigateToURL(browser(), url);
+
+  base::FilePath full_file_name, dir;
+  SaveCurrentTab(url, content::SAVE_PAGE_TYPE_AS_COMPLETE_HTML,
+                 "iframe-src-is-a-download", 2, &dir, &full_file_name);
+  ASSERT_FALSE(HasFailure());
+
+  EXPECT_TRUE(base::PathExists(full_file_name));
+}
+
 class SavePageSitePerProcessBrowserTest : public SavePageBrowserTest {
  public:
   SavePageSitePerProcessBrowserTest() {}