Added member data to classes to support download resumption.

In order to resume a download some more information needs to be gathered.

This CL adds member data to classes to support this.  It is the first of several CLs to implement download resumption.

In addition hash information is propagated from DownloadFile to DownloadItem, because a new DownloadFile will be created when the download is resumed.  It will also need to be persisted in order to resume across chrome runs.

The plan is that data will be collected in DownloadResourceHandler::OnResponseStarted() and passed down to DownloadItem.

On resume, it will pass the information from the resuming DownloadItem to ResourceDispatcherHost::BeginDownload(), which will set any required headers.

BUG=7648
TEST=None


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

[Randy Smith (Not in Mondays), 2011-10-31 18:46:43]

So some high level comments.  These are all from the perspective of "interface
design", where DownloadCreateInfo is an interface, specifically the interface
between the IO and FILE threads:

* As noted below, I'd like to combine the concept of intermediate and final
hashes.  I don't think we need duplicate interfaces (and conceptual buckets) for
those two.  We just always have an ongoing hash.

* At the IO/FILE interface, I don't think we really want the idea of
continuing/restarting/resuming a download.  Instead, I'd suggest we go for the
next lower level.  A download request will either start at the beginning of a
remote resource or somewhere inside it (range request), and the file created to
correspond to a download will either be a new temp file whose name we don't care
about, or an existing file with contents where we want to pick up part way
through.  I think this is more flexible (imagine a future IDM-like feature
through which we could have many requests downloading many different parts of a
file at once).  So our "file description" would have "create new/add to
existing" with "add to existing" also including the offset at which to start
adding/overwriting and the hash for the section before that.   (This is all
related to my question about the use case for "continuing_download" below.)

There are some similar, related questions below.

http://codereview.chromium.org/8404049/diff/3014/content/browser/download/bas...
File content/browser/download/base_file.h (right):

http://codereview.chromium.org/8404049/diff/3014/content/browser/download/bas...
content/browser/download/base_file.h:69: bool GetPartialSha256Hash(std::string*
hash);
I'd like to look at combining the concepts of partial and total hash; just
provide "GetHash" methods, and whether they're partial or total depends on
whether the object on which they're being executed are partial or total.

http://codereview.chromium.org/8404049/diff/3014/content/browser/download/bas...
content/browser/download/base_file.h:74: bool SetPartialSha256Hash(const
std::string& hash);
Hmmm.  This feels like something that's relevant only on creation, and hence
should be part of the constructor.  (I could imagine--not sure it's a good
idea--putting something in debug only that on construction actually recomputed
the partial hash and checked to see if it was valid.)

http://codereview.chromium.org/8404049/diff/3014/content/browser/download/dow...
File content/browser/download/download_create_info.h (right):

http://codereview.chromium.org/8404049/diff/3014/content/browser/download/dow...
content/browser/download/download_create_info.h:91: std::string last_modified;
Why is this a string rather than a base::Time or something like it?

http://codereview.chromium.org/8404049/diff/3014/content/browser/download/dow...
content/browser/download/download_create_info.h:97: bool continued_download;
What's the use case for this field?  Would it make more sense to just pass the
offset from the beginning of the file of the data being received?  I could
imagine that as being more flexible for more purposes in the future (e.g. an IDM
like functionality that had multiple requests fetching different parts of the
file).

http://codereview.chromium.org/8404049/diff/3014/content/browser/download/dow...
content/browser/download/download_create_info.h:100: InterruptReason
server_interrupt_reason;
What's the use case for this field?  DownloadCreateInfo is for passing data from
IO->FILE/UI threads, and we already pass interruption reasons via
OnResponseCompleted.

http://codereview.chromium.org/8404049/diff/3014/content/browser/download/dow...
File content/browser/download/download_item.h (right):

http://codereview.chromium.org/8404049/diff/3014/content/browser/download/dow...
content/browser/download/download_item.h:164: void Update(int64 bytes_so_far,
const std::string& partial_hash);
Currently, Update is just used to send across size data for display purposes
based on a timer on the file thread--it isn't synchronous with OnReadCompleted
or anything.  |hash| isn't for display, so I don't think it makes sense to
include it in the update--it'll come across in OnAllDataSaved or Interrupted.

http://codereview.chromium.org/8404049/diff/3014/content/browser/download/dow...
File content/browser/download/download_resource_handler.cc (right):

http://codereview.chromium.org/8404049/diff/3014/content/browser/download/dow...
content/browser/download/download_resource_handler.cc:112: // If we're starting
a new download, get the last modified time and etag.
Why don't we get these all the time?  Why conditionalize?  My sense is that we
need them on both start and restart to compare against each other, but even if
we don't, the code's simpler if we always get them.

[ahendrickson, 2011-11-13 21:15:19]

http://codereview.chromium.org/8404049/diff/3014/content/browser/download/bas...
File content/browser/download/base_file.h (right):

http://codereview.chromium.org/8404049/diff/3014/content/browser/download/bas...
content/browser/download/base_file.h:69: bool GetPartialSha256Hash(std::string*
hash);
On 2011/10/31 18:46:43, rdsmith wrote:
> I'd like to look at combining the concepts of partial and total hash; just
> provide "GetHash" methods, and whether they're partial or total depends on
> whether the object on which they're being executed are partial or total.  

Done.

http://codereview.chromium.org/8404049/diff/3014/content/browser/download/bas...
content/browser/download/base_file.h:74: bool SetPartialSha256Hash(const
std::string& hash);
On 2011/10/31 18:46:43, rdsmith wrote:
> Hmmm.  This feels like something that's relevant only on creation, and hence
> should be part of the constructor.  (I could imagine--not sure it's a good
> idea--putting something in debug only that on construction actually recomputed
> the partial hash and checked to see if it was valid.)

I'd like to be able to set the partial hash in unit tests.

http://codereview.chromium.org/8404049/diff/3014/content/browser/download/dow...
File content/browser/download/download_create_info.h (right):

http://codereview.chromium.org/8404049/diff/3014/content/browser/download/dow...
content/browser/download/download_create_info.h:91: std::string last_modified;
On 2011/10/31 18:46:43, rdsmith wrote:
> Why is this a string rather than a base::Time or something like it?  

I wanted to return to the server the exact Last-Modified string that we received
in the initial download.  Will base::Time guarantee that it's the save after
converting back?  Or, can we guarantee that the formatting doesn't matter?

http://codereview.chromium.org/8404049/diff/3014/content/browser/download/dow...
content/browser/download/download_create_info.h:97: bool continued_download;
On 2011/10/31 18:46:43, rdsmith wrote:
> What's the use case for this field?  Would it make more sense to just pass the
> offset from the beginning of the file of the data being received?  I could
> imagine that as being more flexible for more purposes in the future (e.g. an
IDM
> like functionality that had multiple requests fetching different parts of the
> file).

We do have the offset for the data being received.  However, I can see
potentially wanting to distinguish multiple partial requests from a 'continue
download' scenario -- for example, in determining whether or not to
automatically resume.

I can remove this variable if you feel strongly about it.

http://codereview.chromium.org/8404049/diff/3014/content/browser/download/dow...
content/browser/download/download_create_info.h:100: InterruptReason
server_interrupt_reason;
On 2011/10/31 18:46:43, rdsmith wrote:
> What's the use case for this field?  DownloadCreateInfo is for passing data
from
> IO->FILE/UI threads, and we already pass interruption reasons via
> OnResponseCompleted.

This is so we can pass in interrupt reasons that are caused by response codes
from the server.

http://codereview.chromium.org/8404049/diff/3014/content/browser/download/dow...
File content/browser/download/download_item.h (right):

http://codereview.chromium.org/8404049/diff/3014/content/browser/download/dow...
content/browser/download/download_item.h:164: void Update(int64 bytes_so_far,
const std::string& partial_hash);
On 2011/10/31 18:46:43, rdsmith wrote:
> Currently, Update is just used to send across size data for display purposes
> based on a timer on the file thread--it isn't synchronous with OnReadCompleted
> or anything.  |hash| isn't for display, so I don't think it makes sense to
> include it in the update--it'll come across in OnAllDataSaved or Interrupted.

No, Update also sets received_bytes_, which is passed along to the persistent
store.  I want the hash to end up there as well.

Specifically, I want the hash to be periodically updated in the persistent
store, so we can use it when we resume a download after a crash (where we won't
necessarily record a download interruption).

Does that seem reasonable to you?

http://codereview.chromium.org/8404049/diff/3014/content/browser/download/dow...
File content/browser/download/download_resource_handler.cc (right):

http://codereview.chromium.org/8404049/diff/3014/content/browser/download/dow...
content/browser/download/download_resource_handler.cc:112: // If we're starting
a new download, get the last modified time and etag.
On 2011/10/31 18:46:43, rdsmith wrote:
> Why don't we get these all the time?  Why conditionalize?  My sense is that we
> need them on both start and restart to compare against each other, but even if
> we don't, the code's simpler if we always get them.

Done.

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

Thoughts on your responses to my initial pass.  My apologies--we're at a
conversation level with this CL, and I was treating it in a more heavyweight,
review mode.  I'll try to do a quicker response on it in the short-term (until
it gets down to code review level, at which point it'll start taking longer, and
the response latency won't matter as much).

http://codereview.chromium.org/8404049/diff/3014/content/browser/download/bas...
File content/browser/download/base_file.h (right):

http://codereview.chromium.org/8404049/diff/3014/content/browser/download/bas...
content/browser/download/base_file.h:74: bool SetPartialSha256Hash(const
std::string& hash);
On 2011/11/13 21:15:20, ahendrickson wrote:
> On 2011/10/31 18:46:43, rdsmith wrote:
> > Hmmm.  This feels like something that's relevant only on creation, and hence
> > should be part of the constructor.  (I could imagine--not sure it's a good
> > idea--putting something in debug only that on construction actually
recomputed
> > the partial hash and checked to see if it was valid.)
> 
> I'd like to be able to set the partial hash in unit tests.

Why can't that be done in the constructor?

http://codereview.chromium.org/8404049/diff/3014/content/browser/download/dow...
File content/browser/download/download_create_info.h (right):

http://codereview.chromium.org/8404049/diff/3014/content/browser/download/dow...
content/browser/download/download_create_info.h:91: std::string last_modified;
On 2011/11/13 21:15:20, ahendrickson wrote:
> On 2011/10/31 18:46:43, rdsmith wrote:
> > Why is this a string rather than a base::Time or something like it?  
> 
> I wanted to return to the server the exact Last-Modified string that we
received
> in the initial download.  Will base::Time guarantee that it's the save after
> converting back?  Or, can we guarantee that the formatting doesn't matter?

Would you do a little investigation of this issue?  I understand the
requirement, and it's probably needed, but I'm not ecstatic about it;
last_modified as a string smells wrong.  Maybe we want to abstract a bit more
and just pass in an (optional) conditional for the server, and format it on the
UI side?

http://codereview.chromium.org/8404049/diff/3014/content/browser/download/dow...
content/browser/download/download_create_info.h:97: bool continued_download;
On 2011/11/13 21:15:20, ahendrickson wrote:
> On 2011/10/31 18:46:43, rdsmith wrote:
> > What's the use case for this field?  Would it make more sense to just pass
the
> > offset from the beginning of the file of the data being received?  I could
> > imagine that as being more flexible for more purposes in the future (e.g. an
> IDM
> > like functionality that had multiple requests fetching different parts of
the
> > file).
> 
> We do have the offset for the data being received.  However, I can see
> potentially wanting to distinguish multiple partial requests from a 'continue
> download' scenario -- for example, in determining whether or not to
> automatically resume.

Sure, but I would imagine that distinction would be best done in management
code, i.e. in DownloadItem on the UI thread.  

> I can remove this variable if you feel strongly about it.

I feel pretty strongly about starting simple and adding complexity only when
it's needed, and it doesn't sound like there's a nailed down use case at the
moment?

http://codereview.chromium.org/8404049/diff/3014/content/browser/download/dow...
content/browser/download/download_create_info.h:100: InterruptReason
server_interrupt_reason;
On 2011/11/13 21:15:20, ahendrickson wrote:
> On 2011/10/31 18:46:43, rdsmith wrote:
> > What's the use case for this field?  DownloadCreateInfo is for passing data
> from
> > IO->FILE/UI threads, and we already pass interruption reasons via
> > OnResponseCompleted.
> 
> This is so we can pass in interrupt reasons that are caused by response codes
> from the server.

Could you flesh out that use case?  We already have a pathway for passing error
codes from the IO thread machinery to the rest of the download system
(OnResponseCompleted()); why not use that?

http://codereview.chromium.org/8404049/diff/3014/content/browser/download/dow...
File content/browser/download/download_item.h (right):

