Implement chrome.experimental.downloads.onChanged

chrome/browser/extensions/extension_downloads_api.cc

Index: chrome/browser/extensions/extension_downloads_api.cc
diff --git a/chrome/browser/extensions/extension_downloads_api.cc b/chrome/browser/extensions/extension_downloads_api.cc
index 2182b000fdcf62ef0ff1d153824ef259c545a0f2..a04138318917af4055b038adb10383d8e44f31a6 100644
--- a/chrome/browser/extensions/extension_downloads_api.cc
+++ b/chrome/browser/extensions/extension_downloads_api.cc
@@ -414,10 +414,9 @@ base::DictionaryValue* DownloadItemToJSON(DownloadItem* item) {
 ExtensionDownloadsEventRouter::ExtensionDownloadsEventRouter(
     Profile* profile)
   : profile_(profile),
-    manager_(profile ? profile->GetDownloadManager() : NULL) {
+    manager_(profile ? profile->GetDownloadManager() : NULL),
+    delete_item_observers_(&item_observers_) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  DCHECK(profile_);
-  DCHECK(manager_);
   manager_->AddObserver(this);
 }
 
@@ -426,6 +425,54 @@ ExtensionDownloadsEventRouter::~ExtensionDownloadsEventRouter() {
     manager_->RemoveObserver(this);
 }
 
+ExtensionDownloadsEventRouter::ItemObserver::ItemObserver(
+    const EventDispatcher& dispatcher, DownloadItem* item)
+  : dispatcher_(dispatcher),
+    item_(item),
+    json_(DownloadItemToJSON(item_)) {
+  item_->AddObserver(this);
+}
+
+ExtensionDownloadsEventRouter::ItemObserver::~ItemObserver() {
+  if (item_ != NULL)
+    item_->RemoveObserver(this);
+}
+
+void ExtensionDownloadsEventRouter::ItemObserver::OnDownloadUpdated(

[Randy Smith (Not in Mondays), 2011/10/12 18:46:30]

This amount of work on each DownloadItem updated strengthens my interest in
getting rid of updates on size changes and shifting over to polling.  Is that on
your agenda?  My sense is that we've agreed we want to do it but that no one's
taken ownership yet, but if that's wrong I want to know :-}.

We might alternatively push determining the delta down into the DownloadItem
class and have OnDownloadUpdated include an argument indicating which fields
have updated.  That'd save this level of deltaing.  WDYT?

[benjhayden, 2011/10/13 16:30:29]

This isn't quite as much work as it looks like. The for-loop is over 10-20
fields, this whole method only does a few small mallocs, and there's just a dash
of string processing in delta->Set(), which is almost never called more than 3
times per OnDownloadUpdated().
If you're talking about polling for bytesReceived changes only, then yes. I
think it makes good API sense to dispatch an event for other changes both at the
extension level and the DownloadItem::Observer level.
Shifting DownloadItem over to requiring its observers to poll for bytesReceived
events sounds to me like part of the refactoring task slated for next quarter. I
think that we can wire up the new ui as it is now, but, yes, not dispatching
bytesReceived will be a Good Thing.
I think that having DownloadItem tell its observers what changed is the Right
Thing To Do, even though it will almost certainly require more code than this
approach. I thought that we had dismissed it a while ago for some reason?

[Randy Smith (Not in Mondays), 2011/10/13 23:42:59]

Yep, that's what I meant.  
Hmmm.  Ok, I'll put it on the list.  Could you put some performance tests in at
some point for the extensions so that we have some idea whether/how much we'll
be in pain from doing the bytesReceived dispatch?  My memory of the code is that
we've got a gratuitous doubling of messages (we both have a timer loop that
fires off events, and we update on every size change explicitly). 
Yeah; I think I was just being resistant to widespread code changes.  I think
given that this code isn't that many lines, I'll put it on the list for the
future.  I don't actually want to send a delta from the base object (though I'm
open to the argument), but at the moment, at least, I'm open to saying "this is
why we're doing the update". 

