Prompt user for file access permission before download starts

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 99bcd7f1e855e09c96b958c6277aeda025c8f3dd..fd9b08c398fc12a2ebf3a981dd31e567f06624ef 100644
--- a/chrome/browser/download/download_target_determiner.cc
+++ b/chrome/browser/download/download_target_determiner.cc
@@ -39,6 +39,10 @@
 #include "content/public/common/webplugininfo.h"
 #endif
 
+#if defined(OS_ANDROID)
+#include "content/public/browser/android/download_controller_android.h"
+#endif
+
 #if defined(OS_WIN)
 #include "chrome/browser/ui/pdf/adobe_reader_info_win.h"
 #endif
@@ -70,6 +74,10 @@ void VisitCountsToVisitedBefore(
 bool g_is_adobe_reader_up_to_date_ = false;
 #endif
 
+#if defined(OS_ANDROID)
+  // Download controller for file access prompt.

[Ted C, 2015/07/06 20:03:39]

these two lines shouldn't be indented.

[qinmin, 2015/07/06 20:36:02]

Done.

+  content::DownloadControllerAndroid* g_download_controller_;

[asanka, 2015/07/06 21:09:41]

I know this'll end up being nullptr, but please initialize it explicitly.

[qinmin, 2015/07/07 00:54:22]

Done. make this a member variable and initialize it in the initializer list.

+#endif
 }  // namespace
 
 DownloadTargetInfo::DownloadTargetInfo()
@@ -86,7 +94,7 @@ DownloadTargetDeterminer::DownloadTargetDeterminer(
     DownloadPrefs* download_prefs,
     DownloadTargetDeterminerDelegate* delegate,
     const CompletionCallback& callback)
-    : next_state_(STATE_GENERATE_TARGET_PATH),
+    : next_state_(STATE_PROMPT_USER_FOR_PERMISSION),
       should_prompt_(false),
       should_notify_extensions_(false),
       create_target_directory_(false),
@@ -107,7 +115,10 @@ DownloadTargetDeterminer::DownloadTargetDeterminer(
   DCHECK(download_);
   DCHECK(delegate);
   download_->AddObserver(this);
-
+#if defined(OS_ANDROID)
+  if (!g_download_controller_)
+    g_download_controller_ = content::DownloadControllerAndroid::Get();
+#endif
   DoLoop();
 }
 
@@ -125,6 +136,9 @@ void DownloadTargetDeterminer::DoLoop() {
     next_state_ = STATE_NONE;
 
     switch (current_state) {
+      case STATE_PROMPT_USER_FOR_PERMISSION:
+        result = DoPromptUserForPermission();
+        break;
       case STATE_GENERATE_TARGET_PATH:
         result = DoGenerateTargetPath();
         break;
@@ -172,12 +186,45 @@ void DownloadTargetDeterminer::DoLoop() {
 }
 
 DownloadTargetDeterminer::Result
+    DownloadTargetDeterminer::DoPromptUserForPermission() {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  DCHECK(local_path_.empty());

[asanka, 2015/07/06 21:09:41]

Why are you checking these (here and below)?

In general you should only DCHECK dependencies for this state.

[qinmin, 2015/07/07 00:54:22]

Ok, removed.

On 2015/07/06 21:09:41, asanka wrote:

+  DCHECK(!should_prompt_);
+  DCHECK(!should_notify_extensions_);
+  DCHECK_EQ(DownloadPathReservationTracker::OVERWRITE, conflict_action_);
+  next_state_ = STATE_GENERATE_TARGET_PATH;
+#if defined(OS_ANDROID)
+  content::WebContents* web_contents = download_->GetWebContents();
+  if (g_download_controller_->RequireUserPermissionPrompt(web_contents)) {
+    g_download_controller_->PromptUserForPermission(
+        web_contents,
+        base::Bind(&DownloadTargetDeterminer::PromptUserForPermissionDone,
+                   weak_ptr_factory_.GetWeakPtr()));
+    return QUIT_DOLOOP;
+  }
+#endif
+  return CONTINUE;
+}
+
+void DownloadTargetDeterminer::PromptUserForPermissionDone(bool granted) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  DCHECK_EQ(STATE_GENERATE_TARGET_PATH, next_state_);
+  if (!granted) {
+    CancelOnFailureAndDeleteSelf();
+    return;
+  }
+
+  DoLoop();
+}
+
+DownloadTargetDeterminer::Result
     DownloadTargetDeterminer::DoGenerateTargetPath() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   DCHECK(local_path_.empty());
   DCHECK(!should_prompt_);
   DCHECK(!should_notify_extensions_);
   DCHECK_EQ(DownloadPathReservationTracker::OVERWRITE, conflict_action_);
+
   bool is_forced_path = !download_->GetForcedFilePath().empty();
 
   next_state_ = STATE_NOTIFY_EXTENSIONS;
@@ -902,3 +949,11 @@ bool DownloadTargetDeterminer::IsAdobeReaderUpToDate() {
   return g_is_adobe_reader_up_to_date_;
 }
 #endif
+
+#if defined(OS_ANDROID)
+// static
+void DownloadTargetDeterminer::SetDownloadControllerAndroidForTestings(

[asanka, 2015/07/06 21:09:41]

Rather than setting a global override here just for DTD, what about having an
override for DownloadControllerAndroid? It seems fragile that a global is
overridden this way just for a single consumer since other consumers or other
code paths might use the real object. Also any tests performed at a higher level
that'll depend on mocking this dependency will need to reach into each consuming
class that might be invoked during testing.

[qinmin, 2015/07/07 00:54:22]

Done. DownloadControllerAndroid::Get() will now returns a mock class by calling
UseMockControllerForTesting() in setup

+    content::DownloadControllerAndroid* download_controller) {
+  g_download_controller_ = download_controller;
+}
+#endif