sql::MetaTable.next_download_id, DownloadManager::GetNextId()

Make a new integer field in sql::MetaTable (a per-profile db) containing a counter for the next download id, so that this id is unique across sessions. This id will allow us to merge download id with db_handle and merge most/all of the maps in DownloadManager in future CLs.
Make DownloadManager read this field to initialize its next_id_ counter in Init().
Put a fine-grained mutex in DownloadManager::GetNextId() so that it can be called directly from any thread.
Define a thunk wrapping DM::GNI() to be passed around between threads to guard against other threads calling any other DM methods.
This thunk owns a scoped_refptr<DM> to manage life-time issues. This pattern is implemented for DM elsewhere.
Store this thunk in ResourceContext to be called by ResourceDispatchHost/DownloadThrottlingResourceHandler on the IO thread. Pass the returned DownloadId into DownloadResourceHandler.
The alternative way to obtain ids on the IO thread is to jump over to the UI thread and back. This way would add significant latency to a critical path. GetNextId() should be fast and easily accessible from any thread.
Now that ids are per-profile, define a class DownloadId containing a per-profile id and an indication of which profile, currently the DownloadManager*. DownloadIds are hashable, comparable, globally unique, not persistent, and are used by DownloadFileManager.
When the download is added to the history, MetaTable.next_download_id will be set to the new download's id +1 if that number is greater than MT.next_download_id. Increasing this counter at the same time as the download is added to the db prevents the counter from desyncing from the db, which was the primary concern re storing the counter in the BrowserPrefs.
See also http://codereview.chromium.org/7192016
LMK what to write a test for if anything.

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

[benjhayden, 2011-07-08 18:54:29]

This Rietveld thing and git cl upload aren't quite like Google's internal
equivalents, so I'm not sure how to signal that this CL is ready for review
except to publish this message.

[Randy Smith (Not in Mondays), 2011-07-10 18:57:45]

[Added Andy to the review list, since he's done the most work in this area, but
Andy, I'd keep comments at a high level until we work out the overarching design
for these CLs.  ]

As far as I know, the way you mark a CL as ready for review in Rietveld is by
sending email requesting review, so this is fine :-}.  A couple of high level
comments:

* One thing I didn't see in your sketch was how to make sure that the new IDs
assigned don't conflict with ids already in the database from previous sessions.
 My assumption had been that, in the case where the preference was
uninitialized, we'd need to delay all downloads until we had read in the entire
downloads database and initialized the browser pref from that read in.  If you
could include in your CL sketch which CL that work will happen in, I'd feel
better about it.

* WRT the above, I was going to say "I don't care what CL it happens in" but if
we can avoid it I'd like to never have a state on ToT in which download ids
could be created that might conflict with previous downloads.  So I think the
order has to be "Create browser pref, write code to initialize it from downloads
history if it's not initialized, start using new download pref in system it to
be used", broken up into CLs as you prefer.

* For reasons unrelated to the work you're doing, I've conceived an interested
in having downloads ids allocatable from any thread (or at least, from the UI
thread).  This is specifically to allow "InvokeDownload" to occur as a
subroutine from the UI thread, which the download id is returned from the
subroutine call and can later be used to "hook up"  with the created download
item.  There are a couple of places in the code where this hookup is done by
scanning for matching URLs, and there are going to be more in the future
(download resumption, an external group requirement that escapes me at the
moment).  No need for you to fully implement this, but be aware of a possible
future requirement for invoking downloads from the UI thread with immediate
return of download id, and don't make it harder :-}.  

For background, the image I had had was making DownloadFileManager::GetNextId()
thread safe via an atomic increment operation, which could be made to work with
the scheme in this CL if we initialized the DFM from the browser_pref and
updated the browser pref on the IO thread at some point before or at the point
where we entered the download into the history.  The scheme in this CL, of just
using the browser pref directly, could also be made to work, but keep in mind
that downloads may in the future be initiated from either UI or IO threads, and
in the UI case may need to bop back to the IO thread to setup the URLRequest
after the download id is allocated.  Just stuff to keep in the back of your
mind.

[benjhayden, 2011-07-10 21:19:06]

On Sun, Jul 10, 2011 at 2:57 PM, <rdsmith@chromium.org> wrote:

> [Added Andy to the review list, since he's done the most work in this area,
> but
> Andy, I'd keep comments at a high level until we work out the overarching
> design
> for these CLs.  ]
>
> As far as I know, the way you mark a CL as ready for review in Rietveld is
> by
> sending email requesting review, so this is fine :-}.  A couple of high
> level
> comments:
>
> * One thing I didn't see in your sketch was how to make sure that the new
> IDs
> assigned don't conflict with ids already in the database from previous
> sessions.
>  My assumption had been that, in the case where the preference was
> uninitialized, we'd need to delay all downloads until we had read in the
> entire
> downloads database and initialized the browser pref from that read in.  If
> you
> could include in your CL sketch which CL that work will happen in, I'd feel
> better about it.
>

This CL doesn't merge the db_handle with the id or store the id in the db.
I was thinking that this CL could basically not change functionality except
to make the id counter persistent. So, at least after this CL, we'll still
have both a db_handle and an id, and the id won't be in the database, and
we'll still have all those maps in DownloadManager. All this CL does is
persist the id counter, just to set up and facilitate the CLs that will
change functionality and the database. Small steps.

In the CL that changes the database, I'm thinking that I'd change
QueryDownloads()/Load() to first check if the table has a column named
"pid", and if not,
read the entire table into a vector (set?) of DownloadHistoryInfos, whose
'db_handle' field will be renamed to 'id' and changed to
DownloadPrefs::GetNextId(),
delete the downloads table,
re-create the downloads table just to rename the "id" column to "pid",
write all the DownloadHistoryInfos into the new downloads table.

All of the methods will use "pid" instead of "id", and the Create method
will set pid = DownloadHistoryInfo.id instead of letting SQL determine the
db_handle.
Renaming "id" to "pid" is just to allow detecting which version of the
schema is being used. Usually, a "meta" table is used for such information,
so perhaps I'll code that up in a separate CL for when we need to change the
schema again.
I think you suggested a schema like this at one point and I described
something a little different, but now I've figured out how to make it work.


> * WRT the above, I was going to say "I don't care what CL it happens in"
> but if
> we can avoid it I'd like to never have a state on ToT in which download ids
> could be created that might conflict with previous downloads.  So I think
> the
> order has to be "Create browser pref, write code to initialize it from
> downloads
> history if it's not initialized, start using new download pref in system it
> to
> be used", broken up into CLs as you prefer.


> * For reasons unrelated to the work you're doing, I've conceived an
> interested
> in having downloads ids allocatable from any thread (or at least, from the
> UI
> thread).  This is specifically to allow "InvokeDownload" to occur as a
> subroutine from the UI thread, which the download id is returned from the
> subroutine call and can later be used to "hook up"  with the created
> download
> item.  There are a couple of places in the code where this hookup is done
> by
> scanning for matching URLs, and there are going to be more in the future
> (download resumption, an external group requirement that escapes me at the
> moment).  No need for you to fully implement this, but be aware of a
> possible
> future requirement for invoking downloads from the UI thread with immediate
> return of download id, and don't make it harder :-}.


> For background, the image I had had was making DownloadFileManager::**
> GetNextId()
> thread safe via an atomic increment operation, which could be made to work
> with
> the scheme in this CL if we initialized the DFM from the browser_pref and
> updated the browser pref on the IO thread at some point before or at the
> point
> where we entered the download into the history.  The scheme in this CL, of
> just
> using the browser pref directly, could also be made to work, but keep in
> mind
> that downloads may in the future be initiated from either UI or IO threads,
> and
> in the UI case may need to bop back to the IO thread to setup the
> URLRequest
> after the download id is allocated.  Just stuff to keep in the back of your
> mind.
>

This CL moves GetNextId to the UI thread, and it looks like
DownloadManager::CreateDownloadItem() and StartDownload() can only be called
on the UI thread. If you want, we could remove id from DownloadCreateInfo
and make CreateDownloadItem() call GetNextId() and return the id of the
newly created DownloadItem.
If a download is going to be Created or Started on the IO thread, then it's
going to require reworking DownloadManager, not just GetNextId, right?.


>
>
>
>
>
http://codereview.chromium.**org/7237034/<http://codereview.chromium.org/7237...
>

[Randy Smith (Not in Mondays), 2011-07-10 23:46:00]

On 2011/07/10 21:19:06, benjhayden wrote:
> On Sun, Jul 10, 2011 at 2:57 PM, <mailto:rdsmith@chromium.org> wrote:
> This CL doesn't merge the db_handle with the id or store the id in the db.
> I was thinking that this CL could basically not change functionality except
> to make the id counter persistent. So, at least after this CL, we'll still
> have both a db_handle and an id, and the id won't be in the database, and
> we'll still have all those maps in DownloadManager. All this CL does is
> persist the id counter, just to set up and facilitate the CLs that will
> change functionality and the database. Small steps.

Whoops, sorry, quite right.  Braino on my part; I was living a bit too much in
the future after the two ids were merged.  Good plan.

> In the CL that changes the database, I'm thinking that I'd change
> QueryDownloads()/Load() to first check if the table has a column named
> "pid", and if not,
> read the entire table into a vector (set?) of DownloadHistoryInfos, whose
> 'db_handle' field will be renamed to 'id' and changed to
> DownloadPrefs::GetNextId(),
> delete the downloads table,
> re-create the downloads table just to rename the "id" column to "pid",
> write all the DownloadHistoryInfos into the new downloads table.

