Prefer opening PDF downloads in the browser.

chrome/browser/download/chrome_download_manager_delegate.cc

Index: chrome/browser/download/chrome_download_manager_delegate.cc
diff --git a/chrome/browser/download/chrome_download_manager_delegate.cc b/chrome/browser/download/chrome_download_manager_delegate.cc
index 0e32cdfae90f971ee66965a55f84482e2bc607dc..5aefb20ff66be64a49c671e7143451784b129c43 100644
--- a/chrome/browser/download/chrome_download_manager_delegate.cc
+++ b/chrome/browser/download/chrome_download_manager_delegate.cc
@@ -6,6 +6,7 @@
 
 #include <string>
 
+#include "base/basictypes.h"
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/callback.h"
@@ -22,6 +23,7 @@
 #include "chrome/browser/download/download_crx_util.h"
 #include "chrome/browser/download/download_file_picker.h"
 #include "chrome/browser/download/download_history.h"
+#include "chrome/browser/download/download_item_model.h"
 #include "chrome/browser/download/download_path_reservation_tracker.h"
 #include "chrome/browser/download/download_prefs.h"
 #include "chrome/browser/download/download_service.h"
@@ -33,13 +35,18 @@
 #include "chrome/browser/platform_util.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/safe_browsing/safe_browsing_service.h"
+#include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/browser_finder.h"
 #include "chrome/common/chrome_constants.h"
 #include "chrome/common/pref_names.h"
 #include "components/user_prefs/pref_registry_syncable.h"
 #include "content/public/browser/download_item.h"
 #include "content/public/browser/download_manager.h"
 #include "content/public/browser/notification_source.h"
+#include "content/public/browser/page_navigator.h"
 #include "extensions/common/constants.h"
+#include "net/base/mime_util.h"
+#include "net/base/net_util.h"
 
 #if defined(OS_CHROMEOS)
 #include "chrome/browser/chromeos/drive/download_handler.h"
@@ -161,6 +168,28 @@ void CheckDownloadUrlDone(
 
 #endif  // FULL_SAFE_BROWSING
 
+// Called on the blocking pool to determine the MIME type for |path|.
+void GetMimeTypeAndReplyOnUIThread(
+    const base::FilePath& path,
+    const base::Callback<void(const std::string&)>& callback) {

[Randy Smith (Not in Mondays), 2013/11/04 19:53:26]

Worth an assertion that we're on the IO thread?

[asanka, 2013/11/04 21:20:11]

This is actually called on the blocking pool. The only requirement is that
GetMimeTypeFromFile be allowed to block on IO if the system MIME associations
have to be looked up. This is asserted with an 
base::ThreadRestrictions::AssertIOAllowed() deep in the guts of
GetMimeTypeFromFile(). 

+  std::string mime_type;
+  net::GetMimeTypeFromFile(path, &mime_type);
+  BrowserThread::PostTask(
+      BrowserThread::UI, FROM_HERE, base::Bind(callback, mime_type));
+}
+
+bool IsOpenInBrowserPreferreredForFile(const base::FilePath& path,
+                                       const std::string& mime_type) {
+  if (net::IsSupportedImageMimeType(mime_type))
+    return true;
+
+#if ENABLE_PLUGINS
+  if (path.MatchesExtension(FILE_PATH_LITERAL(".pdf")))

[Randy Smith (Not in Mondays), 2013/11/04 19:53:26]

Doesn't this duplicate the plugin information as to what plugins handle what
mime-types/file types?  Also, won't this break if the user has specifically
disabled the PDF plugin?

[asanka, 2013/11/04 21:20:11]

I'll fiddle with the names to make this clearer, but there are several inputs
into the decision:

- Whether the browser is capable of handling the file type securely: This is
determined in DownloadTargetDeterminer and takes into consideration whether any
sandboxed plugins are available and enabled to handle the file type.

- Whether we want to handle this file in the browser if it is possible to do so:
Not every file that can be handled in the browser should be handled. At least
for now. This decision is made here in IsOpenInBrowserPreferredForFile().

A file will default to being opened in the browser if both of the above are
true.

+    return true;
+#endif
+  return false;
+}
+
 }  // namespace
 
 ChromeDownloadManagerDelegate::ChromeDownloadManagerDelegate(Profile* profile)
@@ -220,13 +249,17 @@ void ChromeDownloadManagerDelegate::ReturnNextId(
 bool ChromeDownloadManagerDelegate::DetermineDownloadTarget(
     DownloadItem* download,
     const content::DownloadTargetCallback& callback) {
+  DownloadTargetDeterminer::CompletionCallback target_determined_callback =
+      base::Bind(&ChromeDownloadManagerDelegate::OnDownloadTargetDetermined,
+                 this,
+                 download->GetId(),
+                 callback);
   DownloadTargetDeterminer::Start(
       download,
-      GetPlatformDownloadPath(
-          profile_, download, PLATFORM_TARGET_PATH),
+      GetPlatformDownloadPath(profile_, download, PLATFORM_TARGET_PATH),
       download_prefs_.get(),
       this,
-      callback);
+      target_determined_callback);
   return true;
 }
 
