Reland DownloadItem::Observer::OnDownloadDestroyed() replaces DownloadItem::REMOVING

Reland
DownloadItem::Observer::OnDownloadDestroyed() replaces DownloadItem::REMOVING
Changes from original CL only in downloads_api.*

Reviewers:
Ben Goodger: chrome/browser/ui/views/download/download_item_view.*, chrome/browser/plugin_installer.*, chrome/browser/automation/automation_provider.cc
rdsmith, asanka: all
Nico (thakis): chrome/browser/ui/cocoa/download/download_item_mac.*
Achuith: chrome/browser/chromeos/gdata/gdata_download_observer.*
Evan (estade): chrome/browser/ui/gtk/download/download_item_gtk.*, chrome/browser/ui/webui/downloads_dom_handler.*
Scott (sky): chrome/browser/history/history_unittest.cc
Aaron (aa): chrome/browser/extensions/webstore_installer.*

Add DownloadItem::Observer::OnDownloadRemoved() to signal when a download is being removed from history.
Make chrome.downloads.onErased trigger from DownloadItem::Observer::OnDownloadRemoved() so that extensions don't think that all downloads are being erased just because the browser is closing.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=149794
Reverted: http://src.chromium.org/viewvc/chrome?view=rev&revision=150086
http://crbug.com/140687

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

[benjhayden, 2012-06-29 17:18:46]

PTAL

[Randy Smith (Not in Mondays), 2012-06-29 19:34:54]

http://codereview.chromium.org/10704026/diff/4002/chrome/browser/download/dow...
File chrome/browser/download/download_item_model.cc (left):

http://codereview.chromium.org/10704026/diff/4002/chrome/browser/download/dow...
chrome/browser/download/download_item_model.cc:131: case DownloadItem::REMOVING:
Asanka, what protection is there in DownloadItemModel for not continuing to
observer DownloadItems after they've been destroyed?

http://codereview.chromium.org/10704026/diff/4002/content/browser/download/do...
File content/browser/download/download_item_impl.cc (left):

http://codereview.chromium.org/10704026/diff/4002/content/browser/download/do...
content/browser/download/download_item_impl.cc:323:
delegate_->AssertStateConsistent(this);
Why?  I could imagine us yanking out the queue state consistent assertions
sometime around now, but that seems like it should be part of a single CL, and
not done piecemeal.

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

http://codereview.chromium.org/10704026/diff/4002/content/browser/download/do...
content/browser/download/download_item_impl.cc:317: FOR_EACH_OBSERVER(Observer,
observers_, OnDownloadDestructed(this));
nit, suggestion: Destroyed.

http://codereview.chromium.org/10704026/diff/4002/content/browser/download/do...
content/browser/download/download_item_impl.cc:657: FOR_EACH_OBSERVER(Observer,
observers_, OnDownloadRemoved(this));
I'm torn about this.  This *does* seem like a state.  A very short lived state,
but a state that describes an actual transition in the state of the DownloadItem
in terms of how it's being seen by the consumer.  But I realize that it's also
somewhat similar to the REMOVING case (thus the "torn").  How strongly do you
feel about expressing this through a new API rather than through a new state?

[Randy Smith (Not in Mondays), 2012-06-29 19:35:24]

Whoops, one other high level request: Could you change the issue description not
to use abbreviations?  It's written for a much larger audience than just your
reviewers.

[benjhayden, 2012-07-10 16:58:57]

PTAL

http://codereview.chromium.org/10704026/diff/4002/content/browser/download/do...
File content/browser/download/download_item_impl.cc (left):

http://codereview.chromium.org/10704026/diff/4002/content/browser/download/do...
content/browser/download/download_item_impl.cc:323:
delegate_->AssertStateConsistent(this);
On 2012/06/29 19:34:54, rdsmith wrote:
> Why?  I could imagine us yanking out the queue state consistent assertions
> sometime around now, but that seems like it should be part of a single CL, and
> not done piecemeal.
> 

Done.

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

http://codereview.chromium.org/10704026/diff/4002/content/browser/download/do...
content/browser/download/download_item_impl.cc:317: FOR_EACH_OBSERVER(Observer,
observers_, OnDownloadDestructed(this));
On 2012/06/29 19:34:54, rdsmith wrote:
> nit, suggestion: Destroyed.

Done.

http://codereview.chromium.org/10704026/diff/4002/content/browser/download/do...
content/browser/download/download_item_impl.cc:657: FOR_EACH_OBSERVER(Observer,
observers_, OnDownloadRemoved(this));
On 2012/06/29 19:34:54, rdsmith wrote:
> I'm torn about this.  This *does* seem like a state.  A very short lived
state,
> but a state that describes an actual transition in the state of the
DownloadItem
> in terms of how it's being seen by the consumer.  But I realize that it's also
> somewhat similar to the REMOVING case (thus the "torn").  How strongly do you
> feel about expressing this through a new API rather than through a new state?

