DownloadManager::Observer::OnDownloadCreated

DownloadManager::Observer::OnDownloadCreated
Currently, it is fired when the DownloadItem is created, not when it has its filename, not when it's persisted. Listeners should observe the item for those kinds of state changes.
Those Observers that care about the filename (any of the filenames) should check whether it's empty() before using it. The DownloadItem[Impl] will UpdateObservers when the filename changes.
OnDownloadCreated is not fired for SavePackage downloads. It will be fired when we are comfortable with the user interacting with them, which might not happen until after the SavePackage integration.
ExtensionDownloadsEventRouter filters out temporary downloads, which are downloads whose DownloadCreateInfo.save_info.file_path is not empty(). GData also calls SetIsTemporary after OnDownloadCreated is fired, but only for SavePackage downloads, so EDER is safe there.

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

[benjhayden, 2012-07-13 20:15:51]

PTAL

[Randy Smith (Not in Mondays), 2012-07-14 19:39:13]

Two high level comments:
* Don't forget to put some tests for these notifications in the DM unit tests.

* I'd very much like the APIs we export to be internally consistent.  So given
that we now have a way to get visibility into downloads earlier in the process
than we did before, I'd like to have observer notifications on DownloadItems for
state changes at those points in the process, specifically on name
determination.  Could you put that in?

http://codereview.chromium.org/10735089/diff/6004/content/browser/download/do...
File content/browser/download/download_manager_impl.cc (right):

http://codereview.chromium.org/10735089/diff/6004/content/browser/download/do...
content/browser/download/download_manager_impl.cc:968:
FOR_EACH_OBSERVER(Observer, observers_, OnDownloadCreated(this, download));
I'm a little uncomfortable with this, because the history can be so large, and
pretty much arbitrary magic can be triggered for each notification.  How would
you feel about changing the notification to be for a vector (usually singular)
of downloads?  It's not as clean and simple and interface, but I think it could
easily save some pain.

http://codereview.chromium.org/10735089/diff/6004/content/public/browser/down...
File content/public/browser/download_manager.h (right):

http://codereview.chromium.org/10735089/diff/6004/content/public/browser/down...
content/public/browser/download_manager.h:84: // it may be a SavePage download,
it may or may not already be persisted.
I'd feel better about this if we could make guarantees about all the states it
could be in.  I'm happy with that list being long, including things that haven't
been visible before, but "no guarantees" is a very hard position to put
consumers in--if they actually trust the comment, they basically can't use the
call.  Which means we're encouraging them to make assumptions we're not
documenting, which I don't like.

Specifically, I'd like some documentation somewhere (huddle with Asanka?) about
what the "pre-name determination" state looks like and how you can tell it from
the post-name determination state.

http://codereview.chromium.org/10735089/diff/6004/content/public/browser/down...
content/public/browser/download_manager.h:86: DownloadManager* manager,
DownloadItem* item) {}
What are your thoughts about a paired OnDownloadRemoved() notification?  If
we're aiming to eventually deprecate ModelChanged(), I think we need to have
such a notification, but I'm not sure what it should look like.  If a consumer
wants details around the download itself, they should observe that download, so
maybe just the id?  Or id list, so that we can do it during shutdown without
causing pain?

[benjhayden, 2012-07-18 21:28:31]

PTAL

http://codereview.chromium.org/10735089/diff/6004/content/browser/download/do...
File content/browser/download/download_manager_impl.cc (right):

http://codereview.chromium.org/10735089/diff/6004/content/browser/download/do...
content/browser/download/download_manager_impl.cc:968:
FOR_EACH_OBSERVER(Observer, observers_, OnDownloadCreated(this, download));
On 2012/07/14 19:39:13, rdsmith wrote:
> I'm a little uncomfortable with this, because the history can be so large, and
> pretty much arbitrary magic can be triggered for each notification.  How would
> you feel about changing the notification to be for a vector (usually singular)
> of downloads?  It's not as clean and simple and interface, but I think it
could
> easily save some pain.

Discussed offline.

http://codereview.chromium.org/10735089/diff/6004/content/public/browser/down...
File content/public/browser/download_manager.h (right):

