Make SpeechRecognition per RenderFrame instead of per RenderView.

chrome/browser/speech/chrome_speech_recognition_manager_delegate.cc

Index: chrome/browser/speech/chrome_speech_recognition_manager_delegate.cc
diff --git a/chrome/browser/speech/chrome_speech_recognition_manager_delegate.cc b/chrome/browser/speech/chrome_speech_recognition_manager_delegate.cc
index 267b9d2938f4f0d0aa44c008603912cd37f16e6b..5a136ab99e87d329bfc9acf1381cefad95f1523f 100644
--- a/chrome/browser/speech/chrome_speech_recognition_manager_delegate.cc
+++ b/chrome/browser/speech/chrome_speech_recognition_manager_delegate.cc
@@ -21,6 +21,7 @@
 #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_frame_host.h"
 #include "content/public/browser/render_process_host.h"
 #include "content/public/browser/render_view_host.h"
 #include "content/public/browser/resource_context.h"
@@ -42,6 +43,7 @@
 #endif
 
 using content::BrowserThread;
+using content::RenderFrameHost;
 using content::SpeechRecognitionManager;
 using content::WebContents;
 
@@ -49,8 +51,8 @@ namespace speech {
 
 namespace {
 
-void TabClosedCallbackOnIOThread(int render_process_id, int render_view_id) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+void AbortSpeechSessionsForFrameIO(int render_process_id, int render_frame_id) {

[Charlie Reis, 2014/12/01 23:49:43]

nit: OnIO

[mlamouri (slow - plz ping), 2014/12/08 17:41:04]

Done.

+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
   SpeechRecognitionManager* manager = SpeechRecognitionManager::GetInstance();
   // |manager| becomes NULL if a browser shutdown happens between the post of
@@ -59,7 +61,17 @@ void TabClosedCallbackOnIOThread(int render_process_id, int render_view_id) {
   if (!manager)
     return;
 
-  manager->AbortAllSessionsForRenderView(render_process_id, render_view_id);
+  manager->AbortAllSessionsForRenderFrame(render_process_id, render_frame_id);
+}
+
+void AbortSpeechSessionsForFrame(RenderFrameHost* render_frame_host) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+
+  BrowserThread::PostTask(
+      BrowserThread::IO, FROM_HERE,
+      base::Bind(&AbortSpeechSessionsForFrameIO,
+                 render_frame_host->GetProcess()->GetID(),
+                 render_frame_host->GetRoutingID()));
 }
 
 }  // namespace
@@ -140,7 +152,7 @@ class ChromeSpeechRecognitionManagerDelegate::TabWatcher
     : public base::RefCountedThreadSafe<TabWatcher>,
       public content::NotificationObserver {
  public:
-  typedef base::Callback<void(int render_process_id, int render_view_id)>
+  typedef base::Callback<void(WebContents* web_contents)>
       TabClosedCallback;
 
   explicit TabWatcher(TabClosedCallback tab_closed_callback)
@@ -148,17 +160,14 @@ class ChromeSpeechRecognitionManagerDelegate::TabWatcher
   }
 
   // Starts monitoring the WebContents corresponding to the given
-  // |render_process_id|, |render_view_id| pair, invoking |tab_closed_callback_|
-  // if closed/unloaded.
-  void Watch(int render_process_id, int render_view_id) {
-    if (!BrowserThread::CurrentlyOn(BrowserThread::UI)) {
-      BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, base::Bind(
-          &TabWatcher::Watch, this, render_process_id, render_view_id));
-      return;
-    }
+  // |render_process_id|, |render_frame_id| pair, invoking
+  // |tab_closed_callback_| if closed/unloaded.
+  void Watch(int render_process_id, int render_frame_id) {
+    DCHECK_CURRENTLY_ON(BrowserThread::UI);
+
+    WebContents* web_contents = WebContents::FromRenderFrameHost(
+        RenderFrameHost::FromID(render_process_id, render_frame_id));
 
-    WebContents* web_contents = tab_util::GetWebContentsByID(render_process_id,
-                                                             render_view_id);
     // Sessions initiated by speech input extension APIs will end up in a NULL
     // WebContent here, but they are properly managed by the
     // chrome::SpeechInputExtensionManager. However, sessions initiated within a
@@ -169,11 +178,11 @@ class ChromeSpeechRecognitionManagerDelegate::TabWatcher
       return;
 
     // Avoid multiple registrations on |registrar_| for the same |web_contents|.
-    if (FindWebContents(web_contents) !=  registered_web_contents_.end()) {
+    if (registered_web_contents_.find(web_contents) !=
+        registered_web_contents_.end()) {
       return;
     }
-    registered_web_contents_.push_back(
-        WebContentsInfo(web_contents, render_process_id, render_view_id));
+    registered_web_contents_.insert(web_contents);
 
     // Lazy initialize the registrar.
     if (!registrar_.get())
@@ -196,11 +205,9 @@ class ChromeSpeechRecognitionManagerDelegate::TabWatcher
            type == content::NOTIFICATION_RENDER_VIEW_HOST_CHANGED);
 
     WebContents* web_contents = content::Source<WebContents>(source).ptr();
-    std::vector<WebContentsInfo>::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;
-    registered_web_contents_.erase(iter);
+    DCHECK(registered_web_contents_.find(web_contents) !=
+           registered_web_contents_.end());
+    registered_web_contents_.erase(web_contents);
 
     registrar_->Remove(this,
                        content::NOTIFICATION_RENDER_VIEW_HOST_CHANGED,
@@ -209,25 +216,10 @@ class ChromeSpeechRecognitionManagerDelegate::TabWatcher
                        content::NOTIFICATION_WEB_CONTENTS_DISCONNECTED,
                        content::Source<WebContents>(web_contents));
 
-    tab_closed_callback_.Run(render_process_id, render_view_id);
+    tab_closed_callback_.Run(web_contents);
   }
 
  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;
-  };
-
   friend class base::RefCountedThreadSafe<TabWatcher>;
 
   ~TabWatcher() override {
@@ -235,20 +227,6 @@ class ChromeSpeechRecognitionManagerDelegate::TabWatcher
     DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   }
 
