[Mac]: Enable speech sub-menu under the edit menu and pipe it through to the renderer.

[Mac]: Enable speech sub-menu under the edit menu and refactor speech implementation.

MainMenu.xib changes: Add submenu "Speech" with items "Start Speaking" and
"Stop Speaking" to the Edit menu (with localized ids for names).

This is a followup to http://crrev.com/126602, which implemented speech
support in the context menu. This changes also updates the context menu
implementation to go through the new one in the render view.

Also fixes some warnings found by 'gcl lint'.

BUG=31107, 46153
TEST=Select some text on a web page and go to Edit -> Speech -> Start Speaking. The
selected text should be spoken. While some text is being spoken, go to Edit -> Speech
-> Stop Speaking. The speech should stop. When no text is being spoken, Edit -> Speech
-> Stop Speaking should be disabled. Try the same steps as above but with selected
text in the omnibox. It should also work.

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

[Alexei Svitkine (slow), 2012-07-27 22:35:32]

Another patch I wrote up on the plane this morning. This one's a bit bigger...

[Nico, 2012-07-27 22:44:26]

Looks fine. Are you going to change
chrome/browser/tab_contents/render_view_context_menu_mac.mm to send a "start
speaking" ipc instead too? (costs one more ipc hop roundtrip, but then there's
only one tts implementation)

http://codereview.chromium.org/10820062/diff/12005/chrome/app/generated_resou...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/10820062/diff/12005/chrome/app/generated_resou...
chrome/app/generated_resources.grd:12631: +        <message
name="IDS_EDIT_SPEECH_MAC" desc="The Mac menu item for the 'Speech' submenu in
the edit menu. To translate, launch /Applications/TextEdit.app in an
appropriately localized version of OS X, open the Edit menu and use the
translation from there.">
Making these distinct from IDS_CONTENT_CONTEXT_SPEECH_MENU et al seems overkill.

http://codereview.chromium.org/10820062/diff/12005/content/browser/renderer_h...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/10820062/diff/12005/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_mac.mm:757: [NSApp
speakString:base::SysUTF16ToNSString(text)];
"These are not documented, so use only after checking -respondsToSelector:."?

http://codereview.chromium.org/10820062/diff/12005/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_mac.mm:3099: [NSApp
stopSpeaking:sender];
ditto

[Alexei Svitkine (slow), 2012-07-27 22:49:47]

I'll have to think about making the context menu take the same path. I can see
the appeal.

http://codereview.chromium.org/10820062/diff/12005/chrome/app/generated_resou...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/10820062/diff/12005/chrome/app/generated_resou...
chrome/app/generated_resources.grd:12631: +        <message
name="IDS_EDIT_SPEECH_MAC" desc="The Mac menu item for the 'Speech' submenu in
the edit menu. To translate, launch /Applications/TextEdit.app in an
appropriately localized version of OS X, open the Edit menu and use the
translation from there.">
On 2012/07/27 22:44:26, Nico wrote:
> Making these distinct from IDS_CONTENT_CONTEXT_SPEECH_MENU et al seems
overkill.

Do you know if we have a naming convention for strings that are shared thus? I
agree it's tidier to keep a single set.

http://codereview.chromium.org/10820062/diff/12005/content/browser/renderer_h...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/10820062/diff/12005/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_mac.mm:757: [NSApp
speakString:base::SysUTF16ToNSString(text)];
On 2012/07/27 22:44:26, Nico wrote:
> "These are not documented, so use only after checking -respondsToSelector:."?

We wouldn't get here unless respondsToSelector: is true per
validateUserInterfaceItem:.

Do you want me to add the check anyway? Maybe a comment would be better, what do
you think.

http://codereview.chromium.org/10820062/diff/12005/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_mac.mm:3099: [NSApp
stopSpeaking:sender];
On 2012/07/27 22:44:26, Nico wrote:
> ditto

same reply

[Nico, 2012-07-27 22:57:56]

I don't know a naming convention for shared strings. Maybe just "IDS_SPEAK_MAC".
Not terribly important either way.

Let me know once you're done thinking about the code sharing :-)