http://codereview.chromium.org/8404049/diff/3014/content/browser/download/dow...
content/browser/download/download_item.h:164: void Update(int64 bytes_so_far,
const std::string& partial_hash);
On 2011/11/13 21:15:20, ahendrickson wrote:
> On 2011/10/31 18:46:43, rdsmith wrote:
> > Currently, Update is just used to send across size data for display purposes
> > based on a timer on the file thread--it isn't synchronous with
OnReadCompleted
> > or anything.  |hash| isn't for display, so I don't think it makes sense to
> > include it in the update--it'll come across in OnAllDataSaved or
Interrupted.
> 
> No, Update also sets received_bytes_, which is passed along to the persistent
> store.  I want the hash to end up there as well.
> 
> Specifically, I want the hash to be periodically updated in the persistent
> store, so we can use it when we resume a download after a crash (where we
won't
> necessarily record a download interruption).
> 
> Does that seem reasonable to you?

Hmmm.  I understand it, but I don't like it; I want to get the hash less
interwoven with the core downloads system, not more.  Let me pull back a bit and
summarize the requirements, then see if I can come up with a different way to
satisfy them.

As I understand what you're saying, you want the hash stored in the persistent
store on a regular basis, and you want that hash associated with a specific
number of bytes received, so that when you resume downloads you can resume hash
computation from a known position and known partial hash.  That strikes me as a
reasonable requirement.  That requirement means that whenever we update
bytes_received in the persistent store, we also need to update the hash value,
atomically with bytes received.  I'm also good with that.

However, I think the regular, periodic update of the hash is so that we can
resume downloads after a crash.  For either shutdown or error cases we'll flush
the information out to the database when we Cancel/Interrupt.  So it seems like
the need for this extra argument is purely for supporting the resumption after
crash case.  I don't feel like supporting that case is very high priority; at
least, I hope we don't crash that often :-}.  

Having said that, we do need to make sure that we we update the hash and
bytes_so_far atomically.  And *avoiding* updating the bytes-so-far if we don't
have the hash is probably a larger hassle than doing what you've got above.  So
yeah, I'm ok with this.  But if it turns into a large hassle I'd probably go the
other way--simplicity :uber alles is going to be my motto here.

[Randy Smith (Not in Mondays), 2011-11-15 18:45:26]

More interface level comments--I'm holding off on reviewing the implementation
until we're in sync on the interface.

http://codereview.chromium.org/8404049/diff/18001/content/browser/download/do...
File content/browser/download/download_item.h (right):

http://codereview.chromium.org/8404049/diff/18001/content/browser/download/do...
content/browser/download/download_item.h:327: void set_etag(const std::string&
tag) { etag_ = tag; }
Under what circumstances are the setters used?  Can we replace them with an
UpdateOnResumption(DownloadCreateInfo)?

http://codereview.chromium.org/8404049/diff/18001/content/browser/download/do...
content/browser/download/download_item.h:374: void UpdateHash(const std::string&
partial_hash);
Given that we worked out in a different comment thread that it's fairly
important for resumption that the hash and size need to be kept in sync, I'd be
more inclined to fold this argument into UpdateSize (maybe change the name to
UpdateSizeAndHash).

http://codereview.chromium.org/8404049/diff/18001/content/browser/download/do...
File content/browser/download/download_resource_handler.h (right):

http://codereview.chromium.org/8404049/diff/18001/content/browser/download/do...
content/browser/download/download_resource_handler.h:85: void
set_start_offset(int64 bytes) { start_offset_ = bytes; }
Can this be in the constructor?  And commented in a way that makes more sense
for this abstraction level?  From the viewpoint of the IO thread/DRH/RDH we're
just executing a request and piping the results across to the FILE & UI threads,
so this would just be "the offset in the resource targeted at which to start
downloading".

[ahendrickson, 2011-11-16 15:41:08]

http://codereview.chromium.org/8404049/diff/3014/content/browser/download/bas...
File content/browser/download/base_file.h (right):

http://codereview.chromium.org/8404049/diff/3014/content/browser/download/bas...
content/browser/download/base_file.h:74: bool SetPartialSha256Hash(const
std::string& hash);
On 2011/11/15 18:35:25, rdsmith wrote:
> On 2011/11/13 21:15:20, ahendrickson wrote:
> > On 2011/10/31 18:46:43, rdsmith wrote:
> > > Hmmm.  This feels like something that's relevant only on creation, and
hence
> > > should be part of the constructor.  (I could imagine--not sure it's a good
> > > idea--putting something in debug only that on construction actually
> recomputed
> > > the partial hash and checked to see if it was valid.)
> > 
> > I'd like to be able to set the partial hash in unit tests.
> 
> Why can't that be done in the constructor?

Done.

http://codereview.chromium.org/8404049/diff/3014/content/browser/download/dow...
File content/browser/download/download_create_info.h (right):

http://codereview.chromium.org/8404049/diff/3014/content/browser/download/dow...
content/browser/download/download_create_info.h:91: std::string last_modified;
In order to use base::Time, we would be converting from a string to Time, then
reconverting it when saving to the history and when restarting a download.

It seems like a lot of extra work when what we need is the original string.

On 2011/11/15 18:35:25, rdsmith wrote:
> On 2011/11/13 21:15:20, ahendrickson wrote:
> > On 2011/10/31 18:46:43, rdsmith wrote:
> > > Why is this a string rather than a base::Time or something like it?  
> > 
> > I wanted to return to the server the exact Last-Modified string that we
> received
> > in the initial download.  Will base::Time guarantee that it's the save after
> > converting back?  Or, can we guarantee that the formatting doesn't matter?
> 
> Would you do a little investigation of this issue?  I understand the
> requirement, and it's probably needed, but I'm not ecstatic about it;
> last_modified as a string smells wrong.  Maybe we want to abstract a bit more
> and just pass in an (optional) conditional for the server, and format it on
the
> UI side?

http://codereview.chromium.org/8404049/diff/3014/content/browser/download/dow...
content/browser/download/download_create_info.h:97: bool continued_download;
On 2011/11/15 18:35:25, rdsmith wrote:
> On 2011/11/13 21:15:20, ahendrickson wrote:
> > On 2011/10/31 18:46:43, rdsmith wrote:
> > > What's the use case for this field?  Would it make more sense to just pass
> the
> > > offset from the beginning of the file of the data being received?  I could
> > > imagine that as being more flexible for more purposes in the future (e.g.
an
> > IDM
> > > like functionality that had multiple requests fetching different parts of
> the
> > > file).
> > 
> > We do have the offset for the data being received.  However, I can see
> > potentially wanting to distinguish multiple partial requests from a
'continue
> > download' scenario -- for example, in determining whether or not to
> > automatically resume.
> 
> Sure, but I would imagine that distinction would be best done in management
> code, i.e. in DownloadItem on the UI thread.  
> 
> > I can remove this variable if you feel strongly about it.
> 
> I feel pretty strongly about starting simple and adding complexity only when
> it's needed, and it doesn't sound like there's a nailed down use case at the
> moment?
> 
> 

Done.

http://codereview.chromium.org/8404049/diff/3014/content/browser/download/dow...
content/browser/download/download_create_info.h:100: InterruptReason
server_interrupt_reason;
On 2011/11/15 18:35:25, rdsmith wrote:
> On 2011/11/13 21:15:20, ahendrickson wrote:
> > On 2011/10/31 18:46:43, rdsmith wrote:
> > > What's the use case for this field?  DownloadCreateInfo is for passing
data
> > from
> > > IO->FILE/UI threads, and we already pass interruption reasons via
> > > OnResponseCompleted.
> > 
> > This is so we can pass in interrupt reasons that are caused by response
codes
> > from the server.
> 
> Could you flesh out that use case?  We already have a pathway for passing
error
> codes from the IO thread machinery to the rest of the download system
> (OnResponseCompleted()); why not use that?

Done.

http://codereview.chromium.org/8404049/diff/18001/content/browser/download/do...
File content/browser/download/download_item.h (right):

http://codereview.chromium.org/8404049/diff/18001/content/browser/download/do...
content/browser/download/download_item.h:327: void set_etag(const std::string&
tag) { etag_ = tag; }
On 2011/11/15 18:45:26, rdsmith wrote:
> Under what circumstances are the setters used?  Can we replace them with an
> UpdateOnResumption(DownloadCreateInfo)?

We need the getters (or an equivalent function) when we resume a download, and
possibly to save to the history DB.

The setters can be removed, as they only need to be set at construction time
(Done).

http://codereview.chromium.org/8404049/diff/18001/content/browser/download/do...
content/browser/download/download_item.h:374: void UpdateHash(const std::string&
partial_hash);
On 2011/11/15 18:45:26, rdsmith wrote:
> Given that we worked out in a different comment thread that it's fairly
> important for resumption that the hash and size need to be kept in sync, I'd
be
> more inclined to fold this argument into UpdateSize (maybe change the name to
> UpdateSizeAndHash).

Done.

[Randy Smith (Not in Mondays), 2011-11-16 18:29:26]

Just interface level comments.  I think we're converging enough that I'll dive
into a general flow review next (but after taking care of a few administrative
things I've been putting off).

http://codereview.chromium.org/8404049/diff/3014/content/browser/download/dow...
File content/browser/download/download_create_info.h (right):

http://codereview.chromium.org/8404049/diff/3014/content/browser/download/dow...
content/browser/download/download_create_info.h:91: std::string last_modified;
On 2011/11/16 15:41:08, ahendrickson wrote:
> In order to use base::Time, we would be converting from a string to Time, then
> reconverting it when saving to the history and when restarting a download.
> 
> It seems like a lot of extra work when what we need is the original string.

Ok, but include a comment explaining the code smell.  And if we come up with a
use case for the actual time in the code, I'll start pushing on this issue
again.

> 
> On 2011/11/15 18:35:25, rdsmith wrote:
> > On 2011/11/13 21:15:20, ahendrickson wrote:
> > > On 2011/10/31 18:46:43, rdsmith wrote:
> > > > Why is this a string rather than a base::Time or something like it?  
> > > 
> > > I wanted to return to the server the exact Last-Modified string that we
> > received
> > > in the initial download.  Will base::Time guarantee that it's the save
after
> > > converting back?  Or, can we guarantee that the formatting doesn't matter?
> > 
> > Would you do a little investigation of this issue?  I understand the
> > requirement, and it's probably needed, but I'm not ecstatic about it;
> > last_modified as a string smells wrong.  Maybe we want to abstract a bit
more
> > and just pass in an (optional) conditional for the server, and format it on
> the
> > UI side?
>

http://codereview.chromium.org/8404049/diff/18001/content/browser/download/do...
File content/browser/download/download_item.h (right):

http://codereview.chromium.org/8404049/diff/18001/content/browser/download/do...
content/browser/download/download_item.h:327: void set_etag(const std::string&
tag) { etag_ = tag; }
On 2011/11/16 15:41:08, ahendrickson wrote:
> On 2011/11/15 18:45:26, rdsmith wrote:
> > Under what circumstances are the setters used?  Can we replace them with an
> > UpdateOnResumption(DownloadCreateInfo)?
> 
> We need the getters (or an equivalent function) when we resume a download, and
> possibly to save to the history DB.
> 
> The setters can be removed, as they only need to be set at construction time
> (Done).

So you're not planning to update these values when you do a resumption?  I'm
inclined to think you should--if you restart, they may change, and there's no
reason to disable continuation after restart if the restart happened because of
an etag/last_modified mismatch.  I'd just rather than that update get done in a
function that's targeted at that update, thus my suggestion of
UpdateOnResumption.

http://codereview.chromium.org/8404049/diff/25001/content/browser/download/ba...
File content/browser/download/base_file.h (right):

http://codereview.chromium.org/8404049/diff/25001/content/browser/download/ba...
content/browser/download/base_file.h:35: const std::string& initial_hash,
In the cases in which we specify an initial_hash, we're going to want to be
specifying an existing file and an offset within that file at which to start,
correct?  Should those be here as well/how are you planning to specify those?

http://codereview.chromium.org/8404049/diff/25001/content/browser/download/do...
File content/browser/download/download_file.h (right):

http://codereview.chromium.org/8404049/diff/25001/content/browser/download/do...
content/browser/download/download_file.h:32: const std::string& initial_hash);
See comment in base_file.h

http://codereview.chromium.org/8404049/diff/25001/content/browser/download/do...
File content/browser/download/download_item.h (right):

http://codereview.chromium.org/8404049/diff/25001/content/browser/download/do...
content/browser/download/download_item.h:371: void UpdateHash(const std::string&
partial_hash);
Doesn't this go away now?

http://codereview.chromium.org/8404049/diff/25001/content/browser/download/do...
File content/browser/download/download_manager.h (right):