Ah, bringing the mountain to Mohammed (i.e. the reverse of what I was thinking,
not meaning it's any harder).  That sounds good to me--specifically, I think it
solves the synchronization issue I was worried about by relying on fine grained
synchronization through GetNextId().  Does it mean you'll need to be sending
messages to the UI thread to get the next id for each item?  There'll also be
synchronization with the initial query from the DownloadManager to the history
DB for the list of history items, but that's probably simpler than the
whole-browser synchronization I was concerned about.

> All of the methods will use "pid" instead of "id", and the Create method
> will set pid = DownloadHistoryInfo.id instead of letting SQL determine the
> db_handle.
> Renaming "id" to "pid" is just to allow detecting which version of the
> schema is being used. Usually, a "meta" table is used for such information,
> so perhaps I'll code that up in a separate CL for when we need to change the
> schema again.

My naivete' is showing: "Usually" in chrome, or SQL?  Chrome has a meta-pattern
(which I'm not very familiar with) for database updates which you should
probably makes sure you understand and match as far as is possible for our
needs.

> I think you suggested a schema like this at one point and I described
> something a little different, but now I've figured out how to make it work.

No, this sounds good.  I wanted to understand it to something like this level of
granularity before the code starting being landed.

> > * For reasons unrelated to the work you're doing, I've conceived an
> > interested
> > in having downloads ids allocatable from any thread (or at least, from the
> > UI
> > thread).  This is specifically to allow "InvokeDownload" to occur as a
> > subroutine from the UI thread, which the download id is returned from the
> > subroutine call and can later be used to "hook up"  with the created
> > download
> > item.  There are a couple of places in the code where this hookup is done
> > by
> > scanning for matching URLs, and there are going to be more in the future
> > (download resumption, an external group requirement that escapes me at the
> > moment).  No need for you to fully implement this, but be aware of a
> > possible
> > future requirement for invoking downloads from the UI thread with immediate
> > return of download id, and don't make it harder :-}.
> 
> > For background, the image I had had was making DownloadFileManager::**
> > GetNextId()
> > thread safe via an atomic increment operation, which could be made to work
> > with
> > the scheme in this CL if we initialized the DFM from the browser_pref and
> > updated the browser pref on the IO thread at some point before or at the
> > point
> > where we entered the download into the history.  The scheme in this CL, of
> > just
> > using the browser pref directly, could also be made to work, but keep in
> > mind
> > that downloads may in the future be initiated from either UI or IO threads,
> > and
> > in the UI case may need to bop back to the IO thread to setup the
> > URLRequest
> > after the download id is allocated.  Just stuff to keep in the back of your
> > mind.
> 
> This CL moves GetNextId to the UI thread, and it looks like
> DownloadManager::CreateDownloadItem() and StartDownload() can only be called
> on the UI thread. If you want, we could remove id from DownloadCreateInfo
> and make CreateDownloadItem() call GetNextId() and return the id of the
> newly created DownloadItem.
> If a download is going to be Created or Started on the IO thread, then it's
> going to require reworking DownloadManager, not just GetNextId, right?.

Hmmm.  I'm not quite following you.  At a high level, the primary way that
downloads are initiated is from the IO thread, through
DownloadResourceHandler::OnResponseStarted(), when the ResourceDispatcherHost
runs into something that it can't figure out anything better to do than download
it.  So in some sense the common pathway for downloads starting is the IO
thread.  There are currently some pathways that start downloads on the UI
thread; they go through
DownloadManager::DownloadUrl->DownloadManager::DownloadUrlToFile-> [crossing to
IO thread] download_util::DownloadUrl -> ResourceDispatcherHost::BeginDownload,
which sets up a DownloadResourceHandler.  The objection I have to the current
UI->IO pathway is that several of the customers of that pathway (Save Image/Link
As ..., Drag and drop) wait for the download to appear back on the UI thread (on
the download manager) and figure out when "their" download has appeared via URL
matching, which is an inexact match.  The alternative is for the top level UI
functions (DownloadManager::DownloadUrl{,ToFile}) to allocated the download id
and pass it down the stack.  That works fine, except that it's not clear how the
IO thread gets it in the normal case (I'm a bit uncomfortable with this CLs
answer to that question, which I'll address in other comments).

CreateDownloadItem() and StartDownload() strike me as download system internal
entry points, not places from which a download can be initiated.  So returning
the download id from them won't do a whole lot of good; it won't get back to
where it needs to get to.

Does that make my original comment make any more sense?

More detailed comments coming ....

[Randy Smith (Not in Mondays), 2011-07-10 23:49:54]

Apparently only one detailed comment.  Now that I understand it :-}, I like this
CL.  But we may have to do some rework for the cross-thread issue below.

http://codereview.chromium.org/7237034/diff/4001/chrome/browser/renderer_host...
File chrome/browser/renderer_host/download_resource_handler.cc (right):

http://codereview.chromium.org/7237034/diff/4001/chrome/browser/renderer_host...
chrome/browser/renderer_host/download_resource_handler.cc:124: download_id_ =
info->request_handle.GetDownloadManager()->download_prefs()->
I'm concerned about this line--it's setting a value on the UI thread that'll be
referenced on the IO thread, quite possibly in a racy way.  Can we come up with
a way of setting the download_id_ for the DownloadResourceHandler() that doesn't
require this kind of cross thread race concern?    Even if we can convince
ourselves that the assignment will always be strictly before the reading, it's
fragile code.

[benjhayden, 2011-07-11 15:57:50]

http://codereview.chromium.org/7237034/diff/4001/chrome/browser/renderer_host...
File chrome/browser/renderer_host/download_resource_handler.cc (right):

http://codereview.chromium.org/7237034/diff/4001/chrome/browser/renderer_host...
chrome/browser/renderer_host/download_resource_handler.cc:124: download_id_ =
info->request_handle.GetDownloadManager()->download_prefs()->
On 2011/07/10 23:49:55, rdsmith wrote:
> I'm concerned about this line--it's setting a value on the UI thread that'll
be
> referenced on the IO thread, quite possibly in a racy way.  Can we come up
with
> a way of setting the download_id_ for the DownloadResourceHandler() that
doesn't
> require this kind of cross thread race concern?    Even if we can convince
> ourselves that the assignment will always be strictly before the reading, it's
> fragile code.  

Ah, I see it now.
The primary reason that I moved GetNextId to the UI thread is because
GetDownloadManager() is on the UI thread.
I could move GetNextId back to the IO thread if I make the
ResourceDispatcherHost pass a DownloadPrefs into DownloadResourceHander along
with the DownloadFileManager, or if I attach a DownloadPrefs to the
DownloadFileManager. Which would you prefer, or is there a better alternative?

[Randy Smith (Not in Mondays), 2011-07-12 18:12:52]

http://codereview.chromium.org/7237034/diff/4001/chrome/browser/renderer_host...
File chrome/browser/renderer_host/download_resource_handler.cc (right):

http://codereview.chromium.org/7237034/diff/4001/chrome/browser/renderer_host...
chrome/browser/renderer_host/download_resource_handler.cc:124: download_id_ =
info->request_handle.GetDownloadManager()->download_prefs()->
On 2011/07/11 15:57:50, benjhayden wrote:
> On 2011/07/10 23:49:55, rdsmith wrote:
> > I'm concerned about this line--it's setting a value on the UI thread that'll
> be
> > referenced on the IO thread, quite possibly in a racy way.  Can we come up
> with
> > a way of setting the download_id_ for the DownloadResourceHandler() that
> doesn't
> > require this kind of cross thread race concern?    Even if we can convince
> > ourselves that the assignment will always be strictly before the reading,
it's
> > fragile code.  
> 
> Ah, I see it now.
> The primary reason that I moved GetNextId to the UI thread is because
> GetDownloadManager() is on the UI thread.
> I could move GetNextId back to the IO thread if I make the
> ResourceDispatcherHost pass a DownloadPrefs into DownloadResourceHander along
> with the DownloadFileManager, or if I attach a DownloadPrefs to the
> DownloadFileManager. Which would you prefer, or is there a better alternative?

I think DownloadPrefs is a UI thread object, and I'd be reluctant to change that
without a strong reason.  But digging into this issue has a bit of a tugging on
a string feeling (as is appropriate for threading issues :-J):
* If we require an atomic update of the prefs on each GetNextID() we must do it
on the UI thread (PrefService is definitely a UI thread object).
* If we don't require an atomic updates of the prefs (e.g. just update every n
seconds, or at browser shutdown) we can allow allocation of the ID on any thread
we want, or on all threads via the atomic increment idea.
* But in that case, we don't persist the id in the event of a browser crash,
which sounds bad (we'll have multiple downloads allocated with the same id).  
* But even if we do atomically update the prefs on each GetNextID(), are we
guaranteed that that update will make it out to the file system in the event of
a crash?  I'm guessing not.  Which means we need to deal with the desyncing of
the DB from the browser pref *anyway*.  

So I think what this means is that we need to figure out what guarantees the
pref service makes about persistence of preferences, and figure out what
implications that has for the system, then we can come back to the threading
issue.  Does that make sense?  

I don't know how to do the prefs persistence evaluation--I'd either trace the
code or use git blame to get some names of people to ask about it--so I think it
makes sense for you to start that process.  But please grab me if it gets bogged
down.  And definitely grab me if any of this doesn't sound right to you
(possibly we should sync up tomorrow anyway if you're in--this is getting
complicated).

[benjhayden, 2011-07-22 20:26:26]

PTAL.
Passing: browser_tests --gtest_filter='*Download*'

[benjhayden, 2011-07-22 23:04:57]

Hi, Will!
We'd appreciate your thoughts on the ProfileIOData part of this CL.
Thanks!

[willchan no longer on Chromium, 2011-07-23 09:41:07]

http://codereview.chromium.org/7237034/diff/26002/content/browser/renderer_ho...
File content/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/7237034/diff/26002/content/browser/renderer_ho...
content/browser/renderer_host/resource_dispatcher_host.cc:707: ProfileIOData*
profile_io_data = reinterpret_cast<ProfileIOData*>(
content/ is not allowed to depend on chrome/, so you can't use ProfileIOData
directly in content/. The content interface to stuff in ProfileIOData is
ResourceContext. But it's only allowed to expose an interface for content/. My
hunch is something like a GetNextIdThunkType is an implementation detail of
chrome/ and not something that jam@ would want exposed at the content/ level.
You should probably ask him. I don't really know how much of the download
subsystem is needed in the content/ level and how much should live in the
chrome/ level. Definitely all the UI components belong in the chrome/ level.
Anyhow, my suspicion is that the "correct" layering solution is to expose an
interface in ResourceContext that is a DownloadHandlerFactory or something that
produces ResourceHandlers. This DownloadHandlerFactory's implementation in
Chrome would be constructed with this GetNextIdThunkType (and probably also the
DownloadFileManager since I don't think that belongs in content/).

That's just my quick inkling of how this "should" work. You should check with
jam@.

[Randy Smith (Not in Mondays), 2011-07-24 13:46:55]

On 2011/07/23 09:41:07, willchan wrote:
http://codereview.chromium.org/7237034/diff/26002/content/browser/renderer_ho...
> content/browser/renderer_host/resource_dispatcher_host.cc:707: ProfileIOData*
> profile_io_data = reinterpret_cast<ProfileIOData*>(
> content/ is not allowed to depend on chrome/, so you can't use ProfileIOData
> directly in content/. The content interface to stuff in ProfileIOData is
> ResourceContext. But it's only allowed to expose an interface for content/. My
> hunch is something like a GetNextIdThunkType is an implementation detail of
> chrome/ and not something that jam@ would want exposed at the content/ level.
> You should probably ask him. I don't really know how much of the download
> subsystem is needed in the content/ level and how much should live in the
> chrome/ level. Definitely all the UI components belong in the chrome/ level.
> Anyhow, my suspicion is that the "correct" layering solution is to expose an
> interface in ResourceContext that is a DownloadHandlerFactory or something
that
> produces ResourceHandlers. This DownloadHandlerFactory's implementation in
> Chrome would be constructed with this GetNextIdThunkType (and probably also
the
> DownloadFileManager since I don't think that belongs in content/).
> 
> That's just my quick inkling of how this "should" work. You should check with
> jam@.

[Added jam to cc list.  ]

John and I have been talking about the chrome/content split for downloads, and
most of it's actually going to end up in content (not the UI, and there are a
couple of other things that'll be pushed into chrome, but most stays in
content).  So my guess is that we're in good shape putting something into
ResourceContext.  With regard to IDs, I'd like to both allow ids to be allocated
on the IO downloads thread path, and to use specified IDs  (for situations where
something on the UI thread needs to sync up with the creation of a particular
download).  

John: This issue does bring us right back to working out whether downloads
history will be (chrome vs. content) and if in chrome, what the interface will
look like.  We've historically had a "active download id" and a "history db
handle" as two separate ids, and the laters been used as the persistent handle,
while the former is unique just within a browser session.  We're unifying those
two ids to make extensions programming easier, which is requiring us to
initialize the allocated ID from the history database.  So we'll need to include
that initialization in the content downloads<->history interface.

[Randy Smith (Not in Mondays), 2011-07-25 20:20:11]

First round ...

http://codereview.chromium.org/7237034/diff/26002/chrome/browser/download/dow...
File chrome/browser/download/download_file_manager.cc (right):

http://codereview.chromium.org/7237034/diff/26002/chrome/browser/download/dow...
chrome/browser/download/download_file_manager.cc:89:
&DownloadManager::StartDownload, id.id()));
nit: I'd feel somewhat better if we distinguished in variable names between
global and per-profile ids, at least for the cases where it's not really obvious
which one we're using within a function.  Either global_id, local_id, or
global_id, id, or id, local_id would all be fine.  But id.id() is one of those
confusing idioms I think it's worth avoiding.

(Throughout CL.)

http://codereview.chromium.org/7237034/diff/26002/chrome/browser/download/dow...
chrome/browser/download/download_file_manager.cc:217: VLOG(20) << __FUNCTION__
<< "()" << " id = " << id.id();
nit: Reducing the download id down to a local id for logging loses information. 
  I'd replace with both local id and download manager pointer.  (Thoughout file)

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

http://codereview.chromium.org/7237034/diff/26002/chrome/browser/download/dow...
chrome/browser/download/download_history.cc:39: HistoryService* hs =
profile_->GetHistoryService(Profile::EXPLICIT_ACCESS);
So just a note: It turns out that this means that regular and incognito profiles
will share an id space.  I don't think this is a problem, but I watned to make
sure you were aware of it too.

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

http://codereview.chromium.org/7237034/diff/26002/chrome/browser/download/dow...
chrome/browser/download/download_item.h:491: class DownloadId {
A download id seems like a "lower level" concept than a download item; for
instance, most of the places that need to know about DownloadIds (because
they're off the UI thread) don't need to know about DownloadItems (because
they're off the UI thread).  I think I'd suggest putting this in its own file.

http://codereview.chromium.org/7237034/diff/26002/chrome/browser/download/dow...
chrome/browser/download/download_item.h:503: bool operator==(const DownloadId&
that) const {
Chrome biases against creating full types (i.e. supporting ==, <, copy and
assign) so if you can put a comment in for why you need each of these (support
for STL containers mostly, I presume) that would be good.

http://codereview.chromium.org/7237034/diff/26002/chrome/browser/download/dow...
chrome/browser/download/download_item.h:530: namespace __gnu_cxx {
This looks like black magic, and it's specifically scary to me since gcc isn't
the only compiler we're using.  Comment what's happening here?

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

http://codereview.chromium.org/7237034/diff/26002/chrome/browser/download/dow...
chrome/browser/download/download_manager.cc:235: DCHECK(next_id_ >= 0) << "Uh
oh, OnHistoryGetNextId hasn't happened yet!";
I see nothing architectural preventing this DCHECK() from tripping, and I think
there has to be (i.e. we have to have some guarantee that downloads won't be
initiated for this DM until the id is set).  I think this means we have to delay
completion of profile initialization until this value has been set.  

One way to do that would be to:
* Hold off profile initialization until this round trip (see the ProfileImpl
constructor and OnPrefsLoaded() in that file for the current async profile
initialization; you'd have to hook into that).
* Construct the download manager on profile construction (possibly just passing
in the id?)
* Call init on first use.

But there may be other ways to do it--that's just one possibility.  Still, this
is a key problem.

http://codereview.chromium.org/7237034/diff/26002/chrome/browser/download/dow...
chrome/browser/download/download_manager.cc:783: << download->DebugString(true);
Why change the logging?  I don't particularly care, but I do believe that in
most cases logging should be kept minimal in the main tree and cranked up in
developers checkouts unless there's some particular reason to believe the
specific piece of logging added will be useful in the general case.

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

http://codereview.chromium.org/7237034/diff/26002/chrome/browser/download/dow...
chrome/browser/download/download_manager.h:122: // your new DownloadItem is
stored in the history.
nit: It's not really the incremented counter that's being persisted.  I'd be
tempted to phrase this as "The incremented counter is not persisted, but the
base counter for this accessor is initialized from the largest id actually saved
to the download history database."  But your call.

http://codereview.chromium.org/7237034/diff/26002/chrome/browser/download/dow...
chrome/browser/download/download_manager.h:127: // = next_id_thunk.Run();" Pass
the thunk around by const ref and store it by
nit: I may be targeting a less-smart class of developers than we actually have,
but could you make this a bit clearer?  Specifically, I'd suggest saying:

---
Use it like:
    const GetNextIdThunkType& next_id_thunk =
download_manager->GetNextIdThunk();
    // next_id_thunk may be safely copied, including between threads.
    id = next_id_thunk.Run();
----

http://codereview.chromium.org/7237034/diff/26002/chrome/browser/download/dow...
chrome/browser/download/download_manager.h:127: // = next_id_thunk.Run();" Pass
the thunk around by const ref and store it by
I'm a bit worried by this.  My understanding was that when passed between
threads, refs were silently copied (to handle lifetime management
issues--whatever created the object the ref was to might destroy it on the
original thread without warning).  But that obviously won't work if you're
holding on to a reference to a base class, which is what this sounds like. 
Enlighten me as to why this is ok?

http://codereview.chromium.org/7237034/diff/26002/chrome/browser/download/dow...
chrome/browser/download/download_manager.h:132: GetNextIdThunkType
GetNextIdThunk();
If you want to pass it around by const ref, is there a reason it doesn't return
a const ref?

http://codereview.chromium.org/7237034/diff/26002/chrome/browser/history/down...
File chrome/browser/history/download_database.cc (right):

http://codereview.chromium.org/7237034/diff/26002/chrome/browser/history/down...
chrome/browser/history/download_database.cc:173:
meta_table_.SetValue(kNextDownloadId, ++next_id_);
Is there a reason why it's safe not to implement the TODO?  The current behavior
looks like an obvious, dangerous race which the TODO is needed to fix.

http://codereview.chromium.org/7237034/diff/26002/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/download_resource_handler.h (right):

http://codereview.chromium.org/7237034/diff/26002/chrome/browser/renderer_hos...
chrome/browser/renderer_host/download_resource_handler.h:35: const
DownloadManager::GetNextIdThunkType& get_next_id,
My preference would be for DRH to actually take an id as an argument, rather
than the ability to get an id.  That sets us up well for the situations where
the id is either already allocated (resumption) or allocated on the UI thread (a
couple of the programmatic interfaces to downloading).

http://codereview.chromium.org/7237034/diff/26002/content/browser/renderer_ho...
File content/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/7237034/diff/26002/content/browser/renderer_ho...
content/browser/renderer_host/resource_dispatcher_host.cc:707: ProfileIOData*
profile_io_data = reinterpret_cast<ProfileIOData*>(
On 2011/07/23 09:41:07, willchan wrote:
> content/ is not allowed to depend on chrome/, so you can't use ProfileIOData
> directly in content/. The content interface to stuff in ProfileIOData is
> ResourceContext. But it's only allowed to expose an interface for content/. My
> hunch is something like a GetNextIdThunkType is an implementation detail of
> chrome/ and not something that jam@ would want exposed at the content/ level.
> You should probably ask him. I don't really know how much of the download
> subsystem is needed in the content/ level and how much should live in the
> chrome/ level. Definitely all the UI components belong in the chrome/ level.
> Anyhow, my suspicion is that the "correct" layering solution is to expose an
> interface in ResourceContext that is a DownloadHandlerFactory or something
that
> produces ResourceHandlers. This DownloadHandlerFactory's implementation in
> Chrome would be constructed with this GetNextIdThunkType (and probably also
the
> DownloadFileManager since I don't think that belongs in content/).
> 
> That's just my quick inkling of how this "should" work. You should check with
> jam@.

In the absence of commentary from jam, I'll go with my understanding of Will's
description and jam's plans, and say this should be in ResourceContext.

[benjhayden, 2011-07-27 19:40:54]

http://codereview.chromium.org/7237034/diff/26002/chrome/browser/download/dow...
File chrome/browser/download/download_file_manager.cc (right):

http://codereview.chromium.org/7237034/diff/26002/chrome/browser/download/dow...
chrome/browser/download/download_file_manager.cc:89:
&DownloadManager::StartDownload, id.id()));
On 2011/07/25 20:20:12, rdsmith wrote:
> nit: I'd feel somewhat better if we distinguished in variable names between
> global and per-profile ids, at least for the cases where it's not really
obvious
> which one we're using within a function.  Either global_id, local_id, or
> global_id, id, or id, local_id would all be fine.  But id.id() is one of those
> confusing idioms I think it's worth avoiding.
> 
> (Throughout CL.)

PTAL

http://codereview.chromium.org/7237034/diff/26002/chrome/browser/download/dow...
chrome/browser/download/download_file_manager.cc:217: VLOG(20) << __FUNCTION__
<< "()" << " id = " << id.id();
On 2011/07/25 20:20:12, rdsmith wrote:
> nit: Reducing the download id down to a local id for logging loses
information. 
>   I'd replace with both local id and download manager pointer.  (Thoughout
file)

PTAL.

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

http://codereview.chromium.org/7237034/diff/26002/chrome/browser/download/dow...
chrome/browser/download/download_history.cc:39: HistoryService* hs =
profile_->GetHistoryService(Profile::EXPLICIT_ACCESS);
On 2011/07/25 20:20:12, rdsmith wrote:
> So just a note: It turns out that this means that regular and incognito
profiles
> will share an id space.  I don't think this is a problem, but I watned to make
> sure you were aware of it too.

Yes, OTR downloads are not persisted, and the DownloadIds will be distinct
because the DM*s are distinct.
However, OTR chrome://downloads shows both the otr downloads and the on-record
downloads, so it seems that chrome.downloads.search() should return both kinds
of downloads and that the ids should not collide.
I think I'm going to forward otr GetNextId() calls to the on-record DM so that
extensions in otr windows don't have id collisions.
As you say, allowing the ids to collide until the extensions api is in place
shouldn't break anything.

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

http://codereview.chromium.org/7237034/diff/26002/chrome/browser/download/dow...
chrome/browser/download/download_item.h:491: class DownloadId {
On 2011/07/25 20:20:12, rdsmith wrote:
> A download id seems like a "lower level" concept than a download item; for
> instance, most of the places that need to know about DownloadIds (because
> they're off the UI thread) don't need to know about DownloadItems (because
> they're off the UI thread).  I think I'd suggest putting this in its own file.
 

Done.

http://codereview.chromium.org/7237034/diff/26002/chrome/browser/download/dow...
chrome/browser/download/download_item.h:503: bool operator==(const DownloadId&
that) const {
On 2011/07/25 20:20:12, rdsmith wrote:
> Chrome biases against creating full types (i.e. supporting ==, <, copy and
> assign) so if you can put a comment in for why you need each of these (support
> for STL containers mostly, I presume) that would be good.

Done.

http://codereview.chromium.org/7237034/diff/26002/chrome/browser/download/dow...
chrome/browser/download/download_item.h:530: namespace __gnu_cxx {
On 2011/07/25 20:20:12, rdsmith wrote:
> This looks like black magic, and it's specifically scary to me since gcc isn't
> the only compiler we're using.  Comment what's happening here?

Switched to BASE_HASH_NAMESPACE to afford the other compilers and commented, and
made a note to test this CL on the other platforms.

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

http://codereview.chromium.org/7237034/diff/26002/chrome/browser/download/dow...
chrome/browser/download/download_manager.cc:235: DCHECK(next_id_ >= 0) << "Uh
oh, OnHistoryGetNextId hasn't happened yet!";
On 2011/07/25 20:20:12, rdsmith wrote:
> I see nothing architectural preventing this DCHECK() from tripping, and I
think
> there has to be (i.e. we have to have some guarantee that downloads won't be
> initiated for this DM until the id is set).  I think this means we have to
delay
> completion of profile initialization until this value has been set.  
> 
> One way to do that would be to:
> * Hold off profile initialization until this round trip (see the ProfileImpl
> constructor and OnPrefsLoaded() in that file for the current async profile
> initialization; you'd have to hook into that).

I think we decided to do this in the next CL.


> * Construct the download manager on profile construction (possibly just
passing
> in the id?)
> * Call init on first use.
> 
> But there may be other ways to do it--that's just one possibility.  Still,
this
> is a key problem.

http://codereview.chromium.org/7237034/diff/26002/chrome/browser/download/dow...
chrome/browser/download/download_manager.cc:783: << download->DebugString(true);
On 2011/07/25 20:20:12, rdsmith wrote:
> Why change the logging?  I don't particularly care, but I do believe that in
> most cases logging should be kept minimal in the main tree and cranked up in
> developers checkouts unless there's some particular reason to believe the
> specific piece of logging added will be useful in the general case.

Done.

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

http://codereview.chromium.org/7237034/diff/26002/chrome/browser/download/dow...
chrome/browser/download/download_manager.h:122: // your new DownloadItem is
stored in the history.
On 2011/07/25 20:20:12, rdsmith wrote:
> nit: It's not really the incremented counter that's being persisted.  I'd be
> tempted to phrase this as "The incremented counter is not persisted, but the
> base counter for this accessor is initialized from the largest id actually
saved
> to the download history database."  But your call.

Done.

http://codereview.chromium.org/7237034/diff/26002/chrome/browser/download/dow...
chrome/browser/download/download_manager.h:127: // = next_id_thunk.Run();" Pass
the thunk around by const ref and store it by
On 2011/07/25 20:20:12, rdsmith wrote:
> nit: I may be targeting a less-smart class of developers than we actually
have,
> but could you make this a bit clearer?  Specifically, I'd suggest saying:
> 
> ---
> Use it like:
>     const GetNextIdThunkType& next_id_thunk =
> download_manager->GetNextIdThunk();
>     // next_id_thunk may be safely copied, including between threads.
>     id = next_id_thunk.Run();
> ----

Done.

http://codereview.chromium.org/7237034/diff/26002/chrome/browser/download/dow...
chrome/browser/download/download_manager.h:127: // = next_id_thunk.Run();" Pass
the thunk around by const ref and store it by
On 2011/07/25 20:20:12, rdsmith wrote:
> I'm a bit worried by this.  My understanding was that when passed between
> threads, refs were silently copied (to handle lifetime management
> issues--whatever created the ref'd object might destroy it on the
> original thread without warning).  But that obviously won't work if you're
> holding on to a reference to a base class, which is what this sounds like. 
> Enlighten me as to why this is ok?

I'm not sure exactly what you're asking.
If you're asking about destroying DownloadManager, it's ref-counted, so you
don't destroy DM, you just decref it and trust that it will be destroyed when
everybody has decref'd it.
If you're asking about destroying the thunk, that's why it should be stored by
copy instead of by ref. A copy of this thunk is stored in ProfileIOData, which
is created on the UI thread and used on the IO thread. Another copy is stored in
DownloadResourceHandler.
Were you asking about something else?

http://codereview.chromium.org/7237034/diff/26002/chrome/browser/download/dow...
chrome/browser/download/download_manager.h:132: GetNextIdThunkType
GetNextIdThunk();
On 2011/07/25 20:20:12, rdsmith wrote:
> If you want to pass it around by const ref, is there a reason it doesn't
return
> a const ref?  

Error: returning reference to temporary.

http://codereview.chromium.org/7237034/diff/26002/chrome/browser/history/down...
File chrome/browser/history/download_database.cc (right):

http://codereview.chromium.org/7237034/diff/26002/chrome/browser/history/down...
chrome/browser/history/download_database.cc:173:
meta_table_.SetValue(kNextDownloadId, ++next_id_);
On 2011/07/25 20:20:12, rdsmith wrote:
> Is there a reason why it's safe not to implement the TODO?  The current
behavior
> looks like an obvious, dangerous race which the TODO is needed to fix.

This next_id_ is only communicated to DownloadManager once, and is not used to
allocate ids.
The todo cannot be implemented until id is added to DownloadHistoryInfo.

http://codereview.chromium.org/7237034/diff/26002/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/download_resource_handler.h (right):

http://codereview.chromium.org/7237034/diff/26002/chrome/browser/renderer_hos...
chrome/browser/renderer_host/download_resource_handler.h:35: const
DownloadManager::GetNextIdThunkType& get_next_id,
On 2011/07/25 20:20:12, rdsmith wrote:
> My preference would be for DRH to actually take an id as an argument, rather
> than the ability to get an id.  That sets us up well for the situations where
> the id is either already allocated (resumption) or allocated on the UI thread
(a
> couple of the programmatic interfaces to downloading).

Done.

http://codereview.chromium.org/7237034/diff/26002/content/browser/renderer_ho...
File content/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/7237034/diff/26002/content/browser/renderer_ho...
content/browser/renderer_host/resource_dispatcher_host.cc:707: ProfileIOData*
profile_io_data = reinterpret_cast<ProfileIOData*>(
On 2011/07/23 09:41:07, willchan wrote:
> content/ is not allowed to depend on chrome/, so you can't use ProfileIOData
> directly in content/. The content interface to stuff in ProfileIOData is
> ResourceContext. But it's only allowed to expose an interface for content/. My
> hunch is something like a GetNextIdThunkType is an implementation detail of
> chrome/ and not something that jam@ would want exposed at the content/ level.
> You should probably ask him. I don't really know how much of the download
> subsystem is needed in the content/ level and how much should live in the
> chrome/ level. Definitely all the UI components belong in the chrome/ level.
> Anyhow, my suspicion is that the "correct" layering solution is to expose an
> interface in ResourceContext that is a DownloadHandlerFactory or something
that
> produces ResourceHandlers. This DownloadHandlerFactory's implementation in
> Chrome would be constructed with this GetNextIdThunkType (and probably also
the
> DownloadFileManager since I don't think that belongs in content/).
> 
> That's just my quick inkling of how this "should" work. You should check with
> jam@.

DownloadManager is moving to content/. Could a GetNextIdThunkType live in
ProfileIOData once that is done?

http://codereview.chromium.org/7237034/diff/26002/content/browser/renderer_ho...
content/browser/renderer_host/resource_dispatcher_host.cc:707: ProfileIOData*
profile_io_data = reinterpret_cast<ProfileIOData*>(
On 2011/07/25 20:20:12, rdsmith wrote:
> On 2011/07/23 09:41:07, willchan wrote:
> > content/ is not allowed to depend on chrome/, so you can't use ProfileIOData
> > directly in content/. The content interface to stuff in ProfileIOData is
> > ResourceContext. But it's only allowed to expose an interface for content/.
My
> > hunch is something like a GetNextIdThunkType is an implementation detail of
> > chrome/ and not something that jam@ would want exposed at the content/
level.
> > You should probably ask him. I don't really know how much of the download
> > subsystem is needed in the content/ level and how much should live in the
> > chrome/ level. Definitely all the UI components belong in the chrome/ level.
> > Anyhow, my suspicion is that the "correct" layering solution is to expose an
> > interface in ResourceContext that is a DownloadHandlerFactory or something
> that
> > produces ResourceHandlers. This DownloadHandlerFactory's implementation in
> > Chrome would be constructed with this GetNextIdThunkType (and probably also
> the
> > DownloadFileManager since I don't think that belongs in content/).
> > 
> > That's just my quick inkling of how this "should" work. You should check
with
> > jam@.
> 
> In the absence of commentary from jam, I'll go with my understanding of Will's
> description and jam's plans, and say this should be in ResourceContext.

Even though DownloadManager is moving to content/?

[willchan no longer on Chromium, 2011-07-27 19:47:14]

http://codereview.chromium.org/7237034/diff/26002/content/browser/renderer_ho...
File content/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/7237034/diff/26002/content/browser/renderer_ho...
content/browser/renderer_host/resource_dispatcher_host.cc:707: ProfileIOData*
profile_io_data = reinterpret_cast<ProfileIOData*>(
On 2011/07/27 19:40:54, b s h wrote:
> On 2011/07/23 09:41:07, willchan wrote:
> > content/ is not allowed to depend on chrome/, so you can't use ProfileIOData
> > directly in content/. The content interface to stuff in ProfileIOData is
> > ResourceContext. But it's only allowed to expose an interface for content/.
My
> > hunch is something like a GetNextIdThunkType is an implementation detail of
> > chrome/ and not something that jam@ would want exposed at the content/
level.
> > You should probably ask him. I don't really know how much of the download
> > subsystem is needed in the content/ level and how much should live in the
> > chrome/ level. Definitely all the UI components belong in the chrome/ level.
> > Anyhow, my suspicion is that the "correct" layering solution is to expose an
> > interface in ResourceContext that is a DownloadHandlerFactory or something
> that
> > produces ResourceHandlers. This DownloadHandlerFactory's implementation in
> > Chrome would be constructed with this GetNextIdThunkType (and probably also
> the
> > DownloadFileManager since I don't think that belongs in content/).
> > 
> > That's just my quick inkling of how this "should" work. You should check
with
> > jam@.
> 
> DownloadManager is moving to content/. Could a GetNextIdThunkType live in
> ProfileIOData once that is done?

I don't see how DownloadManager moving to content/ is relevant here. content/ is
not allowed to depend on anything in chrome/. In particular, it's not allowed to
depend on Profile&ProfileIOData, as those will always be in chrome/. It doesn't
matter if DownloadManager is moving to content/, Profile&ProfileIOData aren't
moving to content/, so you can't depend on ProfileIOData from content/.

Can't depend on ProfileIOData in content/ :)

[benjhayden, 2011-07-28 15:37:52]

http://codereview.chromium.org/7237034/diff/26002/content/browser/renderer_ho...
File content/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/7237034/diff/26002/content/browser/renderer_ho...
content/browser/renderer_host/resource_dispatcher_host.cc:707: ProfileIOData*
profile_io_data = reinterpret_cast<ProfileIOData*>(
On 2011/07/27 19:47:14, willchan wrote:
> On 2011/07/27 19:40:54, b s h wrote:
> > On 2011/07/23 09:41:07, willchan wrote:
> > > content/ is not allowed to depend on chrome/, so you can't use
ProfileIOData
> > > directly in content/. The content interface to stuff in ProfileIOData is
> > > ResourceContext. But it's only allowed to expose an interface for
content/.
> My
> > > hunch is something like a GetNextIdThunkType is an implementation detail
of
> > > chrome/ and not something that jam@ would want exposed at the content/
> level.
> > > You should probably ask him. I don't really know how much of the download
> > > subsystem is needed in the content/ level and how much should live in the
> > > chrome/ level. Definitely all the UI components belong in the chrome/
level.
> > > Anyhow, my suspicion is that the "correct" layering solution is to expose
an
> > > interface in ResourceContext that is a DownloadHandlerFactory or something
> > that
> > > produces ResourceHandlers. This DownloadHandlerFactory's implementation in
> > > Chrome would be constructed with this GetNextIdThunkType (and probably
also
> > the
> > > DownloadFileManager since I don't think that belongs in content/).
> > > 
> > > That's just my quick inkling of how this "should" work. You should check
> with
> > > jam@.
> > 
> > DownloadManager is moving to content/. Could a GetNextIdThunkType live in
> > ProfileIOData once that is done?
> 
> I don't see how DownloadManager moving to content/ is relevant here. content/
is
> not allowed to depend on anything in chrome/. In particular, it's not allowed
to
> depend on Profile&ProfileIOData, as those will always be in chrome/. It
doesn't
> matter if DownloadManager is moving to content/, Profile&ProfileIOData aren't
> moving to content/, so you can't depend on ProfileIOData from content/.
> 
> Can't depend on ProfileIOData in content/ :)

Done.

[Randy Smith (Not in Mondays), 2011-07-28 21:03:16]

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

http://codereview.chromium.org/7237034/diff/26002/chrome/browser/download/dow...
chrome/browser/download/download_history.cc:39: HistoryService* hs =
profile_->GetHistoryService(Profile::EXPLICIT_ACCESS);
On 2011/07/27 19:40:54, b s h wrote:
> On 2011/07/25 20:20:12, rdsmith wrote:
> > So just a note: It turns out that this means that regular and incognito
> profiles
> > will share an id space.  I don't think this is a problem, but I watned to
make
> > sure you were aware of it too.
> 
> Yes, OTR downloads are not persisted, and the DownloadIds will be distinct
> because the DM*s are distinct.
> However, OTR chrome://downloads shows both the otr downloads and the on-record
> downloads, so it seems that chrome.downloads.search() should return both kinds
> of downloads and that the ids should not collide.
> I think I'm going to forward otr GetNextId() calls to the on-record DM so that
> extensions in otr windows don't have id collisions.
> As you say, allowing the ids to collide until the extensions api is in place
> shouldn't break anything. 

Sounds good.  Answers my question on your other CL, too :-}.

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

http://codereview.chromium.org/7237034/diff/26002/chrome/browser/download/dow...
chrome/browser/download/download_manager.h:127: // = next_id_thunk.Run();" Pass
the thunk around by const ref and store it by
On 2011/07/27 19:40:54, b s h wrote:
> On 2011/07/25 20:20:12, rdsmith wrote:
> > I'm a bit worried by this.  My understanding was that when passed between
> > threads, refs were silently copied (to handle lifetime management
> > issues--whatever created the ref'd object might destroy it on the
> > original thread without warning).  But that obviously won't work if you're
> > holding on to a reference to a base class, which is what this sounds like. 
> > Enlighten me as to why this is ok?
> 
> I'm not sure exactly what you're asking.
> If you're asking about destroying DownloadManager, it's ref-counted, so you
> don't destroy DM, you just decref it and trust that it will be destroyed when
> everybody has decref'd it.
> If you're asking about destroying the thunk, that's why it should be stored by
> copy instead of by ref. A copy of this thunk is stored in ProfileIOData, which
> is created on the UI thread and used on the IO thread. Another copy is stored
in
> DownloadResourceHandler.
> Were you asking about something else?

I think I wasn't understanding how base::Callback and base::Bind were
implemented.  I was worried that when you created an object of
base::Calback<DownloadId(void)> you were actually creating an object of a
(hidden) superclass of that type.  If that was the case, passing the object
around by copy would "sheer off" everything except for the base class contents. 
I no longer think that, so I'm no longer worried about this issue :-}.

http://codereview.chromium.org/7237034/diff/26002/chrome/browser/history/down...
File chrome/browser/history/download_database.cc (right):

http://codereview.chromium.org/7237034/diff/26002/chrome/browser/history/down...
chrome/browser/history/download_database.cc:173:
meta_table_.SetValue(kNextDownloadId, ++next_id_);
On 2011/07/27 19:40:54, b s h wrote:
> On 2011/07/25 20:20:12, rdsmith wrote:
> > Is there a reason why it's safe not to implement the TODO?  The current
> behavior
> > looks like an obvious, dangerous race which the TODO is needed to fix.
> 
> This next_id_ is only communicated to DownloadManager once, and is not used to
> allocate ids.
> The todo cannot be implemented until id is added to DownloadHistoryInfo.

Sorry, still having problems distinguishing non-persistent and persistent ids in
my head.  I think this is ok, since we're not actually storing anything in the
next_id_ namespace on a per-entry basis in the database.

http://codereview.chromium.org/7237034/diff/34029/chrome/browser/download/dow...
File chrome/browser/download/download_file_manager.cc (right):

http://codereview.chromium.org/7237034/diff/34029/chrome/browser/download/dow...
chrome/browser/download/download_file_manager.cc:265:
downloads_.erase(DownloadId((*i)->GetDownloadManager(), (*i)->id()));
Shouldn't DownloadFile have a global id?  It feels a bit inconsistent that it
doesn't.  No need to do it for this CL, but eventually we should update that
(and possibly merge GlobalId and DownloadRequestHandle?  They've got overlapping
functionality now.)

http://codereview.chromium.org/7237034/diff/34029/chrome/browser/download/dow...
File chrome/browser/download/download_history.h (right):

http://codereview.chromium.org/7237034/diff/34029/chrome/browser/download/dow...
chrome/browser/download/download_history.h:35: // Should be much faster than
Load; downloads cannot happen until after this.
nit: This comment is inaccurate for this CL, though should be accurate in a
future CL.  Maybe "so that we may delay downloads after this call with minimal
performance penalty"?

http://codereview.chromium.org/7237034/diff/34029/chrome/browser/download/dow...
File chrome/browser/download/download_id.h (right):

http://codereview.chromium.org/7237034/diff/34029/chrome/browser/download/dow...
chrome/browser/download/download_id.h:17: // sessions, but their local() field
is.
Huh.  This brings up a really interesting question I'm not sure we've fully
addressed: Do extensions see the global id or a local id?  This is tightly
related to: can extensions span profiles?  My guess is that they don't with the
possible exception of a profile and its corresponding incognito profile, but
this should probably be checked with an extension expert.

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

http://codereview.chromium.org/7237034/diff/34029/chrome/browser/download/dow...
chrome/browser/download/download_item.h:282: DownloadId gid() const;
I'd be inclined to call this global_id(); that's what's being used in
download_file_manager.h, and chrome tends to avoid abbreviations.

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

http://codereview.chromium.org/7237034/diff/34029/chrome/browser/download/dow...
chrome/browser/download/download_manager.cc:230: base::AutoLock
_(next_id_lock_);
I puzzled over this for a while before I got it.  It's a fine idiom, but it's
not one I've seen before in Chrome, and if you're not used to it I think it'll
confused people.  I'd use "autolock" instead of "_".

http://codereview.chromium.org/7237034/diff/34029/chrome/browser/download/dow...
chrome/browser/download/download_manager.cc:232: // = next_id.
A bit more of a comment as to why this actually makes sense would make sense, I
think (i.e. explain that we just need to make sure assigned ids are unique per
session for right now, and this does it in the face of the download/getting
first id race).

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

http://codereview.chromium.org/7237034/diff/34029/chrome/browser/download/dow...
chrome/browser/download/download_manager.h:66: }  // namespace base
I'm having a "I don't think this can work" reaction here.  An incomplete type
delcaration is ok for pointers or references, but if you declare an actual
variable, you need a complete type declaration.  And next_id_lock_ is declared
inline, below.  So I presume that this is compiling only because some other
include file is including base/lock.h, and that this file should also include
base/lock.h.

http://codereview.chromium.org/7237034/diff/34029/chrome/browser/download/dow...
chrome/browser/download/download_manager.h:377: void OnHistoryGetNextId(int
next_id);
For the next CL: Consider making the next_id field an argument to the
DownloadManager constructor.  That'll fit well with the "pulling history out"
stuff that jam's aiming to do, and should work well with the delaying of the
profile initialization until we have that information (because we won't need the
download manager until then anyway).

http://codereview.chromium.org/7237034/diff/34029/chrome/browser/history/hist...
File chrome/browser/history/history_backend.cc (right):

http://codereview.chromium.org/7237034/diff/34029/chrome/browser/history/hist...
chrome/browser/history/history_backend.cc:1085:
request->ForwardResult(DownloadNextIdRequest::TupleType(request->value));
This seems like it'll leave an uninitialized value in the case where db_.get()
is false.  Shouldn't we put some kind of value in?  (You should probably try and
figure out in which cases that can happen so we know what the right way to
handle it is.)

http://codereview.chromium.org/7237034/diff/34029/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_io_data.cc (right):

http://codereview.chromium.org/7237034/diff/34029/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_io_data.cc:194: next_download_id_thunk_ =
profile->GetDownloadManager()->GetNextIdThunk();
Why is this in ProfileIOData as well as ResourceContext?  Is it used from
ProfileIOData, and if so, can a resource_context_ indirection be substituted?

http://codereview.chromium.org/7237034/diff/34029/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_io_data.cc:510: &next_download_id_thunk_);
I presume that the whole nasty reinterpret_cast<> business is because we can't
include download_manager.h in the ResourceContext declaration because that's in
content?  If so, put a TODO here to pull it back out once download_manager.h is
moved into content.

(What I'd *like* to say is that you should just include download_manager.h in
the ResourceContext header file, and jam can remove it when he does the move,
but that probably requires pulling him into this review and modifying the
exceptions file, and it probably just makes more sense to clean up once he's
done.)

http://codereview.chromium.org/7237034/diff/34029/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/download_throttling_resource_handler.cc
(right):

http://codereview.chromium.org/7237034/diff/34029/chrome/browser/renderer_hos...
chrome/browser/renderer_host/download_throttling_resource_handler.cc:185: if
(next_download_id_thunk != NULL) {
Under what circumstances can next_download_id_thunk be null?

If next_download_id_thunk's null, we'll DCHECK in the download resource handler
constructor, and I think DCHECKs are more useful the closer they are to the
original source of the error, so I'd argue for one here as well.

[benjhayden, 2011-08-03 17:44:46]

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

http://codereview.chromium.org/7237034/diff/26002/chrome/browser/download/dow...
chrome/browser/download/download_history.cc:39: HistoryService* hs =
profile_->GetHistoryService(Profile::EXPLICIT_ACCESS);
On 2011/07/28 21:03:16, rdsmith wrote:
> On 2011/07/27 19:40:54, b s h wrote:
> > On 2011/07/25 20:20:12, rdsmith wrote:
> > > So just a note: It turns out that this means that regular and incognito
> > profiles
> > > will share an id space.  I don't think this is a problem, but I watned to
> make
> > > sure you were aware of it too.
> > 
> > Yes, OTR downloads are not persisted, and the DownloadIds will be distinct
> > because the DM*s are distinct.
> > However, OTR chrome://downloads shows both the otr downloads and the
on-record
> > downloads, so it seems that chrome.downloads.search() should return both
kinds
> > of downloads and that the ids should not collide.
> > I think I'm going to forward otr GetNextId() calls to the on-record DM so
that
> > extensions in otr windows don't have id collisions.
> > As you say, allowing the ids to collide until the extensions api is in place
> > shouldn't break anything. 
> 
> Sounds good.  Answers my question on your other CL, too :-}.

Done.

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

http://codereview.chromium.org/7237034/diff/26002/chrome/browser/download/dow...
chrome/browser/download/download_manager.h:127: // = next_id_thunk.Run();" Pass
the thunk around by const ref and store it by
On 2011/07/28 21:03:16, rdsmith wrote:
> On 2011/07/27 19:40:54, b s h wrote:
> > On 2011/07/25 20:20:12, rdsmith wrote:
> > > I'm a bit worried by this.  My understanding was that when passed between
> > > threads, refs were silently copied (to handle lifetime management
> > > issues--whatever created the ref'd object might destroy it on the
> > > original thread without warning).  But that obviously won't work if you're
> > > holding on to a reference to a base class, which is what this sounds like.

> > > Enlighten me as to why this is ok?
> > 
> > I'm not sure exactly what you're asking.
> > If you're asking about destroying DownloadManager, it's ref-counted, so you
> > don't destroy DM, you just decref it and trust that it will be destroyed
when
> > everybody has decref'd it.
> > If you're asking about destroying the thunk, that's why it should be stored
by
> > copy instead of by ref. A copy of this thunk is stored in ProfileIOData,
which
> > is created on the UI thread and used on the IO thread. Another copy is
stored
> in
> > DownloadResourceHandler.
> > Were you asking about something else?
> 
> I think I wasn't understanding how base::Callback and base::Bind were
> implemented.  I was worried that when you created an object of
> base::Calback<DownloadId(void)> you were actually creating an object of a
> (hidden) superclass of that type.  If that was the case, passing the object
> around by copy would "sheer off" everything except for the base class
contents. 
> I no longer think that, so I'm no longer worried about this issue :-}.

Done.

http://codereview.chromium.org/7237034/diff/26002/chrome/browser/history/down...
File chrome/browser/history/download_database.cc (right):

http://codereview.chromium.org/7237034/diff/26002/chrome/browser/history/down...
chrome/browser/history/download_database.cc:173:
meta_table_.SetValue(kNextDownloadId, ++next_id_);
On 2011/07/28 21:03:16, rdsmith wrote:
> On 2011/07/27 19:40:54, b s h wrote:
> > On 2011/07/25 20:20:12, rdsmith wrote:
> > > Is there a reason why it's safe not to implement the TODO?  The current
> > behavior
> > > looks like an obvious, dangerous race which the TODO is needed to fix.
> > 
> > This next_id_ is only communicated to DownloadManager once, and is not used
to
> > allocate ids.
> > The todo cannot be implemented until id is added to DownloadHistoryInfo.
> 
> Sorry, still having problems distinguishing non-persistent and persistent ids
in
> my head.  I think this is ok, since we're not actually storing anything in the
> next_id_ namespace on a per-entry basis in the database.

Done.

http://codereview.chromium.org/7237034/diff/34029/chrome/browser/download/dow...
File chrome/browser/download/download_file_manager.cc (right):

http://codereview.chromium.org/7237034/diff/34029/chrome/browser/download/dow...
chrome/browser/download/download_file_manager.cc:265:
downloads_.erase(DownloadId((*i)->GetDownloadManager(), (*i)->id()));
On 2011/07/28 21:03:16, rdsmith wrote:
> Shouldn't DownloadFile have a global id?  It feels a bit inconsistent that it

SGTM.

> doesn't.  No need to do it for this CL, but eventually we should update that
> (and possibly merge GlobalId and DownloadRequestHandle?  They've got
overlapping
> functionality now.)

Can you explain? DownloadRequestHandle seems to be at the level of
ResourceDispatcherHost as opposed to Download{File,}Manager.

http://codereview.chromium.org/7237034/diff/34029/chrome/browser/download/dow...
File chrome/browser/download/download_history.h (right):

http://codereview.chromium.org/7237034/diff/34029/chrome/browser/download/dow...
chrome/browser/download/download_history.h:35: // Should be much faster than
Load; downloads cannot happen until after this.
On 2011/07/28 21:03:16, rdsmith wrote:
> nit: This comment is inaccurate for this CL, though should be accurate in a
> future CL.  Maybe "so that we may delay downloads after this call with minimal
> performance penalty"?

Done.

http://codereview.chromium.org/7237034/diff/34029/chrome/browser/download/dow...
File chrome/browser/download/download_id.h (right):

http://codereview.chromium.org/7237034/diff/34029/chrome/browser/download/dow...
chrome/browser/download/download_id.h:17: // sessions, but their local() field
is.
On 2011/07/28 21:03:16, rdsmith wrote:
> Huh.  This brings up a really interesting question I'm not sure we've fully
> addressed: Do extensions see the global id or a local id?  This is tightly
> related to: can extensions span profiles?  My guess is that they don't with
the
> possible exception of a profile and its corresponding incognito profile, but
> this should probably be checked with an extension expert.

I was going to show extensions the local id, because it's persistent. I was also
going to make a profile share a download id space with its otr counterpart for
this very reason. I think that it would be a bug if an extension could
communicate across profiles, but I'll ask an extension expert.

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

http://codereview.chromium.org/7237034/diff/34029/chrome/browser/download/dow...
chrome/browser/download/download_item.h:282: DownloadId gid() const;
On 2011/07/28 21:03:16, rdsmith wrote:
> I'd be inclined to call this global_id(); that's what's being used in
> download_file_manager.h, and chrome tends to avoid abbreviations.

Done.

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

http://codereview.chromium.org/7237034/diff/34029/chrome/browser/download/dow...
chrome/browser/download/download_manager.cc:230: base::AutoLock
_(next_id_lock_);
On 2011/07/28 21:03:16, rdsmith wrote:
> I puzzled over this for a while before I got it.  It's a fine idiom, but it's
> not one I've seen before in Chrome, and if you're not used to it I think it'll
> confused people.  I'd use "autolock" instead of "_".

Done.

http://codereview.chromium.org/7237034/diff/34029/chrome/browser/download/dow...
chrome/browser/download/download_manager.cc:232: // = next_id.
On 2011/07/28 21:03:16, rdsmith wrote:
> A bit more of a comment as to why this actually makes sense would make sense,
I
> think (i.e. explain that we just need to make sure assigned ids are unique per
> session for right now, and this does it in the face of the download/getting
> first id race).

Done.

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

http://codereview.chromium.org/7237034/diff/34029/chrome/browser/download/dow...
chrome/browser/download/download_manager.h:66: }  // namespace base
On 2011/07/28 21:03:16, rdsmith wrote:
> I'm having a "I don't think this can work" reaction here.  An incomplete type
> delcaration is ok for pointers or references, but if you declare an actual
> variable, you need a complete type declaration.  And next_id_lock_ is declared
> inline, below.  So I presume that this is compiling only because some other
> include file is including base/lock.h, and that this file should also include
> base/lock.h.

Done.

http://codereview.chromium.org/7237034/diff/34029/chrome/browser/download/dow...
chrome/browser/download/download_manager.h:377: void OnHistoryGetNextId(int
next_id);
On 2011/07/28 21:03:16, rdsmith wrote:
> For the next CL: Consider making the next_id field an argument to the
> DownloadManager constructor.  That'll fit well with the "pulling history out"
> stuff that jam's aiming to do, and should work well with the delaying of the
> profile initialization until we have that information (because we won't need
the
> download manager until then anyway).

So, Profile will use its HistoryService directly and skip DownloadHistory?

http://codereview.chromium.org/7237034/diff/34029/chrome/browser/history/hist...
File chrome/browser/history/history_backend.cc (right):

http://codereview.chromium.org/7237034/diff/34029/chrome/browser/history/hist...
chrome/browser/history/history_backend.cc:1085:
request->ForwardResult(DownloadNextIdRequest::TupleType(request->value));
On 2011/07/28 21:03:16, rdsmith wrote:
> This seems like it'll leave an uninitialized value in the case where db_.get()
> is false.  Shouldn't we put some kind of value in?  (You should probably try
and
> figure out in which cases that can happen so we know what the right way to
> handle it is.)

Done.

For this cl, while DownloadManager says next_id_ += next_id; setting it to 0
turns DownloadManager::OnHistoryGetNextId() into a noop.
I think that setting it to 0 will be ok even for future CLs because, if there's
no database, then there won't be any download items loaded from the database,
and new download items won't be written to the database, because there's no
database. That logic works as long as the db_ isn't initialized after
GetNextDownloadId, which we can ensure in ProfileImpl.

http://codereview.chromium.org/7237034/diff/34029/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_io_data.cc (right):

http://codereview.chromium.org/7237034/diff/34029/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_io_data.cc:194: next_download_id_thunk_ =
profile->GetDownloadManager()->GetNextIdThunk();
On 2011/07/28 21:03:16, rdsmith wrote:
> Why is this in ProfileIOData as well as ResourceContext?  Is it used from
> ProfileIOData, and if so, can a resource_context_ indirection be substituted?

Yes, next_download_id_thunk_ is used in LazyInitialize(), which runs on the IO
thread so it can't GetDownloadManager() there.
Nothing accesses next_download_id_thunk_ through ProfileIOData's public
interface, so I removed that public method.

http://codereview.chromium.org/7237034/diff/34029/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_io_data.cc:510: &next_download_id_thunk_);
On 2011/07/28 21:03:16, rdsmith wrote:
> I presume that the whole nasty reinterpret_cast<> business is because we can't
> include download_manager.h in the ResourceContext declaration because that's
in
> content?  If so, put a TODO here to pull it back out once download_manager.h
is
> moved into content.

Done.

> 
> (What I'd *like* to say is that you should just include download_manager.h in
> the ResourceContext header file, and jam can remove it when he does the move,
> but that probably requires pulling him into this review and modifying the
> exceptions file, and it probably just makes more sense to clean up once he's
> done.)

http://codereview.chromium.org/7237034/diff/34029/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/download_throttling_resource_handler.cc
(right):

http://codereview.chromium.org/7237034/diff/34029/chrome/browser/renderer_hos...
chrome/browser/renderer_host/download_throttling_resource_handler.cc:185: if
(next_download_id_thunk != NULL) {
On 2011/07/28 21:03:16, rdsmith wrote:
> Under what circumstances can next_download_id_thunk be null?

Whenever a ProfileIOData hasn't set it. I'm new here.

> 
> If next_download_id_thunk's null, we'll DCHECK in the download resource
handler
> constructor, and I think DCHECKs are more useful the closer they are to the
> original source of the error, so I'd argue for one here as well.

Done.

[Randy Smith (Not in Mondays), 2011-08-03 21:10:29]

http://codereview.chromium.org/7237034/diff/34029/chrome/browser/download/dow...
File chrome/browser/download/download_file_manager.cc (right):

http://codereview.chromium.org/7237034/diff/34029/chrome/browser/download/dow...
chrome/browser/download/download_file_manager.cc:265:
downloads_.erase(DownloadId((*i)->GetDownloadManager(), (*i)->id()));
On 2011/08/03 17:44:46, b s h wrote:
> On 2011/07/28 21:03:16, rdsmith wrote:
> > Shouldn't DownloadFile have a global id?  It feels a bit inconsistent that
it
> 
> SGTM.
> 
> > doesn't.  No need to do it for this CL, but eventually we should update that
> > (and possibly merge GlobalId and DownloadRequestHandle?  They've got
> overlapping
> > functionality now.)
> 
> Can you explain? DownloadRequestHandle seems to be at the level of
> ResourceDispatcherHost as opposed to Download{File,}Manager.

(I'm taking "at the level" to mean "in the context of"--just mentioning it in
case I'm misinterpreting what you mean.)

That's quite true, and probably a good reason to keep them separate.  They're
just both global handles to a specific download (looking forward to SavePackage
integration, to a specific URLRequest->DownloadFile connection).  But you're
quite right that the context for one is IO thread and the other is file thread,
and that probably means they should be kept separate.

http://codereview.chromium.org/7237034/diff/34029/chrome/browser/download/dow...
File chrome/browser/download/download_id.h (right):

http://codereview.chromium.org/7237034/diff/34029/chrome/browser/download/dow...
chrome/browser/download/download_id.h:17: // sessions, but their local() field
is.
On 2011/08/03 17:44:46, b s h wrote:
> On 2011/07/28 21:03:16, rdsmith wrote:
> > Huh.  This brings up a really interesting question I'm not sure we've fully
> > addressed: Do extensions see the global id or a local id?  This is tightly
> > related to: can extensions span profiles?  My guess is that they don't with
> the
> > possible exception of a profile and its corresponding incognito profile, but
> > this should probably be checked with an extension expert.
> 
> I was going to show extensions the local id, because it's persistent. I was
also
> going to make a profile share a download id space with its otr counterpart for
> this very reason. I think that it would be a bug if an extension could
> communicate across profiles, but I'll ask an extension expert.

Sounds good--I'll feel better if we have someone with that context opining that
individual extensions won't need to be cross profile (except maybe for
incognito<->regular).

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

http://codereview.chromium.org/7237034/diff/34029/chrome/browser/download/dow...
chrome/browser/download/download_manager.h:377: void OnHistoryGetNextId(int
next_id);
On 2011/08/03 17:44:46, b s h wrote:
> On 2011/07/28 21:03:16, rdsmith wrote:
> > For the next CL: Consider making the next_id field an argument to the
> > DownloadManager constructor.  That'll fit well with the "pulling history
out"
> > stuff that jam's aiming to do, and should work well with the delaying of the
> > profile initialization until we have that information (because we won't need
> the
> > download manager until then anyway).
> 
> So, Profile will use its HistoryService directly and skip DownloadHistory?

Yep.  Or if there's some reason to, it can construct DownloadHistory, use it,
and pass it in to DownloadManager.  My guess is that leaving this in
DownloadManager will mean there are some difficulties in the profile
synchronization implementation (basically, figuring out when DownloadManager has
gotten the next_id), and that those problems will be avoided if the profile impl
does it itself.  But it's just a suggestion based on my guess as to what'll
produce the most clean code; I wanted it in your head, but use your judgement
when you actually do it.

http://codereview.chromium.org/7237034/diff/34029/chrome/browser/history/hist...
File chrome/browser/history/history_backend.cc (right):

http://codereview.chromium.org/7237034/diff/34029/chrome/browser/history/hist...
chrome/browser/history/history_backend.cc:1085:
request->ForwardResult(DownloadNextIdRequest::TupleType(request->value));
On 2011/08/03 17:44:46, b s h wrote:
> On 2011/07/28 21:03:16, rdsmith wrote:
> > This seems like it'll leave an uninitialized value in the case where
db_.get()
> > is false.  Shouldn't we put some kind of value in?  (You should probably try
> and
> > figure out in which cases that can happen so we know what the right way to
> > handle it is.)
> 
> Done.
> 
> For this cl, while DownloadManager says next_id_ += next_id; setting it to 0
> turns DownloadManager::OnHistoryGetNextId() into a noop.
> I think that setting it to 0 will be ok even for future CLs because, if
there's
> no database, then there won't be any download items loaded from the database,
> and new download items won't be written to the database, because there's no
> database. That logic works as long as the db_ isn't initialized after
> GetNextDownloadId, which we can ensure in ProfileImpl.

Yes--no collisions to avoid.  There may be circumstances in which db_ is null
when there is a database; there are comments sprinkled through the code about
"the database being offline".  But I believe in those circumstances we don't
talk to the database (can't initialize from it, can't save to it) so we're ok.

http://codereview.chromium.org/7237034/diff/34029/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/download_throttling_resource_handler.cc
(right):

http://codereview.chromium.org/7237034/diff/34029/chrome/browser/renderer_hos...
chrome/browser/renderer_host/download_throttling_resource_handler.cc:185: if
(next_download_id_thunk != NULL) {
On 2011/08/03 17:44:46, b s h wrote:
> On 2011/07/28 21:03:16, rdsmith wrote:
> > Under what circumstances can next_download_id_thunk be null?
> 
> Whenever a ProfileIOData hasn't set it. I'm new here.

:-}.  

I'm afraid you still get the unenviable task of nailing down the answer to the
question, since if there are cases in which that can happen in normal operation,
this code'll produce a crash (debug mode) or random values and possibly behavior
(debug mode).  Sorry :-{.

http://codereview.chromium.org/7237034/diff/44001/chrome/browser/download/dow...
File chrome/browser/download/download_id.h (right):

http://codereview.chromium.org/7237034/diff/44001/chrome/browser/download/dow...
chrome/browser/download/download_id.h:34: bool BelongsTo(const DownloadManager*
test_man) const {
What's the intended use for this routine?  It doesn't seem to be used in this
CL.

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

http://codereview.chromium.org/7237034/diff/44001/chrome/browser/download/dow...
chrome/browser/download/download_manager.cc:231: // from the history don't have
valid ids.
Actually, what we're counting on is that GetNextId() can return any number of
contiguous integers starting from 0, which after the += we will never visit
again in this session.  I.e. we're not relying on GetNextId() only being called
once before this function is called.

http://codereview.chromium.org/7237034/diff/44001/chrome/browser/download/dow...
chrome/browser/download/download_manager.cc:255:
download_history_->GetNextId(NewCallback(
You might want to note that this isn't guaranteed to return; specifically, if
the user has disabled history or history initialization has failed, it won't.

http://codereview.chromium.org/7237034/diff/44001/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/download_throttling_resource_handler.cc
(right):

http://codereview.chromium.org/7237034/diff/44001/chrome/browser/renderer_hos...
chrome/browser/renderer_host/download_throttling_resource_handler.cc:182: void*
next_download_id_thunk = info->context()->GetUserData(
Will we also be able to clean this up when download_manager moves into content? 
If so, put in a TODO; if not, let's chat, as this is pretty gnarly casting code
and it would be good to find some other way to do this.

http://codereview.chromium.org/7237034/diff/44001/content/browser/renderer_ho...
File content/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/7237034/diff/44001/content/browser/renderer_ho...
content/browser/renderer_host/resource_dispatcher_host.cc:730: void*
next_download_id_thunk = context.GetUserData(reinterpret_cast<void*>(
Same comment: TODO based on download_manager.h going into content, or let's try
and find some other way to do it.

[benjhayden, 2011-08-04 17:15:00]

http://codereview.chromium.org/7237034/diff/34029/chrome/browser/download/dow...
File chrome/browser/download/download_file_manager.cc (right):

http://codereview.chromium.org/7237034/diff/34029/chrome/browser/download/dow...
chrome/browser/download/download_file_manager.cc:265:
downloads_.erase(DownloadId((*i)->GetDownloadManager(), (*i)->id()));
On 2011/08/03 21:10:29, rdsmith wrote:
> On 2011/08/03 17:44:46, b s h wrote:
> > On 2011/07/28 21:03:16, rdsmith wrote:
> > > Shouldn't DownloadFile have a global id?  It feels a bit inconsistent that
> it
> > 
> > SGTM.
> > 
> > > doesn't.  No need to do it for this CL, but eventually we should update
that
> > > (and possibly merge GlobalId and DownloadRequestHandle?  They've got
> > overlapping
> > > functionality now.)
> > 
> > Can you explain? DownloadRequestHandle seems to be at the level of
> > ResourceDispatcherHost as opposed to Download{File,}Manager.
> 
> (I'm taking "at the level" to mean "in the context of"--just mentioning it in
> case I'm misinterpreting what you mean.)
> 
> That's quite true, and probably a good reason to keep them separate.  They're
> just both global handles to a specific download (looking forward to
SavePackage
> integration, to a specific URLRequest->DownloadFile connection).  But you're
> quite right that the context for one is IO thread and the other is file
thread,
> and that probably means they should be kept separate.

Done.

http://codereview.chromium.org/7237034/diff/34029/chrome/browser/download/dow...
File chrome/browser/download/download_id.h (right):

http://codereview.chromium.org/7237034/diff/34029/chrome/browser/download/dow...
chrome/browser/download/download_id.h:17: // sessions, but their local() field
is.
On 2011/08/03 21:10:29, rdsmith wrote:
> On 2011/08/03 17:44:46, b s h wrote:
> > On 2011/07/28 21:03:16, rdsmith wrote:
> > > Huh.  This brings up a really interesting question I'm not sure we've
fully
> > > addressed: Do extensions see the global id or a local id?  This is tightly
> > > related to: can extensions span profiles?  My guess is that they don't
with
> > the
> > > possible exception of a profile and its corresponding incognito profile,
but
> > > this should probably be checked with an extension expert.
> > 
> > I was going to show extensions the local id, because it's persistent. I was
> also
> > going to make a profile share a download id space with its otr counterpart
for
> > this very reason. I think that it would be a bug if an extension could
> > communicate across profiles, but I'll ask an extension expert.
> 
> Sounds good--I'll feel better if we have someone with that context opining
that
> individual extensions won't need to be cross profile (except maybe for
> incognito<->regular).

ProfileImpl seems to suggest that there is exactly one ExtensionService per
non-otr profile. I'll ask Mihai when I IM him about my extension stubs CL when
he comes back from vacation on Monday.

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

http://codereview.chromium.org/7237034/diff/34029/chrome/browser/download/dow...
chrome/browser/download/download_manager.h:377: void OnHistoryGetNextId(int
next_id);
On 2011/08/03 21:10:29, rdsmith wrote:
> On 2011/08/03 17:44:46, b s h wrote:
> > On 2011/07/28 21:03:16, rdsmith wrote:
> > > For the next CL: Consider making the next_id field an argument to the
> > > DownloadManager constructor.  That'll fit well with the "pulling history
> out"
> > > stuff that jam's aiming to do, and should work well with the delaying of
the
> > > profile initialization until we have that information (because we won't
need
> > the
> > > download manager until then anyway).
> > 
> > So, Profile will use its HistoryService directly and skip DownloadHistory?
> 
> Yep.  Or if there's some reason to, it can construct DownloadHistory, use it,
> and pass it in to DownloadManager.  My guess is that leaving this in
> DownloadManager will mean there are some difficulties in the profile
> synchronization implementation (basically, figuring out when DownloadManager
has
> gotten the next_id), and that those problems will be avoided if the profile
impl
> does it itself.  But it's just a suggestion based on my guess as to what'll
> produce the most clean code; I wanted it in your head, but use your judgement
> when you actually do it.

Done.

http://codereview.chromium.org/7237034/diff/34029/chrome/browser/history/hist...
File chrome/browser/history/history_backend.cc (right):

http://codereview.chromium.org/7237034/diff/34029/chrome/browser/history/hist...
chrome/browser/history/history_backend.cc:1085:
request->ForwardResult(DownloadNextIdRequest::TupleType(request->value));
On 2011/08/03 21:10:29, rdsmith wrote:
> On 2011/08/03 17:44:46, b s h wrote:
> > On 2011/07/28 21:03:16, rdsmith wrote:
> > > This seems like it'll leave an uninitialized value in the case where
> db_.get()
> > > is false.  Shouldn't we put some kind of value in?  (You should probably
try
> > and
> > > figure out in which cases that can happen so we know what the right way to
> > > handle it is.)
> > 
> > Done.
> > 
> > For this cl, while DownloadManager says next_id_ += next_id; setting it to 0
> > turns DownloadManager::OnHistoryGetNextId() into a noop.
> > I think that setting it to 0 will be ok even for future CLs because, if
> there's
> > no database, then there won't be any download items loaded from the
database,
> > and new download items won't be written to the database, because there's no
> > database. That logic works as long as the db_ isn't initialized after
> > GetNextDownloadId, which we can ensure in ProfileImpl.
> 
> Yes--no collisions to avoid.  There may be circumstances in which db_ is null
> when there is a database; there are comments sprinkled through the code about
> "the database being offline".  But I believe in those circumstances we don't
> talk to the database (can't initialize from it, can't save to it) so we're ok.
 

Done.

http://codereview.chromium.org/7237034/diff/34029/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/download_throttling_resource_handler.cc
(right):

http://codereview.chromium.org/7237034/diff/34029/chrome/browser/renderer_hos...
chrome/browser/renderer_host/download_throttling_resource_handler.cc:185: if
(next_download_id_thunk != NULL) {
On 2011/08/03 21:10:29, rdsmith wrote:
> On 2011/08/03 17:44:46, b s h wrote:
> > On 2011/07/28 21:03:16, rdsmith wrote:
> > > Under what circumstances can next_download_id_thunk be null?
> > 
> > Whenever a ProfileIOData hasn't set it. I'm new here.
> 
> :-}.  
> 
> I'm afraid you still get the unenviable task of nailing down the answer to the
> question, since if there are cases in which that can happen in normal
operation,
> this code'll produce a crash (debug mode) or random values and possibly
behavior
> (debug mode).  Sorry :-{.

Apologies for the smartass. Haven't been sleeping well.
I'll start studying ResourceContext today.

http://codereview.chromium.org/7237034/diff/44001/chrome/browser/download/dow...
File chrome/browser/download/download_id.h (right):

http://codereview.chromium.org/7237034/diff/44001/chrome/browser/download/dow...
chrome/browser/download/download_id.h:34: bool BelongsTo(const DownloadManager*
test_man) const {
On 2011/08/03 21:10:29, rdsmith wrote:
> What's the intended use for this routine?  It doesn't seem to be used in this
> CL.

I was imagining sprinkling DCHECK(global_id.BelongsTo(this)) throughout
DownloadManager when I make it only deal in DownloadIds, just in case somebody
ever plays fast and loose with their DM*s. WDYT?

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

http://codereview.chromium.org/7237034/diff/44001/chrome/browser/download/dow...
chrome/browser/download/download_manager.cc:231: // from the history don't have
valid ids.
On 2011/08/03 21:10:29, rdsmith wrote:
> Actually, what we're counting on is that GetNextId() can return any number of
> contiguous integers starting from 0, which after the += we will never visit
> again in this session.  I.e. we're not relying on GetNextId() only being
called
> once before this function is called.

Done.

http://codereview.chromium.org/7237034/diff/44001/chrome/browser/download/dow...
chrome/browser/download/download_manager.cc:255:
download_history_->GetNextId(NewCallback(
On 2011/08/03 21:10:29, rdsmith wrote:
> You might want to note that this isn't guaranteed to return; specifically, if
> the user has disabled history or history initialization has failed, it won't.

Done.

http://codereview.chromium.org/7237034/diff/44001/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/download_throttling_resource_handler.cc
(right):

http://codereview.chromium.org/7237034/diff/44001/chrome/browser/renderer_hos...
chrome/browser/renderer_host/download_throttling_resource_handler.cc:182: void*
next_download_id_thunk = info->context()->GetUserData(
On 2011/08/03 21:10:29, rdsmith wrote:
> Will we also be able to clean this up when download_manager moves into
content? 
> If so, put in a TODO; if not, let's chat, as this is pretty gnarly casting
code
> and it would be good to find some other way to do this.

Done.

http://codereview.chromium.org/7237034/diff/44001/content/browser/renderer_ho...
File content/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/7237034/diff/44001/content/browser/renderer_ho...
content/browser/renderer_host/resource_dispatcher_host.cc:730: void*
next_download_id_thunk = context.GetUserData(reinterpret_cast<void*>(
On 2011/08/03 21:10:29, rdsmith wrote:
> Same comment: TODO based on download_manager.h going into content, or let's
try
> and find some other way to do it.

Done.

[benjhayden, 2011-08-04 18:49:44]

http://codereview.chromium.org/7237034/diff/34029/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/download_throttling_resource_handler.cc
(right):

http://codereview.chromium.org/7237034/diff/34029/chrome/browser/renderer_hos...
chrome/browser/renderer_host/download_throttling_resource_handler.cc:185: if
(next_download_id_thunk != NULL) {
On 2011/08/03 21:10:29, rdsmith wrote:
> On 2011/08/03 17:44:46, b s h wrote:
> > On 2011/07/28 21:03:16, rdsmith wrote:
> > > Under what circumstances can next_download_id_thunk be null?
> > 
> > Whenever a ProfileIOData hasn't set it. I'm new here.
> 
> :-}.  
> 
> I'm afraid you still get the unenviable task of nailing down the answer to the
> question, since if there are cases in which that can happen in normal
operation,
> this code'll produce a crash (debug mode) or random values and possibly
behavior
> (debug mode).  Sorry :-{.

According to codesearch, ProfileIOData is the only thing that instantiates the
only subclass of ResourceContext.
So, there can't be a ResourceContext that wasn't initialized by ProfileIOData,
which always sets the thunk.
I can change the if to a check if you want.

[Randy Smith (Not in Mondays), 2011-08-04 19:52:26]

LGTM with the minor issues below.

http://codereview.chromium.org/7237034/diff/34029/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/download_throttling_resource_handler.cc
(right):

http://codereview.chromium.org/7237034/diff/34029/chrome/browser/renderer_hos...
chrome/browser/renderer_host/download_throttling_resource_handler.cc:185: if
(next_download_id_thunk != NULL) {
On 2011/08/04 17:15:00, b s h wrote:
> On 2011/08/03 21:10:29, rdsmith wrote:
> > On 2011/08/03 17:44:46, b s h wrote:
> > > On 2011/07/28 21:03:16, rdsmith wrote:
> > > > Under what circumstances can next_download_id_thunk be null?
> > > 
> > > Whenever a ProfileIOData hasn't set it. I'm new here.
> > 
> > :-}.  
> > 
> > I'm afraid you still get the unenviable task of nailing down the answer to
the
> > question, since if there are cases in which that can happen in normal
> operation,
> > this code'll produce a crash (debug mode) or random values and possibly
> behavior
> > (debug mode).  Sorry :-{.
> 
> Apologies for the smartass. Haven't been sleeping well.
> I'll start studying ResourceContext today.

No worries; sorry if I came across as harsh.

http://codereview.chromium.org/7237034/diff/34029/chrome/browser/renderer_hos...
chrome/browser/renderer_host/download_throttling_resource_handler.cc:185: if
(next_download_id_thunk != NULL) {
On 2011/08/04 18:49:44, b s h wrote:
> On 2011/08/03 21:10:29, rdsmith wrote:
> > On 2011/08/03 17:44:46, b s h wrote:
> > > On 2011/07/28 21:03:16, rdsmith wrote:
> > > > Under what circumstances can next_download_id_thunk be null?
> > > 
> > > Whenever a ProfileIOData hasn't set it. I'm new here.
> > 
> > :-}.  
> > 
> > I'm afraid you still get the unenviable task of nailing down the answer to
the
> > question, since if there are cases in which that can happen in normal
> operation,
> > this code'll produce a crash (debug mode) or random values and possibly
> behavior
> > (debug mode).  Sorry :-{.
> 
> According to codesearch, ProfileIOData is the only thing that instantiates the
> only subclass of ResourceContext.
> So, there can't be a ResourceContext that wasn't initialized by ProfileIOData,
> which always sets the thunk.
> I can change the if to a check if you want.

Yes, please.  The problem is that the if implies that we might not go through
that loop, and in that case we have an uninitialized download id, which is bad. 
If we believe we're never going to have an uninitialized download id, let's have
the code reflect that, which means a check.

http://codereview.chromium.org/7237034/diff/44001/chrome/browser/download/dow...
File chrome/browser/download/download_id.h (right):

http://codereview.chromium.org/7237034/diff/44001/chrome/browser/download/dow...
chrome/browser/download/download_id.h:34: bool BelongsTo(const DownloadManager*
test_man) const {
On 2011/08/04 17:15:00, b s h wrote:
> On 2011/08/03 21:10:29, rdsmith wrote:
> > What's the intended use for this routine?  It doesn't seem to be used in
this
> > CL.
> 
> I was imagining sprinkling DCHECK(global_id.BelongsTo(this)) throughout
> DownloadManager when I make it only deal in DownloadIds, just in case somebody
> ever plays fast and loose with their DM*s. WDYT?

I'm fine with that.  The perfectionist in me sorta wants this routine going in
when you actually start using it, but that's not a big deal at all. 

I think I would request that you change the argument to "manager" or
"download_manager" from "test_man", both to avoid the abbreviation, and because
the current name sorta implies that the routine is here for testing only.

[benjhayden, 2011-08-04 21:05:13]

http://codereview.chromium.org/7237034/diff/34029/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/download_throttling_resource_handler.cc
(right):

http://codereview.chromium.org/7237034/diff/34029/chrome/browser/renderer_hos...
chrome/browser/renderer_host/download_throttling_resource_handler.cc:185: if
(next_download_id_thunk != NULL) {
On 2011/08/04 19:52:27, rdsmith wrote:
> On 2011/08/04 18:49:44, b s h wrote:
> > On 2011/08/03 21:10:29, rdsmith wrote:
> > > On 2011/08/03 17:44:46, b s h wrote:
> > > > On 2011/07/28 21:03:16, rdsmith wrote:
> > > > > Under what circumstances can next_download_id_thunk be null?
> > > > 
> > > > Whenever a ProfileIOData hasn't set it. I'm new here.
> > > 
> > > :-}.  
> > > 
> > > I'm afraid you still get the unenviable task of nailing down the answer to
> the
> > > question, since if there are cases in which that can happen in normal
> > operation,
> > > this code'll produce a crash (debug mode) or random values and possibly
> > behavior
> > > (debug mode).  Sorry :-{.
> > 
> > According to codesearch, ProfileIOData is the only thing that instantiates
the
> > only subclass of ResourceContext.
> > So, there can't be a ResourceContext that wasn't initialized by
ProfileIOData,
> > which always sets the thunk.
> > I can change the if to a check if you want.
> 
> Yes, please.  The problem is that the if implies that we might not go through
> that loop, and in that case we have an uninitialized download id, which is
bad. 
> If we believe we're never going to have an uninitialized download id, let's
have
> the code reflect that, which means a check.

Done.

http://codereview.chromium.org/7237034/diff/34029/chrome/browser/renderer_hos...
chrome/browser/renderer_host/download_throttling_resource_handler.cc:185: if
(next_download_id_thunk != NULL) {
On 2011/08/04 19:52:27, rdsmith wrote:
> On 2011/08/04 17:15:00, b s h wrote:
> > On 2011/08/03 21:10:29, rdsmith wrote:
> > > On 2011/08/03 17:44:46, b s h wrote:
> > > > On 2011/07/28 21:03:16, rdsmith wrote:
> > > > > Under what circumstances can next_download_id_thunk be null?
> > > > 
> > > > Whenever a ProfileIOData hasn't set it. I'm new here.
> > > 
> > > :-}.  
> > > 
> > > I'm afraid you still get the unenviable task of nailing down the answer to
> the
> > > question, since if there are cases in which that can happen in normal
> > operation,
> > > this code'll produce a crash (debug mode) or random values and possibly
> > behavior
> > > (debug mode).  Sorry :-{.
> > 
> > Apologies for the smartass. Haven't been sleeping well.
> > I'll start studying ResourceContext today.
> 
> No worries; sorry if I came across as harsh.

Not at all.

[benjhayden, 2011-08-05 14:15:34]

Fixing the msvc build and looking into the browser_tests failures now.

[benjhayden, 2011-08-05 17:24:48]

On 2011/08/05 14:15:34, b s h wrote:
> Fixing the msvc build and looking into the browser_tests failures now.

I think I fixed the msvc build. We'll see shortly.

The browser_tests linux "failures" (TwoExtensionsCrashBothAtOnce,
PrerenderDownloadClientRedirect, PrerenderDownloadLocation) say PASSED, and look
the same on a clean client, so I think I'll ignore them.

[benjhayden, 2011-08-05 21:11:09]

On 2011/08/05 17:24:48, b s h wrote:
> On 2011/08/05 14:15:34, b s h wrote:
> > Fixing the msvc build and looking into the browser_tests failures now.
> 
> I think I fixed the msvc build. We'll see shortly.
> 
> The browser_tests linux "failures" (TwoExtensionsCrashBothAtOnce,
> PrerenderDownloadClientRedirect, PrerenderDownloadLocation) say PASSED, and
look
> the same on a clean client, so I think I'll ignore them.

Last call for comments/requests!
I'll submit this on Monday after merging/syncing and rerunning the tests if
there are no more concerns.
Thanks!

[willchan no longer on Chromium, 2011-08-06 14:08:35]

http://codereview.chromium.org/7237034/diff/57029/chrome/browser/download/dow...
File chrome/browser/download/download_id.h (right):

http://codereview.chromium.org/7237034/diff/57029/chrome/browser/download/dow...
chrome/browser/download/download_id.h:20: explicit DownloadId(const
DownloadManager* manager = NULL,
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Default_Arguments
says default arguments are disallowed.

http://codereview.chromium.org/7237034/diff/57029/chrome/browser/download/dow...
chrome/browser/download/download_id.h:41: // Returns true if this refers to the
same DownloadItem as that.
Where do you need this operator==?

http://codereview.chromium.org/7237034/diff/57029/chrome/browser/download/dow...
chrome/browser/download/download_id.h:46: bool operator<(const DownloadId& that)
const {
operator< does not make sense and I'm not sure you need it. |manager_ <
that.manager_| is not a well defined ordering. Are you using std::map? I see a
hash_map use, and hash_map doesn't require operator<.

http://codereview.chromium.org/7237034/diff/57029/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_io_data.cc (right):

http://codereview.chromium.org/7237034/diff/57029/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_io_data.cc:492:
resource_context_.SetUserData(reinterpret_cast<void*>(BASE_HASH_NAMESPACE::
Don't use SetUserData() for this. Create a new setter / accessor for it. void*
lookup tricks are only useful when you want to abstract out the dependencies
between layers. Higher layers may set a void*, and when the lower layer calls
into the higher layer, the higher layer might "know" that the void* corresponds
to a T*, and can safely cast to T*. In this case, content/ is the lower layer
and content/ needs to know the concrete type T anyway, so this gains us nothing.
So, it's better to just use specific setters/getters and thus allow static type
checking to help us.

http://codereview.chromium.org/7237034/diff/57029/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_io_data.h (right):

http://codereview.chromium.org/7237034/diff/57029/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_io_data.h:21: #include
"chrome/browser/download/download_manager.h"
sort in alphabetical order please

[benjhayden, 2011-08-08 16:50:19]

http://codereview.chromium.org/7237034/diff/57029/chrome/browser/download/dow...
File chrome/browser/download/download_id.h (right):

http://codereview.chromium.org/7237034/diff/57029/chrome/browser/download/dow...
chrome/browser/download/download_id.h:20: explicit DownloadId(const
DownloadManager* manager = NULL,
On 2011/08/06 14:08:36, willchan wrote:
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Default_Arguments
> says default arguments are disallowed.

Done.

http://codereview.chromium.org/7237034/diff/57029/chrome/browser/download/dow...
chrome/browser/download/download_id.h:41: // Returns true if this refers to the
same DownloadItem as that.
On 2011/08/06 14:08:36, willchan wrote:
> Where do you need this operator==?

It looks like hash_map uses operator== on its keys.

stl_function.h: In member function bool std::equal_to...
download_file_manager.cc:75: instantiated from here
stl_function.h:203: error: no match for operator== in __x==__y

http://codereview.chromium.org/7237034/diff/57029/chrome/browser/download/dow...
chrome/browser/download/download_id.h:46: bool operator<(const DownloadId& that)
const {
On 2011/08/06 14:08:36, willchan wrote:
> operator< does not make sense and I'm not sure you need it. |manager_ <
> that.manager_| is not a well defined ordering. Are you using std::map? I see a
> hash_map use, and hash_map doesn't require operator<.

Done.

http://codereview.chromium.org/7237034/diff/57029/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_io_data.cc (right):

http://codereview.chromium.org/7237034/diff/57029/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_io_data.cc:492:
resource_context_.SetUserData(reinterpret_cast<void*>(BASE_HASH_NAMESPACE::
On 2011/08/06 14:08:36, willchan wrote:
> Don't use SetUserData() for this. Create a new setter / accessor for it. void*
> lookup tricks are only useful when you want to abstract out the dependencies
> between layers. Higher layers may set a void*, and when the lower layer calls
> into the higher layer, the higher layer might "know" that the void*
corresponds
> to a T*, and can safely cast to T*. In this case, content/ is the lower layer
> and content/ needs to know the concrete type T anyway, so this gains us
nothing.
> So, it's better to just use specific setters/getters and thus allow static
type
> checking to help us.

Sorry, are you suggesting having content/browser/resource_context.h include
chrome/browser/download/download_manager.h so that ResourceContext can contain a
GetNextIdThunkType, or having a void* field in ResourceContext?

http://codereview.chromium.org/7237034/diff/57029/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_io_data.h (right):

http://codereview.chromium.org/7237034/diff/57029/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_io_data.h:21: #include
"chrome/browser/download/download_manager.h"
On 2011/08/06 14:08:36, willchan wrote:
> sort in alphabetical order please

Done.

[willchan no longer on Chromium, 2011-08-10 01:24:20]

http://codereview.chromium.org/7237034/diff/57029/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_io_data.cc (right):

http://codereview.chromium.org/7237034/diff/57029/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_io_data.cc:492:
resource_context_.SetUserData(reinterpret_cast<void*>(BASE_HASH_NAMESPACE::
On 2011/08/08 16:50:19, b s h wrote:
> On 2011/08/06 14:08:36, willchan wrote:
> > Don't use SetUserData() for this. Create a new setter / accessor for it.
void*
> > lookup tricks are only useful when you want to abstract out the dependencies
> > between layers. Higher layers may set a void*, and when the lower layer
calls
> > into the higher layer, the higher layer might "know" that the void*
> corresponds
> > to a T*, and can safely cast to T*. In this case, content/ is the lower
layer
> > and content/ needs to know the concrete type T anyway, so this gains us
> nothing.
> > So, it's better to just use specific setters/getters and thus allow static
> type
> > checking to help us.
> 
> Sorry, are you suggesting having content/browser/resource_context.h include
> chrome/browser/download/download_manager.h so that ResourceContext can contain
a
> GetNextIdThunkType, or having a void* field in ResourceContext?

I'm suggesting including the necessary header.

I'm not sure if I'm reading too much into your question, but I suspect you may
be concerned that this creates an undesirable dependency from ResourceContext
onto chrome/. If so, don't worry about this specific instance. You would be
correct to worry about content/ depending on chrome/. However,
download_manager.h is still being used by content/, in ResourceDispatcherHost.
It sounds from the earlier messages by rdsmith in this thread that
DownloadManager will need to move into content/. If so, this isn't a bad
dependency. If DownloadManager doesn't need to move into content/, then at the
time that DownloadManager gets removed from ResourceDispatcherHost, this
dependency here will need to be removed from ResourceContext.

http://codereview.chromium.org/7237034/diff/65001/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_io_data.h (right):

http://codereview.chromium.org/7237034/diff/65001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_io_data.h:254: mutable
DownloadManager::GetNextIdThunkType next_download_id_thunk_;
Can you move this down to the ResourceContext section? I think that's the only
place you're using it.

[benjhayden, 2011-08-16 19:54:15]

http://codereview.chromium.org/7237034/diff/57029/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_io_data.cc (right):

http://codereview.chromium.org/7237034/diff/57029/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_io_data.cc:492:
resource_context_.SetUserData(reinterpret_cast<void*>(BASE_HASH_NAMESPACE::
On 2011/08/10 01:24:20, willchan wrote:
> On 2011/08/08 16:50:19, b s h wrote:
> > On 2011/08/06 14:08:36, willchan wrote:
> > > Don't use SetUserData() for this. Create a new setter / accessor for it.
> void*
> > > lookup tricks are only useful when you want to abstract out the
dependencies
> > > between layers. Higher layers may set a void*, and when the lower layer
> calls
> > > into the higher layer, the higher layer might "know" that the void*
> > corresponds
> > > to a T*, and can safely cast to T*. In this case, content/ is the lower
> layer
> > > and content/ needs to know the concrete type T anyway, so this gains us
> > nothing.
> > > So, it's better to just use specific setters/getters and thus allow static
> > type
> > > checking to help us.
> > 
> > Sorry, are you suggesting having content/browser/resource_context.h include
> > chrome/browser/download/download_manager.h so that ResourceContext can
contain
> a
> > GetNextIdThunkType, or having a void* field in ResourceContext?
> 
> I'm suggesting including the necessary header.
> 
> I'm not sure if I'm reading too much into your question, but I suspect you may
> be concerned that this creates an undesirable dependency from ResourceContext
> onto chrome/. If so, don't worry about this specific instance. You would be
> correct to worry about content/ depending on chrome/. However,
> download_manager.h is still being used by content/, in ResourceDispatcherHost.
> It sounds from the earlier messages by rdsmith in this thread that
> DownloadManager will need to move into content/. If so, this isn't a bad
> dependency. If DownloadManager doesn't need to move into content/, then at the
> time that DownloadManager gets removed from ResourceDispatcherHost, this
> dependency here will need to be removed from ResourceContext.

Done.

http://codereview.chromium.org/7237034/diff/65001/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_io_data.h (right):

http://codereview.chromium.org/7237034/diff/65001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_io_data.h:254: mutable
DownloadManager::GetNextIdThunkType next_download_id_thunk_;
On 2011/08/10 01:24:20, willchan wrote:
> Can you move this down to the ResourceContext section? I think that's the only
> place you're using it.

Done.

[commit-bot: I haz the power, 2011-08-23 13:38:18]

Can't apply patch for file content/browser/download/download_manager.cc.
While running patch -p1 --forward --force;
patching file content/browser/download/download_manager.cc
Hunk #1 succeeded at 6 with fuzz 1.
Hunk #2 FAILED at 61.
Hunk #3 succeeded at 181 with fuzz 2 (offset -45 lines).
Hunk #4 FAILED at 258.
Hunk #5 succeeded at 366 (offset -51 lines).
Hunk #6 succeeded at 534 (offset -52 lines).
Hunk #7 succeeded at 573 (offset -52 lines).
Hunk #8 succeeded at 608 with fuzz 1 (offset -52 lines).
2 out of 8 hunks FAILED -- saving rejects to file
content/browser/download/download_manager.cc.rej

[commit-bot: I haz the power, 2011-08-23 16:50:39]

Presubmit check for 7237034-86061 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
If this change has an associated bug, add BUG=[bug number].

If this change requires manual test instructions to QA team, add
TEST=[instructions].

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
content/browser/renderer_host/resource_dispatcher_host.cc,content/browser/resource_context.cc,content/browser/resource_context.h,content/browser/renderer_host/resource_dispatcher_host_unittest.cc,content/browser/renderer_host/buffered_resource_handler.cc,chrome/browser/profiles/profile_io_data.cc,chrome/browser/profiles/profile_io_data.h

Presubmit checks took 2.0s to calculate.

[benjhayden, 2011-08-23 16:59:19]

John, can you take a quick look at this CL?

[jam, 2011-08-23 17:54:19]

you'll need someone from the profiles OWNERS to lgtm those files

I just looked at the content\browser code (excluding download)

nit: also please don't add empty BUG= and TEST= lines. BUG= is for associating a
change with a bug. if there's nothing to associate it to, then it's not
necessary. TEST= is to give manual test instructions to the test team. if you
don't have anything to tell them, then the line isn't necessary.

http://codereview.chromium.org/7237034/diff/86061/content/browser/renderer_ho...
File content/browser/renderer_host/buffered_resource_handler.cc (right):

http://codereview.chromium.org/7237034/diff/86061/content/browser/renderer_ho...
content/browser/renderer_host/buffered_resource_handler.cc:313:
CHECK(!info->context()->next_download_id_thunk().is_null());
nit: these two checks are redundant, since if they're null they will crash on
the line below. we usually skip dcheck/dcheck on variables before using them.

http://codereview.chromium.org/7237034/diff/86061/content/browser/renderer_ho...
File content/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/7237034/diff/86061/content/browser/renderer_ho...
content/browser/renderer_host/resource_dispatcher_host.cc:811:
CHECK(!context.next_download_id_thunk().is_null());
nit: ditto

http://codereview.chromium.org/7237034/diff/86061/content/browser/resource_co...
File content/browser/resource_context.h (right):

http://codereview.chromium.org/7237034/diff/86061/content/browser/resource_co...
content/browser/resource_context.h:79: const
DownloadManager::GetNextIdThunkType& next_download_id_thunk() const;
it seems odd to add a thunk to ResourceContext. it feels very C-ish to add a
function pointer there. can we have an interface instead?

willchan (who wrote ResourceContext): I realize you already looked at this, but
I'm curious wdyt?

[benjhayden, 2011-08-24 14:33:00]

http://codereview.chromium.org/7237034/diff/86061/content/browser/renderer_ho...
File content/browser/renderer_host/buffered_resource_handler.cc (right):

http://codereview.chromium.org/7237034/diff/86061/content/browser/renderer_ho...
content/browser/renderer_host/buffered_resource_handler.cc:313:
CHECK(!info->context()->next_download_id_thunk().is_null());
On 2011/08/23 17:54:19, John Abd-El-Malek wrote:
> nit: these two checks are redundant, since if they're null they will crash on
> the line below. we usually skip dcheck/dcheck on variables before using them.

Done.

http://codereview.chromium.org/7237034/diff/86061/content/browser/renderer_ho...
File content/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/7237034/diff/86061/content/browser/renderer_ho...
content/browser/renderer_host/resource_dispatcher_host.cc:811:
CHECK(!context.next_download_id_thunk().is_null());
On 2011/08/23 17:54:19, John Abd-El-Malek wrote:
> nit: ditto

Done.

http://codereview.chromium.org/7237034/diff/86061/content/browser/resource_co...
File content/browser/resource_context.h (right):

http://codereview.chromium.org/7237034/diff/86061/content/browser/resource_co...
content/browser/resource_context.h:79: const
DownloadManager::GetNextIdThunkType& next_download_id_thunk() const;
On 2011/08/23 17:54:19, John Abd-El-Malek wrote:
> it seems odd to add a thunk to ResourceContext. it feels very C-ish to add a
> function pointer there. can we have an interface instead?
> 
> willchan (who wrote ResourceContext): I realize you already looked at this,
but
> I'm curious wdyt?

Yes, we are planning on promoting the thunk to a first-class, um, class in my
next CL. TODone.

[jam, 2011-08-24 16:35:36]

http://codereview.chromium.org/7237034/diff/86061/content/browser/resource_co...
File content/browser/resource_context.h (right):

http://codereview.chromium.org/7237034/diff/86061/content/browser/resource_co...
content/browser/resource_context.h:79: const
DownloadManager::GetNextIdThunkType& next_download_id_thunk() const;
On 2011/08/24 14:33:00, b s h wrote:
> On 2011/08/23 17:54:19, John Abd-El-Malek wrote:
> > it seems odd to add a thunk to ResourceContext. it feels very C-ish to add a
> > function pointer there. can we have an interface instead?
> > 
> > willchan (who wrote ResourceContext): I realize you already looked at this,
> but
> > I'm curious wdyt?
> 
> Yes, we are planning on promoting the thunk to a first-class, um, class in my
> next CL. TODone.

I'm curious, why check in the thunk code if you're planning on changing it in
the next cl? i.e. why not just add a class now?

[benjhayden, 2011-08-24 16:53:27]

Hi, Miranda,
Would you mind taking a quick look at this ProfileIOData change or suggesting
somebody from profiles/OWNERS who can besides Will? Will was looking at it, but
disappeared on vacation without appointing a successor. I picked you out of
profiles/OWNERS because you're in this timezone and because you're at the top of
the file, so feel free to redirect me.
Thanks!

[benjhayden, 2011-08-24 17:22:36]

http://codereview.chromium.org/7237034/diff/86061/content/browser/resource_co...
File content/browser/resource_context.h (right):

http://codereview.chromium.org/7237034/diff/86061/content/browser/resource_co...
content/browser/resource_context.h:79: const
DownloadManager::GetNextIdThunkType& next_download_id_thunk() const;
On 2011/08/24 16:35:36, John Abd-El-Malek wrote:
> On 2011/08/24 14:33:00, b s h wrote:
> > On 2011/08/23 17:54:19, John Abd-El-Malek wrote:
> > > it seems odd to add a thunk to ResourceContext. it feels very C-ish to add
a
> > > function pointer there. can we have an interface instead?
> > > 
> > > willchan (who wrote ResourceContext): I realize you already looked at
this,
> > but
> > > I'm curious wdyt?
> > 
> > Yes, we are planning on promoting the thunk to a first-class, um, class in
my
> > next CL. TODone.
> 
> I'm curious, why check in the thunk code if you're planning on changing it in
> the next cl? i.e. why not just add a class now?

Initializing the DownloadIdFactory will be a very different story than
initializing this thunk, and implementing that story in this CL would make it
larger than I can handle. I'll probably pass the DownloadIdFactory around as a
pointer instead of a const ref, so I'll need to change this file then anyway.

[jam, 2011-08-24 17:30:32]

On 2011/08/24 17:22:36, b s h wrote:
>
http://codereview.chromium.org/7237034/diff/86061/content/browser/resource_co...
> File content/browser/resource_context.h (right):
> 
>
http://codereview.chromium.org/7237034/diff/86061/content/browser/resource_co...
> content/browser/resource_context.h:79: const
> DownloadManager::GetNextIdThunkType& next_download_id_thunk() const;
> On 2011/08/24 16:35:36, John Abd-El-Malek wrote:
> > On 2011/08/24 14:33:00, b s h wrote:
> > > On 2011/08/23 17:54:19, John Abd-El-Malek wrote:
> > > > it seems odd to add a thunk to ResourceContext. it feels very C-ish to
add
> a
> > > > function pointer there. can we have an interface instead?
> > > > 
> > > > willchan (who wrote ResourceContext): I realize you already looked at
> this,
> > > but
> > > > I'm curious wdyt?
> > > 
> > > Yes, we are planning on promoting the thunk to a first-class, um, class in
> my
> > > next CL. TODone.
> > 
> > I'm curious, why check in the thunk code if you're planning on changing it
in
> > the next cl? i.e. why not just add a class now?
> 
> Initializing the DownloadIdFactory will be a very different story than
> initializing this thunk, and implementing that story in this CL would make it
> larger than I can handle. I'll probably pass the DownloadIdFactory around as a
> pointer instead of a const ref, so I'll need to change this file then anyway.

ok, lgtm since it's just temporary

[Miranda Callahan, 2011-08-26 17:59:58]

On 2011/08/24 17:30:32, John Abd-El-Malek wrote:
> On 2011/08/24 17:22:36, b s h wrote:
> >
>
http://codereview.chromium.org/7237034/diff/86061/content/browser/resource_co...
> > File content/browser/resource_context.h (right):
> > 
> >
>
http://codereview.chromium.org/7237034/diff/86061/content/browser/resource_co...
> > content/browser/resource_context.h:79: const
> > DownloadManager::GetNextIdThunkType& next_download_id_thunk() const;
> > On 2011/08/24 16:35:36, John Abd-El-Malek wrote:
> > > On 2011/08/24 14:33:00, b s h wrote:
> > > > On 2011/08/23 17:54:19, John Abd-El-Malek wrote:
> > > > > it seems odd to add a thunk to ResourceContext. it feels very C-ish to
> add
> > a
> > > > > function pointer there. can we have an interface instead?
> > > > > 
> > > > > willchan (who wrote ResourceContext): I realize you already looked at
> > this,
> > > > but
> > > > > I'm curious wdyt?
> > > > 
> > > > Yes, we are planning on promoting the thunk to a first-class, um, class
in
> > my
> > > > next CL. TODone.
> > > 
> > > I'm curious, why check in the thunk code if you're planning on changing it
> in
> > > the next cl? i.e. why not just add a class now?
> > 
> > Initializing the DownloadIdFactory will be a very different story than
> > initializing this thunk, and implementing that story in this CL would make
it
> > larger than I can handle. I'll probably pass the DownloadIdFactory around as
a
> > pointer instead of a const ref, so I'll need to change this file then
anyway.
> 
> ok, lgtm since it's just temporary

Hi, Ben -- unfortunately, this is not my area of expertise, and I don't feel
comfortable reviewing. Perhaps try mpcomplete?

[Miranda Callahan, 2011-08-26 18:07:14]

On 2011/08/26 17:59:58, Miranda Callahan wrote:
> On 2011/08/24 17:30:32, John Abd-El-Malek wrote:
> > On 2011/08/24 17:22:36, b s h wrote:
> > >
> >
>
http://codereview.chromium.org/7237034/diff/86061/content/browser/resource_co...
> > > File content/browser/resource_context.h (right):
> > > 
> > >
> >
>
http://codereview.chromium.org/7237034/diff/86061/content/browser/resource_co...
> > > content/browser/resource_context.h:79: const
> > > DownloadManager::GetNextIdThunkType& next_download_id_thunk() const;
> > > On 2011/08/24 16:35:36, John Abd-El-Malek wrote:
> > > > On 2011/08/24 14:33:00, b s h wrote:
> > > > > On 2011/08/23 17:54:19, John Abd-El-Malek wrote:
> > > > > > it seems odd to add a thunk to ResourceContext. it feels very C-ish
to
> > add
> > > a
> > > > > > function pointer there. can we have an interface instead?
> > > > > > 
> > > > > > willchan (who wrote ResourceContext): I realize you already looked
at
> > > this,
> > > > > but
> > > > > > I'm curious wdyt?
> > > > > 
> > > > > Yes, we are planning on promoting the thunk to a first-class, um,
class
> in
> > > my
> > > > > next CL. TODone.
> > > > 
> > > > I'm curious, why check in the thunk code if you're planning on changing
it
> > in
> > > > the next cl? i.e. why not just add a class now?
> > > 
> > > Initializing the DownloadIdFactory will be a very different story than
> > > initializing this thunk, and implementing that story in this CL would make
> it
> > > larger than I can handle. I'll probably pass the DownloadIdFactory around
as
> a
> > > pointer instead of a const ref, so I'll need to change this file then
> anyway.
> > 
> > ok, lgtm since it's just temporary
> 
> Hi, Ben -- unfortunately, this is not my area of expertise, and I don't feel
> comfortable reviewing. Perhaps try mpcomplete?

After discussion with Ben and reviewing willchan's shepherding of this change, I
am comfortable giving an LGTM for the changes in the Profile directory as an
OWNER. So: LGTM!

[commit-bot: I haz the power, 2011-08-27 00:06:34]

Can't apply patch for file content/browser/download/download_manager.cc.
While running patch -p1 --forward --force;
patching file content/browser/download/download_manager.cc
Hunk #2 succeeded at 54 with fuzz 1.
Hunk #3 succeeded at 182 (offset 1 line).
Hunk #4 succeeded at 357 (offset 3 lines).
Hunk #5 succeeded at 526 (offset 3 lines).
Hunk #6 succeeded at 600 with fuzz 2 (offset 37 lines).
Hunk #7 FAILED at 598.
1 out of 7 hunks FAILED -- saving rejects to file
content/browser/download/download_manager.cc.rej

[commit-bot: I haz the power, 2011-08-29 18:03:22]

Change committed as 98656