[Resumption 9/11] Handle filename determination for resumed downloads.

[Downloads] Handle filename determination for resumed downloads.

Interrupted downloads that are being resumed will go through another
cycle of filename determination. This allows
ChromeDownloadManagerDelegate to properly handle path reservation and
Drive downloads during resumption.

TBR=joi
BUG=7648
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=203780

[Randy Smith (Not in Mondays), 2013-05-15 19:00:18]

Initial comments.  I'm a bit nervous about landing this functionality without
drive download info being persisted; it seems like it'll produce an inconsistent
user experience for drive downloads.

https://codereview.chromium.org/14640020/diff/2001/chrome/browser/chromeos/dr...
File chrome/browser/chromeos/drive/download_handler.cc (right):

https://codereview.chromium.org/14640020/diff/2001/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/download_handler.cc:251: break;
How does this work over a browser restart?  I.e. is the driver user data
persisted?

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

https://codereview.chromium.org/14640020/diff/2001/chrome/browser/download/do...
chrome/browser/download/download_target_determiner.cc:160: // (e.g. "foo (1)
(1).txt").
I consider this sub-ideal.  Are there other options?  A simple thought would be
that if we haven't previously prompted the user, we just discard the old name
and rederive a name.

https://codereview.chromium.org/14640020/diff/2001/content/browser/download/d...
File content/browser/download/download_item_impl.cc (right):

https://codereview.chromium.org/14640020/diff/2001/content/browser/download/d...
content/browser/download/download_item_impl.cc:1180: 
Why aren't we always using the current_path_ as our intermediate file in the
continuation case?  Is there not a clean way to code that?

[Randy Smith (Not in Mondays), 2013-05-15 19:02:13]

And an additional comment :-}.

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

https://codereview.chromium.org/14640020/diff/2001/chrome/browser/download/do...
chrome/browser/download/download_target_determiner.cc:77:
content::DOWNLOAD_INTERRUPT_REASON_NONE &&
This is relying on an aspect of the API that I don't think is intuitive, which
is that GetLastReason() persists through transitions into IN_PROGRESS.  In fact,
the doc on that in download_item.h indicates that it's only valid if the
download is interrupted.  Could you update the doc?

[asanka, 2013-05-23 20:30:37]

https://codereview.chromium.org/14640020/diff/2001/chrome/browser/chromeos/dr...
File chrome/browser/chromeos/drive/download_handler.cc (right):

https://codereview.chromium.org/14640020/diff/2001/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/download_handler.cc:251: break;
On 2013/05/15 19:00:18, rdsmith wrote:
> How does this work over a browser restart?  I.e. is the driver user data
> persisted?

On a browser restart, drive downloads get deleted due to the
DownloadHandler::OnDownloadCreated() above. The only resumption of interrupted
downloads we are handling are those that are resumed during the same browser
session they started in.

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

https://codereview.chromium.org/14640020/diff/2001/chrome/browser/download/do...
chrome/browser/download/download_target_determiner.cc:77:
content::DOWNLOAD_INTERRUPT_REASON_NONE &&
On 2013/05/15 19:02:13, rdsmith wrote:
> This is relying on an aspect of the API that I don't think is intuitive, which
> is that GetLastReason() persists through transitions into IN_PROGRESS.  In
fact,
> the doc on that in download_item.h indicates that it's only valid if the
> download is interrupted.  Could you update the doc?

Updated.

https://codereview.chromium.org/14640020/diff/2001/chrome/browser/download/do...
chrome/browser/download/download_target_determiner.cc:160: // (e.g. "foo (1)
(1).txt").
On 2013/05/15 19:00:18, rdsmith wrote:
> I consider this sub-ideal.  Are there other options?  A simple thought would
be
> that if we haven't previously prompted the user, we just discard the old name
> and rederive a name.

Yeah. I think that's better as well. Done.

https://codereview.chromium.org/14640020/diff/2001/content/browser/download/d...
File content/browser/download/download_item_impl.cc (right):