http://codereview.chromium.org/10735089/diff/6004/content/public/browser/down...
content/public/browser/download_manager.h:84: // it may be a SavePage download,
it may or may not already be persisted.
On 2012/07/14 19:39:13, rdsmith wrote:
> I'd feel better about this if we could make guarantees about all the states it
> could be in.  I'm happy with that list being long, including things that
haven't
> been visible before, but "no guarantees" is a very hard position to put
> consumers in--if they actually trust the comment, they basically can't use the
> call.  Which means we're encouraging them to make assumptions we're not
> documenting, which I don't like.

Updated the comment to clarify which features are guaranteed.

> 
> Specifically, I'd like some documentation somewhere (huddle with Asanka?)
about
> what the "pre-name determination" state looks like and how you can tell it
from
> the post-name determination state.  

I updated the CL description and this comment.

http://codereview.chromium.org/10735089/diff/6004/content/public/browser/down...
content/public/browser/download_manager.h:86: DownloadManager* manager,
DownloadItem* item) {}
On 2012/07/14 19:39:13, rdsmith wrote:
> What are your thoughts about a paired OnDownloadRemoved() notification?  If
> we're aiming to eventually deprecate ModelChanged(), I think we need to have
> such a notification, but I'm not sure what it should look like.  If a consumer
> wants details around the download itself, they should observe that download,
so
> maybe just the id?  Or id list, so that we can do it during shutdown without
> causing pain?

http://codereview.chromium.org/10704026/
Hanging another OnDownloadRemoved on DownloadManager seems unnecessary at this
point in time. I can understand that there might be some observers that only
care about OnDownloadRemoved and don't care about all the other
DownloadItem::Observer events, and I'm open to adding
DownloadManager::Observer::OnDownloadRemoved when we find them if it seems like
it would be prohibitively difficult for those observers to also observe the
download items themselves. YAGNI?

[Randy Smith (Not in Mondays), 2012-07-19 17:25:36]

I'm uncomfortable with this CL in that we now have two different states on the
DownloadItem that consumers can't tell the difference between: File name
determined, and File name determined *and* the user has given their input
through prompting if it's required.  I believe that's going to disappear when
Asanka commits FND III, and I'm ok with the code being fuzzy for that short
window, but I'd like an ok from Asanka that he agrees that his CL will resolve
this issue before I got the OK for this CL to go in.  Asanka?

http://codereview.chromium.org/10735089/diff/19011/chrome/browser/extensions/...
File chrome/browser/extensions/api/downloads/downloads_api.cc (right):

http://codereview.chromium.org/10735089/diff/19011/chrome/browser/extensions/...
chrome/browser/extensions/api/downloads/downloads_api.cc:897: //
TODO(benjhayden): Filter out temporary downloads? save pages?
I'd vote yes to both.  Long term, we want to make save page downloads visible,
but I suspect it would be pretty fragile right now.  I'm inclined to believe we
don't want to make temporary downloads visible, though we can revisit that after
we've evaluated the temporary download consumer requirements.

http://codereview.chromium.org/10735089/diff/19011/content/browser/download/d...
File content/browser/download/download_item_impl.cc (right):

http://codereview.chromium.org/10735089/diff/19011/content/browser/download/d...
content/browser/download/download_item_impl.cc:756: UpdateObservers();
I don't think this is necessary--the notification when we transition to COMPLETE
is going to happen almost immediately, and is almost always what we want
consumers to pay attention to.

http://codereview.chromium.org/10735089/diff/19011/content/browser/download/d...
content/browser/download/download_item_impl.cc:879: // SetDangerType() may call
UpdateObservers() again.
This seems like avoidable redundancy; would it make sense to hoist the
UpdateObservers() call outside of SetDangerType()?  How often is SetDangerType()
called?

http://codereview.chromium.org/10735089/diff/19011/content/browser/download/d...
content/browser/download/download_item_impl.cc:887: UpdateObservers();
This also seems redundant, especially since when Asanka's done with FNDR III,
we'll no longer have the option for two UpdateObservers() calls.  Can we just
call this once once we're comfortable we have the final filename?

http://codereview.chromium.org/10735089/diff/19011/content/browser/download/d...
File content/browser/download/download_manager_impl.cc (right):

http://codereview.chromium.org/10735089/diff/19011/content/browser/download/d...
content/browser/download/download_manager_impl.cc:566: 
small nit: It seems like whether or not there's vertical whitespace should be
consistent at the different locations at which we're notifying
OnDownloadCreated?

[Randy Smith (Not in Mondays), 2012-07-19 17:27:44]

