DownloadManager::Observer::OnDownloadCreated

content/public/browser/download_manager.h

Index: content/public/browser/download_manager.h
diff --git a/content/public/browser/download_manager.h b/content/public/browser/download_manager.h
index 1d60213cc5183bf9a513ec2242ee53a37f6c72b8..48ee33605bba794611b3e0f5b66de3732420b39a 100644
--- a/content/public/browser/download_manager.h
+++ b/content/public/browser/download_manager.h
@@ -79,9 +79,15 @@ class CONTENT_EXPORT DownloadManager
   // to the DownloadManager's collection of downloads.
   class CONTENT_EXPORT Observer {
    public:
+    // A DownloadItem was created. No guarantees are made about its state: it
+    // may have just been loaded from history, it may be an active download, or
+    // it may be a SavePage download, it may or may not already be persisted.

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

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.  

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

Updated the comment to clarify which features are guaranteed.
I updated the CL description and this comment.

+    virtual void OnDownloadCreated(
+        DownloadManager* manager, DownloadItem* item) {}

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

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]

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?

+
     // New or deleted download, observers should query us for the current set
     // of downloads.
-    virtual void ModelChanged(DownloadManager* manager) = 0;
+    virtual void ModelChanged(DownloadManager* manager) {}
 
     // Called when the DownloadManager is being destroyed to prevent Observers
     // from calling back to a stale pointer.