Moved instantiation of SpeechRecognitionManager inside browser_main_loop, instead of Singleton.

Moved instantiation of SpeechRecognitionManager inside browser_main_loop, instead of Singleton. (Speech CL1.10)

Compared to the Singleton solution, this allows the SpeechRecognitionManager to instantiate and cleanup the delegate at the right time.
Also, cleaned-up speech_recognition_browsertest.cc so that the FakeSpeechRecognitionManager extends only the SRM interface, and not the Impl class.

BUG=116954
TEST=none


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=137424

[hans, 2012-05-15 15:32:31]

http://codereview.chromium.org/10399025/diff/3019/content/browser/speech/inpu...
File content/browser/speech/input_tag_speech_dispatcher_host.cc (right):

http://codereview.chromium.org/10399025/diff/3019/content/browser/speech/inpu...
content/browser/speech/input_tag_speech_dispatcher_host.cc:32:
content::SpeechRecognitionManager*
a "using content::SpeechRecognitionManager" at the top would be good, so we
don't have to put content:: everywhere

http://codereview.chromium.org/10399025/diff/3019/content/browser/speech/spee...
File content/browser/speech/speech_recognition_manager_impl.cc (right):

http://codereview.chromium.org/10399025/diff/3019/content/browser/speech/spee...
content/browser/speech/speech_recognition_manager_impl.cc:43: }  // namespace
no need for both anonymous namespace and static

http://codereview.chromium.org/10399025/diff/3019/content/browser/speech/spee...
content/browser/speech/speech_recognition_manager_impl.cc:48:
DCHECK(g_speech_recognition_manager_impl);
but the comment for GetInstance() says this may return NULL?

http://codereview.chromium.org/10399025/diff/3019/content/browser/speech/spee...
File content/browser/speech/speech_recognition_manager_impl.h (right):

http://codereview.chromium.org/10399025/diff/3019/content/browser/speech/spee...
content/browser/speech/speech_recognition_manager_impl.h:58:
SpeechRecognitionManagerImpl();
hmm, it feels weird having a public constructor when this is effectively going
to be a singleton?

[Primiano Tucci (use gerrit), 2012-05-15 16:04:03]

http://codereview.chromium.org/10399025/diff/3019/content/browser/speech/inpu...
File content/browser/speech/input_tag_speech_dispatcher_host.cc (right):

http://codereview.chromium.org/10399025/diff/3019/content/browser/speech/inpu...
content/browser/speech/input_tag_speech_dispatcher_host.cc:32:
content::SpeechRecognitionManager*
On 2012/05/15 15:32:31, hans wrote:
> a "using content::SpeechRecognitionManager" at the top would be good, so we
> don't have to put content:: everywhere

Done.

http://codereview.chromium.org/10399025/diff/3019/content/browser/speech/spee...
File content/browser/speech/speech_recognition_manager_impl.cc (right):

http://codereview.chromium.org/10399025/diff/3019/content/browser/speech/spee...
content/browser/speech/speech_recognition_manager_impl.cc:43: }  // namespace
On 2012/05/15 15:32:31, hans wrote:
> no need for both anonymous namespace and static

Done.

http://codereview.chromium.org/10399025/diff/3019/content/browser/speech/spee...
content/browser/speech/speech_recognition_manager_impl.cc:48:
DCHECK(g_speech_recognition_manager_impl);
On 2012/05/15 15:32:31, hans wrote:
> but the comment for GetInstance() says this may return NULL?
Ah, right. I changed the comment (I copied it from another similar code).

http://codereview.chromium.org/10399025/diff/3019/content/browser/speech/spee...
File content/browser/speech/speech_recognition_manager_impl.h (right):

http://codereview.chromium.org/10399025/diff/3019/content/browser/speech/spee...
content/browser/speech/speech_recognition_manager_impl.h:58:
SpeechRecognitionManagerImpl();
On 2012/05/15 15:32:31, hans wrote:
> hmm, it feels weird having a public constructor when this is effectively going
> to be a singleton?
Hmm, the singleton was removed because it had a too large lifetime. Now it
behaves effectively as a singleton, but it lifecycle is managed by
BrowserMainLoop.
Made it protected and friend of BrowserMainLoop, perhaps it might look better.