On 2012/07/19 17:25:36, rdsmith wrote:
> I'm uncomfortable with this CL in that we now have two different states on the
> DownloadItem that consumers can't tell the difference between: File name
> determined, and File name determined *and* the user has given their input
> through prompting if it's required.  I believe that's going to disappear when
> Asanka commits FND III, and I'm ok with the code being fuzzy for that short
> window, but I'd like an ok from Asanka that he agrees that his CL will resolve
> this issue before I got the OK for this CL to go in.  Asanka?
> 
>
http://codereview.chromium.org/10735089/diff/19011/chrome/browser/extensions/...
> File chrome/browser/extensions/api/downloads/downloads_api.cc (right):
> 
>
http://codereview.chromium.org/10735089/diff/19011/chrome/browser/extensions/...
> chrome/browser/extensions/api/downloads/downloads_api.cc:897: //
> TODO(benjhayden): Filter out temporary downloads? save pages?
> I'd vote yes to both.  Long term, we want to make save page downloads visible,
> but I suspect it would be pretty fragile right now.  I'm inclined to believe
we
> don't want to make temporary downloads visible, though we can revisit that
after
> we've evaluated the temporary download consumer requirements.
> 
>
http://codereview.chromium.org/10735089/diff/19011/content/browser/download/d...
> File content/browser/download/download_item_impl.cc (right):
> 
>
http://codereview.chromium.org/10735089/diff/19011/content/browser/download/d...
> content/browser/download/download_item_impl.cc:756: UpdateObservers();
> I don't think this is necessary--the notification when we transition to
COMPLETE
> is going to happen almost immediately, and is almost always what we want
> consumers to pay attention to.

Another way of thinking about this: We need to have the DownloadItem be in a
self-consistent, probe-able, defined state at two classes of times: a) Whenever
we don't have a downloads-internal task running on the UI thread, and b)
whenever we call UpdateObservers() (because it can run arbitrary code which may
probe back at the DI).  So we want to choose the places where we call
UpdateObservers() with care, and make sure we believe the DI is in a clean state
when we do it.

> 
>
http://codereview.chromium.org/10735089/diff/19011/content/browser/download/d...
> content/browser/download/download_item_impl.cc:879: // SetDangerType() may
call
> UpdateObservers() again.
> This seems like avoidable redundancy; would it make sense to hoist the
> UpdateObservers() call outside of SetDangerType()?  How often is
SetDangerType()
> called?
> 
>
http://codereview.chromium.org/10735089/diff/19011/content/browser/download/d...
> content/browser/download/download_item_impl.cc:887: UpdateObservers();
> This also seems redundant, especially since when Asanka's done with FNDR III,
> we'll no longer have the option for two UpdateObservers() calls.  Can we just
> call this once once we're comfortable we have the final filename?
> 
>
http://codereview.chromium.org/10735089/diff/19011/content/browser/download/d...
> File content/browser/download/download_manager_impl.cc (right):
> 
>
http://codereview.chromium.org/10735089/diff/19011/content/browser/download/d...
> content/browser/download/download_manager_impl.cc:566: 
> small nit: It seems like whether or not there's vertical whitespace should be
> consistent at the different locations at which we're notifying
> OnDownloadCreated?

[asanka, 2012-07-19 18:14:07]

http://codereview.chromium.org/10735089/diff/19011/content/browser/download/d...
File content/browser/download/download_item_impl.cc (right):

http://codereview.chromium.org/10735089/diff/19011/content/browser/download/d...
content/browser/download/download_item_impl.cc:879: // SetDangerType() may call
UpdateObservers() again.
On 2012/07/19 17:25:36, rdsmith wrote:
> This seems like avoidable redundancy; would it make sense to hoist the
> UpdateObservers() call outside of SetDangerType()?  How often is
SetDangerType()
> called?

+1 to avoiding an additional UpdateObservers() call. Also, I think we shouldn't
call UpdateObservers() before seeing the final filename. I.e., avoid calling
UpdateObservers() at all here.

The UpdateObservers() call resulting from SetDangerType() wasn't an issue until
now since no-one outside of downloads/ would have seen DownloadItem yet. But
this assumption is now no longer true.

http://codereview.chromium.org/10735089/diff/19011/content/browser/download/d...
content/browser/download/download_item_impl.cc:887: UpdateObservers();
On 2012/07/19 17:25:36, rdsmith wrote:
> This also seems redundant, especially since when Asanka's done with FNDR III,
> we'll no longer have the option for two UpdateObservers() calls.  Can we just
> call this once once we're comfortable we have the final filename?

