Introduce "disable-speech-api" content flag and add it to WebView.

Introduce "disable-speech-api" content flag and add it to WebView.

We don't want to expose Web Speech API in WebView,
because it is not supported there.

NOTE:
This patch should land after crrev.com/1129213002 (blink-side)

BUG=483889
Committed: https://crrev.com/7afeb6d2e431dbcda688226a434131a33b7ce71f
Cr-Commit-Position: refs/heads/master@{#329645}

[timvolodine, 2015-05-07 16:35:36]

timvolodine@chromium.org changed reviewers:
+ peter@chromium.org, torne@chromium.org

[timvolodine, 2015-05-07 16:35:36]

peter@: for content
torne@: for webview

[Peter Beverloo, 2015-05-07 16:45:24]

lgtm, but with a question.

https://codereview.chromium.org/1127363004/diff/1/android_webview/lib/main/aw...
File android_webview/lib/main/aw_main_delegate.cc (right):

https://codereview.chromium.org/1127363004/diff/1/android_webview/lib/main/aw...
android_webview/lib/main/aw_main_delegate.cc:93: // WebView does not currently
support Web Speech API (crbug.com/483889)
It'd be more useful to point to the bug which is about enabling Web Speech for
WebView.

https://codereview.chromium.org/1127363004/diff/1/content/public/common/conte...
File content/public/common/content_switches.cc (right):

https://codereview.chromium.org/1127363004/diff/1/content/public/common/conte...
content/public/common/content_switches.cc:242: // Disables the Web Speech API.
The Web Speech API defines both speech recognition and speech synthesis:
  https://dvcs.w3.org/hg/speech-api/raw-file/tip/speechapi.html

Your patches only disable speech recognition, which the command line flag name
accurately indicates, however, this comment does not.

What about "Disables the speech recognition part of the Web Speech API."?

On a tangent, does WebView expose window.speechSynthesis? Does that work? If it
doesn't, you may want to change your patches to encapsulate both speech
recognition and synthesis.

https://codereview.chromium.org/1127363004/diff/1/content/public/common/conte...
content/public/common/content_switches.cc:243: const char
kDisableSpeechRecognition[]       = "disable-speech-recognition";
micro nit: one less space in front of the =

[timvolodine, 2015-05-07 18:45:14]

thanks for the comments Peter!

https://codereview.chromium.org/1127363004/diff/1/android_webview/lib/main/aw...
File android_webview/lib/main/aw_main_delegate.cc (right):

https://codereview.chromium.org/1127363004/diff/1/android_webview/lib/main/aw...
android_webview/lib/main/aw_main_delegate.cc:93: // WebView does not currently
support Web Speech API (crbug.com/483889)
On 2015/05/07 16:45:24, Peter Beverloo wrote:
> It'd be more useful to point to the bug which is about enabling Web Speech for
> WebView.

do you mean "disabling"? is there such a bug or should it be created?

https://codereview.chromium.org/1127363004/diff/1/content/public/common/conte...
File content/public/common/content_switches.cc (right):

https://codereview.chromium.org/1127363004/diff/1/content/public/common/conte...
content/public/common/content_switches.cc:242: // Disables the Web Speech API.
On 2015/05/07 16:45:24, Peter Beverloo wrote:
> The Web Speech API defines both speech recognition and speech synthesis:
>   https://dvcs.w3.org/hg/speech-api/raw-file/tip/speechapi.html
> 
> Your patches only disable speech recognition, which the command line flag name
> accurately indicates, however, this comment does not.
> 
> What about "Disables the speech recognition part of the Web Speech API."?
> 
> On a tangent, does WebView expose window.speechSynthesis? Does that work? If
it
> doesn't, you may want to change your patches to encapsulate both speech
> recognition and synthesis.

right, I've tested the speechSynthesis on webview and it doesn't appear to work
there (synthesizer is null so "speak" for example is never called), so I've
amended the blink patch to also disable synthesis for webview (behind the same
runtime blink switch).

So with this change the comment remains valid, and I've also renamed the flag to
"disable-speech".

https://codereview.chromium.org/1127363004/diff/1/content/public/common/conte...
content/public/common/content_switches.cc:243: const char
kDisableSpeechRecognition[]       = "disable-speech-recognition";
On 2015/05/07 16:45:24, Peter Beverloo wrote:
> micro nit: one less space in front of the =

Done.

[Peter Beverloo, 2015-05-08 11:25:00]

Thanks for the update!

https://codereview.chromium.org/1127363004/diff/1/android_webview/lib/main/aw...
File android_webview/lib/main/aw_main_delegate.cc (right):

https://codereview.chromium.org/1127363004/diff/1/android_webview/lib/main/aw...
android_webview/lib/main/aw_main_delegate.cc:93: // WebView does not currently
support Web Speech API (crbug.com/483889)
On 2015/05/07 18:45:14, timvolodine wrote:
> On 2015/05/07 16:45:24, Peter Beverloo wrote:
> > It'd be more useful to point to the bug which is about enabling Web Speech
for
> > WebView.
> 
> do you mean "disabling"? is there such a bug or should it be created?

No - if you look at other linked bugs, e.g. https://crbug.com/434712, they are
about adding support for the given feature to WebView.

I'd prefer if the bug in this comment could link to a bug that's about adding
support for the Web Speech API to WebView. If that bug doesn't exist yet, it
should be filed.

That way, people who stumble across this code and would like to know when it's
OK to remove the flag have a single place to go. The current bug would not be
updated when WebView starts supporting the speech APIs.

https://codereview.chromium.org/1127363004/diff/20001/content/public/common/c...
File content/public/common/content_switches.cc (right):

https://codereview.chromium.org/1127363004/diff/20001/content/public/common/c...
content/public/common/content_switches.cc:243: const char kDisableSpeech[]      
          = "disable-speech";
Sorry to bikeshed over this, but what about "disable-speech-api"?

When reading this flag, one would suspect that all speech related functionality
in the browser would stop. However, Chrome supports various other kinds of
speech input - the on-screen keyboard, the omnibox, and so on.

Naming it "disable-speech-api" makes it clear that it's about an API. :-)

[Torne, 2015-05-08 16:07:33]

rubberstamp LGTM for android_webview

[timvolodine, 2015-05-12 17:09:20]

also added a new bug since I couldn't find any existing bug relating web speech
api to webview.

https://codereview.chromium.org/1127363004/diff/20001/content/public/common/c...
File content/public/common/content_switches.cc (right):

https://codereview.chromium.org/1127363004/diff/20001/content/public/common/c...
content/public/common/content_switches.cc:243: const char kDisableSpeech[]      
          = "disable-speech";
On 2015/05/08 11:25:00, Peter Beverloo wrote:
> Sorry to bikeshed over this, but what about "disable-speech-api"?
> 
> When reading this flag, one would suspect that all speech related
functionality
> in the browser would stop. However, Chrome supports various other kinds of
> speech input - the on-screen keyboard, the omnibox, and so on.
> 
> Naming it "disable-speech-api" makes it clear that it's about an API. :-)

agree, done ;)

