Prefer opening PDF downloads in the browser.

chrome/browser/download/download_target_determiner.cc

Index: chrome/browser/download/download_target_determiner.cc
diff --git a/chrome/browser/download/download_target_determiner.cc b/chrome/browser/download/download_target_determiner.cc
index 7291336364e08ed85d6393a12df346948e3e32ae..99ce57b46dc9279e4fbb7324c54ceaf12e0cf531 100644
--- a/chrome/browser/download/download_target_determiner.cc
+++ b/chrome/browser/download/download_target_determiner.cc
@@ -23,9 +23,16 @@
 #include "content/public/browser/download_interrupt_reasons.h"
 #include "extensions/common/constants.h"
 #include "grit/generated_resources.h"
+#include "net/base/mime_util.h"
 #include "net/base/net_util.h"
 #include "ui/base/l10n/l10n_util.h"
 
+#if defined(ENABLE_PLUGINS)
+#include "chrome/browser/plugins/plugin_prefs.h"
+#include "content/public/browser/plugin_service.h"
+#include "content/public/common/webplugininfo.h"
+#endif
+
 using content::BrowserThread;
 using content::DownloadItem;
 
@@ -51,6 +58,11 @@ void VisitCountsToVisitedBefore(
 
 } // namespace
 
+DownloadTargetInfo::DownloadTargetInfo()
+    : is_filetype_handled_securely(false) {}
+
+DownloadTargetInfo::~DownloadTargetInfo() {}
+
 DownloadTargetDeterminerDelegate::~DownloadTargetDeterminerDelegate() {
 }
 
@@ -59,7 +71,7 @@ DownloadTargetDeterminer::DownloadTargetDeterminer(
     const base::FilePath& initial_virtual_path,
     DownloadPrefs* download_prefs,
     DownloadTargetDeterminerDelegate* delegate,
-    const content::DownloadTargetCallback& callback)
+    const CompletionCallback& callback)
     : next_state_(STATE_GENERATE_TARGET_PATH),
       should_prompt_(false),
       should_notify_extensions_(false),
