Move download filename determintion into a separate class.

Move download filename determination into a separate class. 

* Extracts download filename determination into
  DownloadTargetDeterminer. The new class maintains state and observes
  the download. Doing so eliminates the need to pass state around as
  bound arguments.

* Guarantees that the completion callback to
  DownloadManagerDelegate::DetermineDownloadTarget is always invoked,
  even if the download is interrupted on initialization. This is
  required for reliably resuming downloads.

* The DownloadFilePicker always returns the virtual path for downloads
  to Drive. ChromeDownloadManagerDelegate can use it to keep track of
  the correct last selected directory for 'Save As' downloads.

* Since no path subsubstituion is necessary during prompting,
  DownloadFilePickerChromeOS is no longer necessary.

* Re-orders the sequence of events so that the user is prompted as early
  as possible. Expensive history database lookups won't introduce
  unnecssary jank.

* History database lookups are only performed if the results of the
  lookup are necessary.

* Downloads to Drive don't need the temporary local path to be
  determined twice when the default downloads directory is Drive.

BUG=151618
BUG=104335
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=197518

[asanka, 2013-04-06 01:21:49]

Randy: Sorry to drop this on you. Could you do a high level glance over?

[Randy Smith (Not in Mondays), 2013-04-08 15:39:33]

Very high level opinion: I think this makes sense.  Download file name
determination has gotten to be complex enough that it's worthwhile being able to
contemplate it on its own.

I primarily looked over the interface for DownloadTargetDeterminer.  Areas that
I didn't do but would be the next things I'd do on a more detailed review:
Understand in detail what the re-ordering being done is and make sure there
aren't gotchas, and figure out the hopping back and forth between DTD and CDMD
and see if there are any uglinesses or simplifications in it.  Would you like me
to do that next round?  (My life would be easier if you could easily fully
specify either the change in re-ordering or the hopping back and forth, but if
you'd just be creating it from the code, don't bother, as it's probably overall
faster for me to do the same thing.)

BTW, it's absolutely fine to throw this type of review at me.  I'm not
considering myself as being completely unavailable for downloads consultation;
that seems silly.  I'm just trying to make sure that you guys are the point
people, but you should feel free to grab me as you need me.

https://codereview.chromium.org/12850002/diff/9001/chrome/browser/download/do...
File chrome/browser/download/download_target_determiner.h (right):