I think the UpdateObservers() call in OnDownloadRenamedToIntermediateName()
captures the state where:
- GetTargetFilePath() is the final target file path.
- GetFullPath() is the intermediate (current) file path.

http://codereview.chromium.org/10735089/diff/19011/content/browser/download/d...
content/browser/download/download_item_impl.cc:923: UpdateObservers();
Same as above. This is an intermediate state that I think we  don't want to
announce.

[Randy Smith (Not in Mondays), 2012-07-19 18:16:51]

http://codereview.chromium.org/10735089/diff/19011/content/browser/download/d...
File content/browser/download/download_item_impl.cc (right):

http://codereview.chromium.org/10735089/diff/19011/content/browser/download/d...
content/browser/download/download_item_impl.cc:879: // SetDangerType() may call
UpdateObservers() again.
On 2012/07/19 18:14:07, asanka wrote:
> On 2012/07/19 17:25:36, rdsmith wrote:
> > This seems like avoidable redundancy; would it make sense to hoist the
> > UpdateObservers() call outside of SetDangerType()?  How often is
> SetDangerType()
> > called?
> 
> +1 to avoiding an additional UpdateObservers() call. Also, I think we
shouldn't
> call UpdateObservers() before seeing the final filename. I.e., avoid calling
> UpdateObservers() at all here.
> 
> The UpdateObservers() call resulting from SetDangerType() wasn't an issue
until
> now since no-one outside of downloads/ would have seen DownloadItem yet. But
> this assumption is now no longer true.

I'm thinking this means we should hoist the UpdateObserver() call out of
SetDangerType() (and probably be careful going forward about where we put
UpdateObserver() calls--it's not something I think has received a lot of
attention previously.)

[benjhayden, 2012-07-23 15:58:42]

PTAL

http://codereview.chromium.org/10735089/diff/19011/content/browser/download/d...
File content/browser/download/download_item_impl.cc (right):

http://codereview.chromium.org/10735089/diff/19011/content/browser/download/d...
content/browser/download/download_item_impl.cc:756: UpdateObservers();
On 2012/07/19 17:25:36, rdsmith wrote:
> I don't think this is necessary--the notification when we transition to
COMPLETE
> is going to happen almost immediately, and is almost always what we want
> consumers to pay attention to.

Done.

http://codereview.chromium.org/10735089/diff/19011/content/browser/download/d...
content/browser/download/download_item_impl.cc:879: // SetDangerType() may call
UpdateObservers() again.
On 2012/07/19 17:25:36, rdsmith wrote:
> This seems like avoidable redundancy; would it make sense to hoist the
> UpdateObservers() call outside of SetDangerType()?  How often is
SetDangerType()
> called?

Pending discussion with Asanka re FND3.

http://codereview.chromium.org/10735089/diff/19011/content/browser/download/d...
content/browser/download/download_item_impl.cc:887: UpdateObservers();
On 2012/07/19 17:25:36, rdsmith wrote:
> This also seems redundant, especially since when Asanka's done with FNDR III,
> we'll no longer have the option for two UpdateObservers() calls.  Can we just
> call this once once we're comfortable we have the final filename?

Pending discussion with Asanka re FND3.

http://codereview.chromium.org/10735089/diff/19011/content/browser/download/d...
content/browser/download/download_item_impl.cc:923: UpdateObservers();
On 2012/07/19 18:14:07, asanka wrote:
> Same as above. This is an intermediate state that I think we  don't want to
> announce.

Done.

http://codereview.chromium.org/10735089/diff/19011/content/browser/download/d...
File content/browser/download/download_manager_impl.cc (right):

http://codereview.chromium.org/10735089/diff/19011/content/browser/download/d...
content/browser/download/download_manager_impl.cc:566: 
On 2012/07/19 17:25:36, rdsmith wrote:
> small nit: It seems like whether or not there's vertical whitespace should be
> consistent at the different locations at which we're notifying
> OnDownloadCreated?

I think that whitespace is more context-dependent than that, but it doesn't
matter.

[Randy Smith (Not in Mondays), 2012-07-23 18:40:50]

LGTM modulo nits below and agreement with Asanka about UpdateObservers
modification in FDR III.

http://codereview.chromium.org/10735089/diff/33001/chrome/browser/extensions/...
File chrome/browser/extensions/api/downloads/downloads_api_unittest.cc (right):

