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

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 f22f95541e0bd6f6ef1382511134b637126e3a2c..81b1c7fdfbeed03d1fb601106edf06054c169de6 100644
--- a/chrome/browser/speech/chrome_speech_recognition_manager_delegate.cc
+++ b/chrome/browser/speech/chrome_speech_recognition_manager_delegate.cc
@@ -16,6 +16,7 @@
 #include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/browser/speech/chrome_speech_recognition_preferences.h"
 #include "chrome/browser/speech/speech_recognition_tray_icon_controller.h"
+#include "chrome/browser/speech/tab_watcher.h"
 #include "chrome/browser/tab_contents/tab_util.h"
 #include "chrome/browser/view_type_utils.h"
 #include "chrome/common/pref_names.h"
@@ -39,12 +40,9 @@
 
 using content::BrowserThread;
 using content::SpeechRecognitionManager;
-using content::SpeechRecognitionSessionContext;
 using content::WebContents;
 
 namespace {
-const int kNoActiveBubble =
-    content::SpeechRecognitionManager::kSessionIDInvalid;
 
 const char kExtensionPrefix[] = "chrome-extension://";
 
@@ -124,38 +122,33 @@ class ChromeSpeechRecognitionManagerDelegate::OptionalRequestInfo
 };
 
 ChromeSpeechRecognitionManagerDelegate::ChromeSpeechRecognitionManagerDelegate()
-    : active_bubble_session_id_(kNoActiveBubble) {
+{

[Satish, 2012/07/03 15:11:57]

move to previous line

[Primiano Tucci (use gerrit), 2012/07/03 16:54:35]

It will overflow the row. Chrome....Delegate() takes exactly 80 cols :)

[Satish, 2012/07/04 09:52:51]

In such cases we write it as
ChromeSpeechRecognitionManagerDelegate
::ChromeSpeechRecognitionManagerDelegate() {
}

[Primiano Tucci (use gerrit), 2012/07/04 11:18:15]

Done.

 }
 
 ChromeSpeechRecognitionManagerDelegate::
     ~ChromeSpeechRecognitionManagerDelegate() {

[Satish, 2012/07/03 15:11:57]

could move to previous line

[Primiano Tucci (use gerrit), 2012/07/03 16:54:35]

As above, overflows 80 cols.

[Satish, 2012/07/04 09:52:51]

Ok, no 4 space indentation then

[Primiano Tucci (use gerrit), 2012/07/04 11:18:15]

Done.

   if (tray_icon_controller_.get())
     tray_icon_controller_->Hide();
-  if (active_bubble_session_id_ != kNoActiveBubble) {
-    DCHECK(bubble_controller_.get());
-    bubble_controller_->CloseBubble(active_bubble_session_id_);
-  }
+  if (bubble_controller_.get() && bubble_controller_->GetActiveSessionID())

[Satish, 2012/07/03 15:11:57]

can the second check be moved inside CloseBubble(), so we can remove
GetActiveSessionID() as well? Alternatively SpeechRecognitionBubbleController
can close the active bubble in the destructor without enforcing that the caller
should do it

[Primiano Tucci (use gerrit), 2012/07/03 16:54:35]

Well, the check wasn't needed. Removed.
Mmm see later discussion on this point.
Unfortunately not since closing the bubble requires a PostTask to the UI thread.
But PostTask cannot be called in its (BubbleController) destructor, because the
class is (deliberately for this purpose) RefCounted, and a RefCounted class
cannot be incremented (increment is a side effect of PostTask) on its
destructor. Thus why the CloseBubble should be done here.

+    bubble_controller_->CloseBubble();
 }
 
 void ChromeSpeechRecognitionManagerDelegate::InfoBubbleButtonClicked(
     int session_id, SpeechRecognitionBubble::Button button) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
-  DCHECK_EQ(active_bubble_session_id_, session_id);
 
   // Note, the session might have been destroyed, therefore avoid calls to the
   // manager which imply its existance (e.g., GetSessionContext()).
 
   if (button == SpeechRecognitionBubble::BUTTON_CANCEL) {
-    GetBubbleController()->CloseBubble(session_id);
+    GetBubbleController()->CloseBubble();
     last_session_config_.reset();
-    active_bubble_session_id_ = kNoActiveBubble;
 
     // We can safely call AbortSession even if the session has already ended,
     // the manager's public methods are reliable and will handle it properly.
     SpeechRecognitionManager::GetInstance()->AbortSession(session_id);
   } else if (button == SpeechRecognitionBubble::BUTTON_TRY_AGAIN) {
-    GetBubbleController()->CloseBubble(session_id);
-    active_bubble_session_id_ = kNoActiveBubble;
+    GetBubbleController()->CloseBubble();
     RestartLastSession();
   }