http://codereview.chromium.org/8404049/diff/25001/content/browser/download/do...
content/browser/download/download_manager.h:119: // Notifications sent from the
download thread to the UI thread
nit: Shift this to file thread?  (Anywhere else you see it as well.)

http://codereview.chromium.org/8404049/diff/25001/content/browser/download/do...
File content/browser/download/download_persistent_store_info.h (right):

http://codereview.chromium.org/8404049/diff/25001/content/browser/download/do...
content/browser/download/download_persistent_store_info.h:85: int32 local_id;
Why do we want to persist this?  It's namespace is per-browser instance, and
when that stops being true, we'll have unified it with the db_handle.

http://codereview.chromium.org/8404049/diff/25001/content/browser/download/do...
content/browser/download/download_persistent_store_info.h:98: // Last reason.
A bit more of a comment here?  Duplicating the variable name doesn't really feel
like it deserves the space taken by the comment :-}.

http://codereview.chromium.org/8404049/diff/25001/content/browser/download/do...
File content/browser/download/download_resource_handler.h (right):

http://codereview.chromium.org/8404049/diff/25001/content/browser/download/do...
content/browser/download/download_resource_handler.h:84: // used only by
|ResourceDispatcherHost::BeginDownload()|.
I'd like to think about the interface to the IO and file threads as being just
about the current request, and how they're used (to pick up where we left off)
being in the UI thread/DownloadItem.  So this comment should just say something
like "Location within the remote resource at which to start the download."

[Randy Smith (Not in Mondays), 2011-11-16 21:40:25]

This seems light on tests.  I made on suggestion below, and it may be
appropriate for it to be light on tests since you're changing data structures
rather than behavior, but if you could do a scan through to look for behavior
you are changing and make sure you have good tests in place, I'd appreciate it.

http://codereview.chromium.org/8404049/diff/31002/content/browser/download/ba...
File content/browser/download/base_file.cc (right):

http://codereview.chromium.org/8404049/diff/31002/content/browser/download/ba...
content/browser/download/base_file.cc:385: secure_hash_->Finish(sha256_hash_,
kSha256HashLen);
I may be repenting my earlier comments; I hadn't realized that we transformed
the continuing hash into a completed value.  Doesn't this mean that
GetSha256Hash() won't work at intermediate stages?  So does the current code
actually transmit the intermediate hash values?

http://codereview.chromium.org/8404049/diff/31002/content/browser/download/ba...
File content/browser/download/base_file_unittest.cc (right):

http://codereview.chromium.org/8404049/diff/31002/content/browser/download/ba...
content/browser/download/base_file_unittest.cc:254: TEST_F(BaseFileTest,
SingleWriteWithHash) {
I'd think that we'd also want to confirm that if we have a data chunk X, and we
write it to a file and get a hash, that hash is the same as if we wrote X/2,
closed the file, opened a new one with the partial hash, and did the second
half.

http://codereview.chromium.org/8404049/diff/31002/content/browser/download/do...
File content/browser/download/download_file_manager.cc (right):

http://codereview.chromium.org/8404049/diff/31002/content/browser/download/do...
content/browser/download/download_file_manager.cc:74: ""));
I'm finding myself a bit uncomfortable about the interchangeability of "" and an
empty hash.  Is there an answer to the question of "What's the hash of a zero
length file?"  If so, I'd suggest initializing new DownloadFiles with that
value, using it most places, and using "" to mean "not collecting hash--sorry". 
How hard would that be to do?

http://codereview.chromium.org/8404049/diff/31002/content/browser/download/do...
content/browser/download/download_file_manager.cc:250: partial_hash.clear();
Why duplicate this code?  Can it be hoisted out of the conditional?

http://codereview.chromium.org/8404049/diff/31002/content/browser/download/do...
File content/browser/download/download_item.cc (right):

http://codereview.chromium.org/8404049/diff/31002/content/browser/download/do...
content/browser/download/download_item.cc:198: (info.received_bytes == 0)),
What's the use case for adding this test?  I would think that we'd never be in a
position to restart a temporary file, but if we are, shouldn't it still be
temporary?

http://codereview.chromium.org/8404049/diff/31002/content/browser/download/do...
content/browser/download/download_item.cc:338: hash_ = partial_hash;
Why not unilaterally assign?

http://codereview.chromium.org/8404049/diff/31002/content/browser/download/do...
File content/browser/download/download_resource_handler.h (right):

http://codereview.chromium.org/8404049/diff/31002/content/browser/download/do...
content/browser/download/download_resource_handler.h:84: void
set_start_offset(int64 bytes) { start_offset_ = bytes; }
It occurs to me to ask: Should this be in the constructor?  I can imagine
reasons why it wouldn't be, but if it can be, it should be.

http://codereview.chromium.org/8404049/diff/31002/content/browser/download/do...
content/browser/download/download_resource_handler.h:84: void
set_start_offset(int64 bytes) { start_offset_ = bytes; }
Note that there's an awesome unit-test opportunity here (that we should take) of
trying to download the second half of a local resource and making sure we get
what we expect.  This probably only works if the mock request job supports range
requests.

[Randy Smith (Not in Mondays), 2011-11-16 21:42:43]

On 2011/11/16 21:40:25, rdsmith wrote:
http://codereview.chromium.org/8404049/diff/31002/content/browser/download/do...
> content/browser/download/download_resource_handler.h:84: void
> set_start_offset(int64 bytes) { start_offset_ = bytes; }
> Note that there's an awesome unit-test opportunity here (that we should take)
of
> trying to download the second half of a local resource and making sure we get
> what we expect.  This probably only works if the mock request job supports
range
> requests.

To be clear, I wasn't requesting this unit test in this CL; the functionality
isn't implemented yet.  I wanted to call it out as something to think about.  My
test suggestion was comparing hashes from a file written in two separate chunks
to a file written all at once.

[ahendrickson, 2011-11-19 20:18:03]

http://codereview.chromium.org/8404049/diff/3014/content/browser/download/dow...
File content/browser/download/download_create_info.h (right):

http://codereview.chromium.org/8404049/diff/3014/content/browser/download/dow...
content/browser/download/download_create_info.h:91: std::string last_modified;
On 2011/11/16 18:29:27, rdsmith wrote:
> On 2011/11/16 15:41:08, ahendrickson wrote:
> > In order to use base::Time, we would be converting from a string to Time,
then
> > reconverting it when saving to the history and when restarting a download.
> > 
> > It seems like a lot of extra work when what we need is the original string.
> 
> Ok, but include a comment explaining the code smell.  And if we come up with a
> use case for the actual time in the code, I'll start pushing on this issue
> again.
> 
> > 
> > On 2011/11/15 18:35:25, rdsmith wrote:
> > > On 2011/11/13 21:15:20, ahendrickson wrote:
> > > > On 2011/10/31 18:46:43, rdsmith wrote:
> > > > > Why is this a string rather than a base::Time or something like it?  
> > > > 
> > > > I wanted to return to the server the exact Last-Modified string that we
> > > received
> > > > in the initial download.  Will base::Time guarantee that it's the save
> after
> > > > converting back?  Or, can we guarantee that the formatting doesn't
matter?
> > > 
> > > Would you do a little investigation of this issue?  I understand the
> > > requirement, and it's probably needed, but I'm not ecstatic about it;
> > > last_modified as a string smells wrong.  Maybe we want to abstract a bit
> more
> > > and just pass in an (optional) conditional for the server, and format it
on
> > the
> > > UI side?
> > 
> 

Done.

http://codereview.chromium.org/8404049/diff/18001/content/browser/download/do...
File content/browser/download/download_item.h (right):

http://codereview.chromium.org/8404049/diff/18001/content/browser/download/do...
content/browser/download/download_item.h:327: void set_etag(const std::string&
tag) { etag_ = tag; }
On 2011/11/16 18:29:27, rdsmith wrote:
> On 2011/11/16 15:41:08, ahendrickson wrote:
> > On 2011/11/15 18:45:26, rdsmith wrote:
> > > Under what circumstances are the setters used?  Can we replace them with
an
> > > UpdateOnResumption(DownloadCreateInfo)?
> > 
> > We need the getters (or an equivalent function) when we resume a download,
and
> > possibly to save to the history DB.
> > 
> > The setters can be removed, as they only need to be set at construction time
> > (Done).
> 
> So you're not planning to update these values when you do a resumption?  I'm
> inclined to think you should--if you restart, they may change, and there's no
> reason to disable continuation after restart if the restart happened because
of
> an etag/last_modified mismatch.  I'd just rather than that update get done in
a
> function that's targeted at that update, thus my suggestion of
> UpdateOnResumption.

You're right, and I'll put them back in when they are necessary.

http://codereview.chromium.org/8404049/diff/25001/content/browser/download/ba...
File content/browser/download/base_file.h (right):

http://codereview.chromium.org/8404049/diff/25001/content/browser/download/ba...
content/browser/download/base_file.h:35: const std::string& initial_hash,
On 2011/11/16 18:29:27, rdsmith wrote:
> In the cases in which we specify an initial_hash, we're going to want to be
> specifying an existing file and an offset within that file at which to start,
> correct?  Should those be here as well/how are you planning to specify those?

Those are already here.

file_stream will be used to specify the location of the file that already has
data, or for restarts the location that we've already decided on.

received_bytes will be non-zero for files that already have data (continued).

http://codereview.chromium.org/8404049/diff/25001/content/browser/download/do...
File content/browser/download/download_file.h (right):

http://codereview.chromium.org/8404049/diff/25001/content/browser/download/do...
content/browser/download/download_file.h:32: const std::string& initial_hash);
On 2011/11/16 18:29:27, rdsmith wrote:
> See comment in base_file.h

Done.

http://codereview.chromium.org/8404049/diff/25001/content/browser/download/do...
File content/browser/download/download_item.h (right):

http://codereview.chromium.org/8404049/diff/25001/content/browser/download/do...
content/browser/download/download_item.h:371: void UpdateHash(const std::string&
partial_hash);
On 2011/11/16 18:29:27, rdsmith wrote:
> Doesn't this go away now?

Yes.  I thought I had removed it already.

http://codereview.chromium.org/8404049/diff/25001/content/browser/download/do...
File content/browser/download/download_manager.h (right):

http://codereview.chromium.org/8404049/diff/25001/content/browser/download/do...
content/browser/download/download_manager.h:119: // Notifications sent from the
download thread to the UI thread
On 2011/11/16 18:29:27, rdsmith wrote:
> nit: Shift this to file thread?  (Anywhere else you see it as well.)

Done.

http://codereview.chromium.org/8404049/diff/25001/content/browser/download/do...
File content/browser/download/download_persistent_store_info.h (right):

http://codereview.chromium.org/8404049/diff/25001/content/browser/download/do...
content/browser/download/download_persistent_store_info.h:85: int32 local_id;
On 2011/11/16 18:29:27, rdsmith wrote:
> Why do we want to persist this?  It's namespace is per-browser instance, and
> when that stops being true, we'll have unified it with the db_handle.

Removed.

http://codereview.chromium.org/8404049/diff/25001/content/browser/download/do...
content/browser/download/download_persistent_store_info.h:98: // Last reason.
On 2011/11/16 18:29:27, rdsmith wrote:
> A bit more of a comment here?  Duplicating the variable name doesn't really
feel
> like it deserves the space taken by the comment :-}.

Done.

http://codereview.chromium.org/8404049/diff/25001/content/browser/download/do...
File content/browser/download/download_resource_handler.h (right):

http://codereview.chromium.org/8404049/diff/25001/content/browser/download/do...
content/browser/download/download_resource_handler.h:84: // used only by
|ResourceDispatcherHost::BeginDownload()|.
On 2011/11/16 18:29:27, rdsmith wrote:
> I'd like to think about the interface to the IO and file threads as being just
> about the current request, and how they're used (to pick up where we left off)
> being in the UI thread/DownloadItem.  So this comment should just say
something
> like "Location within the remote resource at which to start the download."

Done.

http://codereview.chromium.org/8404049/diff/31002/content/browser/download/ba...
File content/browser/download/base_file.cc (right):

http://codereview.chromium.org/8404049/diff/31002/content/browser/download/ba...
content/browser/download/base_file.cc:385: secure_hash_->Finish(sha256_hash_,
kSha256HashLen);
On 2011/11/16 21:40:25, rdsmith wrote:
> I may be repenting my earlier comments; I hadn't realized that we transformed
> the continuing hash into a completed value.  Doesn't this mean that
> GetSha256Hash() won't work at intermediate stages?  So does the current code
> actually transmit the intermediate hash values?

You are correct -- GetSha256Hash() will only work properly for the final hash
value.

I have another CL that will allow saving the state of the algorithm, and
restoring it later.  It uses Pickle's to serialize & deserialize the state.

http://codereview.chromium.org/8404049/diff/31002/content/browser/download/ba...
File content/browser/download/base_file_unittest.cc (right):