http://codereview.chromium.org/10735089/diff/33001/chrome/browser/extensions/...
chrome/browser/extensions/api/downloads/downloads_api_unittest.cc:171: LOG(INFO)
<< "occam caught " << new_event->Debug();
Was this intended to be in the CL proposed for landing?  Without some specific
reason, I think LOG(INFO)s are a bad idea.

http://codereview.chromium.org/10735089/diff/33001/chrome/browser/extensions/...
chrome/browser/extensions/api/downloads/downloads_api_unittest.cc:194: LOG(INFO)
<< "occam waiting " << waiting_for_->Debug();
Ditto.

http://codereview.chromium.org/10735089/diff/33001/content/public/browser/dow...
File content/public/browser/download_manager.h (right):

http://codereview.chromium.org/10735089/diff/33001/content/public/browser/dow...
content/public/browser/download_manager.h:91: // of times at once.
Can you shorten this a bit?  I think we're ok just warning that this may make
visible an item before it's file name has been determined, and telling the
consumer how to figure out if it's in that state.  (+ the final warning about
history loading.)

[benjhayden, 2012-07-23 18:54:22]

Randy, not quite done with you yet.

Asanka, PTAL.

http://codereview.chromium.org/10735089/diff/33001/chrome/browser/extensions/...
File chrome/browser/extensions/api/downloads/downloads_api_unittest.cc (right):

http://codereview.chromium.org/10735089/diff/33001/chrome/browser/extensions/...
chrome/browser/extensions/api/downloads/downloads_api_unittest.cc:171: LOG(INFO)
<< "occam caught " << new_event->Debug();
On 2012/07/23 18:40:50, rdsmith wrote:
> Was this intended to be in the CL proposed for landing?  Without some specific
> reason, I think LOG(INFO)s are a bad idea.

Sorry, was having trouble repro-ing trybot failures and forgot to remove them.

http://codereview.chromium.org/10735089/diff/33001/chrome/browser/extensions/...
chrome/browser/extensions/api/downloads/downloads_api_unittest.cc:194: LOG(INFO)
<< "occam waiting " << waiting_for_->Debug();
On 2012/07/23 18:40:50, rdsmith wrote:
> Ditto.

Done.

http://codereview.chromium.org/10735089/diff/33001/content/public/browser/dow...
File content/public/browser/download_manager.h (right):

http://codereview.chromium.org/10735089/diff/33001/content/public/browser/dow...
content/public/browser/download_manager.h:91: // of times at once.
On 2012/07/23 18:40:50, rdsmith wrote:
> Can you shorten this a bit?  I think we're ok just warning that this may make
> visible an item before it's file name has been determined, and telling the
> consumer how to figure out if it's in that state.  (+ the final warning about
> history loading.)

Please just give me a string to copy-paste into here.

[Randy Smith (Not in Mondays), 2012-07-23 19:00:06]

http://codereview.chromium.org/10735089/diff/33001/content/public/browser/dow...
File content/public/browser/download_manager.h (right):

http://codereview.chromium.org/10735089/diff/33001/content/public/browser/dow...
content/public/browser/download_manager.h:91: // of times at once.
On 2012/07/23 18:54:22, benjhayden_chromium wrote:
> On 2012/07/23 18:40:50, rdsmith wrote:
> > Can you shorten this a bit?  I think we're ok just warning that this may
make
> > visible an item before it's file name has been determined, and telling the
> > consumer how to figure out if it's in that state.  (+ the final warning
about
> > history loading.)
> 
> Please just give me a string to copy-paste into here.

Sure, though please confirm with Asanka that what I'm writing is accurate:

--
A DownloadItem was created.  Unlike ModelChanged, this item may be visible
before the filename is determined; in this case the return value of
GetTargetFileName() will be null.  This method may be called an arbitrary number
of times, e.g. when loading history on startup.  As a result, consumers should
avoid doing large amounts of work in OnDownloadCreated().
TODO(<whoever>): When we've fully specified the possible states of the
DownloadItem in download_item.h and removed ModelChanged, we should remove the
caveat above.

[asanka, 2012-07-23 19:10:20]

http://codereview.chromium.org/10735089/diff/19011/content/browser/download/d...
File content/browser/download/download_item_impl.cc (right):