[Satish, 2012/07/03 15:11:57]

if the above are the only 2 possible values for |button|, can you add an else
with NOTREACHED here?

[Primiano Tucci (use gerrit), 2012/07/03 16:54:35]

Done.

 }
@@ -163,14 +156,18 @@ void ChromeSpeechRecognitionManagerDelegate::InfoBubbleButtonClicked(
 void ChromeSpeechRecognitionManagerDelegate::InfoBubbleFocusChanged(
     int session_id) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
-  DCHECK_EQ(active_bubble_session_id_, session_id);
+
+  // This check is needed since on some systems (MacOS), in rare cases, if the
+  // user clicks repeatedly and fast on the input element, the FocusChanged
+  // event (corresponding to the old session that should be aborted) can be
+  // received after a new session (corresponding to the 2nd click) is started.
+  if (GetBubbleController()->GetActiveSessionID() != session_id)

[Satish, 2012/07/03 15:11:57]

If you ignore the focus change event for the first bubble, are you relying on
the speech recognition manager to end the session/close the first bubble when
the second session starts?

Also can this check be moved to the bubble controller so it doesn't even invoke
this delegate method if the focus changed event was received for a bubble other
than the active one?

[Primiano Tucci (use gerrit), 2012/07/03 16:54:35]

Yes, even if the UI events are received out of order (sigh), the
SpeechRecognitionManager will always handle them in order. More in detail, in
this unlucky case, it will happen that:
 - The start for session 2 is delivered in advance (before FocusChanged for
session 1).
 - When the SRManager is requested to start session 2, will notice that session
1 is still in progress, so it will abort session 1.
 - The abort of session 1 (issued by the manager because it's required to start
another session) will trigger the OnEnd event for session 1 (which will close
the current bubble).
 - The manager will subsequently start the session 2, so a new bubble will be
shown.
 - Finally the FocusChanged event for session 1 will be dispatched here, where
we return since the event refers to session 1, which is not active anymore and
has been already aborted by the manager.

+    return;
 
   // Note, the session might have been destroyed, therefore avoid calls to the
   // manager which imply its existance (e.g., GetSessionContext()).
-
-  GetBubbleController()->CloseBubble(session_id);
+  GetBubbleController()->CloseBubble();
   last_session_config_.reset();
-  active_bubble_session_id_ = kNoActiveBubble;
 
   // If the user clicks outside the bubble while capturing audio we abort the
   // session. Otherwise, i.e. audio capture is ended and we are just waiting for
@@ -183,11 +180,25 @@ void ChromeSpeechRecognitionManagerDelegate::RestartLastSession() {
   DCHECK(last_session_config_.get());
   SpeechRecognitionManager* manager = SpeechRecognitionManager::GetInstance();
   const int new_session_id = manager->CreateSession(*last_session_config_);
-  DCHECK_NE(new_session_id, kNoActiveBubble);
+  DCHECK_NE(SpeechRecognitionManager::kSessionIDInvalid, new_session_id);
   last_session_config_.reset();
   manager->StartSession(new_session_id);
 }
 
+void ChromeSpeechRecognitionManagerDelegate::TabClosedCallback(
+    int render_process_id, int render_view_id) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+
+  if (SpeechRecognitionManager* mgr = SpeechRecognitionManager::GetInstance())

[Satish, 2012/07/03 15:11:57]

mgr -> manager

[Primiano Tucci (use gerrit), 2012/07/03 16:54:35]

Done.

+    mgr->AbortAllSessionsForRenderer(render_process_id, render_view_id);
+
+  if (bubble_controller_.get() &&
+      bubble_controller_->IsShowingBubbleOn(render_process_id,

[Satish, 2012/07/03 15:11:57]

looks like bubble controller holds the current render process id and render view
id only for this call. Can they be stored in this class itself so it can compare
locally without asking the controller?

[Primiano Tucci (use gerrit), 2012/07/03 16:54:35]

Hmm similar to the offline discussion about active_session_id_, it would require
that I reset those local field every time I call CloseBubble in this class
(instead of doing only in one point inside the bubble controller as it is now).
Furthermore, the controller needs those IDs anyway to draw the bubble.

+                                            render_view_id)) {
+    bubble_controller_->CloseBubble();
+  }
+}
+
 void ChromeSpeechRecognitionManagerDelegate::OnRecognitionStart(
     int session_id) {
   const content::SpeechRecognitionSessionContext& context =
@@ -199,21 +210,27 @@ void ChromeSpeechRecognitionManagerDelegate::OnRecognitionStart(
         SpeechRecognitionManager::GetInstance()->GetSessionConfig(session_id)));
 
     // Create and show the bubble.
