Make the logic for stream interception by MimeHandlerViews consistent with starting the plugin

chrome/browser/pdf/pdf_extension_test.cc

Index: chrome/browser/pdf/pdf_extension_test.cc
diff --git a/chrome/browser/pdf/pdf_extension_test.cc b/chrome/browser/pdf/pdf_extension_test.cc
index fb1b65b71cbb0020abb7bd2f41496dea6e0a362d..723814e0b5072381056ffcbaddf186d86048bdc2 100644
--- a/chrome/browser/pdf/pdf_extension_test.cc
+++ b/chrome/browser/pdf/pdf_extension_test.cc
@@ -7,14 +7,18 @@
 #include "base/files/file_util.h"
 #include "base/hash.h"
 #include "base/path_service.h"
+#include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/extensions/component_loader.h"
 #include "chrome/browser/extensions/extension_apitest.h"
 #include "chrome/browser/extensions/extension_service.h"
+#include "chrome/browser/plugins/plugin_prefs.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
+#include "chrome/common/chrome_content_client.h"
 #include "chrome/common/chrome_paths.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/test/base/ui_test_utils.h"
 #include "content/public/browser/browser_plugin_guest_manager.h"
+#include "content/public/browser/download_manager.h"
 #include "content/public/browser/plugin_service.h"
 #include "content/public/test/browser_test_utils.h"
 #include "extensions/browser/extension_registry.h"
@@ -164,6 +168,58 @@ IN_PROC_BROWSER_TEST_P(PDFExtensionTest, Load) {
   LoadAllPdfsTest("pdf", GetParam());
 }
 
+class TestDownloadObserver : public content::DownloadManager::Observer {
+ public:
+  TestDownloadObserver() {}
+  virtual ~TestDownloadObserver() {}
+
+  GURL GetAndClearLastUrl() {
+    GURL result = url_;
+    url_ = GURL();

[mmenke, 2015/06/11 19:38:04]

Is there a reason we need to clear the URL?  Should only start one download,
right?

[raymes, 2015/07/01 05:13:18]

Done.

+    return result;
+  }
+
+  void OnDownloadCreated(content::DownloadManager* manager,

[mmenke, 2015/06/11 19:38:04]

Maybe add something like:

// content::DownloadManager::Observer implementation

[raymes, 2015/07/01 05:13:17]

Done.

+                         content::DownloadItem* item) override {

[mmenke, 2015/06/11 19:38:04]

Should include download_item.h

[raymes, 2015/07/01 05:13:18]

Done.

+    CHECK(url_.is_empty());
+    url_ = item->GetURL();
+    CHECK(!url_.is_empty());

[mmenke, 2015/06/11 19:38:03]

Need to include base/logging.h for CHECK, and url/gurl.h for GURL

[raymes, 2015/07/01 05:13:18]

Done.

+  }
+
+ private:
+  GURL url_;
+};
+
+IN_PROC_BROWSER_TEST_F(PDFExtensionTest, DisablePlugin) {
+  // Disable the PDF plugin.
+  content::WebContents* web_contents =
+      browser()->tab_strip_model()->GetActiveWebContents();

[mmenke, 2015/06/11 19:38:03]

Need to include the browser and tab string model headers

[raymes, 2015/07/01 05:13:18]

Done.

+  content::BrowserContext* browser_context = web_contents->GetBrowserContext();

[mmenke, 2015/06/11 19:38:03]

Need the web_contents header.

[raymes, 2015/07/01 05:13:18]

Done.

+  Profile* profile = Profile::FromBrowserContext(browser_context);

[mmenke, 2015/06/11 19:38:04]

Need to include the profile header.

[raymes, 2015/07/01 05:13:18]

Done.

+  scoped_refptr<PluginPrefs> prefs(PluginPrefs::GetForProfile(profile));

[mmenke, 2015/06/11 19:38:03]

Include ref_counted.h?

[raymes, 2015/07/01 05:13:18]

Done.

+  ASSERT_TRUE(prefs.get());
+  prefs->EnablePluginGroup(false,
+      base::UTF8ToUTF16(ChromeContentClient::kPDFPluginName));
+
+  // Register a download observer.
+  content::DownloadManager* download_manager =
+      content::BrowserContext::GetDownloadManager(browser_context);
+  TestDownloadObserver download_observer;
+  download_manager->AddObserver(&download_observer);
+
+  // Navigate to a PDF and test that it is downloaded.
+  GURL url(embedded_test_server()->GetURL("/pdf/test.pdf"));
+  ui_test_utils::NavigateToURL(browser(), url);
+  ASSERT_EQ(url, download_observer.GetAndClearLastUrl());

[mmenke, 2015/06/11 19:38:03]

I'm not sufficiently familiar with navigation/download code to verify this is
non-racy.  Could we add a RunLoop to make GetAndClearLastUrl wait as well as get
the URL?  Otherwise, someone else should probably review this.

[raymes, 2015/07/01 05:13:18]

The comment in NavigateToURL says:
// Navigates the selected tab of |browser| to |url|, blocking until the
navigation finishes

+
+  // Cancel the download to shutdown cleanly.
+  download_manager->RemoveObserver(&download_observer);
+  std::vector<content::DownloadItem*> downloads;

[mmenke, 2015/06/11 19:38:03]

include <vector>

[raymes, 2015/07/01 05:13:18]

Done.

+  download_manager->GetAllDownloads(&downloads);
+  ASSERT_EQ(1u, downloads.size());
+  downloads[0]->Cancel(false);
+}
+
 // We break PDFTest.Load up into kNumberLoadTestParts.
 INSTANTIATE_TEST_CASE_P(PDFTestFiles,
                         PDFExtensionTest,