http://codereview.chromium.org/10735089/diff/19011/content/browser/download/d...
content/browser/download/download_item_impl.cc:879: // SetDangerType() may call
UpdateObservers() again.
On 2012/07/23 15:58:42, benjhayden_chromium wrote:
> On 2012/07/19 17:25:36, rdsmith wrote:
> > This seems like avoidable redundancy; would it make sense to hoist the
> > UpdateObservers() call outside of SetDangerType()?  How often is
> SetDangerType()
> > called?
> 
> Pending discussion with Asanka re FND3.

My suggestion is to pull the UpdateObservers() call out of SetDangerType() and
only call it in OnContentCheckCompleted().

http://codereview.chromium.org/10735089/diff/19011/content/browser/download/d...
content/browser/download/download_item_impl.cc:887: UpdateObservers();
On 2012/07/23 15:58:42, benjhayden_chromium wrote:
> On 2012/07/19 17:25:36, rdsmith wrote:
> > This also seems redundant, especially since when Asanka's done with FNDR
III,
> > we'll no longer have the option for two UpdateObservers() calls.  Can we
just
> > call this once once we're comfortable we have the final filename?
> 
> Pending discussion with Asanka re FND3.

As above, I think we shouldn't call UpdateObservers() here. We can instead rely
on the observers being notified in OnDownloadRenamedToIntermediateName().

[benjhayden, 2012-07-25 16:24:38]

Randy, Asanka, PTAL.

http://codereview.chromium.org/10735089/diff/19011/content/browser/download/d...
File content/browser/download/download_item_impl.cc (right):

http://codereview.chromium.org/10735089/diff/19011/content/browser/download/d...
content/browser/download/download_item_impl.cc:879: // SetDangerType() may call
UpdateObservers() again.
On 2012/07/23 19:10:21, asanka wrote:
> On 2012/07/23 15:58:42, benjhayden_chromium wrote:
> > On 2012/07/19 17:25:36, rdsmith wrote:
> > > This seems like avoidable redundancy; would it make sense to hoist the
> > > UpdateObservers() call outside of SetDangerType()?  How often is
> > SetDangerType()
> > > called?
> > 
> > Pending discussion with Asanka re FND3.
> 
> My suggestion is to pull the UpdateObservers() call out of SetDangerType() and
> only call it in OnContentCheckCompleted().

Done.

http://codereview.chromium.org/10735089/diff/19011/content/browser/download/d...
content/browser/download/download_item_impl.cc:887: UpdateObservers();
On 2012/07/23 19:10:21, asanka wrote:
> On 2012/07/23 15:58:42, benjhayden_chromium wrote:
> > On 2012/07/19 17:25:36, rdsmith wrote:
> > > This also seems redundant, especially since when Asanka's done with FNDR
> III,
> > > we'll no longer have the option for two UpdateObservers() calls.  Can we
> just
> > > call this once once we're comfortable we have the final filename?
> > 
> > Pending discussion with Asanka re FND3.
> 
> As above, I think we shouldn't call UpdateObservers() here. We can instead
rely
> on the observers being notified in OnDownloadRenamedToIntermediateName().

Done.

http://codereview.chromium.org/10735089/diff/33001/content/public/browser/dow...
File content/public/browser/download_manager.h (right):

http://codereview.chromium.org/10735089/diff/33001/content/public/browser/dow...
content/public/browser/download_manager.h:91: // of times at once.
On 2012/07/23 19:00:06, rdsmith wrote:
> On 2012/07/23 18:54:22, benjhayden_chromium wrote:
> > On 2012/07/23 18:40:50, rdsmith wrote:
> > > Can you shorten this a bit?  I think we're ok just warning that this may
> make
> > > visible an item before it's file name has been determined, and telling the
> > > consumer how to figure out if it's in that state.  (+ the final warning
> about
> > > history loading.)
> > 
> > Please just give me a string to copy-paste into here.
> 
> Sure, though please confirm with Asanka that what I'm writing is accurate:
> 
> --
> A DownloadItem was created.  Unlike ModelChanged, this item may be visible
> before the filename is determined; in this case the return value of
> GetTargetFileName() will be null.  This method may be called an arbitrary
number
> of times, e.g. when loading history on startup.  As a result, consumers should
> avoid doing large amounts of work in OnDownloadCreated().
> TODO(<whoever>): When we've fully specified the possible states of the
> DownloadItem in download_item.h and removed ModelChanged, we should remove the
> caveat above.
> 

Done.

[asanka, 2012-07-25 19:36:34]

http://codereview.chromium.org/10735089/diff/36001/chrome/browser/extensions/...
File chrome/browser/extensions/api/downloads/downloads_api.h (right):