-    DCHECK_EQ(active_bubble_session_id_, kNoActiveBubble);
-    active_bubble_session_id_ = session_id;
     GetBubbleController()->CreateBubble(session_id,
                                         context.render_process_id,
                                         context.render_view_id,
                                         context.element_rect);
+  }
 
-    // TODO(primiano) Why not creating directly the bubble in warmup mode?
-    GetBubbleController()->SetBubbleWarmUpMode(session_id);
+  // Register callback to auto abort session on tab closure.
+  // |tab_watcher_| is lazyly istantiated on the first call.
+  if (!tab_watcher_.get()) {
+    tab_watcher_ = new TabWatcher(
+        base::Bind(&ChromeSpeechRecognitionManagerDelegate::TabClosedCallback,
+                   base::Unretained(this)),
+        BrowserThread::IO);
   }
+  tab_watcher_->Watch(context.render_process_id, context.render_view_id);

[Satish, 2012/07/03 15:11:57]

can you also stop observing the tab when the bubble closes, so we don't end up
listening for that tab forever

[Primiano Tucci (use gerrit), 2012/07/03 16:54:35]

I thought about that but it means that in such case every time a recognition is
started and ended I should register/deregister the corresponding tab, paying a
"price"  for the (de)registration of each session on the notification registrar
(which in turn uses NotificationService).
The pro is that if I de-register in advance the tab, I will avoid the call to
manager.AbortAllSessionsForRenderer(...) upon tab closure.

However, that method (AbortAllSessionsForRenderer) in practice iterates upon all
the active sessions in the manager, which realistically is expected to be an
empty collection most of the time, or at worse containing only one session
(theoretically also more than one, yet extremely unlikely).

So, it seems to me that is better to pay a small fixed price upon tab closure,
than a probably bigger price (due to the more complex architecture of the
registrar) for registration/deregistration of each recognition session. What do
you think?

[Satish, 2012/07/04 09:52:51]

If the user had 20 tabs open and used speech recognition in say 5 of those tabs
one time, this class will be listening for those 5 tabs being closed even when
user isn't using those tabs which is necessary. I am not concerned about time
taken to deregister one by one or all at once, but concerned about staying
registered and adding burden to the notification service. Note that every tab
close event will come to TabWatcher which then filters out the ones other than
what we are interested in. So I prefer we deregister once recognition ends

[Primiano Tucci (use gerrit), 2012/07/04 11:18:15]

