Kill DownloadItem::IsOtr()

chrome/browser/extensions/api/downloads/downloads_api.cc

Index: chrome/browser/extensions/api/downloads/downloads_api.cc
diff --git a/chrome/browser/extensions/api/downloads/downloads_api.cc b/chrome/browser/extensions/api/downloads/downloads_api.cc
index bbfe75d755619ac13d455bc1404cd22c1b9eb7fd..f914335dc36e8857607167c55861f101e3ec9937 100644
--- a/chrome/browser/extensions/api/downloads/downloads_api.cc
+++ b/chrome/browser/extensions/api/downloads/downloads_api.cc
@@ -186,7 +186,9 @@ bool ValidateFilename(const string16& filename) {
   return true;
 }
 
-scoped_ptr<base::DictionaryValue> DownloadItemToJSON(DownloadItem* item) {
+scoped_ptr<base::DictionaryValue> DownloadItemToJSON(
+    DownloadItem* item,
+    bool incognito) {
   base::DictionaryValue* json = new base::DictionaryValue();
   json->SetInteger(kIdKey, item->GetId());
   json->SetString(kUrlKey, item->GetOriginalUrl().spec());
@@ -202,7 +204,7 @@ scoped_ptr<base::DictionaryValue> DownloadItemToJSON(DownloadItem* item) {
       (item->GetStartTime() - base::Time::UnixEpoch()).InMilliseconds());
   json->SetInteger(kBytesReceivedKey, item->GetReceivedBytes());
   json->SetInteger(kTotalBytesKey, item->GetTotalBytes());
-  json->SetBoolean(kIncognito, item->IsOtr());
+  json->SetBoolean(kIncognito, incognito);
   if (item->GetState() == DownloadItem::INTERRUPTED) {
     json->SetInteger(kErrorKey, static_cast<int>(item->GetLastReason()));
   } else if (item->GetState() == DownloadItem::CANCELLED) {
@@ -314,15 +316,23 @@ bool IsNotTemporaryDownloadFilter(const DownloadItem& item) {
   return !item.IsTemporary();
 }
 
+// Set |manager| to the on-record DownloadManager, and |incognito_manager| to
+// the off-record DownloadManager if one exists and is requested via
+// |include_incognito|. This should work regardless of whether |profile| is
+// original or incognito.
 void GetManagers(
     Profile* profile,
     bool include_incognito,
-    DownloadManager** manager, DownloadManager** incognito_manager) {
-  *manager = BrowserContext::GetDownloadManager(profile);
-  *incognito_manager = NULL;
-  if (include_incognito && profile->HasOffTheRecordProfile()) {
+    DownloadManager** manager,
+    DownloadManager** incognito_manager) {
+  *manager = BrowserContext::GetDownloadManager(profile->GetOriginalProfile());
+  if (profile->HasOffTheRecordProfile() &&
+      (include_incognito ||
+       (profile != profile->GetOriginalProfile()))) {
     *incognito_manager = BrowserContext::GetDownloadManager(
         profile->GetOffTheRecordProfile());
+  } else {
+    *incognito_manager = NULL;
   }
 }
 
@@ -392,8 +402,8 @@ void CompileDownloadQueryOrderBy(
 
 void RunDownloadQuery(
     const extensions::api::downloads::DownloadQuery& query_in,
-    Profile* profile,
-    bool include_incognito,
+    DownloadManager* manager,
+    DownloadManager* incognito_manager,
     std::string* error,
     DownloadQuery::DownloadVector* results) {
   // TODO(benjhayden): Consider switching from LazyInstance to explicit string
@@ -449,9 +459,6 @@ void RunDownloadQuery(
     }
   }
 
-  DownloadManager* manager = NULL;
-  DownloadManager* incognito_manager = NULL;
-  GetManagers(profile, include_incognito, &manager, &incognito_manager);
   DownloadQuery::DownloadVector all_items;
   if (query_in.id.get()) {
     DownloadItem* item = manager->GetDownload(*query_in.id.get());
@@ -468,6 +475,30 @@ void RunDownloadQuery(
   query_out.Search(all_items.begin(), all_items.end(), results);
 }
 
+void DispatchEventInternal(
+    Profile* target_profile,
+    const char* event_name,
+    const std::string& json_args) {
+  target_profile->GetExtensionEventRouter()->DispatchEventToRenderers(
+      event_name,
+      json_args,
+      target_profile,
+      GURL(),
+      extensions::EventFilteringInfo());
+
+  ExtensionDownloadsEventRouter::DownloadsNotificationSource
+    notification_source;
+  notification_source.event_name = event_name;
+  notification_source.profile = target_profile;
+  content::Source<ExtensionDownloadsEventRouter::DownloadsNotificationSource>
+    content_source(&notification_source);
+  std::string args_copy(json_args);
+  content::NotificationService::current()->Notify(
+      chrome::NOTIFICATION_EXTENSION_DOWNLOADS_EVENT,
+      content_source,
+      content::Details<std::string>(&args_copy));
+}
+
 }  // namespace
 
 DownloadsDownloadFunction::DownloadsDownloadFunction() {}
@@ -570,16 +601,28 @@ bool DownloadsSearchFunction::RunImpl() {
   scoped_ptr<extensions::api::downloads::Search::Params> params(
       extensions::api::downloads::Search::Params::Create(*args_));
   EXTENSION_FUNCTION_VALIDATE(params.get());
+  DownloadManager* manager = NULL;
+  DownloadManager* incognito_manager = NULL;
+  GetManagers(profile(), include_incognito(), &manager, &incognito_manager);
   DownloadQuery::DownloadVector results;
-  RunDownloadQuery(params->query, profile(), include_incognito(),
-                   &error_, &results);
+  RunDownloadQuery(params->query,
+                   manager,
+                   incognito_manager,
+                   &error_,
+                   &results);
   if (!error_.empty())
     return false;
+
   base::ListValue* json_results = new base::ListValue();
   for (DownloadManager::DownloadVector::const_iterator it = results.begin();
        it != results.end(); ++it) {
-    scoped_ptr<base::DictionaryValue> item(DownloadItemToJSON(*it));
-    json_results->Append(item.release());
+    DownloadItem* item = *it;
+    int32 download_id = item->GetId();
+    bool off_record = ((incognito_manager != NULL) &&
+                       (incognito_manager->GetDownload(download_id) != NULL));
+    scoped_ptr<base::DictionaryValue> json_item(DownloadItemToJSON(
+        *it, off_record));
+    json_results->Append(json_item.release());

[Randy Smith (Not in Mondays), 2012/08/02 17:52:17]

I wince a little bit at RunDownloadQuery having the incognito/non-incognito
information and throwing it away so that it has to be regenerated here.  It
looks like this is the only place RunDownloadQuery() is called from.  Maybe
refactor so that RunDownloadQuery creates the json so that you don't need to
regenerate the incognito info?

[benjhayden, 2012/08/13 15:08:09]

The downloads are all mixed together, so that wouldn't actually help. Also,
RunDownloadQuery is about to be used by erase().

   }
   SetResult(json_results);
   RecordApiFunctions(DOWNLOADS_FUNCTION_SEARCH);
@@ -790,10 +833,10 @@ ExtensionDownloadsEventRouter::ExtensionDownloadsEventRouter(
     Profile* profile,
     DownloadManager* manager)
   : profile_(profile),
-    manager_(manager) {
+    manager_(manager),
+    incognito_(profile_->HasOffTheRecordProfile() &&
+               (profile_ == profile_->GetOffTheRecordProfile())) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  DCHECK(profile_);
-  DCHECK(manager_);
   manager_->AddObserver(this);
 }
 
@@ -840,7 +883,8 @@ void ExtensionDownloadsEventRouter::OnDownloadUpdated(DownloadItem* item) {
   }
 
   base::DictionaryValue* old_json = item_jsons_[download_id];
-  scoped_ptr<base::DictionaryValue> new_json(DownloadItemToJSON(item));
+  scoped_ptr<base::DictionaryValue> new_json(DownloadItemToJSON(
+        item, incognito_));

[Randy Smith (Not in Mondays), 2012/08/02 17:52:17]

Won't this produce the wrong result when we're dispatching a regular download to
an incognito profile?

[benjhayden, 2012/08/13 15:08:09]

We discussed offline how EDER handles incognito. To summarize, each EDER
observers exactly one manager, which is always either incognito or regular. The
incognito-ness of the download isn't lost until DispatchEventInternal.

   scoped_ptr<base::DictionaryValue> delta(new base::DictionaryValue());
   delta->SetInteger(kIdKey, download_id);
   std::set<std::string> new_fields;
@@ -898,7 +942,7 @@ void ExtensionDownloadsEventRouter::OnDownloadCreated(
 
   download_item->AddObserver(this);
   scoped_ptr<base::DictionaryValue> json_item(
-      DownloadItemToJSON(download_item));
+      DownloadItemToJSON(download_item, incognito_));
   DispatchEvent(extensions::event_names::kOnDownloadCreated,
                 json_item->DeepCopy());
   int32 download_id = download_item->GetId();
@@ -922,18 +966,10 @@ void ExtensionDownloadsEventRouter::DispatchEvent(
   args.Append(arg);
   std::string json_args;
   base::JSONWriter::Write(&args, &json_args);
-  profile_->GetExtensionEventRouter()->DispatchEventToRenderers(
-      event_name,
-      json_args,
-      profile_,
-      GURL(),
-      extensions::EventFilteringInfo());
-
-  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));
+  DispatchEventInternal(profile_, event_name, json_args);
+  if (profile_->HasOffTheRecordProfile() &&
+      (profile_ == profile_->GetOriginalProfile())) {
+    DispatchEventInternal(
+        profile_->GetOffTheRecordProfile(), event_name, json_args);
+  }
 }