Don't prompt for directory on web intents downloads; handle in current tab.

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 f3e7c054241574c09632573c425fed64a7f8c135..a3af42d00c87e87612544af0397a4b7d546cff1c 100644
--- a/chrome/browser/download/chrome_download_manager_delegate.cc
+++ b/chrome/browser/download/chrome_download_manager_delegate.cc
@@ -29,6 +29,7 @@
 #include "chrome/browser/extensions/api/downloads/downloads_api.h"
 #include "chrome/browser/extensions/crx_installer.h"
 #include "chrome/browser/extensions/extension_service.h"
+#include "chrome/browser/intents/web_intents_util.h"
 #include "chrome/browser/prefs/pref_service.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/safe_browsing/safe_browsing_service.h"
@@ -369,9 +370,6 @@ bool ChromeDownloadManagerDelegate::ShouldOpenDownload(DownloadItem* item) {
 
 bool ChromeDownloadManagerDelegate::ShouldOpenWithWebIntents(
     const DownloadItem* item) {
-  if (!item->GetWebContents() || !item->GetWebContents()->GetDelegate())
-    return false;
-
   std::string mime_type = item->GetMimeType();
   if (mime_type == "application/rss+xml" ||
       mime_type == "application/atom+xml") {
@@ -436,10 +434,16 @@ void ChromeDownloadManagerDelegate::OpenWithWebIntent(
 
   content::WebIntentsDispatcher* dispatcher =
       content::WebIntentsDispatcher::Create(intent_data);
-  // TODO(gbillock): try to get this to be able to delegate to the Browser
-  // object directly, passing a NULL WebContents?
-  item->GetWebContents()->GetDelegate()->WebIntentDispatch(
-      item->GetWebContents(), dispatcher);
+
+  content::WebContentsDelegate* delegate = NULL;
+  if (item->GetWebContents() && item->GetWebContents()->GetDelegate()) {
+    delegate = item->GetWebContents()->GetDelegate();
+  } else {
+    delegate = web_intents::GetBrowserForBackgroundWebIntentDelivery(
+        profile_);
+  }
+  if (delegate)
+    delegate->WebIntentDispatch(NULL, dispatcher);

[Randy Smith (Not in Mondays), 2012/08/28 18:03:43]

Shouldn't this logic be duplicated in ShouldOpenWithWebIntents so that we don't
say "Yes, we'll open it with web intents!" and then don't find a delegate and
skip doing so?

[Greg Billock, 2012/08/28 18:38:00]

What I found in my testing is that for target=_blank cases is that the item-

 }
 
 bool ChromeDownloadManagerDelegate::GenerateFileHash() {
@@ -709,6 +713,8 @@ void ChromeDownloadManagerDelegate::CheckVisitedReferrerBeforeDone(
     }
     if (download_prefs_->IsDownloadPathManaged())
       should_prompt = false;
+    if (ShouldOpenWithWebIntents(download))

[Randy Smith (Not in Mondays), 2012/08/28 18:03:43]

Help me understand the reason for this?  I'm worried, because I think this is
the only signal we currently have that we're doing a "Save As ..." download, and
we want that to bypass web intents.

[Greg Billock, 2012/08/28 18:38:00]

Yes, we definitely do. What I'm trying to do here is suppress the "choose a
directory for this file" which cripples the use case. We definitely want Save
As... to skip WI though. Is there a condition I can add to
ShouldOpenWithWebIntents that detects Save As? That'd be a good fix to include
here.

On 2012/08/28 18:03:43, rdsmith wrote:

[benjhayden, 2012/08/28 18:42:14]

Can you show how Save Pages use this code path?
I couldn't see, so I stuck a print statement in here and saved a page and did
not see the print statement, but I might be missing something.

[Randy Smith (Not in Mondays), 2012/08/28 19:05:19]

I don't completely understand the use case--do you know what triggers the
selecting of a directory for that file?

I think if the download has TARGET_DISPOSITION_PROMPT that means that either a)
the download was triggered by an extension that requested prompting, b) It's
from a context menu, c) It's from a "Save Page As ..." that couldn't use the
regular pathway, or d) It's from a renderer plugin requesting save.  All of
those are cases I think it's fine to disable web intents for, but I'm not sure
why else should_prompt would be true here except for those cases.  Can you give
me a sense as to how should_prompt ends up true here in the case you're trying
to deal with?  (Pretty much the same question as above.)

[Greg Billock, 2012/09/10 20:19:07]

Ah! I somehow didn't see this #11 in the thread. I've 
added the TARGET_DISPOSITION_PROMPT check to ShouldOpenWithWebIntents, which we
definitely want to do. Does that mean that this condition should stay? I'd like
to suppress the choose-the-path dialog, which the comments in 719+ makes it look
like get established here.


On 2012/08/28 19:05:19, rdsmith wrote:

+      should_prompt = false;
 
     // Determine the proper path for a download, by either one of the following:
     // 1) using the default download directory.