https://codereview.chromium.org/14640020/diff/2001/content/browser/download/d...
content/browser/download/download_item_impl.cc:1180: 
On 2013/05/15 19:00:18, rdsmith wrote:
> Why aren't we always using the current_path_ as our intermediate file in the
> continuation case?  Is there not a clean way to code that?

At this point the delegate has requested an intermediate path, so we accommodate
it.

I wanted to reuse the current_path_ in DownloadTargetDeterminer, but that
involved detecting if current_path_ was appropriate for the danger type. So I
decided to re-generate one instead.

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

https://codereview.chromium.org/14640020/diff/11001/chrome/browser/download/d...
chrome/browser/download/download_target_determiner.cc:374:
!download_->GetForcedFilePath().empty()) {
This logic was incorporated into IsDangerousFile()

[Randy Smith (Not in Mondays), 2013-05-24 01:53:55]

https://codereview.chromium.org/14640020/diff/2001/content/browser/download/d...
File content/browser/download/download_item_impl.cc (right):

https://codereview.chromium.org/14640020/diff/2001/content/browser/download/d...
content/browser/download/download_item_impl.cc:1180: 
On 2013/05/23 20:30:37, asanka wrote:
> On 2013/05/15 19:00:18, rdsmith wrote:
> > Why aren't we always using the current_path_ as our intermediate file in the
> > continuation case?  Is there not a clean way to code that?
> 
> At this point the delegate has requested an intermediate path, so we
accommodate
> it.
> 
> I wanted to reuse the current_path_ in DownloadTargetDeterminer, but that
> involved detecting if current_path_ was appropriate for the danger type. So I
> decided to re-generate one instead.

It just seems a bit weird to have this code here.  I think you'll mystify future
generations with it (well, ok, maybe "confuse" is a better adjective :-}). 
What's the use case under which the appropriateness of the current_path_ changes
during resumption?

https://codereview.chromium.org/14640020/diff/28001/chrome/browser/download/d...
File chrome/browser/download/download_target_determiner.cc (left):