@@ -67,6 +79,7 @@ DownloadTargetDeterminer::DownloadTargetDeterminer(
       conflict_action_(DownloadPathReservationTracker::OVERWRITE),
       danger_type_(download->GetDangerType()),
       virtual_path_(initial_virtual_path),
+      is_filetype_handled_securely_(false),
       download_(download),
       is_resumption_(download_->GetLastReason() !=
                          content::DOWNLOAD_INTERRUPT_REASON_NONE &&
@@ -112,6 +125,12 @@ void DownloadTargetDeterminer::DoLoop() {
       case STATE_DETERMINE_LOCAL_PATH:
         result = DoDetermineLocalPath();
         break;
+      case STATE_DETERMINE_MIME_TYPE:
+        result = DoDetermineMimeType();
+        break;
+      case STATE_DETERMINE_IF_HANDLED_BY_BROWSER:
+        result = DoDetermineIfHandledByBrowser();
+        break;
       case STATE_CHECK_DOWNLOAD_URL:
         result = DoCheckDownloadUrl();
         break;
@@ -305,7 +324,7 @@ DownloadTargetDeterminer::Result
   DCHECK(!virtual_path_.empty());
   DCHECK(local_path_.empty());
 
-  next_state_ = STATE_CHECK_DOWNLOAD_URL;
+  next_state_ = STATE_DETERMINE_MIME_TYPE;
 
   delegate_->DetermineLocalPath(
       download_,
@@ -329,6 +348,121 @@ void DownloadTargetDeterminer::DetermineLocalPathDone(
 }
 
 DownloadTargetDeterminer::Result
+    DownloadTargetDeterminer::DoDetermineMimeType() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  DCHECK(!virtual_path_.empty());
+  DCHECK(!local_path_.empty());
+  DCHECK(mime_type_.empty());
+
+  next_state_ = STATE_DETERMINE_IF_HANDLED_BY_BROWSER;
+
+  if (virtual_path_ == local_path_) {
+    delegate_->GetFileMimeType(
+        local_path_,
+        base::Bind(&DownloadTargetDeterminer::DetermineMimeTypeDone,
+                   weak_ptr_factory_.GetWeakPtr()));
+    return QUIT_DOLOOP;
+  }
+  return CONTINUE;
+}
+
+void DownloadTargetDeterminer::DetermineMimeTypeDone(
+    const std::string& mime_type) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  DVLOG(20) << "MIME type: " << mime_type;
+  mime_type_ = mime_type;
+  DoLoop();
+}
+
+#if defined(ENABLE_PLUGINS)
+// The code below is used by DoDetermineIfHandledByBrowser to determine if the
+// file type is handled by a sandboxed plugin.
+namespace {
+
+typedef std::vector<content::WebPluginInfo> PluginVector;
+
+// Returns true if there is a plugin in |plugins| that is sandboxed and enabled
+// for |profile|.
+bool IsSafePluginAvailableForProfile(scoped_ptr<PluginVector> plugins,
+                                     Profile* profile) {
+  using content::WebPluginInfo;
+
+  if (plugins->size() == 0)
+    return false;
+
+  scoped_refptr<PluginPrefs> plugin_prefs = PluginPrefs::GetForProfile(profile);
+  if (!plugin_prefs)
+    return false;
+
+  for (PluginVector::iterator plugin = plugins->begin();
+       plugin != plugins->end(); ++plugin) {
+    if (plugin_prefs->IsPluginEnabled(*plugin) &&
+        (plugin->type == WebPluginInfo::PLUGIN_TYPE_PEPPER_IN_PROCESS ||
+         plugin->type == WebPluginInfo::PLUGIN_TYPE_PEPPER_OUT_OF_PROCESS))
+      return true;
+  }
+  return false;
+}
+
+// Returns a callback that determines if a sandboxed plugin is available to
+// handle |mime_type| for a specific profile. Must be called on the IO thread.

[Randy Smith (Not in Mondays), 2013/11/04 22:57:47]

nit: The second sentence can be ambiguously read as applying either to this
function or the callback.  Disambiguation could be done by saying where the
callback must be called as well?  Or interchanging the two sentences?

[asanka, 2013/11/05 21:31:39]

Done. Reworded to explicitly state thread restrictions for the callback and the
function.

+base::Callback<bool(Profile*)> GetSafePluginChecker(
+    const std::string& mime_type) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  DCHECK(!mime_type.empty());
+
+  scoped_ptr<PluginVector> plugins(new PluginVector);
+  content::PluginService* plugin_service =
+      content::PluginService::GetInstance();
+  if (plugin_service)
+    plugin_service->GetPluginInfoArray(
+        GURL(), mime_type, false, plugins.get(), NULL);
+  return base::Bind(&IsSafePluginAvailableForProfile, base::Passed(&plugins));
+}
+
+} // namespace
+#endif  // ENABLE_PLUGINS
+
+DownloadTargetDeterminer::Result
+    DownloadTargetDeterminer::DoDetermineIfHandledByBrowser() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  DCHECK(!virtual_path_.empty());
+  DCHECK(!local_path_.empty());
+  DCHECK(!is_filetype_handled_securely_);
+
+  next_state_ = STATE_CHECK_DOWNLOAD_URL;
+
+  if (mime_type_.empty())
+    return CONTINUE;
+
+  if (net::IsSupportedMimeType(mime_type_)) {
+    is_filetype_handled_securely_ = true;
+    return CONTINUE;
+  }
+
+#if defined(ENABLE_PLUGINS)
+  BrowserThread::PostTaskAndReplyWithResult(
+      BrowserThread::IO,
+      FROM_HERE,
+      base::Bind(&GetSafePluginChecker, mime_type_),
+      base::Bind(&DownloadTargetDeterminer::DetermineIfHandledByBrowserDone,

[Randy Smith (Not in Mondays), 2013/11/04 22:57:47]

Just confirming: I read this code as using the mime_type to determine the
is_filetype_handled_securely_ question, based (in the PDF case) on whether or
not we have a PDF plugin that securely handles the PDF mime type.  For this to
match the actual open code, the way that an open that uses the PDF plugin would
have to work is that visiting a file:://<path>.pdf file  would need to go
through the net logic to figure out the mime type, and then go through the
plugin list to find a plugin that matches it.  But I have a memory that the
plugin list also includes file suffixes.  Have you confirmed that the actual URL
opening code matches the probe that's being done here (i.e. suffix -> MIME type
-> plugin list, as opposed to suffix -> plugin list directly)?

[asanka, 2013/11/05 21:31:39]

The flow for file:// URLs is:
//net:
  - MIME type for the UrlRequest is determined using net::GetMimeTypeFromFile()
by URLRequestFileJob

//content:
  - MIME type overridden by BufferedResourceHandler if the MIME type is empty or
one of kSniffableTypes.
(https://code.google.com/p/chromium/codesearch#chromium/src/net/base/mime_snif...).

Renderer:
  - Renderer creates WebCore::PluginDocument to render content, which
instantiates the plugin via a WebCore::HTMLEmbedElement with src=<file://url>
type=<MIME type from above>.
  - HTMLEmbedElement instantiates the plug-in which is eventually handled by
RenderFrameImpl::createPlugin().
  - ViewHostMsg_GetPluginInfo is sent back to browser.

Browser:
  - PluginInfoMessageFilter::OnGetPluginInfo() is called with file://url and
MIME type.
  - PluginService::GetPluginInfoArray is called which adds plugins based on MIME
type first and then based on URL.
  - PluginInfoMessageFilter::Context::FindEnabledPlugin() returns the first
plugin that is enabled from previous list.

...

I've updated the logic to use the file:// URL as well as the MIME type to more
closely resemble the underlying logic. Thanks for reminding me!

The difference in logic between the DownloadTargetDeterminer and the browsing
flow is in BufferedResourceHandler. Our decision that the file type is handled
securely might be wrong if something like the following (very contrived
scenario) happens:

- Some application associates .pdf with text/plain. text/plain is handled by the
renderer, so we would consider this to be handled securely. However text/plain
is also a sniffable MIME type.
- BufferedResourceHandler sniffs the contents and determines it's
application/pdf.
- Opening file://.../foo.pdf therefore results in instantiating the plug-in
associated with application/pdf which may not be our sandboxed plug-in.

+                 weak_ptr_factory_.GetWeakPtr()));
+  return QUIT_DOLOOP;
+#else
+  return CONTINUE;
+#endif
+}
+
+void DownloadTargetDeterminer::DetermineIfHandledByBrowserDone(
+    const base::Callback<bool(Profile*)>& callback) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  is_filetype_handled_securely_ = callback.Run(GetProfile());
+  DVLOG(20) << "Is file type handled securely: "
+            << is_filetype_handled_securely_;
+  DoLoop();
+}
+
+DownloadTargetDeterminer::Result
     DownloadTargetDeterminer::DoCheckDownloadUrl() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DCHECK(!virtual_path_.empty());