http://codereview.chromium.org/8404049/diff/31002/content/browser/download/ba...
content/browser/download/base_file_unittest.cc:254: TEST_F(BaseFileTest,
SingleWriteWithHash) {
On 2011/11/16 21:40:25, rdsmith wrote:
> I'd think that we'd also want to confirm that if we have a data chunk X, and
we
> write it to a file and get a hash, that hash is the same as if we wrote X/2,
> closed the file, opened a new one with the partial hash, and did the second
> half.

That sounds like a good idea.

Done.

http://codereview.chromium.org/8404049/diff/31002/content/browser/download/do...
File content/browser/download/download_file_manager.cc (right):

http://codereview.chromium.org/8404049/diff/31002/content/browser/download/do...
content/browser/download/download_file_manager.cc:74: ""));
On 2011/11/16 21:40:25, rdsmith wrote:
> I'm finding myself a bit uncomfortable about the interchangeability of "" and
an
> empty hash.  Is there an answer to the question of "What's the hash of a zero
> length file?"  If so, I'd suggest initializing new DownloadFiles with that
> value, using it most places, and using "" to mean "not collecting
hash--sorry". 
> How hard would that be to do?

We're now using Pickle's instead of strings.

http://codereview.chromium.org/8404049/diff/31002/content/browser/download/do...
content/browser/download/download_file_manager.cc:250: partial_hash.clear();
On 2011/11/16 21:40:25, rdsmith wrote:
> Why duplicate this code?  Can it be hoisted out of the conditional?

One is now a Pickle, but the other is a string.

http://codereview.chromium.org/8404049/diff/31002/content/browser/download/do...
File content/browser/download/download_item.cc (right):

http://codereview.chromium.org/8404049/diff/31002/content/browser/download/do...
content/browser/download/download_item.cc:198: (info.received_bytes == 0)),
On 2011/11/16 21:40:25, rdsmith wrote:
> What's the use case for adding this test?  I would think that we'd never be in
a
> position to restart a temporary file, but if we are, shouldn't it still be
> temporary?

Reverted.  This is not a path that resuming downloads take anyway.

http://codereview.chromium.org/8404049/diff/31002/content/browser/download/do...
content/browser/download/download_item.cc:338: hash_ = partial_hash;
On 2011/11/16 21:40:25, rdsmith wrote:
> Why not unilaterally assign?  

Done.

http://codereview.chromium.org/8404049/diff/31002/content/browser/download/do...
File content/browser/download/download_resource_handler.h (right):

http://codereview.chromium.org/8404049/diff/31002/content/browser/download/do...
content/browser/download/download_resource_handler.h:84: void
set_start_offset(int64 bytes) { start_offset_ = bytes; }
On 2011/11/16 21:40:25, rdsmith wrote:
> It occurs to me to ask: Should this be in the constructor?  I can imagine
> reasons why it wouldn't be, but if it can be, it should be.

We have a download ID at construction time (in BufferedResourceHandler), but we
don't have access to the DownloadItem, which has the number of received bytes.

http://codereview.chromium.org/8404049/diff/31002/content/browser/download/do...
content/browser/download/download_resource_handler.h:84: void
set_start_offset(int64 bytes) { start_offset_ = bytes; }
On 2011/11/16 21:40:25, rdsmith wrote:
> Note that there's an awesome unit-test opportunity here (that we should take)
of
> trying to download the second half of a local resource and making sure we get
> what we expect.  This probably only works if the mock request job supports
range
> requests.  

Done.  The unit test fails currently, but will succeed (I hope!) once
serialization works for SecureHash.

[Randy Smith (Not in Mondays), 2011-11-21 02:10:16]

I'm holding off on my next through review until after we resolve the Pickle
issue; it's now pretty pervasive in the code, and that really feels wrong to me
(see below).  We are looking good beyond that issue, though resolving that issue
may take a bit of work, after that point I think the CL will have a smoother
time.

http://codereview.chromium.org/8404049/diff/25001/content/browser/download/ba...
File content/browser/download/base_file.h (right):

http://codereview.chromium.org/8404049/diff/25001/content/browser/download/ba...
content/browser/download/base_file.h:35: const std::string& initial_hash,
On 2011/11/19 20:18:03, ahendrickson wrote:
> On 2011/11/16 18:29:27, rdsmith wrote:
> > In the cases in which we specify an initial_hash, we're going to want to be
> > specifying an existing file and an offset within that file at which to
start,
> > correct?  Should those be here as well/how are you planning to specify
those?
> 
> Those are already here.
> 
> file_stream will be used to specify the location of the file that already has
> data, or for restarts the location that we've already decided on.

file_stream is fairly often not set; are you making the design decision to set
it always on the continuation pathway?  I'm uncomfortable with that, both
because I don't like that file stream is here (I don't understand the use cases
in which the download system needs to be passed a file descriptor rather than a
file name) and because it represents yet more conditionals as we go through the
code.  
> 
> received_bytes will be non-zero for files that already have data (continued).

D'oh; right.

http://codereview.chromium.org/8404049/diff/31002/content/browser/download/ba...
File content/browser/download/base_file.cc (right):

http://codereview.chromium.org/8404049/diff/31002/content/browser/download/ba...
content/browser/download/base_file.cc:385: secure_hash_->Finish(sha256_hash_,
kSha256HashLen);
On 2011/11/19 20:18:04, ahendrickson wrote:
> On 2011/11/16 21:40:25, rdsmith wrote:
> > I may be repenting my earlier comments; I hadn't realized that we
transformed
> > the continuing hash into a completed value.  Doesn't this mean that
> > GetSha256Hash() won't work at intermediate stages?  So does the current code
> > actually transmit the intermediate hash values?
> 
> You are correct -- GetSha256Hash() will only work properly for the final hash
> value.
> 
> I have another CL that will allow saving the state of the algorithm, and
> restoring it later.  It uses Pickle's to serialize & deserialize the state.
> 

*nod*; makes sense.  I'd hold this CL until that one lands (as this CL promises
functionality it can't currently deliver).

http://codereview.chromium.org/8404049/diff/31002/content/browser/download/do...
File content/browser/download/download_resource_handler.h (right):

http://codereview.chromium.org/8404049/diff/31002/content/browser/download/do...
content/browser/download/download_resource_handler.h:84: void
set_start_offset(int64 bytes) { start_offset_ = bytes; }
On 2011/11/19 20:18:04, ahendrickson wrote:
> On 2011/11/16 21:40:25, rdsmith wrote:
> > It occurs to me to ask: Should this be in the constructor?  I can imagine
> > reasons why it wouldn't be, but if it can be, it should be.
> 
> We have a download ID at construction time (in BufferedResourceHandler), but
we
> don't have access to the DownloadItem, which has the number of received bytes.

We never have access to the DownloadItem on the IO thread.  What's the pathway
by which this setter will get called?

http://codereview.chromium.org/8404049/diff/44001/content/browser/download/ba...
File content/browser/download/base_file.h (right):

http://codereview.chromium.org/8404049/diff/44001/content/browser/download/ba...
content/browser/download/base_file.h:22: class Pickle;
I don't think Pickle belongs in the interface to *File; it's an implementation
detail of how we save and restore the hash.  The pickle class is used to
transform between an arbitrary data structure and a byte chunk of of a
particular size.  I believe (?) that that can be wrapped in a string, and I'd
much rather have a string or some other opaque object than a Pickle.  Pickle's
are for transformation, not for passing stuff around.

Having said that, the current situation makes it very clear that partial and
complete hashes are really different things (one's a serialized intermediate
state of a class we don't know much about, the other has some meaning to it). 
So I'd suggest (with sadness; it's complexity in the interface we don't need)
that you shift back to have separate getters for the intermediate and final
hash.  

Note that it's probably *not* safe to store the intermediate hash value (==
pickle output) as a string in the database; that probably has to be a blob.

http://codereview.chromium.org/8404049/diff/44001/content/browser/download/do...
File content/browser/download/download_item.h (right):

http://codereview.chromium.org/8404049/diff/44001/content/browser/download/do...
content/browser/download/download_item.h:153: void Update(int64 bytes_so_far,
const Pickle& hash_state);
So this makes me concerned, as pickling and unpickling can be something of a
heavyweight operation, and I'd rather not be doing it on a regular, updating
basis.  I'm not sure what the right answer is here, but by code smell I'm
tempted to reverse myself on my earlier acceptance and say that we should avoid
updating the size/hash pair in the DB except on interrupt and completion (and
I'd leave the hash blank on completion--I don't think it'd be used on restart,
and as previously noted, the complete hash is a completely different beast than
the intermediate hash).  All of which will take some plumbing :-{.  

I could be argued out of this--it's probably not *too* expensive to pickle on
every timer interrupt on the FILE thread.  But it really doesn't feel like the
right thing to be doing.

[ahendrickson, 2011-11-21 20:34:46]

Do you think I should add any particular reviewers, once you're happy with this?

http://codereview.chromium.org/8404049/diff/25001/content/browser/download/ba...
File content/browser/download/base_file.h (right):

http://codereview.chromium.org/8404049/diff/25001/content/browser/download/ba...
content/browser/download/base_file.h:35: const std::string& initial_hash,
On 2011/11/21 02:10:16, rdsmith wrote:
> On 2011/11/19 20:18:03, ahendrickson wrote:
> > On 2011/11/16 18:29:27, rdsmith wrote:
> > > In the cases in which we specify an initial_hash, we're going to want to
be
> > > specifying an existing file and an offset within that file at which to
> start,
> > > correct?  Should those be here as well/how are you planning to specify
> those?
> > 
> > Those are already here.
> > 
> > file_stream will be used to specify the location of the file that already
has
> > data, or for restarts the location that we've already decided on.
> 
> file_stream is fairly often not set; are you making the design decision to set
> it always on the continuation pathway?  I'm uncomfortable with that, both
> because I don't like that file stream is here (I don't understand the use
cases
> in which the download system needs to be passed a file descriptor rather than
a
> file name) and because it represents yet more conditionals as we go through
the
> code.  
> > 

My mistake; the file is specified in full_path, not file_stream.

BaseFile doesn't know the difference between the intermediate and final files.

When we discover an existing download item, we will set DownloadCreateInfo's
save_info.file_path to the current file name that we are writing to.  When
DownloadFile is constructed, it uses that value to open the file.

> > received_bytes will be non-zero for files that already have data
(continued).
> 
> D'oh; right.
>

http://codereview.chromium.org/8404049/diff/31002/content/browser/download/ba...
File content/browser/download/base_file.cc (right):

http://codereview.chromium.org/8404049/diff/31002/content/browser/download/ba...
content/browser/download/base_file.cc:385: secure_hash_->Finish(sha256_hash_,
kSha256HashLen);
On 2011/11/21 02:10:17, rdsmith wrote:
> On 2011/11/19 20:18:04, ahendrickson wrote:
> > On 2011/11/16 21:40:25, rdsmith wrote:
> > > I may be repenting my earlier comments; I hadn't realized that we
> transformed
> > > the continuing hash into a completed value.  Doesn't this mean that
> > > GetSha256Hash() won't work at intermediate stages?  So does the current
code
> > > actually transmit the intermediate hash values?
> > 
> > You are correct -- GetSha256Hash() will only work properly for the final
hash
> > value.
> > 
> > I have another CL that will allow saving the state of the algorithm, and
> > restoring it later.  It uses Pickle's to serialize & deserialize the state.
> > 
> 
> *nod*; makes sense.  I'd hold this CL until that one lands (as this CL
promises
> functionality it can't currently deliver).

The CL has landed! $-)

http://codereview.chromium.org/8404049/diff/31002/content/browser/download/do...
File content/browser/download/download_resource_handler.h (right):

http://codereview.chromium.org/8404049/diff/31002/content/browser/download/do...
content/browser/download/download_resource_handler.h:84: void
set_start_offset(int64 bytes) { start_offset_ = bytes; }
On 2011/11/19 20:18:04, ahendrickson wrote:
> On 2011/11/16 21:40:25, rdsmith wrote:
> > Note that there's an awesome unit-test opportunity here (that we should
take)
> of
> > trying to download the second half of a local resource and making sure we
get
> > what we expect.  This probably only works if the mock request job supports
> range
> > requests.  
> 
> Done.  The unit test fails currently, but will succeed (I hope!) once
> serialization works for SecureHash.

The unit test now succeeds.