https://codereview.chromium.org/12850002/diff/9001/chrome/browser/download/do...
chrome/browser/download/download_target_determiner.h:92: enum State {
I understand that keeping it in sync with the code will be a bear, but I think
that there's real value to having the state transitions described somewhere in
comments; it'd be a real benefit to anyone coming to this code without the
background.  (I speak as someone who'd love to see this in the cache :-}.)

https://codereview.chromium.org/12850002/diff/9001/chrome/browser/download/do...
chrome/browser/download/download_target_determiner.h:120:
DownloadTargetDeterminerDelegate* delegate,
Lifetime guarantees?  At least spec the assumptions in the class decl.

https://codereview.chromium.org/12850002/diff/9001/chrome/browser/download/do...
chrome/browser/download/download_target_determiner.h:193: Result
DoDetermineIntermediatePath();
We had talked about separating out intermediate path name determination from the
rest of this, so that it could be re-done independently in the case of
resumption.  Do you want to factor that potential requirement into the design?

https://codereview.chromium.org/12850002/diff/9001/chrome/browser/download/do...
File chrome/browser/download/download_target_determiner_delegate.h (right):

https://codereview.chromium.org/12850002/diff/9001/chrome/browser/download/do...
chrome/browser/download/download_target_determiner_delegate.h:24: class
DownloadTargetDeterminerDelegate {
Given that this is the base class and that the derived classes won't have any,
probably worthwhile having some documentation comments here.

[asanka, 2013-04-08 22:44:57]

Thanks! Could you do the next round? I updated the CL description to call out
the changes explicitly. I added some comments as well.

https://chromiumcodereview.appspot.com/12850002/diff/9001/chrome/browser/down...
File chrome/browser/download/download_target_determiner.h (right):

https://chromiumcodereview.appspot.com/12850002/diff/9001/chrome/browser/down...
chrome/browser/download/download_target_determiner.h:92: enum State {
On 2013/04/08 15:39:33, rdsmith wrote:
> I understand that keeping it in sync with the code will be a bear, but I think
> that there's real value to having the state transitions described somewhere in
> comments; it'd be a real benefit to anyone coming to this code without the
> background.  (I speak as someone who'd love to see this in the cache :-}.)

I added comments next to each declaration.

https://chromiumcodereview.appspot.com/12850002/diff/9001/chrome/browser/down...
chrome/browser/download/download_target_determiner.h:120:
DownloadTargetDeterminerDelegate* delegate,
On 2013/04/08 15:39:33, rdsmith wrote:
> Lifetime guarantees?  At least spec the assumptions in the class decl.

Added a comment indicating that the constraints are as per Start() above.

https://chromiumcodereview.appspot.com/12850002/diff/9001/chrome/browser/down...
chrome/browser/download/download_target_determiner.h:193: Result
DoDetermineIntermediatePath();
On 2013/04/08 15:39:33, rdsmith wrote:
> We had talked about separating out intermediate path name determination from
the
> rest of this, so that it could be re-done independently in the case of
> resumption.  Do you want to factor that potential requirement into the design?

Good point. I think that will be a static method + overloaded constructor that
starts the workflow at STATE_DETERMINE_INTERMEDIATE_PATH step. I can add it now,
but I think I'll wait until we handle that case in the downloads resumption. Let
me know if you would like me to add it now.

https://chromiumcodereview.appspot.com/12850002/diff/9001/chrome/browser/down...
File chrome/browser/download/download_target_determiner_delegate.h (right):

https://chromiumcodereview.appspot.com/12850002/diff/9001/chrome/browser/down...
chrome/browser/download/download_target_determiner_delegate.h:24: class
DownloadTargetDeterminerDelegate {
On 2013/04/08 15:39:33, rdsmith wrote:
> Given that this is the base class and that the derived classes won't have any,
> probably worthwhile having some documentation comments here.

Done.

[benjhayden, 2013-04-09 15:46:32]

https://codereview.chromium.org/12850002/diff/9001/chrome/browser/download/ch...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

https://codereview.chromium.org/12850002/diff/9001/chrome/browser/download/ch...
chrome/browser/download/chrome_download_manager_delegate.cc:162: //
TODO(asanka): This determination is done based on |path|, while elsewhere
Where else?

https://codereview.chromium.org/12850002/diff/9001/chrome/browser/download/ch...
chrome/browser/download/chrome_download_manager_delegate.cc:439: const
base::FilePath& virtual_path,
I spent a few seconds looking for where this was used.
s/virtual_path/unused_virtual_path/ ?

https://codereview.chromium.org/12850002/diff/9001/chrome/browser/download/do...
File chrome/browser/download/download_file_picker.cc (right):

https://codereview.chromium.org/12850002/diff/9001/chrome/browser/download/do...
chrome/browser/download/download_file_picker.cc:63: const FileSelectedCallback&
callback) : download_id_(0) {
Why not download_id_(item->GetId())?

https://codereview.chromium.org/12850002/diff/9001/chrome/browser/download/do...
File chrome/browser/download/download_file_picker_chromeos.cc (right):

https://codereview.chromium.org/12850002/diff/9001/chrome/browser/download/do...
chrome/browser/download/download_file_picker_chromeos.cc:79:
OnFileSelected(baes::FilePath(), base::FilePath());
s/baes/base/

https://codereview.chromium.org/12850002/diff/9001/chrome/browser/download/do...
File chrome/browser/download/download_prefs.cc (right):

https://codereview.chromium.org/12850002/diff/9001/chrome/browser/download/do...
chrome/browser/download/download_prefs.cc:210: DCHECK(profile_);
I thought it was unnecessary to dcheck pointers that you're about to deref? The
stack trace will look the same.

https://codereview.chromium.org/12850002/diff/9001/chrome/browser/download/do...
File chrome/browser/download/download_target_determiner.cc (right):

https://codereview.chromium.org/12850002/diff/9001/chrome/browser/download/do...
chrome/browser/download/download_target_determiner.cc:176: virtual_path_ =
suggested_path;
Why not assign directly to virtual_path_ above instead of copying through
suggested_path?

https://codereview.chromium.org/12850002/diff/9001/chrome/browser/download/do...
chrome/browser/download/download_target_determiner.cc:534: // wanted to be
prompted for "all" downloads, or if this was a 'Save As'
What if the user wants to change the basename?

https://codereview.chromium.org/12850002/diff/9001/chrome/browser/download/do...
chrome/browser/download/download_target_determiner.cc:547: //    an extension is
installed, not when one is downloaded with "save as...".
Won't L552 prevent prompting if a user Saves As a .crx?

https://codereview.chromium.org/12850002/diff/9001/chrome/browser/download/do...
File chrome/browser/download/download_target_determiner.h (right):

https://codereview.chromium.org/12850002/diff/9001/chrome/browser/download/do...
chrome/browser/download/download_target_determiner.h:67: //   determining the
user's preferences regarding the default downloads
Would DTD be more modular if those strings were passed in?

https://codereview.chromium.org/12850002/diff/9001/chrome/browser/download/do...
chrome/browser/download/download_target_determiner.h:233: };
DISALLOW_COPY_AND_ASSIGN?

https://codereview.chromium.org/12850002/diff/9001/chrome/browser/download/do...
File chrome/browser/download/download_target_determiner_delegate.h (right):

https://codereview.chromium.org/12850002/diff/9001/chrome/browser/download/do...
chrome/browser/download/download_target_determiner_delegate.h:37: virtual
ExtensionDownloadsEventRouter* GetExtensionEventRouter() = 0;
Would DTD be more modular if Start() took a callback that dispatched
chrome.downloads.onDeterminingFilename? Or could Start() take the EDER or the
Profile/BrowserContext and go through DownloadService itself?

https://codereview.chromium.org/12850002/diff/9001/chrome/browser/download/do...
chrome/browser/download/download_target_determiner_delegate.h:39: virtual void
PromptUserForDownloadPath(
Why can't DTD do this itself?

https://codereview.chromium.org/12850002/diff/9001/chrome/browser/download/do...
chrome/browser/download/download_target_determiner_delegate.h:44: virtual void
DetermineLocalPath(content::DownloadItem* download,
Why can't DTD do this itself?

https://codereview.chromium.org/12850002/diff/9001/chrome/browser/download/do...
chrome/browser/download/download_target_determiner_delegate.h:48: virtual void
ReserveVirtualPath(content::DownloadItem* download,
Why can't DTD do this itself?

https://codereview.chromium.org/12850002/diff/9001/chrome/browser/download/do...
File chrome/browser/download/download_target_determiner_unittest.cc (right):

https://codereview.chromium.org/12850002/diff/9001/chrome/browser/download/do...
chrome/browser/download/download_target_determiner_unittest.cc:106: // as
necessary for flagging the download with as a dangerous download of type
with as?

https://codereview.chromium.org/12850002/diff/9001/chrome/browser/download/do...
chrome/browser/download/download_target_determiner_unittest.cc:117: const
base::FilePath::CharType*   forced_file_path;
What are all the spaces for?

[Randy Smith (Not in Mondays), 2013-04-09 19:32:17]

Next round.  Two high level comments:

* Relying on you and Ben to get the relationship between extension and user
prompting right (I've forgotten already what we spec for the extensions in the
non overwrite case).

* I'd like to talk to you in person about the concept of virtual paths; they
feel funny to me.  As if we're trying to do an abstraction but not finishing it.
 I want to make sure I understand what the use cases are, and what entities are
expected to understand them.

https://codereview.chromium.org/12850002/diff/21002/chrome/browser/download/d...
File chrome/browser/download/download_target_determiner.cc (right):

https://codereview.chromium.org/12850002/diff/21002/chrome/browser/download/d...
chrome/browser/download/download_target_determiner.cc:187: return COMPLETE;
What's the logic for having this only here?  This looks like trying to handle a
race with the callback from the DownloadFile::Initialize() method, and such a
callback could come back in at any point.  If we don't expect that failure to
happen very often, I'm inclined to think we shouldn't bother to handle the race
(why add racy complexity) in which case we could get rid of this.  If we do
expect it to happen often, shouldn't we have it in DoLoop() or somewhere like
that?  Though that might require more careful specification of what the callback
might say if we bail at different points.

https://codereview.chromium.org/12850002/diff/21002/chrome/browser/download/d...
chrome/browser/download/download_target_determiner.cc:251:
DownloadTargetDeterminer::DoReserveVirtualPath() {
What are your conceptual thoughts about reserving paths in file systems that may
be shared between devices?  We can keep from colliding locally, but we can't
really keep from colliding with what some other device is doing.  Does that
imply any differences in how we approach reservation?

https://codereview.chromium.org/12850002/diff/21002/chrome/browser/download/d...
chrome/browser/download/download_target_determiner.cc:280: // We also prompt if
the download path couldn't be verified.
I was a bit confused by this (I thought it might be a todo).  Maybe
"ReserveVirtualPathDone (above) will also have set this if the path couldn't be
verified."?

https://codereview.chromium.org/12850002/diff/21002/chrome/browser/download/d...
chrome/browser/download/download_target_determiner.cc:350: #if
defined(FULL_SAFE_BROWSING)
Given that we have to have the ifdef anyway, I wonder if just duplicating the
logic around s_b::DownloadProtectionService from the CDMD here rather than
having the extra interface might make more sense?  Though maybe this is better
for testing.  

(I continue to wish that a null SB service was indicated by having an
implementation of the SB service that was null, rather than us having to test
for null.  But there are probably good reasons against that.)

https://codereview.chromium.org/12850002/diff/21002/chrome/browser/download/d...
chrome/browser/download/download_target_determiner.cc:434: if
(IsDangerousFile(false))
I'm not clear it's worth it, but you could refactor this to reduce history pings
by a little bit more.  visited_referrer_before is only relevant if the file type
is AllowOnUserGesture.  This does reduce flexibility of changing how we decide
on danger in the future, though.

https://codereview.chromium.org/12850002/diff/21002/chrome/browser/download/d...
File chrome/browser/download/download_target_determiner.h (right):

https://codereview.chromium.org/12850002/diff/21002/chrome/browser/download/d...
chrome/browser/download/download_target_determiner.h:31: //     point.
I wince a little bit at this, in that restricting it to have a virtual mount
point seems overly file-system centric.  What entities need to understand a
virtual path?  Is it worthwhile considering making it a URL?

https://codereview.chromium.org/12850002/diff/21002/chrome/browser/download/d...
chrome/browser/download/download_target_determiner.h:73: // |delegate| is
required and must outlive the |download|.
In this practical situation (with the delegate being the CDMD) how is this
guaranteed?

https://codereview.chromium.org/12850002/diff/21002/chrome/browser/download/d...
chrome/browser/download/download_target_determiner.h:75: //   download| is
destroyed prior to that, then |callback| will be invoked as
nit: missing '|'

https://codereview.chromium.org/12850002/diff/21002/chrome/browser/download/d...
chrome/browser/download/download_target_determiner.h:76: //   well. However the
paths will be empty.
Maybe make clear that the callback may be invoked after download destruction? 
(I think that's what I'm seeing in the code because we end up posting a task to
the current message loop after we receive the OnDownloadDestroyed() message).

https://codereview.chromium.org/12850002/diff/21002/chrome/browser/download/d...
chrome/browser/download/download_target_determiner.h:111: // callback is
received.
I'd modify this comment to include the possibility that the DoLoop() has already
been invoked (in the case of synchronous execution of callback).  Maybe "If a
handler returns this value, it has taken responsibility for ensuring that
DoLoop() is invoked (and possibly has already invoked it)."

I'd also like some tag other than "PENDING" since that pretty clearly implies
the execution of DoLoop() will be in the future, but I'm not coming up with a
good alternative :-J.

https://codereview.chromium.org/12850002/diff/21002/chrome/browser/download/d...
chrome/browser/download/download_target_determiner.h:162: // -
STATE_DETERMINE_LOCAL_PATH : All other downloads.
(Thanks very much for doing these comments, by the way--it really helps.)

I'm confused; why do we skipping determining the local path if a prompt was
shown to the user?  Do we only prompt for local paths?

https://codereview.chromium.org/12850002/diff/21002/chrome/browser/download/d...
chrome/browser/download/download_target_determiner.h:210: // Determins the
intermediate path. Once this step completes, downloads target
Worth mentioning that we implicitly assuming in doing this that the intermediate
file will be uniquified if needed when it hits the disk?

https://codereview.chromium.org/12850002/diff/21002/chrome/browser/download/d...
File chrome/browser/download/download_target_determiner_delegate.h (right):

https://codereview.chromium.org/12850002/diff/21002/chrome/browser/download/d...
chrome/browser/download/download_target_determiner_delegate.h:64: virtual
ExtensionDownloadsEventRouter* GetExtensionEventRouter() = 0;
Does this need to be a delegate routine?  It doesn't look like it's used in CDMD
after this refactor, so you could just pull the CDMD implementation into DTD. 
It might make testing a bit trickier, but I'd be inclined to think that was
worth it for a cleaner interface here.

https://codereview.chromium.org/12850002/diff/21002/chrome/browser/download/d...
chrome/browser/download/download_target_determiner_delegate.h:68: virtual void
PromptUserForDownloadPath(
Same comment as above--this doesn't seem like it needs to be CDMD/DTDD based.

[asanka, 2013-04-16 20:34:01]

This is more of an interim patchset, since I want to solicit opinions
testability before soliciting a detailed review.

Testability is the reason why the DownloadTargetDeterminerDelegate exists.
Loosely coupled dependencies mean that DTD's behavior can be tested without
being bogged down in the implementation details of a dependency. Since both of
you raised questions, I tried removing a couple of the callbacks. But I think we
should figure out a testing strategy before putting any more work.

Here are a few options:
*) Inject dependencies at DTD creation. E.g.: DownloadFilePickerFactory in
patchset 3.
   Pro: Somewhat easy to test.
   Con: Not every dependency is injectable without substantial refactors. Also,
CDMD is still responsible for providing dependencies.

*) Delegate finer grained operations. E.g.: DTDD::DetermineLocalPath().
   Pro: Very easy to test.
   Con: The delegate is responsible for picking out dependencies. However, we
can move this code to a default delegate that DTD uses if one isn't provided.

*) Anything else?

I'm leaving out having hard coded dependencies in DTD since it doesn't meet the
testability requirement. I'd be happy to hear other suggestions.

What do you think?

Regarding virtual paths: As I mentioned in person, this CL doesn't intend to
define a new abstraction for dealing with virtual paths. That's something that
will have to happen along with changes to /ui/shell_dialogs after we have some
understanding of how we want to support cloud storage in general. In this CL,
I'm separating out a virtual path from a local path.

https://chromiumcodereview.appspot.com/12850002/diff/9001/chrome/browser/down...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

https://chromiumcodereview.appspot.com/12850002/diff/9001/chrome/browser/down...
chrome/browser/download/chrome_download_manager_delegate.cc:162: //
TODO(asanka): This determination is done based on |path|, while elsewhere
On 2013/04/09 15:46:32, benjhayden_chromium wrote:
> Where else?

Updated comment.

https://chromiumcodereview.appspot.com/12850002/diff/9001/chrome/browser/down...
File chrome/browser/download/download_file_picker.cc (right):

https://chromiumcodereview.appspot.com/12850002/diff/9001/chrome/browser/down...
chrome/browser/download/download_file_picker.cc:63: const FileSelectedCallback&
callback) : download_id_(0) {
On 2013/04/09 15:46:32, benjhayden_chromium wrote:
> Why not download_id_(item->GetId())?

Done.

https://chromiumcodereview.appspot.com/12850002/diff/9001/chrome/browser/down...
File chrome/browser/download/download_prefs.cc (right):

https://chromiumcodereview.appspot.com/12850002/diff/9001/chrome/browser/down...
chrome/browser/download/download_prefs.cc:210: DCHECK(profile_);
On 2013/04/09 15:46:32, benjhayden_chromium wrote:
> I thought it was unnecessary to dcheck pointers that you're about to deref?
The
> stack trace will look the same.

Done.

https://chromiumcodereview.appspot.com/12850002/diff/9001/chrome/browser/down...
File chrome/browser/download/download_target_determiner.cc (right):

https://chromiumcodereview.appspot.com/12850002/diff/9001/chrome/browser/down...
chrome/browser/download/download_target_determiner.cc:176: virtual_path_ =
suggested_path;
On 2013/04/09 15:46:32, benjhayden_chromium wrote:
> Why not assign directly to virtual_path_ above instead of copying through
> suggested_path?

Done.

https://chromiumcodereview.appspot.com/12850002/diff/9001/chrome/browser/down...
chrome/browser/download/download_target_determiner.cc:534: // wanted to be
prompted for "all" downloads, or if this was a 'Save As'
On 2013/04/09 15:46:32, benjhayden_chromium wrote:
> What if the user wants to change the basename?

I don't intend to change the semantics of managed download paths in this CL.

https://chromiumcodereview.appspot.com/12850002/diff/9001/chrome/browser/down...
chrome/browser/download/download_target_determiner.cc:547: //    an extension is
installed, not when one is downloaded with "save as...".
On 2013/04/09 15:46:32, benjhayden_chromium wrote:
> Won't L552 prevent prompting if a user Saves As a .crx?

Save As downloads have TARGET_DISPOSITION_PROMPT set. They'll return early at
L542.

https://chromiumcodereview.appspot.com/12850002/diff/9001/chrome/browser/down...
File chrome/browser/download/download_target_determiner.h (right):

https://chromiumcodereview.appspot.com/12850002/diff/9001/chrome/browser/down...
chrome/browser/download/download_target_determiner.h:67: //   determining the
user's preferences regarding the default downloads
On 2013/04/09 15:46:32, benjhayden_chromium wrote:
> Would DTD be more modular if those strings were passed in?

It also checks for auto-open behavior, which isn't a simple string. I updated
the comment.

https://chromiumcodereview.appspot.com/12850002/diff/9001/chrome/browser/down...
chrome/browser/download/download_target_determiner.h:233: };
On 2013/04/09 15:46:32, benjhayden_chromium wrote:
> DISALLOW_COPY_AND_ASSIGN?

Done.

https://chromiumcodereview.appspot.com/12850002/diff/9001/chrome/browser/down...
File chrome/browser/download/download_target_determiner_delegate.h (right):

https://chromiumcodereview.appspot.com/12850002/diff/9001/chrome/browser/down...
chrome/browser/download/download_target_determiner_delegate.h:37: virtual
ExtensionDownloadsEventRouter* GetExtensionEventRouter() = 0;
On 2013/04/09 15:46:32, benjhayden_chromium wrote:
> Would DTD be more modular if Start() took a callback that dispatched
> chrome.downloads.onDeterminingFilename? Or could Start() take the EDER or the
> Profile/BrowserContext and go through DownloadService itself?

I'd like DTD to be unit testable and getting an EDER* currently doesn't allow it
to test the NotifyExtensions logic. Ideally we should be able to mock out EDER*.

For the moment, I've removed this delegate method and DTD is now looking up EDER
manually.

Passing a bunch of callbacks to Start() is semantically the same as passing a
delegate interface, which is what the CL was doing. Perhaps the delegate
interface should've been NotifyExtensions(..) instead of
GetExtensionEventRouter()?

https://chromiumcodereview.appspot.com/12850002/diff/9001/chrome/browser/down...
chrome/browser/download/download_target_determiner_delegate.h:39: virtual void
PromptUserForDownloadPath(
On 2013/04/09 15:46:32, benjhayden_chromium wrote:
> Why can't DTD do this itself?

Testability.

https://chromiumcodereview.appspot.com/12850002/diff/9001/chrome/browser/down...
chrome/browser/download/download_target_determiner_delegate.h:44: virtual void
DetermineLocalPath(content::DownloadItem* download,
On 2013/04/09 15:46:32, benjhayden_chromium wrote:
> Why can't DTD do this itself?

Testability.

https://chromiumcodereview.appspot.com/12850002/diff/9001/chrome/browser/down...
chrome/browser/download/download_target_determiner_delegate.h:48: virtual void
ReserveVirtualPath(content::DownloadItem* download,
On 2013/04/09 15:46:32, benjhayden_chromium wrote:
> Why can't DTD do this itself?

Testability.

https://chromiumcodereview.appspot.com/12850002/diff/9001/chrome/browser/down...
File chrome/browser/download/download_target_determiner_unittest.cc (right):

https://chromiumcodereview.appspot.com/12850002/diff/9001/chrome/browser/down...
chrome/browser/download/download_target_determiner_unittest.cc:106: // as
necessary for flagging the download with as a dangerous download of type
On 2013/04/09 15:46:32, benjhayden_chromium wrote:
> with as?

Done.

https://chromiumcodereview.appspot.com/12850002/diff/9001/chrome/browser/down...
chrome/browser/download/download_target_determiner_unittest.cc:117: const
base::FilePath::CharType*   forced_file_path;
On 2013/04/09 15:46:32, benjhayden_chromium wrote:
> What are all the spaces for?

This was moved from CDMD unit tests. These used to line up, but got moved around
when FilePath moved to the base namespace. I've removed the extra whitespace.

https://chromiumcodereview.appspot.com/12850002/diff/21002/chrome/browser/dow...
File chrome/browser/download/download_target_determiner.cc (right):

https://chromiumcodereview.appspot.com/12850002/diff/21002/chrome/browser/dow...
chrome/browser/download/download_target_determiner.cc:187: return COMPLETE;
On 2013/04/09 19:32:17, rdsmith wrote:
> What's the logic for having this only here?  This looks like trying to handle
a
> race with the callback from the DownloadFile::Initialize() method, and such a
> callback could come back in at any point.  If we don't expect that failure to
> happen very often, I'm inclined to think we shouldn't bother to handle the
race
> (why add racy complexity) in which case we could get rid of this.  If we do
> expect it to happen often, shouldn't we have it in DoLoop() or somewhere like
> that?  Though that might require more careful specification of what the
callback
> might say if we bail at different points.

This early return is there so that a download that's already interrupted can
still be used with DownloadTargetDeterminer (e.g. so that we can figure out a
filename to show the user). It's not trying to handle a race.

DTD doesn't assume that DownloadManagerDelegate::DetermineDownloadTarget() is
the only way that DTD can be invoked.

https://chromiumcodereview.appspot.com/12850002/diff/21002/chrome/browser/dow...
chrome/browser/download/download_target_determiner.cc:251:
DownloadTargetDeterminer::DoReserveVirtualPath() {
On 2013/04/09 19:32:17, rdsmith wrote:
> What are your conceptual thoughts about reserving paths in file systems that
may
> be shared between devices?  We can keep from colliding locally, but we can't
> really keep from colliding with what some other device is doing.  Does that
> imply any differences in how we approach reservation?

Reservation is supposed the check ...:
- Whether the target is writeable.
- Uniquify around known collisions.

All of these can change in a fleeting instant. We may lose tokens, thus
rendering the target unwriteable. Or someone could create a conflicting file
moments after we checked for collisions. This is true of the local file system
as well as a remote one since we don't control other processes on the same
system. We make decisions based on what's known in the present.

The only reliable way, I think, is to create a marker file with our target name
in the target directory. The complexity of managing the marker file, and the
security issues surrounding some filenames is the reason we went with a
reservation tracker. Has the tracker reached the level of complexity that it
makes sense to manage the marker file instead? I'm not sure. Perhaps we should
discuss this offline.

https://chromiumcodereview.appspot.com/12850002/diff/21002/chrome/browser/dow...
chrome/browser/download/download_target_determiner.cc:280: // We also prompt if
the download path couldn't be verified.
On 2013/04/09 19:32:17, rdsmith wrote:
> I was a bit confused by this (I thought it might be a todo).  Maybe
> "ReserveVirtualPathDone (above) will also have set this if the path couldn't
be
> verified."?

Removed the comment. It was stale.

https://chromiumcodereview.appspot.com/12850002/diff/21002/chrome/browser/dow...
chrome/browser/download/download_target_determiner.cc:350: #if
defined(FULL_SAFE_BROWSING)
On 2013/04/09 19:32:17, rdsmith wrote:
> Given that we have to have the ifdef anyway, I wonder if just duplicating the
> logic around s_b::DownloadProtectionService from the CDMD here rather than
> having the extra interface might make more sense?  Though maybe this is better
> for testing.  
> 
> (I continue to wish that a null SB service was indicated by having an
> implementation of the SB service that was null, rather than us having to test
> for null.  But there are probably good reasons against that.)

Discussed elsewhere.

https://chromiumcodereview.appspot.com/12850002/diff/21002/chrome/browser/dow...
chrome/browser/download/download_target_determiner.cc:434: if
(IsDangerousFile(false))
On 2013/04/09 19:32:17, rdsmith wrote:
> I'm not clear it's worth it, but you could refactor this to reduce history
pings
> by a little bit more.  visited_referrer_before is only relevant if the file
type
> is AllowOnUserGesture.  This does reduce flexibility of changing how we decide
> on danger in the future, though.

I decided to only ping the history thread if (!IsDangerousFile(true) &&
IsDangerousFile(false)).

https://chromiumcodereview.appspot.com/12850002/diff/21002/chrome/browser/dow...
File chrome/browser/download/download_target_determiner.h (right):

https://chromiumcodereview.appspot.com/12850002/diff/21002/chrome/browser/dow...
chrome/browser/download/download_target_determiner.h:31: //     point.
On 2013/04/09 19:32:17, rdsmith wrote:
> I wince a little bit at this, in that restricting it to have a virtual mount
> point seems overly file-system centric.  What entities need to understand a
> virtual path?  Is it worthwhile considering making it a URL?

Addressed elsewhere.

https://chromiumcodereview.appspot.com/12850002/diff/21002/chrome/browser/dow...
chrome/browser/download/download_target_determiner.h:73: // |delegate| is
required and must outlive the |download|.
On 2013/04/09 19:32:17, rdsmith wrote:
> In this practical situation (with the delegate being the CDMD) how is this
> guaranteed?

The actual lifetime requirement is that the delegate must live until |callback|
is invoked. I've updated the comment to reflect this. In this CL, |callback|
carries a reference to CDMD which accomplishes the goal.

https://chromiumcodereview.appspot.com/12850002/diff/21002/chrome/browser/dow...
chrome/browser/download/download_target_determiner.h:75: //   download| is
destroyed prior to that, then |callback| will be invoked as
On 2013/04/09 19:32:17, rdsmith wrote:
> nit: missing '|'

Done.

https://chromiumcodereview.appspot.com/12850002/diff/21002/chrome/browser/dow...
chrome/browser/download/download_target_determiner.h:75: //   download| is
destroyed prior to that, then |callback| will be invoked as
On 2013/04/09 19:32:17, rdsmith wrote:
> nit: missing '|'

Done.

https://chromiumcodereview.appspot.com/12850002/diff/21002/chrome/browser/dow...
chrome/browser/download/download_target_determiner.h:76: //   well. However the
paths will be empty.
On 2013/04/09 19:32:17, rdsmith wrote:
> Maybe make clear that the callback may be invoked after download destruction? 
> (I think that's what I'm seeing in the code because we end up posting a task
to
> the current message loop after we receive the OnDownloadDestroyed() message).

Done.

https://chromiumcodereview.appspot.com/12850002/diff/21002/chrome/browser/dow...
chrome/browser/download/download_target_determiner.h:111: // callback is
received.
On 2013/04/09 19:32:17, rdsmith wrote:
> I'd modify this comment to include the possibility that the DoLoop() has
already
> been invoked (in the case of synchronous execution of callback).  Maybe "If a
> handler returns this value, it has taken responsibility for ensuring that
> DoLoop() is invoked (and possibly has already invoked it)."
> 
> I'd also like some tag other than "PENDING" since that pretty clearly implies
> the execution of DoLoop() will be in the future, but I'm not coming up with a
> good alternative :-J.

Done.

https://chromiumcodereview.appspot.com/12850002/diff/21002/chrome/browser/dow...
chrome/browser/download/download_target_determiner.h:162: // -
STATE_DETERMINE_LOCAL_PATH : All other downloads.
On 2013/04/09 19:32:17, rdsmith wrote:
> (Thanks very much for doing these comments, by the way--it really helps.)
> 
> I'm confused; why do we skipping determining the local path if a prompt was
> shown to the user?  Do we only prompt for local paths?

The file picker returns both a virtual path and a local path.

https://chromiumcodereview.appspot.com/12850002/diff/21002/chrome/browser/dow...
chrome/browser/download/download_target_determiner.h:210: // Determins the
intermediate path. Once this step completes, downloads target
On 2013/04/09 19:32:17, rdsmith wrote:
> Worth mentioning that we implicitly assuming in doing this that the
intermediate
> file will be uniquified if needed when it hits the disk?

Done.

https://chromiumcodereview.appspot.com/12850002/diff/21002/chrome/browser/dow...
File chrome/browser/download/download_target_determiner_delegate.h (right):

https://chromiumcodereview.appspot.com/12850002/diff/21002/chrome/browser/dow...
chrome/browser/download/download_target_determiner_delegate.h:64: virtual
ExtensionDownloadsEventRouter* GetExtensionEventRouter() = 0;
On 2013/04/09 19:32:17, rdsmith wrote:
> Does this need to be a delegate routine?  It doesn't look like it's used in
CDMD
> after this refactor, so you could just pull the CDMD implementation into DTD. 
> It might make testing a bit trickier, but I'd be inclined to think that was
> worth it for a cleaner interface here.  

Moved it out of the delegate.

https://chromiumcodereview.appspot.com/12850002/diff/21002/chrome/browser/dow...
chrome/browser/download/download_target_determiner_delegate.h:68: virtual void
PromptUserForDownloadPath(
On 2013/04/09 19:32:17, rdsmith wrote:
> Same comment as above--this doesn't seem like it needs to be CDMD/DTDD based.

DownloadTestFileActivityObserver relies on being able to override the prompting
behavior within CDMD.

I introduced a DownloadFilePickerFactory for testing purposes. It's injected
when the DownloadTargetDeterminer is created. This allows overriding the
prompting behavior for testing without having to swap out CDMD.

[benjhayden, 2013-04-18 17:33:46]

I think that I would vote for finer grained delegate methods like
NotifyExtensions() in general so that DTD doesn't need to know about EDER,
history, file pickers, etc. and can focus on its FilePaths and other bits of
data and application logic. I think that it would make for a cleaner
architecture if CDMD contained less application logic and more glue code.
But I'm curious to see what Randy thinks.

https://chromiumcodereview.appspot.com/12850002/diff/9001/chrome/browser/down...
File chrome/browser/download/download_target_determiner_delegate.h (right):

https://chromiumcodereview.appspot.com/12850002/diff/9001/chrome/browser/down...
chrome/browser/download/download_target_determiner_delegate.h:37: virtual
ExtensionDownloadsEventRouter* GetExtensionEventRouter() = 0;
On 2013/04/16 20:34:01, asanka wrote:
> On 2013/04/09 15:46:32, benjhayden_chromium wrote:
> > Would DTD be more modular if Start() took a callback that dispatched
> > chrome.downloads.onDeterminingFilename? Or could Start() take the EDER or
the
> > Profile/BrowserContext and go through DownloadService itself?
> 
> I'd like DTD to be unit testable and getting an EDER* currently doesn't allow
it
> to test the NotifyExtensions logic. Ideally we should be able to mock out
EDER*.
> 
> For the moment, I've removed this delegate method and DTD is now looking up
EDER
> manually.
> 
> Passing a bunch of callbacks to Start() is semantically the same as passing a
> delegate interface, which is what the CL was doing. Perhaps the delegate
> interface should've been NotifyExtensions(..) instead of
> GetExtensionEventRouter()?

Yes, DTDD::NotifyExtensions() would provide better decoupling.

https://chromiumcodereview.appspot.com/12850002/diff/42001/chrome/browser/dow...
File chrome/browser/download/download_target_determiner.h (right):

https://chromiumcodereview.appspot.com/12850002/diff/42001/chrome/browser/dow...
chrome/browser/download/download_target_determiner.h:124: enum
PriorVisitsToReferrer {
Is there some reason to prefer an enum over a bool for this?

[asanka, 2013-04-24 23:16:30]

Can you take another look at the DownloadTargetDeterminerDelegate interface?
I've changed it to use finer grained methods as we talked about and also added
several tests that weren't possible earlier.

I removed DownloadFilePickerChromeOS as well since it's no longer necessary for
the file picker to do path substitution. It also simplifies the control flow in
DTD.

https://codereview.chromium.org/12850002/diff/42001/chrome/browser/download/d...
File chrome/browser/download/download_target_determiner.h (right):

https://codereview.chromium.org/12850002/diff/42001/chrome/browser/download/d...
chrome/browser/download/download_target_determiner.h:124: enum
PriorVisitsToReferrer {
On 2013/04/18 17:33:46, benjhayden_chromium wrote:
> Is there some reason to prefer an enum over a bool for this?

It's an argument to IsDangerousFile(). It turns this:

   IsDangerousFile(true)

To this:

   IsDangerousFile(VISITED_REFERRER)

I didn't think it was obvious what a boolean argument to IsDangerousFile() would
mean.

[benjhayden, 2013-04-25 17:16:04]

The approach looks good. Is there anything else you want to do to the CL before
I take a closer look?

https://codereview.chromium.org/12850002/diff/53001/chrome/browser/download/d...
File chrome/browser/download/download_target_determiner.cc (right):

https://codereview.chromium.org/12850002/diff/53001/chrome/browser/download/d...
chrome/browser/download/download_target_determiner.cc:365:
history_service->GetVisibleVisitCountToHost(
Would moving this out to the delegate be worth the extra testability?

https://codereview.chromium.org/12850002/diff/53001/chrome/browser/download/d...
File chrome/browser/download/download_target_determiner_delegate.h (right):

https://codereview.chromium.org/12850002/diff/53001/chrome/browser/download/d...
chrome/browser/download/download_target_determiner_delegate.h:75: // - If
|should_uniquify_path| is true, then |virtual_path| should be modified
s/should_uniquify_path/conflict_action/g

[Randy Smith (Not in Mondays), 2013-04-25 17:32:28]

On 2013/04/25 17:25:45, rdsmith wrote:

Whoops, but in Rietveld.  The new interface looks basically good to me; I'm glad
to hear about the tests, but am going to postpone reviewing them until I do the
detailed review (but ask me if you'd like me to do it now).

[asanka, 2013-04-25 19:08:26]

Thanks for the high level reviews. Since the approach looks okay I'd request a
detailed review.

https://codereview.chromium.org/12850002/diff/53001/chrome/browser/download/d...
File chrome/browser/download/download_target_determiner.cc (right):

https://codereview.chromium.org/12850002/diff/53001/chrome/browser/download/d...
chrome/browser/download/download_target_determiner.cc:365:
history_service->GetVisibleVisitCountToHost(
On 2013/04/25 17:16:04, benjhayden_chromium wrote:
> Would moving this out to the delegate be worth the extra testability?

The history service is currently testable. See the
DownloadTargetDeterminerTest.TargetDeterminer_VisitedReferrer test. Although I
could be convinced to move it out for consistency.

https://codereview.chromium.org/12850002/diff/53001/chrome/browser/download/d...
File chrome/browser/download/download_target_determiner_delegate.h (right):

https://codereview.chromium.org/12850002/diff/53001/chrome/browser/download/d...
chrome/browser/download/download_target_determiner_delegate.h:75: // - If
|should_uniquify_path| is true, then |virtual_path| should be modified
On 2013/04/25 17:16:04, benjhayden_chromium wrote:
> s/should_uniquify_path/conflict_action/g

Thanks. Fixed locally and will show up in the next round.

[benjhayden, 2013-04-26 15:07:37]

https://codereview.chromium.org/12850002/diff/53001/chrome/browser/download/c...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

https://codereview.chromium.org/12850002/diff/53001/chrome/browser/download/c...
chrome/browser/download/chrome_download_manager_delegate.cc:499:
callback.Run(virtual_path);
Is the callback necessary? This looks like a synchronous side-effect, aka
notification. It looks like the caller should be able to carry on its merry way
without waiting for CDMD.

https://codereview.chromium.org/12850002/diff/53001/chrome/browser/download/d...
File chrome/browser/download/download_file_picker.cc (right):

https://codereview.chromium.org/12850002/diff/53001/chrome/browser/download/d...
chrome/browser/download/download_file_picker.cc:68: download_id_(item->GetId())
{
Is this used anywhere?

https://codereview.chromium.org/12850002/diff/53001/chrome/browser/download/d...
File chrome/browser/download/download_target_determiner.cc (right):

https://codereview.chromium.org/12850002/diff/53001/chrome/browser/download/d...
chrome/browser/download/download_target_determiner.cc:40: // single bool. A host
is considered visited before if prior visible vists were
s/vists/visits/

https://codereview.chromium.org/12850002/diff/53001/chrome/browser/download/d...
chrome/browser/download/download_target_determiner.cc:181: if
(!download_->IsInProgress())
Why not do this before computing virtual_path_?

https://codereview.chromium.org/12850002/diff/53001/chrome/browser/download/d...
chrome/browser/download/download_target_determiner.cc:211: // TODO(asanka,
benjhayden): The suggested path may contain path fragments.
Remove this TODO since I have a pending CL to handle it?

https://codereview.chromium.org/12850002/diff/53001/chrome/browser/download/d...
chrome/browser/download/download_target_determiner.cc:266: if (should_prompt_) {
Why doesn't ReserveVirtualPathDone() skip past
STATE_PROMPT_USER_FOR_DOWNLOAD_PATH to STATE_DETERMINE_LOCAL_PATH if
!should_prompt?
It seems like it would make more sense for DoPromptUserForDownloadPath to always
actually do the prompting. "Do" is right there in the name.

https://codereview.chromium.org/12850002/diff/53001/chrome/browser/download/d...
chrome/browser/download/download_target_determiner.cc:309: local_path_ =
local_path;
Why set virtual_path_ *after* maybe calling CancelOnFailureAndDeleteSelf() but
set local_path_ *before*?

https://codereview.chromium.org/12850002/diff/53001/chrome/browser/download/d...
chrome/browser/download/download_target_determiner.cc:349: if (danger_type_ !=
content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS ||
Again, why not move this into CheckDownloadUrlDone and maybe skip
STATE_CHECK_VISITED_REFERRER_BEFORE altogether?

https://codereview.chromium.org/12850002/diff/53001/chrome/browser/download/d...
chrome/browser/download/download_target_determiner.cc:365:
history_service->GetVisibleVisitCountToHost(
On 2013/04/25 19:08:26, asanka wrote:
> On 2013/04/25 17:16:04, benjhayden_chromium wrote:
> > Would moving this out to the delegate be worth the extra testability?
> 
> The history service is currently testable. See the
> DownloadTargetDeterminerTest.TargetDeterminer_VisitedReferrer test. Although I
> could be convinced to move it out for consistency.

nm, this looks good.

https://codereview.chromium.org/12850002/diff/53001/chrome/browser/download/d...
chrome/browser/download/download_target_determiner.cc:377: if
(IsDangerousFile(NO_VISITS_TO_REFERRER))
Can you merge this with the previous if-block so that this IsDangerousFile call
only runs once?

if (IsDangerousFile(NO_VISITS_TO_REFERRER)) {
  if (IsDangerousFile(VISITED_REFERRER)) {
    ...
  } else {
    ...
  }
}

https://codereview.chromium.org/12850002/diff/53001/chrome/browser/download/d...
chrome/browser/download/download_target_determiner.cc:392:
DownloadTargetDeterminer::DoDetermineIntermediatePath() {
Should this line be indented like the others like it?

https://codereview.chromium.org/12850002/diff/53001/chrome/browser/download/d...
chrome/browser/download/download_target_determiner.cc:429: FILE_PATH_LITERAL("
%d.crdownload")).c_str(),
Should that string be a kConstant?

https://codereview.chromium.org/12850002/diff/53001/chrome/browser/download/d...
chrome/browser/download/download_target_determiner.cc:430: base::RandInt(0,
1000000));
kConstant = 1000000?

https://codereview.chromium.org/12850002/diff/53001/chrome/browser/download/d...
chrome/browser/download/download_target_determiner.cc:502: // Otherwise, don't
prompt.
Can you comment that we still could decide to prompt later if filename already
exists and conflict_action is PROMPT?

https://codereview.chromium.org/12850002/diff/53001/chrome/browser/download/d...
chrome/browser/download/download_target_determiner.cc:535: case
download_util::NotDangerous:
These enums look funny. Is this the right CL to fix them?

[asanka, 2013-04-26 17:02:21]

https://codereview.chromium.org/12850002/diff/53001/chrome/browser/download/c...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

https://codereview.chromium.org/12850002/diff/53001/chrome/browser/download/c...
chrome/browser/download/chrome_download_manager_delegate.cc:499:
callback.Run(virtual_path);
On 2013/04/26 15:07:37, benjhayden_chromium wrote:
> Is the callback necessary? This looks like a synchronous side-effect, aka
> notification. It looks like the caller should be able to carry on its merry
way
> without waiting for CDMD.

I don't follow. |callback| isn't specified by the caller of
OnDownloadPathSelected(). It's set by the caller of PromptUserForDownloadPath().
I.e. DownloadTargetDeterminer.

OnDownloadPathSelected() wraps DTD's callback for the purpose of capturing the
last selected download path.

https://codereview.chromium.org/12850002/diff/53001/chrome/browser/download/d...
File chrome/browser/download/download_file_picker.cc (right):

https://codereview.chromium.org/12850002/diff/53001/chrome/browser/download/d...
chrome/browser/download/download_file_picker.cc:68: download_id_(item->GetId())
{
On 2013/04/26 15:07:37, benjhayden_chromium wrote:
> Is this used anywhere?

Apparently not. Removed.

https://codereview.chromium.org/12850002/diff/53001/chrome/browser/download/d...
File chrome/browser/download/download_target_determiner.cc (right):

https://codereview.chromium.org/12850002/diff/53001/chrome/browser/download/d...
chrome/browser/download/download_target_determiner.cc:40: // single bool. A host
is considered visited before if prior visible vists were
On 2013/04/26 15:07:37, benjhayden_chromium wrote:
> s/vists/visits/

Done.

https://codereview.chromium.org/12850002/diff/53001/chrome/browser/download/d...
chrome/browser/download/download_target_determiner.cc:181: if
(!download_->IsInProgress())
On 2013/04/26 15:07:37, benjhayden_chromium wrote:
> Why not do this before computing virtual_path_?

The point is to be able to compute a suggested path for a download that's DOA.
The path will be needed for display purposes.

https://codereview.chromium.org/12850002/diff/53001/chrome/browser/download/d...
chrome/browser/download/download_target_determiner.cc:211: // TODO(asanka,
benjhayden): The suggested path may contain path fragments.
On 2013/04/26 15:07:37, benjhayden_chromium wrote:
> Remove this TODO since I have a pending CL to handle it?

Done.

https://codereview.chromium.org/12850002/diff/53001/chrome/browser/download/d...
chrome/browser/download/download_target_determiner.cc:266: if (should_prompt_) {
On 2013/04/26 15:07:37, benjhayden_chromium wrote:
> Why doesn't ReserveVirtualPathDone() skip past
> STATE_PROMPT_USER_FOR_DOWNLOAD_PATH to STATE_DETERMINE_LOCAL_PATH if
> !should_prompt?
> It seems like it would make more sense for DoPromptUserForDownloadPath to
always
> actually do the prompting. "Do" is right there in the name.

This is roughly following the pattern elsewhere the Do* is also responsible for
determining what to do and also the next state. Trying to skip over the PROMPT
state based on should_prompt_ would require ReserveVirtualPathDone:
* decide whether or not to prompt.
* know what the state is after prompting.

I think it's simpler to keep both the prompting decision and next state in
DoPromptUser*.

https://codereview.chromium.org/12850002/diff/53001/chrome/browser/download/d...
chrome/browser/download/download_target_determiner.cc:309: local_path_ =
local_path;
On 2013/04/26 15:07:37, benjhayden_chromium wrote:
> Why set virtual_path_ *after* maybe calling CancelOnFailureAndDeleteSelf() but
> set local_path_ *before*?

Done.

https://codereview.chromium.org/12850002/diff/53001/chrome/browser/download/d...
chrome/browser/download/download_target_determiner.cc:349: if (danger_type_ !=
content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS ||
On 2013/04/26 15:07:37, benjhayden_chromium wrote:
> Again, why not move this into CheckDownloadUrlDone and maybe skip
> STATE_CHECK_VISITED_REFERRER_BEFORE altogether?

As above.

https://codereview.chromium.org/12850002/diff/53001/chrome/browser/download/d...
chrome/browser/download/download_target_determiner.cc:377: if
(IsDangerousFile(NO_VISITS_TO_REFERRER))
On 2013/04/26 15:07:37, benjhayden_chromium wrote:
> Can you merge this with the previous if-block so that this IsDangerousFile
call
> only runs once?
> 
> if (IsDangerousFile(NO_VISITS_TO_REFERRER)) {
>   if (IsDangerousFile(VISITED_REFERRER)) {
>     ...
>   } else {
>     ...
>   }
> }

Done.

https://codereview.chromium.org/12850002/diff/53001/chrome/browser/download/d...
chrome/browser/download/download_target_determiner.cc:392:
DownloadTargetDeterminer::DoDetermineIntermediatePath() {
On 2013/04/26 15:07:37, benjhayden_chromium wrote:
> Should this line be indented like the others like it?

Done.

https://codereview.chromium.org/12850002/diff/53001/chrome/browser/download/d...
chrome/browser/download/download_target_determiner.cc:429: FILE_PATH_LITERAL("
%d.crdownload")).c_str(),
On 2013/04/26 15:07:37, benjhayden_chromium wrote:
> Should that string be a kConstant?

Done. I was just moving code around, but personally I prefer single use
unlocalized format strings to be literals. However this one is partly localized
and generated.

https://codereview.chromium.org/12850002/diff/53001/chrome/browser/download/d...
chrome/browser/download/download_target_determiner.cc:430: base::RandInt(0,
1000000));
On 2013/04/26 15:07:37, benjhayden_chromium wrote:
> kConstant = 1000000?

Done.

https://codereview.chromium.org/12850002/diff/53001/chrome/browser/download/d...
chrome/browser/download/download_target_determiner.cc:502: // Otherwise, don't
prompt.
On 2013/04/26 15:07:37, benjhayden_chromium wrote:
> Can you comment that we still could decide to prompt later if filename already
> exists and conflict_action is PROMPT?

Done.

https://codereview.chromium.org/12850002/diff/53001/chrome/browser/download/d...
chrome/browser/download/download_target_determiner.cc:535: case
download_util::NotDangerous:
On 2013/04/26 15:07:37, benjhayden_chromium wrote:
> These enums look funny. Is this the right CL to fix them?

I'll do a separate CL to fix them.

[benjhayden, 2013-04-26 17:25:55]

LGTM, one nit. Please wait for Randy.

https://codereview.chromium.org/12850002/diff/53001/chrome/browser/download/d...
File chrome/browser/download/download_target_determiner.cc (right):

https://codereview.chromium.org/12850002/diff/53001/chrome/browser/download/d...
chrome/browser/download/download_target_determiner.cc:181: if
(!download_->IsInProgress())
On 2013/04/26 17:02:21, asanka wrote:
> On 2013/04/26 15:07:37, benjhayden_chromium wrote:
> > Why not do this before computing virtual_path_?
> 
> The point is to be able to compute a suggested path for a download that's DOA.
> The path will be needed for display purposes.

Can you comment that this check happens after computing virtual_path_ because it
will be used for display purposes even if the download is DOA?

[Randy Smith (Not in Mondays), 2013-04-26 19:05:45]

I like it.  LGTM with the comment request below; the rest are up to you.

(Confession: I didn't review the tests; I figured one pair of eyes was enough
there.  Let me know if you want me to go over them too.)

https://codereview.chromium.org/12850002/diff/21002/chrome/browser/download/d...
File chrome/browser/download/download_target_determiner.cc (right):

https://codereview.chromium.org/12850002/diff/21002/chrome/browser/download/d...
chrome/browser/download/download_target_determiner.cc:187: return COMPLETE;
On 2013/04/16 20:34:01, asanka wrote:
> On 2013/04/09 19:32:17, rdsmith wrote:
> > What's the logic for having this only here?  This looks like trying to
handle
> a
> > race with the callback from the DownloadFile::Initialize() method, and such
a
> > callback could come back in at any point.  If we don't expect that failure
to
> > happen very often, I'm inclined to think we shouldn't bother to handle the
> race
> > (why add racy complexity) in which case we could get rid of this.  If we do
> > expect it to happen often, shouldn't we have it in DoLoop() or somewhere
like
> > that?  Though that might require more careful specification of what the
> callback
> > might say if we bail at different points.
> 
> This early return is there so that a download that's already interrupted can
> still be used with DownloadTargetDeterminer (e.g. so that we can figure out a
> filename to show the user). It's not trying to handle a race.
> 
> DTD doesn't assume that DownloadManagerDelegate::DetermineDownloadTarget() is
> the only way that DTD can be invoked.

Makes sense.  I don't find the comment clear for that purpose, though; it
doesn't talk about the DTD being used on a interrupted download, but (at least
to me) implies a race.  Could you make the use case a bit clearer?

https://codereview.chromium.org/12850002/diff/72001/chrome/browser/download/c...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

https://codereview.chromium.org/12850002/diff/72001/chrome/browser/download/c...
chrome/browser/download/chrome_download_manager_delegate.cc:494: void
ChromeDownloadManagerDelegate::OnDownloadPathSelected(
Any reason not to put this next to PromptUserForDownload?

https://codereview.chromium.org/12850002/diff/72001/chrome/browser/download/d...
File chrome/browser/download/download_file_picker.h (right):

https://codereview.chromium.org/12850002/diff/72001/chrome/browser/download/d...
chrome/browser/download/download_file_picker.h:61:
scoped_refptr<content::DownloadManager> download_manager_;
Suggestion: It looks as if this is only included to conditionalize UMA recording
of what the user chose on the the prompt for download preference.  Worthwhile to
shrink what's stored down to just that boolean?

https://codereview.chromium.org/12850002/diff/72001/chrome/browser/download/d...
File chrome/browser/download/download_target_determiner.h (right):

https://codereview.chromium.org/12850002/diff/72001/chrome/browser/download/d...
chrome/browser/download/download_target_determiner.h:203: // Determins the
intermediate path. Once this step completes, downloads target
nit: Determines

https://codereview.chromium.org/12850002/diff/72001/chrome/browser/download/d...
File chrome/browser/download/download_test_file_activity_observer.cc (right):

https://codereview.chromium.org/12850002/diff/72001/chrome/browser/download/d...
chrome/browser/download/download_test_file_activity_observer.cc:50: FROM_HERE,
base::Bind(callback, target_path));
Why the change?  I don't object, but it looks semantically null to me, and I
don't see the readability improvement.

[asanka, 2013-04-29 18:43:19]

Thanks for the reviews!

https://codereview.chromium.org/12850002/diff/53001/chrome/browser/download/d...
File chrome/browser/download/download_target_determiner.cc (right):

https://codereview.chromium.org/12850002/diff/53001/chrome/browser/download/d...
chrome/browser/download/download_target_determiner.cc:181: if
(!download_->IsInProgress())
On 2013/04/26 17:25:56, benjhayden_chromium wrote:
> On 2013/04/26 17:02:21, asanka wrote:
> > On 2013/04/26 15:07:37, benjhayden_chromium wrote:
> > > Why not do this before computing virtual_path_?
> > 
> > The point is to be able to compute a suggested path for a download that's
DOA.
> > The path will be needed for display purposes.
> 
> Can you comment that this check happens after computing virtual_path_ because
it
> will be used for display purposes even if the download is DOA?

Done.

https://codereview.chromium.org/12850002/diff/72001/chrome/browser/download/c...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

https://codereview.chromium.org/12850002/diff/72001/chrome/browser/download/c...
chrome/browser/download/chrome_download_manager_delegate.cc:494: void
ChromeDownloadManagerDelegate::OnDownloadPathSelected(
On 2013/04/26 19:05:46, rdsmith wrote:
> Any reason not to put this next to PromptUserForDownload?

Moved it next to PromptUserForDownloadPath(). I originally placed it here
because the DTD delegate methods were together.

https://codereview.chromium.org/12850002/diff/72001/chrome/browser/download/d...
File chrome/browser/download/download_file_picker.h (right):

https://codereview.chromium.org/12850002/diff/72001/chrome/browser/download/d...
chrome/browser/download/download_file_picker.h:61:
scoped_refptr<content::DownloadManager> download_manager_;
On 2013/04/26 19:05:46, rdsmith wrote:
> Suggestion: It looks as if this is only included to conditionalize UMA
recording
> of what the user chose on the the prompt for download preference.  Worthwhile
to
> shrink what's stored down to just that boolean?

Good suggestion! Done.

https://codereview.chromium.org/12850002/diff/72001/chrome/browser/download/d...
File chrome/browser/download/download_target_determiner.h (right):

https://codereview.chromium.org/12850002/diff/72001/chrome/browser/download/d...
chrome/browser/download/download_target_determiner.h:203: // Determins the
intermediate path. Once this step completes, downloads target
On 2013/04/26 19:05:46, rdsmith wrote:
> nit: Determines

Done.

https://codereview.chromium.org/12850002/diff/72001/chrome/browser/download/d...
File chrome/browser/download/download_test_file_activity_observer.cc (right):

https://codereview.chromium.org/12850002/diff/72001/chrome/browser/download/d...
chrome/browser/download/download_test_file_activity_observer.cc:50: FROM_HERE,
base::Bind(callback, target_path));
On 2013/04/26 19:05:46, rdsmith wrote:
> Why the change?  I don't object, but it looks semantically null to me, and I
> don't see the readability improvement.

Ah. This was an artifact of doing and undoing a few changes by hand :-/. Fixed.

[commit-bot: I haz the power, 2013-04-30 16:54:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/asanka@chromium.org/12850002/87001

[commit-bot: I haz the power, 2013-04-30 21:41:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/asanka@chromium.org/12850002/107001

[commit-bot: I haz the power, 2013-05-01 00:03:39]

Change committed as 197518