Remove NOTIFICATION_EXTENSION_ENABLED.

extensions/browser/event_router.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 "extensions/browser/event_router.h"
 
 #include <stddef.h>
 
 #include <utility>
 
 #include "base/atomic_sequence_num.h"
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/lazy_instance.h"
 #include "base/memory/ptr_util.h"
 #include "base/message_loop/message_loop.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/stl_util.h"
 #include "base/values.h"
-#include "content/public/browser/notification_service.h"
 #include "content/public/browser/render_process_host.h"
 #include "extensions/browser/api_activity_monitor.h"
 #include "extensions/browser/event_router_factory.h"
 #include "extensions/browser/extension_host.h"
 #include "extensions/browser/extension_prefs.h"
 #include "extensions/browser/extension_registry.h"
 #include "extensions/browser/extension_system.h"
 #include "extensions/browser/extensions_browser_client.h"
 #include "extensions/browser/lazy_background_task_queue.h"
-#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"
 #include "extensions/common/extension_urls.h"
 #include "extensions/common/features/feature.h"
 #include "extensions/common/features/feature_provider.h"
 #include "extensions/common/manifest_handlers/background_info.h"
@@ skipping 97 matching lines @@
                            event_id, event_name, event_args.get(), user_gesture,
                            info);
 }
 
 EventRouter::EventRouter(BrowserContext* browser_context,
                          ExtensionPrefs* extension_prefs)
     : browser_context_(browser_context),
       extension_prefs_(extension_prefs),
       extension_registry_observer_(this),
       listeners_(this) {
-  registrar_.Add(this,
-                 extensions::NOTIFICATION_EXTENSION_ENABLED,
-                 content::Source<BrowserContext>(browser_context_));
   extension_registry_observer_.Add(ExtensionRegistry::Get(browser_context_));
 }
 
 EventRouter::~EventRouter() {
   for (auto* process : observed_process_set_)
     process->RemoveObserver(this);
 }
 
 void EventRouter::AddEventListener(const std::string& event_name,
                                    content::RenderProcessHost* process,
@@ skipping 585 matching lines @@
   ListValue* filter_list = nullptr;
   if (!filtered_events->GetListWithoutPathExpansion(event_name, &filter_list)) {
     filter_list = new ListValue;
     filtered_events->SetWithoutPathExpansion(event_name,
                                              base::WrapUnique(filter_list));
   }
 
   filter_list->Append(filter->CreateDeepCopy());
 }
 
-void EventRouter::Observe(int type,
-                          const content::NotificationSource& source,
-                          const content::NotificationDetails& details) {
-  switch (type) {
-    case extensions::NOTIFICATION_EXTENSION_ENABLED: {
-      // If the extension has a lazy background page, make sure it gets loaded
-      // to register the events the extension is interested in.
-      const Extension* extension =
-          content::Details<const Extension>(details).ptr();
-      if (BackgroundInfo::HasLazyBackgroundPage(extension)) {
-        LazyBackgroundTaskQueue* queue =
-            LazyBackgroundTaskQueue::Get(browser_context_);
-        queue->AddPendingTask(browser_context_, extension->id(),
-                              base::Bind(&DoNothing));
-      }
-      break;
-    }
-    default:
-      NOTREACHED();
-  }
-}
-
 void EventRouter::OnExtensionLoaded(content::BrowserContext* browser_context,
                                     const Extension* extension) {
   // Add all registered lazy listeners to our cache.
   std::set<std::string> registered_events =
       GetRegisteredEvents(extension->id());
   listeners_.LoadUnfilteredLazyListeners(extension->id(), registered_events);
   const DictionaryValue* filtered_events = GetFilteredEvents(extension->id());
   if (filtered_events)
     listeners_.LoadFilteredLazyListeners(extension->id(), *filtered_events);
 }
 
+void EventRouter::OnExtensionEnabled(content::BrowserContext* browser_context,

[Devlin, 2017/05/17 18:37:48]

Why do we do this for enabled, but not loaded?

[lazyboy, 2017/05/17 22:23:25]

This seems to be fixing
https://bugs.chromium.org/p/chromium/issues/detail?id=163379 by
https://chromiumcodereview.appspot.com/11348301.

The flow I've understood is:
v1 extension w/ event page does not have foo listener.
update to v2 extension with increased permission that has foo listener.
v2 is disabled due to permission increase.
Enable v2, OnExtensionLoaded does not run the event page. So we don't have any
clue that foo listener was ever registered.

[Devlin, 2017/05/18 01:34:45]

Ah, thanks for digging that up!  The motivation here now makes sense.  But, I'm
not convinced it's the right thing to do ;)

Why isn't this a problem for the flow of v1 -> v2, with no increased
permissions?  v2 could still have new events that need registering, so when do
we register those?  Why can't we do this in a similar location?  And, does this
mean there might be other underlying bugs with extension upgrade flow?  If we're
not going through the same path, perhaps we're also not dispatching events like
runtime.onInstalled() (which is also called for installing a new version).

[lazyboy, 2017/05/18 18:10:52]

We forcefully send an onInstalled in that case via
DispatchEventWithLazyListener:
https://cs.chromium.org/chromium/src/extensions/browser/api/runtime/runtime_a...
Note that it is using a combination of
OnExtensionWillBeInstalled + remember in pref + OnExtensionLoaded

So the event page will run and get all the new events.
  Why can't we do this in a similar location? 
Good point, suggest how that would look like? We definitely don't want to put
this logic
in RuntimeAPI.

 And, does this

[Devlin, 2017/05/18 18:39:47]

Oof, this is quite the rat's nest.

So, RuntimeAPI uses DispatchEventWithLazyListener(), which
//  Dispatches |event| to the given extension as if the extension has a lazy
// listener for it.

That's very odd.  (What's more, it might render this code unnecessary, since
that will load the background page whenever an update happens...)

At a high level, my proposal would be that when a new version of an extension is
loaded (which could be either initial installation, or update, or
update->disable->enable), we load the background page and register events. 
After that, we should have an accurate accounting of all events that are
registered, and APIs could dispatch normally.

Now, all that said, that will turn this into a very different CL.  I'd like to
do it at some point, and we should add TODOs or a bug for it, but I totally
understand if we want to hold off on that refactor for now (in which case the
general approach of this CL is probably fine).  Lemme know what you'd like to
do.

+                                     const Extension* extension) {
+  DCHECK_EQ(browser_context_, browser_context);
+  // If the extension has a lazy background page, make sure it gets loaded
+  // to register the events the extension is interested in.
+  if (BackgroundInfo::HasLazyBackgroundPage(extension)) {
+    LazyBackgroundTaskQueue* queue =
+        LazyBackgroundTaskQueue::Get(browser_context_);
+    queue->AddPendingTask(browser_context_, extension->id(),
+                          base::Bind(&DoNothing));
+  }
+}
+
 void EventRouter::OnExtensionUnloaded(content::BrowserContext* browser_context,
                                       const Extension* extension,
                                       UnloadedExtensionReason reason) {
   // Remove all registered listeners from our cache.
   listeners_.RemoveListenersForExtension(extension->id());
 }
 
 Event::Event(events::HistogramValue histogram_value,
              const std::string& event_name,
              std::unique_ptr<base::ListValue> event_args)
@@ skipping 49 matching lines @@
                                      const std::string& extension_id,
                                      const GURL& listener_url,
                                      content::BrowserContext* browser_context)
     : event_name(event_name),
       extension_id(extension_id),
       listener_url(listener_url),
       browser_context(browser_context) {
 }
 
 }  // namespace extensions