Reduced the lifetime of DownloadCreateInfo.

Part 2 of downloads data flow refactor.

Reduced the lifetime of DownloadCreateInfo. 

DownloadCreateInfo now only lives until DownloadFileManager::CreateDownloadFile(). It's contents are put into DownloadItem, and then only the download ID is passed around to get at the information. 

In addition, most of DownloadItem's members are grouped into sub-structures by functionality: 
- history_ 
Passed to the history data base, and includes the data relevant to that. 

- request_ 
Contains the information about the request, which is not present in the other structures. 

- state_ 
Contains data that is used to manage the download process. It is part of DownloadItem because it is specific to that item, but it could instead be passed around as an extra parameter. 

Some notes on the refactoring: 

- DownloadItem is only accessed in the UI thread. This means that the state_ structure needs to be passed by value to DownloadManager::CheckIfSuggestedPathExists(), modified there, and then passed by value again to DownloadManager::OnPathExistenceAvailable(), where it is copied bach into DownloadItem. 

BUG=78084, 77188
TEST=Passes all download unit tests.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=86319

[ahendrickson, 2011-05-12 20:25:40]

Here's the second CL for refactoring the downloads data flow!

[Paweł Hajdan Jr., 2011-05-13 08:47:20]

Sorry, I'm not convinced this is the right way (the split seems quite
arbitrary). Do we need the split to shorten the lifetime of DownloadCreateInfo?

[hendrickson_a, 2011-05-13 13:50:29]

On 2011/05/13 08:47:20, Paweł Hajdan Jr. wrote:
> Sorry, I'm not convinced this is the right way (the split seems quite
> arbitrary). Do we need the split to shorten the lifetime of
DownloadCreateInfo?

We need DownloadHistoryInfo to pass to the history DB.

Technically, it doesn't have to be in DownloadItem, but then we would have to
assemble it in download_manager.cc.  I think this way is better.

The manager status structure is likewise necessary, but it technically doesn't
have to be part of DownloadItem at all, at the cost of passing it along the
chain of function calls.

We don't need the request info to be a structure either, but it does make some
calls simpler.

[Randy Smith (Not in Mondays), 2011-05-16 19:50:54]

I'm in the tricky position of not caring a lot about the extra structures, but
caring a lot about getting some version of this CL into the code base soon, as
we've got a couple of other changes waiting on it.  So I'd like to propose a
compromise that involves taking my mild biases and amplifying them.  Hopefully
that means there's something in it for both of you to dislike :-}.  If some
compromise like this doesn't work, I'll probably make some arbitrary decision
and enforce it--we need to get moving in this area.

The principles on which I'd like to base my recommenation:
+ Class interfaces are one heck of a lot more important than implementation
(i.e. internal data members).
+ You don't want to expose class implementation in the interface.  This may mean
that, even though you have an internal structure/class that has exactly the
interface that you want, you delegate in your member functions rather than
allowing access directly to that class (1).
+ Returning a structure from a class is rarely the right interface; either it's
copied or its a reference to something which exists inside the class.  Instead,
when structures need to be created from classes, their constructor can take the
class and pull information from the class interface.

(1) Implementation inheritance is a way to get around this; I'm not sure how
kosher that is in Chrome, but I tend to dislike it for the same reasons that
Java outlawed it in the language.

So what I'd like to propose:
+ We get rid of the history_info(), request_info(), and manager_state()
accessors.
   	  
+ All information needed from internal structures in DownloadItem is exposed
directly via an accessor on the DownloadItem.
   	  
+ Structures that need to be created from the download item will take a download
item in their constructor.   If they're nested classes/structs of DownloadItem
(DownloadItem::X) they can be friends of DownloadItem and do "funky things" in
the constructor (e.g. copy directly from an internal instance of the same
class/struct).  Otherwise, they need to construct themselves the hard way (by
getting the values from the accessors).  (I'm not dead set on this one, but I'd
like either this pattern or one where you call a "void
get<Structure>(<Structure> *)" on the DownloadItem; returning a reference to the
internal object feels like blurring interface and implementation, and returning
a full copy goes gently against my understanding of Chrome style).  
   	  
+ DownloadHistoryInfo and DownloadState will be visible outside of DownloadItem;
DownloadHistoryInfo because it's how we'll be communicating with the history
system, and DownloadStateInfo because it's how we ping information off the file
thread for file determination.  
   	  
+ DownloadRequestInfo may stick around, but if so, it'll be only in the
implementation of DownloadItem. 

This allows us to shorten the lifetime of DownloadCreateInfo while keeping the
interface to DownloadItem (IMO) no worse than it currently is--it'll still be a
grab bag, with a few extra accessors required because of nuking
DownloadCreateInfo, but understanding that interface will not require
understanding several new structures/objects. 

Comments?

[Paweł Hajdan Jr., 2011-05-17 19:48:51]

On 2011/05/16 19:50:54, rdsmith wrote:
> The principles on which I'd like to base my recommenation:
> + Class interfaces are one heck of a lot more important than implementation
> (i.e. internal data members).

Agreed.

> + You don't want to expose class implementation in the interface.  This may
mean
> that, even though you have an internal structure/class that has exactly the
> interface that you want, you delegate in your member functions rather than
> allowing access directly to that class (1).

I don't think it should be applied as a rule. It may or may not result in
cleaner code, depending on the situation.

> + Returning a structure from a class is rarely the right interface; either
it's
> copied or its a reference to something which exists inside the class. 
Instead,
> when structures need to be created from classes, their constructor can take
the
> class and pull information from the class interface.

Similar to the above comment, but in this case I think it may be a step in the
right direction. The split into structs is not so obvious (to me), so just
passing individual members should make sense.

> (1) Implementation inheritance is a way to get around this; I'm not sure how
> kosher that is in Chrome, but I tend to dislike it for the same reasons that
> Java outlawed it in the language.

See
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Multip...

> So what I'd like to propose:
> + We get rid of the history_info(), request_info(), and manager_state()
> accessors.    	  
> + All information needed from internal structures in DownloadItem is exposed
> directly via an accessor on the DownloadItem.

Makes sense to me.

> + Structures that need to be created from the download item will take a
download
> item in their constructor.   If they're nested classes/structs of DownloadItem
> (DownloadItem::X) they can be friends of DownloadItem and do "funky things" in
> the constructor (e.g. copy directly from an internal instance of the same
> class/struct).  Otherwise, they need to construct themselves the hard way (by
> getting the values from the accessors).  (I'm not dead set on this one, but
I'd
> like either this pattern or one where you call a "void
> get<Structure>(<Structure> *)" on the DownloadItem; returning a reference to
the
> internal object feels like blurring interface and implementation, and
returning
> a full copy goes gently against my understanding of Chrome style).  

Please be careful with the above. Try to make the resulting code look and read
good, and not necessarily follow some hard rules.
   	  
> + DownloadHistoryInfo and DownloadState will be visible outside of
DownloadItem;
> DownloadHistoryInfo because it's how we'll be communicating with the history
> system, and DownloadStateInfo because it's how we ping information off the
file
> thread for file determination.  

I'm concerned about those classes, it seems that'd be creating too many of them.
I'd like to move towards less classes, and not more of them.
    	  
> + DownloadRequestInfo may stick around, but if so, it'll be only in the
> implementation of DownloadItem. 

Not sure why we want to have a class/struct named DownloadRequestInfo then.

> This allows us to shorten the lifetime of DownloadCreateInfo while keeping the
> interface to DownloadItem (IMO) no worse than it currently is--it'll still be
a
> grab bag, with a few extra accessors required because of nuking
> DownloadCreateInfo, but understanding that interface will not require
> understanding several new structures/objects. 

Reducing the lifetime of DownloadCreateInfo generally sounds like a good goal.
However, I think we should avoid doing potentially bad things for the sake of
very general rules or goals that just sound good. I'd love this change to
happen, but let's avoid introducing other potentially ugly or controversial
pieces to the download system.

[Randy Smith (Not in Mondays), 2011-05-17 20:06:51]

On 2011/05/17 19:48:51, Paweł Hajdan Jr. wrote:
> On 2011/05/16 19:50:54, rdsmith wrote:
> > + You don't want to expose class implementation in the interface.  This may
> mean
> > that, even though you have an internal structure/class that has exactly the
> > interface that you want, you delegate in your member functions rather than
> > allowing access directly to that class (1).
> 
> I don't think it should be applied as a rule. It may or may not result in
> cleaner code, depending on the situation.

If there are instances of breaking this rule that you're happy with, I think
I'll be happy with them--I put it in because I thought it was in line with some
of the objections you raised.  (I do believe in the general principle, but not
in a fanatical way--as I say, I'm mostly just proposing a compromise to get this
fix in place).  

> > (1) Implementation inheritance is a way to get around this; I'm not sure how
> > kosher that is in Chrome, but I tend to dislike it for the same reasons that
> > Java outlawed it in the language.
> 
> See
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Multip...

Yeah, I just see it all over the place :-}.  But maybe it's always n-1 interface
classes and one implementation class (but I doubt it; RefCountedThreadSafe<>
comes to mind).  But this isn't relevant to this discussion; I was just musing
on possible directions that I didn't want to follow.

> > + Structures that need to be created from the download item will take a
> download
> > item in their constructor.   If they're nested classes/structs of
DownloadItem
> > (DownloadItem::X) they can be friends of DownloadItem and do "funky things"
in
> > the constructor (e.g. copy directly from an internal instance of the same
> > class/struct).  Otherwise, they need to construct themselves the hard way
(by
> > getting the values from the accessors).  (I'm not dead set on this one, but
> I'd
> > like either this pattern or one where you call a "void
> > get<Structure>(<Structure> *)" on the DownloadItem; returning a reference to
> the
> > internal object feels like blurring interface and implementation, and
> returning
> > a full copy goes gently against my understanding of Chrome style).  
> 
> Please be careful with the above. Try to make the resulting code look and read
> good, and not necessarily follow some hard rules.

If you want to argue with me on this point for specific portions of the CL, I'll
yield in the name of getting the CL in, but I do think that this really is a
good rule.

> > + DownloadHistoryInfo and DownloadState will be visible outside of
> DownloadItem;
> > DownloadHistoryInfo because it's how we'll be communicating with the history
> > system, and DownloadStateInfo because it's how we ping information off the
> file
> > thread for file determination.  
> 
> I'm concerned about those classes, it seems that'd be creating too many of
them.
> I'd like to move towards less classes, and not more of them.

So I think this is the key point, and the place where I'm willing to go to the
wall.  You and I may need to accept that we simply disagree here.  We have, to
my mind, two choices:

* Keep DownloadCreateInfo, which is a pretty large grab bag that satisfies
several different purposes, alive for most of the lifetime of the download item
so that we can pass it to the file thread (for file determination) and the
history thread (for creation of a history entry).  

* Create two new structures that are very specifically targeted at those two
purposes with many many fewer members than DownloadCreateInfo, and limit
DownloadCreateInfo simply to the purpose of creating DownloadItems with
information from the IO thread.   I'll note that I expect DownloadStateInfo to
be further refined by the upcoming file determination refactor, which is
dependent on this CL.

I believe the second is clearly better, from a high level architectural point of
view.  It allows optimizations for each structure to proceed in parallel, and
with knowledge of a much smaller body of code, because the lifetime and hence
functions touching the structure is much more limited.  It also allows each
structure to have a conceptual unity that makes it easier to reason about. 
Another way to think about it is that more classes can be better, *if* the new
classes are all smaller than the original ones and can be reasoned about in
isolation from each other.  The key issue, to my mind, is how much information
someone needs when thinking about any particular body of code/bug.  I believe
this change will both reduce that amount of information across the downloads
system, and enable further reductions in the future.  

If you still disagree with me, and want to block the CL on that basis (because I
do think there's no way to do this CL without those two classes), we should
probably escalate.  Is there someone who's opinion you'd accept for an override
decision?  Maybe Brett?

> > + DownloadRequestInfo may stick around, but if so, it'll be only in the
> > implementation of DownloadItem. 
> 
> Not sure why we want to have a class/struct named DownloadRequestInfo then.

Yep.  I don't care a lot, but I also consider that what happens inside the
implementation of DownloadItem doesn't matter a whole lot.  Code cleanliness is
(to my mind) very much about interface cleanliness, and class internal
interfaces matter less than external ones.

[Paweł Hajdan Jr., 2011-05-18 19:39:01]

On 2011/05/17 20:06:51, rdsmith wrote:
> So I think this is the key point, and the place where I'm willing to go to the
> wall.  You and I may need to accept that we simply disagree here.  We have, to
> my mind, two choices:
> 
> * Keep DownloadCreateInfo, which is a pretty large grab bag that satisfies
> several different purposes, alive for most of the lifetime of the download
item
> so that we can pass it to the file thread (for file determination) and the
> history thread (for creation of a history entry).  
> 
> * Create two new structures that are very specifically targeted at those two
> purposes with many many fewer members than DownloadCreateInfo, and limit
> DownloadCreateInfo simply to the purpose of creating DownloadItems with
> information from the IO thread.   I'll note that I expect DownloadStateInfo to
> be further refined by the upcoming file determination refactor, which is
> dependent on this CL.

Right, reducing the lifetime of DownloadCreateInfo clearly sounds good. The main
issue is how the new classes/structs will look like. I'd like to reduce the
number of those new classes/structs unless there is _clearly_ need to keep
something separate.

> I believe the second is clearly better, from a high level architectural point
of
> view.  It allows optimizations for each structure to proceed in parallel, and
> with knowledge of a much smaller body of code, because the lifetime and hence
> functions touching the structure is much more limited.  It also allows each
> structure to have a conceptual unity that makes it easier to reason about. 

I think that what matters most for reasoning about code here is immutability and
good interface. Dividing things into too small pieces may make analysis harder.

I'm just wondering why don't we move everything to DownloadItem? I really see no
reason to create structs or classes just for grouping purposes.

> Another way to think about it is that more classes can be better, *if* the new
> classes are all smaller than the original ones and can be reasoned about in
> isolation from each other.

Agreed. Last time I looked though, they were often used together, possibly
negating this benefit.

> The key issue, to my mind, is how much information
> someone needs when thinking about any particular body of code/bug.  I believe
> this change will both reduce that amount of information across the downloads
> system, and enable further reductions in the future.  

Reducing the lifetime of DownloadCreateInfo would help here indeed. My only
concern is about the arbitrary split of the member variables and creating many
new structs.

I'd suggest to move everything to DownloadItem, and then try to reduce the
number of accessors.

> If you still disagree with me, and want to block the CL on that basis (because
I
> do think there's no way to do this CL without those two classes), we should
> probably escalate.  Is there someone who's opinion you'd accept for an
override
> decision?  Maybe Brett?

I think my concerns still apply to Patch Set 8. Feel free to add more reviewers
- I don't really have a preference here. I'll appreciate more feedback about
this CL.

> > > + DownloadRequestInfo may stick around, but if so, it'll be only in the
> > > implementation of DownloadItem. 
> > 
> > Not sure why we want to have a class/struct named DownloadRequestInfo then.
> 
> Yep.  I don't care a lot, but I also consider that what happens inside the
> implementation of DownloadItem doesn't matter a whole lot.  Code cleanliness
is
> (to my mind) very much about interface cleanliness, and class internal
> interfaces matter less than external ones.

I think that what happens inside DownloadItem also matters. This code just has a
long history of being hard to understand and buggy. I'd prefer to avoid leaving
any ugly places if possible.

[Randy Smith (Not in Mondays), 2011-05-18 19:47:09]

I've just pinged you with a request to chat, as it's sounding like we're not
making sense to each other, and interactivity might help.  But I'll try to just
address what I think is the highest level confusion between us: The need for the
new classes and why we can't use DownloadItem.  Two issues: Whether they exist
at all, and whether they are used to store information within DownloadItem.
* Whether they exist at all.  DownloadItem is a one-thread class: The UI thread.
 There are two places after its creation where we need to send data off-thread:
pinging the file thread for file determination, and pinging the history thread
for creating a history entry.  I'd like to have two structures for each of those
two purposes, targeted at those purposes.  The two alternatives are to send the
information as separate arguments (which strikes me as uglier than bundling into
a structure), or to keep DownloadCreateInfo around (which strikes me as *much*
uglier than bundling into a structure).
* Whether they're used inside of DownloadItem.  I don't really care.  I think
Andy would like them for conceptual data grouping, but if you feel strongly, I'm
willing to go the other direction.  

If we get agreement on this point, I can address the rest of your comments with
more confidence that we're not talking past each other :-}.


On 2011/05/18 19:39:01, Paweł Hajdan Jr. wrote:
> On 2011/05/17 20:06:51, rdsmith wrote:
> > So I think this is the key point, and the place where I'm willing to go to
the
> > wall.  You and I may need to accept that we simply disagree here.  We have,
to
> > my mind, two choices:
> > 
> > * Keep DownloadCreateInfo, which is a pretty large grab bag that satisfies
> > several different purposes, alive for most of the lifetime of the download
> item
> > so that we can pass it to the file thread (for file determination) and the
> > history thread (for creation of a history entry).  
> > 
> > * Create two new structures that are very specifically targeted at those two
> > purposes with many many fewer members than DownloadCreateInfo, and limit
> > DownloadCreateInfo simply to the purpose of creating DownloadItems with
> > information from the IO thread.   I'll note that I expect DownloadStateInfo
to
> > be further refined by the upcoming file determination refactor, which is
> > dependent on this CL.
> 
> Right, reducing the lifetime of DownloadCreateInfo clearly sounds good. The
main
> issue is how the new classes/structs will look like. I'd like to reduce the
> number of those new classes/structs unless there is _clearly_ need to keep
> something separate.
> 
> > I believe the second is clearly better, from a high level architectural
point
> of
> > view.  It allows optimizations for each structure to proceed in parallel,
and
> > with knowledge of a much smaller body of code, because the lifetime and
hence
> > functions touching the structure is much more limited.  It also allows each
> > structure to have a conceptual unity that makes it easier to reason about. 
> 
> I think that what matters most for reasoning about code here is immutability
and
> good interface. Dividing things into too small pieces may make analysis
harder.
> 
> I'm just wondering why don't we move everything to DownloadItem? I really see
no
> reason to create structs or classes just for grouping purposes.
> 
> > Another way to think about it is that more classes can be better, *if* the
new
> > classes are all smaller than the original ones and can be reasoned about in
> > isolation from each other.
> 
> Agreed. Last time I looked though, they were often used together, possibly
> negating this benefit.
> 
> > The key issue, to my mind, is how much information
> > someone needs when thinking about any particular body of code/bug.  I
believe
> > this change will both reduce that amount of information across the downloads
> > system, and enable further reductions in the future.  
> 
> Reducing the lifetime of DownloadCreateInfo would help here indeed. My only
> concern is about the arbitrary split of the member variables and creating many
> new structs.
> 
> I'd suggest to move everything to DownloadItem, and then try to reduce the
> number of accessors.
> 
> > If you still disagree with me, and want to block the CL on that basis
(because
> I
> > do think there's no way to do this CL without those two classes), we should
> > probably escalate.  Is there someone who's opinion you'd accept for an
> override
> > decision?  Maybe Brett?
> 
> I think my concerns still apply to Patch Set 8. Feel free to add more
reviewers
> - I don't really have a preference here. I'll appreciate more feedback about
> this CL.
> 
> > > > + DownloadRequestInfo may stick around, but if so, it'll be only in the
> > > > implementation of DownloadItem. 
> > > 
> > > Not sure why we want to have a class/struct named DownloadRequestInfo
then.
> > 
> > Yep.  I don't care a lot, but I also consider that what happens inside the
> > implementation of DownloadItem doesn't matter a whole lot.  Code cleanliness
> is
> > (to my mind) very much about interface cleanliness, and class internal
> > interfaces matter less than external ones.
> 
> I think that what happens inside DownloadItem also matters. This code just has
a
> long history of being hard to understand and buggy. I'd prefer to avoid
leaving
> any ugly places if possible.

[ahendrickson, 2011-05-18 20:14:15]

Removed DownloadRequestInfo, and updated per Randy's request.

Randy, let me know if I forgot anything, or did it differently from what you had
in mind.

Pawel, let me know what you think.

[Paweł Hajdan Jr., 2011-05-18 20:20:32]

I think it's going in a good direction.

I won't be able to do a full review today (sorry about that), but I'll do it
tomorrow (feel free to ping me early and often if I don't). Most of my original
concerns are no longer valid, and Randy explained to me that DownloadItem can
only be used on UI thread (that was one of the key points).

Please make sure all of the struct member variables are well commented.

http://codereview.chromium.org/6969009/diff/16001/chrome/browser/history/down...
File chrome/browser/history/download_create_info.h (right):

http://codereview.chromium.org/6969009/diff/16001/chrome/browser/history/down...
chrome/browser/history/download_create_info.h:25: struct DownloadHistoryInfo {
Could you extract this to its own header file? The point is to make it easy to
find.

[ahendrickson, 2011-05-18 23:17:51]

Updated with Randy's suggestions.

[ahendrickson, 2011-05-19 13:51:11]

Moved DownloadHistoryInfo to it's own set of files, and moved DownloadCreateInfo
from chrome/browser/history to chrome/browser/download, as the history system no
longer uses it.

[ahendrickson, 2011-05-19 13:51:38]

http://codereview.chromium.org/6969009/diff/16001/chrome/browser/history/down...
File chrome/browser/history/download_create_info.h (right):

http://codereview.chromium.org/6969009/diff/16001/chrome/browser/history/down...
chrome/browser/history/download_create_info.h:25: struct DownloadHistoryInfo {
On 2011/05/18 20:20:33, Paweł Hajdan Jr. wrote:
> Could you extract this to its own header file? The point is to make it easy to
> find.

Done.

[Paweł Hajdan Jr., 2011-05-19 16:18:25]

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/download/dow...
File chrome/browser/download/download_create_info.h (right):

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/download/dow...
chrome/browser/download/download_create_info.h:5: // Download creation struct
used for querying the history service.
Is it really primarily used for querying the history service? That comment seems
confusing.

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/download/dow...
chrome/browser/download/download_create_info.h:8: #define
CHROME_BROWSER_DOWNLOAD_DOWNLOAD_CREATE_INFO_H_
Is this file moved from chrome/browser/history? I don't see any
chrome/browser/history/download_create_info.h deletions in this CL.

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/download/dow...
chrome/browser/download/download_create_info.h:22: // want to pass DownloadItems
between threads. The history service also uses a
Let's describe the history service parts in HistoryService, especially the info
what the history service uses. That part of the comment is very likely to get
out of sync with the code.

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/download/dow...
chrome/browser/download/download_create_info.h:28: base::Time start_time,
nit: Why not const base::Time& for consistency with the above?

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/download/dow...
chrome/browser/download/download_create_info.h:37: // Indicates if the download
is dangerous.
nit: "Indicates" -> "Returns true". Why not const?

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/download/dow...
chrome/browser/download/download_create_info.h:46: // DownloadItem fields
Instead of saying DownloadItem fields, please make sure each member has a brief
description. Especially "int32 state" is a bit surprising.

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/download/dow...
chrome/browser/download/download_create_info.h:76: bool
prompt_user_for_save_location;
nit: Please add empty lines between member variables, this should be easier to
read.

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/download/dow...
File chrome/browser/download/download_item.cc (right):

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/download/dow...
chrome/browser/download/download_item.cc:626: DownloadItem::DownloadState
DownloadItem::state() const {
nit: This isn't a simple inlined accessor, could you rename it to GetState()?

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/download/dow...
chrome/browser/download/download_item.cc:632: return IN_PROGRESS;
I think it'd be simpler to write NOTREACHED() << history_info_.state inside the
"if" instead of the two DCHECKs above.

Also, the indentation of the return seems to be off.

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/download/dow...
chrome/browser/download/download_item.cc:637: const GURL& DownloadItem::url()
const {
nit: This isn't a simple inlined accessor, could you rename it to GetURL()?

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/download/dow...
File chrome/browser/download/download_item.h (right):

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/download/dow...
chrome/browser/download/download_item.h:92: struct DownloadStateInfo {
Why is it declared inside DownloadInfo? Is it possible to extract it to its own
file?

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/download/dow...
chrome/browser/download/download_item.h:383: DownloadStateInfo manager_state_;
nit: How about just state_info for consistency? I think it's also easier to
remember.

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/download/dow...
File chrome/browser/download/download_manager.cc (right):

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/download/dow...
chrome/browser/download/download_manager.cc:282: DownloadItem::DownloadStateInfo
state(*download);
This is strange. DownloadItem has a DownloadStateInfo member, and here we're
creating one based on the DownloadItem... it's weird. If needed, it's much more
straightforward to just have a DownloadStateInfo accessor in the DownloadItem.

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/download/dow...
chrome/browser/download/download_manager.cc:479: // |id_ptr| will be deleted in
either FileSelected() or
After thinking more about it - if we're using reinterpret_cast anyway, why do we
allocate a new int32 instead of just casting download_id to void*?

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/download/dow...
chrome/browser/download/download_manager.cc:518: if (!download)
If we return here, the DCHECK above should have fired, right? That means we
don't expect download to be NULL here and the NULL check should be removed, or
the DCHECK above should be removed/adjusted.

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/download/dow...
chrome/browser/download/download_manager.cc:1171:
DCHECK(ContainsKey(active_downloads_, download_id));
Is it possible to make GetDownloadItem and GetActiveDownloadItem more similar to
each other? The former uses a loop, and the latter just does a map lookup.

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/download/dow...
File chrome/browser/download/download_manager.h (right):

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/download/dow...
chrome/browser/download/download_manager.h:318: DownloadItem*
GetActiveDownloadItem(int id);
It is definitely worth to document what's the difference between those two
methods.

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/download/dow...
File chrome/browser/download/download_util.h (right):

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/download/dow...
chrome/browser/download/download_util.h:68: FilePath* generated_name);
Instead of having a return parameter and void return type, how about just
converting the return type to FilePath? Should be more natural.

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/download/dow...
chrome/browser/download/download_util.h:279: bool IsDangerous(const GURL& url,
This has a lot of parameters now, and two bool parameters next to each other,
very easy to confuse. Could you somehow reduce the number of parameters to
something more like 4?

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/history/down...
File chrome/browser/history/download_history_info.cc (right):

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/history/down...
chrome/browser/history/download_history_info.cc:7: #include
"chrome/browser/download/download_item.h"
If possible I'd like to avoid a dependency on download system here.

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/history/down...
File chrome/browser/history/download_history_info.h (right):

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/history/down...
chrome/browser/history/download_history_info.h:5: // Download creation struct
used for querying the history service.
Is "Download creation struct" applicable here?

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/history/down...
chrome/browser/history/download_history_info.h:19: // Contains the information
that is stored in the History database (or refers
nit: Make this comment say something about downloads.

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/history/down...
chrome/browser/history/download_history_info.h:22: DownloadHistoryInfo();
Why do we have 5 different ctors here? Could you reduce that number?

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/history/down...
chrome/browser/history/download_history_info.h:60: base::Time start_time;
nit: Please comment all member variables.

[Randy Smith (Not in Mondays), 2011-05-19 17:05:27]

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/download/dow...
File chrome/browser/download/download_create_info.h (right):

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/download/dow...
chrome/browser/download/download_create_info.h:1: // Copyright (c) 2011 The
Chromium Authors. All rights reserved.
I don't see the corresponding delete to this creation?

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/download/dow...
File chrome/browser/download/download_item.cc (right):

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/download/dow...
chrome/browser/download/download_item.cc:628: DCHECK_GE(INTERRUPTED,
history_info_.state);
I'd rather have these DCHECKs (and the comparisons below) be against 0 and
"MAX_DOWNLOAD_STATE" as I think those values are more likely to stay accurate (0
won't ever be modified, and MAX_DOWNLOAD_STATE will be at the end of the enum
and very clearly needing to stay at the end of it as the enum is modified.)

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/download/dow...
File chrome/browser/download/download_item.h (right):

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/download/dow...
chrome/browser/download/download_item.h:332: void set_path(const FilePath& path)
{ history_info_.path = path; }
I get nervous when we create new setters, specifically--it adds to the "bag of
bits" feeling of this structure.  Can you change this over to something like
"OnPathDetermined(const FilePath& path)" (i.e. something that indicates what is
happening in the lifecycle of the download)?  If there are any assertions about
that lifecycle that might be appropriate (download in progress, etc.) including
them would be good too.

Not a demand--it's not that bad.  But it's only used once, at one specific place
in the download process, and naming it that way would help call that out.

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/download/dow...
chrome/browser/download/download_item.h:335: bool IsDangerous() const { return
manager_state_.IsDangerous(); }
Why isn't this redundant with danger_type() == NOT_DANGEROUS?  I'm not sure it's
worth the extra accessor to save the callers a comparison.

More generally, now that we've got an abstraction barrier between inside and
outside of the DownloadItem, do we need to duplicate the dangerousness
information between the main structure and the manager_state_?

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/download/dow...
chrome/browser/download/download_item.h:338: void set_dangerous_url(bool
is_dangerous_url) {
I'm uncomfortable about this setter for the same reasons as above.  Maybe change
it to "MarkUrlDangerous" without an argument, and only call it if we've found a
dangerous URL?

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/download/dow...
File chrome/browser/download/download_manager.cc (right):

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/download/dow...
chrome/browser/download/download_manager.cc:282: DownloadItem::DownloadStateInfo
state(*download);
On 2011/05/19 16:18:25, Paweł Hajdan Jr. wrote:
> This is strange. DownloadItem has a DownloadStateInfo member, and here we're
> creating one based on the DownloadItem... it's weird. If needed, it's much
more
> straightforward to just have a DownloadStateInfo accessor in the DownloadItem.

This was at my request.  I'm ok with having an accessor, as long as the accessor
doesn't return a reference (implementation/interface confusion, worries about
lifetimes).  The accessor could also fill in a passed in structure if preferred.

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/download/dow...
chrome/browser/download/download_manager.cc:460: DownloadItem::DownloadStateInfo
state(*download);
At this point, is there any need for another object?  Can't you just pull things
from the download item via accessors for the rest of the routing?

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/download/dow...
chrome/browser/download/download_manager.cc:479: // |id_ptr| will be deleted in
either FileSelected() or
On 2011/05/19 16:18:25, Paweł Hajdan Jr. wrote:
> After thinking more about it - if we're using reinterpret_cast anyway, why do
we
> allocate a new int32 instead of just casting download_id to void*?

I thought about requesting this, and have no objections if you want to push for
it.  But I pinged someone more senior to me on the chrome team (cbentzel@) who
indicated that the int->void* cast was less Chroogley (:-}) than the allocation
and freeing.  Neither options awesome, so I'm cool with either.

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/download/dow...
chrome/browser/download/download_manager.cc:518: if (!download)
On 2011/05/19 16:18:25, Paweł Hajdan Jr. wrote:
> If we return here, the DCHECK above should have fired, right? That means we
> don't expect download to be NULL here and the NULL check should be removed, or
> the DCHECK above should be removed/adjusted.

Both the routines that call ContinueDownloadWithPath have the download pointer;
why not just change the method signature to take that?

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/download/dow...
chrome/browser/download/download_manager.cc:522:
DCHECK(download->full_path().empty());
Doesn't the drag-n-drop initiation set the full_path?

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/download/dow...
chrome/browser/download/download_manager.cc:1011: DCHECK_EQ(index, download_id);
Why are we expecting the index and the download_id to be the same?  I'm really
confused.

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/history/down...
File chrome/browser/history/download_history_info.cc (right):

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/history/down...
chrome/browser/history/download_history_info.cc:7: #include
"chrome/browser/download/download_item.h"
On 2011/05/19 16:18:25, Paweł Hajdan Jr. wrote:
> If possible I'd like to avoid a dependency on download system here.

That makes sense to me.  My guess is that the dependency is just for the
constructor based on DownloadItem, which was at my request.  I'm happy to accept
either:
* A DownloadItem::GetDownloadHistoryInfo(DownloadHistoryInfo* info) routine that
fills in the values, or
* The same routine with a DownloadHistoryInfo return value (note: Not a
reference return please).

[Paweł Hajdan Jr., 2011-05-19 19:11:24]

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/download/dow...
File chrome/browser/download/download_manager.cc (right):

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/download/dow...
chrome/browser/download/download_manager.cc:282: DownloadItem::DownloadStateInfo
state(*download);
On 2011/05/19 17:05:27, rdsmith wrote:
> On 2011/05/19 16:18:25, Paweł Hajdan Jr. wrote:
> > This is strange. DownloadItem has a DownloadStateInfo member, and here we're
> > creating one based on the DownloadItem... it's weird. If needed, it's much
> more
> > straightforward to just have a DownloadStateInfo accessor in the
DownloadItem.
> 
> This was at my request.  I'm ok with having an accessor, as long as the
accessor
> doesn't return a reference (implementation/interface confusion, worries about
> lifetimes).  The accessor could also fill in a passed in structure if
preferred.

I'd prefer an accessor that returns a copy then.

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/download/dow...
chrome/browser/download/download_manager.cc:479: // |id_ptr| will be deleted in
either FileSelected() or
On 2011/05/19 17:05:27, rdsmith wrote:
> On 2011/05/19 16:18:25, Paweł Hajdan Jr. wrote:
> > After thinking more about it - if we're using reinterpret_cast anyway, why
do
> we
> > allocate a new int32 instead of just casting download_id to void*?
> 
> I thought about requesting this, and have no objections if you want to push
for
> it.  But I pinged someone more senior to me on the chrome team (cbentzel@) who
> indicated that the int->void* cast was less Chroogley (:-}) than the
allocation
> and freeing.  Neither options awesome, so I'm cool with either.  

I code-searched the source, and it seems most invocations of SelectFile use NULL
as the last parameter. I wonder if we could do the same here (I just didn't
check how SelectFile really works).

There is at least one place that just casts an int or something, see
http://codesearch.google.com/codesearch/p#OAMlx_jo-ck/src/chrome/browser/ui/w...
(it's not really a good argument though).

I think both of the options here are not pretty, so I don't have a strong
preference. I'd be interested though why the int->void* cast is less Chroogley
than a dummy allocation.

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/history/down...
File chrome/browser/history/download_history_info.cc (right):

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/history/down...
chrome/browser/history/download_history_info.cc:7: #include
"chrome/browser/download/download_item.h"
On 2011/05/19 17:05:27, rdsmith wrote:
> On 2011/05/19 16:18:25, Paweł Hajdan Jr. wrote:
> > If possible I'd like to avoid a dependency on download system here.
> 
> That makes sense to me.  My guess is that the dependency is just for the
> constructor based on DownloadItem, which was at my request.  I'm happy to
accept
> either:
> * A DownloadItem::GetDownloadHistoryInfo(DownloadHistoryInfo* info) routine
that
> fills in the values, or
> * The same routine with a DownloadHistoryInfo return value (note: Not a
> reference return please).
> 

I'd prefer an accessor that returns a copy of DownloadHistoryInfo then.

[achuithb, 2011-05-19 19:47:36]

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/download/dow...
File chrome/browser/download/download_manager.cc (right):

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/download/dow...
chrome/browser/download/download_manager.cc:479: // |id_ptr| will be deleted in
either FileSelected() or
I think download_manager may be the only case of params being non-NULL. It would
be nice if we could get rid of it altogether. 

SelectFile launches a file picker dialog (When you rt-click and select Save As,
you get a file-picker dialog, for instance). In this case, I believe we're
allowing the user to choose the download location.

On 2011/05/19 19:11:24, Paweł Hajdan Jr. wrote:
> On 2011/05/19 17:05:27, rdsmith wrote:
> > On 2011/05/19 16:18:25, Paweł Hajdan Jr. wrote:
> > > After thinking more about it - if we're using reinterpret_cast anyway, why
> do
> > we
> > > allocate a new int32 instead of just casting download_id to void*?
> > 
> > I thought about requesting this, and have no objections if you want to push
> for
> > it.  But I pinged someone more senior to me on the chrome team (cbentzel@)
who
> > indicated that the int->void* cast was less Chroogley (:-}) than the
> allocation
> > and freeing.  Neither options awesome, so I'm cool with either.  
> 
> I code-searched the source, and it seems most invocations of SelectFile use
NULL
> as the last parameter. I wonder if we could do the same here (I just didn't
> check how SelectFile really works).
> 
> There is at least one place that just casts an int or something, see
>
http://codesearch.google.com/codesearch/p#OAMlx_jo-ck/src/chrome/browser/ui/w...
> (it's not really a good argument though).
> 
> I think both of the options here are not pretty, so I don't have a strong
> preference. I'd be interested though why the int->void* cast is less Chroogley
> than a dummy allocation.

[ahendrickson, 2011-05-19 20:16:49]

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/download/dow...
File chrome/browser/download/download_create_info.h (right):

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/download/dow...
chrome/browser/download/download_create_info.h:1: // Copyright (c) 2011 The
Chromium Authors. All rights reserved.
On 2011/05/19 17:05:27, rdsmith wrote:
> I don't see the corresponding delete to this creation?

That's odd.

On my machine, it detected that DownloadCreateInfo.* was renamed to
DownloadHistoryInfo.*.

I don't know why that didn't appear on the CL.

I may have to fix it by creating another CL that removes the originals, and
commit it after this one . . .

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/download/dow...
chrome/browser/download/download_create_info.h:5: // Download creation struct
used for querying the history service.
On 2011/05/19 16:18:25, Paweł Hajdan Jr. wrote:
> Is it really primarily used for querying the history service? That comment
seems
> confusing.

Fixed.

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/download/dow...
chrome/browser/download/download_create_info.h:8: #define
CHROME_BROWSER_DOWNLOAD_DOWNLOAD_CREATE_INFO_H_
On 2011/05/19 16:18:25, Paweł Hajdan Jr. wrote:
> Is this file moved from chrome/browser/history? I don't see any
> chrome/browser/history/download_create_info.h deletions in this CL.

Yes it is.

I'm not sure how to fix the CL, as it is correct on my machine.

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/download/dow...
chrome/browser/download/download_create_info.h:22: // want to pass DownloadItems
between threads. The history service also uses a
On 2011/05/19 16:18:25, Paweł Hajdan Jr. wrote:
> Let's describe the history service parts in HistoryService, especially the
info
> what the history service uses. That part of the comment is very likely to get
> out of sync with the code.

Done.

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/download/dow...
chrome/browser/download/download_create_info.h:28: base::Time start_time,
On 2011/05/19 16:18:25, Paweł Hajdan Jr. wrote:
> nit: Why not const base::Time& for consistency with the above?

Done.

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/download/dow...
chrome/browser/download/download_create_info.h:37: // Indicates if the download
is dangerous.
On 2011/05/19 16:18:25, Paweł Hajdan Jr. wrote:
> nit: "Indicates" -> "Returns true". Why not const?

Done.

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/download/dow...
chrome/browser/download/download_create_info.h:46: // DownloadItem fields
On 2011/05/19 16:18:25, Paweł Hajdan Jr. wrote:
> Instead of saying DownloadItem fields, please make sure each member has a
brief
> description. Especially "int32 state" is a bit surprising.

Done.

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/download/dow...
chrome/browser/download/download_create_info.h:76: bool
prompt_user_for_save_location;
On 2011/05/19 16:18:25, Paweł Hajdan Jr. wrote:
> nit: Please add empty lines between member variables, this should be easier to
> read.

Done.

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/download/dow...
File chrome/browser/download/download_item.cc (right):

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/download/dow...
chrome/browser/download/download_item.cc:626: DownloadItem::DownloadState
DownloadItem::state() const {
On 2011/05/19 16:18:25, Paweł Hajdan Jr. wrote:
> nit: This isn't a simple inlined accessor, could you rename it to GetState()?

Would you be willing to let me do this in a separate CL?  It will touch a lot of
files that aren't currently affected, due to all the observers.

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/download/dow...
chrome/browser/download/download_item.cc:628: DCHECK_GE(INTERRUPTED,
history_info_.state);
On 2011/05/19 17:05:27, rdsmith wrote:
> I'd rather have these DCHECKs (and the comparisons below) be against 0 and
> "MAX_DOWNLOAD_STATE" as I think those values are more likely to stay accurate
(0
> won't ever be modified, and MAX_DOWNLOAD_STATE will be at the end of the enum
> and very clearly needing to stay at the end of it as the enum is modified.)

Done.

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/download/dow...
chrome/browser/download/download_item.cc:632: return IN_PROGRESS;
On 2011/05/19 16:18:25, Paweł Hajdan Jr. wrote:
> I think it'd be simpler to write NOTREACHED() << history_info_.state inside
the
> "if" instead of the two DCHECKs above.
> 
> Also, the indentation of the return seems to be off.

Done.

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/download/dow...
chrome/browser/download/download_item.cc:637: const GURL& DownloadItem::url()
const {
On 2011/05/19 16:18:25, Paweł Hajdan Jr. wrote:
> nit: This isn't a simple inlined accessor, could you rename it to GetURL()?

Done.

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/download/dow...
File chrome/browser/download/download_item.h (right):

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/download/dow...
chrome/browser/download/download_item.h:92: struct DownloadStateInfo {
On 2011/05/19 16:18:25, Paweł Hajdan Jr. wrote:
> Why is it declared inside DownloadInfo? Is it possible to extract it to its
own
> file?

I've been having problems with creating new files . . .

I think this struct belongs in here, as it is not a member of any other struct
or class.

It is only used here and in download_manager.cc.

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/download/dow...
chrome/browser/download/download_item.h:332: void set_path(const FilePath& path)
{ history_info_.path = path; }
On 2011/05/19 17:05:27, rdsmith wrote:
> I get nervous when we create new setters, specifically--it adds to the "bag of
> bits" feeling of this structure.  Can you change this over to something like
> "OnPathDetermined(const FilePath& path)" (i.e. something that indicates what
is
> happening in the lifecycle of the download)?  If there are any assertions
about
> that lifecycle that might be appropriate (download in progress, etc.)
including
> them would be good too.
> 
> Not a demand--it's not that bad.  But it's only used once, at one specific
place
> in the download process, and naming it that way would help call that out.

Done.

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/download/dow...
chrome/browser/download/download_item.h:335: bool IsDangerous() const { return
manager_state_.IsDangerous(); }
On 2011/05/19 17:05:27, rdsmith wrote:
> Why isn't this redundant with danger_type() == NOT_DANGEROUS?  I'm not sure
it's
> worth the extra accessor to save the callers a comparison.
> 
> More generally, now that we've got an abstraction barrier between inside and
> outside of the DownloadItem, do we need to duplicate the dangerousness
> information between the main structure and the manager_state_?

Done.

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/download/dow...
chrome/browser/download/download_item.h:338: void set_dangerous_url(bool
is_dangerous_url) {
On 2011/05/19 17:05:27, rdsmith wrote:
> I'm uncomfortable about this setter for the same reasons as above.  Maybe
change
> it to "MarkUrlDangerous" without an argument, and only call it if we've found
a
> dangerous URL? 

Done.

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/download/dow...
chrome/browser/download/download_item.h:383: DownloadStateInfo manager_state_;
On 2011/05/19 16:18:25, Paweł Hajdan Jr. wrote:
> nit: How about just state_info for consistency? I think it's also easier to
> remember.

I called it |manager_state_| because it's information that the manager sets &
uses, on a per-item basis.  This distinguishes it from state internal to
DownloadItem.

A later refactor will probably get rid of this structure anyway.

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/download/dow...
File chrome/browser/download/download_manager.cc (right):

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/download/dow...
chrome/browser/download/download_manager.cc:282: DownloadItem::DownloadStateInfo
state(*download);
On 2011/05/19 19:11:24, Paweł Hajdan Jr. wrote:
> On 2011/05/19 17:05:27, rdsmith wrote:
> > On 2011/05/19 16:18:25, Paweł Hajdan Jr. wrote:
> > > This is strange. DownloadItem has a DownloadStateInfo member, and here
we're
> > > creating one based on the DownloadItem... it's weird. If needed, it's much
> > more
> > > straightforward to just have a DownloadStateInfo accessor in the
> DownloadItem.
> > 
> > This was at my request.  I'm ok with having an accessor, as long as the
> accessor
> > doesn't return a reference (implementation/interface confusion, worries
about
> > lifetimes).  The accessor could also fill in a passed in structure if
> preferred.
> 
> I'd prefer an accessor that returns a copy then.

Done.

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/download/dow...
chrome/browser/download/download_manager.cc:460: DownloadItem::DownloadStateInfo
state(*download);
On 2011/05/19 17:05:27, rdsmith wrote:
> At this point, is there any need for another object?  Can't you just pull
things
> from the download item via accessors for the rest of the routing?

I was trying to avoid adding more accessors to DownloadItem, but I guess I only
need to add one for suggested_path.

Done.

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/download/dow...
chrome/browser/download/download_manager.cc:479: // |id_ptr| will be deleted in
either FileSelected() or
On 2011/05/19 17:05:27, rdsmith wrote:
> On 2011/05/19 16:18:25, Paweł Hajdan Jr. wrote:
> > After thinking more about it - if we're using reinterpret_cast anyway, why
do
> we
> > allocate a new int32 instead of just casting download_id to void*?
> 
> I thought about requesting this, and have no objections if you want to push
for
> it.  But I pinged someone more senior to me on the chrome team (cbentzel@) who
> indicated that the int->void* cast was less Chroogley (:-}) than the
allocation
> and freeing.  Neither options awesome, so I'm cool with either.  

I actually tried this first, and got GCC complaints when I tried to cast the
void* back to an int (loss of data) in the callbacks.

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/download/dow...
chrome/browser/download/download_manager.cc:479: // |id_ptr| will be deleted in
either FileSelected() or
On 2011/05/19 19:11:24, Paweł Hajdan Jr. wrote:
> On 2011/05/19 17:05:27, rdsmith wrote:
> > On 2011/05/19 16:18:25, Paweł Hajdan Jr. wrote:
> > > After thinking more about it - if we're using reinterpret_cast anyway, why
> do
> > we
> > > allocate a new int32 instead of just casting download_id to void*?
> > 
> > I thought about requesting this, and have no objections if you want to push
> for
> > it.  But I pinged someone more senior to me on the chrome team (cbentzel@)
who
> > indicated that the int->void* cast was less Chroogley (:-}) than the
> allocation
> > and freeing.  Neither options awesome, so I'm cool with either.  
> 
> I code-searched the source, and it seems most invocations of SelectFile use
NULL
> as the last parameter. I wonder if we could do the same here (I just didn't
> check how SelectFile really works).
> 
> There is at least one place that just casts an int or something, see
>
http://codesearch.google.com/codesearch/p#OAMlx_jo-ck/src/chrome/browser/ui/w...
> (it's not really a good argument though).
> 
> I think both of the options here are not pretty, so I don't have a strong
> preference. I'd be interested though why the int->void* cast is less Chroogley
> than a dummy allocation.

NULL won't work, because we need the download ID in the callbacks.

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/download/dow...
chrome/browser/download/download_manager.cc:518: if (!download)
On 2011/05/19 16:18:25, Paweł Hajdan Jr. wrote:
> If we return here, the DCHECK above should have fired, right? That means we
> don't expect download to be NULL here and the NULL check should be removed, or
> the DCHECK above should be removed/adjusted.

Done.

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/download/dow...
chrome/browser/download/download_manager.cc:518: if (!download)
On 2011/05/19 17:05:27, rdsmith wrote:
> On 2011/05/19 16:18:25, Paweł Hajdan Jr. wrote:
> > If we return here, the DCHECK above should have fired, right? That means we
> > don't expect download to be NULL here and the NULL check should be removed,
or
> > the DCHECK above should be removed/adjusted.
> 
> Both the routines that call ContinueDownloadWithPath have the download
pointer;
> why not just change the method signature to take that?

Done.

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/download/dow...
chrome/browser/download/download_manager.cc:522:
DCHECK(download->full_path().empty());
On 2011/05/19 17:05:27, rdsmith wrote:
> Doesn't the drag-n-drop initiation set the full_path?  

I don't believe so; I think it sets save_info.

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/download/dow...
chrome/browser/download/download_manager.cc:1011: DCHECK_EQ(index, download_id);
On 2011/05/19 17:05:27, rdsmith wrote:
> Why are we expecting the index and the download_id to be the same?  I'm really
> confused.

Oops, my bad.  I was confusing it with the history callback.

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/download/dow...
chrome/browser/download/download_manager.cc:1171:
DCHECK(ContainsKey(active_downloads_, download_id));
On 2011/05/19 16:18:25, Paweł Hajdan Jr. wrote:
> Is it possible to make GetDownloadItem and GetActiveDownloadItem more similar
to
> each other? The former uses a loop, and the latter just does a map lookup.

I don't think so, because |history_downloads_| is indexed by the |db_handle|,
where |active_downloads_| is indexed by the download id.

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/download/dow...
File chrome/browser/download/download_manager.h (right):

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/download/dow...
chrome/browser/download/download_manager.h:318: DownloadItem*
GetActiveDownloadItem(int id);
On 2011/05/19 16:18:25, Paweł Hajdan Jr. wrote:
> It is definitely worth to document what's the difference between those two
> methods.

Done.

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/download/dow...
File chrome/browser/download/download_util.h (right):

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/download/dow...
chrome/browser/download/download_util.h:68: FilePath* generated_name);
On 2011/05/19 16:18:25, Paweł Hajdan Jr. wrote:
> Instead of having a return parameter and void return type, how about just
> converting the return type to FilePath? Should be more natural.

I think that's for another CL.

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/download/dow...
chrome/browser/download/download_util.h:279: bool IsDangerous(const GURL& url,
On 2011/05/19 16:18:25, Paweł Hajdan Jr. wrote:
> This has a lot of parameters now, and two bool parameters next to each other,
> very easy to confuse. Could you somehow reduce the number of parameters to
> something more like 4?

I used to have a DownloadRequestInfo structure that I passed in (replacing 3
parameters with 1), but I'm trying to minimize the new structures.

There's an additional complexity here to minimize complexity elsewhere.

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/history/down...
File chrome/browser/history/download_history_info.cc (right):

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/history/down...
chrome/browser/history/download_history_info.cc:7: #include
"chrome/browser/download/download_item.h"
On 2011/05/19 19:11:24, Paweł Hajdan Jr. wrote:
> On 2011/05/19 17:05:27, rdsmith wrote:
> > On 2011/05/19 16:18:25, Paweł Hajdan Jr. wrote:
> > > If possible I'd like to avoid a dependency on download system here.
> > 
> > That makes sense to me.  My guess is that the dependency is just for the
> > constructor based on DownloadItem, which was at my request.  I'm happy to
> accept
> > either:
> > * A DownloadItem::GetDownloadHistoryInfo(DownloadHistoryInfo* info) routine
> that
> > fills in the values, or
> > * The same routine with a DownloadHistoryInfo return value (note: Not a
> > reference return please).
> > 
> 
> I'd prefer an accessor that returns a copy of DownloadHistoryInfo then.

Done.

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/history/down...
File chrome/browser/history/download_history_info.h (right):

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/history/down...
chrome/browser/history/download_history_info.h:5: // Download creation struct
used for querying the history service.
On 2011/05/19 16:18:25, Paweł Hajdan Jr. wrote:
> Is "Download creation struct" applicable here?

Fixed.

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/history/down...
chrome/browser/history/download_history_info.h:19: // Contains the information
that is stored in the History database (or refers
On 2011/05/19 16:18:25, Paweł Hajdan Jr. wrote:
> nit: Make this comment say something about downloads.

Done.

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/history/down...
chrome/browser/history/download_history_info.h:22: DownloadHistoryInfo();
On 2011/05/19 16:18:25, Paweł Hajdan Jr. wrote:
> Why do we have 5 different ctors here? Could you reduce that number?

They are all being used, mostly by DownloadItem's constructors (3 of them).

Another one is used to get the history to pass to the DB.

The last is the default constructor.  I might be able to get rid of that one,
but I'm not sure.

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/history/down...
chrome/browser/history/download_history_info.h:60: base::Time start_time;
On 2011/05/19 16:18:25, Paweł Hajdan Jr. wrote:
> nit: Please comment all member variables.

Done.

[Paweł Hajdan Jr., 2011-05-20 09:04:41]

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/download/dow...
File chrome/browser/download/download_create_info.h (right):

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/download/dow...
chrome/browser/download/download_create_info.h:8: #define
CHROME_BROWSER_DOWNLOAD_DOWNLOAD_CREATE_INFO_H_
On 2011/05/19 20:16:49, ahendrickson wrote:
> On 2011/05/19 16:18:25, Paweł Hajdan Jr. wrote:
> > Is this file moved from chrome/browser/history? I don't see any
> > chrome/browser/history/download_create_info.h deletions in this CL.
> 
> Yes it is.
> 
> I'm not sure how to fix the CL, as it is correct on my machine.

I see, I suspected that kind of problem. Should be fine when committing, just
please make sure the old files really got deleted in svn.

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/download/dow...
File chrome/browser/download/download_item.cc (right):

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/download/dow...
chrome/browser/download/download_item.cc:626: DownloadItem::DownloadState
DownloadItem::state() const {
On 2011/05/19 20:16:49, ahendrickson wrote:
> On 2011/05/19 16:18:25, Paweł Hajdan Jr. wrote:
> > nit: This isn't a simple inlined accessor, could you rename it to
GetState()?
> 
> Would you be willing to let me do this in a separate CL?  It will touch a lot
of
> files that aren't currently affected, due to all the observers.

Yes, that makes sense. Please add a TODO in this CL to remember about this.

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/download/dow...
File chrome/browser/download/download_item.h (right):

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/download/dow...
chrome/browser/download/download_item.h:92: struct DownloadStateInfo {
On 2011/05/19 20:16:49, ahendrickson wrote:
> On 2011/05/19 16:18:25, Paweł Hajdan Jr. wrote:
> > Why is it declared inside DownloadInfo? Is it possible to extract it to its
> own
> > file?
> 
> I've been having problems with creating new files . . .

I'm not sure whether it's some local problem or some design issue... Could you
explain more?

> I think this struct belongs in here, as it is not a member of any other struct
> or class.

Right, but it's passed around. Even if it's just between DownloadItem and
DownloadManager, I think it's better to extract it.

> It is only used here and in download_manager.cc.

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/download/dow...
chrome/browser/download/download_item.h:383: DownloadStateInfo manager_state_;
On 2011/05/19 20:16:49, ahendrickson wrote:
> On 2011/05/19 16:18:25, Paweł Hajdan Jr. wrote:
> > nit: How about just state_info for consistency? I think it's also easier to
> > remember.
> 
> I called it |manager_state_| because it's information that the manager sets &
> uses, on a per-item basis.  This distinguishes it from state internal to
> DownloadItem.

I think that consistency is still more important. The comment has the necessary
details.

> A later refactor will probably get rid of this structure anyway.

Great - how about a TODO or some comment that would clearly indicate we'd like
to get rid of it?

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/download/dow...
File chrome/browser/download/download_manager.cc (right):

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/download/dow...
chrome/browser/download/download_manager.cc:479: // |id_ptr| will be deleted in
either FileSelected() or
On 2011/05/19 20:16:49, ahendrickson wrote:
> On 2011/05/19 17:05:27, rdsmith wrote:
> > On 2011/05/19 16:18:25, Paweł Hajdan Jr. wrote:
> > > After thinking more about it - if we're using reinterpret_cast anyway, why
> do
> > we
> > > allocate a new int32 instead of just casting download_id to void*?
> > 
> > I thought about requesting this, and have no objections if you want to push
> for
> > it.  But I pinged someone more senior to me on the chrome team (cbentzel@)
who
> > indicated that the int->void* cast was less Chroogley (:-}) than the
> allocation
> > and freeing.  Neither options awesome, so I'm cool with either.  
> 
> I actually tried this first, and got GCC complaints when I tried to cast the
> void* back to an int (loss of data) in the callbacks.

I see - hm, I think that means the current version has to be fine.

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/download/dow...
chrome/browser/download/download_manager.cc:479: // |id_ptr| will be deleted in
either FileSelected() or
On 2011/05/19 20:16:49, ahendrickson wrote:
> On 2011/05/19 19:11:24, Paweł Hajdan Jr. wrote:
> > On 2011/05/19 17:05:27, rdsmith wrote:
> > > On 2011/05/19 16:18:25, Paweł Hajdan Jr. wrote:
> > > > After thinking more about it - if we're using reinterpret_cast anyway,
why
> > do
> > > we
> > > > allocate a new int32 instead of just casting download_id to void*?
> > > 
> > > I thought about requesting this, and have no objections if you want to
push
> > for
> > > it.  But I pinged someone more senior to me on the chrome team (cbentzel@)
> who
> > > indicated that the int->void* cast was less Chroogley (:-}) than the
> > allocation
> > > and freeing.  Neither options awesome, so I'm cool with either.  
> > 
> > I code-searched the source, and it seems most invocations of SelectFile use
> NULL
> > as the last parameter. I wonder if we could do the same here (I just didn't
> > check how SelectFile really works).
> > 
> > There is at least one place that just casts an int or something, see
> >
>
http://codesearch.google.com/codesearch/p#OAMlx_jo-ck/src/chrome/browser/ui/w...
> > (it's not really a good argument though).
> > 
> > I think both of the options here are not pretty, so I don't have a strong
> > preference. I'd be interested though why the int->void* cast is less
Chroogley
> > than a dummy allocation.
> 
> NULL won't work, because we need the download ID in the callbacks.

I see, thank you for checking.

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/download/dow...
chrome/browser/download/download_manager.cc:1171:
DCHECK(ContainsKey(active_downloads_, download_id));
On 2011/05/19 20:16:49, ahendrickson wrote:
> On 2011/05/19 16:18:25, Paweł Hajdan Jr. wrote:
> > Is it possible to make GetDownloadItem and GetActiveDownloadItem more
similar
> to
> > each other? The former uses a loop, and the latter just does a map lookup.
> 
> I don't think so, because |history_downloads_| is indexed by the |db_handle|,
> where |active_downloads_| is indexed by the download id.

Ah, I see. Could you add a comment (probably in GetDownloadItem) about that?

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/download/dow...
File chrome/browser/download/download_util.h (right):

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/download/dow...
chrome/browser/download/download_util.h:68: FilePath* generated_name);
On 2011/05/19 20:16:49, ahendrickson wrote:
> On 2011/05/19 16:18:25, Paweł Hajdan Jr. wrote:
> > Instead of having a return parameter and void return type, how about just
> > converting the return type to FilePath? Should be more natural.
> 
> I think that's for another CL.

Okay, I don't want to increase the already high burden for this CL. Please just
add a TODO.

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/download/dow...
chrome/browser/download/download_util.h:279: bool IsDangerous(const GURL& url,
On 2011/05/19 20:16:49, ahendrickson wrote:
> On 2011/05/19 16:18:25, Paweł Hajdan Jr. wrote:
> > This has a lot of parameters now, and two bool parameters next to each
other,
> > very easy to confuse. Could you somehow reduce the number of parameters to
> > something more like 4?
> 
> I used to have a DownloadRequestInfo structure that I passed in (replacing 3
> parameters with 1), but I'm trying to minimize the new structures.
> 
> There's an additional complexity here to minimize complexity elsewhere.

Hmm, okay, I trust this judgment.

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/history/down...
File chrome/browser/history/download_history_info.h (right):

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/history/down...
chrome/browser/history/download_history_info.h:22: DownloadHistoryInfo();
On 2011/05/19 20:16:49, ahendrickson wrote:
> On 2011/05/19 16:18:25, Paweł Hajdan Jr. wrote:
> > Why do we have 5 different ctors here? Could you reduce that number?
> 
> They are all being used, mostly by DownloadItem's constructors (3 of them).
> 
> Another one is used to get the history to pass to the DB.
> 
> The last is the default constructor.  I might be able to get rid of that one,
> but I'm not sure.

Right, if they're here they're obviously used by some code. However, having too
many ctors is a bad sign. Please try to reduce their number by modifying the
callers' code. If that seems too complicated for now, please add a TODO and then
we should  shortly do a follow-up to fix it.

http://codereview.chromium.org/6969009/diff/26007/chrome/browser/download/dow...
File chrome/browser/download/download_create_info.h (right):

http://codereview.chromium.org/6969009/diff/26007/chrome/browser/download/dow...
chrome/browser/download/download_create_info.h:5: // Download creation struct
used for initializing the download information.
nit: I think this comment is very vague, and the comment right above the struct
is much better. Could you just remove this comment (line 5)?

http://codereview.chromium.org/6969009/diff/26007/chrome/browser/download/dow...
File chrome/browser/download/download_item.cc (right):

http://codereview.chromium.org/6969009/diff/26007/chrome/browser/download/dow...
chrome/browser/download/download_item.cc:116: : path_uniquifier(0),
nit: Shouldn't this be indented +4?

http://codereview.chromium.org/6969009/diff/26007/chrome/browser/download/dow...
chrome/browser/download/download_item.cc:620: // A DCHECK() will be fire if it
has an illegal value.
nit: This is an implementation comment, please remove.

http://codereview.chromium.org/6969009/diff/26007/chrome/browser/download/dow...
chrome/browser/download/download_item.cc:622: if ((history_info_.state < 0) ||
To make sure it works, could you explicitly initialize the first entry of the
enum to 0?

http://codereview.chromium.org/6969009/diff/26007/chrome/browser/download/dow...
chrome/browser/download/download_item.cc:623: (history_info_.state >
MAX_DOWNLOAD_STATE)) {
nit: Shouldn't it be >= MAX_DOWNLOAD_STATE? I think MAX_DOWNLOAD_STATE is an
invalid state, right?

http://codereview.chromium.org/6969009/diff/26007/chrome/browser/download/dow...
File chrome/browser/download/download_item.h (right):

http://codereview.chromium.org/6969009/diff/26007/chrome/browser/download/dow...
chrome/browser/download/download_item.h:114: FilePath suggested_path;
nit: Some member variables are still not commented (this is not the only one).

http://codereview.chromium.org/6969009/diff/26007/chrome/browser/download/dow...
chrome/browser/download/download_item.h:138: // Whether this download's file
name was specified initially.
nit: A "whether" applied to FilePath is very cumbersome. Please fix the comment,
and also try to use an unambiguous "True if" instead of "whether" (applies to
entire CL).

http://codereview.chromium.org/6969009/diff/26007/chrome/browser/download/dow...
chrome/browser/download/download_item.h:300: const GURL& GetUrl() const;
nit: Wouldn't GetURL be more consistent (capitalized URL)?

http://codereview.chromium.org/6969009/diff/26007/chrome/browser/download/dow...
chrome/browser/download/download_item.h:341: DownloadHistoryInfo history() const
{ return history_info_; }
nit: history_info() please, for consistency.

http://codereview.chromium.org/6969009/diff/26007/chrome/browser/download/dow...
chrome/browser/download/download_item.h:395: std::string mime_type_;
Now mime_type is a part of the structs we have. Can we get rid of the
duplication? If it's non-trivial, a TODO must be added.

http://codereview.chromium.org/6969009/diff/26007/chrome/browser/download/dow...
File chrome/browser/download/download_manager.cc (right):

http://codereview.chromium.org/6969009/diff/26007/chrome/browser/download/dow...
chrome/browser/download/download_manager.cc:490: reinterpret_cast<void
*>(id_ptr));
nit: Is it really "void *" and not "void*"? I think we keep the star "on the
left".

http://codereview.chromium.org/6969009/diff/26007/chrome/browser/download/dow...
chrome/browser/download/download_manager.cc:1011: int32* id_ptr =
reinterpret_cast<int32 *>(params);
nit: Is it really "int32 *" and not "int32*"? I think we keep the star "on the
left".

http://codereview.chromium.org/6969009/diff/26007/chrome/browser/download/dow...
chrome/browser/download/download_manager.cc:1033: int32* id_ptr =
reinterpret_cast<int32 *>(params);
nit: Is it really "int32 *" and not "int32*"? I think we keep the star "on the
left".

http://codereview.chromium.org/6969009/diff/26007/chrome/browser/download/dow...
File chrome/browser/download/download_manager.h (right):

http://codereview.chromium.org/6969009/diff/26007/chrome/browser/download/dow...
chrome/browser/download/download_manager.h:319: // been remove from the active
map, or was retrieved from the history DB.
nit: "remove" -> "removed"

[ahendrickson, 2011-05-20 18:31:24]

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/download/dow...
File chrome/browser/download/download_item.h (right):

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/download/dow...
chrome/browser/download/download_item.h:92: struct DownloadStateInfo {
On 2011/05/20 09:04:42, Paweł Hajdan Jr. wrote:
> On 2011/05/19 20:16:49, ahendrickson wrote:
> > On 2011/05/19 16:18:25, Paweł Hajdan Jr. wrote:
> > > Why is it declared inside DownloadInfo? Is it possible to extract it to
its
> > own
> > > file?
> > 
> > I've been having problems with creating new files . . .
> 
> I'm not sure whether it's some local problem or some design issue... Could you
> explain more?
> 
> > I think this struct belongs in here, as it is not a member of any other
struct
> > or class.
> 
> Right, but it's passed around. Even if it's just between DownloadItem and
> DownloadManager, I think it's better to extract it.
> 
> > It is only used here and in download_manager.cc.
> 

Extracted.

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/download/dow...
chrome/browser/download/download_item.h:383: DownloadStateInfo manager_state_;
On 2011/05/20 09:04:42, Paweł Hajdan Jr. wrote:
> On 2011/05/19 20:16:49, ahendrickson wrote:
> > On 2011/05/19 16:18:25, Paweł Hajdan Jr. wrote:
> > > nit: How about just state_info for consistency? I think it's also easier
to
> > > remember.
> > 
> > I called it |manager_state_| because it's information that the manager sets
&
> > uses, on a per-item basis.  This distinguishes it from state internal to
> > DownloadItem.
> 
> I think that consistency is still more important. The comment has the
necessary
> details.
> 
> > A later refactor will probably get rid of this structure anyway.
> 
> Great - how about a TODO or some comment that would clearly indicate we'd like
> to get rid of it?

Done.

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/download/dow...
File chrome/browser/download/download_manager.cc (right):

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/download/dow...
chrome/browser/download/download_manager.cc:1171:
DCHECK(ContainsKey(active_downloads_, download_id));
On 2011/05/20 09:04:42, Paweł Hajdan Jr. wrote:
> On 2011/05/19 20:16:49, ahendrickson wrote:
> > On 2011/05/19 16:18:25, Paweł Hajdan Jr. wrote:
> > > Is it possible to make GetDownloadItem and GetActiveDownloadItem more
> similar
> > to
> > > each other? The former uses a loop, and the latter just does a map lookup.
> > 
> > I don't think so, because |history_downloads_| is indexed by the
|db_handle|,
> > where |active_downloads_| is indexed by the download id.
> 
> Ah, I see. Could you add a comment (probably in GetDownloadItem) about that?

Done.

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/history/down...
File chrome/browser/history/download_history_info.h (right):

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/history/down...
chrome/browser/history/download_history_info.h:22: DownloadHistoryInfo();
On 2011/05/20 09:04:42, Paweł Hajdan Jr. wrote:
> On 2011/05/19 20:16:49, ahendrickson wrote:
> > On 2011/05/19 16:18:25, Paweł Hajdan Jr. wrote:
> > > Why do we have 5 different ctors here? Could you reduce that number?
> > 
> > They are all being used, mostly by DownloadItem's constructors (3 of them).
> > 
> > Another one is used to get the history to pass to the DB.
> > 
> > The last is the default constructor.  I might be able to get rid of that
one,
> > but I'm not sure.
> 
> Right, if they're here they're obviously used by some code. However, having
too
> many ctors is a bad sign. Please try to reduce their number by modifying the
> callers' code. If that seems too complicated for now, please add a TODO and
then
> we should  shortly do a follow-up to fix it.

Done.

http://codereview.chromium.org/6969009/diff/26007/chrome/browser/download/dow...
File chrome/browser/download/download_create_info.h (right):

http://codereview.chromium.org/6969009/diff/26007/chrome/browser/download/dow...
chrome/browser/download/download_create_info.h:5: // Download creation struct
used for initializing the download information.
On 2011/05/20 09:04:42, Paweł Hajdan Jr. wrote:
> nit: I think this comment is very vague, and the comment right above the
struct
> is much better. Could you just remove this comment (line 5)?

Done.

http://codereview.chromium.org/6969009/diff/26007/chrome/browser/download/dow...
File chrome/browser/download/download_item.cc (right):

http://codereview.chromium.org/6969009/diff/26007/chrome/browser/download/dow...
chrome/browser/download/download_item.cc:116: : path_uniquifier(0),
On 2011/05/20 09:04:42, Paweł Hajdan Jr. wrote:
> nit: Shouldn't this be indented +4?

Done.

http://codereview.chromium.org/6969009/diff/26007/chrome/browser/download/dow...
chrome/browser/download/download_item.cc:620: // A DCHECK() will be fire if it
has an illegal value.
On 2011/05/20 09:04:42, Paweł Hajdan Jr. wrote:
> nit: This is an implementation comment, please remove.

Done.

http://codereview.chromium.org/6969009/diff/26007/chrome/browser/download/dow...
chrome/browser/download/download_item.cc:622: if ((history_info_.state < 0) ||
On 2011/05/20 09:04:42, Paweł Hajdan Jr. wrote:
> To make sure it works, could you explicitly initialize the first entry of the
> enum to 0?

Done.

http://codereview.chromium.org/6969009/diff/26007/chrome/browser/download/dow...
chrome/browser/download/download_item.cc:623: (history_info_.state >
MAX_DOWNLOAD_STATE)) {
On 2011/05/20 09:04:42, Paweł Hajdan Jr. wrote:
> nit: Shouldn't it be >= MAX_DOWNLOAD_STATE? I think MAX_DOWNLOAD_STATE is an
> invalid state, right?

Done.

http://codereview.chromium.org/6969009/diff/26007/chrome/browser/download/dow...
File chrome/browser/download/download_item.h (right):

http://codereview.chromium.org/6969009/diff/26007/chrome/browser/download/dow...
chrome/browser/download/download_item.h:114: FilePath suggested_path;
On 2011/05/20 09:04:42, Paweł Hajdan Jr. wrote:
> nit: Some member variables are still not commented (this is not the only one).

Done.

http://codereview.chromium.org/6969009/diff/26007/chrome/browser/download/dow...
chrome/browser/download/download_item.h:138: // Whether this download's file
name was specified initially.
On 2011/05/20 09:04:42, Paweł Hajdan Jr. wrote:
> nit: A "whether" applied to FilePath is very cumbersome. Please fix the
comment,
> and also try to use an unambiguous "True if" instead of "whether" (applies to
> entire CL).

Done.

http://codereview.chromium.org/6969009/diff/26007/chrome/browser/download/dow...
chrome/browser/download/download_item.h:300: const GURL& GetUrl() const;
On 2011/05/20 09:04:42, Paweł Hajdan Jr. wrote:
> nit: Wouldn't GetURL be more consistent (capitalized URL)?

Done.

http://codereview.chromium.org/6969009/diff/26007/chrome/browser/download/dow...
chrome/browser/download/download_item.h:341: DownloadHistoryInfo history() const
{ return history_info_; }
On 2011/05/20 09:04:42, Paweł Hajdan Jr. wrote:
> nit: history_info() please, for consistency.

Done.

http://codereview.chromium.org/6969009/diff/26007/chrome/browser/download/dow...
chrome/browser/download/download_item.h:395: std::string mime_type_;
On 2011/05/20 09:04:42, Paweł Hajdan Jr. wrote:
> Now mime_type is a part of the structs we have. Can we get rid of the
> duplication? If it's non-trivial, a TODO must be added.

It is?  Other than DownloadCreateInfo, where is the duplicate?

http://codereview.chromium.org/6969009/diff/26007/chrome/browser/download/dow...
File chrome/browser/download/download_manager.cc (right):

http://codereview.chromium.org/6969009/diff/26007/chrome/browser/download/dow...
chrome/browser/download/download_manager.cc:490: reinterpret_cast<void
*>(id_ptr));
On 2011/05/20 09:04:42, Paweł Hajdan Jr. wrote:
> nit: Is it really "void *" and not "void*"? I think we keep the star "on the
> left".

Done.

http://codereview.chromium.org/6969009/diff/26007/chrome/browser/download/dow...
chrome/browser/download/download_manager.cc:1011: int32* id_ptr =
reinterpret_cast<int32 *>(params);
On 2011/05/20 09:04:42, Paweł Hajdan Jr. wrote:
> nit: Is it really "int32 *" and not "int32*"? I think we keep the star "on the
> left".

Done.

http://codereview.chromium.org/6969009/diff/26007/chrome/browser/download/dow...
chrome/browser/download/download_manager.cc:1033: int32* id_ptr =
reinterpret_cast<int32 *>(params);
On 2011/05/20 09:04:42, Paweł Hajdan Jr. wrote:
> nit: Is it really "int32 *" and not "int32*"? I think we keep the star "on the
> left".

Done.

http://codereview.chromium.org/6969009/diff/26007/chrome/browser/download/dow...
File chrome/browser/download/download_manager.h (right):

http://codereview.chromium.org/6969009/diff/26007/chrome/browser/download/dow...
chrome/browser/download/download_manager.h:319: // been remove from the active
map, or was retrieved from the history DB.
On 2011/05/20 09:04:42, Paweł Hajdan Jr. wrote:
> nit: "remove" -> "removed"

Done.

[Randy Smith (Not in Mondays), 2011-05-20 18:35:25]

Tossing this one comment out to reduce latency--about to do another full review.

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/history/down...
File chrome/browser/history/download_history_info.h (right):

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/history/down...
chrome/browser/history/download_history_info.h:22: DownloadHistoryInfo();
On 2011/05/19 20:16:49, ahendrickson wrote:
> On 2011/05/19 16:18:25, Paweł Hajdan Jr. wrote:
> > Why do we have 5 different ctors here? Could you reduce that number?
> 
> They are all being used, mostly by DownloadItem's constructors (3 of them).
> 
> Another one is used to get the history to pass to the DB.
> 
> The last is the default constructor.  I might be able to get rid of that one,
> but I'm not sure.

You can't get rid of the default constructor because we're using this in STL
containers.  However, I agree with Pawel that we really don't want may
constructors for this object.  Keep in mind that it's a structure, which is thus
intended to have visibility for all member variables; we really don't get much
encapsulation or abstraction out of having constructors.  

Let me ask a more basic question: Why does this structure have any elements
other than those required for the DB?  I'd like there to be a 1-1 correlation
between membership in this structure and what we put in the database; that's its
purpose in life, and putting more elements than that in taints that purpose. 
We'd then have two constructors: The default constructor, and the full (six
argument) constructor.  

(I hadn't actually realized it had extra elements in it--I'm afraid it's
important to me to cut those back.)

(If I were going to fully abstract this structure and turn it into a class, I'd
go a different direction:
* Constructor from a DownloadItem that used download item accessors.
* Constructor from an sql::Statement.
* Default constructor (for vectors & etc.).
And that'd be it.  But that's a copyable class, which is generally avoided in
Chrome.)

[Randy Smith (Not in Mondays), 2011-05-20 19:49:29]

Converging, hopefully not asymptotically :-}.

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/download/dow...
File chrome/browser/download/download_manager.cc (right):

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/download/dow...
chrome/browser/download/download_manager.cc:1171:
DCHECK(ContainsKey(active_downloads_, download_id));
On 2011/05/20 18:31:24, ahendrickson wrote:
> On 2011/05/20 09:04:42, Paweł Hajdan Jr. wrote:
> > On 2011/05/19 20:16:49, ahendrickson wrote:
> > > On 2011/05/19 16:18:25, Paweł Hajdan Jr. wrote:
> > > > Is it possible to make GetDownloadItem and GetActiveDownloadItem more
> > similar
> > > to
> > > > each other? The former uses a loop, and the latter just does a map
lookup.
> > > 
> > > I don't think so, because |history_downloads_| is indexed by the
> |db_handle|,
> > > where |active_downloads_| is indexed by the download id.
> > 
> > Ah, I see. Could you add a comment (probably in GetDownloadItem) about that?
> 
> Done.

Sorry to keep this thread going, but my scan of the file suggests that
everywhere we use GetDownloadItem we could, and should, use
GetActiveDownloadItem.  Could you check me on that, and if I'm right, nuke
GetDownloadItem?

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/download/dow...
File chrome/browser/download/download_util.h (right):

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/download/dow...
chrome/browser/download/download_util.h:279: bool IsDangerous(const GURL& url,
On 2011/05/19 20:16:49, ahendrickson wrote:
> On 2011/05/19 16:18:25, Paweł Hajdan Jr. wrote:
> > This has a lot of parameters now, and two bool parameters next to each
other,
> > very easy to confuse. Could you somehow reduce the number of parameters to
> > something more like 4?
> 
> I used to have a DownloadRequestInfo structure that I passed in (replacing 3
> parameters with 1), but I'm trying to minimize the new structures.
> 
> There's an additional complexity here to minimize complexity elsewhere.

Let me suggest an alternative (Pawel, your opinion is solicited).  This routine
is, as best I can tell, called from only one place:
DownloadManager::CheckDownloadUrlDone.  Given that I'd like to be moving stuff
out of download_util:: anyway, how about pulling it into DownloadManager?   I
believe that means we could collapse the seven args down to two: download, and
state; the other arguments come from the DownloadManager object and could be
folded in.  And we drop the number of functions in download_util by one, which
is a win.

Please keep the todo about tests; we really should write a unit test for this
function (would now go into download_manager_unittest.cc).

http://codereview.chromium.org/6969009/diff/28004/chrome/browser/download/dow...
File chrome/browser/download/download_create_info.h (right):

http://codereview.chromium.org/6969009/diff/28004/chrome/browser/download/dow...
chrome/browser/download/download_create_info.h:19: // Used for informing the
download database of a new download, where we don't
I think this comment is out of date.  Maybe "Used for creating a new download
item from information on the IO thread"?

http://codereview.chromium.org/6969009/diff/28004/chrome/browser/download/dow...
chrome/browser/download/download_create_info.h:34: bool IsDangerous() const;
Do we need this anymore?

http://codereview.chromium.org/6969009/diff/28004/chrome/browser/download/dow...
File chrome/browser/download/download_history.cc (right):

http://codereview.chromium.org/6969009/diff/28004/chrome/browser/download/dow...
chrome/browser/download/download_history.cc:45: DCHECK(download_item != NULL);
nit: Why not DCHECK(download_item), as is used in this CL in
DownloadFileManager?  (If there's some consistency issue between the two files,
that's fine--just checking.)

http://codereview.chromium.org/6969009/diff/28004/chrome/browser/download/dow...
File chrome/browser/download/download_item.h (right):

http://codereview.chromium.org/6969009/diff/28004/chrome/browser/download/dow...
chrome/browser/download/download_item.h:247: DownloadState state() const;  //
TODO(ahendrickson) -- Rename to GetState().
nit: Make the comment explain why.  The goal is, if you get hit by a bus or do a
Rip Van Winkle, someone else can figure out the context if they want to execute
the TODO.

http://codereview.chromium.org/6969009/diff/28004/chrome/browser/download/dow...
chrome/browser/download/download_item.h:259: std::string referrer_charset()
const { return referrer_charset_; }
This looks like it was added by this CL, but doesn't seem to me to be
conceptually part of the CL.  Did it creep in accidentally somewhere?

[Randy Smith (Not in Mondays), 2011-05-20 23:11:29]

I can't find anything more to ask for changes in :-}.  So: LGTM (with the nit
below).

I reserve the right to put my oar back in if Pawel brings up any interesting
issues, though :-}.

http://codereview.chromium.org/6969009/diff/28007/chrome/browser/download/dow...
File chrome/browser/download/download_manager.cc (right):

http://codereview.chromium.org/6969009/diff/28007/chrome/browser/download/dow...
chrome/browser/download/download_manager.cc:514: return;
This routine is only called from routines which have gotten the download from
active_downloads_, so I think this check is spurious (I don't mind the DCHECKS,
but this implies download_id might not be in active_downloads_ under expected
circumstances, which isn't true).

[ahendrickson, 2011-05-21 15:47:03]

http://codereview.chromium.org/6969009/diff/28007/chrome/browser/download/dow...
File chrome/browser/download/download_manager.cc (right):

http://codereview.chromium.org/6969009/diff/28007/chrome/browser/download/dow...
chrome/browser/download/download_manager.cc:514: return;
On 2011/05/20 23:11:29, rdsmith wrote:
> This routine is only called from routines which have gotten the download from
> active_downloads_, so I think this check is spurious (I don't mind the
DCHECKS,
> but this implies download_id might not be in active_downloads_ under expected
> circumstances, which isn't true).

Good point.

Changed to a DCHECK.

[Paweł Hajdan Jr., 2011-05-23 14:04:23]

LGTM

Please make sure that the commit message is accurate and that you have all
necessary OWNER approvals.

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/download/dow...
File chrome/browser/download/download_util.h (right):

http://codereview.chromium.org/6969009/diff/22009/chrome/browser/download/dow...
chrome/browser/download/download_util.h:279: bool IsDangerous(const GURL& url,
On 2011/05/20 19:49:29, rdsmith wrote:
> Let me suggest an alternative (Pawel, your opinion is solicited).  This
routine
> is, as best I can tell, called from only one place:
> DownloadManager::CheckDownloadUrlDone.  Given that I'd like to be moving stuff
> out of download_util:: anyway, how about pulling it into DownloadManager?   I
> believe that means we could collapse the seven args down to two: download, and
> state; the other arguments come from the DownloadManager object and could be
> folded in.  And we drop the number of functions in download_util by one, which
> is a win.
> 
> Please keep the todo about tests; we really should write a unit test for this
> function (would now go into download_manager_unittest.cc).

Sounds good to me.

http://codereview.chromium.org/6969009/diff/29003/chrome/browser/download/dow...
File chrome/browser/download/download_manager.cc (right):

http://codereview.chromium.org/6969009/diff/29003/chrome/browser/download/dow...
chrome/browser/download/download_manager.cc:1038: // TODO(erikkay,phajdan.jr):
This is apparently not being exercised in tests.
nit: Just TODO(phajdan.jr), more greppable and I don't think Erik is interested
in this.

http://codereview.chromium.org/6969009/diff/29003/chrome/browser/download/dow...
File chrome/browser/download/download_state_info.h (right):

http://codereview.chromium.org/6969009/diff/29003/chrome/browser/download/dow...
chrome/browser/download/download_state_info.h:4: //
nit: Remove this line.

[commit-bot: I haz the power, 2011-05-23 16:06:31]

Try job failure for 6969009-23031 on linux_clang:
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...

[commit-bot: I haz the power, 2011-05-23 17:40:20]

Try job failure for 6969009-38001 on linux_clang:
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...

[commit-bot: I haz the power, 2011-05-23 19:15:04]

Change committed as 86319

[Peter Kasting, 2011-05-24 00:58:25]

This change doesn't seem to have deleted download_xxx_info.* in
chrome/browser/history/.  Is that coming as a separate change?

[hendrickson_a, 2011-05-24 04:35:30]

Yeah, something broke with the CL mechanism, so I'm creating a new one to do
that.

Andy

On Mon, May 23, 2011 at 8:58 PM, <pkasting@chromium.org> wrote:

> This change doesn't seem to have deleted download_xxx_info.* in
> chrome/browser/history/.  Is that coming as a separate change?
>
>
> http://codereview.chromium.org/6969009/
>