Deflakify DownloadExtensionTest by not depending on the icon loader

chrome/browser/extensions/api/downloads/downloads_api_unittest.cc

Index: chrome/browser/extensions/api/downloads/downloads_api_unittest.cc
diff --git a/chrome/browser/extensions/api/downloads/downloads_api_unittest.cc b/chrome/browser/extensions/api/downloads/downloads_api_unittest.cc
index f81e73680ee5ba76eff1f865ef64db78b69beed6..3ba7d64f0dfda94e613aaf1cffa6eca3cd34e743 100644
--- a/chrome/browser/extensions/api/downloads/downloads_api_unittest.cc
+++ b/chrome/browser/extensions/api/downloads/downloads_api_unittest.cc
@@ -45,7 +45,6 @@
 #include "net/url_request/url_request_context.h"
 #include "net/url_request/url_request_job.h"
 #include "net/url_request/url_request_job_factory.h"
-#include "ui/gfx/codec/png_codec.h"
 #include "webkit/blob/blob_data.h"
 #include "webkit/blob/blob_storage_controller.h"
 #include "webkit/blob/blob_url_request_job.h"
@@ -507,29 +506,6 @@ class DownloadExtensionTest : public ExtensionApiTest {
     return base::StringPrintf("[%d]", download_item->GetId());
   }
 
-  // Checks if a data URL encoded image is a PNG of a given size.
-  void ExpectDataURLIsPNGWithSize(const std::string& url, int expected_size) {
-    std::string mime_type;
-    std::string charset;
-    std::string data;
-    EXPECT_FALSE(url.empty());
-    EXPECT_TRUE(net::DataURL::Parse(GURL(url), &mime_type, &charset, &data));
-    EXPECT_STREQ("image/png", mime_type.c_str());
-    EXPECT_FALSE(data.empty());
-
-    if (data.empty())
-      return;
-
-    int width = -1, height = -1;
-    std::vector<unsigned char> decoded_data;
-    EXPECT_TRUE(gfx::PNGCodec::Decode(
-        reinterpret_cast<const unsigned char*>(data.c_str()), data.length(),
-        gfx::PNGCodec::FORMAT_RGBA, &decoded_data,
-        &width, &height));
-    EXPECT_EQ(expected_size, width);
-    EXPECT_EQ(expected_size, height);
-  }
-
   const FilePath& downloads_directory() {
     return downloads_directory_.path();
   }
@@ -897,6 +873,17 @@ IN_PROC_BROWSER_TEST_F(
                error.c_str());
 }
 
+UIThreadExtensionFunction* MockedGetFileIconFunction(
+    const FilePath& expected_path,
+    IconLoader::IconSize icon_size,
+    const std::string& response) {
+  scoped_refptr<DownloadsGetFileIconFunction> function(
+      new DownloadsGetFileIconFunction());
+  function->SetIconExtractorForTesting(new MockIconExtractorImpl(
+      expected_path, icon_size, response));
+  return function.release();
+}
+
 // Test downloads.getFileIcon() on in-progress, finished, cancelled and deleted
 // download items.
 // The test fails under ASan, see http://crbug.com/138211

[asanka, 2012/08/17 20:35:36]

You should re-enable these tests. The failure mode is the same.

[benjhayden, 2012/08/17 21:35:57]

Done.

