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

chrome/browser/speech/tab_watcher.h

Index: chrome/browser/speech/tab_watcher.h
diff --git a/chrome/browser/speech/tab_watcher.h b/chrome/browser/speech/tab_watcher.h
new file mode 100644
index 0000000000000000000000000000000000000000..393b87049a7732be8e15fe7fa4cb13227f0ab0e4
--- /dev/null
+++ b/chrome/browser/speech/tab_watcher.h
@@ -0,0 +1,68 @@
+// 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.
+
+#ifndef CHROME_BROWSER_SPEECH_TAB_WATCHER_H_
+#define CHROME_BROWSER_SPEECH_TAB_WATCHER_H_
+#pragma once
+
+#include <set>
+
+#include "base/callback.h"
+#include "base/memory/ref_counted.h"
+#include "base/memory/scoped_ptr.h"
+#include "content/public/browser/browser_thread.h"
+#include "content/public/browser/notification_observer.h"
+
+namespace content {
+class NotificationRegistrar;
+class WebContents;
+}
+
+// Simple utility to get notified when a WebContent (a tab or an extension's
+// background page) is closed or crashes. Both the callback site and the
+// callback thread are passed by the caller in the constructor.
+// There is no restriction on the constructor, however this class must be
+// destroyed on the UI thread, due to the NotificationRegistrar dependency.
+class TabWatcher

[jam, 2012/07/05 17:58:56]

this class seems unnecessary, i.e. this functionality (listening for one
notification) is pretty simple and is usually done in the class that needs it.
then you don't need extra complexity like posting tasks to the right thread (the
one caller now is only from UI), and the mental overhead of  flipping between
this source file and the caller at the same to see what's going on.

[Primiano Tucci (use gerrit), 2012/07/05 21:21:50]

Unfortunately the only caller (the ChromeSpeechRecognitionManagerDelegate) uses
this class from the IO thread, not UI (OnRecognitionStart is called on IO). Rhe
purpose of this class was to hide and incapsulate most of these thread switching
details.
Otherwise, in order to do the equivalent of the TabWatcher.Watch(..) call, I
should post a task on the UI thread for the registration, and then, once the
Observe method is called back (on UI), I should post another task from UI to IO
thread for aborting the sessions. 
The intent here, instead, was to try to keep the
ChromeSpeechRecognitionManagerDelegate a bit cleaner.

[jam, 2012/07/06 06:58:59]

ah, I see. I got thrown off because of the GetBubbleController calls which
looked liked they were on the UI thread.

since no one else is using this class, and it's really an implementation detail
of ChromeSpeechManagerDelegate, how about forward declaring it as an inner
class, and then have the entire implementation in the chrome speech delegate's
cc file?

[Primiano Tucci (use gerrit), 2012/07/06 10:36:45]

Done.

+    : public base::RefCountedThreadSafe<TabWatcher>,
+      public content::NotificationObserver {
+ public:
+  typedef base::Callback<void(int render_process_id, int render_view_id)>
+      TabClosedCallback;
+
+  TabWatcher(TabClosedCallback tab_closed_callback,
+             content::BrowserThread::ID callback_thread);
+
+  // 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);
+
+ private:
+  friend class base::RefCountedThreadSafe<TabWatcher>;
+
+  virtual ~TabWatcher();
+
+  // content::NotificationObserver implementation.
+  virtual void Observe(int type,
+                       const content::NotificationSource& source,
+                       const content::NotificationDetails& details) OVERRIDE;
+
+  // 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_|
+  std::set<content::WebContents*> registered_web_contents_;
+
+  // Callback used to notify, on the thread specified by |callback_thread_| the
+  // closure of a registered tab.
+  TabClosedCallback tab_closed_callback_;
+  content::BrowserThread::ID callback_thread_;
+
+  DISALLOW_COPY_AND_ASSIGN(TabWatcher);
+};
+
+#endif  // CHROME_BROWSER_SPEECH_TAB_WATCHER_H_