@@ -371,16 +404,45 @@ void ChromeDownloadManagerDelegate::ChooseSavePath(
       callback);
 }
 
-void ChromeDownloadManagerDelegate::OpenDownload(DownloadItem* download) {
-  DCHECK_EQ(DownloadItem::COMPLETE, download->GetState());
-  if (!download->CanOpenDownload())
-    return;
+void ChromeDownloadManagerDelegate::OpenDownloadUsingPlatformHandler(
+    DownloadItem* download) {
   base::FilePath platform_path(
       GetPlatformDownloadPath(profile_, download, PLATFORM_TARGET_PATH));
   DCHECK(!platform_path.empty());
   platform_util::OpenItem(platform_path);
 }
 
+void ChromeDownloadManagerDelegate::OpenDownload(DownloadItem* download) {
+  DCHECK_EQ(DownloadItem::COMPLETE, download->GetState());
+  if (!download->CanOpenDownload())
+    return;
+#if !defined(OS_ANDROID)

[Randy Smith (Not in Mondays), 2013/11/04 19:53:26]

Is there a reason not to hide the ifdef in the ShouldPreferOpeningInBrowser()
logic?

[asanka, 2013/11/04 21:20:11]

Moved to IsOpenInBrowserPreferredForFile.

+  if (DownloadItemModel(download).ShouldPreferOpeningInBrowser()) {

[Randy Smith (Not in Mondays), 2013/11/04 19:53:26]

Suggestion: I'm tempted to suggest reversing the text and code inclusion (i.e.
making the OpenDownloadUsingPlatformHandler() the if dependency).  That goes
(somewhat) against the "common path should be outside of if" guideline, but it
will result in a lot less indented code, which might make the code a bit easier
to read.  Not sure what's best here, so just raising the idea.

[asanka, 2013/11/04 21:20:11]

Done.

+    content::WebContents* web_contents = download->GetWebContents();
+    Browser* browser =
+        web_contents ? chrome::FindBrowserWithWebContents(web_contents) : NULL;
+    if (!browser ||
+        !browser->CanSupportWindowFeature(Browser::FEATURE_TABSTRIP))
+      browser = chrome::FindTabbedBrowser(
+          profile_, true, chrome::GetActiveDesktop());
+    if (!browser)
+      browser = chrome::FindOrCreateTabbedBrowser(
+          profile_, chrome::GetActiveDesktop());
+    if (browser) {
+      content::OpenURLParams params(
+          net::FilePathToFileURL(download->GetTargetFilePath()),
+          content::Referrer(),
+          NEW_FOREGROUND_TAB,
+          content::PAGE_TRANSITION_LINK,
+          false);
+      browser->OpenURL(params);
+      return;
+    }
+  }
+#endif
+  OpenDownloadUsingPlatformHandler(download);
+}
+
 void ChromeDownloadManagerDelegate::ShowDownloadInShell(
     DownloadItem* download) {
   if (!download->CanShowInFolder())
@@ -523,6 +585,14 @@ void ChromeDownloadManagerDelegate::CheckDownloadUrl(
   callback.Run(content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS);
 }
 
+void ChromeDownloadManagerDelegate::GetFileMimeType(
+    const base::FilePath& path,
+    const GetFileMimeTypeCallback& callback) {
+  BrowserThread::PostBlockingPoolTask(
+      FROM_HERE,
+      base::Bind(&GetMimeTypeAndReplyOnUIThread, path, callback));
+}
+
 #if defined(FULL_SAFE_BROWSING)
 void ChromeDownloadManagerDelegate::CheckClientDownloadDone(
     uint32 download_id,
@@ -586,3 +656,20 @@ void ChromeDownloadManagerDelegate::Observe(
   crx_installers_.erase(installer.get());
   callback.Run(installer->did_handle_successfully());
 }
+
+void ChromeDownloadManagerDelegate::OnDownloadTargetDetermined(
+    int32 download_id,
+    const content::DownloadTargetCallback& callback,
+    scoped_ptr<DownloadTargetInfo> target_info) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  DownloadItem* item = download_manager_->GetDownload(download_id);
+  if (!target_info->target_path.empty() && item &&
+      IsOpenInBrowserPreferreredForFile(target_info->target_path,
+                                        target_info->mime_type) &&
+      target_info->is_filetype_handled_securely)
+    DownloadItemModel(item).SetShouldPreferOpeningInBrowser(true);
+  callback.Run(target_info->target_path,
+               target_info->target_disposition,
+               target_info->danger_type,
+               target_info->intermediate_path);
+}