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..a33e1c8fb04f9014ecb6e75e92323ba7d1ebf9e1 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"
@@ -48,6 +51,20 @@ struct ExtensionEventRouter::EventListener {
   }
 };
 
+struct ExtensionEventRouter::PendingEvent {
+  std::string name;
+  std::string args;
+  Profile* restrict_to_profile;
+  std::string cross_incognito_args;
+  GURL url;
+
+  PendingEvent(const std::string& name, const std::string& args,
+               Profile* restrict_to_profile,
+               const std::string& cross_incognito_args, const GURL& url)
+      : name(name), args(args), restrict_to_profile(restrict_to_profile),
+        cross_incognito_args(cross_incognito_args), url(url) {}
+};
+
 // static
 void ExtensionEventRouter::DispatchEvent(IPC::Message::Sender* ipc_sender,
                                          const std::string& extension_id,
@@ -68,6 +85,9 @@ ExtensionEventRouter::ExtensionEventRouter(Profile* profile)
                  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() {
@@ -142,8 +162,8 @@ void ExtensionEventRouter::DispatchEventToRenderers(
     const std::string& event_args,
     Profile* restrict_to_profile,
     const GURL& event_url) {
-  DispatchEventImpl("", event_name, event_args, restrict_to_profile, "",
-                    event_url);
+  MaybeDispatchEventImpl("", event_name, event_args, restrict_to_profile, "",
+                         event_url);
 }
 
 void ExtensionEventRouter::DispatchEventToExtension(
@@ -153,8 +173,8 @@ void ExtensionEventRouter::DispatchEventToExtension(
     Profile* restrict_to_profile,
     const GURL& event_url) {
   DCHECK(!extension_id.empty());
-  DispatchEventImpl(extension_id, event_name, event_args, restrict_to_profile,
-                    "", event_url);
+  MaybeDispatchEventImpl(extension_id, event_name, event_args,
+                         restrict_to_profile, "", event_url);
 }
 
 void ExtensionEventRouter::DispatchEventsToRenderersAcrossIncognito(
@@ -163,7 +183,40 @@ void ExtensionEventRouter::DispatchEventsToRenderersAcrossIncognito(
     Profile* restrict_to_profile,
     const std::string& cross_incognito_args,
     const GURL& event_url) {
-  DispatchEventImpl("", event_name, event_args, restrict_to_profile,
+  MaybeDispatchEventImpl("", event_name, event_args, restrict_to_profile,
+                         cross_incognito_args, event_url);
+}
+
+void ExtensionEventRouter::MaybeDispatchEventImpl(

[Aaron Boodman, 2011/08/18 19:25:55]

Another 'maybe' function. You could eliminate this one by putting the code
directly in DispatchEventImpl. The logic would basically be:

<snip>
if (CanDispatchEventNow()) {
  StartBackgroundPage();
  Queue();
  return;
}
</snip>

You can factor the checking of whether the background page needs to be created
into CanDispatchEventNow() without adding too much to DispatchEventImpl().

[Tessa MacDuff, 2011/08/23 18:18:42]

Done.

+    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) {
+  if (!profile_)
+    return;
+
+  if (CommandLine::ForCurrentProcess()->HasSwitch(
+          switches::kEnableLazyBackgroundPages) &&
+      !extension_id.empty()) {
+    // TODO(tessamac): Decide what to when there's no extension id.
+    //                 Create all background pages?

[Aaron Boodman, 2011/08/18 19:25:55]

I think creating all background pages is the right answer for now. It might need
a little refactoring to handle cleanly. You could do that separately if you
want.

[Tessa MacDuff, 2011/08/23 18:18:42]

I think I'll do this in a separate CL as since it will take a bit of refactoring
and it will be much more interesting when I can shut background pages down.

+    const Extension* extension = profile_->GetExtensionService()->
+        GetExtensionById(extension_id, false);  // exclude disable extensions
+    if (extension && extension->background_url().is_valid()) {
+      ExtensionProcessManager* pm = profile_->GetExtensionProcessManager();
+      if (pm->GetBackgroundHostForExtension(extension) == NULL) {
+        EnqueueEvent(extension_id, event_name, event_args, restrict_to_profile,
+                     cross_incognito_args, event_url);
+        pm->CreateBackgroundHost(extension, extension->background_url());
+        return;
+      }
+    }
+
+  }
+
+  DispatchEventImpl(extension_id, event_name, event_args, restrict_to_profile,
                     cross_incognito_args, event_url);
 }
 
@@ -220,6 +273,35 @@ void ExtensionEventRouter::DispatchEventImpl(
   }
 }
 
+void ExtensionEventRouter::EnqueueEvent(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) {
+  PendingEvent event(event_name, event_args, restrict_to_profile,
+                     cross_incognito_args, event_url);
+  // TODO: use insert instead of [].
+  pending_events_[extension_id].push_back(event);

[Aaron Boodman, 2011/08/18 19:25:55]

I'm fine with this for now, but just so you know the task.h stuff is actually
very easy to use and would eliminate some of this manual labor.

It would allow you to do basically:

pending_events_[extension_id].push_back(
  factory_.NewRunnableMethod(
    this,
    &ExtensionEventRouter::DispatchEventImpl,
    extensin_id,
    event_name,
    event_args,
    ... etc ..
    );

and then to dispatch them, just loop through and call Run() on each one. No need
for a separate struct. New RunnableMethod() returns a Task that essentially
replaces your struct.

Basically ScopedRunnableMethodFactory.NewRunnableMethod() allows you to say 'run
this method with these arguments "later"'.

We use them in lots of places in the extension system if you want to have a
look.

[Tessa MacDuff, 2011/08/23 18:18:42]

Done.

+}
+
+void ExtensionEventRouter::ClearPendingEvents(const std::string &extension_id) {
+  pending_events_[extension_id].clear();

[Aaron Boodman, 2011/08/18 19:29:06]

Little nit: If these one-liner methods aren't called more than one place in the
code, I'd probably just put the code at the call site. It's not saving any
complexity to pull out one line.

[Tessa MacDuff, 2011/08/23 18:18:42]

Done.

I had to add an additional NewRunnableMethod template to task.h to accomodate 7
args.  Let me know if you'd rather I put it in a separate CL or if you'd like me
to create a single DispatchEventParams object.

+}
+
+void ExtensionEventRouter::DispatchPendingEvents(
+    const std::string &extension_id) {
+  const PendingEventsQueue& queue = pending_events_[extension_id];
+  for (PendingEventsQueue::const_iterator it = queue.begin();
+       it != queue.end(); ++it) {
+    // Don't call MaybeDispatchEventImpl() to guard against these events
+    // being re-enqueued if the Background Page still isn't all set.

[Aaron Boodman, 2011/08/18 19:29:06]

Oh, I see. I don't think it's a big deal if they get re-enqueued. It simplifies
the code and it's not like we are going to loop. Once the extension loads, if
EPM doesn't have it for some reason, we have a serious implementation bug and
lots of things are broken.

[Tessa MacDuff, 2011/08/18 20:49:39]

I was worried about what would happen if we were adding items to pending_events_
as we were iterating thought it.   Seems like it could infinite loop.  If you'd
prefer, I can pass an extra arg that indicates this event was waiting on the
background page so we can failure if it's still not there.

Although this case might see unlikely know, it could happen when i start trying
to shutdown background pages (i.e. if I shut them down too early).

[Aaron Boodman, 2011/08/18 21:19:23]

ok (to extra arg).

[Tessa MacDuff, 2011/08/23 18:18:42]

Done.

+    DispatchEventImpl(extension_id, it->name, it->args, it->restrict_to_profile,
+                       it->cross_incognito_args, it->url);
+  }
+  ClearPendingEvents(extension_id);
+}
+
 void ExtensionEventRouter::Observe(int type,
                                    const NotificationSource& source,
                                    const NotificationDetails& details) {
@@ -243,6 +325,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;