[timvolodine, 2015-05-12 17:09:57]

timvolodine@chromium.org changed reviewers:
+ avi@chromium.org

[timvolodine, 2015-05-12 17:09:57]

+avi: for content/

[timvolodine, 2015-05-13 11:25:48]

timvolodine@chromium.org changed reviewers:
+ jochen@chromium.org

[timvolodine, 2015-05-13 11:25:49]

+jochen@: for content/

[Avi (use Gerrit), 2015-05-13 15:22:04]

On 2015/05/13 11:25:49, timvolodine wrote:
> +jochen@: for content/

:(

OK, but I hope there's a plan to drop this flag again.

LGTM

[timvolodine, 2015-05-13 16:01:43]

The CQ bit was checked by timvolodine@chromium.org

[timvolodine, 2015-05-13 16:01:43]

The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org,
torne@chromium.org
Link to the patchset: https://codereview.chromium.org/1127363004/#ps40001
(title: "rename flag, add crbug")

[commit-bot: I haz the power, 2015-05-13 16:03:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1127363004/40001

[commit-bot: I haz the power, 2015-05-13 16:08:17]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-05-13 16:09:33]

Patchset 3 (id:??) landed as
https://crrev.com/7afeb6d2e431dbcda688226a434131a33b7ce71f
Cr-Commit-Position: refs/heads/master@{#329645}