Lazy creating of background pages --enable-lazy-background-pages)

chrome/browser/extensions/extension_event_router.cc

Index: chrome/browser/extensions/extension_event_router.cc
diff --git a/chrome/browser/extensions/extension_event_router.cc b/chrome/browser/extensions/extension_event_router.cc
index 7a9cb34065807cd76bf647eaab015609c542f117..f6a66e6d7866224600eb547175dab2f8b4f06ed9 100644
--- a/chrome/browser/extensions/extension_event_router.cc
+++ b/chrome/browser/extensions/extension_event_router.cc
@@ -4,14 +4,17 @@
 
 #include "chrome/browser/extensions/extension_event_router.h"
 
+#include "base/command_line.h"
 #include "base/values.h"
 #include "chrome/browser/extensions/extension_devtools_manager.h"
+#include "chrome/browser/extensions/extension_host.h"
 #include "chrome/browser/extensions/extension_processes_api.h"
 #include "chrome/browser/extensions/extension_processes_api_constants.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/extensions/extension_tabs_module.h"
 #include "chrome/browser/extensions/extension_webrequest_api.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/common/chrome_switches.h"
 #include "chrome/common/extensions/extension.h"
 #include "chrome/common/extensions/extension_messages.h"
 #include "content/browser/child_process_security_policy.h"