http://codereview.chromium.org/8404049/diff/31002/content/browser/download/do...
content/browser/download/download_resource_handler.h:84: void
set_start_offset(int64 bytes) { start_offset_ = bytes; }
On 2011/11/21 02:10:17, rdsmith wrote:
> On 2011/11/19 20:18:04, ahendrickson wrote:
> > On 2011/11/16 21:40:25, rdsmith wrote:
> > > It occurs to me to ask: Should this be in the constructor?  I can imagine
> > > reasons why it wouldn't be, but if it can be, it should be.
> > 
> > We have a download ID at construction time (in BufferedResourceHandler), but
> we
> > don't have access to the DownloadItem, which has the number of received
bytes.
> 
> We never have access to the DownloadItem on the IO thread.  What's the pathway
> by which this setter will get called?  

Changed it to be in the constructor, so this setter is removed.  We will pass in
a DownloadResumeInfo to RDH::BeginDownload(), which will provide the start
offset.

http://codereview.chromium.org/8404049/diff/44001/content/browser/download/ba...
File content/browser/download/base_file.h (right):

http://codereview.chromium.org/8404049/diff/44001/content/browser/download/ba...
content/browser/download/base_file.h:22: class Pickle;
On 2011/11/21 02:10:17, rdsmith wrote:
> I don't think Pickle belongs in the interface to *File; it's an implementation
> detail of how we save and restore the hash.  The pickle class is used to
> transform between an arbitrary data structure and a byte chunk of of a
> particular size.  I believe (?) that that can be wrapped in a string, and I'd
> much rather have a string or some other opaque object than a Pickle.  Pickle's
> are for transformation, not for passing stuff around.

Done.

> 
> Having said that, the current situation makes it very clear that partial and
> complete hashes are really different things (one's a serialized intermediate
> state of a class we don't know much about, the other has some meaning to it). 
> So I'd suggest (with sadness; it's complexity in the interface we don't need)
> that you shift back to have separate getters for the intermediate and final
> hash.  

This is done.

> 
> Note that it's probably *not* safe to store the intermediate hash value (==
> pickle output) as a string in the database; that probably has to be a blob.

Agreed, but that will be in a separate CL.

http://codereview.chromium.org/8404049/diff/44001/content/browser/download/do...
File content/browser/download/download_item.h (right):

http://codereview.chromium.org/8404049/diff/44001/content/browser/download/do...
content/browser/download/download_item.h:153: void Update(int64 bytes_so_far,
const Pickle& hash_state);
On 2011/11/21 02:10:17, rdsmith wrote:
> So this makes me concerned, as pickling and unpickling can be something of a
> heavyweight operation, and I'd rather not be doing it on a regular, updating
> basis.  I'm not sure what the right answer is here, but by code smell I'm
> tempted to reverse myself on my earlier acceptance and say that we should
avoid
> updating the size/hash pair in the DB except on interrupt and completion (and
> I'd leave the hash blank on completion--I don't think it'd be used on restart,
> and as previously noted, the complete hash is a completely different beast
than
> the intermediate hash).  All of which will take some plumbing :-{.  
> 
> I could be argued out of this--it's probably not *too* expensive to pickle on
> every timer interrupt on the FILE thread.  But it really doesn't feel like the
> right thing to be doing.

FWIW, the size of the pickle payload is around 300 bytes (for the larger hash
scheme), and the overhead is on the order of 32 more bytes.

Pickle copying is mainly a call to memcpy(), and WriteData() is a memcpy() plus
some accounting.

That doesn't seem to me an expensive operation; what do you think?

[Randy Smith (Not in Mondays), 2011-11-22 17:37:49]

Interface (.h file) round.  A couple of high level comments:

* Reviewers: I'd bring Ben in for the DB/PersistentStoreInfo changes (but see
next).  If you keep a change to resource_dispatcher_host_unittest.cc, you'll
probably need an OWNER from that area (rvargas?) but I'm not sure you need that
change.

* I'm finding myself uncomfortable making changes to DownloadPersistentStoreInfo
without making changes to what actually goes into the DB in the same CL; it
feels like there's an invariant connection there.  How hard would it be to hold
off on the changes to that structure until you're ready to make the changes to
the DB, or to make the changes to the DB in this CL?  (I'm not saying I'll block
it going in like this, but if one of the other two options isn't that much work,
I may request that.)

http://codereview.chromium.org/8404049/diff/44001/content/browser/download/do...
File content/browser/download/download_item.h (right):

