Context menu for "Voice recognition options"

Context menu for "Voice recognition options"

Patch from Glen Shires <gshires@google.com>;

BUG=79991
Test=

[Satish, 2011-07-19 08:22:39]

Good work.

http://codereview.chromium.org/7086005/diff/4001/chrome/browser/tab_contents/...
File chrome/browser/tab_contents/render_view_context_menu.cc (right):

http://codereview.chromium.org/7086005/diff/4001/chrome/browser/tab_contents/...
chrome/browser/tab_contents/render_view_context_menu.cc:860: // Add 'Block
offensive words' item in the sub menu.
suggest removing this and the next comment as the code is quite clear on what it
does

http://codereview.chromium.org/7086005/diff/4001/chrome/browser/tab_contents/...
chrome/browser/tab_contents/render_view_context_menu.cc:1282: return
(profile_->GetPrefs()->GetBoolean(
remove brackets around the return value

http://codereview.chromium.org/7086005/diff/4001/chrome/browser/tab_contents/...
chrome/browser/tab_contents/render_view_context_menu.cc:1705: case
IDC_CONTENT_CONTEXT_SPEECH_INPUT_ABOUT: {
add a newline above?

http://codereview.chromium.org/7086005/diff/4001/content/browser/renderer_hos...
File content/browser/renderer_host/render_view_host.cc (right):

http://codereview.chromium.org/7086005/diff/4001/content/browser/renderer_hos...
content/browser/renderer_host/render_view_host.cc:70: #if defined(OS_CHROMEOS)
any reason why we need two different urls here? If there is a need to
differentiate could that not be done by the server based on the user agent?

http://codereview.chromium.org/7086005/diff/4001/content/browser/renderer_hos...
File content/browser/renderer_host/render_view_host.h (right):

http://codereview.chromium.org/7086005/diff/4001/content/browser/renderer_hos...
content/browser/renderer_host/render_view_host.h:265: void SpeechInputAbout();
add a comment?

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

http://codereview.chromium.org/7086005/diff/4001/content/browser/speech/speec...
content/browser/speech/speech_input_manager.h:68: void NotifyCensorResults(bool
censor_speech_results) {
suggest renaming to set_censor_results(), this goes with the following comments
to make this into a simple setter

http://codereview.chromium.org/7086005/diff/4001/content/browser/speech/speec...
content/browser/speech/speech_input_manager.h:69: profanity_filter_ =
censor_speech_results ? 2 : 0; }
indent to left by 2 spaces and move closing brace to the next line.

http://codereview.chromium.org/7086005/diff/4001/content/browser/speech/speec...
content/browser/speech/speech_input_manager.h:71: int ProfanityFilter() { return
profanity_filter_; }
getters have the same name as the member variable they return without the
trailing underscore, i.e. "profanity_filter()" or "censor_results()" per the
next suggestion

http://codereview.chromium.org/7086005/diff/4001/content/browser/speech/speec...
content/browser/speech/speech_input_manager.h:73: int profanity_filter_;
It may be preferable to have this as a boolean (e.g. 'censor_results_') as
opposed to a value which makes sense to the speech service (0 and 2). In
speech_recognition_request.cc where the request is sent over the wire you can
select the appropriate value based on the boolean.

[gshires, 2011-07-19 22:56:31]

Addressed all review comments.

Note that issue 79991 corresponds to these changes.
http://code.google.com/p/chromium/issues/detail?id=79991

http://codereview.chromium.org/7086005/diff/4001/chrome/browser/tab_contents/...
File chrome/browser/tab_contents/render_view_context_menu.cc (right):

http://codereview.chromium.org/7086005/diff/4001/chrome/browser/tab_contents/...
chrome/browser/tab_contents/render_view_context_menu.cc:860: // Add 'Block
offensive words' item in the sub menu.
On 2011/07/19 08:22:39, Satish wrote:
> suggest removing this and the next comment as the code is quite clear on what
it
> does

Done.

http://codereview.chromium.org/7086005/diff/4001/chrome/browser/tab_contents/...
chrome/browser/tab_contents/render_view_context_menu.cc:1282: return
(profile_->GetPrefs()->GetBoolean(
On 2011/07/19 08:22:39, Satish wrote:
> remove brackets around the return value

Done.

http://codereview.chromium.org/7086005/diff/4001/chrome/browser/tab_contents/...
chrome/browser/tab_contents/render_view_context_menu.cc:1705: case
IDC_CONTENT_CONTEXT_SPEECH_INPUT_ABOUT: {
On 2011/07/19 08:22:39, Satish wrote:
> add a newline above?

Done.

http://codereview.chromium.org/7086005/diff/4001/content/browser/renderer_hos...
File content/browser/renderer_host/render_view_host.cc (right):

http://codereview.chromium.org/7086005/diff/4001/content/browser/renderer_hos...
content/browser/renderer_host/render_view_host.cc:70: #if defined(OS_CHROMEOS)
On 2011/07/19 08:22:39, Satish wrote:
> any reason why we need two different urls here? If there is a need to
> differentiate could that not be done by the server based on the user agent?

No need to differentiate.
I made this consistent with other URLs, and moved to url_constants.cc

http://codereview.chromium.org/7086005/diff/4001/content/browser/renderer_hos...
File content/browser/renderer_host/render_view_host.h (right):

http://codereview.chromium.org/7086005/diff/4001/content/browser/renderer_hos...
content/browser/renderer_host/render_view_host.h:265: void SpeechInputAbout();
On 2011/07/19 08:22:39, Satish wrote:
> add a comment?

Done.

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

http://codereview.chromium.org/7086005/diff/4001/content/browser/speech/speec...
content/browser/speech/speech_input_manager.h:68: void NotifyCensorResults(bool
censor_speech_results) {
On 2011/07/19 08:22:39, Satish wrote:
> suggest renaming to set_censor_results(), this goes with the following
comments
> to make this into a simple setter

Done.

http://codereview.chromium.org/7086005/diff/4001/content/browser/speech/speec...
content/browser/speech/speech_input_manager.h:69: profanity_filter_ =
censor_speech_results ? 2 : 0; }
On 2011/07/19 08:22:39, Satish wrote:
> indent to left by 2 spaces and move closing brace to the next line.

Shortened to a single line

http://codereview.chromium.org/7086005/diff/4001/content/browser/speech/speec...
content/browser/speech/speech_input_manager.h:71: int ProfanityFilter() { return
profanity_filter_; }
On 2011/07/19 08:22:39, Satish wrote:
> getters have the same name as the member variable they return without the
> trailing underscore, i.e. "profanity_filter()" or "censor_results()" per the
> next suggestion 

Done.

http://codereview.chromium.org/7086005/diff/4001/content/browser/speech/speec...
content/browser/speech/speech_input_manager.h:73: int profanity_filter_;
On 2011/07/19 08:22:39, Satish wrote:
> It may be preferable to have this as a boolean (e.g. 'censor_results_') as
> opposed to a value which makes sense to the speech service (0 and 2). In
> speech_recognition_request.cc where the request is sent over the wire you can
> select the appropriate value based on the boolean.

Done.

[jam, 2011-07-19 23:06:53]

http://codereview.chromium.org/7086005/diff/16001/content/browser/renderer_ho...
File content/browser/renderer_host/render_view_host.cc (right):

http://codereview.chromium.org/7086005/diff/16001/content/browser/renderer_ho...
content/browser/renderer_host/render_view_host.cc:18: #include
"chrome/browser/google/google_util.h"
you're going to get a deps failure for including chrome paths from content (see
http://dev.chromium.org/developers/content-module).

it seems unnecessary to add a method to RVH that is called by chrome code which
then just calls out to chrome. so in the right click menu handler (in chrome),
make it do whatever you want directly

[gshires, 2011-07-20 13:48:38]

This change is dependent upon, and should not be submitted before, pending
WebKit change https://bugs.webkit.org/show_bug.cgi?id=64625

http://codereview.chromium.org/7086005/diff/16001/content/browser/renderer_ho...
File content/browser/renderer_host/render_view_host.cc (right):

http://codereview.chromium.org/7086005/diff/16001/content/browser/renderer_ho...
content/browser/renderer_host/render_view_host.cc:18: #include
"chrome/browser/google/google_util.h"
On 2011/07/19 23:06:54, John Abd-El-Malek wrote:
> you're going to get a deps failure for including chrome paths from content
(see
> http://dev.chromium.org/developers/content-module).
> 
> it seems unnecessary to add a method to RVH that is called by chrome code
which
> then just calls out to chrome. so in the right click menu handler (in chrome),
> make it do whatever you want directly

Done.

[Satish, 2011-07-20 15:23:13]

Thanks for the changes. A small list of comments below.

Can you also run this CL by the try servers? ("git try" from your command line).
For more info see http://www.chromium.org/developers/testing/try-server-usage.
That will make sure this CL doesn't break any existing speech related tests as
well.

http://codereview.chromium.org/7086005/diff/22001/chrome/browser/tab_contents...
File chrome/browser/tab_contents/render_view_context_menu.cc (right):

http://codereview.chromium.org/7086005/diff/22001/chrome/browser/tab_contents...
chrome/browser/tab_contents/render_view_context_menu.cc:1706: GURL
localized_about_url = google_util::AppendGoogleLocaleParam(about_url);
line too long?

http://codereview.chromium.org/7086005/diff/22001/chrome/browser/tab_contents...
chrome/browser/tab_contents/render_view_context_menu.cc:1709:
localized_about_url,
suggest moving the first parameter to the previous line and aligning the rest
with that, matching the rest of the code.

    OpenURL(localized_about_url, GURL(), NEW_FOREGROUND_TAB,
            PageTransition::LINK);

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

http://codereview.chromium.org/7086005/diff/22001/content/browser/speech/spee...
content/browser/speech/speech_input_manager.h:72: bool censor_results_;
Sorry I didn't notice this earlier. This class is used as an interface and
doesn't have an implementation by itself. SpeechInputManagerImpl implements it.
So this member variable ideally doesn't belong in here but in the implementation
class. It should also be initialised to an appropriate value (true?) in the
class's constructor.

However moving this variable to the Impl class means the 2 new methods would
have to be virtual, making it a bit ugly to have them as get_xxx and set_xxx. So
I think it is ok to leave this here, but just add a default constructor to this
class for initializing the variable.

[jam, 2011-07-20 15:49:27]

definitely run a try change for all changes before committing like Satish says
:)

http://codereview.chromium.org/7086005/diff/16001/content/browser/renderer_ho...
File content/browser/renderer_host/render_view_host.cc (right):

http://codereview.chromium.org/7086005/diff/16001/content/browser/renderer_ho...
content/browser/renderer_host/render_view_host.cc:18: #include
"chrome/browser/google/google_util.h"
On 2011/07/20 13:48:43, gshires wrote:
> On 2011/07/19 23:06:54, John Abd-El-Malek wrote:
> > you're going to get a deps failure for including chrome paths from content
> (see
> > http://dev.chromium.org/developers/content-module).
> > 
> > it seems unnecessary to add a method to RVH that is called by chrome code
> which
> > then just calls out to chrome. so in the right click menu handler (in
chrome),
> > make it do whatever you want directly
> 
> Done.

thanks!

[gshires, 2011-07-20 17:16:10]

Added the requested initializer and style changes

http://codereview.chromium.org/7086005/diff/22001/chrome/browser/tab_contents...
File chrome/browser/tab_contents/render_view_context_menu.cc (right):

http://codereview.chromium.org/7086005/diff/22001/chrome/browser/tab_contents...
chrome/browser/tab_contents/render_view_context_menu.cc:1706: GURL
localized_about_url = google_util::AppendGoogleLocaleParam(about_url);
On 2011/07/20 15:23:13, Satish wrote:
> line too long?

Done.

http://codereview.chromium.org/7086005/diff/22001/chrome/browser/tab_contents...
chrome/browser/tab_contents/render_view_context_menu.cc:1709:
localized_about_url,
On 2011/07/20 15:23:13, Satish wrote:
> suggest moving the first parameter to the previous line and aligning the rest
> with that, matching the rest of the code.
> 
>     OpenURL(localized_about_url, GURL(), NEW_FOREGROUND_TAB,
>             PageTransition::LINK);

Done.

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

http://codereview.chromium.org/7086005/diff/22001/content/browser/speech/spee...
content/browser/speech/speech_input_manager.h:72: bool censor_results_;
On 2011/07/20 15:23:13, Satish wrote:
> Sorry I didn't notice this earlier. This class is used as an interface and
> doesn't have an implementation by itself. SpeechInputManagerImpl implements
it.
> So this member variable ideally doesn't belong in here but in the
implementation
> class. It should also be initialised to an appropriate value (true?) in the
> class's constructor.
> 
> However moving this variable to the Impl class means the 2 new methods would
> have to be virtual, making it a bit ugly to have them as get_xxx and set_xxx.
So
> I think it is ok to leave this here, but just add a default constructor to
this
> class for initializing the variable.

Done.

[gshires, 2011-07-21 05:07:38]

WebKit change has landed: https://bugs.webkit.org/show_bug.cgi?id=64625
So this patch can now be committed when approved. 

I cannot run "git try" because I do not have commit access.

[Satish, 2011-07-21 08:15:03]

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

http://codereview.chromium.org/7086005/diff/24001/content/browser/speech/spee...
content/browser/speech/speech_recognizer.h:85: int profanity_filter,
missed changing this to 'bool censor_results'

http://codereview.chromium.org/7086005/diff/24001/content/browser/speech/spee...
content/browser/speech/speech_recognizer.h:135: int profanity_filter_;
ditto

[gshires, 2011-07-21 18:53:12]

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

http://codereview.chromium.org/7086005/diff/24001/content/browser/speech/spee...
content/browser/speech/speech_recognizer.h:85: int profanity_filter,
On 2011/07/21 08:15:03, Satish wrote:
> missed changing this to 'bool censor_results'

Done.

http://codereview.chromium.org/7086005/diff/24001/content/browser/speech/spee...
content/browser/speech/speech_recognizer.h:135: int profanity_filter_;
On 2011/07/21 08:15:03, Satish wrote:
> ditto

Done.

[Satish, 2011-07-25 11:33:33]

LGTM

The latest patch passes through try servers
http://build.chromium.org/p/tryserver.chromium/builders/linux/builds/39027
http://build.chromium.org/p/tryserver.chromium/builders/mac/builds/39368
http://build.chromium.org/p/tryserver.chromium/builders/win/builds/44561

[commit-bot: I haz the power, 2011-07-25 11:33:59]

Can't apply patch for file content/browser/speech/speech_recognizer.cc.
While running patch -p1 --forward --force;
patching file content/browser/speech/speech_recognizer.cc
Hunk #2 FAILED at 225.
1 out of 2 hunks FAILED -- saving rejects to file
content/browser/speech/speech_recognizer.cc.rej

[jam, 2011-07-25 16:30:22]

thanks for the change

[jam, 2011-07-25 16:45:40]

content common lgtm

[commit-bot: I haz the power, 2011-07-25 23:05:33]

Presubmit check for 7086005-27003 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
If this change requires manual test instructions to QA team, add
TEST=[instructions].

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

Presubmit checks took 1.7s to calculate.

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

[willchan no longer on Chromium, 2011-07-25 23:16:48]

stamping for profiles/: LGTM

[commit-bot: I haz the power, 2011-07-26 05:08:37]

Change committed as 94041