DownloadManager::Observer::OnDownloadCreated

content/public/browser/download_manager.h

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 //
 // The DownloadManager object manages the process of downloading, including
 // updates to the history system and providing the information for displaying
 // the downloads view in the Destinations tab. There is one DownloadManager per
 // active browser context in Chrome.
 //
 // Download observers:
@@ skipping 61 matching lines @@
   // Shutdown the download manager. Content calls this when BrowserContext is
   // being destructed. If the embedder needs this to be called earlier, it can
   // call it. In that case, the delegate's Shutdown() method will only be called
   // once.
   virtual void Shutdown() = 0;
 
   // Interface to implement for observers that wish to be informed of changes
   // 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.
     virtual void ManagerGoingDown(DownloadManager* manager) {}
 
     // Called immediately after the DownloadManager puts up a select file
     // dialog.
     // |id| indicates which download opened the dialog.
     virtual void SelectFileDialogDisplayed(
         DownloadManager* manager, int32 id) {}
@@ skipping 152 matching lines @@
  protected:
   virtual ~DownloadManager() {}
 
  private:
   friend class base::RefCountedThreadSafe<DownloadManager>;
 };
 
 }  // namespace content
 
 #endif  // CONTENT_PUBLIC_BROWSER_DOWNLOAD_MANAGER_H_