Implement chrome.downloads.onDeterminingFilename()

chrome/browser/download/chrome_download_manager_delegate.h

Index: chrome/browser/download/chrome_download_manager_delegate.h
diff --git a/chrome/browser/download/chrome_download_manager_delegate.h b/chrome/browser/download/chrome_download_manager_delegate.h
index 92241657f1f67a73e53a36f070d95599304a1e60..7352d07184e54b116ec6d13ec4fdcd36c6d84700 100644
--- a/chrome/browser/download/chrome_download_manager_delegate.h
+++ b/chrome/browser/download/chrome_download_manager_delegate.h
@@ -19,7 +19,6 @@
 #include "content/public/browser/notification_registrar.h"
 
 class DownloadPrefs;
-class ExtensionDownloadsEventRouter;
 class Profile;
 
 namespace content {
@@ -136,6 +135,17 @@ class ChromeDownloadManagerDelegate
  private:
   friend class base::RefCountedThreadSafe<ChromeDownloadManagerDelegate>;
 
+  struct ContinueFilenameDeterminationInfo {

[Randy Smith (Not in Mondays), 2013/01/14 21:02:29]

References to this structure below are references :-}, so you just need a
forward decl not a full defn.

[benjhayden, 2013/01/17 02:52:05]

Done.

+    ContinueFilenameDeterminationInfo();
+    ~ContinueFilenameDeterminationInfo();
+
+    int32 download_id;
+    content::DownloadTargetCallback callback;
+    content::DownloadDangerType danger_type;
+    bool visited_referrer_before;
+    bool should_prompt;
+  };
+
   // content::NotificationObserver implementation.
   virtual void Observe(int type,
                        const content::NotificationSource& source,
@@ -194,6 +204,22 @@ class ChromeDownloadManagerDelegate
       const FilePath& reserved_path,
       bool reserved_path_verified);
 
+  // When an extension opts to change a download's target filename, this
+  // sanitizes it before continuing with the filename determination process.
+  void OnExtensionsChangingFilename(
+      const ContinueFilenameDeterminationInfo& continue_info,
+      const FilePath& changed_filename,
+      bool overwrite);
+
+  // When extensions either opt not to change a download's target filename, or
+  // the changed filename has been sanitized, this method continues with the
+  // filename determination process, optionally prompting the user to manually
+  // set the filename.
+  void OnExtensionsDeterminedFilename(
+      const ContinueFilenameDeterminationInfo& continue_info,
+      const FilePath& suggested_path,
+      bool is_forced_path);
+

[Randy Smith (Not in Mondays), 2013/01/14 21:02:29]

I'm concerned about the names of these two functions and
EDER::OnDownloadFilenameDetermined.  (If you don't care about my philosophy and
just want my request, skip to the last paragraph :-}).

I think of "On*" as being either notifications or returns from async calls--i.e.
when information is passed in without expectation of a response.  In contrast, I
think of action method names (DetermineDownloadFilename) as being used to
initiate actions (that may or may not return results).  I'm also uncomfortable
with the second of the two names above, since it's used when the extension
decides not to determine the filename and when it does and we've sanitized it.  

So I'd like to request something like:
* EDER::DetermineDownloadFilename (or DetermineFilename, or something fancier)
* CDMD::OnExtensionSpecifyingFilename
* CDMD::OnFilenameDetermined (since it's not clear by who).

WDYT?

[benjhayden, 2013/01/17 02:52:05]

I didn't put any thought into naming these methods, so it's good that we're
talking about them.

I'm getting hung up on the word 'determine', which suggests finality. This is
all right in the middle of the determination process, so the filename is far
from determined. Maybe the imperfect/progressive 'determining' would fit better?
WDYT of these?
EDER::DeterminingFilename
CDMD::OnExtensionOverridingFilename
CDMD::ContinueDeterminingFilename

[Randy Smith (Not in Mondays), 2013/01/17 19:15:42]

Can we find something that's an imperative?  I'm good with
ContinueDeterminingFilename (and OnExtensionOverridingFilename) but I'd like
"DeterminingFilename" to sound something more like an order the caller gives the
routine.  (I keep thinking of "CommentOnFilename", but I don't think that's
good, just amusing :-}.)  

What changes to the filename can happen after the extension comes back?  I guess
if we don't specify overwrite we can get suffixes?  MaybeOverrideBaseFilename?

[benjhayden, 2013/01/18 20:59:50]

All of the other methods on EDER that dispatch events to the renderer start with
On.
EDER::OnDeterminingFilename?

[Randy Smith (Not in Mondays), 2013/01/22 19:43:07]

Sure.

   // Called on the UI thread once the final target path is available.
   void OnTargetPathDetermined(
       int32 download_id,
@@ -230,20 +256,6 @@ class ChromeDownloadManagerDelegate
 
   content::NotificationRegistrar registrar_;
 
-  // On Android, GET downloads are not handled by the DownloadManager.
-  // Once we have extensions on android, we probably need the EventRouter
-  // in ContentViewDownloadDelegate which knows about both GET and POST
-  // downloads.
-#if !defined(OS_ANDROID)
-  // The ExtensionDownloadsEventRouter dispatches download creation, change, and
-  // erase events to extensions. Like ChromeDownloadManagerDelegate, it's a
-  // chrome-level concept and its lifetime should match DownloadManager. There
-  // should be a separate EDER for on-record and off-record managers.
-  // There does not appear to be a separate ExtensionSystem for on-record and
-  // off-record profiles, so ExtensionSystem cannot own the EDER.
-  scoped_ptr<ExtensionDownloadsEventRouter> extension_event_router_;

[Randy Smith (Not in Mondays), 2013/01/14 21:02:29]

So do you remember why we wanted to move EDER to DS?  After having read through
this CL, I'd be inclined to leave it on CDMD.  Specifically, I don't see that
anything outside of CDMD is currently referencing it (maybe we have plans?) and
this CL means that CDMD needs to reference it, so minimal visibility would make
it (from the C++ perspective) an implementation detail of CDMD.  WDYT?

[benjhayden, 2013/01/17 02:52:05]

The downloadsInternal API needs to access the EDER.
https://codereview.chromium.org/11574006/diff/53009/chrome/browser/extensions...

[Randy Smith (Not in Mondays), 2013/01/17 19:15:42]

Glancing at that file, it looks like it's only accessing static functions.  So
it needs to be able to read the .h file, not access the actual object.  What am
I missing?

(If I'm not missing anything, that means there are unneeded include files in the
internal API implementation.)

[benjhayden, 2013/01/18 20:59:50]

The EDER static methods that downloadsInternal calls go to the
DownloadServiceFactory to get the EDER instance.
There were.

-#endif
-
   // The directory most recently chosen by the user in response to a Save As
   // dialog for a regular download.
   FilePath last_download_path_;