Events are pre-filtered by the notification service (actually they're not
filtered at all, the notification service is implemented as an hashtable).
Handling prompt de-registration on our side would require more code in order to
keep track of the number of SR sessions registered for each tab, doing in
essence a refcount, and deregistering from the notification service only when
the count reaches 0 (similarly for registration).
As discussed offline, we opt for keeping the current implementation.

 }
 
 void ChromeSpeechRecognitionManagerDelegate::OnAudioStart(int session_id) {
   if (RequiresBubble(session_id)) {
-    GetBubbleController()->SetBubbleRecordingMode(session_id);
+    DCHECK_EQ(session_id, GetBubbleController()->GetActiveSessionID());

[Satish, 2012/07/03 15:11:57]

It doesn't feel right to ask the bubble controller about what is the currently
active recording session (even though that has that information for its own use,
i.e.to pass it back here in callbacks). Can you store the current session id in
this class and make these checks with it?

[Primiano Tucci (use gerrit), 2012/07/03 16:54:35]

As discussed offline, it doesn't look pretty nice, since it looks like a
duplicated code for CloseBubble and CreateBubble methods. Furthermore it will
increase chances of errors in future, if one adds o remove a CloseBubble call,
without setting/clearing the local field corresponding.

+    GetBubbleController()->SetBubbleRecordingMode();
   } else if (RequiresTrayIcon(session_id)) {
     // We post the action to the UI thread for sessions requiring a tray icon,
     // since ChromeSpeechRecognitionPreferences (which requires UI thread) is
@@ -242,8 +259,9 @@ void ChromeSpeechRecognitionManagerDelegate::OnSoundEnd(int session_id) {
 void ChromeSpeechRecognitionManagerDelegate::OnAudioEnd(int session_id) {
   // OnAudioEnd can be also raised after an abort, when the bubble has already
   // been closed.
-  if (RequiresBubble(session_id) && active_bubble_session_id_ == session_id) {
-    GetBubbleController()->SetBubbleRecognizingMode(session_id);
+  if (GetBubbleController()->GetActiveSessionID() == session_id) {

[Satish, 2012/07/03 15:11:57]

why is this condition different from what you have in OnAudioStart, i.e. the if
statement and DCHECK are reversed?

[Primiano Tucci (use gerrit), 2012/07/03 16:54:35]

In the AudioStart event, in the case that that particular session requires a
bubble, the session id MUST match the current bubble (i.e. the bubble for that
session must have been shown) otherwise it means that something has gone wrong,
thus the DCHECK on GetActiveSessionID.

In this case, instead, a session requiring a bubble can end even if its bubble
has been previously closed. So, if the session currently ending corresponds to
the active bubble, it implies that it MUST be a session requiring a bubble. The
vice-versa is not necessarily true: a session requiring a bubble can end, but it
is not implied, in this case, that its bubble must be active.

In more practical terms, if we look at production code, removing the paranoid
DCHECKS, this single "if" is sufficient to determine whether to call the
SetRecognizing mode or not.
If I start, as in the AudioStart case, with "if RequiresBubble(...)" this
condition will not be enough, and I've to add the second check
(ActiveSessionID() == session_id) anyway, making two comparison instead of one
in production code.

+    DCHECK(RequiresBubble(session_id));
+    GetBubbleController()->SetBubbleRecognizingMode();
   } else if (RequiresTrayIcon(session_id)) {
     GetTrayIconController()->Hide();
   }
@@ -251,19 +269,13 @@ void ChromeSpeechRecognitionManagerDelegate::OnAudioEnd(int session_id) {
 
 void ChromeSpeechRecognitionManagerDelegate::OnRecognitionResult(
     int session_id, const content::SpeechRecognitionResult& result) {
-  // A result can be dispatched when the bubble is not visible anymore (e.g.,
-  // lost focus while waiting for a result, thus continuing in background).
-  if (RequiresBubble(session_id) && active_bubble_session_id_ == session_id) {
-    GetBubbleController()->CloseBubble(session_id);
-    last_session_config_.reset();
-    active_bubble_session_id_ = kNoActiveBubble;
-  }
+  // The bubble will be closed upon the OnEnd event, which will follow soon.
 }
 
 void ChromeSpeechRecognitionManagerDelegate::OnRecognitionError(
     int session_id, const content::SpeechRecognitionError& error) {
   // An error can be dispatched when the bubble is not visible anymore.
-  if (active_bubble_session_id_ != session_id)
+  if (GetBubbleController()->GetActiveSessionID() != session_id)
     return;
   DCHECK(RequiresBubble(session_id));
 
@@ -299,28 +311,29 @@ void ChromeSpeechRecognitionManagerDelegate::OnRecognitionError(
       return;
   }
   GetBubbleController()->SetBubbleMessage(
-      session_id, l10n_util::GetStringUTF16(error_message_id));
+      l10n_util::GetStringUTF16(error_message_id));
 }
 
 void ChromeSpeechRecognitionManagerDelegate::OnAudioLevelsChange(
     int session_id, float volume, float noise_volume) {
-  if (active_bubble_session_id_ == session_id) {
+  if (GetBubbleController()->GetActiveSessionID() == session_id) {
     DCHECK(RequiresBubble(session_id));
-    GetBubbleController()->SetBubbleInputVolume(session_id,
-                                                volume, noise_volume);
+    GetBubbleController()->SetBubbleInputVolume(volume, noise_volume);
   } else if (RequiresTrayIcon(session_id)) {
     GetTrayIconController()->SetVUMeterVolume(volume);
   }
 }
 
 void ChromeSpeechRecognitionManagerDelegate::OnRecognitionEnd(int session_id) {
-  // No need to remove the bubble here, since either one of the following events
-  // must have happened prior to this callback:
-  // - A previous OnRecognitionResult event already closed the bubble.
-  // - An error occurred, so the bubble is showing the error and will be closed
-  //   when it will lose focus (by InfoBubbleFocusChanged()).
-  // - The bubble lost focus or the user pressed the Cancel button, thus it has
-  //   been closed by InfoBubbleFocusChanged(), which triggered an AbortSession.
+  // The only case in which the OnRecognitionEnd should not close the bubble is
+  // when we are showing an error. In this case the bubble will be closed by
+  // the |InfoBubbleFocusChanged| method, when the users clicks either the
+  // "Cancel" button or outside of the bubble.
+  if (GetBubbleController()->GetActiveSessionID() == session_id &&
+      !GetBubbleController()->IsShowingMessage()) {
+    DCHECK(RequiresBubble(session_id));
+    GetBubbleController()->CloseBubble();
+  }
 }
 
 void ChromeSpeechRecognitionManagerDelegate::GetDiagnosticInformation(