Allow webkitSpeechRecognition work from an app's background page.

Allow webkitSpeechRecognition work from an app's background page.

The manifest would already decide whether or not to allow the request (via
    audioCapture permission).
Note that I'm enabling it only for speech coming through JS API. Speech
coming through <input> element are still discarded since the showing speech
bubble is not trivial as is, especially from popups.

BUG=291140
Test=From a chrome v2 packaged app with audioCapture permission, speech
api (webkitSpeechRecognition) should work from the app's background page.
Added browser_test that makes sure onerror ("not-allowed") doesn't get
called as it used to.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=225866

[lazyboy, 2013-09-20 22:21:39]

+kalman for checking from apps/background_page point of view.

[not at google - send to devlin, 2013-09-20 22:36:46]

it seems ok but just those 2 little questions.

https://chromiumcodereview.appspot.com/24292004/diff/1/chrome/browser/speech/...
File chrome/browser/speech/chrome_speech_recognition_manager_delegate.cc
(right):

https://chromiumcodereview.appspot.com/24292004/diff/1/chrome/browser/speech/...
chrome/browser/speech/chrome_speech_recognition_manager_delegate.cc:425:
ask_permission = true;
do you know how this ask_permission thing is supposed to work? comments imply
otherwise.

https://chromiumcodereview.appspot.com/24292004/diff/1/chrome/test/data/exten...
File chrome/test/data/extensions/platform_apps/speech/background_page/test.js
(right):

https://chromiumcodereview.appspot.com/24292004/diff/1/chrome/test/data/exten...
chrome/test/data/extensions/platform_apps/speech/background_page/test.js:14: }
when does it get onerror?

[lazyboy, 2013-09-20 22:49:08]

https://chromiumcodereview.appspot.com/24292004/diff/1/chrome/browser/speech/...
File chrome/browser/speech/chrome_speech_recognition_manager_delegate.cc
(right):

https://chromiumcodereview.appspot.com/24292004/diff/1/chrome/browser/speech/...
chrome/browser/speech/chrome_speech_recognition_manager_delegate.cc:425:
ask_permission = true;
On 2013/09/20 22:36:46, kalman wrote:
> do you know how this ask_permission thing is supposed to work? comments imply
> otherwise.

The |callback| carries |ask_permission|, which eventually checks the manifest
for apps:

The callback here corresponds to
SpeechRecognitionManagerImpl::RecognitionAllowedCallback(), with
ask_permission/ask_user set to true, requests for MEDIA_DEVICE_AUDIO_CAPTURE:
for regular page, that would show infobar, and for the apps case, it would check
the manifest for "audioCapture".

The comment here about tab is quite stale, we need to fix that.

https://chromiumcodereview.appspot.com/24292004/diff/1/chrome/test/data/exten...
File chrome/test/data/extensions/platform_apps/speech/background_page/test.js
(right):

https://chromiumcodereview.appspot.com/24292004/diff/1/chrome/test/data/exten...
chrome/test/data/extensions/platform_apps/speech/background_page/test.js:14: }
On 2013/09/20 22:36:46, kalman wrote:
> when does it get onerror?

When "audioCapture" permission is missing from the app's manifest.json.

[not at google - send to devlin, 2013-09-20 23:03:41]

https://chromiumcodereview.appspot.com/24292004/diff/1/chrome/browser/speech/...
File chrome/browser/speech/chrome_speech_recognition_manager_delegate.cc
(right):

https://chromiumcodereview.appspot.com/24292004/diff/1/chrome/browser/speech/...
chrome/browser/speech/chrome_speech_recognition_manager_delegate.cc:425:
ask_permission = true;
On 2013/09/20 22:49:08, lazyboy wrote:
> On 2013/09/20 22:36:46, kalman wrote:
> > do you know how this ask_permission thing is supposed to work? comments
imply
> > otherwise.
> 
> The |callback| carries |ask_permission|, which eventually checks the manifest
> for apps:
> 
> The callback here corresponds to
> SpeechRecognitionManagerImpl::RecognitionAllowedCallback(), with
> ask_permission/ask_user set to true, requests for MEDIA_DEVICE_AUDIO_CAPTURE:
> for regular page, that would show infobar, and for the apps case, it would
check
> the manifest for "audioCapture".
> 
> The comment here about tab is quite stale, we need to fix that.

