Rewrite DownloadsDOMHandler

chrome/browser/ui/webui/downloads_dom_handler.h

Index: chrome/browser/ui/webui/downloads_dom_handler.h
diff --git a/chrome/browser/ui/webui/downloads_dom_handler.h b/chrome/browser/ui/webui/downloads_dom_handler.h
index d57df0a9524d9094c0d6d05b65108716d00e5f28..b90d66d776b305133de24a3a50597d7fa495fb19 100644
--- a/chrome/browser/ui/webui/downloads_dom_handler.h
+++ b/chrome/browser/ui/webui/downloads_dom_handler.h
@@ -5,6 +5,7 @@
 #ifndef CHROME_BROWSER_UI_WEBUI_DOWNLOADS_DOM_HANDLER_H_
 #define CHROME_BROWSER_UI_WEBUI_DOWNLOADS_DOM_HANDLER_H_
 
+#include <set>
 #include <vector>
 
 #include "base/compiler_specific.h"
@@ -17,6 +18,10 @@ namespace base {
 class ListValue;
 }
 
+namespace content {
+class WebContents;
+}
+
 // The handler for Javascript messages related to the "downloads" view,
 // also observes changes to the download manager.
 class DownloadsDOMHandler : public content::WebUIMessageHandler,
@@ -32,11 +37,14 @@ class DownloadsDOMHandler : public content::WebUIMessageHandler,
   virtual void RegisterMessages() OVERRIDE;
 
   // content::DownloadItem::Observer interface
-  virtual void OnDownloadUpdated(content::DownloadItem* download) OVERRIDE;
-  virtual void OnDownloadDestroyed(content::DownloadItem* download) OVERRIDE;
+  virtual void OnDownloadUpdated(
+      content::DownloadItem* download_item) OVERRIDE;
+  virtual void OnDownloadDestroyed(
+      content::DownloadItem* download_item) OVERRIDE;
 
   // content::DownloadManager::Observer interface
-  virtual void ModelChanged(content::DownloadManager* manager) OVERRIDE;
+  virtual void OnDownloadCreated(content::DownloadManager* manager,
+                                 content::DownloadItem* download_item) OVERRIDE;
   virtual void ManagerGoingDown(content::DownloadManager* manager) OVERRIDE;
 
   // Callback for the "onPageLoaded" message.
@@ -79,12 +87,29 @@ class DownloadsDOMHandler : public content::WebUIMessageHandler,
   // folder.
   void HandleOpenDownloadsFolder(const base::ListValue* args);
 
+ protected:
+  // These methods are for mocking so that most of this class does not actually
+  // depend on WebUI. The other methods that depend on WebUI are
+  // RegisterMessages() and HandleDrag().
+  virtual content::WebContents* GetWebUIWebContents();
+  virtual void CallDownloadsList(const base::ListValue& downloads);
+  virtual void CallDownloadUpdated(const base::ListValue& download);
+
  private:
-  class OriginalDownloadManagerObserver;
+  // A set of DownloadItem*s.

[James Hawkins, 2012/08/22 17:39:01]

Nix this.  The documentation should describe the purpose, e.g. to have a
shorthand notation, and how it's used, e.g. DownloadSets are passed from the
model and displayed in the view (or what have you).

[benjhayden, 2012/08/22 20:19:22]

Done.

+  typedef std::set<content::DownloadItem*> DownloadSet;
+
+  // Schedule a call to SendCurrentDownloads() in the next message loop

[James Hawkins, 2012/08/22 17:39:01]

nit: Method documentation should use third-person active, i.e., Schedules.

[benjhayden, 2012/08/22 20:19:22]

Done.

+  // iteration.
+  void ScheduleSendCurrentDownloads();
 
   // Send the current list of downloads to the page.
   void SendCurrentDownloads();
 
+  // Fills |downloads| with all the items for both DownloadManagers matching
+  // |search_text_|.
+  void SearchDownloads(content::DownloadManager::DownloadVector* downloads);
+
   // Clear all download items and their observers.
   void ClearDownloadItems();
 
@@ -100,35 +125,26 @@ class DownloadsDOMHandler : public content::WebUIMessageHandler,
   // DownloadDangerPrompt invoked via ShowDangerPrompt().
   void DangerPromptAccepted(int download_id);
 
-  // Return the download that corresponds to a given id.
-  content::DownloadItem* GetDownloadById(int id);
-
   // Return the download that is referred to in a given value.
   content::DownloadItem* GetDownloadByValue(const base::ListValue* args);
 
   // Current search text.
-  std::wstring search_text_;
+  string16 search_text_;
+
+  // Keep track of all items that this is observing so that RemoveObserver will
+  // be called for all of them.
+  DownloadSet observing_items_;
 
   // Our model
   content::DownloadManager* download_manager_;
 
-  // If |download_manager_| belongs to an incognito profile than this
+  // If |download_manager_| belongs to an incognito profile then this
   // is the DownloadManager for the original profile; otherwise, this is
   // NULL.
   content::DownloadManager* original_profile_download_manager_;
 
-  // True once the page has loaded the first time (it may load multiple times,
-  // e.g. on reload).
-  bool initialized_;
-
-  // The current set of visible DownloadItems for this view received from the
-  // DownloadManager. DownloadManager owns the DownloadItems. The vector is
-  // kept in order, sorted by ascending start time.
-  // Note that when a download item is removed, the entry in the vector becomes
-  // null.  This should only be a transient state, as a ModelChanged()
-  // notification should follow close on the heels of such a change.
-  typedef std::vector<content::DownloadItem*> OrderedDownloads;
-  OrderedDownloads download_items_;
+  // Whether a call to SendCurrentDownloads() is currently scheduled.
+  bool update_scheduled_;
 
   base::WeakPtrFactory<DownloadsDOMHandler> weak_ptr_factory_;