Implement chrome.experimental.downloads.onChanged

chrome/browser/download/download_extension_api.cc

 // 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.
 
 #include "chrome/browser/download/download_extension_api.h"
 
 #include <algorithm>
 #include <cctype>
 #include <iterator>
 #include <set>
@@ skipping 17 matching lines @@
 #include "chrome/browser/download/download_util.h"
 #include "chrome/browser/extensions/extension_event_names.h"
 #include "chrome/browser/extensions/extension_event_router.h"
 #include "chrome/browser/icon_loader.h"
 #include "chrome/browser/icon_manager.h"
 #include "chrome/browser/renderer_host/chrome_render_message_filter.h"
 #include "chrome/browser/ui/browser_list.h"
 #include "chrome/browser/ui/webui/web_ui_util.h"
 #include "content/browser/download/download_id.h"
 #include "content/browser/download/download_state_info.h"
+#include "content/browser/download/download_stats.h"
 #include "content/browser/download/download_types.h"
 #include "content/browser/download/interrupt_reasons.h"
 #include "content/browser/renderer_host/render_view_host.h"
 #include "content/browser/renderer_host/resource_dispatcher_host.h"
 #include "content/public/browser/download_item.h"
 #include "content/public/browser/render_process_host.h"
 #include "net/http/http_util.h"
-#include "net/url_request/url_request.h"
 
 using content::BrowserThread;
 using content::DownloadItem;
 using content::DownloadManager;
 
 namespace download_extension_errors {
 
 // Error messages
 const char kGenericError[] = "I'm afraid I can't do that.";
 const char kIconNotFoundError[] = "Icon not found.";
@@ skipping 602 matching lines @@
       (item->GetStartTime() - base::Time::UnixEpoch()).InMilliseconds());
   json->SetInteger(kBytesReceivedKey, item->GetReceivedBytes());
   json->SetInteger(kTotalBytesKey, item->GetTotalBytes());
   if (item->GetState() == DownloadItem::INTERRUPTED)
     json->SetInteger(kErrorKey, static_cast<int>(item->GetLastReason()));
   // TODO(benjhayden): Implement endTime and fileSize.
   // json->SetInteger(kEndTimeKey, -1);
   json->SetInteger(kFileSizeKey, item->GetTotalBytes());
   return json;
 }
+
+typedef std::set<int> DownloadIdSet;
+
 }  // anonymous namespace
 
 ExtensionDownloadsEventRouter::ExtensionDownloadsEventRouter(Profile* profile)
   : profile_(profile),
-    manager_(NULL) {
+    manager_(NULL),
+    delete_item_jsons_(&item_jsons_),
+    delete_on_changed_stats_(&on_changed_stats_) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DCHECK(profile_);
   // Register a callback with the DownloadService for this profile to be called
   // when it creates the DownloadManager, or now if the manager already exists.
   DownloadServiceFactory::GetForProfile(profile)->OnManagerCreated(base::Bind(
       &ExtensionDownloadsEventRouter::Init, base::Unretained(this)));
 }
 
 // The only public methods on this class are ModelChanged() and
 // ManagerGoingDown(), and they are only called by DownloadManager, so
 // there's no way for any methods on this class to be called before
 // DownloadService calls Init() via the OnManagerCreated Callback above.
 void ExtensionDownloadsEventRouter::Init(DownloadManager* manager) {
   DCHECK(manager_ == NULL);
   manager_ = manager;
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   manager_->AddObserver(this);
 }
 
 ExtensionDownloadsEventRouter::~ExtensionDownloadsEventRouter() {
   if (manager_ != NULL)
     manager_->RemoveObserver(this);
+  for (ItemMap::const_iterator iter = downloads_.begin();
+       iter != downloads_.end(); ++iter) {
+    if (iter->second != NULL)
+      iter->second->RemoveObserver(this);
+  }
+}
+
+ExtensionDownloadsEventRouter::OnChangedStat::OnChangedStat()
+  : fires(0), total(0) {
+}
+
+ExtensionDownloadsEventRouter::OnChangedStat::~OnChangedStat() {
+  download_stats::RecordOnChanged(fires * 100 / total);
+}
+
+void ExtensionDownloadsEventRouter::OnDownloadUpdated(DownloadItem* item) {
+  int download_id = item->GetId();
+  if (item->GetState() == DownloadItem::REMOVING) {
+    // Remove item from downloads_, but keep the id in downloads_ so that we
+    // notice its absence in ModelChanged() and fire an onErased.

[Randy Smith (Not in Mondays), 2012/01/15 18:47:13]

Idea (i.e. the notch below "suggestion"; just something I'd like you to think
about): The obvious alternative to this is to fire the OnErased from here.  The
argument to fire the OnErased() from ModelChanged() is pretty clear; it's where
you have to fire the OnCreated() from, and that's a natural symmetry.  But if
you fire the OnErased from here, you can keep the downloads_ data structure
cleaner, because you won't have the special meaning of "NULL".  Given the choice
between clean procedural code and clean data structures, I lean towards clean
data structures, as the data structures need to be utilized and understood from
multiple different sections of the code, and putting a comment in ModelChanged()
(when a download is erased) pointing here mostly deals with the dangers of the
unclean code.

[benjhayden, 2012/02/01 21:42:40]

Done.

+    ItemMap::const_iterator it = downloads_.find(download_id);
+    DCHECK(it != downloads_.end());
+    DCHECK(it->second == item);
+    downloads_[download_id] = NULL;
+    item->RemoveObserver(this);
+    return;
+  }
+  base::DictionaryValue* current_json = item_jsons_[download_id];
+  scoped_ptr<base::DictionaryValue> new_json(DownloadItemToJSON(item));
+  scoped_ptr<base::DictionaryValue> delta(new base::DictionaryValue());
+  delta->SetInteger(kIdKey, download_id);
+  bool changed = false;
+  for (base::DictionaryValue::key_iterator key = new_json->begin_keys();
+       key != new_json->end_keys(); ++key) {
+    if (*key != kBytesReceivedKey) {
+      base::Value* new_value = NULL;
+      base::Value* old_value = NULL;
+      if (new_json->Get(*key, &new_value) &&

[Randy Smith (Not in Mondays), 2012/01/15 18:47:13]

So this isn't performance critical code, but (:-}): Did you consider using a
DictionaryValue::Iterator to drop this from O(n log n) to O(n) (I'm presuming
the Get is log n).  There isn't a lot of value in doing it the more performing
way, but I don't see the argument on the other side; I think the code would
still be as clear.

[benjhayden, 2012/02/01 21:42:40]

Done.

+          (!current_json->HasKey(*key) ||
+          (current_json->Get(*key, &old_value) &&
+            !new_value->Equals(old_value)))) {
+        delta->Set(*key + ".old", (old_value ? old_value->DeepCopy()
+                                            : base::Value::CreateNullValue()));
+        delta->Set(*key + ".new", new_value->DeepCopy());
+        changed = true;
+      }
+    }
+  }
+  ++(on_changed_stats_[download_id]->total);
+  if (changed) {
+    DispatchEvent(extension_event_names::kOnDownloadChanged, delta.release());
+    ++(on_changed_stats_[download_id]->fires);
+  }
+  current_json->Swap(new_json.get());
+}
+
+void ExtensionDownloadsEventRouter::OnDownloadOpened(DownloadItem* item) {
 }
 
 void ExtensionDownloadsEventRouter::ModelChanged() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   if (manager_ == NULL)
     return;
   DownloadManager::DownloadVector current_vec;
   manager_->SearchDownloads(string16(), &current_vec);