https://codereview.chromium.org/14640020/diff/28001/chrome/browser/download/d...
chrome/browser/download/download_target_determiner.cc:352:
!download_->GetForcedFilePath().empty()) {
Sorry, confused.  Why remove the other two checks?

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

https://codereview.chromium.org/14640020/diff/28001/chrome/browser/download/d...
chrome/browser/download/download_target_determiner.cc:147:
DCHECK(!should_notify_extensions_);
Why is this a member variable rather than a local variable?  It looks like it's
just used in this routine.

https://codereview.chromium.org/14640020/diff/28001/chrome/browser/download/d...
chrome/browser/download/download_target_determiner.cc:159: // (e.g. "foo (1)
(1).txt").
Why is this comment still relevant?  If we prompted them, we're going to
overwrite, right?

https://codereview.chromium.org/14640020/diff/28001/chrome/browser/download/d...
chrome/browser/download/download_target_determiner.cc:190:
DCHECK(!should_prompt_);
nit: Feels a little redundant with identical DCHECK() at top of function.

https://codereview.chromium.org/14640020/diff/28001/chrome/browser/download/d...
chrome/browser/download/download_target_determiner.cc:523: DCHECK(is_resumption_
||
This portion of the DCHECK seems strange, since there was just a return on the
is_resumption_ test above.

https://codereview.chromium.org/14640020/diff/28001/chrome/browser/download/d...
chrome/browser/download/download_target_determiner.cc:570: // download.
Programmatic downloads are considered safe unless it contains
nit: "it contains" -> "they contain" (or "A programmatic download is")

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

https://codereview.chromium.org/14640020/diff/28001/chrome/browser/download/d...
chrome/browser/download/download_target_determiner.h:236: //
DownloadTargetDeterminer.
If I read the code right, this will return true after the prompt decision is
made but before the actual prompt is done, which makes me feel like the comment
and code aren't in sync ... ?

https://codereview.chromium.org/14640020/diff/28001/chrome/browser/download/d...
File chrome/browser/download/download_target_determiner_unittest.cc (right):

https://codereview.chromium.org/14640020/diff/28001/chrome/browser/download/d...
chrome/browser/download/download_target_determiner_unittest.cc:1511: for (size_t
i = 0; i < arraysize(kInitialPathTestCases); ++i) {
Why bother to have the test case loop if we're only going to do one of them?  Do
you anticipate adding more test cases to this test in the future?

[asanka, 2013-05-29 22:30:11]

https://codereview.chromium.org/14640020/diff/2001/content/browser/download/d...
File content/browser/download/download_item_impl.cc (right):

https://codereview.chromium.org/14640020/diff/2001/content/browser/download/d...
content/browser/download/download_item_impl.cc:1180: 
On 2013/05/24 01:53:55, rdsmith wrote:
> On 2013/05/23 20:30:37, asanka wrote:
> > On 2013/05/15 19:00:18, rdsmith wrote:
> > > Why aren't we always using the current_path_ as our intermediate file in
the
> > > continuation case?  Is there not a clean way to code that?
> > 
> > At this point the delegate has requested an intermediate path, so we
> accommodate
> > it.
> > 
> > I wanted to reuse the current_path_ in DownloadTargetDeterminer, but that
> > involved detecting if current_path_ was appropriate for the danger type. So
I
> > decided to re-generate one instead.
> 
> It just seems a bit weird to have this code here.  I think you'll mystify
future
> generations with it (well, ok, maybe "confuse" is a better adjective :-}). 
> What's the use case under which the appropriateness of the current_path_
changes
> during resumption?  

Can you explain why skipping a rename if the new name is the same as the old one
is "mystifying"?

The intermediate path could change if the user (or the embedder) chooses a
different target path during resumption because the original target path was out
of space. This would make a rename necessary and would also make it necessary
for content/ to anticipate a name change.

The danger level could change if the user was prompted during resumption when
they weren't initially prompted.

As an option, we could re-generate an intermediate path in chrome/ only if the
target directory changes.

https://codereview.chromium.org/14640020/diff/28001/chrome/browser/download/d...
File chrome/browser/download/download_target_determiner.cc (left):

https://codereview.chromium.org/14640020/diff/28001/chrome/browser/download/d...
chrome/browser/download/download_target_determiner.cc:352:
!download_->GetForcedFilePath().empty()) {
On 2013/05/24 01:53:55, rdsmith wrote:
> Sorry, confused.  Why remove the other two checks?

The checks were moved to IsDangerousFile().

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

https://codereview.chromium.org/14640020/diff/28001/chrome/browser/download/d...
chrome/browser/download/download_target_determiner.cc:147:
DCHECK(!should_notify_extensions_);
On 2013/05/24 01:53:55, rdsmith wrote:
> Why is this a member variable rather than a local variable?  It looks like
it's
> just used in this routine.

It's set here, but is read in DoNotifyExtensions()

https://codereview.chromium.org/14640020/diff/28001/chrome/browser/download/d...
chrome/browser/download/download_target_determiner.cc:159: // (e.g. "foo (1)
(1).txt").
On 2013/05/24 01:53:55, rdsmith wrote:
> Why is this comment still relevant?  If we prompted them, we're going to
> overwrite, right?

Yes. The comment was explaining why we aren't setting conflict_action_ to
UNIQUIFY in case someone feels like doing so in the future.

Comment removed.

https://codereview.chromium.org/14640020/diff/28001/chrome/browser/download/d...
chrome/browser/download/download_target_determiner.cc:190:
DCHECK(!should_prompt_);
On 2013/05/24 01:53:55, rdsmith wrote:
> nit: Feels a little redundant with identical DCHECK() at top of function.

Done.

https://codereview.chromium.org/14640020/diff/28001/chrome/browser/download/d...
chrome/browser/download/download_target_determiner.cc:523: DCHECK(is_resumption_
||
On 2013/05/24 01:53:55, rdsmith wrote:
> This portion of the DCHECK seems strange, since there was just a return on the
> is_resumption_ test above.

Done.

https://codereview.chromium.org/14640020/diff/28001/chrome/browser/download/d...
chrome/browser/download/download_target_determiner.cc:570: // download.
Programmatic downloads are considered safe unless it contains
On 2013/05/24 01:53:55, rdsmith wrote:
> nit: "it contains" -> "they contain" (or "A programmatic download is")

Done.

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

https://codereview.chromium.org/14640020/diff/28001/chrome/browser/download/d...
chrome/browser/download/download_target_determiner.h:236: //
DownloadTargetDeterminer.
On 2013/05/24 01:53:55, rdsmith wrote:
> If I read the code right, this will return true after the prompt decision is
> made but before the actual prompt is done, which makes me feel like the
comment
> and code aren't in sync ... ?

Done.

https://codereview.chromium.org/14640020/diff/28001/chrome/browser/download/d...
File chrome/browser/download/download_target_determiner_unittest.cc (right):

https://codereview.chromium.org/14640020/diff/28001/chrome/browser/download/d...
chrome/browser/download/download_target_determiner_unittest.cc:1511: for (size_t
i = 0; i < arraysize(kInitialPathTestCases); ++i) {
On 2013/05/24 01:53:55, rdsmith wrote:
> Why bother to have the test case loop if we're only going to do one of them? 
Do
> you anticipate adding more test cases to this test in the future?

Removed loop. It was supposed to contain multiple tests.

[Randy Smith (Not in Mondays), 2013-05-30 21:26:23]

LGTM with below comment request; you're welcome to use the boolean arg or not.

https://codereview.chromium.org/14640020/diff/2001/content/browser/download/d...
File content/browser/download/download_item_impl.cc (right):

https://codereview.chromium.org/14640020/diff/2001/content/browser/download/d...
content/browser/download/download_item_impl.cc:1180: 
On 2013/05/29 22:30:11, asanka wrote:
> On 2013/05/24 01:53:55, rdsmith wrote:
> > On 2013/05/23 20:30:37, asanka wrote:
> > > On 2013/05/15 19:00:18, rdsmith wrote:
> > > > Why aren't we always using the current_path_ as our intermediate file in
> the
> > > > continuation case?  Is there not a clean way to code that?
> > > 
> > > At this point the delegate has requested an intermediate path, so we
> > accommodate
> > > it.
> > > 
> > > I wanted to reuse the current_path_ in DownloadTargetDeterminer, but that
> > > involved detecting if current_path_ was appropriate for the danger type.
So
> I
> > > decided to re-generate one instead.
> > 
> > It just seems a bit weird to have this code here.  I think you'll mystify
> future
> > generations with it (well, ok, maybe "confuse" is a better adjective :-}). 
> > What's the use case under which the appropriateness of the current_path_
> changes
> > during resumption?  
> 
> Can you explain why skipping a rename if the new name is the same as the old
one
> is "mystifying"?

Well, no, but I *did* back off from calling it "mystifying" :-}.

(The confusing part was that, if looked at across resumptions, that we're
repeatedly doing the intermediate file rename, when you wouldn't think we'd need
to do it more than once.)

> The intermediate path could change if the user (or the embedder) chooses a
> different target path during resumption because the original target path was
out
> of space. This would make a rename necessary and would also make it necessary
> for content/ to anticipate a name change.

Gotcha; that makes sense.  I just wanted some comment about why we might need a
new intermediate rename.

> The danger level could change if the user was prompted during resumption when
> they weren't initially prompted.

Presumably the prompt might occur if the failure error suggested that a new
filesystem would be best, as above.  Ok.

> As an option, we could re-generate an intermediate path in chrome/ only if the
> target directory changes.

My goals were twofold: a) to understand the use cases (which I now do--thanks),
and b) to get a comment of some sort in here to reduce the potential future
confusion of people thinking the same thing I was.  Maybe "The intermediate name
may change from its original value during file name determination on resumption,
for example if the interruption reason resulting in a user prompt."?

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

