Remove NotificationService notifications from WebNavigationApi.

Bug: 268984
Change-Id: Idd1b13ad08f66f029706a1b04ad811aa148e29a1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1719275
Commit-Queue: Evan Stade <estade@chromium.org>
Reviewed-by: Istiaque Ahmed <lazyboy@chromium.org>
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#686246}

chrome/browser/extensions/api/web_navigation/web_navigation_api.cc

@@ -9,7 +9,6 @@
 #include <memory>
 
 #include "base/no_destructor.h"
-#include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/extensions/api/web_navigation/web_navigation_api_constants.h"
 #include "chrome/browser/extensions/api/web_navigation/web_navigation_api_helpers.h"
 #include "chrome/browser/extensions/extension_tab_util.h"
@@ -19,8 +18,6 @@
 #include "chrome/common/extensions/api/web_navigation.h"
 #include "content/public/browser/navigation_details.h"
 #include "content/public/browser/navigation_handle.h"
-#include "content/public/browser/notification_service.h"
-#include "content/public/browser/notification_types.h"
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/render_process_host.h"
 #include "content/public/browser/web_contents.h"
@@ -51,41 +48,36 @@ TabObserverMap& GetTabObserverMap() {
 
 // WebNavigtionEventRouter -------------------------------------------
 
-WebNavigationEventRouter::PendingWebContents::PendingWebContents()
-    : source_web_contents(NULL),
-      source_frame_host(NULL),
-      target_web_contents(NULL),
-      target_url() {
-}
+WebNavigationEventRouter::PendingWebContents::PendingWebContents() = default;
+WebNavigationEventRouter::PendingWebContents::~PendingWebContents() {}
 
-WebNavigationEventRouter::PendingWebContents::PendingWebContents(
-    content::WebContents* source_web_contents,
-    content::RenderFrameHost* source_frame_host,
+void WebNavigationEventRouter::PendingWebContents::Set(
+    int source_tab_id,
+    int source_render_process_id,
+    int source_extension_frame_id,
     content::WebContents* target_web_contents,
-    const GURL& target_url)
-    : source_web_contents(source_web_contents),
-      source_frame_host(source_frame_host),
-      target_web_contents(target_web_contents),
-      target_url(target_url) {
+    const GURL& target_url,
+    base::OnceCallback<void(content::WebContents*)> on_destroy) {
+  Observe(target_web_contents);
+  source_tab_id_ = source_tab_id;
+  source_render_process_id_ = source_render_process_id;
+  source_extension_frame_id_ = source_extension_frame_id;
+  target_web_contents_ = target_web_contents;
+  target_url_ = target_url;
+  on_destroy_ = std::move(on_destroy);
 }
 
-WebNavigationEventRouter::PendingWebContents::~PendingWebContents() {}
+void WebNavigationEventRouter::PendingWebContents::WebContentsDestroyed() {
+  std::move(on_destroy_).Run(target_web_contents_);
+  // |this| is deleted!
+}
 
 WebNavigationEventRouter::WebNavigationEventRouter(Profile* profile)
     : profile_(profile), browser_tab_strip_tracker_(this, this, nullptr) {
-  CHECK(registrar_.IsEmpty());
-  registrar_.Add(this,
-                 chrome::NOTIFICATION_TAB_ADDED,
-                 content::NotificationService::AllSources());
-  registrar_.Add(this,
-                 content::NOTIFICATION_WEB_CONTENTS_DESTROYED,
-                 content::NotificationService::AllSources());
-
   browser_tab_strip_tracker_.Init();
 }
 
-WebNavigationEventRouter::~WebNavigationEventRouter() {
-}
+WebNavigationEventRouter::~WebNavigationEventRouter() = default;
 
 bool WebNavigationEventRouter::ShouldTrackBrowser(Browser* browser) {
   return profile_->IsSameProfile(browser->profile());
@@ -95,42 +87,26 @@ void WebNavigationEventRouter::OnTabStripModelChanged(
     TabStripModel* tab_strip_model,
     const TabStripModelChange& change,
     const TabStripSelectionChange& selection) {
-  if (change.type() != TabStripModelChange::kReplaced)
-    return;
-
-  auto* replace = change.GetReplace();
-  WebNavigationTabObserver* tab_observer =
-      WebNavigationTabObserver::Get(replace->old_contents);
-
-  if (!tab_observer) {
-    // If you hit this DCHECK(), please add reproduction steps to
-    // http://crbug.com/109464.
-    DCHECK(GetViewType(replace->old_contents) != VIEW_TYPE_TAB_CONTENTS);
-    return;
-  }
-  if (!FrameNavigationState::IsValidUrl(replace->old_contents->GetURL()) ||
-      !FrameNavigationState::IsValidUrl(replace->new_contents->GetURL()))
-    return;
-
-  web_navigation_api_helpers::DispatchOnTabReplaced(
-      replace->old_contents, profile_, replace->new_contents);
-}
-
-void WebNavigationEventRouter::Observe(
-    int type,
-    const content::NotificationSource& source,
-    const content::NotificationDetails& details) {
-  switch (type) {
-    case chrome::NOTIFICATION_TAB_ADDED:
-      TabAdded(content::Details<content::WebContents>(details).ptr());
-      break;
-
-    case content::NOTIFICATION_WEB_CONTENTS_DESTROYED:
-      TabDestroyed(content::Source<content::WebContents>(source).ptr());
-      break;
-
-    default:
-      NOTREACHED();
+  if (change.type() == TabStripModelChange::kReplaced) {
+    auto* replace = change.GetReplace();
+    WebNavigationTabObserver* tab_observer =
+        WebNavigationTabObserver::Get(replace->old_contents);
+
+    if (!tab_observer) {
+      // If you hit this DCHECK(), please add reproduction steps to
+      // http://crbug.com/109464.
+      DCHECK(GetViewType(replace->old_contents) != VIEW_TYPE_TAB_CONTENTS);
+      return;
+    }
+    if (!FrameNavigationState::IsValidUrl(replace->old_contents->GetURL()) ||
+        !FrameNavigationState::IsValidUrl(replace->new_contents->GetURL()))
+      return;
+
+    web_navigation_api_helpers::DispatchOnTabReplaced(
+        replace->old_contents, profile_, replace->new_contents);
+  } else if (change.type() == TabStripModelChange::kInserted) {
+    for (auto& tab : change.GetInsert()->contents)
+      TabAdded(tab.contents);
   }
 }
 
@@ -159,15 +135,23 @@ void WebNavigationEventRouter::RecordNewWebContents(
   if (!frame_navigation_state.CanSendEvents(frame_host))
     return;
 
+  int source_extension_frame_id =
+      ExtensionApiFrameIdMap::GetFrameId(frame_host);
+  int source_tab_id = ExtensionTabUtil::GetTabId(source_web_contents);
+
   // If the WebContents isn't yet inserted into a tab strip, we need to delay
   // the extension event until the WebContents is fully initialized.
   if (not_yet_in_tabstrip) {
-    pending_web_contents_[target_web_contents] = PendingWebContents(
-        source_web_contents, frame_host, target_web_contents, target_url);
+    pending_web_contents_[target_web_contents].Set(
+        source_tab_id, source_render_process_id, source_extension_frame_id,
+        target_web_contents, target_url,
+        base::BindOnce(&WebNavigationEventRouter::PendingWebContentsDestroyed,
+                       base::Unretained(this)));
   } else {
     web_navigation_api_helpers::DispatchOnCreatedNavigationTarget(
-        source_web_contents, target_web_contents->GetBrowserContext(),
-        frame_host, target_web_contents, target_url);
+        source_tab_id, source_render_process_id, source_extension_frame_id,
+        target_web_contents->GetBrowserContext(), target_web_contents,
+        target_url);
   }
 }
 
@@ -176,34 +160,18 @@ void WebNavigationEventRouter::TabAdded(content::WebContents* tab) {
   if (iter == pending_web_contents_.end())
     return;
 
-  WebNavigationTabObserver* tab_observer =
-      WebNavigationTabObserver::Get(iter->second.source_web_contents);
-  if (!tab_observer) {
-    NOTREACHED();
-    return;
-  }
-  const FrameNavigationState& frame_navigation_state =
-      tab_observer->frame_navigation_state();
-
-  if (frame_navigation_state.CanSendEvents(iter->second.source_frame_host)) {
-    web_navigation_api_helpers::DispatchOnCreatedNavigationTarget(
-        iter->second.source_web_contents,
-        iter->second.target_web_contents->GetBrowserContext(),
-        iter->second.source_frame_host, iter->second.target_web_contents,
-        iter->second.target_url);
-  }
+  const PendingWebContents& pending_tab = iter->second;
+  web_navigation_api_helpers::DispatchOnCreatedNavigationTarget(
+      pending_tab.source_tab_id(), pending_tab.source_render_process_id(),
+      pending_tab.source_extension_frame_id(),
+      pending_tab.target_web_contents()->GetBrowserContext(),
+      pending_tab.target_web_contents(), pending_tab.target_url());
   pending_web_contents_.erase(iter);
 }
 
-void WebNavigationEventRouter::TabDestroyed(content::WebContents* tab) {
+void WebNavigationEventRouter::PendingWebContentsDestroyed(
+    content::WebContents* tab) {
   pending_web_contents_.erase(tab);
-  for (auto i = pending_web_contents_.begin();
-       i != pending_web_contents_.end();) {
-    if (i->second.source_web_contents == tab)
-      pending_web_contents_.erase(i++);
-    else
-      ++i;
-  }
 }
 
 // WebNavigationTabObserver ------------------------------------------
@@ -264,11 +232,11 @@ void WebNavigationTabObserver::DidStartNavigation(
   // and sent after the addition, to preserve the ordering of events.
   //
   // TODO(nasko|devlin): This check is necessary because chrome::Navigate()
-  // begins the navigation before the sending the TAB_ADDED notification, and it
+  // begins the navigation before adding the tab to the TabStripModel, and it
   // is used an indication of that. It would be best if instead it was known
   // when the tab was created and immediately sent the created event instead of
-  // waiting for the later TAB_ADDED notification, but this appears to work for
-  // now.
+  // waiting for the later TabStripModel kInserted change, but this appears to
+  // work for now.
   if (ExtensionTabUtil::GetTabById(ExtensionTabUtil::GetTabId(web_contents()),
                                    web_contents()->GetBrowserContext(), false,
                                    nullptr)) {
@@ -400,7 +368,6 @@ void WebNavigationTabObserver::DidOpenRequestedURL(
 
 void WebNavigationTabObserver::WebContentsDestroyed() {
   GetTabObserverMap().erase(web_contents());
-  registrar_.RemoveAll();
 }
 
 void WebNavigationTabObserver::DispatchCachedOnBeforeNavigate() {

chrome/browser/extensions/api/web_navigation/web_navigation_api.h

@@ -11,6 +11,7 @@
 #include <map>
 #include <set>
 
+#include "base/callback.h"
 #include "base/compiler_specific.h"
 #include "base/macros.h"
 #include "chrome/browser/extensions/api/web_navigation/frame_navigation_state.h"
@@ -19,8 +20,6 @@
 #include "chrome/browser/ui/browser_tab_strip_tracker.h"
 #include "chrome/browser/ui/browser_tab_strip_tracker_delegate.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
-#include "content/public/browser/notification_observer.h"
-#include "content/public/browser/notification_registrar.h"
 #include "content/public/browser/web_contents_observer.h"
 #include "content/public/browser/web_contents_user_data.h"
 #include "extensions/browser/browser_context_keyed_api_factory.h"
@@ -97,19 +96,14 @@ class WebNavigationTabObserver
   // the tab strip and is ready to dispatch events.
   std::unique_ptr<Event> pending_on_before_navigate_event_;
 
-  // Used for tracking registrations to redirect notifications.
-  content::NotificationRegistrar registrar_;
-
   WEB_CONTENTS_USER_DATA_KEY_DECL();
 
   DISALLOW_COPY_AND_ASSIGN(WebNavigationTabObserver);
 };
 
-// Observes navigation notifications and routes them as events to the extension
-// system.
+// Tracks new tab navigations and routes them as events to the extension system.
 class WebNavigationEventRouter : public TabStripModelObserver,
-                                 public BrowserTabStripTrackerDelegate,
-                                 public content::NotificationObserver {
+                                 public BrowserTabStripTrackerDelegate {
  public:
   explicit WebNavigationEventRouter(Profile* profile);
   ~WebNavigationEventRouter() override;
@@ -117,7 +111,7 @@ class WebNavigationEventRouter : public TabStripModelObserver,
   // Router level handler for the creation of WebContents. Stores information
   // about the newly created WebContents. This information is later used when
   // the WebContents for the tab is added to the tabstrip and we receive the
-  // TAB_ADDED notification.
+  // TabStripModelChanged insertion.
   void RecordNewWebContents(content::WebContents* source_web_contents,
                             int source_render_process_id,
                             int source_render_frame_id,
@@ -127,18 +121,42 @@ class WebNavigationEventRouter : public TabStripModelObserver,
 
  private:
   // Used to cache the information about newly created WebContents objects.
-  struct PendingWebContents{
+  // Will run |on_destroy_| if/when the target WebContents is destroyed.
+  class PendingWebContents : public content::WebContentsObserver {
+   public:
     PendingWebContents();
-    PendingWebContents(content::WebContents* source_web_contents,
-                       content::RenderFrameHost* source_frame_host,
-                       content::WebContents* target_web_contents,
-                       const GURL& target_url);
-    ~PendingWebContents();
-
-    content::WebContents* source_web_contents;
-    content::RenderFrameHost* source_frame_host;
-    content::WebContents* target_web_contents;
-    GURL target_url;
+    ~PendingWebContents() override;
+
+    void Set(int source_tab_id,
+             int source_render_process_id,
+             int source_extension_frame_id,
+             content::WebContents* target_web_contents,
+             const GURL& target_url,
+             base::OnceCallback<void(content::WebContents*)> on_destroy);
+
+    // content::WebContentsObserver:
+    void WebContentsDestroyed() override;
+
+    int source_tab_id() const { return source_tab_id_; }
+    int source_render_process_id() const { return source_render_process_id_; }
+    int source_extension_frame_id() const { return source_extension_frame_id_; }
+    content::WebContents* target_web_contents() const {
+      return target_web_contents_;
+    }
+    GURL target_url() const { return target_url_; }
+
+   private:
+    // The Extensions API ID for the source tab.
+    int source_tab_id_ = -1;
+    // The source frame's RenderProcessHost ID.
+    int source_render_process_id_ = -1;
+    // The Extensions API ID for the source frame.
+    int source_extension_frame_id_ = -1;
+    content::WebContents* target_web_contents_ = nullptr;
+    GURL target_url_;
+    base::OnceCallback<void(content::WebContents*)> on_destroy_;
+
+    DISALLOW_COPY_AND_ASSIGN(PendingWebContents);
   };
 
   // BrowserTabStripTrackerDelegate implementation.
@@ -150,26 +168,17 @@ class WebNavigationEventRouter : public TabStripModelObserver,
       const TabStripModelChange& change,
       const TabStripSelectionChange& selection) override;
 
-  // content::NotificationObserver implementation.
-  void Observe(int type,
-               const content::NotificationSource& source,
-               const content::NotificationDetails& details) override;
-
-  // Handler for the NOTIFICATION_TAB_ADDED event. The method takes the details
-  // of such an event and creates a JSON formated extension event from it.
+  // The method takes the details of such an event and creates a JSON formatted
+  // extension event from it.
   void TabAdded(content::WebContents* tab);
 
-  // Handler for NOTIFICATION_WEB_CONTENTS_DESTROYED. If |tab| is in
-  // |pending_web_contents_|, it is removed.
-  void TabDestroyed(content::WebContents* tab);
+  // Removes |tab| from |pending_web_contents_| if it is there.
+  void PendingWebContentsDestroyed(content::WebContents* tab);
 
   // Mapping pointers to WebContents objects to information about how they got
   // created.
   std::map<content::WebContents*, PendingWebContents> pending_web_contents_;
 
-  // Used for tracking registrations to navigation notifications.
-  content::NotificationRegistrar registrar_;
-
   // The profile that owns us via ExtensionService.
   Profile* profile_;
 

chrome/browser/extensions/api/web_navigation/web_navigation_api_helpers.cc

@@ -192,9 +192,10 @@ void DispatchOnCompleted(content::WebContents* web_contents,
 
 // Constructs and dispatches an onCreatedNavigationTarget event.
 void DispatchOnCreatedNavigationTarget(
-    content::WebContents* web_contents,
+    int source_tab_id,
+    int source_render_process_id,
+    int source_extension_frame_id,
     content::BrowserContext* browser_context,
-    content::RenderFrameHost* source_frame_host,
     content::WebContents* target_web_contents,
     const GURL& target_url) {
   // Check that the tab is already inserted into a tab strip model. This code
@@ -205,10 +206,9 @@ void DispatchOnCreatedNavigationTarget(
       false, nullptr));
 
   web_navigation::OnCreatedNavigationTarget::Details details;
-  details.source_tab_id = ExtensionTabUtil::GetTabId(web_contents);
-  details.source_process_id = source_frame_host->GetProcess()->GetID();
-  details.source_frame_id =
-      ExtensionApiFrameIdMap::GetFrameId(source_frame_host);
+  details.source_tab_id = source_tab_id;
+  details.source_process_id = source_render_process_id;
+  details.source_frame_id = source_extension_frame_id;
   details.url = target_url.possibly_invalid_spec();
   details.tab_id = ExtensionTabUtil::GetTabId(target_web_contents);
   details.time_stamp = MilliSecondsFromTime(base::Time::Now());

chrome/browser/extensions/api/web_navigation/web_navigation_api_helpers.h

@@ -48,9 +48,10 @@ void DispatchOnCompleted(content::WebContents* web_contents,
                          const GURL& url);
 
 void DispatchOnCreatedNavigationTarget(
-    content::WebContents* web_contents,
+    int source_tab_id,
+    int source_render_process_id,
+    int source_extension_frame_id,
     content::BrowserContext* browser_context,
-    content::RenderFrameHost* source_frame_host,
     content::WebContents* target_web_contents,
     const GURL& target_url);