When we talked about this, we were talking about these odd things that you
called "feelings". Let's define "state" and "event" so that we can talk about
our "feelings" more clearly. Here're my ideas, please suggest alternatives or
refinements.

State: a period of time, typically lasting longer than a single MessageLoop flop
(loop? turn?), during which certain operations may or may not be performed,
bounded by externally-driven events

Event: an instantaneous change in a particular piece of state

I think we can agree that [destroyed] is a state. It is indefinite and so longer
than a flop. No operations can be performed on the download while it is
[destroyed]. It is bounded on the left by OnDownloadDestroyed, and on the right
by OnUniverseIcyDeath.

I don't think that there's an externally-driven event that transitions from
REMOVING to [destroyed]. That process appears to be automatic. Sure, I can
imagine a GHOST state, but that is not the same as the concept that you are
proposing for REMOVING. If and when we have need for a GHOST state, it will make
sense as a state.
In addition to being automatic, the transition from REMOVING to [destroyed]
seems to be effectively instantaneous, in contrast with the "longer than a
single flop" clause.
I am also unaware of any operations that may be performed on the download item
between OnDownloadRemoved and OnDownloadDestroyed. The operations that may be
performed during REMOVING seem to be the same operations that may be performed
while the download is [destroyed]. You can't even delete a REMOVING download,
because that happens automatically.

So it seems to me that REMOVING fails all of the criteria for a State, but I
fully expect to learn something new and surprising about how the downloads
system works that will change this analysis, or have shown to me a better
definition for State or Event.

[Randy Smith (Not in Mondays), 2012-07-10 18:52:37]

On 2012/07/10 16:58:57, benjhayden_chromium wrote:
> PTAL
> 
>
http://codereview.chromium.org/10704026/diff/4002/content/browser/download/do...
> File content/browser/download/download_item_impl.cc (left):
> 
>
http://codereview.chromium.org/10704026/diff/4002/content/browser/download/do...
> content/browser/download/download_item_impl.cc:323:
> delegate_->AssertStateConsistent(this);
> On 2012/06/29 19:34:54, rdsmith wrote:
> > Why?  I could imagine us yanking out the queue state consistent assertions
> > sometime around now, but that seems like it should be part of a single CL,
and
> > not done piecemeal.
> > 
> 
> Done.
> 
>
http://codereview.chromium.org/10704026/diff/4002/content/browser/download/do...
> File content/browser/download/download_item_impl.cc (right):
> 
>
http://codereview.chromium.org/10704026/diff/4002/content/browser/download/do...
> content/browser/download/download_item_impl.cc:317:
FOR_EACH_OBSERVER(Observer,
> observers_, OnDownloadDestructed(this));
> On 2012/06/29 19:34:54, rdsmith wrote:
> > nit, suggestion: Destroyed.
> 
> Done.
> 
>
http://codereview.chromium.org/10704026/diff/4002/content/browser/download/do...
> content/browser/download/download_item_impl.cc:657:
FOR_EACH_OBSERVER(Observer,
> observers_, OnDownloadRemoved(this));
> On 2012/06/29 19:34:54, rdsmith wrote:
> > I'm torn about this.  This *does* seem like a state.  A very short lived
> state,
> > but a state that describes an actual transition in the state of the
> DownloadItem
> > in terms of how it's being seen by the consumer.  But I realize that it's
also
> > somewhat similar to the REMOVING case (thus the "torn").  How strongly do
you
> > feel about expressing this through a new API rather than through a new
state?
> 
> When we talked about this, we were talking about these odd things that you
> called "feelings". Let's define "state" and "event" so that we can talk about
> our "feelings" more clearly. Here're my ideas, please suggest alternatives or
> refinements.
> 
> State: a period of time, typically lasting longer than a single MessageLoop
flop
> (loop? turn?), during which certain operations may or may not be performed,
> bounded by externally-driven events
> 
> Event: an instantaneous change in a particular piece of state
> 
> I think we can agree that [destroyed] is a state. It is indefinite and so
longer
> than a flop. No operations can be performed on the download while it is
> [destroyed]. It is bounded on the left by OnDownloadDestroyed, and on the
right
> by OnUniverseIcyDeath.
> 
> I don't think that there's an externally-driven event that transitions from
> REMOVING to [destroyed]. That process appears to be automatic. Sure, I can
> imagine a GHOST state, but that is not the same as the concept that you are
> proposing for REMOVING. If and when we have need for a GHOST state, it will
make
> sense as a state.
> In addition to being automatic, the transition from REMOVING to [destroyed]
> seems to be effectively instantaneous, in contrast with the "longer than a
> single flop" clause.
> I am also unaware of any operations that may be performed on the download item
> between OnDownloadRemoved and OnDownloadDestroyed. The operations that may be
> performed during REMOVING seem to be the same operations that may be performed
> while the download is [destroyed]. You can't even delete a REMOVING download,
> because that happens automatically.
> 
> So it seems to me that REMOVING fails all of the criteria for a State, but I
> fully expect to learn something new and surprising about how the downloads
> system works that will change this analysis, or have shown to me a better
> definition for State or Event.