Yeah; "check_permission" would be better. "ask_permission" is confusing and as
you say the comment is out of date. Could you update it now?

https://chromiumcodereview.appspot.com/24292004/diff/1/chrome/test/data/exten...
File chrome/test/data/extensions/platform_apps/speech/background_page/test.js
(right):

https://chromiumcodereview.appspot.com/24292004/diff/1/chrome/test/data/exten...
chrome/test/data/extensions/platform_apps/speech/background_page/test.js:14: }
On 2013/09/20 22:49:08, lazyboy wrote:
> On 2013/09/20 22:36:46, kalman wrote:
> > when does it get onerror?
> 
> When "audioCapture" permission is missing from the app's manifest.json.

Ok. The manifest.json here has "audioCapture" though. Perhaps you should add two
separate tests here.

[lazyboy, 2013-09-21 00:48:04]

https://chromiumcodereview.appspot.com/24292004/diff/1/chrome/browser/speech/...
File chrome/browser/speech/chrome_speech_recognition_manager_delegate.cc
(right):

https://chromiumcodereview.appspot.com/24292004/diff/1/chrome/browser/speech/...
chrome/browser/speech/chrome_speech_recognition_manager_delegate.cc:425:
ask_permission = true;
On 2013/09/20 23:03:41, kalman wrote:
> On 2013/09/20 22:49:08, lazyboy wrote:
> > On 2013/09/20 22:36:46, kalman wrote:
> > > do you know how this ask_permission thing is supposed to work? comments
> imply
> > > otherwise.
> > 
> > The |callback| carries |ask_permission|, which eventually checks the
manifest
> > for apps:
> > 
> > The callback here corresponds to
> > SpeechRecognitionManagerImpl::RecognitionAllowedCallback(), with
> > ask_permission/ask_user set to true, requests for
MEDIA_DEVICE_AUDIO_CAPTURE:
> > for regular page, that would show infobar, and for the apps case, it would
> check
> > the manifest for "audioCapture".
> > 
> > The comment here about tab is quite stale, we need to fix that.
> 
> Yeah; "check_permission" would be better. "ask_permission" is confusing and as
> you say the comment is out of date. Could you update it now?

Done.

https://chromiumcodereview.appspot.com/24292004/diff/1/chrome/test/data/exten...
File chrome/test/data/extensions/platform_apps/speech/background_page/test.js
(right):

https://chromiumcodereview.appspot.com/24292004/diff/1/chrome/test/data/exten...
chrome/test/data/extensions/platform_apps/speech/background_page/test.js:14: }
On 2013/09/20 23:03:41, kalman wrote:
> On 2013/09/20 22:49:08, lazyboy wrote:
> > On 2013/09/20 22:36:46, kalman wrote:
> > > when does it get onerror?
> > 
> > When "audioCapture" permission is missing from the app's manifest.json.
> 
> Ok. The manifest.json here has "audioCapture" though. Perhaps you should add
two
> separate tests here.

Added separate test w/o permission.

[not at google - send to devlin, 2013-09-21 04:13:55]

lgtm

https://chromiumcodereview.appspot.com/24292004/diff/9001/chrome/test/data/ex...
File chrome/test/data/extensions/platform_apps/speech/background_page/test.js
(right):

https://chromiumcodereview.appspot.com/24292004/diff/9001/chrome/test/data/ex...
chrome/test/data/extensions/platform_apps/speech/background_page/test.js:13:
return;
so.. you shouldn't need this anymore right? if I understood correctly? since it
won't fail?

[lazyboy, 2013-09-21 06:07:19]

+xians for chrome/browser/speech/
+tapted for chrome/browser/apps/

