Implement chrome.downloads.onDeterminingFilename()

chrome/common/extensions/api/downloads.idl

Index: chrome/common/extensions/api/downloads.idl
diff --git a/chrome/common/extensions/api/downloads.idl b/chrome/common/extensions/api/downloads.idl
index db7116520a5fc74a55124a2a378bf4446fd3d33f..425080f982390312230be1c014cec94916314d9a 100644
--- a/chrome/common/extensions/api/downloads.idl
+++ b/chrome/common/extensions/api/downloads.idl
@@ -12,6 +12,19 @@ namespace downloads {
     DOMString value;
   };
 
+  // $ref:onDeterminingFilename handlers may return a FilenameDetermination
+  // object in order to override a download's target filename.

[mkearney1, 2013/02/05 22:00:02]

Should this link to filename? I can't see the staged docs at the moment to check
the flow, so maybe it isn't necessary.

[benjhayden, 2013/02/21 22:29:33]

Done.

+  dictionary FilenameDetermination {
+    // The download's new target filename, as a path relative to the user's
+    // default Downloads directory, possibly containing subdirectories. Absolute

[Matt Perry, 2013/02/04 22:30:19]

How do we handle path separators in a cross-platform manner? On Windows, will
"../foo.txt" show up as a back reference given that \ is the path separator?

[benjhayden, 2013/02/05 15:08:05]

FilePath allows either / or \ as a path separator on windows.
https://code.google.com/p/chromium/codesearch#chrome/src/base/file_path.h&q=f...

+    // paths, empty paths, and paths containing back-references ".." will be
+    // ignored.
+    DOMString filename;
+
+    // Whether to overwrite any existing files.
+    boolean? overwrite;
+  };
+
   [inline_doc] enum HttpMethod {GET, POST};
 
   [inline_doc] dictionary DownloadOptions {
@@ -396,9 +409,6 @@ namespace downloads {
     // then <code>callback</code> will be called.
     static void erase(DownloadQuery query, optional EraseCallback callback);
 
-    // TODO(benjhayden) Comment.
-    [nodoc] static void setDestination(long downloadId, DOMString relativePath);
-
     // Prompt the user to accept a dangerous download. Does not automatically
     // accept dangerous downloads. If the download is accepted, then an
     // $ref:onChanged event will fire, otherwise nothing will happen.  When all
@@ -427,5 +437,15 @@ namespace downloads {
     // except <code>bytesReceived</code> changes, this event fires with the
     // <code>downloadId</code> and an object containing the properties that changed.
     static void onChanged(DownloadDelta downloadDelta);
+
+    // During the filename determination process, extensions will be given the
+    // opportunity to override the target filename. Handlers may return null in

[mkearney1, 2013/02/05 22:00:02]

Should target filename be a link?

[benjhayden, 2013/02/21 22:29:33]

Done.

+    // order to allow the target filename to be used, or return a
+    // $ref:FilenameDetermination object in order to override the target
+    // filename. If more than one extension overrides the filename, then the
+    // last handler registered in the last extension installed that returns a
+    // $ref:FilenameDetermination object wins. Users should not install
+    // extensions that may conflict.
+    static void onDeterminingFilename(DownloadItem downloadItem);

[Matt Perry, 2013/02/04 22:30:19]

Does this really need to be a synchronous API? What if we did this instead:
- Downloads system starts writing to the default filename immediately.
- Extensions have a certain amount of time to rename the file to something else.

[benjhayden, 2013/02/05 15:08:05]

The download system does already start writing to a temporary "Unconfirmed
XXXXX.crdownload" filename immediately.
I think that Asanka explained to me that it would be untenable in the current
download system architecture to determine the filename multiple times.
How much time? What if the system is being slow? What about when I change the
API to allow extensions to pop up a dialog to allow the user to choose whether
to overwrite?

[Randy Smith (Not in Mondays), 2013/02/05 16:40:41]

I'd like to second this point--this type of race strikes me as a really bad
idea.  The extension writer will not be able to depend on the API to react
reliably; whether the rename happens will depend on system loader, user
distraction (in the case Ben raises), and the phase of the moon.  Let's please
not do timing window dependent extension APIs.

[Matt Perry, 2013/02/05 20:49:39]

OK, sounds like this won't work.
Is the prompt blocking from the JavaScript context's point of view? The
onDeterminingFilename event requires a synchronous response - any asynchronous
query won't work.

[benjhayden, 2013/02/05 21:12:19]

I am planning on augmenting the onDeterminingFilename event in a followup CL to
allow extensions to request downloads_custom_bindings.js to pass the handler a
callback. This future change will only affect downloads_custom_bindings.js.
Handlers may then look like this:
chrome.downloads.onDeterminingFilename.addListener(function(item, callback) {
  chrome.tabs.sendMessage(some_tab_id, item, function(response) {
    callback(response);
  });
}, true/*handler is asynchronous*/);

[Matt Perry, 2013/02/05 21:57:10]

I see. In that case, I'd prefer we always use the callback for consistency. See
https://developer.chrome.com/trunk/extensions/runtime.html#event-onMessage for
an API that functions similarly (you return true from the event handler to
indicate it's asynchronous).

[benjhayden, 2013/02/21 22:29:33]

Done.

   };
 };