Note that that updating will end up using a symbol for each (conceptual) field
that's updated (based on my shoot-without-removing-gun-from-holster as to the
design), which we might usefully combine with the symbols I asked you for in the
other CL (to refer to the DownloadItem fields for querying).

[benjhayden, 2011/10/17 19:14:00]

I just added a uma stat to count the number of OnDownloadUpdated calls that
do/don't trigger a downloads.onChanged event. (2 enum buckets)
I'll put an AI on my whiteboard for writing a better perf test.
We could also resurrect the SimpleDownloadItem, but this time as part of the
DownloadItem[Interface] hierarchy.
DownloadItemInterface -> SimpleDownloadItem -> DownloadItem
So, SimpleDownloadItem could be a dumb copyable struct-of-POD that would just be
a cheaper version of the cached jsons. We could also merge SimpleDownloadItem
with DownloadPersistentStoreInfo.
Things to think about for another CL.

[Randy Smith (Not in Mondays), 2011/10/18 21:15:32]

Sounds good; thanks.
Possibly.  My shoot from the hip reaction is that it's not worthwhile making the
hierarchy more complex to save a constant factor--the thing I'm really worried
about is having to keep extra bytes around on a per-history entry basis.  But
we're already doing that, so maybe reducing the total amount would be worth it? 


If we could combine it with DownloadPersistentStoreInfo I'd be in favor and my
only issue would be whether it was the best place to put our energies.  (I have
a dream of combining DownloadPersistentStoreInfo and DownloadCreateInfo too,
though that's probably not practical.  Still, it makes some sense to me that
"amount of information needed to start a download" would be approximately the
same as "amount of information needed to restart a download", and the latter is
where DPSI is going.

+    DownloadItem* item) {
+  CHECK_EQ(item, item_);
+  if (item_->state() == DownloadItem::REMOVING)
+    return;
+  scoped_ptr<base::DictionaryValue> new_json(DownloadItemToJSON(item_));
+  base::DictionaryValue* delta = new base::DictionaryValue();
+  delta->SetInteger(constants::kIdKey, item_->id());
+  bool changed = false;
+  for (base::DictionaryValue::key_iterator key = new_json->begin_keys();
+       key != new_json->end_keys(); ++key) {
+    if (*key != constants::kBytesReceivedKey) {
+      base::Value* new_value = NULL;
+      base::Value* old_value = NULL;
+      if (new_json->Get(*key, &new_value) &&
+          (!json_->HasKey(*key) ||
+          (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;
+      }
+    }
+  }
+  if (changed)
+    dispatcher_.Run(delta);
+  json_.swap(new_json);
+}
+
+void ExtensionDownloadsEventRouter::ItemObserver::OnDownloadOpened(
+    DownloadItem* item) {
+  CHECK_EQ(item, item_);
+}
+
 void ExtensionDownloadsEventRouter::ModelChanged() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   if (manager_ == NULL)
@@ -461,11 +508,17 @@ void ExtensionDownloadsEventRouter::ModelChanged() {
        iter != new_set.end(); ++iter) {
     DispatchEvent(extension_event_names::kOnDownloadCreated,
                   DownloadItemToJSON(current_map[*iter]));
+    DCHECK(item_observers_.find(*iter) == item_observers_.end());
+    item_observers_[*iter] = new ItemObserver(base::Bind(
+        &ExtensionDownloadsEventRouter::DispatchEvent, base::Unretained(this),
+        extension_event_names::kOnDownloadChanged), current_map[*iter]);
   }
   for (DownloadIdSet::const_iterator iter = erased_set.begin();
        iter != erased_set.end(); ++iter) {
     DispatchEvent(extension_event_names::kOnDownloadErased,
                   base::Value::CreateIntegerValue(*iter));
+    delete item_observers_[*iter];
+    item_observers_.erase(*iter);
   }
   downloads_.swap(current_set);
 }