http://codereview.chromium.org/10735089/diff/36001/chrome/browser/extensions/...
chrome/browser/extensions/api/downloads/downloads_api.h:222: virtual void
OnDownloadCreated(content::DownloadManager* manager,
Nit: // content::DownloadManager::Observer

http://codereview.chromium.org/10735089/diff/36001/chrome/browser/extensions/...
chrome/browser/extensions/api/downloads/downloads_api.h:225: virtual void
OnDownloadUpdated(content::DownloadItem* download) OVERRIDE;
Nit: // content::DownloadItem::Observer

http://codereview.chromium.org/10735089/diff/36001/content/browser/download/d...
File content/browser/download/download_item_impl.cc (right):

http://codereview.chromium.org/10735089/diff/36001/content/browser/download/d...
content/browser/download/download_item_impl.cc:876: // SetDangerType() may call
UpdateObservers() again.
Stale comment?

http://codereview.chromium.org/10735089/diff/36001/content/browser/download/d...
content/browser/download/download_item_impl.cc:889: // TODO(rdsmith): Change to
DCHECK after http://crbug.com/85408 resolved.
Nit: Remove this comment?

http://codereview.chromium.org/10735089/diff/36001/content/browser/download/d...
content/browser/download/download_item_impl.cc:893: UpdateObservers();
I was under the impression that we weren't going to do an UpdateObservers() here
since we do that immediately after the intermediate file is renamed. Is that not
the case?

[benjhayden, 2012-07-25 21:26:25]

PTAL

http://codereview.chromium.org/10735089/diff/36001/chrome/browser/extensions/...
File chrome/browser/extensions/api/downloads/downloads_api.h (right):

http://codereview.chromium.org/10735089/diff/36001/chrome/browser/extensions/...
chrome/browser/extensions/api/downloads/downloads_api.h:222: virtual void
OnDownloadCreated(content::DownloadManager* manager,
On 2012/07/25 19:36:35, asanka wrote:
> Nit: // content::DownloadManager::Observer

Done.

http://codereview.chromium.org/10735089/diff/36001/chrome/browser/extensions/...
chrome/browser/extensions/api/downloads/downloads_api.h:225: virtual void
OnDownloadUpdated(content::DownloadItem* download) OVERRIDE;
On 2012/07/25 19:36:35, asanka wrote:
> Nit: // content::DownloadItem::Observer

Done.

http://codereview.chromium.org/10735089/diff/36001/content/browser/download/d...
File content/browser/download/download_item_impl.cc (right):

http://codereview.chromium.org/10735089/diff/36001/content/browser/download/d...
content/browser/download/download_item_impl.cc:876: // SetDangerType() may call
UpdateObservers() again.
On 2012/07/25 19:36:35, asanka wrote:
> Stale comment?

Done.

http://codereview.chromium.org/10735089/diff/36001/content/browser/download/d...
content/browser/download/download_item_impl.cc:889: // TODO(rdsmith): Change to
DCHECK after http://crbug.com/85408 resolved.
On 2012/07/25 19:36:35, asanka wrote:
> Nit: Remove this comment?

Done.

http://codereview.chromium.org/10735089/diff/36001/content/browser/download/d...
content/browser/download/download_item_impl.cc:893: UpdateObservers();
On 2012/07/25 19:36:35, asanka wrote:
> I was under the impression that we weren't going to do an UpdateObservers()
here
> since we do that immediately after the intermediate file is renamed. Is that
not
> the case?

I am so far down the rabbit hole, I don't know anything anymore. I was just
following this:
http://codereview.chromium.org/10735089/diff/19011/content/browser/download/d...
asanka 2012/07/23 19:10:21
My suggestion is to pull the UpdateObservers() call out of SetDangerType() and
only call it in OnContentCheckCompleted().

Just let me know exactly where to move this call if you want it moved. I'll fix
the tests when we figure this out.

[asanka, 2012-07-25 21:56:03]

http://codereview.chromium.org/10735089/diff/36001/content/browser/download/d...
File content/browser/download/download_item_impl.cc (right):

http://codereview.chromium.org/10735089/diff/36001/content/browser/download/d...
content/browser/download/download_item_impl.cc:893: UpdateObservers();
On 2012/07/25 21:26:26, benjhayden_chromium wrote:
> On 2012/07/25 19:36:35, asanka wrote:
> > I was under the impression that we weren't going to do an UpdateObservers()
> here
> > since we do that immediately after the intermediate file is renamed. Is that
> not
> > the case?
> 
> I am so far down the rabbit hole, I don't know anything anymore. I was just
> following this:
>
http://codereview.chromium.org/10735089/diff/19011/content/browser/download/d...
> asanka 2012/07/23 19:10:21
> My suggestion is to pull the UpdateObservers() call out of SetDangerType() and
> only call it in OnContentCheckCompleted().
> 
> Just let me know exactly where to move this call if you want it moved. I'll
fix
> the tests when we figure this out.

Whoa. Apparently I clicked on the wrong UpdateObservers() call when entering the
comment. Sorry about that. I blame this on the broken coffee machine.

To recap:

- UpdateObservers() in OnContentCheckCompleted() is necessary since
SetDangerType() doesn't update observers anymore.

- UpdateObservers() in OnTargetPathSelected() is unnecessary since we end up
calling UpdateObservers() once the download has been renamed. The call in
OnDownloadRenamedToIntermediateName() is better because we have both
GetTargetFilePath() and GetFullPath() by then.

[benjhayden, 2012-07-26 15:03:42]

http://codereview.chromium.org/10735089/diff/36001/content/browser/download/d...
File content/browser/download/download_item_impl.cc (right):

http://codereview.chromium.org/10735089/diff/36001/content/browser/download/d...
content/browser/download/download_item_impl.cc:893: UpdateObservers();
On 2012/07/25 21:56:04, asanka wrote:
> On 2012/07/25 21:26:26, benjhayden_chromium wrote:
> > On 2012/07/25 19:36:35, asanka wrote:
> > > I was under the impression that we weren't going to do an
UpdateObservers()
> > here
> > > since we do that immediately after the intermediate file is renamed. Is
that
> > not
> > > the case?
> > 
> > I am so far down the rabbit hole, I don't know anything anymore. I was just
> > following this:
> >
>
http://codereview.chromium.org/10735089/diff/19011/content/browser/download/d...
> > asanka 2012/07/23 19:10:21
> > My suggestion is to pull the UpdateObservers() call out of SetDangerType()
and
> > only call it in OnContentCheckCompleted().
> > 
> > Just let me know exactly where to move this call if you want it moved. I'll
> fix
> > the tests when we figure this out.
> 
> Whoa. Apparently I clicked on the wrong UpdateObservers() call when entering
the
> comment. Sorry about that. I blame this on the broken coffee machine.
> 
> To recap:
> 
> - UpdateObservers() in OnContentCheckCompleted() is necessary since
> SetDangerType() doesn't update observers anymore.
> 
> - UpdateObservers() in OnTargetPathSelected() is unnecessary since we end up
> calling UpdateObservers() once the download has been renamed. The call in
> OnDownloadRenamedToIntermediateName() is better because we have both
> GetTargetFilePath() and GetFullPath() by then.

Thank you! That was very helpful. :-)
PTAL

[asanka, 2012-07-26 15:08:13]

LGTM

[benjhayden, 2012-07-26 15:21:15]

Randy, PTAL?

On 2012/07/26 15:08:13, asanka wrote:
> LGTM

Thanks!

[Randy Smith (Not in Mondays), 2012-07-26 15:40:29]

On 2012/07/26 15:21:15, benjhayden_chromium wrote:
> Randy, PTAL?

Does my previous LGTM not count?  If not, what should I be looking at
specifically?

(Probably won't be available online for rest of the day.)

> 
> On 2012/07/26 15:08:13, asanka wrote:
> > LGTM
> 
> Thanks!

[benjhayden, 2012-07-26 15:45:16]

On 2012/07/26 15:40:29, rdsmith wrote:
> On 2012/07/26 15:21:15, benjhayden_chromium wrote:
> > Randy, PTAL?
> 
> Does my previous LGTM not count?  If not, what should I be looking at
> specifically?

Anything you care to look at, for example, the changes since your original l-g.
I'll take that as a rubber-stamp and commit now.


> 
> (Probably won't be available online for rest of the day.)
> 
> > 
> > On 2012/07/26 15:08:13, asanka wrote:
> > > LGTM
> > 
> > Thanks!

[commit-bot: I haz the power, 2012-07-26 15:46:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benjhayden@chromium.org/10735089/38005

[commit-bot: I haz the power, 2012-07-26 17:26:42]

Change committed as 148576