-  // Helper function to find the iterator in |registered_web_contents_| which
-  // contains |web_contents|.
-  std::vector<WebContentsInfo>::iterator FindWebContents(
-      content::WebContents* web_contents) {
-    for (std::vector<WebContentsInfo>::iterator i(
-         registered_web_contents_.begin());
-         i != registered_web_contents_.end(); ++i) {
-      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_;
@@ -257,7 +235,7 @@ class ChromeSpeechRecognitionManagerDelegate::TabWatcher
   // double registrations on |registrar_| and to pass the correct render
   // process id and render view id to |tab_closed_callback_| after the process

[Charlie Reis, 2014/12/01 23:49:42]

This comment looks stale.

[mlamouri (slow - plz ping), 2014/12/08 17:41:04]

Done.

   // has gone away.
-  std::vector<WebContentsInfo> registered_web_contents_;
+  std::set<WebContents*> registered_web_contents_;
 
   // Callback used to notify, on the thread specified by |callback_thread_| the

[Charlie Reis, 2014/12/01 23:49:42]

callback_thread_ looks stale.  This gets called on the UI thread, right?

[mlamouri (slow - plz ping), 2014/12/08 17:41:04]

Done.

   // closure of a registered tab.
@@ -275,13 +253,10 @@ ChromeSpeechRecognitionManagerDelegate
 }
 
 void ChromeSpeechRecognitionManagerDelegate::TabClosedCallback(
-    int render_process_id, int render_view_id) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+    WebContents* web_contents) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
-  // Tell the S.R. Manager (which lives on the IO thread) to abort all the
-  // sessions for the given renderer view.
-  BrowserThread::PostTask(BrowserThread::IO, FROM_HERE, base::Bind(
-      &TabClosedCallbackOnIOThread, render_process_id, render_view_id));
+  web_contents->ForEachFrame(base::Bind(&AbortSpeechSessionsForFrame));
 }
 
 void ChromeSpeechRecognitionManagerDelegate::OnRecognitionStart(
@@ -296,7 +271,12 @@ void ChromeSpeechRecognitionManagerDelegate::OnRecognitionStart(
         base::Bind(&ChromeSpeechRecognitionManagerDelegate::TabClosedCallback,
                    base::Unretained(this)));
   }