-  DownloadIdSet current_set;
+  DownloadIdSet current_set, prev_set;
+  for (ItemMap::const_iterator iter = downloads_.begin();
+       iter != downloads_.end(); ++iter) {
+    prev_set.insert(iter->first);
+  }
   ItemMap current_map;
   for (DownloadManager::DownloadVector::const_iterator iter =
        current_vec.begin();
        iter != current_vec.end(); ++iter) {
     DownloadItem* item = *iter;
     int item_id = item->GetId();
     // TODO(benjhayden): Remove the following line when every item's id >= 0,
     // which will allow firing onErased events for items from the history.
     if (item_id < 0) continue;
     DCHECK(current_map.find(item_id) == current_map.end());
     current_set.insert(item_id);
     current_map[item_id] = item;
   }
-  DownloadIdSet new_set;  // current_set - downloads_;
-  DownloadIdSet erased_set;  // downloads_ - current_set;
+  DownloadIdSet new_set;  // current_set - prev_set;

[Randy Smith (Not in Mondays), 2012/01/15 18:47:13]

semi-nit: I think this section would be clearer if you separated out the
"construction" of the two sets.  They're basically identical, and a hypothetical
reader reading through the code only really needs to read through the C++ syntax
for creating a new set that's the different of two old sets once.  

It might even be worthwhile creating a static helper, and turning this into two
lines worth of code.  

[benjhayden, 2012/02/01 21:42:40]

erased_set isn't necessary now that the onErased is fired in
OnDownloadUpdated().

+  DownloadIdSet erased_set;  // prev_set - current_set;
   std::insert_iterator<DownloadIdSet> new_insertor(new_set, new_set.begin());
   std::insert_iterator<DownloadIdSet> erased_insertor(
       erased_set, erased_set.begin());
   std::set_difference(current_set.begin(), current_set.end(),
-                      downloads_.begin(), downloads_.end(),
+                      prev_set.begin(), prev_set.end(),
                       new_insertor);
-  std::set_difference(downloads_.begin(), downloads_.end(),
+  std::set_difference(prev_set.begin(), prev_set.end(),
                       current_set.begin(), current_set.end(),
                       erased_insertor);
   for (DownloadIdSet::const_iterator iter = new_set.begin();
        iter != new_set.end(); ++iter) {
     DispatchEvent(extension_event_names::kOnDownloadCreated,
                   DownloadItemToJSON(current_map[*iter]));
+    DCHECK(item_jsons_.find(*iter) == item_jsons_.end());
+    on_changed_stats_[*iter] = new OnChangedStat();
+    item_jsons_[*iter] = DownloadItemToJSON(current_map[*iter]);
+    current_map[*iter]->AddObserver(this);
   }
   for (DownloadIdSet::const_iterator iter = erased_set.begin();
        iter != erased_set.end(); ++iter) {
     DispatchEvent(extension_event_names::kOnDownloadErased,
                   base::Value::CreateIntegerValue(*iter));
+    delete item_jsons_[*iter];
+    item_jsons_.erase(*iter);
+    delete on_changed_stats_[*iter];
+    on_changed_stats_.erase(*iter);
   }
-  downloads_.swap(current_set);
+  downloads_.swap(current_map);
 }
 
 void ExtensionDownloadsEventRouter::ManagerGoingDown() {
   manager_->RemoveObserver(this);
   manager_ = NULL;
 }
 
 void ExtensionDownloadsEventRouter::DispatchEvent(
     const char* event_name, base::Value* arg) {
   ListValue args;
   args.Append(arg);
   std::string json_args;
   base::JSONWriter::Write(&args, false, &json_args);
   profile_->GetExtensionEventRouter()->DispatchEventToRenderers(
       event_name,
       json_args,
       profile_,
       GURL());
 }