Hmmm.  I like your definitions, and I think you've convinced me, but let me work
this out in writing.  

Event: Agreed (instantaneous change, or notification of instantaneous change).

State: In the abstract, I can quibble--I think of a state as more a
self-consistent place that an object might be in that will be visible to some
pieces of code.  Most states stick around for a while, but some states do
auto-transition to new states after those pieces of code have had a chance to
poke at them.  But to date we haven't had states like that in the download
system.

Notifications to observers are events, generally caused by transitions into a
new state.  But they are about the arc between one state and the next, more than
they are about the target state itself.  Still, I think the pattern we've had in
the code to date is that the target state is the one that is visible when
notified code probes the download item.  I think this is a key/important point.

The other point that is important (that's a bit orthogonal to your framework) is
the  layers of abstraction.  The download itself is an abstract entity, which is
concretely represented by a DownloadItem{,Impl} object.  I would claim that
"Removed" is a state on the abstract entity, and "Destroyed" a state on the
concrete one.  I would claim that the pattern we've had in the code to date is
that GetState() on the DownloadItem represents the state of the abstract entity,
not the concrete one.

So where does this leave us?  I think it gets me back to being torn.  What I
*want* is to notify the observers of the transition of the abstract entity to
the target state "Removed" the same way they've been notified of all the other
states.  But that means that when they probe at the DownloadItem, the download
needs to *be* in the Removed state.  But there's a relationship between that and
the Destroyed state on the actual object, and we can't let the observers probe
at *that*.  

So I think I'm back where I was.  If I were doing this, I'd probably make it a
state (and thank you for helping me to more thoroughly flesh out why I'm pulled
in that direction).  But that edges up close to the to-date invariant that
states stick around for a while, and the Removed (now that I've worked it
through, the state's clearly Removed, not Removing) state won't.  So I'm ok with
you having it as a separate notification.  But hopefully the above helps you
understand why I'm pulled towards keeping it a state.

I will disagree with one part of your analysis, though.  Our feelings are
concrete things, not abstract entities like "states" or "events".  As such, they
clearly shouldn't be described with double quotes.  (:-} :-}).

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

Could you add some tests for the new notifications in
download_item_impl_unittest.cc?

Didn't review download_path_reservation_tracker_unittest.cc; presuming Asanka
will do that.

http://codereview.chromium.org/10704026/diff/17003/chrome/browser/automation/...
File chrome/browser/automation/automation_provider.cc (left):

http://codereview.chromium.org/10704026/diff/17003/chrome/browser/automation/...
chrome/browser/automation/automation_provider.cc:307:
state_to_string[DownloadItem::REMOVING] = std::string("REMOVING");
Can you say a word or two about why you don't think the consumers (direct and
indirect) of GetDictionaryFromDownloadItem() don't need a notification about a
download going away?  (It may be completely safe, I'm just looking for
reassurance of due diligence.)

http://codereview.chromium.org/10704026/diff/17003/chrome/browser/download/do...
File chrome/browser/download/download_path_reservation_tracker.cc (right):

http://codereview.chromium.org/10704026/diff/17003/chrome/browser/download/do...
chrome/browser/download/download_path_reservation_tracker.cc:109: // INTERRUPTED
first.
NOTREACHED()?

http://codereview.chromium.org/10704026/diff/17003/chrome/browser/download/do...
File chrome/browser/download/download_util.cc (left):

http://codereview.chromium.org/10704026/diff/17003/chrome/browser/download/do...
chrome/browser/download/download_util.cc:518: file_value->SetString("state",
"REMOVING");
Same question as elsewhere: Have you checked the consumers of this function to
make sure they don't need this notification?

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

http://codereview.chromium.org/10704026/diff/17003/chrome/browser/extensions/...
chrome/browser/extensions/api/downloads/downloads_api.cc:953: return;
If we're getting notification and the download is being destroyed, we have to
get rid of our pointer to it, whether there's a profile around or not.

http://codereview.chromium.org/10704026/diff/17003/chrome/browser/plugin_inst...
File chrome/browser/plugin_installer.cc (right):

http://codereview.chromium.org/10704026/diff/17003/chrome/browser/plugin_inst...
chrome/browser/plugin_installer.cc:212: make_scoped_refptr(download_manager))));
*Not* relevant to this CL, but this code makes it look like it would be easy to
get rid of the call to RDH::BeginDownload() above if we wanted.

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

http://codereview.chromium.org/10704026/diff/17003/content/browser/download/d...
content/browser/download/download_manager_impl.cc:304:
downloads_to_delete.swap(downloads_);
Is there a reason for moving this line?  (I don't care, but I don't want to miss
something I should review.)

http://codereview.chromium.org/10704026/diff/17003/content/browser/download/d...
content/browser/download/download_manager_impl.cc:818: 
Why the shift?  This seems more complicated.  Simpler would be to just put a
"delete download" into the loop, above, and just delete everything at once.

