Remove NOTIFICATION_RENDER_VIEW_HOST_CHANGED.

Use WebContentsObserver::RenderViewHostChanged() instead.

BUG=170921
Test=No behavioral change.

Sanity checked with JS speech, whether WebContentsTracker class gets created and destroyed properly.

Review URL: https://codereview.chromium.org/1132163004

Cr-Commit-Position: refs/heads/master@{#330213}

chrome/browser/file_select_helper.cc

@@ -292,6 +292,16 @@ void FileSelectHelper::DeleteTemporaryFiles() {
   temporary_files_.clear();
 }
 
+void FileSelectHelper::CleanUpOnRenderViewHostChange() {
+  if (!temporary_files_.empty()) {
+    DeleteTemporaryFiles();
+
+    // Now that the temporary files have been scheduled for deletion, there
+    // is no longer any reason to keep this instance around.
+    Release();
+  }
+}
+
 scoped_ptr<ui::SelectFileDialog::FileTypeInfo>
 FileSelectHelper::GetFileTypesFromAcceptType(
     const std::vector<base::string16>& accept_types) {
@@ -390,16 +400,11 @@ void FileSelectHelper::RunFileChooser(RenderViewHost* render_view_host,
   render_view_host_ = render_view_host;
   web_contents_ = web_contents;
   notification_registrar_.RemoveAll();
-  notification_registrar_.Add(this,
-                              content::NOTIFICATION_RENDER_VIEW_HOST_CHANGED,
-                              content::Source<WebContents>(web_contents_));
+  content::WebContentsObserver::Observe(web_contents_);
   notification_registrar_.Add(
       this,
       content::NOTIFICATION_RENDER_WIDGET_HOST_DESTROYED,
       content::Source<RenderWidgetHost>(render_view_host_));
-  notification_registrar_.Add(
-      this, content::NOTIFICATION_WEB_CONTENTS_DESTROYED,
-      content::Source<WebContents>(web_contents_));
 
   BrowserThread::PostTask(
       BrowserThread::FILE, FROM_HERE,
@@ -534,29 +539,21 @@ void FileSelectHelper::Observe(int type,
       render_view_host_ = NULL;
       break;
     }
-
-    case content::NOTIFICATION_WEB_CONTENTS_DESTROYED: {
-      DCHECK(content::Source<WebContents>(source).ptr() == web_contents_);
-      web_contents_ = NULL;
-    }
-
-    // Intentional fall through.
-    case content::NOTIFICATION_RENDER_VIEW_HOST_CHANGED:
-      if (!temporary_files_.empty()) {
-        DeleteTemporaryFiles();
-
-        // Now that the temporary files have been scheduled for deletion, there
-        // is no longer any reason to keep this instance around.
-        Release();
-      }
-
-      break;
-
     default:
       NOTREACHED();
   }
 }
 
+void FileSelectHelper::RenderViewHostChanged(RenderViewHost* old_host,
+                                             RenderViewHost* new_host) {
+  CleanUpOnRenderViewHostChange();
+}
+
+void FileSelectHelper::WebContentsDestroyed() {
+  web_contents_ = nullptr;
+  CleanUpOnRenderViewHostChange();
+}
+
 // static
 bool FileSelectHelper::IsAcceptTypeValid(const std::string& accept_type) {
   // TODO(raymes): This only does some basic checks, extend to test more cases.

chrome/browser/file_select_helper.h

@@ -12,6 +12,7 @@
 #include "base/gtest_prod_util.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/common/file_chooser_params.h"
 #include "net/base/directory_lister.h"
 #include "ui/shell_dialogs/select_file_dialog.h"
@@ -31,10 +32,10 @@ struct SelectedFileInfo;
 // This class handles file-selection requests coming from WebUI elements
 // (via the extensions::ExtensionHost class). It implements both the
 // initialisation and listener functions for file-selection dialogs.
-class FileSelectHelper
-    : public base::RefCountedThreadSafe<FileSelectHelper>,
-      public ui::SelectFileDialog::Listener,
-      public content::NotificationObserver {
+class FileSelectHelper : public base::RefCountedThreadSafe<FileSelectHelper>,
+                         public ui::SelectFileDialog::Listener,
+                         public content::WebContentsObserver,
+                         public content::NotificationObserver {
  public:
 
   // Show the file chooser dialog.
@@ -105,6 +106,11 @@ class FileSelectHelper
                const content::NotificationSource& source,
                const content::NotificationDetails& details) override;
 
+  // content::WebContentsObserver overrides.
+  void RenderViewHostChanged(content::RenderViewHost* old_host,
+                             content::RenderViewHost* new_host) override;
+  void WebContentsDestroyed() override;
+
   void EnumerateDirectory(int request_id,
                           content::RenderViewHost* render_view_host,
                           const base::FilePath& path);
@@ -155,6 +161,9 @@ class FileSelectHelper
   // vector.
   void DeleteTemporaryFiles();
 
+  // Cleans up when the RenderViewHost of our WebContents changes.
+  void CleanUpOnRenderViewHostChange();
+
   // Helper method to get allowed extensions for select file dialog from
   // the specified accept types as defined in the spec:
   //   http://whatwg.org/html/number-state.html#attr-input-accept

chrome/browser/speech/chrome_speech_recognition_manager_delegate.cc

@@ -18,9 +18,6 @@
 #include "chrome/common/pref_names.h"
 #include "chrome/common/url_constants.h"
 #include "content/public/browser/browser_thread.h"
-#include "content/public/browser/notification_registrar.h"
-#include "content/public/browser/notification_source.h"
-#include "content/public/browser/notification_types.h"
 #include "content/public/browser/render_process_host.h"
 #include "content/public/browser/render_view_host.h"
 #include "content/public/browser/resource_context.h"
@@ -28,6 +25,7 @@
 #include "content/public/browser/speech_recognition_session_config.h"
 #include "content/public/browser/speech_recognition_session_context.h"
 #include "content/public/browser/web_contents.h"
+#include "content/public/browser/web_contents_observer.h"
 #include "content/public/common/speech_recognition_error.h"
 #include "content/public/common/speech_recognition_result.h"
 #include "net/url_request/url_request_context_getter.h"
@@ -137,8 +135,7 @@ class ChromeSpeechRecognitionManagerDelegate::OptionalRequestInfo
 // There is no restriction on the constructor, however this class must be
 // destroyed on the UI thread, due to the NotificationRegistrar dependency.
 class ChromeSpeechRecognitionManagerDelegate::TabWatcher
-    : public base::RefCountedThreadSafe<TabWatcher>,
-      public content::NotificationObserver {
+    : public base::RefCountedThreadSafe<TabWatcher> {
  public:
   typedef base::Callback<void(int render_process_id, int render_view_id)>
       TabClosedCallback;
@@ -168,96 +165,91 @@ class ChromeSpeechRecognitionManagerDelegate::TabWatcher
     if (!web_contents)
       return;
 
-    // Avoid multiple registrations on |registrar_| for the same |web_contents|.
+    // Avoid multiple registrations for the same |web_contents|.
     if (FindWebContents(web_contents) !=  registered_web_contents_.end()) {
       return;
     }
-    registered_web_contents_.push_back(
-        WebContentsInfo(web_contents, render_process_id, render_view_id));
-
-    // Lazy initialize the registrar.
-    if (!registrar_.get())
-      registrar_.reset(new content::NotificationRegistrar());
-
-    registrar_->Add(this,
-                    content::NOTIFICATION_RENDER_VIEW_HOST_CHANGED,
-                    content::Source<WebContents>(web_contents));
-    registrar_->Add(this,
-                    content::NOTIFICATION_WEB_CONTENTS_DISCONNECTED,
-                    content::Source<WebContents>(web_contents));
+    registered_web_contents_.push_back(new WebContentsTracker(
+        web_contents, base::Bind(&TabWatcher::OnTabClosed,
+                                 // |this| outlives WebContentsTracker.
+                                 base::Unretained(this), web_contents),
+        render_process_id, render_view_id));
   }
 
-  // content::NotificationObserver implementation.
-  void Observe(int type,
-               const content::NotificationSource& source,
-               const content::NotificationDetails& details) override {
-    DCHECK_CURRENTLY_ON(BrowserThread::UI);
-    DCHECK(type == content::NOTIFICATION_WEB_CONTENTS_DISCONNECTED ||
-           type == content::NOTIFICATION_RENDER_VIEW_HOST_CHANGED);
-
-    WebContents* web_contents = content::Source<WebContents>(source).ptr();
-    std::vector<WebContentsInfo>::iterator iter = FindWebContents(web_contents);
+  void OnTabClosed(content::WebContents* web_contents) {
+    ScopedVector<WebContentsTracker>::iterator iter =
+        FindWebContents(web_contents);
     DCHECK(iter != registered_web_contents_.end());
-    int render_process_id = iter->render_process_id;
-    int render_view_id = iter->render_view_id;
+    int render_process_id = (*iter)->render_process_id();
+    int render_view_id = (*iter)->render_view_id();
     registered_web_contents_.erase(iter);
 
-    registrar_->Remove(this,
-                       content::NOTIFICATION_RENDER_VIEW_HOST_CHANGED,
-                       content::Source<WebContents>(web_contents));
-    registrar_->Remove(this,
-                       content::NOTIFICATION_WEB_CONTENTS_DISCONNECTED,
-                       content::Source<WebContents>(web_contents));
-
     tab_closed_callback_.Run(render_process_id, render_view_id);
   }
 
  private:
-  struct WebContentsInfo {
-    WebContentsInfo(content::WebContents* web_contents,
-                    int render_process_id,
-                    int render_view_id)
-        : web_contents(web_contents),
-          render_process_id(render_process_id),
-          render_view_id(render_view_id) {}
-
-    ~WebContentsInfo() {}
-
-    content::WebContents* web_contents;
-    int render_process_id;
-    int render_view_id;
+  class WebContentsTracker : public content::WebContentsObserver {
+   public:
+    WebContentsTracker(content::WebContents* web_contents,
+                       const base::Closure& finished_callback,
+                       int render_process_id,
+                       int render_view_id)
+        : content::WebContentsObserver(web_contents),
+          finished_callback_(finished_callback),
+          render_process_id_(render_process_id),
+          render_view_id_(render_view_id) {}
+
+    ~WebContentsTracker() override {}
+
+    int render_process_id() const { return render_process_id_; }
+    int render_view_id() const { return render_view_id_; }
+
+   private:
+    // content::WebContentsObserver overrides.
+    void WebContentsDestroyed() override {
+      Observe(nullptr);
+      finished_callback_.Run();
+      // NOTE: We are deleted now.
+    }
+    void RenderViewHostChanged(content::RenderViewHost* old_host,
+                               content::RenderViewHost* new_host) override {
+      Observe(nullptr);
+      finished_callback_.Run();
+      // NOTE: We are deleted now.
+    }
+
+    const base::Closure finished_callback_;
+    const int render_process_id_;
+    const int render_view_id_;
   };
 
   friend class base::RefCountedThreadSafe<TabWatcher>;
 
-  ~TabWatcher() override {
+  ~TabWatcher() {
     // Must be destroyed on the UI thread due to |registrar_| non thread-safety.
+    // TODO(lazyboy): Do we still need this?
     DCHECK_CURRENTLY_ON(BrowserThread::UI);
   }
 
   // Helper function to find the iterator in |registered_web_contents_| which
   // contains |web_contents|.
-  std::vector<WebContentsInfo>::iterator FindWebContents(
+  ScopedVector<WebContentsTracker>::iterator FindWebContents(
       content::WebContents* web_contents) {
-    for (std::vector<WebContentsInfo>::iterator i(
-         registered_web_contents_.begin());
+    for (ScopedVector<WebContentsTracker>::iterator i(
+             registered_web_contents_.begin());
          i != registered_web_contents_.end(); ++i) {
-      if (i->web_contents == web_contents)
+      if ((*i)->web_contents() == web_contents)
         return i;
     }
 
     return registered_web_contents_.end();
   }
 
-  // Lazy-initialized and used on the UI thread to handle web contents
-  // notifications (tab closing).
-  scoped_ptr<content::NotificationRegistrar> registrar_;
-
   // Keeps track of which WebContent(s) have been registered, in order to avoid
-  // double registrations on |registrar_| and to pass the correct render
+  // double registrations on WebContentsObserver and to pass the correct render
   // process id and render view id to |tab_closed_callback_| after the process
   // has gone away.
-  std::vector<WebContentsInfo> registered_web_contents_;
+  ScopedVector<WebContentsTracker> registered_web_contents_;
 
   // Callback used to notify, on the thread specified by |callback_thread_| the
   // closure of a registered tab.

chrome/browser/ui/panels/panel.cc

@@ -437,9 +437,6 @@ void Panel::Observe(int type,
                     const content::NotificationSource& source,
                     const content::NotificationDetails& details) {
   switch (type) {
-    case content::NOTIFICATION_RENDER_VIEW_HOST_CHANGED:
-      ConfigureAutoResize(content::Source<content::WebContents>(source).ptr());
-      break;
     case chrome::NOTIFICATION_APP_TERMINATING:
       Close();
       break;
@@ -448,6 +445,11 @@ void Panel::Observe(int type,
   }
 }
 
+void Panel::RenderViewHostChanged(content::RenderViewHost* old_host,
+                                  content::RenderViewHost* new_host) {
+  ConfigureAutoResize(web_contents());
+}
+
 void Panel::OnExtensionUnloaded(
     content::BrowserContext* browser_context,
     const extensions::Extension* extension,
@@ -593,8 +595,7 @@ void Panel::SetAutoResizable(bool resizable) {
       EnableWebContentsAutoResize(web_contents);
   } else {
     if (web_contents) {
-      registrar_.Remove(this, content::NOTIFICATION_RENDER_VIEW_HOST_CHANGED,
-                        content::Source<content::WebContents>(web_contents));
+      content::WebContentsObserver::Observe(nullptr);
 
       // NULL might be returned if the tab has not been added.
       RenderViewHost* render_view_host = web_contents->GetRenderViewHost();
@@ -610,14 +611,7 @@ void Panel::EnableWebContentsAutoResize(content::WebContents* web_contents) {
 
   // We also need to know when the render view host changes in order
   // to turn on auto-resize notifications in the new render view host.
-  if (!registrar_.IsRegistered(
-          this, content::NOTIFICATION_RENDER_VIEW_HOST_CHANGED,
-          content::Source<content::WebContents>(web_contents))) {
-    registrar_.Add(
-        this,
-        content::NOTIFICATION_RENDER_VIEW_HOST_CHANGED,
-        content::Source<content::WebContents>(web_contents));
-  }
+  content::WebContentsObserver::Observe(web_contents);
 }
 
 void Panel::OnContentsAutoResized(const gfx::Size& new_content_size) {

chrome/browser/ui/panels/panel.h

@@ -17,6 +17,7 @@
 #include "components/sessions/session_id.h"
 #include "content/public/browser/notification_observer.h"
 #include "content/public/browser/notification_registrar.h"
+#include "content/public/browser/web_contents_observer.h"
 #include "extensions/browser/extension_registry_observer.h"
 #include "ui/base/base_window.h"
 #include "ui/gfx/geometry/rect.h"
@@ -53,6 +54,7 @@ class WindowController;
 class Panel : public ui::BaseWindow,
               public CommandUpdaterDelegate,
               public content::NotificationObserver,
+              public content::WebContentsObserver,
               public extensions::ExtensionRegistryObserver {
  public:
   enum ExpansionState {
@@ -147,6 +149,10 @@ class Panel : public ui::BaseWindow,
                const content::NotificationSource& source,
                const content::NotificationDetails& details) override;
 
+  // content::WebContentsObserver overrides.
+  void RenderViewHostChanged(content::RenderViewHost* old_host,
+                             content::RenderViewHost* new_host) override;
+
   //  extensions::ExtensionRegistryObserver.
   void OnExtensionUnloaded(
       content::BrowserContext* browser_context,

content/browser/frame_host/render_frame_host_manager_unittest.cc

@@ -202,6 +202,31 @@ class RenderFrameHostDeletedObserver : public WebContentsObserver {
   DISALLOW_COPY_AND_ASSIGN(RenderFrameHostDeletedObserver);
 };
 
+// This WebContents observer keep track of its RVH change.
+class RenderViewHostChangedObserver : public WebContentsObserver {
+ public:
+  RenderViewHostChangedObserver(WebContents* web_contents)
+      : WebContentsObserver(web_contents), host_changed_(false) {}
+
+  // WebContentsObserver.
+  void RenderViewHostChanged(RenderViewHost* old_host,
+                             RenderViewHost* new_host) override {
+    host_changed_ = true;
+  }
+
+  bool DidHostChange() {
+    bool host_changed = host_changed_;
+    Reset();
+    return host_changed;
+  }
+
+  void Reset() { host_changed_ = false; }
+
+ private:
+  bool host_changed_;
+  DISALLOW_COPY_AND_ASSIGN(RenderViewHostChangedObserver);
+};
+
 // This observer is used to check whether IPC messages are being filtered for
 // swapped out RenderFrameHost objects. It observes the plugin crash and favicon
 // update events, which the FilterMessagesWhileSwappedOut test simulates being
@@ -932,14 +957,11 @@ TEST_F(RenderFrameHostManagerTest, Init) {
 // Tests the Navigate function. We navigate three sites consecutively and check
 // how the pending/committed RenderViewHost are modified.
 TEST_F(RenderFrameHostManagerTest, Navigate) {
-  TestNotificationTracker notifications;
-
   SiteInstance* instance = SiteInstance::Create(browser_context());
 
   scoped_ptr<TestWebContents> web_contents(
       TestWebContents::Create(browser_context(), instance));
-  notifications.ListenFor(NOTIFICATION_RENDER_VIEW_HOST_CHANGED,
-                          Source<WebContents>(web_contents.get()));
+  RenderViewHostChangedObserver change_observer(web_contents.get());
 
   RenderFrameHostManager* manager = web_contents->GetRenderManagerForTesting();
   RenderFrameHostImpl* host = NULL;
@@ -996,7 +1018,7 @@ TEST_F(RenderFrameHostManagerTest, Navigate) {
   EXPECT_TRUE(GetPendingFrameHost(manager));
   ASSERT_EQ(host, GetPendingFrameHost(manager));
 
-  notifications.Reset();
+  change_observer.Reset();
 
   // Commit.
   manager->DidNavigateFrame(GetPendingFrameHost(manager), true);
@@ -1006,9 +1028,8 @@ TEST_F(RenderFrameHostManagerTest, Navigate) {
   // Check the pending RenderFrameHost has been committed.
   EXPECT_FALSE(GetPendingFrameHost(manager));
 
-  // We should observe a notification.
-  EXPECT_TRUE(
-      notifications.Check1AndReset(NOTIFICATION_RENDER_VIEW_HOST_CHANGED));
+  // We should observe RVH changed event.
+  EXPECT_TRUE(change_observer.DidHostChange());
 }
 
 // Tests WebUI creation.
@@ -1612,8 +1633,6 @@ TEST_F(RenderFrameHostManagerTest, EnableWebUIWithSwappedOutOpener) {
 
 // Test that we reuse the same guest SiteInstance if we navigate across sites.
 TEST_F(RenderFrameHostManagerTest, NoSwapOnGuestNavigations) {
-  TestNotificationTracker notifications;
-
   GURL guest_url(std::string(kGuestScheme).append("://abc123"));
   SiteInstance* instance =
       SiteInstance::CreateForURL(browser_context(), guest_url);
@@ -1675,9 +1694,8 @@ TEST_F(RenderFrameHostManagerTest, NavigateWithEarlyClose) {
   BeforeUnloadFiredWebContentsDelegate delegate;
   scoped_ptr<TestWebContents> web_contents(
       TestWebContents::Create(browser_context(), instance));
+  RenderViewHostChangedObserver change_observer(web_contents.get());
   web_contents->SetDelegate(&delegate);
-  notifications.ListenFor(NOTIFICATION_RENDER_VIEW_HOST_CHANGED,
-                          Source<WebContents>(web_contents.get()));
 
   RenderFrameHostManager* manager = web_contents->GetRenderManagerForTesting();
 
@@ -1693,10 +1711,8 @@ TEST_F(RenderFrameHostManagerTest, NavigateWithEarlyClose) {
   EXPECT_EQ(host, manager->current_frame_host());
   EXPECT_FALSE(GetPendingFrameHost(manager));
 
-  // We should observe a notification.
-  EXPECT_TRUE(
-      notifications.Check1AndReset(NOTIFICATION_RENDER_VIEW_HOST_CHANGED));
-  notifications.Reset();
+  // We should observe RVH changed event.
+  EXPECT_TRUE(change_observer.DidHostChange());
 
   // Commit.
   manager->DidNavigateFrame(host, true);

content/browser/web_contents/web_contents_impl.cc

@@ -3347,14 +3347,6 @@ void WebContentsImpl::NotifyViewSwapped(RenderViewHost* old_host,
   FOR_EACH_OBSERVER(WebContentsObserver, observers_,
                     RenderViewHostChanged(old_host, new_host));
 
-  // TODO(avi): Remove. http://crbug.com/170921
-  std::pair<RenderViewHost*, RenderViewHost*> details =
-      std::make_pair(old_host, new_host);
-  NotificationService::current()->Notify(
-      NOTIFICATION_RENDER_VIEW_HOST_CHANGED,
-      Source<WebContents>(this),
-      Details<std::pair<RenderViewHost*, RenderViewHost*> >(&details));
-
   // Ensure that the associated embedder gets cleared after a RenderViewHost
   // gets swapped, so we don't reuse the same embedder next time a
   // RenderViewHost is attached to this WebContents.

content/public/browser/notification_types.h

@@ -112,13 +112,6 @@ enum NotificationType {
   // DEPRECATED: Use WebContentsObserver::RenderViewReady()
   NOTIFICATION_WEB_CONTENTS_CONNECTED,
 
-  // This notification is sent when a WebContents swaps its render view host
-  // with another one, possibly changing processes. The source is a
-  // Source<WebContents> with a pointer to the WebContents, details is a
-  // std::pair::<old RenderViewHost, new RenderViewHost>.
-  // DEPRECATED: Use WebContentsObserver::RenderViewHostChanged()
-  NOTIFICATION_RENDER_VIEW_HOST_CHANGED,
-
   // This message is sent after a WebContents is disconnected from the
   // renderer process.  The source is a Source<WebContents> with a pointer to
   // the WebContents (the pointer is usable).  No details are expected.