Implement native speech recognition for the launcher.

Implement native speech recognition for the launcher.

BUG=397019
Committed: https://crrev.com/0c4422d9a0e89da3ca708f7bfdbab3603a4a7279
Cr-Commit-Position: refs/heads/master@{#302767}

[Anand Mistry (off Chromium), 2014-10-30 03:44:29]

amistry@chromium.org changed reviewers:
+ mgiuca@chromium.org, mukai@chromium.org, thestig@chromium.org

[Anand Mistry (off Chromium), 2014-10-30 03:44:30]

mukai@chromium.org: You probably know the start_page_service  /speech stuff the
best.

mgiuca@chromium.org: Please review changes in app_list in general.

thestig@chromium.org: Please review changes in *.gypi

[Lei Zhang, 2014-10-30 05:15:42]

gypi files lgtm

https://codereview.chromium.org/676593003/diff/80001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_speech_recognizer.cc (right):

https://codereview.chromium.org/676593003/diff/80001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_speech_recognizer.cc:18: #include
"ui/app_list/speech_ui_model_observer.h"
not used?

https://codereview.chromium.org/676593003/diff/80001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_speech_recognizer.cc:23: static const int
kNoSpeechTimeout = 5;
Add "InSeconds" to the name so it's obvious you got the units correct.

https://codereview.chromium.org/676593003/diff/80001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_speech_recognizer.cc:60:
scoped_refptr<net::URLRequestContextGetter> url_request_context_getter) {
You should add a DCHECK_CURRENTLY_ON() call here, same with StopOnIOThread().

Please use it liberally with other methods. It's not clear what methods should
run on what thread, and what variables should be accessed on which thread. If
you can avoid having a multi-threaded class, even better.

https://codereview.chromium.org/676593003/diff/80001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_speech_recognizer.cc:144: unsigned int
final_count = 0;
use size_t

https://codereview.chromium.org/676593003/diff/80001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_speech_recognizer.cc:186: int16_t
sound_level = static_cast<int16_t>(INT16_MAX * volume);
can't this overflow if |volume| > 1 ?

https://codereview.chromium.org/676593003/diff/80001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_speech_recognizer.h (right):

https://codereview.chromium.org/676593003/diff/80001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_speech_recognizer.h:14: #include
"ui/app_list/speech_ui_model_observer.h"
not used?

https://codereview.chromium.org/676593003/diff/80001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_speech_recognizer.h:17: struct
SpeechRecognitionSessionConfig;
not need?

https://codereview.chromium.org/676593003/diff/80001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_speech_recognizer.h:24: class
AppListSpeechRecognizer :
colon goes on the next line.

https://codereview.chromium.org/676593003/diff/80001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_speech_recognizer.h:24: class
AppListSpeechRecognizer :
Please write a brief description of what AppListSpeechRecognizer does.

https://codereview.chromium.org/676593003/diff/80001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_speech_recognizer.h:49: void
OnEnvironmentEstimationComplete(int session_id) override {}
implementations, even empties ones, should go in the .cc file.

https://codereview.chromium.org/676593003/diff/80001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_speech_recognizer.h:74: };
DISALLOW_COPY_AND_ASSIGN

[Matt Giuca, 2014-10-30 06:54:13]

Note: I don't have time to look at this today, and I am away tomorrow. I will
prioritize it for Monday but if you need it before then, please reassign.

[Jun Mukai, 2014-10-30 07:08:34]

https://codereview.chromium.org/676593003/diff/80001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_speech_recognizer.h (right):

https://codereview.chromium.org/676593003/diff/80001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_speech_recognizer.h:24: class
AppListSpeechRecognizer :
This is my personal preference, but app_list::AppListSpeechRecognizer sounds a
bit redundant.  Could it be app_list::SpeechRecognizer?

https://codereview.chromium.org/676593003/diff/80001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_speech_recognizer_browsertest.cc
(right):

https://codereview.chromium.org/676593003/diff/80001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_speech_recognizer_browsertest.cc:27: class
MockStartPageService : public StartPageService {
Does it have to be a StartPageService?

If all you need are the following mock methods, I think it's better to declare a
new class AppListSpeechRecognizerDelegate, and both this class and
StartPageService inherit it.

[Anand Mistry (off Chromium), 2014-10-31 00:50:07]

https://codereview.chromium.org/676593003/diff/80001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_speech_recognizer.cc (right):

https://codereview.chromium.org/676593003/diff/80001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_speech_recognizer.cc:18: #include
"ui/app_list/speech_ui_model_observer.h"
On 2014/10/30 05:15:42, Lei Zhang wrote:
> not used?

For SpeechRecognitionState, but also declared in the header.

https://codereview.chromium.org/676593003/diff/80001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_speech_recognizer.cc:23: static const int
kNoSpeechTimeout = 5;
On 2014/10/30 05:15:42, Lei Zhang wrote:
> Add "InSeconds" to the name so it's obvious you got the units correct.

Done.

https://codereview.chromium.org/676593003/diff/80001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_speech_recognizer.cc:60:
scoped_refptr<net::URLRequestContextGetter> url_request_context_getter) {
On 2014/10/30 05:15:42, Lei Zhang wrote:
> You should add a DCHECK_CURRENTLY_ON() call here, same with StopOnIOThread().

Done.

> Please use it liberally with other methods. It's not clear what methods should
> run on what thread, and what variables should be accessed on which thread. If
> you can avoid having a multi-threaded class, even better.

Unfortunately, it can't be avoided. The speech recognizer must be used from the
IO thread, but this class needs to be used from the UI thread.

https://codereview.chromium.org/676593003/diff/80001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_speech_recognizer.cc:144: unsigned int
final_count = 0;
On 2014/10/30 05:15:42, Lei Zhang wrote:
> use size_t

Done.

https://codereview.chromium.org/676593003/diff/80001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_speech_recognizer.cc:186: int16_t
sound_level = static_cast<int16_t>(INT16_MAX * volume);
On 2014/10/30 05:15:42, Lei Zhang wrote:
> can't this overflow if |volume| > 1 ?

It is defined to be in the range [0.0, 1.0]

https://codereview.chromium.org/676593003/diff/80001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_speech_recognizer.h (right):

https://codereview.chromium.org/676593003/diff/80001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_speech_recognizer.h:14: #include
"ui/app_list/speech_ui_model_observer.h"
On 2014/10/30 05:15:42, Lei Zhang wrote:
> not used?

Used for app_list::SpeechRecognitionState enum.

https://codereview.chromium.org/676593003/diff/80001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_speech_recognizer.h:17: struct
SpeechRecognitionSessionConfig;
On 2014/10/30 05:15:42, Lei Zhang wrote:
> not need?

Done.

https://codereview.chromium.org/676593003/diff/80001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_speech_recognizer.h:24: class
AppListSpeechRecognizer :
On 2014/10/30 05:15:42, Lei Zhang wrote:
> colon goes on the next line.

Done.

https://codereview.chromium.org/676593003/diff/80001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_speech_recognizer.h:24: class
AppListSpeechRecognizer :
On 2014/10/30 05:15:42, Lei Zhang wrote:
> Please write a brief description of what AppListSpeechRecognizer does.

Done.

https://codereview.chromium.org/676593003/diff/80001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_speech_recognizer.h:49: void
OnEnvironmentEstimationComplete(int session_id) override {}
On 2014/10/30 05:15:42, Lei Zhang wrote:
> implementations, even empties ones, should go in the .cc file.

Done.

https://codereview.chromium.org/676593003/diff/80001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_speech_recognizer.h:74: };
On 2014/10/30 05:15:42, Lei Zhang wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

https://codereview.chromium.org/676593003/diff/80001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_speech_recognizer_browsertest.cc
(right):

https://codereview.chromium.org/676593003/diff/80001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_speech_recognizer_browsertest.cc:27: class
MockStartPageService : public StartPageService {
On 2014/10/30 07:08:34, Jun Mukai wrote:
> Does it have to be a StartPageService?
> 
> If all you need are the following mock methods, I think it's better to declare
a
> new class AppListSpeechRecognizerDelegate, and both this class and
> StartPageService inherit it.

Done.

[Lei Zhang, 2014-10-31 01:18:04]

https://codereview.chromium.org/676593003/diff/80001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_speech_recognizer.cc (right):

https://codereview.chromium.org/676593003/diff/80001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_speech_recognizer.cc:60:
scoped_refptr<net::URLRequestContextGetter> url_request_context_getter) {
On 2014/10/31 00:50:06, Anand Mistry wrote:
> On 2014/10/30 05:15:42, Lei Zhang wrote:
> > You should add a DCHECK_CURRENTLY_ON() call here, same with
StopOnIOThread().
> 
> Done.
> 
> > Please use it liberally with other methods. It's not clear what methods
should
> > run on what thread, and what variables should be accessed on which thread.
If
> > you can avoid having a multi-threaded class, even better.
> 
> Unfortunately, it can't be avoided. The speech recognizer must be used from
the
> IO thread, but this class needs to be used from the UI thread.

An alternate strategy that may work is to have SpeechRecognizer live completely
on the UI thread, and internal to speech_recognizer.cc, have a class that lives
on the IO thread, with the two classes using PostTask() to talk to each other.
Then each of the two classes is thread-safe and it's easier to reason about
their correctness.

https://codereview.chromium.org/676593003/diff/80001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_speech_recognizer.cc:186: int16_t
sound_level = static_cast<int16_t>(INT16_MAX * volume);
On 2014/10/31 00:50:06, Anand Mistry wrote:
> On 2014/10/30 05:15:42, Lei Zhang wrote:
> > can't this overflow if |volume| > 1 ?
> 
> It is defined to be in the range [0.0, 1.0]

Add a DCHECK?

[Matt Giuca, 2014-11-03 02:35:52]

app_list: lgtm with nits.

I didn't go into a lot of detail about the new SpeechRecognizer class, since
it's not really app_list related. Perhaps you could become a per-file owner of
this file so you can address CLs that come in for this code?

https://codereview.chromium.org/676593003/diff/100001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/speech_recognizer.cc (right):

https://codereview.chromium.org/676593003/diff/100001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/speech_recognizer.cc:21: // 5 second timeout to
cancel if there's no speech heard.
"// Length of timeout ..." (don't include the value in the documentation).

https://codereview.chromium.org/676593003/diff/100001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/speech_recognizer.h (right):

https://codereview.chromium.org/676593003/diff/100001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/speech_recognizer.h:49: // Overridden, but with
trivial implementations.
I don't think this line is necessary.

https://codereview.chromium.org/676593003/diff/100001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/start_page_service.cc (right):

https://codereview.chromium.org/676593003/diff/100001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/start_page_service.cc:142:
!HotwordService::IsExperimentalHotwordingEnabled()) {
Why did you add this?

https://codereview.chromium.org/676593003/diff/100001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/start_page_service.cc:146:
!HotwordService::IsExperimentalHotwordingEnabled();
The second clause can be removed as it is now always true. Rework the comment.

https://codereview.chromium.org/676593003/diff/100001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/start_page_service.h (right):

https://codereview.chromium.org/676593003/diff/100001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/start_page_service.h:66: // Overridden from
app_list::SpeechRecognizerDelegate.
Replace the '.' with a ':'.

(Also, I prefer "app_list::SpeechRecognizerDelegate overrides:", but this is a
personal preference; the two styles are split about 50/50.)

[Anand Mistry (off Chromium), 2014-11-03 06:51:56]

https://codereview.chromium.org/676593003/diff/80001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_speech_recognizer.cc (right):

https://codereview.chromium.org/676593003/diff/80001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_speech_recognizer.cc:60:
scoped_refptr<net::URLRequestContextGetter> url_request_context_getter) {
On 2014/10/31 01:18:04, Lei Zhang wrote:
> On 2014/10/31 00:50:06, Anand Mistry wrote:
> > On 2014/10/30 05:15:42, Lei Zhang wrote:
> > > You should add a DCHECK_CURRENTLY_ON() call here, same with
> StopOnIOThread().
> > 
> > Done.
> > 
> > > Please use it liberally with other methods. It's not clear what methods
> should
> > > run on what thread, and what variables should be accessed on which thread.
> If
> > > you can avoid having a multi-threaded class, even better.
> > 
> > Unfortunately, it can't be avoided. The speech recognizer must be used from
> the
> > IO thread, but this class needs to be used from the UI thread.
> 
> An alternate strategy that may work is to have SpeechRecognizer live
completely
> on the UI thread, and internal to speech_recognizer.cc, have a class that
lives
> on the IO thread, with the two classes using PostTask() to talk to each other.
> Then each of the two classes is thread-safe and it's easier to reason about
> their correctness.

I've tried to do this, and the lifecycle is more correct now. But it's kinda
complicated.

https://codereview.chromium.org/676593003/diff/80001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_speech_recognizer.cc:186: int16_t
sound_level = static_cast<int16_t>(INT16_MAX * volume);
On 2014/10/31 01:18:04, Lei Zhang wrote:
> On 2014/10/31 00:50:06, Anand Mistry wrote:
> > On 2014/10/30 05:15:42, Lei Zhang wrote:
> > > can't this overflow if |volume| > 1 ?
> > 
> > It is defined to be in the range [0.0, 1.0]
> 
> Add a DCHECK?

Done.

https://codereview.chromium.org/676593003/diff/100001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/speech_recognizer.cc (right):

https://codereview.chromium.org/676593003/diff/100001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/speech_recognizer.cc:21: // 5 second timeout to
cancel if there's no speech heard.
On 2014/11/03 02:35:52, Matt Giuca wrote:
> "// Length of timeout ..." (don't include the value in the documentation).

Done.

https://codereview.chromium.org/676593003/diff/100001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/speech_recognizer.h (right):

https://codereview.chromium.org/676593003/diff/100001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/speech_recognizer.h:49: // Overridden, but with
trivial implementations.
On 2014/11/03 02:35:52, Matt Giuca wrote:
> I don't think this line is necessary.

Removed.

https://codereview.chromium.org/676593003/diff/100001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/start_page_service.cc (right):

https://codereview.chromium.org/676593003/diff/100001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/start_page_service.cc:142:
!HotwordService::IsExperimentalHotwordingEnabled()) {
On 2014/11/03 02:35:52, Matt Giuca wrote:
> Why did you add this?

New hotwording doesn't use the web speech api, which is the only thing
onAppListShown does. Once the new stuff is the default, a lot of this code can
be simplified/deleted.

https://codereview.chromium.org/676593003/diff/100001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/start_page_service.cc:146:
!HotwordService::IsExperimentalHotwordingEnabled();
On 2014/11/03 02:35:52, Matt Giuca wrote:
> The second clause can be removed as it is now always true. Rework the comment.

Done.

https://codereview.chromium.org/676593003/diff/100001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/start_page_service.h (right):

https://codereview.chromium.org/676593003/diff/100001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/start_page_service.h:66: // Overridden from
app_list::SpeechRecognizerDelegate.
On 2014/11/03 02:35:52, Matt Giuca wrote:
> Replace the '.' with a ':'.
> 
> (Also, I prefer "app_list::SpeechRecognizerDelegate overrides:", but this is a
> personal preference; the two styles are split about 50/50.)

Done.

[Matt Giuca, 2014-11-04 00:41:39]

lgtm with nits

https://codereview.chromium.org/676593003/diff/140001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/speech_recognizer.cc (right):

https://codereview.chromium.org/676593003/diff/140001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/speech_recognizer.cc:223: DCHECK(volume >= 0.0 &&
volume <= 1.0);
Split these into two separate DCHECKs each (so, four total), and use DCHECK_GE
and DCHECK_LE.

https://codereview.chromium.org/676593003/diff/140001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/speech_recognizer.cc:269: return NULL;
nullptr

[Anand Mistry (off Chromium), 2014-11-05 00:31:09]

If there are no objections, I'll commit this today.

https://codereview.chromium.org/676593003/diff/140001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/speech_recognizer.cc (right):

https://codereview.chromium.org/676593003/diff/140001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/speech_recognizer.cc:223: DCHECK(volume >= 0.0 &&
volume <= 1.0);
On 2014/11/04 00:41:39, Matt Giuca wrote:
> Split these into two separate DCHECKs each (so, four total), and use DCHECK_GE
> and DCHECK_LE.

Done.

https://codereview.chromium.org/676593003/diff/140001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/speech_recognizer.cc:269: return NULL;
On 2014/11/04 00:41:39, Matt Giuca wrote:
> nullptr

Done.

[Lei Zhang, 2014-11-05 00:54:15]

https://codereview.chromium.org/676593003/diff/180001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/speech_recognizer.cc (right):

https://codereview.chromium.org/676593003/diff/180001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/speech_recognizer.cc:249: void
SpeechRecognizer::EventListener::OnSpeechResult(
Why not put this in SpeechRecognizer?

https://codereview.chromium.org/676593003/diff/180001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/speech_recognizer.h (right):

https://codereview.chromium.org/676593003/diff/180001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/speech_recognizer.h:24: class SpeechRecognizer :
public base::RefCountedThreadSafe<SpeechRecognizer> {
Does this still need to be RefCountedThreadSafe now that it lives on a single
thread?

[Lei Zhang, 2014-11-05 01:02:37]

On 2014/11/03 06:51:56, Anand Mistry wrote:
> I've tried to do this, and the lifecycle is more correct now. But it's kinda
> complicated.

BTW, this is exactly why I'm giving this extra attention. I want to help
simplify the design so it's easier to understand and maintain. I don't
understand all the bits here, so I'm just concentrating on SpeechRecognizer and
trusting you/Matt to made sure the rest is ok.

[Anand Mistry (off Chromium), 2014-11-05 02:58:33]

https://codereview.chromium.org/676593003/diff/180001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/speech_recognizer.cc (right):

https://codereview.chromium.org/676593003/diff/180001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/speech_recognizer.cc:249: void
SpeechRecognizer::EventListener::OnSpeechResult(
On 2014/11/05 00:54:15, Lei Zhang wrote:
> Why not put this in SpeechRecognizer?

I've learned that using WeakPtr<> with callbacks "Just Works", so none of this
is necessary. Yay WeakPtr!!!

https://codereview.chromium.org/676593003/diff/180001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/speech_recognizer.h (right):

https://codereview.chromium.org/676593003/diff/180001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/speech_recognizer.h:24: class SpeechRecognizer :
public base::RefCountedThreadSafe<SpeechRecognizer> {
On 2014/11/05 00:54:15, Lei Zhang wrote:
> Does this still need to be RefCountedThreadSafe now that it lives on a single
> thread?

No it doesn't. Removed.

[Anand Mistry (off Chromium), 2014-11-05 02:59:48]

On 2014/11/05 01:02:37, Lei Zhang wrote:
> On 2014/11/03 06:51:56, Anand Mistry wrote:
> > I've tried to do this, and the lifecycle is more correct now. But it's kinda
> > complicated.
> 
> BTW, this is exactly why I'm giving this extra attention. I want to help
> simplify the design so it's easier to understand and maintain. I don't
> understand all the bits here, so I'm just concentrating on SpeechRecognizer
and
> trusting you/Matt to made sure the rest is ok.

And thanks for doing so. I've learned a lot about how threading in chrome works
and the primitives that exist in base/.

[Lei Zhang, 2014-11-05 03:44:38]

https://codereview.chromium.org/676593003/diff/200001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/speech_recognizer.cc (right):

https://codereview.chromium.org/676593003/diff/200001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/speech_recognizer.cc:76:
scoped_refptr<net::URLRequestContextGetter> url_request_context_getter_;
And everything here down is only accessed from the IO thread, right?

https://codereview.chromium.org/676593003/diff/200001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/speech_recognizer.cc:87:
base::WeakPtr<SpeechRecognizerDelegate> delegate,
can this be const ref?

https://codereview.chromium.org/676593003/diff/200001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/speech_recognizer.cc:94: session_(-1),
Maybe define a kInvalidSession constant?

https://codereview.chromium.org/676593003/diff/200001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/speech_recognizer.h (right):

https://codereview.chromium.org/676593003/diff/200001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/speech_recognizer.h:37: SpeechRecognizerDelegate*
delegate_;  // Not owned.
Can you just use save the content::WebContents* instead?

https://codereview.chromium.org/676593003/diff/200001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/speech_recognizer.h:41:
base::WeakPtrFactory<SpeechRecognizerDelegate> delegate_weak_factory_;
The delegate should have its own WeakPtrFactory() and hand out WeakPtrs. i.e.
SpeechRecognizer should take in const WeakPtr<SpeechRecognizerDelegate>&

[Anand Mistry (off Chromium), 2014-11-05 04:44:11]

https://codereview.chromium.org/676593003/diff/200001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/speech_recognizer.cc (right):

https://codereview.chromium.org/676593003/diff/200001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/speech_recognizer.cc:76:
scoped_refptr<net::URLRequestContextGetter> url_request_context_getter_;
On 2014/11/05 03:44:38, Lei Zhang wrote:
> And everything here down is only accessed from the IO thread, right?

Yes. Added comment.

https://codereview.chromium.org/676593003/diff/200001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/speech_recognizer.cc:87:
base::WeakPtr<SpeechRecognizerDelegate> delegate,
On 2014/11/05 03:44:37, Lei Zhang wrote:
> can this be const ref?

Done.

https://codereview.chromium.org/676593003/diff/200001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/speech_recognizer.cc:94: session_(-1),
On 2014/11/05 03:44:38, Lei Zhang wrote:
> Maybe define a kInvalidSession constant?

Done.

https://codereview.chromium.org/676593003/diff/200001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/speech_recognizer.h (right):

https://codereview.chromium.org/676593003/diff/200001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/speech_recognizer.h:37: SpeechRecognizerDelegate*
delegate_;  // Not owned.
On 2014/11/05 03:44:38, Lei Zhang wrote:
> Can you just use save the content::WebContents* instead?

No, because it might change. However, the current one will always be available
via SpeechRecognizerDelegate.

https://codereview.chromium.org/676593003/diff/200001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/speech_recognizer.h:41:
base::WeakPtrFactory<SpeechRecognizerDelegate> delegate_weak_factory_;
On 2014/11/05 03:44:38, Lei Zhang wrote:
> The delegate should have its own WeakPtrFactory() and hand out WeakPtrs. i.e.
> SpeechRecognizer should take in const WeakPtr<SpeechRecognizerDelegate>&

Done.

[Lei Zhang, 2014-11-05 05:58:39]

lgtm++

[Jun Mukai, 2014-11-05 06:00:14]

lgtm

[Anand Mistry (off Chromium), 2014-11-05 06:10:31]

The CQ bit was checked by amistry@chromium.org

[commit-bot: I haz the power, 2014-11-05 06:10:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/676593003/220001

[commit-bot: I haz the power, 2014-11-05 07:32:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-11-05 07:32:50]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Anand Mistry (off Chromium), 2014-11-05 07:38:32]

The CQ bit was checked by amistry@chromium.org

[commit-bot: I haz the power, 2014-11-05 07:39:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/676593003/220001

[commit-bot: I haz the power, 2014-11-05 08:37:42]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2014-11-05 08:38:21]

Patchset 12 (id:??) landed as
https://crrev.com/0c4422d9a0e89da3ca708f7bfdbab3603a4a7279
Cr-Commit-Position: refs/heads/master@{#302767}