@@ -484,15 +618,20 @@ void DownloadTargetDeterminer::ScheduleCallbackAndDeleteSelf() {
             << " Intermediate:" << intermediate_path_.AsUTF8Unsafe()
             << " Should prompt:" << should_prompt_
             << " Danger type:" << danger_type_;
+  scoped_ptr<DownloadTargetInfo> target_info(new DownloadTargetInfo);
+
+  target_info->target_path = local_path_;
+  target_info->target_disposition =
+      (HasPromptedForPath() || should_prompt_
+           ? DownloadItem::TARGET_DISPOSITION_PROMPT
+           : DownloadItem::TARGET_DISPOSITION_OVERWRITE);
+  target_info->danger_type = danger_type_;
+  target_info->intermediate_path = intermediate_path_;
+  target_info->mime_type = mime_type_;
+  target_info->is_filetype_handled_securely = is_filetype_handled_securely_;
+
   base::MessageLoop::current()->PostTask(
-      FROM_HERE,
-      base::Bind(completion_callback_,
-                 local_path_,
-                 (HasPromptedForPath() || should_prompt_
-                      ? DownloadItem::TARGET_DISPOSITION_PROMPT
-                      : DownloadItem::TARGET_DISPOSITION_OVERWRITE),
-                 danger_type_,
-                 intermediate_path_));
+      FROM_HERE, base::Bind(completion_callback_, base::Passed(&target_info)));
   completion_callback_.Reset();
   delete this;
 }
@@ -631,12 +770,11 @@ void DownloadTargetDeterminer::OnDownloadDestroyed(
 }
 
 // static
-void DownloadTargetDeterminer::Start(
-    content::DownloadItem* download,
-    const base::FilePath& initial_virtual_path,
-    DownloadPrefs* download_prefs,
-    DownloadTargetDeterminerDelegate* delegate,
-    const content::DownloadTargetCallback& callback) {
+void DownloadTargetDeterminer::Start(content::DownloadItem* download,
+                                     const base::FilePath& initial_virtual_path,
+                                     DownloadPrefs* download_prefs,
+                                     DownloadTargetDeterminerDelegate* delegate,
+                                     const CompletionCallback& callback) {
   // DownloadTargetDeterminer owns itself and will self destruct when the job is
   // complete or the download item is destroyed. The callback is always invoked
   // asynchronously.