Refactor SpeechInputManager between chrome/ and content/.

Refactor SpeechInputManager between chrome/ and content/.

Moved requests_ (and related code) to SpeechInputManager. Added some pure virtual methods in SpeechInputManager that are implemented by ChromeSpeechInputManager.

BUG=95792
TEST=None


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

[Satish, 2011-09-12 12:22:43]

Thanks for doing this. Some comments below.

Can you also run this by try servers? There may be changes required in the test
files as well to match this.

[Satish, 2011-09-12 12:23:05]

Thanks for doing this. Some comments below.

Can you also run this by try servers? There may be changes required in the test
files as well to match this.

http://codereview.chromium.org/7838028/diff/5001/chrome/browser/speech/chrome...
File chrome/browser/speech/chrome_speech_input_manager.cc (right):

http://codereview.chromium.org/7838028/diff/5001/chrome/browser/speech/chrome...
chrome/browser/speech/chrome_speech_input_manager.cc:129: request_info_ =
optional_request_info_->value();
instead of directly setting the member vars of the base class, can these be
returned to the caller so those variables can be made private?

http://codereview.chromium.org/7838028/diff/5001/content/browser/speech/speec...
File content/browser/speech/speech_input_manager.cc (right):

http://codereview.chromium.org/7838028/diff/5001/content/browser/speech/speec...
content/browser/speech/speech_input_manager.cc:15: censor_results_(true) {
in http://crbug.com/92366 it appears we should be making this a per-profile flag
instead of a single flag for all requests. Can you remove this member variable
and read it from the profile on every use? (not sure if profile is accessible
from content/)

http://codereview.chromium.org/7838028/diff/5001/content/browser/speech/speec...
File content/browser/speech/speech_input_manager.h (right):

http://codereview.chromium.org/7838028/diff/5001/content/browser/speech/speec...
content/browser/speech/speech_input_manager.h:82: virtual void CreateBubble(int
caller_id,
add comments for each of these explaining what they should do?

http://codereview.chromium.org/7838028/diff/5001/content/browser/speech/speec...
content/browser/speech/speech_input_manager.h:87: virtual void
NoMicBubbleMessage(int caller_id) = 0;
perhaps rename this to HandleNoMicError() similar to HandleRecognizerError()
below?

http://codereview.chromium.org/7838028/diff/5001/content/browser/speech/speec...
content/browser/speech/speech_input_manager.h:88: virtual void CloseBubble(int
caller_id) = 0;
Since we are separating chrome/ from content/, we should probably change mention
of 'bubble' to something else in these virtuals.

http://codereview.chromium.org/7838028/diff/5001/content/browser/speech/speec...
content/browser/speech/speech_input_manager.h:98: void OnFocusChanged(int
caller_id);
add explanatory comment since this is used by the derived class as well.

Should the name change to OnBubbleFocusChanged ?

[allanwoj, 2011-09-19 18:30:38]

http://codereview.chromium.org/7838028/diff/5001/chrome/browser/speech/chrome...
File chrome/browser/speech/chrome_speech_input_manager.cc (right):

http://codereview.chromium.org/7838028/diff/5001/chrome/browser/speech/chrome...
chrome/browser/speech/chrome_speech_input_manager.cc:129: request_info_ =
optional_request_info_->value();
On 2011/09/12 12:23:05, Satish wrote:
> instead of directly setting the member vars of the base class, can these be
> returned to the caller so those variables can be made private?

Done.

http://codereview.chromium.org/7838028/diff/5001/content/browser/speech/speec...
File content/browser/speech/speech_input_manager.cc (right):

http://codereview.chromium.org/7838028/diff/5001/content/browser/speech/speec...
content/browser/speech/speech_input_manager.cc:15: censor_results_(true) {
On 2011/09/12 12:23:05, Satish wrote:
> in http://crbug.com/92366 it appears we should be making this a per-profile
flag
> instead of a single flag for all requests. Can you remove this member variable
> and read it from the profile on every use? (not sure if profile is accessible
> from content/)

I'll do this in a separate cl.

http://codereview.chromium.org/7838028/diff/5001/content/browser/speech/speec...
File content/browser/speech/speech_input_manager.h (right):

http://codereview.chromium.org/7838028/diff/5001/content/browser/speech/speec...
content/browser/speech/speech_input_manager.h:82: virtual void CreateBubble(int
caller_id,
On 2011/09/12 12:23:05, Satish wrote:
> add comments for each of these explaining what they should do?

Done.

http://codereview.chromium.org/7838028/diff/5001/content/browser/speech/speec...
content/browser/speech/speech_input_manager.h:87: virtual void
NoMicBubbleMessage(int caller_id) = 0;
On 2011/09/12 12:23:05, Satish wrote:
> perhaps rename this to HandleNoMicError() similar to HandleRecognizerError()
> below?

Done.

http://codereview.chromium.org/7838028/diff/5001/content/browser/speech/speec...
content/browser/speech/speech_input_manager.h:88: virtual void CloseBubble(int
caller_id) = 0;
On 2011/09/12 12:23:05, Satish wrote:
> Since we are separating chrome/ from content/, we should probably change
mention
> of 'bubble' to something else in these virtuals. 

Done.

http://codereview.chromium.org/7838028/diff/5001/content/browser/speech/speec...
content/browser/speech/speech_input_manager.h:98: void OnFocusChanged(int
caller_id);
On 2011/09/12 12:23:05, Satish wrote:
> add explanatory comment since this is used by the derived class as well.
> 
> Should the name change to OnBubbleFocusChanged ?

Added comment but left the name as it is, as I have removed all mention of
bubble.

[Satish, 2011-09-20 20:09:55]

LGTM

minor style comments below.

http://codereview.chromium.org/7838028/diff/21001/content/browser/speech/spee...
File content/browser/speech/speech_input_manager.h (right):

http://codereview.chromium.org/7838028/diff/21001/content/browser/speech/spee...
content/browser/speech/speech_input_manager.h:87: virtual void
FetchRequestInfo(bool* can_report_metrics,
Fetch -> Get as it is more commonly used?

http://codereview.chromium.org/7838028/diff/21001/content/browser/speech/spee...
content/browser/speech/speech_input_manager.h:112: virtual void
HandleNoMicError(int caller_id) = 0;
In context of the above methods, ShowNoMicError seems more appropriate.

http://codereview.chromium.org/7838028/diff/21001/content/browser/speech/spee...
content/browser/speech/speech_input_manager.h:115: virtual void
HandleRecognizerError(int caller_id,
similar to above, how about ShowRecognizerError ?

[allanwoj, 2011-09-21 11:17:54]

Done changes, will wait for trybots before submitting. Also some comments on the
censor results profile issue. It seems that whenever a profile is loaded
|censor_results_| is set
http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/profil...
so we already have per profile adjustments.

http://codereview.chromium.org/7838028/diff/21001/content/browser/speech/spee...
File content/browser/speech/speech_input_manager.h (right):

http://codereview.chromium.org/7838028/diff/21001/content/browser/speech/spee...
content/browser/speech/speech_input_manager.h:87: virtual void
FetchRequestInfo(bool* can_report_metrics,
On 2011/09/20 20:09:55, Satish wrote:
> Fetch -> Get as it is more commonly used?

Done.

http://codereview.chromium.org/7838028/diff/21001/content/browser/speech/spee...
content/browser/speech/speech_input_manager.h:112: virtual void
HandleNoMicError(int caller_id) = 0;
On 2011/09/20 20:09:55, Satish wrote:
> In context of the above methods, ShowNoMicError seems more appropriate.

Done.

http://codereview.chromium.org/7838028/diff/21001/content/browser/speech/spee...
content/browser/speech/speech_input_manager.h:115: virtual void
HandleRecognizerError(int caller_id,
On 2011/09/20 20:09:55, Satish wrote:
> similar to above, how about ShowRecognizerError ?

Done.

[Satish, 2011-09-21 12:29:25]

On 2011/09/21 11:17:54, allanwoj wrote:
> Done changes, will wait for trybots before submitting. Also some comments on
the
> censor results profile issue. It seems that whenever a profile is loaded
> |censor_results_| is set
>
http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/profil...
> so we already have per profile adjustments.

As we discussed offline, seems like we should still take care of this because
the way it is now each profile loaded will overwrite the global flag in the
singleton so effectively sharing of preferences across profiles. It may be an
issue with incognito vs. normal usage as well. Ok to do it in a future CL.

[commit-bot: I haz the power, 2011-09-21 16:45:08]

Change committed as 102117