Remove use of default request context and fix use of speech censor flag

Remove use of default request context and fix use of speech censor flag

Now gets the request context and censor preference in BrowserRenderProcessHost and passes them through to the SpeechInputDispatcherHost. Each time a speech request is made, the SpeechInputDispatcherHost passes through these values to the SpeechInputManager. Also each time the censor preference changes, RenderViewContextMenu updates the current profile's SpeechInputPreferences.

BUG=92366
TEST=Speech recognition works.


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

[Satish, 2011-09-23 09:44:57]

http://codereview.chromium.org/7989001/diff/2020/chrome/browser/profiles/off_...
File chrome/browser/profiles/off_the_record_profile_impl.cc (right):

http://codereview.chromium.org/7989001/diff/2020/chrome/browser/profiles/off_...
chrome/browser/profiles/off_the_record_profile_impl.cc:429: return
profile_->GetPrefs()->GetBoolean(prefs::kSpeechInputCensorResults);
perhaps simpler to do profile_->GetSpeechCensorPref() ?

http://codereview.chromium.org/7989001/diff/2020/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile.h (right):

http://codereview.chromium.org/7989001/diff/2020/chrome/browser/profiles/prof...
chrome/browser/profiles/profile.h:207: virtual bool GetSpeechCensorPref() = 0;
perhaps this should return pointer to a SpeechInputPreferences object and that
object can hold the censor pref? That would allow us to add more speech related
preferences easily in future

http://codereview.chromium.org/7989001/diff/2020/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_impl.h (right):

http://codereview.chromium.org/7989001/diff/2020/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_impl.h:99: virtual bool GetSpeechCensorPref();
any reason not to tag these with OVERRIDE similar to
off_the_record_profile_impl.h ?

http://codereview.chromium.org/7989001/diff/2020/chrome/test/base/testing_pro...
File chrome/test/base/testing_profile.h (right):

http://codereview.chromium.org/7989001/diff/2020/chrome/test/base/testing_pro...
chrome/test/base/testing_profile.h:224: virtual bool GetSpeechCensorPref();
any reason not to tag these with OVERRIDE similar to
off_the_record_profile_impl.h ?

http://codereview.chromium.org/7989001/diff/2020/content/browser/browser_cont...
File content/browser/browser_context.h (right):

http://codereview.chromium.org/7989001/diff/2020/content/browser/browser_cont...
content/browser/browser_context.h:90: virtual bool GetSpeechCensorPref() = 0;
similar to comment in profile.h, could this return a SpeechInputPreferences
object?

http://codereview.chromium.org/7989001/diff/2020/content/browser/speech/speec...
File content/browser/speech/speech_input_dispatcher_host.h (right):

