Changing tab closure handling logic in speech recognition code and cleaning bubble controller. (Spe…

chrome/browser/speech/tab_watcher.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 "chrome/browser/speech/tab_watcher.h"
+
+#include "base/bind.h"
+#include "chrome/browser/tab_contents/tab_util.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/web_contents.h"
+
+using content::BrowserThread;
+using content::WebContents;
+
+namespace {
+const int kNotificationType = content::NOTIFICATION_WEB_CONTENTS_DISCONNECTED;
+}
+
+TabWatcher::TabWatcher(
+    TabClosedCallback tab_closed_callback, BrowserThread::ID callback_thread)
+  : tab_closed_callback_(tab_closed_callback),

[Satish, 2012/07/02 21:44:41]

indent this and next line by 2 spaces

[Primiano Tucci (use gerrit), 2012/07/03 10:05:02]

Done.

+    callback_thread_(callback_thread) {
+}
+
+TabWatcher::~TabWatcher() {
+  // Must be destroyed on the UI thread due to |registrar_| non thread-safety.
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+}
+
+void TabWatcher::Watch(
+    int render_process_id, int render_view_id) {

[Satish, 2012/07/02 21:44:41]

seems like this can fit in previous line?

[Primiano Tucci (use gerrit), 2012/07/03 10:05:02]

Oh right!

+  if (!BrowserThread::CurrentlyOn(BrowserThread::UI)) {
+    BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, base::Bind(
+        &TabWatcher::Watch, this, render_process_id, render_view_id));
+    return;
+  }
+
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));

[Satish, 2012/07/02 21:44:41]

this seems unnecessary given the if statement above

[Primiano Tucci (use gerrit), 2012/07/03 10:05:02]

Done.

+  WebContents* web_contents = tab_util::GetWebContentsByID(render_process_id,
+                                                           render_view_id);
+
+  // Sessions initiated by extensions might not have a corresponding WebContent.

[Satish, 2012/07/02 21:44:41]

for extensions should you listen for background page getting unloaded, e.g. user
uninstalls extension while recognition is going on? or is that handled by the
extension manager separately?

[Primiano Tucci (use gerrit), 2012/07/03 10:05:02]

Well, there is a bit of confusion here using the term "extension". I changed the
comment to make it clearer.
Anyway, the use case you mentioned (removing extension while recognizing, using
new JS APIs) is cleanly handled here.
The only uncovered case (which require this "if") are the (soon to be
deprecated) speech input extension APIs, which are handled separately by
chrome::SpeechInputExtensionManager.

+  if (!web_contents)
+    return;
+
+  // Avoid multiple registrations on |registrar_| for the same |web_contents|.
+  if (registered_contents_.find(web_contents) != registered_contents_.end())
+    return;
+  registered_contents_.insert(web_contents);
+
+  // Lazy initialize the registrar.
+  if (!registrar_.get())
+    registrar_.reset(new content::NotificationRegistrar());
+
+  registrar_->Add(this,
+                  kNotificationType,
+                  content::Source<WebContents>(web_contents));
+}
+
+void TabWatcher::Observe(
+    int type,
+    const content::NotificationSource& source,
+    const content::NotificationDetails& details) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  DCHECK_EQ(kNotificationType, type);
+
+  WebContents* web_contents = content::Source<WebContents>(source).ptr();

[Satish, 2012/07/02 21:44:41]

any chance this could return null, i.e. this notification coming in twice? just
checking if that is a valid scenario when things are slow and if we should check
for null here

[Primiano Tucci (use gerrit), 2012/07/03 10:05:02]

I added a paranoid DCHECK, however IMHO, even in case of double notification
(hard to imagine, but let's assume that the webcontent can be reattached and
re-disconnected in some corner cases), I hope that the notification service will
always provide the corresponding source (thus a non-null web_contents) of the
event.

+  const int render_process_id = web_contents->GetRenderProcessHost()->GetID();
+  const int render_view_id = web_contents->GetRenderViewHost()->GetRoutingID();
+
+  registrar_->Remove(this,
+                     kNotificationType,
+                     content::Source<WebContents>(web_contents));
+  registered_contents_.erase(web_contents);
+
+  BrowserThread::PostTask(callback_thread_, FROM_HERE, base::Bind(
+      tab_closed_callback_, render_process_id, render_view_id));
+}