@@ -63,14 +66,20 @@ void ExtensionEventRouter::DispatchEvent(IPC::Message::Sender* ipc_sender,
 
 ExtensionEventRouter::ExtensionEventRouter(Profile* profile)
     : profile_(profile),
-      extension_devtools_manager_(profile->GetExtensionDevToolsManager()) {
+      extension_devtools_manager_(profile->GetExtensionDevToolsManager()),
+      ALLOW_THIS_IN_INITIALIZER_LIST(task_factory_(this)) {
   registrar_.Add(this, content::NOTIFICATION_RENDERER_PROCESS_TERMINATED,
                  NotificationService::AllSources());
   registrar_.Add(this, content::NOTIFICATION_RENDERER_PROCESS_CLOSED,
                  NotificationService::AllSources());
+  registrar_.Add(this, chrome::NOTIFICATION_EXTENSION_HOST_DID_STOP_LOADING,
+                 Source<Profile>(profile_));
+  // TODO(tessamac): also get notified for background page crash/failure.
 }
 
 ExtensionEventRouter::~ExtensionEventRouter() {
+  // May not be necessary...
+  task_factory_.RevokeAll();

[Aaron Boodman, 2011/08/23 22:41:16]

Not necessary -- this happens automatically when task_factory_ is deleted.

[Tessa MacDuff, 2011/08/24 00:34:23]

Done.

 }
 
 void ExtensionEventRouter::AddEventListener(
@@ -143,7 +152,7 @@ void ExtensionEventRouter::DispatchEventToRenderers(
     Profile* restrict_to_profile,
     const GURL& event_url) {
   DispatchEventImpl("", event_name, event_args, restrict_to_profile, "",
-                    event_url);
+                    event_url, false);
 }
 
 void ExtensionEventRouter::DispatchEventToExtension(
@@ -154,7 +163,7 @@ void ExtensionEventRouter::DispatchEventToExtension(
     const GURL& event_url) {
   DCHECK(!extension_id.empty());
   DispatchEventImpl(extension_id, event_name, event_args, restrict_to_profile,
-                    "", event_url);
+                    "", event_url, false);
 }
 
 void ExtensionEventRouter::DispatchEventsToRenderersAcrossIncognito(
@@ -164,7 +173,31 @@ void ExtensionEventRouter::DispatchEventsToRenderersAcrossIncognito(
     const std::string& cross_incognito_args,
     const GURL& event_url) {
   DispatchEventImpl("", event_name, event_args, restrict_to_profile,
-                    cross_incognito_args, event_url);
+                    cross_incognito_args, event_url, false);
+}
+
+bool ExtensionEventRouter::CanDispatchEventNow(
+    const std::string& extension_id, bool was_pending) {
+  if (CommandLine::ForCurrentProcess()->HasSwitch(
+          switches::kEnableLazyBackgroundPages)) {

[Aaron Boodman, 2011/08/23 22:41:16]

Invert test and return early to reduce indenting.

[Tessa MacDuff, 2011/08/24 00:34:23]

Done.

+    if (extension_id.empty())
+      // TODO(tessamac): Create all background pages. Wait for all to be loaded?
+      //                 or dispatch event to each extension when it's ready?
+      return true;
+    const Extension* extension = profile_->GetExtensionService()->
+        GetExtensionById(extension_id, false);  // exclude disabled extensions
+    if (extension && extension->background_url().is_valid()) {
+      ExtensionProcessManager* pm = profile_->GetExtensionProcessManager();
+      if (!pm->GetBackgroundHostForExtension(extension)) {
+        // Pending events are dispatched when the background page is done
+        // loading. Maybe it was then shutdown too soon?
+        DCHECK(!was_pending);

[Aaron Boodman, 2011/08/23 22:41:16]

Can you move this to the call site?

if (!CanDispatchEventNow(extension_id)) {
  CHECK(!was_pending);  // shouldn't happen twice!
  AppendEvent(...);
  return;
}

Then you won't have to mess with task.h.

Also, I usually prefer CHECK to DCHECK since we will get error reports from the
field rather than complaints of silent failure.

[Tessa MacDuff, 2011/08/24 00:34:23]

Done.

But why does this mean I don't have to mess with task.h?  DispatchEventImpl()
still has 7 args.

[Aaron Boodman, 2011/08/24 18:49:32]

Whoops, you're right. I suspect the owner of base/task.h might ask to use a
structure here instead of a seven-param method. But I actually prefer the
seven-param function I think so here's hoping...

+        pm->CreateBackgroundHost(extension, extension->background_url());
+        return false;
+      }
+    }
+  }
+  return true;
 }
 
 void ExtensionEventRouter::DispatchEventImpl(
@@ -173,13 +206,22 @@ void ExtensionEventRouter::DispatchEventImpl(
     const std::string& event_args,
     Profile* restrict_to_profile,
     const std::string& cross_incognito_args,
-    const GURL& event_url) {
+    const GURL& event_url,
+    bool was_pending) {
   if (!profile_)
     return;
 
   // We don't expect to get events from a completely different profile.
   DCHECK(!restrict_to_profile || profile_->IsSameProfile(restrict_to_profile));
 
+  if (!CanDispatchEventNow(extension_id, was_pending)) {
+    // TODO(tessamac): make sure Background Page notification doesn't
+    //                 happen before the event is added to the pending list.
+    AppendEvent(extension_id, event_name, event_args, restrict_to_profile,
+                cross_incognito_args, event_url);
+    return;
+  }
+
   ListenerMap::iterator it = listeners_.find(event_name);
   if (it == listeners_.end())
     return;
@@ -220,6 +262,50 @@ void ExtensionEventRouter::DispatchEventImpl(
   }
 }
 
+void ExtensionEventRouter::AppendEvent(const std::string &extension_id,
+                                       const std::string& event_name,
+                                       const std::string& event_args,
+                                       Profile* restrict_to_profile,
+                                       const std::string& cross_incognito_args,
+                                       const GURL& event_url) {
+  // Put a new list of pending tasks into the map (more common) or get a
+  //  pointer to the list that is already there (less common).
+  PendingTasksList* tasks_list = new PendingTasksList();
+  PendingTasksPerExtMap::const_iterator it = pending_tasks_.find(extension_id);

[Aaron Boodman, 2011/08/23 22:41:16]

You do the find(), then always end up trying to insert anyway, so I don't think
you're saving anything. Anyway, better to be clear since this isn't performance
critical.

The more common pattern is:

Thinger* thinger = NULL;
ThingerContainer::Iterator it = pending_tasks.find(id);
if (it == pending_tasks.end()) {
  thinger = new Thinger();
  pending_tasks_[id] = thinger;
} else {
  thinger = *(it->second);
}

[Tessa MacDuff, 2011/08/24 00:34:23]

Oops I just forgot to remove that line. 'it' is never used...

Done.

+  std::pair<PendingTasksPerExtMap::iterator, bool> ret = pending_tasks_.insert(
+      std::make_pair(extension_id, linked_ptr<PendingTasksList>(tasks_list)));
+  if (!ret.second) {  // Insert failed.
+    // First part of return value is a pointer to what was already there.
+    tasks_list = ret.first->second.get();
+  }
+
+  // Append the new task.
+  tasks_list->push_back(linked_ptr<Task>(
+      task_factory_.NewRunnableMethod(&ExtensionEventRouter::DispatchEventImpl,
+                                      extension_id, event_name, event_args,
+                                      restrict_to_profile, cross_incognito_args,
+                                      event_url, true)));
+}
+
+void ExtensionEventRouter::DispatchPendingEvents(
+    const std::string &extension_id) {
+  // Find the list of pending tasks for this extension.
+  PendingTasksPerExtMap::const_iterator map_it =
+      pending_tasks_.find(extension_id);
+  if (map_it == pending_tasks_.end())
+    return;
+
+  // The second element is the list of tasks (the first is the key/ext id).

[Aaron Boodman, 2011/08/23 22:41:16]

Nit: You don't have to explain this since it is clear from the type of the
container.

[Tessa MacDuff, 2011/08/24 00:34:23]

Done.

+  PendingTasksList* tasks_list = map_it->second.get();
+  for (PendingTasksList::const_iterator it = tasks_list->begin();
+       it != tasks_list->end(); ++it)
+    it->get()->Run();
+
+  // Delete list.
+  tasks_list->clear();
+  pending_tasks_.erase(extension_id);
+}
+
 void ExtensionEventRouter::Observe(int type,
                                    const NotificationSource& source,
                                    const NotificationDetails& details) {
@@ -243,6 +329,13 @@ void ExtensionEventRouter::Observe(int type,
       }
       break;
     }
+    case chrome::NOTIFICATION_EXTENSION_HOST_DID_STOP_LOADING: {
+      // TODO: dispatch events in queue.  ExtensionHost is in the details.
+      ExtensionHost* eh = Details<ExtensionHost>(details).ptr();
+      DispatchPendingEvents(eh->extension_id());
+      break;
+    }
+    // TODO(tessamac): if background page crashed/failed clear queue.
     default:
       NOTREACHED();
       return;