http://codereview.chromium.org/8404049/diff/44001/content/browser/download/do...
content/browser/download/download_item.h:153: void Update(int64 bytes_so_far,
const Pickle& hash_state);
On 2011/11/21 20:34:47, ahendrickson wrote:
> On 2011/11/21 02:10:17, rdsmith wrote:
> > So this makes me concerned, as pickling and unpickling can be something of a
> > heavyweight operation, and I'd rather not be doing it on a regular, updating
> > basis.  I'm not sure what the right answer is here, but by code smell I'm
> > tempted to reverse myself on my earlier acceptance and say that we should
> avoid
> > updating the size/hash pair in the DB except on interrupt and completion
(and
> > I'd leave the hash blank on completion--I don't think it'd be used on
restart,
> > and as previously noted, the complete hash is a completely different beast
> than
> > the intermediate hash).  All of which will take some plumbing :-{.  
> > 
> > I could be argued out of this--it's probably not *too* expensive to pickle
on
> > every timer interrupt on the FILE thread.  But it really doesn't feel like
the
> > right thing to be doing.
> 
> FWIW, the size of the pickle payload is around 300 bytes (for the larger hash
> scheme), and the overhead is on the order of 32 more bytes.
> 
> Pickle copying is mainly a call to memcpy(), and WriteData() is a memcpy()
plus
> some accounting.
> 
> That doesn't seem to me an expensive operation; what do you think?

That sounds ok.  Could you figure out about how often (in time or bytes, either
way) the operation will occur?  I'd like to have a handle on the likely
performance implications before signing off.

http://codereview.chromium.org/8404049/diff/53001/content/browser/download/ba...
File content/browser/download/base_file.h (right):

http://codereview.chromium.org/8404049/diff/53001/content/browser/download/ba...
content/browser/download/base_file.h:77: virtual bool SetSha256HashState(const
std::string& hash_state_bytes);
Given that this is also in the constructor, is this setter necessary?

http://codereview.chromium.org/8404049/diff/53001/content/browser/download/ba...
content/browser/download/base_file.h:125: std::string initial_hash_state_;
Why can't you just unpickle in the constructor directly into secure_hash_?

http://codereview.chromium.org/8404049/diff/53001/content/browser/download/do...
File content/browser/download/download_create_info.h (right):

http://codereview.chromium.org/8404049/diff/53001/content/browser/download/do...
content/browser/download/download_create_info.h:91: // For restarting a
download, the modification time of the file.
nit: This is really (as I understand your vocabulary) for continuing a download;
it's not used on restarting.  Same comment applies to etag.

http://codereview.chromium.org/8404049/diff/53001/content/browser/download/do...
File content/browser/download/download_file.h (right):

http://codereview.chromium.org/8404049/diff/53001/content/browser/download/do...
content/browser/download/download_file.h:64: virtual bool
SetSha256HashState(const std::string& hash_state) = 0;
Same comment as in base file; can we get rid of this setter?

http://codereview.chromium.org/8404049/diff/53001/content/browser/download/do...
File content/browser/download/download_item.h (right):

http://codereview.chromium.org/8404049/diff/53001/content/browser/download/do...
content/browser/download/download_item.h:357: void UpdateSize(int64
bytes_so_far);
Given that the reason we have UpdateSizeAndHashState is to always keep the two
values in sync, I'd like to have functions at this level always update both
(where in these terms the hash state also includes the "Have we finished and do
we have a complete hash?")  

Maybe UpdateProgress and ProgressComplete (called when we're done)?  Those
functions have the advantage that they're talking about the higher level goals
of the functions, not about how they change the underlying data members.  Even
though these are private functions, so of the same principles of interface
design apply--you want the function names and arguments to be targeted at the
callers context, not the implementation of the function.

http://codereview.chromium.org/8404049/diff/53001/content/browser/download/do...
content/browser/download/download_item.h:431: // hash is being computed.
nit: At this level "while the hash is being computed" isn't really meaningful;
that's more a DownloadFile concept.  Maybe "until the download has completed".

http://codereview.chromium.org/8404049/diff/53001/content/browser/download/do...
File content/browser/download/download_persistent_store_info.h (right):

http://codereview.chromium.org/8404049/diff/53001/content/browser/download/do...
content/browser/download/download_persistent_store_info.h:45: const std::string&
hash_state_pickle,
nit: I'd like to use consistent vocabulary for things throughout the code base,
so I'm inclined to think this should be just "hash_state".

http://codereview.chromium.org/8404049/diff/53001/content/browser/download/do...
File content/browser/download/download_types.h (right):

http://codereview.chromium.org/8404049/diff/53001/content/browser/download/do...
content/browser/download/download_types.h:14: // Holds the information about how
to save a download file.
I think it makes sense to have a comment here something like "In the case of
download resumption, file_path is set to ... , offset ... hash_state ..." for
context.  I'd very much like to have the interface (the contents of the
structure) be independent of resumption and flexible, but given the context will
probably help people understand the "why" of the new items.

http://codereview.chromium.org/8404049/diff/53001/content/browser/download/do...
content/browser/download/download_types.h:21: // This is usually the tentative
final name, but not during resumption.
nit: Specify what it is during resumption.

http://codereview.chromium.org/8404049/diff/53001/content/browser/renderer_ho...
File content/browser/renderer_host/resource_dispatcher_host_unittest.cc (right):

http://codereview.chromium.org/8404049/diff/53001/content/browser/renderer_ho...
content/browser/renderer_host/resource_dispatcher_host_unittest.cc:17: #include
"content/browser/download/download_manager.h"
Remind me why this is needed?

[ahendrickson, 2011-11-22 23:46:44]

Removed the DownloadPersistentStoreInfo changes.

http://codereview.chromium.org/8404049/diff/44001/content/browser/download/do...
File content/browser/download/download_item.h (right):

http://codereview.chromium.org/8404049/diff/44001/content/browser/download/do...
content/browser/download/download_item.h:153: void Update(int64 bytes_so_far,
const Pickle& hash_state);
On 2011/11/22 17:37:49, rdsmith wrote:
> On 2011/11/21 20:34:47, ahendrickson wrote:
> > On 2011/11/21 02:10:17, rdsmith wrote:
> > > So this makes me concerned, as pickling and unpickling can be something of
a
> > > heavyweight operation, and I'd rather not be doing it on a regular,
updating
> > > basis.  I'm not sure what the right answer is here, but by code smell I'm
> > > tempted to reverse myself on my earlier acceptance and say that we should
> > avoid
> > > updating the size/hash pair in the DB except on interrupt and completion
> (and
> > > I'd leave the hash blank on completion--I don't think it'd be used on
> restart,
> > > and as previously noted, the complete hash is a completely different beast
> > than
> > > the intermediate hash).  All of which will take some plumbing :-{.  
> > > 
> > > I could be argued out of this--it's probably not *too* expensive to pickle
> on
> > > every timer interrupt on the FILE thread.  But it really doesn't feel like
> the
> > > right thing to be doing.
> > 
> > FWIW, the size of the pickle payload is around 300 bytes (for the larger
hash
> > scheme), and the overhead is on the order of 32 more bytes.
> > 
> > Pickle copying is mainly a call to memcpy(), and WriteData() is a memcpy()
> plus
> > some accounting.
> > 
> > That doesn't seem to me an expensive operation; what do you think?
> 
> That sounds ok.  Could you figure out about how often (in time or bytes,
either
> way) the operation will occur?  I'd like to have a handle on the likely
> performance implications before signing off.

Operations are in bytes

SHA256ContextStr has 3 fields:

Type      Name                 Size
--------- -------------------- ----
byte[]    u:                    256
int32[]   h:                     32
int32     sizeHi:                 4
int32     sizeLo:                 4
                               -----
Total:                          296

We add to the pickle:

Type      Name                 Size
--------- -------------------- ----
int32     version:                4
char[]    "NSS":                  4
                               -----
Total:                            8

Pickle overhead (depending on 32/64-bit arch):

Type      Name                 Size
--------- -------------------- ----
Header*   header_                 4/8
size_t    header_size_            4/8
size_t    capacity_               4/8
size_t    variable_buffer_offset_ 4/8
                               -------
Total:                           16/32

Pickle size:                    296
                                  8
                                 16/32
                               -------
Total:                          320/352

SecureHashSHA256NSS::Serialize()
    fills pickle:                                                   320/352
  BaseFile::GetSha256HashState()
      creates string:                                               320/352
  BaseFile::GetSha256HashState()
      copies string to return:                                      320/352
    DownloadFileManager::UpdateInProgressDownloads()
        copies returned string:                                     320/352
    DownloadFileManager::UpdateInProgressDownloads()
        POSTs copy of string:                                       320/352
      DownloadManagerImpl::UpdateDownload()
        DownloadItemImpl::Update()
          DownloadItemImpl::UpdateSizeAndHashState()
            copies string:                                          320/352
        ChromeDownloadManagerDelegate::UpdateItemInPersistentStore()
              copies string:                                        320/352
          DownloadHistory::UpdateEntry()
            ...
            HistoryBackend::UpdateDownload()
              DownloadDatabase::UpdateDownload()
                  saves data in DB                                  320/352
                                                                   ---------
Total:                                                             2560/2816

This happens twice a second.

http://codereview.chromium.org/8404049/diff/53001/content/browser/download/ba...
File content/browser/download/base_file.h (right):

http://codereview.chromium.org/8404049/diff/53001/content/browser/download/ba...
content/browser/download/base_file.h:77: virtual bool SetSha256HashState(const
std::string& hash_state_bytes);
On 2011/11/22 17:37:49, rdsmith wrote:
> Given that this is also in the constructor, is this setter necessary?

It doesn't actually get set in the constructor, but in Initialize().  The latter
calls this setter.  Otherwise, we don't really need it.

http://codereview.chromium.org/8404049/diff/53001/content/browser/download/ba...
content/browser/download/base_file.h:125: std::string initial_hash_state_;
On 2011/11/22 17:37:49, rdsmith wrote:
> Why can't you just unpickle in the constructor directly into secure_hash_?

Initialize() would reset it.

http://codereview.chromium.org/8404049/diff/53001/content/browser/download/do...
File content/browser/download/download_create_info.h (right):

http://codereview.chromium.org/8404049/diff/53001/content/browser/download/do...
content/browser/download/download_create_info.h:91: // For restarting a
download, the modification time of the file.
On 2011/11/22 17:37:49, rdsmith wrote:
> nit: This is really (as I understand your vocabulary) for continuing a
download;
> it's not used on restarting.  Same comment applies to etag.

Done.

http://codereview.chromium.org/8404049/diff/53001/content/browser/download/do...
File content/browser/download/download_file.h (right):

http://codereview.chromium.org/8404049/diff/53001/content/browser/download/do...
content/browser/download/download_file.h:64: virtual bool
SetSha256HashState(const std::string& hash_state) = 0;
On 2011/11/22 17:37:49, rdsmith wrote:
> Same comment as in base file; can we get rid of this setter?

This one we can get rid of.

Done.

http://codereview.chromium.org/8404049/diff/53001/content/browser/download/do...
File content/browser/download/download_item.h (right):

http://codereview.chromium.org/8404049/diff/53001/content/browser/download/do...
content/browser/download/download_item.h:357: void UpdateSize(int64
bytes_so_far);
On 2011/11/22 17:37:49, rdsmith wrote:
> Given that the reason we have UpdateSizeAndHashState is to always keep the two
> values in sync, I'd like to have functions at this level always update both
> (where in these terms the hash state also includes the "Have we finished and
do
> we have a complete hash?")  
> 
> Maybe UpdateProgress and ProgressComplete (called when we're done)?  Those
> functions have the advantage that they're talking about the higher level goals
> of the functions, not about how they change the underlying data members.  Even
> though these are private functions, so of the same principles of interface
> design apply--you want the function names and arguments to be targeted at the
> callers context, not the implementation of the function.

Done.

http://codereview.chromium.org/8404049/diff/53001/content/browser/download/do...
content/browser/download/download_item.h:431: // hash is being computed.
On 2011/11/22 17:37:49, rdsmith wrote:
> nit: At this level "while the hash is being computed" isn't really meaningful;
> that's more a DownloadFile concept.  Maybe "until the download has completed".

Done.

http://codereview.chromium.org/8404049/diff/53001/content/browser/download/do...
File content/browser/download/download_persistent_store_info.h (right):

http://codereview.chromium.org/8404049/diff/53001/content/browser/download/do...
content/browser/download/download_persistent_store_info.h:45: const std::string&
hash_state_pickle,
On 2011/11/22 17:37:49, rdsmith wrote:
> nit: I'd like to use consistent vocabulary for things throughout the code
base,
> so I'm inclined to think this should be just "hash_state".

Done.

http://codereview.chromium.org/8404049/diff/53001/content/browser/download/do...
File content/browser/download/download_types.h (right):

http://codereview.chromium.org/8404049/diff/53001/content/browser/download/do...
content/browser/download/download_types.h:14: // Holds the information about how
to save a download file.
On 2011/11/22 17:37:49, rdsmith wrote:
> I think it makes sense to have a comment here something like "In the case of
> download resumption, file_path is set to ... , offset ... hash_state ..." for
> context.  I'd very much like to have the interface (the contents of the
> structure) be independent of resumption and flexible, but given the context
will
> probably help people understand the "why" of the new items.

Done.

http://codereview.chromium.org/8404049/diff/53001/content/browser/download/do...
content/browser/download/download_types.h:21: // This is usually the tentative
final name, but not during resumption.
On 2011/11/22 17:37:49, rdsmith wrote:
> nit: Specify what it is during resumption.

Done.

http://codereview.chromium.org/8404049/diff/53001/content/browser/renderer_ho...
File content/browser/renderer_host/resource_dispatcher_host_unittest.cc (right):

http://codereview.chromium.org/8404049/diff/53001/content/browser/renderer_ho...
content/browser/renderer_host/resource_dispatcher_host_unittest.cc:17: #include
"content/browser/download/download_manager.h"
On 2011/11/22 17:37:49, rdsmith wrote:
> Remind me why this is needed?

It can be replaced with a forward declaration of DownloadManager.

Fixed.

[Randy Smith (Not in Mondays), 2011-11-23 19:25:13]

.h file (interface) comments.  Will continue review at .cc file level.

http://codereview.chromium.org/8404049/diff/44001/content/browser/download/do...
File content/browser/download/download_item.h (right):

http://codereview.chromium.org/8404049/diff/44001/content/browser/download/do...
content/browser/download/download_item.h:153: void Update(int64 bytes_so_far,
const Pickle& hash_state);
On 2011/11/22 23:46:44, ahendrickson wrote:
> On 2011/11/22 17:37:49, rdsmith wrote:
> > On 2011/11/21 20:34:47, ahendrickson wrote:
> > > On 2011/11/21 02:10:17, rdsmith wrote:
> > > > So this makes me concerned, as pickling and unpickling can be something
of
> a
> > > > heavyweight operation, and I'd rather not be doing it on a regular,
> updating
> > > > basis.  I'm not sure what the right answer is here, but by code smell
I'm
> > > > tempted to reverse myself on my earlier acceptance and say that we
should
> > > avoid
> > > > updating the size/hash pair in the DB except on interrupt and completion
> > (and
> > > > I'd leave the hash blank on completion--I don't think it'd be used on
> > restart,
> > > > and as previously noted, the complete hash is a completely different
beast
> > > than
> > > > the intermediate hash).  All of which will take some plumbing :-{.  
> > > > 
> > > > I could be argued out of this--it's probably not *too* expensive to
pickle
> > on
> > > > every timer interrupt on the FILE thread.  But it really doesn't feel
like
> > the
> > > > right thing to be doing.
> > > 
> > > FWIW, the size of the pickle payload is around 300 bytes (for the larger
> hash
> > > scheme), and the overhead is on the order of 32 more bytes.
> > > 
> > > Pickle copying is mainly a call to memcpy(), and WriteData() is a memcpy()
> > plus
> > > some accounting.
> > > 
> > > That doesn't seem to me an expensive operation; what do you think?
> > 
> > That sounds ok.  Could you figure out about how often (in time or bytes,
> either
> > way) the operation will occur?  I'd like to have a handle on the likely
> > performance implications before signing off.
> 
> Operations are in bytes
> 
> SHA256ContextStr has 3 fields:
> 
> Type      Name                 Size
> --------- -------------------- ----
> byte[]    u:                    256
> int32[]   h:                     32
> int32     sizeHi:                 4
> int32     sizeLo:                 4
>                                -----
> Total:                          296
> 
> We add to the pickle:
> 
> Type      Name                 Size
> --------- -------------------- ----
> int32     version:                4
> char[]    "NSS":                  4
>                                -----
> Total:                            8
> 
> Pickle overhead (depending on 32/64-bit arch):
> 
> Type      Name                 Size
> --------- -------------------- ----
> Header*   header_                 4/8
> size_t    header_size_            4/8
> size_t    capacity_               4/8
> size_t    variable_buffer_offset_ 4/8
>                                -------
> Total:                           16/32
> 
> Pickle size:                    296
>                                   8
>                                  16/32
>                                -------
> Total:                          320/352
> 
> SecureHashSHA256NSS::Serialize()
>     fills pickle:                                                   320/352
>   BaseFile::GetSha256HashState()
>       creates string:                                               320/352
>   BaseFile::GetSha256HashState()
>       copies string to return:                                      320/352
>     DownloadFileManager::UpdateInProgressDownloads()
>         copies returned string:                                     320/352
>     DownloadFileManager::UpdateInProgressDownloads()
>         POSTs copy of string:                                       320/352
>       DownloadManagerImpl::UpdateDownload()
>         DownloadItemImpl::Update()
>           DownloadItemImpl::UpdateSizeAndHashState()
>             copies string:                                          320/352
>         ChromeDownloadManagerDelegate::UpdateItemInPersistentStore()
>               copies string:                                        320/352
>           DownloadHistory::UpdateEntry()
>             ...
>             HistoryBackend::UpdateDownload()
>               DownloadDatabase::UpdateDownload()
>                   saves data in DB                                  320/352
>                                                                    ---------
> Total:                                                             2560/2816
> 
> This happens twice a second.

I'm not too worried about the space costs; more about the overhead.  OTOH, it's
sounding like the copying of the bytes is the major cost.  And that's a fair
amount of copying.  

I think what I'll say is: Do some manual testing (before/after) using jar's
performance monitor--how much extra CPU time does this cost us for some
representative downloads?  If it's negligible, great.  If not, raise the flag
again--we probably don't want to do something extravagant to avoid this, but we
might (e.g.) reduce the frequency with which we send updates.

http://codereview.chromium.org/8404049/diff/53001/content/browser/download/ba...
File content/browser/download/base_file.h (right):

http://codereview.chromium.org/8404049/diff/53001/content/browser/download/ba...
content/browser/download/base_file.h:77: virtual bool SetSha256HashState(const
std::string& hash_state_bytes);
On 2011/11/22 23:46:44, ahendrickson wrote:
> On 2011/11/22 17:37:49, rdsmith wrote:
> > Given that this is also in the constructor, is this setter necessary?
> 
> It doesn't actually get set in the constructor, but in Initialize().  The
latter
> calls this setter.  Otherwise, we don't really need it.

Then shouldn't it be private?  I'm good with either private or eliminated (and
code incorporated into Initialize()).

http://codereview.chromium.org/8404049/diff/53001/content/browser/download/ba...
content/browser/download/base_file.h:125: std::string initial_hash_state_;
On 2011/11/22 23:46:44, ahendrickson wrote:
> On 2011/11/22 17:37:49, rdsmith wrote:
> > Why can't you just unpickle in the constructor directly into secure_hash_?
> 
> Initialize() would reset it.

That seems like an incomplete answer, since we control the code in Initialize()
too :-} (i.e. it begs for the question "Why couldn't we avoid resetting it in
Initialize()?").  Looking at the code, I think the answer I was looking for was:
We don't know whether we want to unpack the hash until we get to Initialize()
(because Initialize() gets the "calculate_hash_" argument).  

That brings up the question of whether the hash unpack should be in the
constructor or in Initialize().  The rule of thumb I have for constructor vs.
Initialize is that if something can return an error it should be in Initialize,
otherwise it should/could be in the constructor, and setting the hash doesn't
involve any possible error returns.  Looking at all the constructions I could
easily find (DFM for DF creation, SFM for SF creation), they're all immediately
followed by initialization.  So we *could* move the hash calculation into the
constructor if we want to.

Given that, I'm inclined to say that we should.  I really don't like tossing
extra data elements into classes; it expands complexity and testing in a subtle
way.  And there's no real need for this one--it's lifetime is just
construction->Initialize.  Alternatively, we could shift the hash argument to
initialize; that would be a simpler code change, and would allow us to get rid
of this member variable (which is my primary goal).  Abstractly, I'm inclined to
think this belongs in the constructor, though.

http://codereview.chromium.org/8404049/diff/61001/content/browser/download/ba...
File content/browser/download/base_file.cc (right):

http://codereview.chromium.org/8404049/diff/61001/content/browser/download/ba...
content/browser/download/base_file.cc:230: if (!initial_hash_state_.empty() &&
(bytes_so_far_ > 0))
I'm inclined to leave out the "bytes_so_far_" test.  I can't think of a
mechanism for initial_hash_state_ being non-empty and bytes_so_far_ being 0, but
if we want to eliminate that option we want a DCHECK or error return, not a
silent test.  And if we want to allow that option, I think that the signal of
passing in a non-zero initial hash state is the signal we should be paying
attention to.

http://codereview.chromium.org/8404049/diff/61001/content/browser/download/do...
File content/browser/download/download_file_impl.h (right):

http://codereview.chromium.org/8404049/diff/61001/content/browser/download/do...
content/browser/download/download_file_impl.h:42: virtual std::string
GetSha256HashState() OVERRIDE;
It occurs to me to wonder--if the underlying implementation hides what type of
hash is actually being used from us, should we claim it's the Sha256 state? 
Should all of these functions just be Get/SetHashState?

http://codereview.chromium.org/8404049/diff/61001/content/browser/download/do...
File content/browser/download/download_item_impl.h (right):

http://codereview.chromium.org/8404049/diff/61001/content/browser/download/do...
content/browser/download/download_item_impl.h:161: // Internal helper for
maintaining consistent received and total sizes.
nit: Also hash state (conceptually, though you might want to null it in this
function if you don't currently).  You could combine these two comments if you
wanted.

http://codereview.chromium.org/8404049/diff/61001/content/browser/renderer_ho...
File content/browser/renderer_host/resource_dispatcher_host_unittest.cc (right):

http://codereview.chromium.org/8404049/diff/61001/content/browser/renderer_ho...
content/browser/renderer_host/resource_dispatcher_host_unittest.cc:36: class
DownloadManager;
I'm still confused as to what change in this CL made this addition to
resource_dispatcher_host_unittest.cc necessary?  (Assuming there is one, but
wanting to understand.)

[Randy Smith (Not in Mondays), 2011-11-23 19:40:24]

Only one non-interface comment (makes sense, as this CL is primarily about
interface changes.)

http://codereview.chromium.org/8404049/diff/61001/content/browser/download/do...
File content/browser/download/download_item_impl.cc (right):

http://codereview.chromium.org/8404049/diff/61001/content/browser/download/do...
content/browser/download/download_item_impl.cc:878: const std::string&
DownloadItemImpl::GetHashState() const {
Just noticed that we use HashState here and Sha256HashState on *File.  Is there
a reason for the difference?  (This comment should probably be bundled with my
"Should we be using Sha256?" comment.)

[ahendrickson, 2011-12-20 00:04:13]

PTAL.

http://codereview.chromium.org/8404049/diff/44001/content/browser/download/do...
File content/browser/download/download_item.h (right):

http://codereview.chromium.org/8404049/diff/44001/content/browser/download/do...
content/browser/download/download_item.h:153: void Update(int64 bytes_so_far,
const Pickle& hash_state);
On 2011/11/23 19:25:13, rdsmith wrote:
> I'm not too worried about the space costs; more about the overhead.  OTOH,
it's
> sounding like the copying of the bytes is the major cost.  And that's a fair
> amount of copying.  
> 
> I think what I'll say is: Do some manual testing (before/after) using jar's
> performance monitor--how much extra CPU time does this cost us for some
> representative downloads?  If it's negligible, great.  If not, raise the flag
> again--we probably don't want to do something extravagant to avoid this, but
we
> might (e.g.) reduce the frequency with which we send updates.

I downloaded a 50MB file, which took about 10-15 seconds.

http://ftp.belnet.be/packages/damnsmalllinux/current/dsl-4.4.10-syslinux.iso

From the profiler:

DRH::OnReadCompleted() was called 1,694 times, for a total run time of 2.29 sec
(avg 0.001 sec).

DFM::UpdateInProgressDownloads() was called 56 times, for a total run time of
0.090 sec (avg 0.002 sec).

Other points of interest, not related to the above:

SBS::StartDownloadCheck() was called twice, with a total run time 0f 0.5 sec
(avg 0.26 sec).

CDMD::CheckIfSuggestedPathExists() was called once, with a run time of 0.27 sec.

DFM::StartDownload() was called once, with a run time of 1.67 sec.

http://codereview.chromium.org/8404049/diff/53001/content/browser/download/ba...
File content/browser/download/base_file.h (right):

http://codereview.chromium.org/8404049/diff/53001/content/browser/download/ba...
content/browser/download/base_file.h:77: virtual bool SetSha256HashState(const
std::string& hash_state_bytes);
On 2011/11/23 19:25:13, rdsmith wrote:
> On 2011/11/22 23:46:44, ahendrickson wrote:
> > On 2011/11/22 17:37:49, rdsmith wrote:
> > > Given that this is also in the constructor, is this setter necessary?
> > 
> > It doesn't actually get set in the constructor, but in Initialize().  The
> latter
> > calls this setter.  Otherwise, we don't really need it.
> 
> Then shouldn't it be private?  I'm good with either private or eliminated (and
> code incorporated into Initialize()).
> 

Made it private.

http://codereview.chromium.org/8404049/diff/53001/content/browser/download/ba...
content/browser/download/base_file.h:125: std::string initial_hash_state_;
On 2011/11/23 19:25:13, rdsmith wrote:
> That brings up the question of whether the hash unpack should be in the
> constructor or in Initialize().  The rule of thumb I have for constructor vs.
> Initialize is that if something can return an error it should be in
Initialize,
> otherwise it should/could be in the constructor, and setting the hash doesn't
> involve any possible error returns.  Looking at all the constructions I could
> easily find (DFM for DF creation, SFM for SF creation), they're all
immediately
> followed by initialization.  So we *could* move the hash calculation into the
> constructor if we want to.
> 
> Given that, I'm inclined to say that we should.  I really don't like tossing
> extra data elements into classes; it expands complexity and testing in a
subtle
> way.  And there's no real need for this one--it's lifetime is just
> construction->Initialize.  Alternatively, we could shift the hash argument to
> initialize; that would be a simpler code change, and would allow us to get rid
> of this member variable (which is my primary goal).  Abstractly, I'm inclined
to
> think this belongs in the constructor, though.

Moved the hash initialization into the constructor.

http://codereview.chromium.org/8404049/diff/61001/content/browser/download/ba...
File content/browser/download/base_file.cc (right):

http://codereview.chromium.org/8404049/diff/61001/content/browser/download/ba...
content/browser/download/base_file.cc:230: if (!initial_hash_state_.empty() &&
(bytes_so_far_ > 0))
On 2011/11/23 19:25:13, rdsmith wrote:
> I'm inclined to leave out the "bytes_so_far_" test.  I can't think of a
> mechanism for initial_hash_state_ being non-empty and bytes_so_far_ being 0,
but
> if we want to eliminate that option we want a DCHECK or error return, not a
> silent test.  And if we want to allow that option, I think that the signal of
> passing in a non-zero initial hash state is the signal we should be paying
> attention to.

I want to be able to zero the bytes_so_far_ value to indicate that we want to
start the download from the beginning, without having to clear the hash state as
well.

http://codereview.chromium.org/8404049/diff/61001/content/browser/download/do...
File content/browser/download/download_item_impl.cc (right):

http://codereview.chromium.org/8404049/diff/61001/content/browser/download/do...
content/browser/download/download_item_impl.cc:878: const std::string&
DownloadItemImpl::GetHashState() const {
On 2011/11/23 19:40:24, rdsmith wrote:
> Just noticed that we use HashState here and Sha256HashState on *File.  Is
there
> a reason for the difference?  (This comment should probably be bundled with my
> "Should we be using Sha256?" comment.)

Changed to using just "Hash" instead of "Sha256Hash".

http://codereview.chromium.org/8404049/diff/61001/content/browser/download/do...
File content/browser/download/download_item_impl.h (right):

http://codereview.chromium.org/8404049/diff/61001/content/browser/download/do...
content/browser/download/download_item_impl.h:161: // Internal helper for
maintaining consistent received and total sizes.
On 2011/11/23 19:25:13, rdsmith wrote:
> nit: Also hash state (conceptually, though you might want to null it in this
> function if you don't currently).  You could combine these two comments if you
> wanted.

Done.

http://codereview.chromium.org/8404049/diff/61001/content/browser/renderer_ho...
File content/browser/renderer_host/resource_dispatcher_host_unittest.cc (right):

http://codereview.chromium.org/8404049/diff/61001/content/browser/renderer_ho...
content/browser/renderer_host/resource_dispatcher_host_unittest.cc:36: class
DownloadManager;
On 2011/11/23 19:25:13, rdsmith wrote:
> I'm still confused as to what change in this CL made this addition to
> resource_dispatcher_host_unittest.cc necessary?  (Assuming there is one, but
> wanting to understand.)

I removed:

#include "download_file.h"

from download_resource_handler.h, which was included in
resource_dispatcher_host.h (above).

download_file.h had a forward declaration of DownloadManager, which is used in
this file.

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

Very close.  Thanks for being patient with my reivews....

http://codereview.chromium.org/8404049/diff/44001/content/browser/download/do...
File content/browser/download/download_item.h (right):

http://codereview.chromium.org/8404049/diff/44001/content/browser/download/do...
content/browser/download/download_item.h:153: void Update(int64 bytes_so_far,
const Pickle& hash_state);
On 2011/12/20 00:04:13, ahendrickson wrote:
> On 2011/11/23 19:25:13, rdsmith wrote:
> > I'm not too worried about the space costs; more about the overhead.  OTOH,
> it's
> > sounding like the copying of the bytes is the major cost.  And that's a fair
> > amount of copying.  
> > 
> > I think what I'll say is: Do some manual testing (before/after) using jar's
> > performance monitor--how much extra CPU time does this cost us for some
> > representative downloads?  If it's negligible, great.  If not, raise the
flag
> > again--we probably don't want to do something extravagant to avoid this, but
> we
> > might (e.g.) reduce the frequency with which we send updates.
> 
> I downloaded a 50MB file, which took about 10-15 seconds.
> 
> http://ftp.belnet.be/packages/damnsmalllinux/current/dsl-4.4.10-syslinux.iso
> 
> From the profiler:
> 
> DRH::OnReadCompleted() was called 1,694 times, for a total run time of 2.29
sec
> (avg 0.001 sec).
> 
> DFM::UpdateInProgressDownloads() was called 56 times, for a total run time of
> 0.090 sec (avg 0.002 sec).
> 
> Other points of interest, not related to the above:
> 
> SBS::StartDownloadCheck() was called twice, with a total run time 0f 0.5 sec
> (avg 0.26 sec).
> 
> CDMD::CheckIfSuggestedPathExists() was called once, with a run time of 0.27
sec.
> 
> DFM::StartDownload() was called once, with a run time of 1.67 sec.

Cool.  What's the comparison?  (I.e. what that would look like without your
changes?)

http://codereview.chromium.org/8404049/diff/61001/content/browser/download/ba...
File content/browser/download/base_file.cc (right):

http://codereview.chromium.org/8404049/diff/61001/content/browser/download/ba...
content/browser/download/base_file.cc:230: if (!initial_hash_state_.empty() &&
(bytes_so_far_ > 0))
On 2011/12/20 00:04:14, ahendrickson wrote:
> On 2011/11/23 19:25:13, rdsmith wrote:
> > I'm inclined to leave out the "bytes_so_far_" test.  I can't think of a
> > mechanism for initial_hash_state_ being non-empty and bytes_so_far_ being 0,
> but
> > if we want to eliminate that option we want a DCHECK or error return, not a
> > silent test.  And if we want to allow that option, I think that the signal
of
> > passing in a non-zero initial hash state is the signal we should be paying
> > attention to.
> 
> I want to be able to zero the bytes_so_far_ value to indicate that we want to
> start the download from the beginning, without having to clear the hash state
as
> well.

Why?  That seems like it's just inconsistent data, which is ok if you ignore one
piece.  But why not keep them consistent?

http://codereview.chromium.org/8404049/diff/73004/content/browser/download/ba...
File content/browser/download/base_file.h (right):

http://codereview.chromium.org/8404049/diff/73004/content/browser/download/ba...
content/browser/download/base_file.h:73: // Fills |hash| with the sha256 digest
for the file.
Basic question: Is the fact that the hash is an sha256 hash part of the
interface?  I.e. does anything outside of download file and crypto need to know
about it?  If yes, then I'll retract my earlier request that the interfaces not
name the type of hash.  If not, then the comment shouldn't refer to it either.

Now that I ask the question, I'm somewhat afraid that the safebrowsing system
*does* need to know that it's sha256, since it's comparing against stored
hashes.  Which means I was wrong in my initial comment.  But either way, the
comments and interface should be in alignment, and the naming/commenting should
be based on whether external customers need to know what kind of hash it is.

http://codereview.chromium.org/8404049/diff/73004/content/browser/download/ba...
File content/browser/download/base_file_unittest.cc (right):

http://codereview.chromium.org/8404049/diff/73004/content/browser/download/ba...
content/browser/download/base_file_unittest.cc:227:
EXPECT_EQ("0B2D3F3F7943AD64B860DF94D05CB56A8A97C6EC5768B5B70B930C5AA7FA9ADE",
nit, suggestion: An ideal world would have this automatically computed from the
test data you threw at the file (it'd be fine to use the crypto libraries for
this--you aren't testing them).  It's also ok this way; doing it the other way
just makes this test less easily broken by changes.

[ahendrickson, 2011-12-20 22:06:54]

http://codereview.chromium.org/8404049/diff/44001/content/browser/download/do...
File content/browser/download/download_item.h (right):

http://codereview.chromium.org/8404049/diff/44001/content/browser/download/do...
content/browser/download/download_item.h:153: void Update(int64 bytes_so_far,
const Pickle& hash_state);
On 2011/12/20 17:05:40, rdsmith wrote:
> On 2011/12/20 00:04:13, ahendrickson wrote:
> > I downloaded a 50MB file, which took about 10-15 seconds.
> > 
> > http://ftp.belnet.be/packages/damnsmalllinux/current/dsl-4.4.10-syslinux.iso
> > 
> > From the profiler:
> > 
> > DRH::OnReadCompleted() was called 1,694 times, for a total run time of 2.29
> sec
> > (avg 0.001 sec).
> > 
> > DFM::UpdateInProgressDownloads() was called 56 times, for a total run time
of
> > 0.090 sec (avg 0.002 sec).
> > 
> > Other points of interest, not related to the above:
> > 
> > SBS::StartDownloadCheck() was called twice, with a total run time 0f 0.5 sec
> > (avg 0.26 sec).
> > 
> > CDMD::CheckIfSuggestedPathExists() was called once, with a run time of 0.27
> sec.
> > 
> > DFM::StartDownload() was called once, with a run time of 1.67 sec.
> 
> Cool.  What's the comparison?  (I.e. what that would look like without your
> changes?)

Without the hash state being passed around:


DRH::OnReadCompleted() was called 1,688 times, for a total run time of 2.20 sec
(avg 0.001 sec).

DFM::UpdateInProgressDownloads() was called 40 times, for a total run time of
0.20 sec (avg 0.005 sec).

Other points of interest, not related to the above:

SBS::StartDownloadCheck() was called twice, with a total run time 0f 1.5 sec
(avg 0.78 sec).

CDMD::CheckIfSuggestedPathExists() was called once, with a run time of 0.011
sec.

DFM::StartDownload() was called once, with a run time of 1.11 sec.

--

I'd say the hash copying is lost in the noise.

http://codereview.chromium.org/8404049/diff/61001/content/browser/download/ba...
File content/browser/download/base_file.cc (right):

http://codereview.chromium.org/8404049/diff/61001/content/browser/download/ba...
content/browser/download/base_file.cc:230: if (!initial_hash_state_.empty() &&
(bytes_so_far_ > 0))
On 2011/12/20 17:05:40, rdsmith wrote:
> On 2011/12/20 00:04:14, ahendrickson wrote:
> > On 2011/11/23 19:25:13, rdsmith wrote:
> > > I'm inclined to leave out the "bytes_so_far_" test.  I can't think of a
> > > mechanism for initial_hash_state_ being non-empty and bytes_so_far_ being
0,
> > but
> > > if we want to eliminate that option we want a DCHECK or error return, not
a
> > > silent test.  And if we want to allow that option, I think that the signal
> of
> > > passing in a non-zero initial hash state is the signal we should be paying
> > > attention to.
> > 
> > I want to be able to zero the bytes_so_far_ value to indicate that we want
to
> > start the download from the beginning, without having to clear the hash
state
> as
> > well.
> 
> Why?  That seems like it's just inconsistent data, which is ok if you ignore
one
> piece.  But why not keep them consistent?

I will try to keep them consistent.  I just think that |bytes_so_far_| is the
more important factor -- if it's 0, we have to ignore any passed-in hash state.

http://codereview.chromium.org/8404049/diff/73004/content/browser/download/ba...
File content/browser/download/base_file.h (right):

http://codereview.chromium.org/8404049/diff/73004/content/browser/download/ba...
content/browser/download/base_file.h:73: // Fills |hash| with the sha256 digest
for the file.
On 2011/12/20 17:05:40, rdsmith wrote:
> Basic question: Is the fact that the hash is an sha256 hash part of the
> interface?  I.e. does anything outside of download file and crypto need to
know
> about it?  If yes, then I'll retract my earlier request that the interfaces
not
> name the type of hash.  If not, then the comment shouldn't refer to it either.
> 
> Now that I ask the question, I'm somewhat afraid that the safebrowsing system
> *does* need to know that it's sha256, since it's comparing against stored
> hashes.  Which means I was wrong in my initial comment.  But either way, the
> comments and interface should be in alignment, and the naming/commenting
should
> be based on whether external customers need to know what kind of hash it is.  

Changed the comments to match the functions.

http://codereview.chromium.org/8404049/diff/73004/content/browser/download/ba...
File content/browser/download/base_file_unittest.cc (right):

http://codereview.chromium.org/8404049/diff/73004/content/browser/download/ba...
content/browser/download/base_file_unittest.cc:227:
EXPECT_EQ("0B2D3F3F7943AD64B860DF94D05CB56A8A97C6EC5768B5B70B930C5AA7FA9ADE",
On 2011/12/20 17:05:40, rdsmith wrote:
> nit, suggestion: An ideal world would have this automatically computed from
the
> test data you threw at the file (it'd be fine to use the crypto libraries for
> this--you aren't testing them).  It's also ok this way; doing it the other way
> just makes this test less easily broken by changes.

Done.

[Randy Smith (Not in Mondays), 2011-12-21 19:14:49]

LGTM, with the caveat agreed on offline that I'd like the initial hash state
nulled in the server forced restart case (responds to a range request with the
whole thing) at the UI->File post task boundary.

http://codereview.chromium.org/8404049/diff/44001/content/browser/download/do...
File content/browser/download/download_item.h (right):

http://codereview.chromium.org/8404049/diff/44001/content/browser/download/do...
content/browser/download/download_item.h:153: void Update(int64 bytes_so_far,
const Pickle& hash_state);
On 2011/12/20 22:06:54, ahendrickson wrote:
> On 2011/12/20 17:05:40, rdsmith wrote:
> > On 2011/12/20 00:04:13, ahendrickson wrote:
> > > I downloaded a 50MB file, which took about 10-15 seconds.
> > > 
> > >
http://ftp.belnet.be/packages/damnsmalllinux/current/dsl-4.4.10-syslinux.iso
> > > 
> > > From the profiler:
> > > 
> > > DRH::OnReadCompleted() was called 1,694 times, for a total run time of
2.29
> > sec
> > > (avg 0.001 sec).
> > > 
> > > DFM::UpdateInProgressDownloads() was called 56 times, for a total run time
> of
> > > 0.090 sec (avg 0.002 sec).
> > > 
> > > Other points of interest, not related to the above:
> > > 
> > > SBS::StartDownloadCheck() was called twice, with a total run time 0f 0.5
sec
> > > (avg 0.26 sec).
> > > 
> > > CDMD::CheckIfSuggestedPathExists() was called once, with a run time of
0.27
> > sec.
> > > 
> > > DFM::StartDownload() was called once, with a run time of 1.67 sec.
> > 
> > Cool.  What's the comparison?  (I.e. what that would look like without your
> > changes?)
> 
> Without the hash state being passed around:
> 
> 
> DRH::OnReadCompleted() was called 1,688 times, for a total run time of 2.20
sec
> (avg 0.001 sec).
> 
> DFM::UpdateInProgressDownloads() was called 40 times, for a total run time of
> 0.20 sec (avg 0.005 sec).
> 
> Other points of interest, not related to the above:
> 
> SBS::StartDownloadCheck() was called twice, with a total run time 0f 1.5 sec
> (avg 0.78 sec).
> 
> CDMD::CheckIfSuggestedPathExists() was called once, with a run time of 0.011
> sec.
> 
> DFM::StartDownload() was called once, with a run time of 1.11 sec.
> 
> --
> 
> I'd say the hash copying is lost in the noise.

Agreed.  Thanks for the due diligence.

[commit-bot: I haz the power, 2011-12-21 19:56:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ahendrickson@chromium.org/8404049/84002

[commit-bot: I haz the power, 2011-12-21 19:56:33]

Presubmit check for 8404049-84002 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
content/public/browser/download_file.h,content/public/browser/download_manager.h,content/public/browser/download_item.h,content/browser/renderer_host/resource_dispatcher_host_unittest.cc

Presubmit checks took 4.0s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[ahendrickson, 2011-12-21 20:01:21]

jam, PTAL at the stuff outside of content/browser/download/

[jam, 2011-12-21 21:48:31]

lgtm

[commit-bot: I haz the power, 2011-12-22 03:36:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ahendrickson@chromium.org/8404049/84002

[commit-bot: I haz the power, 2011-12-22 03:36:33]

Presubmit check for 8404049-84002 failed and returned exit status 1.

Running presubmit commit checks ...
Traceback (most recent call last):
  File "/mnt/data/b/commit-queue/verification/presubmit_shim.py", line 43, in
<module>
    sys.exit(presubmit_support.Main(argv))
  File "/b/depot_tools/presubmit_support.py", line 1256, in Main
    rietveld_obj)
  File "/b/depot_tools/presubmit_support.py", line 1104, in DoPresubmitChecks
    results += executer.ExecPresubmitScript(presubmit_script, filename)
  File "/b/depot_tools/presubmit_support.py", line 1021, in ExecPresubmitScript
    result = eval(function_name + '(*__args)', context)
  File "<string>", line 1, in <module>
  File "<string>", line 313, in CheckChangeOnCommit
  File "<string>", line 218, in _CommonChecks
  File "/b/depot_tools/presubmit_canned_checks.py", line 888, in
PanProjectChecks
    input_api, output_api, source_file_filter=None))
  File "/b/depot_tools/presubmit_canned_checks.py", line 746, in CheckOwners
    approvers_plus_owner)
  File "/b/depot_tools/owners.py", line 93, in files_not_covered_by
    self._load_data_needed_for(files)
  File "/b/depot_tools/owners.py", line 136, in _load_data_needed_for
    self._read_owners_in_dir(dirpath)
  File "/b/depot_tools/owners.py", line 164, in _read_owners_in_dir
    'or an email address: "%s"' % line))
owners.SyntaxErrorInOwnersFile:
/mnt/data/b/commit-queue/workdir/chromium/content/public/browser/OWNERS:1 syntax
error: line is not a comment, a "set" directive, or an email address:
"rdsmith@chromium.org  # for download related code"

[commit-bot: I haz the power, 2011-12-22 12:02:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ahendrickson@chromium.org/8404049/84002

[commit-bot: I haz the power, 2011-12-22 12:03:09]

Presubmit check for 8404049-84002 failed and returned exit status 1.

Running presubmit commit checks ...
Traceback (most recent call last):
  File "/mnt/data/b/commit-queue/verification/presubmit_shim.py", line 43, in
<module>
    sys.exit(presubmit_support.Main(argv))
  File "/b/depot_tools/presubmit_support.py", line 1256, in Main
    rietveld_obj)
  File "/b/depot_tools/presubmit_support.py", line 1104, in DoPresubmitChecks
    results += executer.ExecPresubmitScript(presubmit_script, filename)
  File "/b/depot_tools/presubmit_support.py", line 1021, in ExecPresubmitScript
    result = eval(function_name + '(*__args)', context)
  File "<string>", line 1, in <module>
  File "<string>", line 313, in CheckChangeOnCommit
  File "<string>", line 218, in _CommonChecks
  File "/b/depot_tools/presubmit_canned_checks.py", line 888, in
PanProjectChecks
    input_api, output_api, source_file_filter=None))
  File "/b/depot_tools/presubmit_canned_checks.py", line 746, in CheckOwners
    approvers_plus_owner)
  File "/b/depot_tools/owners.py", line 93, in files_not_covered_by
    self._load_data_needed_for(files)
  File "/b/depot_tools/owners.py", line 136, in _load_data_needed_for
    self._read_owners_in_dir(dirpath)
  File "/b/depot_tools/owners.py", line 164, in _read_owners_in_dir
    'or an email address: "%s"' % line))
owners.SyntaxErrorInOwnersFile:
/mnt/data/b/commit-queue/workdir/chromium/content/public/browser/OWNERS:1 syntax
error: line is not a comment, a "set" directive, or an email address:
"rdsmith@chromium.org  # for download related code"

[commit-bot: I haz the power, 2011-12-22 17:03:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ahendrickson@chromium.org/8404049/84002

[commit-bot: I haz the power, 2011-12-22 19:22:44]

Change committed as 115573