-  tab_watcher_->Watch(context.render_process_id, context.render_view_id);
+
+  BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
+                          base::Bind(&TabWatcher::Watch,
+                                     tab_watcher_,
+                                     context.render_process_id,
+                                     context.render_frame_id));
 }
 
 void ChromeSpeechRecognitionManagerDelegate::OnAudioStart(int session_id) {
@@ -357,26 +337,10 @@ void ChromeSpeechRecognitionManagerDelegate::CheckRecognitionIsAllowed(
   const content::SpeechRecognitionSessionContext& context =
       SpeechRecognitionManager::GetInstance()->GetSessionContext(session_id);
 
-  // Make sure that initiators (extensions/web pages) properly set the
-  // |render_process_id| field, which is needed later to retrieve the profile.
-  DCHECK_NE(context.render_process_id, 0);

[Charlie Reis, 2014/12/01 23:49:42]

I see that the rest of this block has been moved inside CheckRenderViewType. 
Can we keep this DCHECK as well, or is there a reason to remove it?

[mlamouri (slow - plz ping), 2014/12/08 17:41:04]

I removed that because the check sounded pretty arbitrary. It's not clear to me
how we might end up in this situation and why checking render_process_id but not
render_view_id/render_frame_id.

I'm happy to put it back though.

[Charlie Reis, 2014/12/08 20:02:57]

Looks like it's initialized to 0 in the context's constructor, so it's not
entirely arbitrary; let's keep it.  Adding a matching DCHECK for render_frame_id
is probably reasonable.

[mlamouri (slow - plz ping), 2014/12/08 21:07:59]

Done.

-
-  int render_process_id = context.render_process_id;
-  int render_view_id = context.render_view_id;
-  if (context.embedder_render_process_id) {
-    // If this is a request originated from a guest, we need to re-route the
-    // permission check through the embedder (app).
-    render_process_id = context.embedder_render_process_id;
-    render_view_id = context.embedder_render_view_id;
-  }
-
-  // Check that the render view type is appropriate, and whether or not we
-  // need to request permission from the user.
+  // Check that the context is appropriate, and whether or not we need to
+  // request permission from the user.
   BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
-                          base::Bind(&CheckRenderViewType,
-                                     callback,
-                                     render_process_id,
-                                     render_view_id));
+                          base::Bind(&CheckRenderViewType, callback, context));
 }
 
 content::SpeechRecognitionEventListener*
@@ -398,16 +362,23 @@ bool ChromeSpeechRecognitionManagerDelegate::FilterProfanities(
 // static.
 void ChromeSpeechRecognitionManagerDelegate::CheckRenderViewType(
     base::Callback<void(bool ask_user, bool is_allowed)> callback,
-    int render_process_id,
-    int render_view_id) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  const content::RenderViewHost* render_view_host =
-      content::RenderViewHost::FromID(render_process_id, render_view_id);
+    const content::SpeechRecognitionSessionContext& context) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
   bool allowed = false;
   bool check_permission = false;
 
-  if (!render_view_host) {
+  RenderFrameHost* render_frame_host = RenderFrameHost::FromID(
+      context.render_process_id, context.render_frame_id);
+
+  if (context.embedder_render_process_id) {
+    // If this is a request originated from a guest, we need to re-route the
+    // permission check through the embedder (app).
+    render_frame_host = RenderFrameHost::FromID(
+        context.embedder_render_process_id, context.embedder_render_frame_id);
+  }
+
+  if (!render_frame_host) {
     // This happens for extensions. Manifest should be checked for permission.
     allowed = true;
     check_permission = false;
@@ -416,7 +387,8 @@ void ChromeSpeechRecognitionManagerDelegate::CheckRenderViewType(
     return;
   }
 
-  WebContents* web_contents = WebContents::FromRenderViewHost(render_view_host);
+  WebContents* web_contents =
+      WebContents::FromRenderFrameHost(render_frame_host);
 
   // chrome://app-list/ uses speech recognition.
   if (web_contents->GetCommittedWebUI() &&