https://codereview.chromium.org/14640020/diff/66001/chrome/browser/download/c...
chrome/browser/download/chrome_download_manager_delegate.cc:111: //  
PATH_FOR_TARGET_DETERMINATION: Always returns the target path.
Is there a reason not to make this an "always_return_target" boolean and move
the DCHECKs up a level?  I'm good with your preference, but how the consumer is
going to use it seems like Too Much Information for this routine.

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

https://codereview.chromium.org/14640020/diff/66001/chrome/browser/download/d...
chrome/browser/download/download_target_determiner.h:230: // target
determination is complete.
Just calling out that our tentative plan to restart cancelled downloads will
probably have to change this assumption.  No need to do anything about that in
this CL.

[asanka, 2013-05-31 19:23:58]

Could you take a quick peek at the delta from patchset 6?

https://codereview.chromium.org/14640020/diff/2001/content/browser/download/d...
File content/browser/download/download_item_impl.cc (right):

https://codereview.chromium.org/14640020/diff/2001/content/browser/download/d...
content/browser/download/download_item_impl.cc:1180: 
On 2013/05/30 21:26:24, rdsmith wrote:
> On 2013/05/29 22:30:11, asanka wrote:
> > On 2013/05/24 01:53:55, rdsmith wrote:
> > > On 2013/05/23 20:30:37, asanka wrote:
> > > > On 2013/05/15 19:00:18, rdsmith wrote:
> > > > > Why aren't we always using the current_path_ as our intermediate file
in
> > the
> > > > > continuation case?  Is there not a clean way to code that?
> > > > 
> > > > At this point the delegate has requested an intermediate path, so we
> > > accommodate
> > > > it.
> > > > 
> > > > I wanted to reuse the current_path_ in DownloadTargetDeterminer, but
that
> > > > involved detecting if current_path_ was appropriate for the danger type.
> So
> > I
> > > > decided to re-generate one instead.
> > > 
> > > It just seems a bit weird to have this code here.  I think you'll mystify
> > future
> > > generations with it (well, ok, maybe "confuse" is a better adjective :-}).