http://codereview.chromium.org/10820062/diff/12005/content/browser/renderer_h...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/10820062/diff/12005/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_mac.mm:757: [NSApp
speakString:base::SysUTF16ToNSString(text)];
On 2012/07/27 22:49:48, Alexei Svitkine wrote:
> On 2012/07/27 22:44:26, Nico wrote:
> > "These are not documented, so use only after checking
-respondsToSelector:."?
> 
> We wouldn't get here unless respondsToSelector: is true per
> validateUserInterfaceItem:.
> 
> Do you want me to add the check anyway? Maybe a comment would be better, what
do
> you think.

Comment sounds good.

[Alexei Svitkine (slow), 2012-07-28 15:29:12]

I've updated the CL to use a common TTS implementation that lives on the Mac
render view. I've avoided introducing an extra IPC round trip for the context
menu case.

PTAL.

http://codereview.chromium.org/10820062/diff/12005/chrome/app/generated_resou...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/10820062/diff/12005/chrome/app/generated_resou...
chrome/app/generated_resources.grd:12631: +        <message
name="IDS_EDIT_SPEECH_MAC" desc="The Mac menu item for the 'Speech' submenu in
the edit menu. To translate, launch /Applications/TextEdit.app in an
appropriately localized version of OS X, open the Edit menu and use the
translation from there.">
On 2012/07/27 22:44:26, Nico wrote:
> Making these distinct from IDS_CONTENT_CONTEXT_SPEECH_MENU et al seems
overkill.

Done.

http://codereview.chromium.org/10820062/diff/12005/content/browser/renderer_h...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/10820062/diff/12005/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_mac.mm:757: [NSApp
speakString:base::SysUTF16ToNSString(text)];
On 2012/07/27 22:57:56, Nico wrote:
> On 2012/07/27 22:49:48, Alexei Svitkine wrote:
> > On 2012/07/27 22:44:26, Nico wrote:
> > > "These are not documented, so use only after checking
> -respondsToSelector:."?
> > 
> > We wouldn't get here unless respondsToSelector: is true per
> > validateUserInterfaceItem:.
> > 
> > Do you want me to add the check anyway? Maybe a comment would be better,
what
> do
> > you think.
> 
> Comment sounds good.

Done.

[Nico, 2012-07-28 16:13:14]

LGTM!

[commit-bot: I haz the power, 2012-07-28 16:14:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/asvitkine@chromium.org/10820062/1033

[commit-bot: I haz the power, 2012-07-28 16:14:23]

Presubmit check for 10820062-1033 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    content/common
    content/renderer
    content/public/browser
    content/port

Presubmit checks took 1.5s to calculate.

[Alexei Svitkine (slow), 2012-07-28 16:14:47]

+avi for content/ review

[Alexei Svitkine (slow), 2012-08-02 18:32:53]

ping

[Nico, 2012-08-02 18:33:25]

avi: your ping

[Alexei Svitkine (slow), 2012-08-04 02:15:37]

I now realise this can be done simpler without needing an IPC round-trip to the
renderer, since RWHVM already has a cached copy of the selected text.

I'll simplify it next week.

[Alexei Svitkine (slow), 2012-08-06 15:30:31]

sp

[Alexei Svitkine (slow), 2012-08-06 15:32:42]

Simplified the implementation to remove the IPCs to and back from the renderer.
PTAL.

[Nico, 2012-08-06 15:47:31]

lgtm

http://codereview.chromium.org/10820062/diff/17001/content/browser/renderer_h...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/10820062/diff/17001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_mac.mm:774: // validates
this before enabling the item.
With this now being a public method, you probably should just check instead of
having this comment.

[Alexei Svitkine (slow), 2012-08-06 16:30:41]

http://codereview.chromium.org/10820062/diff/17001/content/browser/renderer_h...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/10820062/diff/17001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_view_mac.mm:774: // validates
this before enabling the item.
On 2012/08/06 15:47:31, Nico wrote:
> With this now being a public method, you probably should just check instead of
> having this comment.

Done.

[Avi (use Gerrit), 2012-08-07 14:21:40]

lgtm

[commit-bot: I haz the power, 2012-08-07 14:24:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/asvitkine@chromium.org/10820062/2015

[commit-bot: I haz the power, 2012-08-07 17:20:52]

Change committed as 150364