[jam, 2012-05-15 16:38:31]

http://codereview.chromium.org/10399025/diff/14001/content/browser/browser_ma...
File content/browser/browser_main_loop.cc (right):

http://codereview.chromium.org/10399025/diff/14001/content/browser/browser_ma...
content/browser/browser_main_loop.cc:484: // Cleanup the speech recognition
manager.
nit: this comment and the one below are just describing what the code does, so
no need to have them

http://codereview.chromium.org/10399025/diff/14001/content/browser/speech/inp...
File content/browser/speech/input_tag_speech_dispatcher_host.h (right):

http://codereview.chromium.org/10399025/diff/14001/content/browser/speech/inp...
content/browser/speech/input_tag_speech_dispatcher_host.h:58: static void
set_manager_for_tests(content::SpeechRecognitionManager* manager);
nit: per style guide this should be SetManagerForTests

http://codereview.chromium.org/10399025/diff/14001/content/browser/speech/spe...
File content/browser/speech/speech_recognition_manager_impl.h (right):

http://codereview.chromium.org/10399025/diff/14001/content/browser/speech/spe...
content/browser/speech/speech_recognition_manager_impl.h:15: #include
"base/memory/singleton.h"
nit: not needed anymore

[Primiano Tucci (use gerrit), 2012-05-15 17:17:12]

https://chromiumcodereview.appspot.com/10399025/diff/14001/content/browser/br...
File content/browser/browser_main_loop.cc (right):

https://chromiumcodereview.appspot.com/10399025/diff/14001/content/browser/br...
content/browser/browser_main_loop.cc:484: // Cleanup the speech recognition
manager.
On 2012/05/15 16:38:31, John Abd-El-Malek wrote:
> nit: this comment and the one below are just describing what the code does, so
> no need to have them

Done.

https://chromiumcodereview.appspot.com/10399025/diff/14001/content/browser/sp...
File content/browser/speech/input_tag_speech_dispatcher_host.h (right):

https://chromiumcodereview.appspot.com/10399025/diff/14001/content/browser/sp...
content/browser/speech/input_tag_speech_dispatcher_host.h:58: static void
set_manager_for_tests(content::SpeechRecognitionManager* manager);
On 2012/05/15 16:38:31, John Abd-El-Malek wrote:
> nit: per style guide this should be SetManagerForTests

Changed it. However, can I just ask where I can find that rule, in order to
avoid that nit in future?

I named it so according to:
- From Google C++ style guide: "...For example if a function were so cheap you
would not cache ... lowercase naming would 
be acceptable"
- From chromium style guide: "Google style strongly encourages (but does not
mandate) unix_hacker style on member functions that do so little work they're
effectively free, such as simple, non-virtual getters and setters. This is also
the team position."

[jam, 2012-05-15 17:20:47]

lgtm

https://chromiumcodereview.appspot.com/10399025/diff/14001/content/browser/sp...
File content/browser/speech/input_tag_speech_dispatcher_host.h (right):

https://chromiumcodereview.appspot.com/10399025/diff/14001/content/browser/sp...
content/browser/speech/input_tag_speech_dispatcher_host.h:58: static void
set_manager_for_tests(content::SpeechRecognitionManager* manager);
On 2012/05/15 17:17:12, Primiano Tucci wrote:
> On 2012/05/15 16:38:31, John Abd-El-Malek wrote:
> > nit: per style guide this should be SetManagerForTests
> 
> Changed it. However, can I just ask where I can find that rule, in order to
> avoid that nit in future?
> 
> I named it so according to:
> - From Google C++ style guide: "...For example if a function were so cheap you
> would not cache ... lowercase naming would 
> be acceptable"
> - From chromium style guide: "Google style strongly encourages (but does not
> mandate) unix_hacker style on member functions that do so little work they're
> effectively free, such as simple, non-virtual getters and setters. This is
also
> the team position."

we only allow these unix_hacker naming when the functions are inlined. if the
definition is in a separate file, we always use CamelCase. i would assume that
this is in a style guide somewhere, if not, that should be fixed. afaik this
isn't personal style but a rule that we follow.