http://codereview.chromium.org/10704026/diff/17003/content/browser/download/d...
File content/browser/download/drag_download_file.cc (right):

http://codereview.chromium.org/10704026/diff/17003/content/browser/download/d...
content/browser/download/drag_download_file.cc:213: void
DragDownloadFile::OnDownloadDestroyed(content::DownloadItem* download) {
NOTREACHED?

http://codereview.chromium.org/10704026/diff/17003/content/public/browser/dow...
File content/public/browser/download_item.h (right):

http://codereview.chromium.org/10704026/diff/17003/content/public/browser/dow...
content/public/browser/download_item.h:107: // Called when the user removed the
download either from UI or at shutdown.
This implies it's user action that removes the download at shutdown.  More, I
don't think we should remove the download at shutdown.  The abstract download
entity (as opposed to the DownloadItem) is *not* being removed; it'll still be
around when they start up the browser next.

http://codereview.chromium.org/10704026/diff/17003/content/public/browser/dow...
content/public/browser/download_item.h:112: // down.
These comments have slightly contradictory implications.  If OnDownloadRemoved()
occurs at shutdown, OnDownloadDestroyed() will always be called after
OnDownloadRemoved().

[asanka, 2012-07-11 18:12:19]

http://codereview.chromium.org/10704026/diff/4002/chrome/browser/download/dow...
File chrome/browser/download/download_item_model.cc (left):

http://codereview.chromium.org/10704026/diff/4002/chrome/browser/download/dow...
chrome/browser/download/download_item_model.cc:131: case DownloadItem::REMOVING:
On 2012/06/29 19:34:54, rdsmith wrote:
> Asanka, what protection is there in DownloadItemModel for not continuing to
> observer DownloadItems after they've been destroyed?

DownloadItemModel doesn't observe DownloadItems. It merely helps interpret the
current state of the DownloadItem. Currently it is the responsibility of the
owner of the download item model to make sure the model doesn't outlive the
download item.

http://codereview.chromium.org/10704026/diff/17003/chrome/browser/download/do...
File chrome/browser/download/download_path_reservation_tracker_unittest.cc
(left):

http://codereview.chromium.org/10704026/diff/17003/chrome/browser/download/do...
chrome/browser/download/download_path_reservation_tracker_unittest.cc:37:
SetState(REMOVING);
You are going to need a FOR_EACH_OBSERVER(.. OnDownloadDestroyed(this)) here.
Otherwise the tests will fail.

[benjhayden, 2012-07-13 20:03:17]

PTAL

http://codereview.chromium.org/10704026/diff/17003/chrome/browser/automation/...
File chrome/browser/automation/automation_provider.cc (left):

http://codereview.chromium.org/10704026/diff/17003/chrome/browser/automation/...
chrome/browser/automation/automation_provider.cc:307:
state_to_string[DownloadItem::REMOVING] = std::string("REMOVING");
On 2012/07/11 17:55:37, rdsmith wrote:
> Can you say a word or two about why you don't think the consumers (direct and
> indirect) of GetDictionaryFromDownloadItem() don't need a notification about a
> download going away?  (It may be completely safe, I'm just looking for
> reassurance of due diligence.)

grep -i REMOVING $(find . -iname '*.py'|grep download)
Looks like no pyauto tests rely on the REMOVING state. They'll continue to have
access to the DownloadManager::Observer::ModelChanged event.

http://codereview.chromium.org/10704026/diff/17003/chrome/browser/download/do...
File chrome/browser/download/download_path_reservation_tracker.cc (right):

http://codereview.chromium.org/10704026/diff/17003/chrome/browser/download/do...
chrome/browser/download/download_path_reservation_tracker.cc:109: // INTERRUPTED
first.
On 2012/07/11 17:55:37, rdsmith wrote:
> NOTREACHED()?

Maybe in another CL. I'd rather this CL not change functionality like that.

http://codereview.chromium.org/10704026/diff/17003/chrome/browser/download/do...
File chrome/browser/download/download_path_reservation_tracker_unittest.cc
(left):

http://codereview.chromium.org/10704026/diff/17003/chrome/browser/download/do...
chrome/browser/download/download_path_reservation_tracker_unittest.cc:37:
SetState(REMOVING);
On 2012/07/11 18:12:19, asanka wrote:
> You are going to need a FOR_EACH_OBSERVER(.. OnDownloadDestroyed(this)) here.
> Otherwise the tests will fail.

Done.

http://codereview.chromium.org/10704026/diff/17003/chrome/browser/download/do...
File chrome/browser/download/download_util.cc (left):

http://codereview.chromium.org/10704026/diff/17003/chrome/browser/download/do...
chrome/browser/download/download_util.cc:518: file_value->SetString("state",
"REMOVING");
On 2012/07/11 17:55:37, rdsmith wrote:
> Same question as elsewhere: Have you checked the consumers of this function to
> make sure they don't need this notification?

codesearch says that the only caller of CreateDownloadItemValue() is
chrome/browser/ui/webui/downloads_dom_handler.cc. downloads.js does not use the
REMOVING state. The |downloadsList| message is sent when downloads disappear
from ModelChanged().

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