@@ -911,91 +898,68 @@ IN_PROC_BROWSER_TEST_F(DownloadExtensionTest,
     MAYBE_DownloadExtensionTest_FileIcon_Active) {
   DownloadItem* download_item = CreateSlowTestDownload();
   ASSERT_TRUE(download_item);
+  std::string args32(base::StringPrintf("[%d, {\"size\": 32}]",
+                     download_item->GetId()));
+  std::string result_string;
 
   // Get the icon for the in-progress download.  This call should succeed even
   // if the file type isn't registered.
-  std::string args = base::StringPrintf("[%d, {}]", download_item->GetId());
-  std::string result_string;
-  EXPECT_TRUE(RunFunctionAndReturnString(new DownloadsGetFileIconFunction(),
-                                         args, &result_string));
-
-  // Note: We are checking if the result is a Data URL that has encoded
-  // image/png data for an icon of a specific size. Of these, only the icon size
-  // is specified in the API contract. The image type (image/png) and URL type
-  // (Data) are implementation details.
-
-  // The default size for icons returned by getFileIcon() is 32x32.
-  ExpectDataURLIsPNGWithSize(result_string, 32);
-
   // Test whether the correct path is being pased into the icon extractor.
-  FilePath expected_path(download_item->GetTargetFilePath());
-  scoped_refptr<DownloadsGetFileIconFunction> function(
-      new DownloadsGetFileIconFunction());
-  function->SetIconExtractorForTesting(new MockIconExtractorImpl(
-      expected_path, IconLoader::NORMAL, "foo"));
-  EXPECT_TRUE(RunFunctionAndReturnString(function.release(), args,
-                                         &result_string));
+  EXPECT_TRUE(RunFunctionAndReturnString(MockedGetFileIconFunction(
+          download_item->GetTargetFilePath(), IconLoader::NORMAL, "foo"),
+      base::StringPrintf("[%d, {}]", download_item->GetId()), &result_string));
 
   // Now try a 16x16 icon.
-  args = base::StringPrintf("[%d, {\"size\": 16}]", download_item->GetId());
-  EXPECT_TRUE(RunFunctionAndReturnString(new DownloadsGetFileIconFunction(),
-                                         args, &result_string));
-  ExpectDataURLIsPNGWithSize(result_string, 16);
+  EXPECT_TRUE(RunFunctionAndReturnString(MockedGetFileIconFunction(
+          download_item->GetTargetFilePath(), IconLoader::SMALL, "foo"),
+      base::StringPrintf("[%d, {\"size\": 16}]", download_item->GetId()),
+      &result_string));
 
   // Explicitly asking for 32x32 should give us a 32x32 icon.
-  args = base::StringPrintf("[%d, {\"size\": 32}]", download_item->GetId());
-  EXPECT_TRUE(RunFunctionAndReturnString(new DownloadsGetFileIconFunction(),
-                                         args, &result_string));
-  ExpectDataURLIsPNGWithSize(result_string, 32);
+  EXPECT_TRUE(RunFunctionAndReturnString(MockedGetFileIconFunction(
+          download_item->GetTargetFilePath(), IconLoader::NORMAL, "foo"),
+      args32, &result_string));
 
   // Finish the download and try again.
   FinishPendingSlowDownloads();
   EXPECT_EQ(DownloadItem::COMPLETE, download_item->GetState());
-  EXPECT_TRUE(RunFunctionAndReturnString(new DownloadsGetFileIconFunction(),
-                                         args, &result_string));
-  ExpectDataURLIsPNGWithSize(result_string, 32);
+  EXPECT_TRUE(RunFunctionAndReturnString(MockedGetFileIconFunction(
+          download_item->GetTargetFilePath(), IconLoader::NORMAL, "foo"),
+      args32, &result_string));
 
   // Check the path passed to the icon extractor post-completion.
-  function = new DownloadsGetFileIconFunction();
-  function->SetIconExtractorForTesting(new MockIconExtractorImpl(
-      expected_path, IconLoader::NORMAL, "foo"));
-  EXPECT_TRUE(RunFunctionAndReturnString(function.release(), args,
-                                         &result_string));
+  EXPECT_TRUE(RunFunctionAndReturnString(MockedGetFileIconFunction(
+          download_item->GetTargetFilePath(), IconLoader::NORMAL, "foo"),
+      args32, &result_string));
 
   // Now create another download.
   download_item = CreateSlowTestDownload();
+  args32 = base::StringPrintf("[%d, {\"size\": 32}]", download_item->GetId());
   ASSERT_TRUE(download_item);
-  expected_path = download_item->GetTargetFilePath();
 
   // Cancel the download. As long as the download has a target path, we should
   // be able to query the file icon.
   download_item->Cancel(true);
   // Let cleanup complete on the FILE thread.
   content::RunAllPendingInMessageLoop(BrowserThread::FILE);
-  args = base::StringPrintf("[%d, {\"size\": 32}]", download_item->GetId());
-  EXPECT_TRUE(RunFunctionAndReturnString(new DownloadsGetFileIconFunction(),
-                                         args, &result_string));
-  ExpectDataURLIsPNGWithSize(result_string, 32);
-
   // Check the path passed to the icon extractor post-cancellation.
-  function = new DownloadsGetFileIconFunction();
-  function->SetIconExtractorForTesting(new MockIconExtractorImpl(
-      expected_path, IconLoader::NORMAL, "foo"));
-  EXPECT_TRUE(RunFunctionAndReturnString(function.release(), args,
-                                         &result_string));
+  EXPECT_TRUE(RunFunctionAndReturnString(MockedGetFileIconFunction(
+          download_item->GetTargetFilePath(), IconLoader::NORMAL, "foo"),
+      args32,
+      &result_string));
 
   // Simulate an error during icon load by invoking the mock with an empty
   // result string.
-  function = new DownloadsGetFileIconFunction();
-  function->SetIconExtractorForTesting(new MockIconExtractorImpl(
-      expected_path, IconLoader::NORMAL, ""));
-  std::string error = RunFunctionAndReturnError(function.release(), args);
+  std::string error = RunFunctionAndReturnError(MockedGetFileIconFunction(
+        download_item->GetTargetFilePath(), IconLoader::NORMAL, ""),
+      args32);
   EXPECT_STREQ(download_extension_errors::kIconNotFoundError,
                error.c_str());
 
   // Once the download item is deleted, we should return kInvalidOperationError.
   download_item->Delete(DownloadItem::DELETE_DUE_TO_USER_DISCARD);
-  error = RunFunctionAndReturnError(new DownloadsGetFileIconFunction(), args);
+  download_item = NULL;
+  error = RunFunctionAndReturnError(new DownloadsGetFileIconFunction(), args32);
   EXPECT_STREQ(download_extension_errors::kInvalidOperationError,
                error.c_str());
 }
