Add extension event histogram values for messaging, webRequest, and webview.

extensions/browser/event_router.cc

Index: extensions/browser/event_router.cc
diff --git a/extensions/browser/event_router.cc b/extensions/browser/event_router.cc
index e70d02b65a88c5d2cb1ee5cd19904d5958413246..d5019a6e955b3bc3742c2fda861f9ae53163c479 100644
--- a/extensions/browser/event_router.cc
+++ b/extensions/browser/event_router.cc
@@ -26,6 +26,7 @@
 #include "extensions/browser/notification_types.h"
 #include "extensions/browser/process_manager.h"
 #include "extensions/browser/process_map.h"
+#include "extensions/common/constants.h"
 #include "extensions/common/extension.h"
 #include "extensions/common/extension_api.h"
 #include "extensions/common/extension_messages.h"
@@ -147,25 +148,29 @@ std::string EventRouter::GetBaseEventName(const std::string& full_event_name) {
 }
 
 // static
-void EventRouter::DispatchEvent(IPC::Sender* ipc_sender,
-                                void* browser_context_id,
-                                const std::string& extension_id,
-                                const std::string& event_name,
-                                scoped_ptr<ListValue> event_args,
-                                UserGestureState user_gesture,
-                                const EventFilteringInfo& info) {
+void EventRouter::DispatchEventToSender(IPC::Sender* ipc_sender,
+                                        void* browser_context_id,
+                                        const GURL& url,
+                                        const std::string& extension_id,
+                                        events::HistogramValue histogram_value,
+                                        const std::string& event_name,
+                                        scoped_ptr<ListValue> event_args,
+                                        UserGestureState user_gesture,
+                                        const EventFilteringInfo& info) {
   int event_id = g_extension_event_id.GetNext();
 
-  if (!BrowserThread::CurrentlyOn(BrowserThread::UI)) {
+  if (BrowserThread::CurrentlyOn(BrowserThread::UI)) {
+    DoDispatchEventToSenderBookkeepingOnUI(browser_context_id, extension_id,
+                                           url, event_id, histogram_value,
+                                           event_name);
+  } else {
     // This is called from WebRequest API.
     // TODO(lazyboy): Skip this entirely: http://crbug.com/488747.
     BrowserThread::PostTask(
         BrowserThread::UI, FROM_HERE,
-        base::Bind(&EventRouter::IncrementInFlightEventsOnUI,
-                   browser_context_id, extension_id, event_id, event_name));
-  } else {
-    IncrementInFlightEventsOnUI(browser_context_id, extension_id, event_id,
-                                event_name);
+        base::Bind(&EventRouter::DoDispatchEventToSenderBookkeepingOnUI,
+                   browser_context_id, extension_id, url, event_id,
+                   histogram_value, event_name));
   }
 
   DispatchExtensionMessage(ipc_sender, browser_context_id, extension_id,
@@ -523,7 +528,7 @@ void EventRouter::DispatchEventImpl(const std::string& restrict_to_extension_id,
         if (!ContainsKey(already_dispatched, dispatch_id)) {
           DispatchEventToProcess(listener->extension_id(),
                                  listener->listener_url(), listener->process(),
-                                 event, listener->filter());
+                                 event, listener->filter(), false);

[Devlin, 2015/08/05 21:34:09]

there are a few more of the anonymous bools than I'd like.  Can you document
them with the typical /* didn't enqueue */?

[not at google - send to devlin, 2015/08/07 20:55:03]

Sure.

         }
       }
     }
@@ -567,7 +572,8 @@ void EventRouter::DispatchEventToProcess(
     const GURL& listener_url,
     content::RenderProcessHost* process,
     const linked_ptr<Event>& event,
-    const base::DictionaryValue* listener_filter) {
+    const base::DictionaryValue* listener_filter,
+    bool did_enqueue) {
   BrowserContext* listener_context = process->GetBrowserContext();
   ProcessMap* process_map = ProcessMap::Get(listener_context);
 
@@ -635,6 +641,7 @@ void EventRouter::DispatchEventToProcess(
   DispatchExtensionMessage(process, listener_context, extension_id, event_id,
                            event->event_name, event->event_args.get(),
                            event->user_gesture, event->filter_info);
+  ReportEvent(event->histogram_value, listener_url, did_enqueue);
 
   if (extension) {
     IncrementInFlightEvents(listener_context, extension, event_id,
@@ -693,25 +700,27 @@ bool EventRouter::MaybeLoadLazyBackgroundPageToDispatchEvent(
 }
 
 // static
-void EventRouter::IncrementInFlightEventsOnUI(void* browser_context_id,
-                                              const std::string& extension_id,
-                                              int event_id,
-                                              const std::string& event_name) {
+void EventRouter::DoDispatchEventToSenderBookkeepingOnUI(
+    void* browser_context_id,
+    const std::string& extension_id,
+    const GURL& url,
+    int event_id,
+    events::HistogramValue histogram_value,
+    const std::string& event_name) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   BrowserContext* browser_context =
       reinterpret_cast<BrowserContext*>(browser_context_id);
   if (!ExtensionsBrowserClient::Get()->IsValidContext(browser_context))
     return;
-  EventRouter* event_router = EventRouter::Get(browser_context);
-  if (!event_router)
-    return;
   const Extension* extension =
       ExtensionRegistry::Get(browser_context)->enabled_extensions().GetByID(
           extension_id);
   if (!extension)
     return;
+  EventRouter* event_router = EventRouter::Get(browser_context);
   event_router->IncrementInFlightEvents(browser_context, extension, event_id,
                                         event_name);
+  event_router->ReportEvent(histogram_value, url, false);
 }
 
 void EventRouter::IncrementInFlightEvents(BrowserContext* context,
@@ -747,6 +756,60 @@ void EventRouter::OnEventAck(BrowserContext* context,
     pm->DecrementLazyKeepaliveCount(host->extension());
 }
 
+void EventRouter::ReportEvent(events::HistogramValue histogram_value,
+                              const GURL& url,
+                              bool did_enqueue) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+
+  // At this point, we're only interested in reporting events for extensions.
+  if (!url.SchemeIs(kExtensionScheme))

[Devlin, 2015/08/05 21:34:09]

Doesn't this mean we won't log any events for messaging?  Or webRequest?

[not at google - send to devlin, 2015/08/07 20:55:03]

The URL was the target URL which assuming messaging works fine will be correct.
I've changed this to be an extension now though, so moot.

+    return;
+
+  ExtensionRegistry* registry = ExtensionRegistry::Get(browser_context_);
+  const Extension* extension =
+      registry->enabled_extensions().GetExtensionOrAppByURL(url);
+  if (!extension) {
+    NOTREACHED() << "Reporting event for non-existent extension at " << url;

[Devlin, 2015/08/05 21:34:09]

Doing a NOTREACHED(); return; is typically a discouraged (anti)pattern, with the
exception of IPCs.  Is there a reason not to just DCHECK (or CHECK, if you like)
here?

[not at google - send to devlin, 2015/08/07 20:55:03]

Deliberate choice - I don't trust call sites, especially webview, but I want the
tests to fail at least. Maybe once this patch has baked this could be upgraded
to a CHECK.

+    return;
+  }
+
+  // TODO(kalman): UMA for dispatched event.
+  // TODO(kalman): UMA specifically for component extensions.

[not at google - send to devlin, 2015/08/05 20:23:43]

I will add the UMA_HISTOGRAM_ENUMERATION calls in a follow up, to reduce the
size of this change, and isolate the conversation about that. As you can see all
the info that I plan on using has been plumbed into here.

+
+  switch (GetExtensionPageType(extension, url, did_enqueue)) {
+    case EXTENSION_PAGE_NONE:
+      break;
+    case EXTENSION_PAGE_VIEW:
+      // TODO(kalman): UMA for dispatched event to view.
+      break;
+    case EXTENSION_PAGE_PERSISTENT_BACKGROUND:
+      // TODO(kalman): UMA for dispatched event to persistent background page.
+      break;
+    case EXTENSION_PAGE_DORMANT_EVENT:
+      // TODO(kalman): UMA for dispatched event that woke up event page.
+      break;
+    case EXTENSION_PAGE_AWAKE_EVENT:
+      // TODO(kalman): UMA for dispatched event to awake up event page.
+      break;
+  }
+}
+
+EventRouter::ExtensionPageType EventRouter::GetExtensionPageType(
+    const Extension* extension,
+    const GURL& url,
+    bool did_enqueue) {
+  if (!extension)

[Devlin, 2015/08/05 21:34:09]

When could this happen?

[not at google - send to devlin, 2015/08/07 20:55:03]

In a previous version of the patch, I guess - and it was a great find, because
in removing it, it led to nice CL-wide change. Instead of passing the URL in
here, I'm just going to pass the Extension, which makes this simpler and less
error-prone since it's explicitly only logging Extension events.

+    return EXTENSION_PAGE_NONE;
+  if (url == BackgroundInfo::GetBackgroundURL(extension)) {
+    if (BackgroundInfo::HasPersistentBackgroundPage(extension))
+      return EXTENSION_PAGE_PERSISTENT_BACKGROUND;
+    if (BackgroundInfo::HasLazyBackgroundPage(extension))
+      return did_enqueue ? EXTENSION_PAGE_DORMANT_EVENT
+                         : EXTENSION_PAGE_AWAKE_EVENT;
+  }
+  return EXTENSION_PAGE_VIEW;
+}
+
 void EventRouter::DispatchPendingEvent(const linked_ptr<Event>& event,
                                        ExtensionHost* host) {
   if (!host)
@@ -756,7 +819,7 @@ void EventRouter::DispatchPendingEvent(const linked_ptr<Event>& event,
                                     host->extension()->id())) {
     // URL events cannot be lazy therefore can't be pending, hence the GURL().
     DispatchEventToProcess(host->extension()->id(), GURL(),
-                           host->render_process_host(), event, nullptr);
+                           host->render_process_host(), event, nullptr, true);
   }
 }