> > > What's the use case under which the appropriateness of the current_path_
> > changes
> > > during resumption?  
> > 
> > Can you explain why skipping a rename if the new name is the same as the old
> one
> > is "mystifying"?
> 
> Well, no, but I *did* back off from calling it "mystifying" :-}.
> 
> (The confusing part was that, if looked at across resumptions, that we're
> repeatedly doing the intermediate file rename, when you wouldn't think we'd
need
> to do it more than once.)
> 
> > The intermediate path could change if the user (or the embedder) chooses a
> > different target path during resumption because the original target path was
> out
> > of space. This would make a rename necessary and would also make it
necessary
> > for content/ to anticipate a name change.
> 
> Gotcha; that makes sense.  I just wanted some comment about why we might need
a
> new intermediate rename.
> 
> > The danger level could change if the user was prompted during resumption
when
> > they weren't initially prompted.
> 
> Presumably the prompt might occur if the failure error suggested that a new
> filesystem would be best, as above.  Ok.
> 
> > As an option, we could re-generate an intermediate path in chrome/ only if
the
> > target directory changes.
> 
> My goals were twofold: a) to understand the use cases (which I now
do--thanks),
> and b) to get a comment of some sort in here to reduce the potential future
> confusion of people thinking the same thing I was.  Maybe "The intermediate
name
> may change from its original value during file name determination on
resumption,
> for example if the interruption reason resulting in a user prompt."?
> 

Ah. Cool. I added a comment as suggested. I should have probably explained the
situation better.

I also added code to preserve a non-empty intermediate name if the download is
dangerous. This makes the assumption that downloads can become less dangerous on
resumption, but not the other way around. Also it avoid re-generating random
intermediate names for resumed dangerous downloads.

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

https://codereview.chromium.org/14640020/diff/66001/chrome/browser/download/c...
chrome/browser/download/chrome_download_manager_delegate.cc:111: //  
PATH_FOR_TARGET_DETERMINATION: Always returns the target path.
On 2013/05/30 21:26:24, rdsmith wrote:
> Is there a reason not to make this an "always_return_target" boolean and move
> the DCHECKs up a level?  I'm good with your preference, but how the consumer
is
> going to use it seems like Too Much Information for this routine.

Done.

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

https://codereview.chromium.org/14640020/diff/66001/chrome/browser/download/d...
chrome/browser/download/download_target_determiner.h:230: // target
determination is complete.
On 2013/05/30 21:26:24, rdsmith wrote:
> Just calling out that our tentative plan to restart cancelled downloads will
> probably have to change this assumption.  No need to do anything about that in
> this CL.