https://chromiumcodereview.appspot.com/24292004/diff/9001/chrome/test/data/ex...
File chrome/test/data/extensions/platform_apps/speech/background_page/test.js
(right):

https://chromiumcodereview.appspot.com/24292004/diff/9001/chrome/test/data/ex...
chrome/test/data/extensions/platform_apps/speech/background_page/test.js:13:
return;
On 2013/09/21 04:13:55, kalman wrote:
> so.. you shouldn't need this anymore right? if I understood correctly? since
it
> won't fail?

This is just a safeguard. A request can start and then return onerror because
for tests there won't be any speech recognition servers. I tried to use
mock_speech, but that does too much mocking and bypasses the manifest check.
So for my test case, an onstart at the beginning is good enough to say the test
is exercising what I want.

[tapted, 2013-09-23 00:05:51]

c/b/apps lgtm

[no longer working on chromium, 2013-09-23 08:11:52]

On 2013/09/23 00:05:51, tapted wrote:
> c/b/apps lgtm


lgtm on speech c++ code.

[lazyboy, 2013-09-24 02:32:02]

FYI, I had to change the test to use FakeSpeechRecognitionManager to pass on
bots.
changes are in:
speech_recognition_browsertest.cc and
chrome/test/data/extensions/platform_apps/speech/background_page/test.js

[tapted, 2013-09-24 02:53:41]

slgtm with some minor nits

https://chromiumcodereview.appspot.com/24292004/diff/24001/chrome/browser/app...
File chrome/browser/apps/speech_recognition_browsertest.cc (right):

https://chromiumcodereview.appspot.com/24292004/diff/24001/chrome/browser/app...
chrome/browser/apps/speech_recognition_browsertest.cc:34: private:
nit: blank line before

https://chromiumcodereview.appspot.com/24292004/diff/24001/chrome/browser/app...
chrome/browser/apps/speech_recognition_browsertest.cc:37: };
nit: DISALLOW_COPY_AND_ASSIGN(..)

[lazyboy, 2013-09-24 07:03:53]

https://chromiumcodereview.appspot.com/24292004/diff/24001/chrome/browser/app...
File chrome/browser/apps/speech_recognition_browsertest.cc (right):

https://chromiumcodereview.appspot.com/24292004/diff/24001/chrome/browser/app...
chrome/browser/apps/speech_recognition_browsertest.cc:34: private:
On 2013/09/24 02:53:41, tapted wrote:
> nit: blank line before

Done.

https://chromiumcodereview.appspot.com/24292004/diff/24001/chrome/browser/app...
chrome/browser/apps/speech_recognition_browsertest.cc:37: };
On 2013/09/24 02:53:41, tapted wrote:
> nit: DISALLOW_COPY_AND_ASSIGN(..)

Done.

[commit-bot: I haz the power, 2013-09-24 07:05:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/lazyboy@chromium.org/24292004/38001

[commit-bot: I haz the power, 2013-09-24 07:49:27]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-09-27 21:47:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/lazyboy@chromium.org/24292004/66001

[commit-bot: I haz the power, 2013-09-28 01:21:41]

Retried try job too often on linux_aura for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[commit-bot: I haz the power, 2013-09-28 01:47:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/lazyboy@chromium.org/24292004/66001

[commit-bot: I haz the power, 2013-09-28 03:00:35]

Retried try job too often on linux_chromeos for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-09-28 03:07:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/lazyboy@chromium.org/24292004/66001

[commit-bot: I haz the power, 2013-09-28 04:20:38]

Retried try job too often on linux_chromeos for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-09-28 14:29:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/lazyboy@chromium.org/24292004/66001

[commit-bot: I haz the power, 2013-09-28 17:44:19]

Retried try job too often on linux_aura for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[commit-bot: I haz the power, 2013-09-28 19:01:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/lazyboy@chromium.org/24292004/66001

[commit-bot: I haz the power, 2013-09-29 00:08:32]

Change committed as 225866