http://codereview.chromium.org/10704026/diff/17003/chrome/browser/extensions/...
chrome/browser/extensions/api/downloads/downloads_api.cc:953: return;
On 2012/07/11 17:55:37, rdsmith wrote:
> If we're getting notification and the download is being destroyed, we have to
> get rid of our pointer to it, whether there's a profile around or not.

Done.

http://codereview.chromium.org/10704026/diff/17003/chrome/browser/plugin_inst...
File chrome/browser/plugin_installer.cc (right):

http://codereview.chromium.org/10704026/diff/17003/chrome/browser/plugin_inst...
chrome/browser/plugin_installer.cc:212: make_scoped_refptr(download_manager))));
On 2012/07/11 17:55:37, rdsmith wrote:
> *Not* relevant to this CL, but this code makes it look like it would be easy
to
> get rid of the call to RDH::BeginDownload() above if we wanted.

The more downloads code we rediscover, the more of it we want to fix. :-)

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

http://codereview.chromium.org/10704026/diff/17003/content/browser/download/d...
content/browser/download/download_manager_impl.cc:304:
downloads_to_delete.swap(downloads_);
On 2012/07/11 17:55:37, rdsmith wrote:
> Is there a reason for moving this line?  (I don't care, but I don't want to
miss
> something I should review.)

~DownloadItemImpl() calls AssertStateConsistent() which checks that the download
is still in |downloads_|.

http://codereview.chromium.org/10704026/diff/17003/content/browser/download/d...
content/browser/download/download_manager_impl.cc:818: 
On 2012/07/11 17:55:37, rdsmith wrote:
> Why the shift?  This seems more complicated.  Simpler would be to just put a
> "delete download" into the loop, above, and just delete everything at once.

Done.

http://codereview.chromium.org/10704026/diff/17003/content/browser/download/d...
File content/browser/download/drag_download_file.cc (right):

http://codereview.chromium.org/10704026/diff/17003/content/browser/download/d...
content/browser/download/drag_download_file.cc:213: void
DragDownloadFile::OnDownloadDestroyed(content::DownloadItem* download) {
On 2012/07/11 17:55:37, rdsmith wrote:
> NOTREACHED?

TODone. Again, I'd rather not change functionality like that in this CL.

http://codereview.chromium.org/10704026/diff/17003/content/public/browser/dow...
File content/public/browser/download_item.h (right):

http://codereview.chromium.org/10704026/diff/17003/content/public/browser/dow...
content/public/browser/download_item.h:107: // Called when the user removed the
download either from UI or at shutdown.
On 2012/07/11 17:55:37, rdsmith wrote:
> This implies it's user action that removes the download at shutdown.  More, I
> don't think we should remove the download at shutdown.  The abstract download
> entity (as opposed to the DownloadItem) is *not* being removed; it'll still be
> around when they start up the browser next.

Done.

http://codereview.chromium.org/10704026/diff/17003/content/public/browser/dow...
content/public/browser/download_item.h:112: // down.
On 2012/07/11 17:55:37, rdsmith wrote:
> These comments have slightly contradictory implications.  If
OnDownloadRemoved()
> occurs at shutdown, OnDownloadDestroyed() will always be called after
> OnDownloadRemoved().

Done.

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

I didn't see new tests for the new notifications in
download_item_impl_unittests.cc?

http://codereview.chromium.org/10704026/diff/17003/chrome/browser/automation/...
File chrome/browser/automation/automation_provider.cc (left):

http://codereview.chromium.org/10704026/diff/17003/chrome/browser/automation/...
chrome/browser/automation/automation_provider.cc:307:
state_to_string[DownloadItem::REMOVING] = std::string("REMOVING");
On 2012/07/13 20:03:17, benjhayden_chromium wrote:
> On 2012/07/11 17:55:37, rdsmith wrote:
> > Can you say a word or two about why you don't think the consumers (direct
and
> > indirect) of GetDictionaryFromDownloadItem() don't need a notification about
a
> > download going away?  (It may be completely safe, I'm just looking for
> > reassurance of due diligence.)
> 
> grep -i REMOVING $(find . -iname '*.py'|grep download)
> Looks like no pyauto tests rely on the REMOVING state. They'll continue to
have
> access to the DownloadManager::Observer::ModelChanged event.