[jam, 2012-05-15 17:26:56]

https://chromiumcodereview.appspot.com/10399025/diff/14001/content/browser/sp...
File content/browser/speech/input_tag_speech_dispatcher_host.h (right):

https://chromiumcodereview.appspot.com/10399025/diff/14001/content/browser/sp...
content/browser/speech/input_tag_speech_dispatcher_host.h:58: static void
set_manager_for_tests(content::SpeechRecognitionManager* manager);
On 2012/05/15 17:20:47, John Abd-El-Malek wrote:
> On 2012/05/15 17:17:12, Primiano Tucci wrote:
> > On 2012/05/15 16:38:31, John Abd-El-Malek wrote:
> > > nit: per style guide this should be SetManagerForTests
> > 
> > Changed it. However, can I just ask where I can find that rule, in order to
> > avoid that nit in future?
> > 
> > I named it so according to:
> > - From Google C++ style guide: "...For example if a function were so cheap
you
> > would not cache ... lowercase naming would 
> > be acceptable"
> > - From chromium style guide: "Google style strongly encourages (but does not
> > mandate) unix_hacker style on member functions that do so little work
they're
> > effectively free, such as simple, non-virtual getters and setters. This is
> also
> > the team position."
> 
> we only allow these unix_hacker naming when the functions are inlined. if the
> definition is in a separate file, we always use CamelCase. i would assume that
> this is in a style guide somewhere, if not, that should be fixed. afaik this
> isn't personal style but a rule that we follow.

btw looking at the google style guide:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Names

it says that unix_hacker is for inline functions and accessors/mutators. it also
says "The definitions of accessors are usually inlined in the header file."

[Primiano Tucci (use gerrit), 2012-05-15 18:26:25]

On 2012/05/15 17:26:56, John Abd-El-Malek wrote:
>
https://chromiumcodereview.appspot.com/10399025/diff/14001/content/browser/sp...
> File content/browser/speech/input_tag_speech_dispatcher_host.h (right):
> 
>
https://chromiumcodereview.appspot.com/10399025/diff/14001/content/browser/sp...
> content/browser/speech/input_tag_speech_dispatcher_host.h:58: static void
> set_manager_for_tests(content::SpeechRecognitionManager* manager);
> On 2012/05/15 17:20:47, John Abd-El-Malek wrote:
> > On 2012/05/15 17:17:12, Primiano Tucci wrote:
> > > On 2012/05/15 16:38:31, John Abd-El-Malek wrote:
> > > > nit: per style guide this should be SetManagerForTests
> > > 
> > > Changed it. However, can I just ask where I can find that rule, in order
to
> > > avoid that nit in future?
> > > 
> > > I named it so according to:
> > > - From Google C++ style guide: "...For example if a function were so cheap
> you
> > > would not cache ... lowercase naming would 
> > > be acceptable"
> > > - From chromium style guide: "Google style strongly encourages (but does
not
> > > mandate) unix_hacker style on member functions that do so little work
> they're
> > > effectively free, such as simple, non-virtual getters and setters. This is
> > also
> > > the team position."
> > 
> > we only allow these unix_hacker naming when the functions are inlined. if
the
> > definition is in a separate file, we always use CamelCase. i would assume
that
> > this is in a style guide somewhere, if not, that should be fixed. afaik this
> > isn't personal style but a rule that we follow.
> 
> btw looking at the google style guide:
> http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Names
> 
> it says that unix_hacker is for inline functions and accessors/mutators. it
also
> says "The definitions of accessors are usually inlined in the header file."

Ah thanks, missed that.
I guess http://www.chromium.org/developers/coding-style should  highlight the
inline constraint, IMHO it is currently a bit confusing.

[commit-bot: I haz the power, 2012-05-16 10:34:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/primiano@chromium.org/10399025/9006

[hans, 2012-05-16 11:14:39]

lgtm

[commit-bot: I haz the power, 2012-05-16 13:20:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/primiano@chromium.org/10399025/9008

[commit-bot: I haz the power, 2012-05-16 14:51:45]

Change committed as 137424