http://codereview.chromium.org/7989001/diff/2020/content/browser/speech/speec...
content/browser/speech/speech_input_dispatcher_host.h:25: explicit
SpeechInputDispatcherHost(
remove 'explicit' since you have more than 1 parameter now

[Avi (use Gerrit), 2011-09-23 13:16:05]

I can't comment on the speech code, as I'm not qualified, but it looks plausible
and you dealt with my big issue. lgtm from me and thanks!

http://codereview.chromium.org/7989001/diff/2020/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_impl.h (right):

http://codereview.chromium.org/7989001/diff/2020/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_impl.h:99: virtual bool GetSpeechCensorPref();
On 2011/09/23 09:44:57, Satish wrote:
> any reason not to tag these with OVERRIDE similar to
> off_the_record_profile_impl.h ?

While you're in here, yeah, that's a really good idea.

[allanwoj, 2011-09-23 14:12:44]

http://codereview.chromium.org/7989001/diff/2020/chrome/browser/profiles/off_...
File chrome/browser/profiles/off_the_record_profile_impl.cc (right):

http://codereview.chromium.org/7989001/diff/2020/chrome/browser/profiles/off_...
chrome/browser/profiles/off_the_record_profile_impl.cc:429: return
profile_->GetPrefs()->GetBoolean(prefs::kSpeechInputCensorResults);
On 2011/09/23 09:44:57, Satish wrote:
> perhaps simpler to do profile_->GetSpeechCensorPref() ?

Done.

http://codereview.chromium.org/7989001/diff/2020/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile.h (right):

http://codereview.chromium.org/7989001/diff/2020/chrome/browser/profiles/prof...
chrome/browser/profiles/profile.h:207: virtual bool GetSpeechCensorPref() = 0;
On 2011/09/23 09:44:57, Satish wrote:
> perhaps this should return pointer to a SpeechInputPreferences object and that
> object can hold the censor pref? That would allow us to add more speech
related
> preferences easily in future

Done.

http://codereview.chromium.org/7989001/diff/2020/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_impl.h (right):

http://codereview.chromium.org/7989001/diff/2020/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_impl.h:99: virtual bool GetSpeechCensorPref();
On 2011/09/23 13:16:06, Avi wrote:
> On 2011/09/23 09:44:57, Satish wrote:
> > any reason not to tag these with OVERRIDE similar to
> > off_the_record_profile_impl.h ?
> 
> While you're in here, yeah, that's a really good idea.

Done.

http://codereview.chromium.org/7989001/diff/2020/chrome/test/base/testing_pro...
File chrome/test/base/testing_profile.h (right):

http://codereview.chromium.org/7989001/diff/2020/chrome/test/base/testing_pro...
chrome/test/base/testing_profile.h:224: virtual bool GetSpeechCensorPref();
On 2011/09/23 09:44:57, Satish wrote:
> any reason not to tag these with OVERRIDE similar to
> off_the_record_profile_impl.h ?

Yup I don't see why not. I'll do it in another cl if that's ok.

http://codereview.chromium.org/7989001/diff/2020/content/browser/browser_cont...
File content/browser/browser_context.h (right):

http://codereview.chromium.org/7989001/diff/2020/content/browser/browser_cont...
content/browser/browser_context.h:90: virtual bool GetSpeechCensorPref() = 0;
On 2011/09/23 09:44:57, Satish wrote:
> similar to comment in profile.h, could this return a SpeechInputPreferences
> object?

Done.

http://codereview.chromium.org/7989001/diff/2020/content/browser/speech/speec...
File content/browser/speech/speech_input_dispatcher_host.h (right):

http://codereview.chromium.org/7989001/diff/2020/content/browser/speech/speec...
content/browser/speech/speech_input_dispatcher_host.h:25: explicit
SpeechInputDispatcherHost(
On 2011/09/23 09:44:57, Satish wrote:
> remove 'explicit' since you have more than 1 parameter now

Done.

[allanwoj, 2011-09-23 15:48:29]

Changes to the way SpeechInputPreferences is updated. RenderViewContextMenu
updates the current profile's SpeechInputPreferences when the censor results
flag is changed.

[Satish, 2011-09-23 16:31:54]

Thanks for the changes, looks better. More comments below..

http://codereview.chromium.org/7989001/diff/20001/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile.h (right):

http://codereview.chromium.org/7989001/diff/20001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile.h:207: virtual SpeechInputPreferences*
GetSpeechInputPreferences() = 0;
should SpeechInputPreferences be fwd declared above?

http://codereview.chromium.org/7989001/diff/20001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile.h:207: virtual SpeechInputPreferences*
GetSpeechInputPreferences() = 0;
Please also add a comment that the returned object is a ref-ptr and the caller
should take a reference to it if necessary

http://codereview.chromium.org/7989001/diff/20001/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/7989001/diff/20001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl.cc:1146: if
(!speech_input_preferences_.get())
suggest using braces for multi-line if blocks like this

http://codereview.chromium.org/7989001/diff/20001/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_impl.h (right):

http://codereview.chromium.org/7989001/diff/20001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl.h:236:
scoped_refptr<SpeechInputPreferences> speech_input_preferences_;
should we include speech_input_preferences.h in this file or fwd declare the
class?

http://codereview.chromium.org/7989001/diff/20001/content/browser/speech/spee...
File content/browser/speech/speech_input_preferences.cc (right):

http://codereview.chromium.org/7989001/diff/20001/content/browser/speech/spee...
content/browser/speech/speech_input_preferences.cc:11: :
censor_results_(censor_results){
if we pass a pref store here, there should be a private method which takes all
the values and sets them into the member vars in the IO thread

http://codereview.chromium.org/7989001/diff/20001/content/browser/speech/spee...
content/browser/speech/speech_input_preferences.cc:18: return censor_results_;
add a dcheck to verify that we are in the IO thread here

http://codereview.chromium.org/7989001/diff/20001/content/browser/speech/spee...
File content/browser/speech/speech_input_preferences.h (right):

http://codereview.chromium.org/7989001/diff/20001/content/browser/speech/spee...
content/browser/speech/speech_input_preferences.h:15:
SpeechInputPreferences(bool censor_results);
instead of passing the option values in the constructor, can we pass the Pref
store pointer? That will allow us to easily add new prefs later while not
changing this interface.. only the code in this object will change.

http://codereview.chromium.org/7989001/diff/20001/content/browser/speech/spee...
content/browser/speech/speech_input_preferences.h:18: bool censor_results()
const;
perhaps add a mention that all accessors can only be called from the IO thread

http://codereview.chromium.org/7989001/diff/20001/content/browser/speech/spee...
content/browser/speech/speech_input_preferences.h:24: bool censor_results_;
since this object is used across threads, we should either have a mutex to guard
the variables or access the variables only in one thread. I suggest the latter.
So add a comment here mentioning all vars should be accessed/modified only from
the IO thread.

http://codereview.chromium.org/7989001/diff/20001/content/browser/speech/spee...
File content/browser/speech/speech_recognizer.h (right):

http://codereview.chromium.org/7989001/diff/20001/content/browser/speech/spee...
content/browser/speech/speech_recognizer.h:93: SpeechInputPreferences*
speech_input_prefs,
the prefs object could remain with SpeechInputManager and this as well as
SpeechRecognitionRequest operate with the censor_results boolean as they were
before

[allanwoj, 2011-09-26 13:05:15]

http://codereview.chromium.org/7989001/diff/20001/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile.h (right):

http://codereview.chromium.org/7989001/diff/20001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile.h:207: virtual SpeechInputPreferences*
GetSpeechInputPreferences() = 0;
On 2011/09/23 16:31:54, Satish wrote:
> should SpeechInputPreferences be fwd declared above?

Done.

http://codereview.chromium.org/7989001/diff/20001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile.h:207: virtual SpeechInputPreferences*
GetSpeechInputPreferences() = 0;
On 2011/09/23 16:31:54, Satish wrote:
> Please also add a comment that the returned object is a ref-ptr and the caller
> should take a reference to it if necessary

Put comment in its super class in browser_context.h instead.

http://codereview.chromium.org/7989001/diff/20001/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/7989001/diff/20001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl.cc:1146: if
(!speech_input_preferences_.get())
On 2011/09/23 16:31:54, Satish wrote:
> suggest using braces for multi-line if blocks like this

Done.

http://codereview.chromium.org/7989001/diff/20001/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_impl.h (right):

http://codereview.chromium.org/7989001/diff/20001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl.h:236:
scoped_refptr<SpeechInputPreferences> speech_input_preferences_;
On 2011/09/23 16:31:54, Satish wrote:
> should we include speech_input_preferences.h in this file or fwd declare the
> class?

Done.

http://codereview.chromium.org/7989001/diff/20001/content/browser/speech/spee...
File content/browser/speech/speech_input_preferences.cc (right):

http://codereview.chromium.org/7989001/diff/20001/content/browser/speech/spee...
content/browser/speech/speech_input_preferences.cc:11: :
censor_results_(censor_results){
On 2011/09/23 16:31:54, Satish wrote:
> if we pass a pref store here, there should be a private method which takes all
> the values and sets them into the member vars in the IO thread

Uses initializer list, as discussed.

http://codereview.chromium.org/7989001/diff/20001/content/browser/speech/spee...
content/browser/speech/speech_input_preferences.cc:18: return censor_results_;
On 2011/09/23 16:31:54, Satish wrote:
> add a dcheck to verify that we are in the IO thread here

Done.

http://codereview.chromium.org/7989001/diff/20001/content/browser/speech/spee...
File content/browser/speech/speech_input_preferences.h (right):

http://codereview.chromium.org/7989001/diff/20001/content/browser/speech/spee...
content/browser/speech/speech_input_preferences.h:15:
SpeechInputPreferences(bool censor_results);
On 2011/09/23 16:31:54, Satish wrote:
> instead of passing the option values in the constructor, can we pass the Pref
> store pointer? That will allow us to easily add new prefs later while not
> changing this interface.. only the code in this object will change.

Made a chrome class to do this.

http://codereview.chromium.org/7989001/diff/20001/content/browser/speech/spee...
content/browser/speech/speech_input_preferences.h:18: bool censor_results()
const;
On 2011/09/23 16:31:54, Satish wrote:
> perhaps add a mention that all accessors can only be called from the IO thread

Done.

http://codereview.chromium.org/7989001/diff/20001/content/browser/speech/spee...
content/browser/speech/speech_input_preferences.h:24: bool censor_results_;
On 2011/09/23 16:31:54, Satish wrote:
> since this object is used across threads, we should either have a mutex to
guard
> the variables or access the variables only in one thread. I suggest the
latter.
> So add a comment here mentioning all vars should be accessed/modified only
from
> the IO thread.

Done.

http://codereview.chromium.org/7989001/diff/20001/content/browser/speech/spee...
File content/browser/speech/speech_recognizer.h (right):

http://codereview.chromium.org/7989001/diff/20001/content/browser/speech/spee...
content/browser/speech/speech_recognizer.h:93: SpeechInputPreferences*
speech_input_prefs,
On 2011/09/23 16:31:54, Satish wrote:
> the prefs object could remain with SpeechInputManager and this as well as
> SpeechRecognitionRequest operate with the censor_results boolean as they were
> before

Done.

[Satish, 2011-09-26 16:11:05]

LGTM

http://codereview.chromium.org/7989001/diff/22037/content/shell/shell_browser...
File content/shell/shell_browser_context.h (right):

http://codereview.chromium.org/7989001/diff/22037/content/shell/shell_browser...
content/shell/shell_browser_context.h:18: class SpeechInputPreferences;
this isn't required since you have content/browser/browser_context.h above

[allanwoj, 2011-09-26 16:16:02]

http://codereview.chromium.org/7989001/diff/22037/content/shell/shell_browser...
File content/shell/shell_browser_context.h (right):

http://codereview.chromium.org/7989001/diff/22037/content/shell/shell_browser...
content/shell/shell_browser_context.h:18: class SpeechInputPreferences;
On 2011/09/26 16:11:06, Satish wrote:
> this isn't required since you have content/browser/browser_context.h above

Done.

[commit-bot: I haz the power, 2011-09-27 08:45:31]

CQ is trying tha patch. Follow status at
https://chromium-status.appspot.com/cq/allanwoj@chromium.org/7989001/26063

[commit-bot: I haz the power, 2011-09-27 08:45:35]

Can't process patch for file chrome/chrome_browser.gypi.
File's status is None, patchset upload is incomplete.

[commit-bot: I haz the power, 2011-09-27 09:07:15]

CQ is trying tha patch. Follow status at
https://chromium-status.appspot.com/cq/allanwoj@chromium.org/7989001/30001

[commit-bot: I haz the power, 2011-09-27 09:07:27]

Presubmit check for 7989001-30001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
chrome/browser/profiles/profile.h,chrome/browser/profiles/profile_impl.cc,chrome/browser/profiles/profile_impl.h,chrome/browser/profiles/off_the_record_profile_impl.h,chrome/browser/profiles/off_the_record_profile_impl.cc

Presubmit checks took 3.3s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[allanwoj, 2011-09-27 09:15:04]

WillChan: Could you please take a look at this, I need someone from the profiles
OWNERS to lgtm before I can commit this. Thanks.

[willchan no longer on Chromium, 2011-09-27 18:29:20]

Sending to Elliot's way since it's more Profile/ProfileImpl related. He may ask
you to use ProfileKeyedService. I defer.

[Elliot Glaysher, 2011-09-27 20:10:31]

On 2011/09/27 18:29:20, willchan wrote:
> Sending to Elliot's way since it's more Profile/ProfileImpl related. He may
ask
> you to use ProfileKeyedService. I defer.

And I do.

Please don't add new things on Profile; we're trying to drive the number of
things it owns to zero. Please see
http://groups.google.com/a/chromium.org/group/chromium-dev/browse_thread/thre...
for instructions.

[allanwoj, 2011-09-28 11:35:22]

On 2011/09/27 20:10:31, Elliot Glaysher wrote:
> On 2011/09/27 18:29:20, willchan wrote:
> > Sending to Elliot's way since it's more Profile/ProfileImpl related. He may
> ask
> > you to use ProfileKeyedService. I defer.
> 
> And I do.
> 
> Please don't add new things on Profile; we're trying to drive the number of
> things it owns to zero. Please see
>
http://groups.google.com/a/chromium.org/group/chromium-dev/browse_thread/thre...
> for instructions.

Elliot: I started work on removing GetSpeechInputPreferences when I stumbled on
an issue. BrowserRenderProcessHost currently depends on a BrowserContext to get
stuff from chrome/. If we don't have a GetSpeechInputPreferences method in
Profile (which implements the BrowserContext) how can we obtain them in
BrowserRenderProcessHost to pass through to the dispatcher host? This problem
also applies to all other objects that need to pass from chrome/ to content/ if
we want to remove the Get* methods.

[Elliot Glaysher, 2011-09-28 17:37:16]

On 2011/09/28 11:35:22, allanwoj wrote:
> Elliot: I started work on removing GetSpeechInputPreferences when I stumbled
on
> an issue. BrowserRenderProcessHost currently depends on a BrowserContext to
get
> stuff from chrome/. If we don't have a GetSpeechInputPreferences method in
> Profile (which implements the BrowserContext) how can we obtain them in
> BrowserRenderProcessHost to pass through to the dispatcher host? This problem
> also applies to all other objects that need to pass from chrome/ to content/
if
> we want to remove the Get* methods.

I see. I'm still not happy about that, but it's defensible if you're adding and
overriding a BrowserContext method.

Since you're basically rewriting the profile_impl.h file with your commit
anyway, could you please rewrite it to match profile.h's order and separation
into BrowserContext implementations and Profile implementations? Like this:

  // content::BrowserContext implementation:
  virtual FilePath GetPath() OVERRIDE;
  /// ...the rest...

  // Profile implementation:
  virtual std::string GetProfileName() OVERRIDE;
  /// ...the rest...

I made a snap judgement based on your BrowserContext::Get* method being in a big
block of Profile::Get* methods. My bad, I admit. Could you please separate them
per above?

[Elliot Glaysher, 2011-09-28 20:19:01]

On 2011/09/28 17:37:16, Elliot Glaysher wrote:
> On 2011/09/28 11:35:22, allanwoj wrote:
> > Elliot: I started work on removing GetSpeechInputPreferences when I stumbled
> on
> > an issue. BrowserRenderProcessHost currently depends on a BrowserContext to
> get
> > stuff from chrome/. If we don't have a GetSpeechInputPreferences method in
> > Profile (which implements the BrowserContext) how can we obtain them in
> > BrowserRenderProcessHost to pass through to the dispatcher host? This
problem
> > also applies to all other objects that need to pass from chrome/ to content/
> if
> > we want to remove the Get* methods.
> 
> I see. I'm still not happy about that, but it's defensible if you're adding
and
> overriding a BrowserContext method.
> 
> Since you're basically rewriting the profile_impl.h file with your commit
> anyway, could you please rewrite it to match profile.h's order and separation
> into BrowserContext implementations and Profile implementations? Like this:
> 
>   // content::BrowserContext implementation:
>   virtual FilePath GetPath() OVERRIDE;
>   /// ...the rest...
> 
>   // Profile implementation:
>   virtual std::string GetProfileName() OVERRIDE;
>   /// ...the rest...
> 
> I made a snap judgement based on your BrowserContext::Get* method being in a
big
> block of Profile::Get* methods. My bad, I admit. Could you please separate
them
> per above?

lgtm conditional on the above

[allanwoj, 2011-09-29 11:29:35]

Cheers Elliot, I have separated the overriden methods in profile_impl.h. I also
changed the NewRunnableMethod call in ChromeSpeechInputPreferences to use the
new base::Bind way of doing it. Will commit following try results.

[commit-bot: I haz the power, 2011-09-29 13:20:55]

CQ is trying tha patch. Follow status at
https://chromium-status.appspot.com/cq/allanwoj@chromium.org/7989001/40001

[commit-bot: I haz the power, 2011-09-29 15:13:04]

Change committed as 103278