And you're comfortable that the pyauto tests are the only AutomationProvider
customers?  (I think that's true; the other options are ui_tests and
interactive_ui_tests, which I believe don't have any download tests in them, but
it's worth checking.)

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

http://codereview.chromium.org/10704026/diff/10053/content/browser/download/d...
content/browser/download/download_item_impl.cc:661: FOR_EACH_OBSERVER(Observer,
observers_, OnDownloadRemoved(this));
What about the RemoveDownloadsBetween path from the DownloadManager?

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

http://codereview.chromium.org/10704026/diff/10053/content/browser/download/d...
content/browser/download/download_manager_impl.cc:321:
STLDeleteValues(&downloads_);
Shouldn't we also do a downloads_.clear() here?  We don't want the pointers
lying around.  (The previous "swap()" did this.)

http://codereview.chromium.org/10704026/diff/10053/content/public/browser/dow...
File content/public/browser/download_item.h (right):

http://codereview.chromium.org/10704026/diff/10053/content/public/browser/dow...
content/public/browser/download_item.h:107: // Called when the user removed the
download from history.
nit: The action is the user removing the download (the history isn't really
relevant), so I'd change this to "Called when the user removes a Download".

[benjhayden, 2012-07-23 15:43:09]

PTAL.

http://codereview.chromium.org/10704026/diff/17003/chrome/browser/automation/...
File chrome/browser/automation/automation_provider.cc (left):

http://codereview.chromium.org/10704026/diff/17003/chrome/browser/automation/...
chrome/browser/automation/automation_provider.cc:307:
state_to_string[DownloadItem::REMOVING] = std::string("REMOVING");
On 2012/07/14 19:25:53, rdsmith wrote:
> On 2012/07/13 20:03:17, benjhayden_chromium wrote:
> > On 2012/07/11 17:55:37, rdsmith wrote:
> > > Can you say a word or two about why you don't think the consumers (direct
> and
> > > indirect) of GetDictionaryFromDownloadItem() don't need a notification
about
> a
> > > download going away?  (It may be completely safe, I'm just looking for
> > > reassurance of due diligence.)
> > 
> > grep -i REMOVING $(find . -iname '*.py'|grep download)
> > Looks like no pyauto tests rely on the REMOVING state. They'll continue to
> have
> > access to the DownloadManager::Observer::ModelChanged event.
> 
> And you're comfortable that the pyauto tests are the only AutomationProvider
> customers?  (I think that's true; the other options are ui_tests and
> interactive_ui_tests, which I believe don't have any download tests in them,
but
> it's worth checking.)

ui_unittests and interactive_ui_tests pass on all bots for this cl.
If there are any automation scripts that rely on REMOVING, then they'll break in
a way that can be fixed easily, and we can add comments to this file about them
so they don't get lost again.

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

http://codereview.chromium.org/10704026/diff/10053/content/browser/download/d...
content/browser/download/download_item_impl.cc:661: FOR_EACH_OBSERVER(Observer,
observers_, OnDownloadRemoved(this));
On 2012/07/14 19:25:53, rdsmith wrote:
> What about the RemoveDownloadsBetween path from the DownloadManager?

What about it?

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

http://codereview.chromium.org/10704026/diff/10053/content/browser/download/d...
content/browser/download/download_manager_impl.cc:321:
STLDeleteValues(&downloads_);
On 2012/07/14 19:25:53, rdsmith wrote:
> Shouldn't we also do a downloads_.clear() here?  We don't want the pointers
> lying around.  (The previous "swap()" did this.)

Done.

http://codereview.chromium.org/10704026/diff/10053/content/public/browser/dow...
File content/public/browser/download_item.h (right):

http://codereview.chromium.org/10704026/diff/10053/content/public/browser/dow...
content/public/browser/download_item.h:107: // Called when the user removed the
download from history.
On 2012/07/14 19:25:53, rdsmith wrote:
> nit: The action is the user removing the download (the history isn't really
> relevant), so I'd change this to "Called when the user removes a Download".

Done.

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

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

http://codereview.chromium.org/10704026/diff/10053/content/browser/download/d...
content/browser/download/download_item_impl.cc:661: FOR_EACH_OBSERVER(Observer,
observers_, OnDownloadRemoved(this));
On 2012/07/23 15:43:09, benjhayden_chromium wrote:
> On 2012/07/14 19:25:53, rdsmith wrote:
> > What about the RemoveDownloadsBetween path from the DownloadManager?
> 
> What about it?

I don't believe it goes through the DII::Remove() path, and hence won't produce
an OnDownloadRemoved() notification.  Am I wrong?

[benjhayden, 2012-07-25 15:37:48]

I can kill DII::NotifyRemoved() with the DownloadHistoryUpdater, if that sounds
good to you.

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

http://codereview.chromium.org/10704026/diff/10053/content/browser/download/d...
content/browser/download/download_item_impl.cc:661: FOR_EACH_OBSERVER(Observer,
observers_, OnDownloadRemoved(this));
On 2012/07/23 18:30:42, rdsmith wrote:
> On 2012/07/23 15:43:09, benjhayden_chromium wrote:
> > On 2012/07/14 19:25:53, rdsmith wrote:
> > > What about the RemoveDownloadsBetween path from the DownloadManager?
> > 
> > What about it?
> 
> I don't believe it goes through the DII::Remove() path, and hence won't
produce
> an OnDownloadRemoved() notification.  Am I wrong?

Facepalm. PTAL.

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

LGTM.

[benjhayden, 2012-07-26 14:37:39]

Asanka, PTAL?

On 2012/07/26 13:45:40, rdsmith wrote:
> LGTM.

Thanks!

[asanka, 2012-07-26 15:30:53]

LGTM

http://codereview.chromium.org/10704026/diff/53001/chrome/browser/ui/views/do...
File chrome/browser/ui/views/download/download_item_view.h (right):

http://codereview.chromium.org/10704026/diff/53001/chrome/browser/ui/views/do...
chrome/browser/ui/views/download/download_item_view.h:78: // DownloadObserver
methods
Nit: DownloadItem::Observer

[benjhayden, 2012-07-26 15:54:05]

Thanks!
I'll hit the CQ when the bots come back.

http://codereview.chromium.org/10704026/diff/53001/chrome/browser/ui/views/do...
File chrome/browser/ui/views/download/download_item_view.h (right):

http://codereview.chromium.org/10704026/diff/53001/chrome/browser/ui/views/do...
chrome/browser/ui/views/download/download_item_view.h:78: // DownloadObserver
methods
On 2012/07/26 15:30:53, asanka wrote:
> Nit: DownloadItem::Observer

Done.

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

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

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

Presubmit check for 10704026-68002 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 files in these directories:
    chrome/browser/chromeos/gdata
    chrome/browser/automation
    chrome/browser/ui/cocoa/download
    chrome/browser/ui/gtk
    chrome/browser/extensions
    chrome
    chrome/browser/ui/webui
    chrome/browser/history
    chrome/browser/ui

Presubmit checks took 2.7s to calculate.

[benjhayden, 2012-07-26 17:30:51]

I'll ping all of you separately, but PTAL.


On 2012/07/26 17:06:14, I haz the power (commit-bot) wrote:
> Presubmit check for 10704026-68002 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 files in these directories:
>     chrome/browser/chromeos/gdata
>     chrome/browser/automation
>     chrome/browser/ui/cocoa/download
>     chrome/browser/ui/gtk
>     chrome/browser/extensions
>     chrome
>     chrome/browser/ui/webui
>     chrome/browser/history
>     chrome/browser/ui
> 
> Presubmit checks took 2.7s to calculate.

[Nico, 2012-07-26 17:32:09]

_mac lgtm

[Evan Stade, 2012-07-26 21:34:17]

lgtm

http://codereview.chromium.org/10704026/diff/68002/chrome/browser/ui/webui/do...
File chrome/browser/ui/webui/downloads_dom_handler.cc (right):

http://codereview.chromium.org/10704026/diff/68002/chrome/browser/ui/webui/do...
chrome/browser/ui/webui/downloads_dom_handler.cc:195: *it = NULL;
why are you nulling rather than erasing?

[achuithb, 2012-07-26 23:17:58]

gdata/* changes lgtm

[benjhayden, 2012-07-27 19:43:34]

http://codereview.chromium.org/10704026/diff/68002/chrome/browser/ui/webui/do...
File chrome/browser/ui/webui/downloads_dom_handler.cc (right):

http://codereview.chromium.org/10704026/diff/68002/chrome/browser/ui/webui/do...
chrome/browser/ui/webui/downloads_dom_handler.cc:195: *it = NULL;
On 2012/07/26 21:34:18, Evan Stade wrote:
> why are you nulling rather than erasing?

I presume in case we receive an event from the downloads page JS. Instead of
using the download's ids, this class uses their indices in download_items_ to
identify the items.
It's on our todo list. :-)

[sky, 2012-07-31 16:19:42]

LGTM

[Aaron Boodman, 2012-08-01 09:34:47]

LGTM

[Ben Goodger (Google), 2012-08-01 15:16:23]

lgtm for download_item_view*, plugin_installer* and automation_provider.cc

[commit-bot: I haz the power, 2012-08-01 20:29:49]

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

[commit-bot: I haz the power, 2012-08-01 23:12:32]

Try job failure for 10704026-65040 (retry) on win_rel for step "unit_tests".
It's a second try, previously, step "unit_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-08-02 23:41:14]

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

[commit-bot: I haz the power, 2012-08-03 03:07:52]

Change committed as 149794

[Randy Smith (Not in Mondays), 2012-08-08 20:24:10]

So just to confirm: The protection against the recent crashes seen in canary
from this CL is the state mapping already committed in the corruption fixer this
morning?  I.e. this is pretty much the same CL as you had before?

If my understanding is correct, LGTM.

[benjhayden, 2012-08-08 20:46:46]

Anybody else following along, please feel free to take another look now before I
reland. To recap, nothing changed in this CL; see also
http://codereview.chromium.org/10823203/

On 2012/08/08 20:24:10, rdsmith wrote:
> So just to confirm: The protection against the recent crashes seen in canary
> from this CL is the state mapping already committed in the corruption fixer
this
> morning?  I.e. this is pretty much the same CL as you had before?

Yup.
Plus the early-return in downloads_api.cc:DownloadItemToJson/StateString, just
to handle the "impossible".

> 
> If my understanding is correct, LGTM.

Thanks!
I'll repro the crasher conditions before hitting the cq just to be sure.

[Randy Smith (Not in Mondays), 2012-08-08 21:22:40]

Actually, one more thought.  I just noticed that you don't have the
DownloadStatusUpdater as one of the things modified in this CL.  It currently
takes transitions out of IN_PROGRESS as a signal to update its observers.  I'd
feel better if it also recognized OnDownloadDestroyed(), just in case there's
some future state change.  Willing to do that?

[Aaron Boodman, 2012-08-08 21:45:22]

c/b/e lgtm

[sky, 2012-08-08 22:14:40]

LGTM

[Randy Smith (Not in Mondays), 2012-08-09 17:03:44]

download_status_updater.*, downloads_completion_observer_win.* LGTM.

http://codereview.chromium.org/10704026/diff/73080/chrome/browser/download/do...
File chrome/browser/download/download_status_updater.cc (right):

http://codereview.chromium.org/10704026/diff/73080/chrome/browser/download/do...
chrome/browser/download/download_status_updater.cc:86:
UpdateAppIconDownloadProgress();
I don't actually think you need this--OnDownloadDestroyed() is about a
transition of the object, not of the conceptual download, so there shouldn't be
any change that UAIDP cares about.  I don't object to it, just don't think it
needs to be here.

[benjhayden, 2012-08-09 18:35:06]

http://codereview.chromium.org/10704026/diff/73080/chrome/browser/download/do...
File chrome/browser/download/download_status_updater.cc (right):

http://codereview.chromium.org/10704026/diff/73080/chrome/browser/download/do...
chrome/browser/download/download_status_updater.cc:86:
UpdateAppIconDownloadProgress();
On 2012/08/09 17:03:44, rdsmith wrote:
> I don't actually think you need this--OnDownloadDestroyed() is about a
> transition of the object, not of the conceptual download, so there shouldn't
be
> any change that UAIDP cares about.  I don't object to it, just don't think it
> needs to be here.

I don't know this code, but it appears that UAIDP uses items_ to calculate
everything, so failing to call UAIDP every time that items_ changes would cause
the app icon to fall out of sync.
Also, OnDownloadDestroyed is exactly analogous to REMOVING, so if it was
unnecessary to call UAIDP when REMOVING, then OnDownloadUpdated should have said
if (state != REMOVING). So, at worst, this is preserving existing functionality,
and at best this is necessary for keeping the app icon up to date. If this
functionality should not be preserved, then shouldn't that happen in a separate
CL?

[commit-bot: I haz the power, 2012-08-09 19:09:24]

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

[Randy Smith (Not in Mondays), 2012-08-09 20:27:32]

http://codereview.chromium.org/10704026/diff/73080/chrome/browser/download/do...
File chrome/browser/download/download_status_updater.cc (right):

http://codereview.chromium.org/10704026/diff/73080/chrome/browser/download/do...
chrome/browser/download/download_status_updater.cc:86:
UpdateAppIconDownloadProgress();
On 2012/08/09 18:35:06, benjhayden_chromium wrote:
> On 2012/08/09 17:03:44, rdsmith wrote:
> > I don't actually think you need this--OnDownloadDestroyed() is about a
> > transition of the object, not of the conceptual download, so there shouldn't
> be
> > any change that UAIDP cares about.  I don't object to it, just don't think
it
> > needs to be here.
> 
> I don't know this code, but it appears that UAIDP uses items_ to calculate
> everything, so failing to call UAIDP every time that items_ changes would
cause
> the app icon to fall out of sync.
> Also, OnDownloadDestroyed is exactly analogous to REMOVING, so if it was
> unnecessary to call UAIDP when REMOVING, then OnDownloadUpdated should have
said
> if (state != REMOVING). So, at worst, this is preserving existing
functionality,
> and at best this is necessary for keeping the app icon up to date. If this
> functionality should not be preserved, then shouldn't that happen in a
separate
> CL?

The major issue here is my distinction between the abstract download and the
concrete item.  Changes in the abstract download are reflected in the state. 
Destruction of the item is reflected via OnDownloadDestroyed(). 
UpdateAppIconDownloadProgress only cares about changes in the abstract download,
specifically in this case transitions out of IN_PROGRESS, which are caught
elsewhere.  I want to hook OnDownloadDestroyed() because I'm a paranoid person
and it's the notification "we're destroying the download", and we might
(accidentally or semantically) change the logic at some point so that the
download gets destroyed straight from IN_PROGRESS (though I think that would be
a bad idea) and I'd like to avoid use-after-free in that case.  

But it's not particularly important.  This code should never be called, so we
could conceivably put in a DCHECK.  But I wanted it there because
OnDownloadDestroyed() is how we officially notify that we're about to destroy
the download, and it strikes me as good programming to always release pointers
if held in OnDownloadDestroyed().  UpdateAppIconDownloadProgress() doesn't hurt,
and will help in some of the "can't happen" situations I'm targeting by asking
for this at all.

[commit-bot: I haz the power, 2012-08-09 20:39:00]

Change committed as 150890