Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10663018:
Changing tab closure handling logic in speech recognition code and cleaning bubble controller. (Spe… (Closed)
|
Created:
8 years, 6 months ago by Primiano Tucci (use gerrit) Modified:
8 years, 5 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, Satish, jam, jochen+watch-content_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionChanging tab closure handling logic in speech recognition code and cleaning bubble controller. (Speech CL2.6)
- Introduced TabWatcher facility class to handle de/registration for tab closure events.
- Tab closure is now handled directly in the ChromeSpeechRecognitionManagerDelegate, aborting all sessions for the corresponding renderer.
- Cleaned-up SpeechRecognitionBubbleController code: removed support for multiple bubbles (not used anymore) and tab events registration.
BUG=116954, 132627
TEST=none
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=145586
Patch Set 1 : #
Total comments: 14
Patch Set 2 : Hans review #Patch Set 3 : Slight refactor to fix a bug on mac + rebase #
Total comments: 16
Patch Set 4 : Satish review + rebase #
Total comments: 38
Patch Set 5 : Satish review #Patch Set 6 : gfx::Rect forward declaration #Patch Set 7 : Relaxed thread checks for tests. #
Total comments: 2
Patch Set 8 : Satish Review #
Total comments: 6
Patch Set 9 : jochen review #
Total comments: 8
Patch Set 10 : Moved TabWatcher as a private class in chrome_speech_recognition_manager_delegate.cc #Messages
Total messages: 21 (0 generated)
This CL can be reviewed independently of CL2.4 and CL2.5
This CL can be reviewed independently of CL2.4 and CL2.5
http://codereview.chromium.org/10663018/diff/1001/chrome/browser/speech/chrom... File chrome/browser/speech/chrome_speech_recognition_manager_delegate.h (right): http://codereview.chromium.org/10663018/diff/1001/chrome/browser/speech/chrom... chrome/browser/speech/chrome_speech_recognition_manager_delegate.h:83: // Callback called by |tab_watcher_| on the IO thread to notify tab closure. "to notify tab closure" doesn't sound quite right.. maybe "to signal tab closure" is better? http://codereview.chromium.org/10663018/diff/1001/chrome/browser/speech/speec... File chrome/browser/speech/speech_recognition_bubble_controller.cc (right): http://codereview.chromium.org/10663018/diff/1001/chrome/browser/speech/speec... chrome/browser/speech/speech_recognition_bubble_controller.cc:31: current_bubble_render_view_id_() { i'd prefer current_bubble_render_process_id_(0) instead of current_bubble_render_process_id_().. the same for the render view id http://codereview.chromium.org/10663018/diff/1001/chrome/browser/speech/speec... chrome/browser/speech/speech_recognition_bubble_controller.cc:43: BrowserThread::CurrentlyOn(BrowserThread::IO); is this supposed to be in a DCHECK? http://codereview.chromium.org/10663018/diff/1001/chrome/browser/speech/speec... File chrome/browser/speech/speech_recognition_bubble_controller.h (right): http://codereview.chromium.org/10663018/diff/1001/chrome/browser/speech/speec... chrome/browser/speech/speech_recognition_bubble_controller.h:109: // *** The following are accessed only in the IO thread. i prefer "on ... thread" rather than "in ... thread" http://codereview.chromium.org/10663018/diff/1001/chrome/browser/speech/tab_w... File chrome/browser/speech/tab_watcher.h (right): http://codereview.chromium.org/10663018/diff/1001/chrome/browser/speech/tab_w... chrome/browser/speech/tab_watcher.h:27: // destroyed on the UI thread, due to the NotificationRegistrar dependency. i wonder if the description could be shorted a little... "Simple utility to get a callback when a tab closes." ? http://codereview.chromium.org/10663018/diff/1001/chrome/browser/speech/tab_w... chrome/browser/speech/tab_watcher.h:29: : public base::RefCountedThreadSafe<TabWatcher>, looks like some extra space between the : and the p http://codereview.chromium.org/10663018/diff/1001/chrome/browser/speech/tab_w... chrome/browser/speech/tab_watcher.h:43: static void AbortAllSessionsForWebContentOnIOThread(int render_process_id, what does this do? what's a session in the context of TabWatcher? Hmm, I can't even find where this is defined.. doesn't seem to be in the .cc file?
http://codereview.chromium.org/10663018/diff/1001/chrome/browser/speech/chrom... File chrome/browser/speech/chrome_speech_recognition_manager_delegate.h (right): http://codereview.chromium.org/10663018/diff/1001/chrome/browser/speech/chrom... chrome/browser/speech/chrome_speech_recognition_manager_delegate.h:83: // Callback called by |tab_watcher_| on the IO thread to notify tab closure. On 2012/06/26 13:10:44, hans wrote: > "to notify tab closure" doesn't sound quite right.. maybe "to signal tab > closure" is better? yup http://codereview.chromium.org/10663018/diff/1001/chrome/browser/speech/speec... File chrome/browser/speech/speech_recognition_bubble_controller.cc (right): http://codereview.chromium.org/10663018/diff/1001/chrome/browser/speech/speec... chrome/browser/speech/speech_recognition_bubble_controller.cc:31: current_bubble_render_view_id_() { On 2012/06/26 13:10:44, hans wrote: > i'd prefer current_bubble_render_process_id_(0) instead of > current_bubble_render_process_id_().. the same for the render view id Done. http://codereview.chromium.org/10663018/diff/1001/chrome/browser/speech/speec... chrome/browser/speech/speech_recognition_bubble_controller.cc:43: BrowserThread::CurrentlyOn(BrowserThread::IO); On 2012/06/26 13:10:44, hans wrote: > is this supposed to be in a DCHECK? Ops, yes! :) http://codereview.chromium.org/10663018/diff/1001/chrome/browser/speech/speec... File chrome/browser/speech/speech_recognition_bubble_controller.h (right): http://codereview.chromium.org/10663018/diff/1001/chrome/browser/speech/speec... chrome/browser/speech/speech_recognition_bubble_controller.h:109: // *** The following are accessed only in the IO thread. On 2012/06/26 13:10:44, hans wrote: > i prefer "on ... thread" rather than "in ... thread" Fixed here and in the other comments. http://codereview.chromium.org/10663018/diff/1001/chrome/browser/speech/tab_w... File chrome/browser/speech/tab_watcher.h (right): http://codereview.chromium.org/10663018/diff/1001/chrome/browser/speech/tab_w... chrome/browser/speech/tab_watcher.h:27: // destroyed on the UI thread, due to the NotificationRegistrar dependency. On 2012/06/26 13:10:44, hans wrote: > i wonder if the description could be shorted a little... "Simple utility to get > a callback when a tab closes." ? Done. http://codereview.chromium.org/10663018/diff/1001/chrome/browser/speech/tab_w... chrome/browser/speech/tab_watcher.h:29: : public base::RefCountedThreadSafe<TabWatcher>, On 2012/06/26 13:10:44, hans wrote: > looks like some extra space between the : and the p Done. http://codereview.chromium.org/10663018/diff/1001/chrome/browser/speech/tab_w... chrome/browser/speech/tab_watcher.h:43: static void AbortAllSessionsForWebContentOnIOThread(int render_process_id, On 2012/06/26 13:10:44, hans wrote: > what does this do? what's a session in the context of TabWatcher? > Hmm, I can't even find where this is defined.. doesn't seem to be in the .cc > file? Ops, I guess it was just a too large copy/paste, which unfortunately did not end into any compiler error :/
On 2012/06/26 14:27:07, Primiano Tucci wrote: > http://codereview.chromium.org/10663018/diff/1001/chrome/browser/speech/chrom... > File chrome/browser/speech/chrome_speech_recognition_manager_delegate.h (right): > > http://codereview.chromium.org/10663018/diff/1001/chrome/browser/speech/chrom... > chrome/browser/speech/chrome_speech_recognition_manager_delegate.h:83: // > Callback called by |tab_watcher_| on the IO thread to notify tab closure. > On 2012/06/26 13:10:44, hans wrote: > > "to notify tab closure" doesn't sound quite right.. maybe "to signal tab > > closure" is better? > yup > > http://codereview.chromium.org/10663018/diff/1001/chrome/browser/speech/speec... > File chrome/browser/speech/speech_recognition_bubble_controller.cc (right): > > http://codereview.chromium.org/10663018/diff/1001/chrome/browser/speech/speec... > chrome/browser/speech/speech_recognition_bubble_controller.cc:31: > current_bubble_render_view_id_() { > On 2012/06/26 13:10:44, hans wrote: > > i'd prefer current_bubble_render_process_id_(0) instead of > > current_bubble_render_process_id_().. the same for the render view id > > Done. > > http://codereview.chromium.org/10663018/diff/1001/chrome/browser/speech/speec... > chrome/browser/speech/speech_recognition_bubble_controller.cc:43: > BrowserThread::CurrentlyOn(BrowserThread::IO); > On 2012/06/26 13:10:44, hans wrote: > > is this supposed to be in a DCHECK? > Ops, yes! :) > > http://codereview.chromium.org/10663018/diff/1001/chrome/browser/speech/speec... > File chrome/browser/speech/speech_recognition_bubble_controller.h (right): > > http://codereview.chromium.org/10663018/diff/1001/chrome/browser/speech/speec... > chrome/browser/speech/speech_recognition_bubble_controller.h:109: // *** The > following are accessed only in the IO thread. > On 2012/06/26 13:10:44, hans wrote: > > i prefer "on ... thread" rather than "in ... thread" > > Fixed here and in the other comments. > > http://codereview.chromium.org/10663018/diff/1001/chrome/browser/speech/tab_w... > File chrome/browser/speech/tab_watcher.h (right): > > http://codereview.chromium.org/10663018/diff/1001/chrome/browser/speech/tab_w... > chrome/browser/speech/tab_watcher.h:27: // destroyed on the UI thread, due to > the NotificationRegistrar dependency. > On 2012/06/26 13:10:44, hans wrote: > > i wonder if the description could be shorted a little... "Simple utility to > get > > a callback when a tab closes." ? > > Done. > > http://codereview.chromium.org/10663018/diff/1001/chrome/browser/speech/tab_w... > chrome/browser/speech/tab_watcher.h:29: : public > base::RefCountedThreadSafe<TabWatcher>, > On 2012/06/26 13:10:44, hans wrote: > > looks like some extra space between the : and the p > > Done. > > http://codereview.chromium.org/10663018/diff/1001/chrome/browser/speech/tab_w... > chrome/browser/speech/tab_watcher.h:43: static void > AbortAllSessionsForWebContentOnIOThread(int render_process_id, > On 2012/06/26 13:10:44, hans wrote: > > what does this do? what's a session in the context of TabWatcher? > > Hmm, I can't even find where this is defined.. doesn't seem to be in the .cc > > file? > Ops, I guess it was just a too large copy/paste, which unfortunately did not end > into any compiler error :/ lgtm
Partial review comments below. I found that in http://codereview.chromium.org/3352018 we added the concept of multiple bubble objects being alive in the controller even though only 1 is visible at a time. The use case mentioned in that CL is when a network error or server error happens and we let the user retry by clicking the retry button in the bubble itself. Have you tested that scenario with this CL to verify if it works as expected? http://codereview.chromium.org/10663018/diff/14001/chrome/browser/speech/tab_... File chrome/browser/speech/tab_watcher.cc (right): http://codereview.chromium.org/10663018/diff/14001/chrome/browser/speech/tab_... chrome/browser/speech/tab_watcher.cc:25: : tab_closed_callback_(tab_closed_callback), indent this and next line by 2 spaces http://codereview.chromium.org/10663018/diff/14001/chrome/browser/speech/tab_... chrome/browser/speech/tab_watcher.cc:35: int render_process_id, int render_view_id) { seems like this can fit in previous line? http://codereview.chromium.org/10663018/diff/14001/chrome/browser/speech/tab_... chrome/browser/speech/tab_watcher.cc:42: DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); this seems unnecessary given the if statement above http://codereview.chromium.org/10663018/diff/14001/chrome/browser/speech/tab_... chrome/browser/speech/tab_watcher.cc:46: // Sessions initiated by extensions might not have a corresponding WebContent. 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? http://codereview.chromium.org/10663018/diff/14001/chrome/browser/speech/tab_... chrome/browser/speech/tab_watcher.cc:71: WebContents* web_contents = content::Source<WebContents>(source).ptr(); 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 http://codereview.chromium.org/10663018/diff/14001/chrome/browser/speech/tab_... File chrome/browser/speech/tab_watcher.h (right): http://codereview.chromium.org/10663018/diff/14001/chrome/browser/speech/tab_... chrome/browser/speech/tab_watcher.h:22: // Simple utility to get a callback when a tab closes. Both the callback site get a callback -> get notified http://codereview.chromium.org/10663018/diff/14001/chrome/browser/speech/tab_... chrome/browser/speech/tab_watcher.h:36: void Watch(int render_process_id, int render_view_id); add comment http://codereview.chromium.org/10663018/diff/14001/chrome/browser/speech/tab_... chrome/browser/speech/tab_watcher.h:54: std::set<content::WebContents*> registered_contents_; registered_web_contents_
I saw that CL, however cannot still understand why, even in case of error, we need to keep track of more than one bubble. I checked the cases of server error, try again, cancel and so on and they seem to work fine also after this CL. In any case, in fact, it seems to me that we never have more than one active bubble. https://chromiumcodereview.appspot.com/10663018/diff/14001/chrome/browser/spe... File chrome/browser/speech/tab_watcher.cc (right): https://chromiumcodereview.appspot.com/10663018/diff/14001/chrome/browser/spe... chrome/browser/speech/tab_watcher.cc:25: : tab_closed_callback_(tab_closed_callback), On 2012/07/02 21:44:41, Satish wrote: > indent this and next line by 2 spaces Done. https://chromiumcodereview.appspot.com/10663018/diff/14001/chrome/browser/spe... chrome/browser/speech/tab_watcher.cc:35: int render_process_id, int render_view_id) { On 2012/07/02 21:44:41, Satish wrote: > seems like this can fit in previous line? Oh right! https://chromiumcodereview.appspot.com/10663018/diff/14001/chrome/browser/spe... chrome/browser/speech/tab_watcher.cc:42: DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); On 2012/07/02 21:44:41, Satish wrote: > this seems unnecessary given the if statement above Done. https://chromiumcodereview.appspot.com/10663018/diff/14001/chrome/browser/spe... chrome/browser/speech/tab_watcher.cc:46: // Sessions initiated by extensions might not have a corresponding WebContent. On 2012/07/02 21:44:41, Satish wrote: > 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? 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. https://chromiumcodereview.appspot.com/10663018/diff/14001/chrome/browser/spe... chrome/browser/speech/tab_watcher.cc:71: WebContents* web_contents = content::Source<WebContents>(source).ptr(); On 2012/07/02 21:44:41, Satish wrote: > 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 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. https://chromiumcodereview.appspot.com/10663018/diff/14001/chrome/browser/spe... File chrome/browser/speech/tab_watcher.h (right): https://chromiumcodereview.appspot.com/10663018/diff/14001/chrome/browser/spe... chrome/browser/speech/tab_watcher.h:22: // Simple utility to get a callback when a tab closes. Both the callback site On 2012/07/02 21:44:41, Satish wrote: > get a callback -> get notified Done. https://chromiumcodereview.appspot.com/10663018/diff/14001/chrome/browser/spe... chrome/browser/speech/tab_watcher.h:36: void Watch(int render_process_id, int render_view_id); On 2012/07/02 21:44:41, Satish wrote: > add comment Done. https://chromiumcodereview.appspot.com/10663018/diff/14001/chrome/browser/spe... chrome/browser/speech/tab_watcher.h:54: std::set<content::WebContents*> registered_contents_; On 2012/07/02 21:44:41, Satish wrote: > registered_web_contents_ Done.
Remaining comments.. https://chromiumcodereview.appspot.com/10663018/diff/15015/chrome/browser/spe... File chrome/browser/speech/chrome_speech_recognition_manager_delegate.cc (right): https://chromiumcodereview.appspot.com/10663018/diff/15015/chrome/browser/spe... chrome/browser/speech/chrome_speech_recognition_manager_delegate.cc:125: { move to previous line https://chromiumcodereview.appspot.com/10663018/diff/15015/chrome/browser/spe... chrome/browser/speech/chrome_speech_recognition_manager_delegate.cc:129: ~ChromeSpeechRecognitionManagerDelegate() { could move to previous line https://chromiumcodereview.appspot.com/10663018/diff/15015/chrome/browser/spe... chrome/browser/speech/chrome_speech_recognition_manager_delegate.cc:132: if (bubble_controller_.get() && bubble_controller_->GetActiveSessionID()) 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 https://chromiumcodereview.appspot.com/10663018/diff/15015/chrome/browser/spe... chrome/browser/speech/chrome_speech_recognition_manager_delegate.cc:153: } if the above are the only 2 possible values for |button|, can you add an else with NOTREACHED here? https://chromiumcodereview.appspot.com/10663018/diff/15015/chrome/browser/spe... chrome/browser/speech/chrome_speech_recognition_manager_delegate.cc:164: if (GetBubbleController()->GetActiveSessionID() != session_id) 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? https://chromiumcodereview.appspot.com/10663018/diff/15015/chrome/browser/spe... chrome/browser/speech/chrome_speech_recognition_manager_delegate.cc:192: if (SpeechRecognitionManager* mgr = SpeechRecognitionManager::GetInstance()) mgr -> manager https://chromiumcodereview.appspot.com/10663018/diff/15015/chrome/browser/spe... chrome/browser/speech/chrome_speech_recognition_manager_delegate.cc:196: bubble_controller_->IsShowingBubbleOn(render_process_id, 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? https://chromiumcodereview.appspot.com/10663018/diff/15015/chrome/browser/spe... chrome/browser/speech/chrome_speech_recognition_manager_delegate.cc:227: tab_watcher_->Watch(context.render_process_id, context.render_view_id); can you also stop observing the tab when the bubble closes, so we don't end up listening for that tab forever https://chromiumcodereview.appspot.com/10663018/diff/15015/chrome/browser/spe... chrome/browser/speech/chrome_speech_recognition_manager_delegate.cc:232: DCHECK_EQ(session_id, GetBubbleController()->GetActiveSessionID()); 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? https://chromiumcodereview.appspot.com/10663018/diff/15015/chrome/browser/spe... chrome/browser/speech/chrome_speech_recognition_manager_delegate.cc:262: if (GetBubbleController()->GetActiveSessionID() == session_id) { why is this condition different from what you have in OnAudioStart, i.e. the if statement and DCHECK are reversed? https://chromiumcodereview.appspot.com/10663018/diff/15015/chrome/browser/spe... File chrome/browser/speech/speech_recognition_bubble_controller.cc (right): https://chromiumcodereview.appspot.com/10663018/diff/15015/chrome/browser/spe... chrome/browser/speech/speech_recognition_bubble_controller.cc:21: const int kNoBubble = 0; since this is used for the session id, rename to kInvalidSessionId ? https://chromiumcodereview.appspot.com/10663018/diff/15015/chrome/browser/spe... File chrome/browser/speech/speech_recognition_bubble_controller.h (right): https://chromiumcodereview.appspot.com/10663018/diff/15015/chrome/browser/spe... chrome/browser/speech/speech_recognition_bubble_controller.h:14: namespace gfx { should remove this now since you are including the header? https://chromiumcodereview.appspot.com/10663018/diff/15015/chrome/browser/spe... File chrome/browser/speech/tab_watcher.cc (right): https://chromiumcodereview.appspot.com/10663018/diff/15015/chrome/browser/spe... chrome/browser/speech/tab_watcher.cc:43: // Sessions initiated by speech input extension APIs (soon to be deprecated) could remove '(soon to be deprecated)' https://chromiumcodereview.appspot.com/10663018/diff/15015/chrome/browser/spe... chrome/browser/speech/tab_watcher.cc:48: // APIs will be deprecated. 'APIs will be' -> 'API is' https://chromiumcodereview.appspot.com/10663018/diff/15015/chrome/browser/spe... File chrome/browser/speech/tab_watcher.h (right): https://chromiumcodereview.appspot.com/10663018/diff/15015/chrome/browser/spe... chrome/browser/speech/tab_watcher.h:37: // Register, for later receiving a closure notification, the WebContent this comment is talking about how the method is implemented. Instead can you mention what is useful for the caller, e.g. "Start monitoring the WebContents corresponding to the given |render_process_id|, |render_view_id| pair and if it is closed/unloaded invoke the callback" https://chromiumcodereview.appspot.com/10663018/diff/15015/content/public/bro... File content/public/browser/speech_recognition_manager.h (right): https://chromiumcodereview.appspot.com/10663018/diff/15015/content/public/bro... content/public/browser/speech_recognition_manager.h:53: virtual void AbortAllSessionsForRenderer( should be 'ForTab' since you are also passing a render-view-id ?
https://chromiumcodereview.appspot.com/10663018/diff/15015/chrome/browser/spe... File chrome/browser/speech/chrome_speech_recognition_manager_delegate.cc (right): https://chromiumcodereview.appspot.com/10663018/diff/15015/chrome/browser/spe... chrome/browser/speech/chrome_speech_recognition_manager_delegate.cc:125: { On 2012/07/03 15:11:57, Satish wrote: > move to previous line It will overflow the row. Chrome....Delegate() takes exactly 80 cols :) https://chromiumcodereview.appspot.com/10663018/diff/15015/chrome/browser/spe... chrome/browser/speech/chrome_speech_recognition_manager_delegate.cc:129: ~ChromeSpeechRecognitionManagerDelegate() { On 2012/07/03 15:11:57, Satish wrote: > could move to previous line As above, overflows 80 cols. https://chromiumcodereview.appspot.com/10663018/diff/15015/chrome/browser/spe... chrome/browser/speech/chrome_speech_recognition_manager_delegate.cc:132: if (bubble_controller_.get() && bubble_controller_->GetActiveSessionID()) On 2012/07/03 15:11:57, Satish wrote: > can the second check be moved inside CloseBubble() Well, the check wasn't needed. Removed. >, so we can remove > GetActiveSessionID() as well? Mmm see later discussion on this point. > Alternatively SpeechRecognitionBubbleController > can close the active bubble in the destructor without > enforcing that the caller > should do it 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. https://chromiumcodereview.appspot.com/10663018/diff/15015/chrome/browser/spe... chrome/browser/speech/chrome_speech_recognition_manager_delegate.cc:153: } On 2012/07/03 15:11:57, Satish wrote: > if the above are the only 2 possible values for |button|, can you add an else > with NOTREACHED here? Done. https://chromiumcodereview.appspot.com/10663018/diff/15015/chrome/browser/spe... chrome/browser/speech/chrome_speech_recognition_manager_delegate.cc:164: if (GetBubbleController()->GetActiveSessionID() != session_id) On 2012/07/03 15:11:57, Satish wrote: > 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? 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. > 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? https://chromiumcodereview.appspot.com/10663018/diff/15015/chrome/browser/spe... chrome/browser/speech/chrome_speech_recognition_manager_delegate.cc:192: if (SpeechRecognitionManager* mgr = SpeechRecognitionManager::GetInstance()) On 2012/07/03 15:11:57, Satish wrote: > mgr -> manager Done. https://chromiumcodereview.appspot.com/10663018/diff/15015/chrome/browser/spe... chrome/browser/speech/chrome_speech_recognition_manager_delegate.cc:196: bubble_controller_->IsShowingBubbleOn(render_process_id, On 2012/07/03 15:11:57, Satish wrote: > 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? 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. https://chromiumcodereview.appspot.com/10663018/diff/15015/chrome/browser/spe... chrome/browser/speech/chrome_speech_recognition_manager_delegate.cc:227: tab_watcher_->Watch(context.render_process_id, context.render_view_id); On 2012/07/03 15:11:57, Satish wrote: > can you also stop observing the tab when the bubble closes, so we don't end up > listening for that tab forever 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? https://chromiumcodereview.appspot.com/10663018/diff/15015/chrome/browser/spe... chrome/browser/speech/chrome_speech_recognition_manager_delegate.cc:232: DCHECK_EQ(session_id, GetBubbleController()->GetActiveSessionID()); On 2012/07/03 15:11:57, Satish wrote: > 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? 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. https://chromiumcodereview.appspot.com/10663018/diff/15015/chrome/browser/spe... chrome/browser/speech/chrome_speech_recognition_manager_delegate.cc:262: if (GetBubbleController()->GetActiveSessionID() == session_id) { On 2012/07/03 15:11:57, Satish wrote: > why is this condition different from what you have in OnAudioStart, i.e. the if > statement and DCHECK are reversed? 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. https://chromiumcodereview.appspot.com/10663018/diff/15015/chrome/browser/spe... File chrome/browser/speech/speech_recognition_bubble_controller.cc (right): https://chromiumcodereview.appspot.com/10663018/diff/15015/chrome/browser/spe... chrome/browser/speech/speech_recognition_bubble_controller.cc:21: const int kNoBubble = 0; On 2012/07/03 15:11:57, Satish wrote: > since this is used for the session id, rename to kInvalidSessionId ? Done. https://chromiumcodereview.appspot.com/10663018/diff/15015/chrome/browser/spe... File chrome/browser/speech/speech_recognition_bubble_controller.h (right): https://chromiumcodereview.appspot.com/10663018/diff/15015/chrome/browser/spe... chrome/browser/speech/speech_recognition_bubble_controller.h:14: namespace gfx { On 2012/07/03 15:11:57, Satish wrote: > should remove this now since you are including the header? Done. https://chromiumcodereview.appspot.com/10663018/diff/15015/chrome/browser/spe... File chrome/browser/speech/tab_watcher.cc (right): https://chromiumcodereview.appspot.com/10663018/diff/15015/chrome/browser/spe... chrome/browser/speech/tab_watcher.cc:43: // Sessions initiated by speech input extension APIs (soon to be deprecated) On 2012/07/03 15:11:57, Satish wrote: > could remove '(soon to be deprecated)' Done. https://chromiumcodereview.appspot.com/10663018/diff/15015/chrome/browser/spe... chrome/browser/speech/tab_watcher.cc:48: // APIs will be deprecated. On 2012/07/03 15:11:57, Satish wrote: > 'APIs will be' -> 'API is' Done. https://chromiumcodereview.appspot.com/10663018/diff/15015/chrome/browser/spe... File chrome/browser/speech/tab_watcher.h (right): https://chromiumcodereview.appspot.com/10663018/diff/15015/chrome/browser/spe... chrome/browser/speech/tab_watcher.h:37: // Register, for later receiving a closure notification, the WebContent On 2012/07/03 15:11:57, Satish wrote: > this comment is talking about how the method is implemented. Instead can you > mention what is useful for the caller, e.g. > "Start monitoring the WebContents corresponding to the given > |render_process_id|, |render_view_id| pair and if it is closed/unloaded invoke > the callback" Done. https://chromiumcodereview.appspot.com/10663018/diff/15015/content/public/bro... File content/public/browser/speech_recognition_manager.h (right): https://chromiumcodereview.appspot.com/10663018/diff/15015/content/public/bro... content/public/browser/speech_recognition_manager.h:53: virtual void AbortAllSessionsForRenderer( On 2012/07/03 15:11:57, Satish wrote: > should be 'ForTab' since you are also passing a render-view-id ? Wouldn't such rename exclude the background renderer of extensions?
https://chromiumcodereview.appspot.com/10663018/diff/15015/chrome/browser/spe... File chrome/browser/speech/chrome_speech_recognition_manager_delegate.cc (right): https://chromiumcodereview.appspot.com/10663018/diff/15015/chrome/browser/spe... chrome/browser/speech/chrome_speech_recognition_manager_delegate.cc:125: { On 2012/07/03 16:54:35, Primiano Tucci wrote: > On 2012/07/03 15:11:57, Satish wrote: > > move to previous line > It will overflow the row. Chrome....Delegate() takes exactly 80 cols :) In such cases we write it as ChromeSpeechRecognitionManagerDelegate ::ChromeSpeechRecognitionManagerDelegate() { } https://chromiumcodereview.appspot.com/10663018/diff/15015/chrome/browser/spe... chrome/browser/speech/chrome_speech_recognition_manager_delegate.cc:129: ~ChromeSpeechRecognitionManagerDelegate() { On 2012/07/03 16:54:35, Primiano Tucci wrote: > On 2012/07/03 15:11:57, Satish wrote: > > could move to previous line > As above, overflows 80 cols. Ok, no 4 space indentation then https://chromiumcodereview.appspot.com/10663018/diff/15015/chrome/browser/spe... chrome/browser/speech/chrome_speech_recognition_manager_delegate.cc:227: tab_watcher_->Watch(context.render_process_id, context.render_view_id); On 2012/07/03 16:54:35, Primiano Tucci wrote: > On 2012/07/03 15:11:57, Satish wrote: > > can you also stop observing the tab when the bubble closes, so we don't end up > > listening for that tab forever > 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? 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 https://chromiumcodereview.appspot.com/10663018/diff/21006/chrome/browser/spe... File chrome/browser/speech/speech_recognition_bubble_controller.cc (right): https://chromiumcodereview.appspot.com/10663018/diff/21006/chrome/browser/spe... chrome/browser/speech/speech_recognition_bubble_controller.cc:90: bool SpeechRecognitionBubbleController::IsShowingBubbleOn(int render_process_id, suggest renaming this to IsShowingBubbleForTab or something similar
https://chromiumcodereview.appspot.com/10663018/diff/15015/chrome/browser/spe... File chrome/browser/speech/chrome_speech_recognition_manager_delegate.cc (right): https://chromiumcodereview.appspot.com/10663018/diff/15015/chrome/browser/spe... chrome/browser/speech/chrome_speech_recognition_manager_delegate.cc:125: { On 2012/07/04 09:52:51, Satish wrote: > On 2012/07/03 16:54:35, Primiano Tucci wrote: > > On 2012/07/03 15:11:57, Satish wrote: > > > move to previous line > > It will overflow the row. Chrome....Delegate() takes exactly 80 cols :) > > In such cases we write it as > ChromeSpeechRecognitionManagerDelegate > ::ChromeSpeechRecognitionManagerDelegate() { > } Done. https://chromiumcodereview.appspot.com/10663018/diff/15015/chrome/browser/spe... chrome/browser/speech/chrome_speech_recognition_manager_delegate.cc:129: ~ChromeSpeechRecognitionManagerDelegate() { On 2012/07/04 09:52:51, Satish wrote: > On 2012/07/03 16:54:35, Primiano Tucci wrote: > > On 2012/07/03 15:11:57, Satish wrote: > > > could move to previous line > > As above, overflows 80 cols. > > Ok, no 4 space indentation then Done. https://chromiumcodereview.appspot.com/10663018/diff/15015/chrome/browser/spe... chrome/browser/speech/chrome_speech_recognition_manager_delegate.cc:227: tab_watcher_->Watch(context.render_process_id, context.render_view_id); On 2012/07/04 09:52:51, Satish wrote: > On 2012/07/03 16:54:35, Primiano Tucci wrote: > > On 2012/07/03 15:11:57, Satish wrote: > > > can you also stop observing the tab when the bubble closes, so we don't end > up > > > listening for that tab forever > > 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? > > 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 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. https://chromiumcodereview.appspot.com/10663018/diff/21006/chrome/browser/spe... File chrome/browser/speech/speech_recognition_bubble_controller.cc (right): https://chromiumcodereview.appspot.com/10663018/diff/21006/chrome/browser/spe... chrome/browser/speech/speech_recognition_bubble_controller.cc:90: bool SpeechRecognitionBubbleController::IsShowingBubbleOn(int render_process_id, On 2012/07/04 09:52:51, Satish wrote: > suggest renaming this to IsShowingBubbleForTab or something similar IsShowingBubbleForWebContent
lgtm
+jam@ for owners approval on /content/public
http://codereview.chromium.org/10663018/diff/29001/content/browser/speech/spe... File content/browser/speech/speech_recognition_browsertest.cc (right): http://codereview.chromium.org/10663018/diff/29001/content/browser/speech/spe... content/browser/speech/speech_recognition_browsertest.cc:129: virtual void AbortAllSessionsForWebContent(int, int) { NOTREACHED(); } OVERRIDE missing? also, I wouldn't leave out the parameter names, as it's not clear from the function names what they mean http://codereview.chromium.org/10663018/diff/29001/content/browser/speech/spe... File content/browser/speech/speech_recognition_manager_impl.cc (right): http://codereview.chromium.org/10663018/diff/29001/content/browser/speech/spe... content/browser/speech/speech_recognition_manager_impl.cc:341: int render_process_id, int render_view_id) { nit. each argument should go on it's own line if you wrap like this http://codereview.chromium.org/10663018/diff/29001/content/public/browser/spe... File content/public/browser/speech_recognition_manager.h (right): http://codereview.chromium.org/10663018/diff/29001/content/public/browser/spe... content/public/browser/speech_recognition_manager.h:54: int render_process_id, int render_view_id) = 0; What about claling this ...ForRenderView? A web contents can have multiple render views attached at the same time. and a render view doesn't necessarily have to have a web contents as delegate. nit. each parameter on its own line
http://codereview.chromium.org/10663018/diff/29001/content/browser/speech/spe... File content/browser/speech/speech_recognition_browsertest.cc (right): http://codereview.chromium.org/10663018/diff/29001/content/browser/speech/spe... content/browser/speech/speech_recognition_browsertest.cc:129: virtual void AbortAllSessionsForWebContent(int, int) { NOTREACHED(); } On 2012/07/05 07:21:46, jochen wrote: > OVERRIDE missing? > > also, I wouldn't leave out the parameter names, as it's not clear from the > function names what they mean Oh right. I omitted the names since they were not used in this method, to keep its signature compact. Btw now I've to wrap anyway, so I restored them. http://codereview.chromium.org/10663018/diff/29001/content/browser/speech/spe... File content/browser/speech/speech_recognition_manager_impl.cc (right): http://codereview.chromium.org/10663018/diff/29001/content/browser/speech/spe... content/browser/speech/speech_recognition_manager_impl.cc:341: int render_process_id, int render_view_id) { On 2012/07/05 07:21:46, jochen wrote: > nit. each argument should go on it's own line if you wrap like this Done. http://codereview.chromium.org/10663018/diff/29001/content/public/browser/spe... File content/public/browser/speech_recognition_manager.h (right): http://codereview.chromium.org/10663018/diff/29001/content/public/browser/spe... content/public/browser/speech_recognition_manager.h:54: int render_process_id, int render_view_id) = 0; On 2012/07/05 07:21:46, jochen wrote: > What about claling this ...ForRenderView? A web contents can have multiple > render views attached at the same time. and a render view doesn't necessarily > have to have a web contents as delegate. > > nit. each parameter on its own line Ah sorry it was not completely clear to me the difference between the two. Renamed here and in the speech_recognition_bubble_controller.cc. Thanks
http://codereview.chromium.org/10663018/diff/16017/chrome/browser/speech/tab_... File chrome/browser/speech/tab_watcher.h (right): http://codereview.chromium.org/10663018/diff/16017/chrome/browser/speech/tab_... chrome/browser/speech/tab_watcher.h:27: class TabWatcher 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.
http://codereview.chromium.org/10663018/diff/16017/chrome/browser/speech/tab_... File chrome/browser/speech/tab_watcher.h (right): http://codereview.chromium.org/10663018/diff/16017/chrome/browser/speech/tab_... chrome/browser/speech/tab_watcher.h:27: class TabWatcher On 2012/07/05 17:58:56, John Abd-El-Malek wrote: > 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 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.
content/public lgtm, two small nits, and a suggestion I hope you'll take to hide the TabWatcher class http://codereview.chromium.org/10663018/diff/16017/chrome/browser/speech/tab_... File chrome/browser/speech/tab_watcher.cc (right): http://codereview.chromium.org/10663018/diff/16017/chrome/browser/speech/tab_... chrome/browser/speech/tab_watcher.cc:20: const int kNotificationType = content::NOTIFICATION_WEB_CONTENTS_DISCONNECTED; nit: i think this is unnecessary, it's ok to repeat this below twice; the extra level of indirection makes it harder to see which notification is used http://codereview.chromium.org/10663018/diff/16017/chrome/browser/speech/tab_... chrome/browser/speech/tab_watcher.cc:76: DCHECK(web_contents); nit: this is unnecessary. if it's NULL, the line below will crash which is just as good of a signal to the developer http://codereview.chromium.org/10663018/diff/16017/chrome/browser/speech/tab_... File chrome/browser/speech/tab_watcher.h (right): http://codereview.chromium.org/10663018/diff/16017/chrome/browser/speech/tab_... chrome/browser/speech/tab_watcher.h:27: class TabWatcher On 2012/07/05 21:21:50, Primiano Tucci wrote: > On 2012/07/05 17:58:56, John Abd-El-Malek wrote: > > 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 > Unfortunately the only caller (the ChromeSpeechRecognitionManagerDelegate) uses > this class from the IO thread, not UI (OnRecognitionStart is called on IO). 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? > 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.
https://chromiumcodereview.appspot.com/10663018/diff/16017/chrome/browser/spe... File chrome/browser/speech/tab_watcher.cc (right): https://chromiumcodereview.appspot.com/10663018/diff/16017/chrome/browser/spe... chrome/browser/speech/tab_watcher.cc:20: const int kNotificationType = content::NOTIFICATION_WEB_CONTENTS_DISCONNECTED; On 2012/07/06 06:58:59, John Abd-El-Malek wrote: > nit: i think this is unnecessary, it's ok to repeat this below twice; the extra > level of indirection makes it harder to see which notification is used Done. https://chromiumcodereview.appspot.com/10663018/diff/16017/chrome/browser/spe... chrome/browser/speech/tab_watcher.cc:76: DCHECK(web_contents); On 2012/07/06 06:58:59, John Abd-El-Malek wrote: > nit: this is unnecessary. if it's NULL, the line below will crash which is just > as good of a signal to the developer Done. https://chromiumcodereview.appspot.com/10663018/diff/16017/chrome/browser/spe... File chrome/browser/speech/tab_watcher.h (right): https://chromiumcodereview.appspot.com/10663018/diff/16017/chrome/browser/spe... chrome/browser/speech/tab_watcher.h:27: class TabWatcher On 2012/07/06 06:58:59, John Abd-El-Malek wrote: > On 2012/07/05 21:21:50, Primiano Tucci wrote: > > On 2012/07/05 17:58:56, John Abd-El-Malek wrote: > > > 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 > > Unfortunately the only caller (the ChromeSpeechRecognitionManagerDelegate) > uses > > this class from the IO thread, not UI (OnRecognitionStart is called on IO). > > 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? > > > 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. > Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/primiano@chromium.org/10663018/22009
Change committed as 145586 |