@@ -1036,25 +1000,13 @@ IN_PROC_BROWSER_TEST_F(DownloadExtensionTest,
   for (DownloadManager::DownloadVector::iterator iter = all_downloads.begin();
        iter != all_downloads.end();
        ++iter) {
-    std::string args(base::StringPrintf("[%d, {\"size\": 32}]",
-                                        (*iter)->GetId()));
     std::string result_string;
-    EXPECT_TRUE(RunFunctionAndReturnString(
-        new DownloadsGetFileIconFunction(), args, &result_string));
-    // Note: We are checking if the result is a Data URL that has encoded
-    // image/png data for an icon of a specific size. Of these, only the icon
-    // size is specified in the API contract. The image type (image/png) and URL
-    // type (Data) are implementation details.
-    ExpectDataURLIsPNGWithSize(result_string, 32);
-
     // Use a MockIconExtractorImpl to test if the correct path is being passed
     // into the DownloadFileIconExtractor.
-    scoped_refptr<DownloadsGetFileIconFunction> function(
-        new DownloadsGetFileIconFunction());
-    function->SetIconExtractorForTesting(new MockIconExtractorImpl(
-        (*iter)->GetFullPath(), IconLoader::NORMAL, "hello"));
-    EXPECT_TRUE(RunFunctionAndReturnString(function.release(), args,
-                                           &result_string));
+    EXPECT_TRUE(RunFunctionAndReturnString(MockedGetFileIconFunction(
+            (*iter)->GetFullPath(), IconLoader::NORMAL, "hello"),
+        base::StringPrintf("[%d, {\"size\": 32}]", (*iter)->GetId()),
+        &result_string));
     EXPECT_STREQ("hello", result_string.c_str());
   }
 
@@ -1411,11 +1363,11 @@ IN_PROC_BROWSER_TEST_F(DownloadExtensionTest,
   // Do the FileIcon test for both the on- and off-items while off the record.
   // NOTE(benjhayden): This does not include the FileIcon test from history,
   // just active downloads. This shouldn't be a problem.
-  EXPECT_TRUE(RunFunctionAndReturnString(
-      new DownloadsGetFileIconFunction(),
+  EXPECT_TRUE(RunFunctionAndReturnString(MockedGetFileIconFunction(
+          on_item->GetTargetFilePath(), IconLoader::NORMAL, "foo"),
       base::StringPrintf("[%d, {}]", on_item->GetId()), &result_string));
-  EXPECT_TRUE(RunFunctionAndReturnString(
-      new DownloadsGetFileIconFunction(),
+  EXPECT_TRUE(RunFunctionAndReturnString(MockedGetFileIconFunction(
+          off_item->GetTargetFilePath(), IconLoader::NORMAL, "foo"),
       base::StringPrintf("[%d, {}]", off_item->GetId()), &result_string));
 
   // Do the pause/resume/cancel test for both the on- and off-items while off