Yup.

[Randy Smith (Not in Mondays), 2013-05-31 21:18:19]

Diffs from PS6 LGTM.

https://codereview.chromium.org/14640020/diff/100001/chrome/browser/download/...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

https://codereview.chromium.org/14640020/diff/100001/chrome/browser/download/...
chrome/browser/download/chrome_download_manager_delegate.cc:114: // Drive
downloads always return the target path for all types.
To me, this comment is a code smell here: the consumer asks for a specific type
of path, and we silently override based on a very specific feature of the
download.  It suggests to me that the override determination should be made at a
higher level, probably based on whether we've virtualized the path.  I'm
disinclined towards changing it in this CL, but you might want to keep in the
back of your head the question of whether there's a better way to do it.

[asanka, 2013-05-31 21:58:40]

https://codereview.chromium.org/14640020/diff/100001/chrome/browser/download/...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

https://codereview.chromium.org/14640020/diff/100001/chrome/browser/download/...
chrome/browser/download/chrome_download_manager_delegate.cc:114: // Drive
downloads always return the target path for all types.
On 2013/05/31 21:18:19, rdsmith wrote:
> To me, this comment is a code smell here: the consumer asks for a specific
type
> of path, and we silently override based on a very specific feature of the
> download.  It suggests to me that the override determination should be made at
a
> higher level, probably based on whether we've virtualized the path.  I'm
> disinclined towards changing it in this CL, but you might want to keep in the
> back of your head the question of whether there's a better way to do it.

Yup. Thanks.

This CL doesn't change 'show in folder' behavior for Drive downloads, but you're
right that the API behavior is non-ideal.

[asanka, 2013-05-31 22:06:54]

+satorux: For chrome/browser/chromeos/drive/
+joi: TBR for comment change.

[Jói, 2013-06-02 09:49:07]

LGTM for //content/public.

[satorux1, 2013-06-03 00:58:50]

hashimoto would be the right person to review
chrome/browser/chromeos/drive/download_handler.cc

[hashimoto, 2013-06-03 06:31:03]

chrome/browser/chromeos/drive/download_handler.cc lgtm with nits, assuming you
tested the new code works as intended.

https://codereview.chromium.org/14640020/diff/100001/chrome/browser/chromeos/...
File chrome/browser/chromeos/drive/download_handler.cc (right):

https://codereview.chromium.org/14640020/diff/100001/chrome/browser/chromeos/...
chrome/browser/chromeos/drive/download_handler.cc:247: case
DownloadItem::INTERRUPTED:
nit: Please make this the same order as written in download_item.h.

https://codereview.chromium.org/14640020/diff/100001/chrome/browser/download/...
File chrome/browser/download/download_target_determiner.cc (right):

https://codereview.chromium.org/14640020/diff/100001/chrome/browser/download/...
chrome/browser/download/download_target_determiner.cc:30: #if
defined(OS_CHROMEOS)
nit: Is this include needed?

[asanka, 2013-06-03 18:35:31]

Thanks!

https://codereview.chromium.org/14640020/diff/100001/chrome/browser/chromeos/...
File chrome/browser/chromeos/drive/download_handler.cc (right):

https://codereview.chromium.org/14640020/diff/100001/chrome/browser/chromeos/...
chrome/browser/chromeos/drive/download_handler.cc:247: case
DownloadItem::INTERRUPTED:
On 2013/06/03 06:31:03, hashimoto wrote:
> nit: Please make this the same order as written in download_item.h.

Done.

https://codereview.chromium.org/14640020/diff/100001/chrome/browser/download/...
File chrome/browser/download/download_target_determiner.cc (right):

https://codereview.chromium.org/14640020/diff/100001/chrome/browser/download/...
chrome/browser/download/download_target_determiner.cc:30: #if
defined(OS_CHROMEOS)
On 2013/06/03 06:31:03, hashimoto wrote:
> nit: Is this include needed?

Apparently not. Thanks!

[commit-bot: I haz the power, 2013-06-03 20:38:10]

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

[commit-bot: I haz the power, 2013-06-03 21:55:47]

Change committed as 203780