Rewrite DownloadsApiTest in C++.

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 19 matching lines @@
 #include "chrome/browser/download/download_service.h"
 #include "chrome/browser/download/download_service_factory.h"
 #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 "chrome/common/chrome_notification_types.h"
 #include "content/public/browser/download_interrupt_reasons.h"
 #include "content/public/browser/download_item.h"
 #include "content/public/browser/download_save_info.h"
+#include "content/public/browser/notification_service.h"
 #include "content/public/browser/render_process_host.h"
 #include "content/public/browser/render_view_host.h"
 #include "content/public/browser/resource_dispatcher_host.h"
 #include "net/base/load_flags.h"
 #include "net/http/http_util.h"
 #include "net/url_request/url_request.h"
 
 using content::BrowserContext;
 using content::BrowserThread;
 using content::DownloadId;
@@ skipping 922 matching lines @@
   : fires(0),
     total(0) {
 }
 
 ExtensionDownloadsEventRouter::OnChangedStat::~OnChangedStat() {
   if (total > 0)
     UMA_HISTOGRAM_PERCENTAGE("Download.OnChanged", (fires * 100 / total));
 }
 
 void ExtensionDownloadsEventRouter::OnDownloadUpdated(DownloadItem* item) {
+  if (!profile_)
+    return;
   int download_id = item->GetId();
   if (item->GetState() == DownloadItem::REMOVING) {
     // The REMOVING state indicates that this item is being erased.
     // Let's unregister as an observer so that we don't see any more updates
     // from it, dispatch the onErased event, and remove its json and is
     // OnChangedStat from our maps.
     downloads_.erase(download_id);
     item->RemoveObserver(this);
     DispatchEvent(extension_event_names::kOnDownloadErased,
                   base::Value::CreateIntegerValue(download_id));
@@ skipping 104 matching lines @@
   downloads_.swap(current_map);
 
   // Dispatching onErased is handled in OnDownloadUpdated when an item
   // transitions to the REMOVING state.
 }
 
 void ExtensionDownloadsEventRouter::ManagerGoingDown(
     DownloadManager* manager) {
   manager_->RemoveObserver(this);
   manager_ = NULL;
+  profile_ = NULL;

[Randy Smith (Not in Mondays), 2012/06/18 18:42:58]

So this means that you'll not handle the notifications that are generated for
individual download items during the shutdown process.  Is that the behavior you
want?  Can you give me a sense as to why?  (Not saying I object, just that I
want to make sure we explore the implications.)

[benjhayden, 2012/06/19 15:01:59]

When I made ChromeDownloadManagerDelegate own ExtensionDownloadsEventRouter,
DispatchEvent() started crashing. Either profile_ or
profile_->GetExtensionEventRouter() had already been destroyed by the time EDER
observed the shutdown events.

[Randy Smith (Not in Mondays), 2012/06/19 19:23:23]

Sure, that's a problem, but you've chosen one particular way of solving it. 
Another way might be to make sure that whatever notifications show up after
DownloadManager::Shutdown() are noticed as being spurious and dropped on the
floor (I don't know how we'd do that because I don't know what such
notifications would show up--we shouldn't be generating any notifications after
that point.  So this would take some debugging.)  But I'm not sure that's the
right thing to do either, so I'm asking the behavioral question.  From the
viewpoint of an extension writer, would getting notifications from the
DownloadItems as they're being torn down by the DownloadManager during shutdown
be valuable?

I could argue "Yes!  If we don't get those notifications, we won't know that the
download items were actually interrupted/cancelled/whatever, and that's vital
for tracking their life cycle."  I could also argue "No!  By the time we're
calling DownloadManager::Shutdown, the profile is on its way out, and all
extensions have been torn down, so it doesn't make any difference."  (I don't
know that that's true, but it's plausible.)  Since I don't know what the right
argument is, I'm asking the Download extensions owner :-}.

[benjhayden, 2012/06/21 17:50:48]

An extension that wants to keep track of the state of some items should call
search() when it starts up.
I am inclined to do the simple thing until it proves insufficient somehow, then
consider addressing the particular insufficiency. This route does not preclude
figuring out how to fire the shutdown events at some point.

 }
 
 void ExtensionDownloadsEventRouter::DispatchEvent(
     const char* event_name, base::Value* arg) {
+  if (!profile_)
+    return;
   ListValue args;
   args.Append(arg);
   std::string json_args;
   base::JSONWriter::Write(&args, &json_args);
   profile_->GetExtensionEventRouter()->DispatchEventToRenderers(
       event_name,
       json_args,
       profile_,
       GURL());
+  DownloadsNotificationSource notification_source;
+  notification_source.event_name = event_name;
+  notification_source.profile = profile_;
+  content::NotificationService::current()->Notify(
+      chrome::NOTIFICATION_EXTENSION_DOWNLOADS_EVENT,
+      content::Source<DownloadsNotificationSource>(&notification_source),
+      content::Details<std::string>(&json_args));

[Randy Smith (Not in Mondays), 2012/06/18 18:42:58]

This is two different mechanisms for notifying about changes to a download item,
on top of the DownloadItem observer mechanism.  Would it make sense to, rather
than doing both of these, make the ExtensionDownloadsEventRouter automatically
become a DownloadManager observer in its constructor?  And maybe find some way
(this is probably a large hassle in C++, but I figure it can't hurt to ask) to
hook the renderer event dispatch in C++ to avoid the extra NotificationService
notification?

[benjhayden, 2012/06/19 15:01:59]

EDER is-a DM::O already. How would making it a DM::O make it unnecessary for
EDER to give events to the extension system?

I could add a delegate to EDER so that EDER could call out to the testcase
directly, but that would require exposing CDMD's EDER somehow, which seems like
bad encapsulation.

[Randy Smith (Not in Mondays), 2012/06/19 19:23:23]

Sorry, this is my cluelessness; I didn't notice that we were already inside
ExtensionDownloadsEventRouter (should it have its own file?).  Ignore that
suggestion.
Yeah.  Both directions seem messy to me, but I certainly agree with you that the
EDER delegate is no neater than what you've done.  Ok, I yield :-}.

 }