Add a tray notification UI for speech input recording in the extension API.

Add a tray notification UI for speech input recording in the extension API.


BUG=97388
TEST=existing apitests + status icon unittests

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

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

[Leandro Graciá Gil, 2011-11-03 11:51:07]

Aaron, could you please take a look to the chrome/browser/extension changes?
I'll need an owner.

Satish, could you take a general look to the patch?

All new icons have been approved by UI and PNG-crunched.

[Aaron Boodman, 2011-11-04 06:01:15]

Can you please move this API out of chrome/browser/extensions? See
crbug.com/101244 for more information and examples.

[Leandro Graciá Gil, 2011-11-04 17:24:25]

On 2011/11/04 06:01:15, Aaron Boodman wrote:
> Can you please move this API out of chrome/browser/extensions? See
> crbug.com/101244 for more information and examples.

Added to the patch.

[Aaron Boodman, 2011-11-06 22:55:38]

http://codereview.chromium.org/8386074/diff/9002/chrome/browser/speech/extens...
File chrome/browser/speech/extension_speech_input_api.h (right):

http://codereview.chromium.org/8386074/diff/9002/chrome/browser/speech/extens...
chrome/browser/speech/extension_speech_input_api.h:5: #ifndef
CHROME_BROWSER_SPEECH_EXTENSION_SPEECH_INPUT_API_H_
To be consistent with the previous changes on the bug, these files should be
called speech_input_extension_api.*. Sorry :-/.

http://codereview.chromium.org/8386074/diff/9002/chrome/browser/speech/extens...
File chrome/browser/speech/extension_speech_input_api_constants.h (right):

http://codereview.chromium.org/8386074/diff/9002/chrome/browser/speech/extens...
chrome/browser/speech/extension_speech_input_api_constants.h:5: #ifndef
CHROME_BROWSER_SPEECH_EXTENSION_SPEECH_INPUT_API_CONSTANTS_H_
There's no need for these separate 'constants' files anymore. They are an
artifact of some requirements we had once upon a time but no longer do.

Can you move these constants into an anonymous namespace of
speech_input_extension_api.cc? If you need to expose a few to the manager class,
you can expose them in the speech_input_extension_api.h file.

[Leandro Graciá Gil, 2011-11-07 17:11:43]

http://codereview.chromium.org/8386074/diff/9002/chrome/browser/speech/extens...
File chrome/browser/speech/extension_speech_input_api.h (right):

http://codereview.chromium.org/8386074/diff/9002/chrome/browser/speech/extens...
chrome/browser/speech/extension_speech_input_api.h:5: #ifndef
CHROME_BROWSER_SPEECH_EXTENSION_SPEECH_INPUT_API_H_
On 2011/11/06 22:55:38, Aaron Boodman wrote:
> To be consistent with the previous changes on the bug, these files should be
> called speech_input_extension_api.*. Sorry :-/.

Done.

http://codereview.chromium.org/8386074/diff/9002/chrome/browser/speech/extens...
File chrome/browser/speech/extension_speech_input_api_constants.h (right):

http://codereview.chromium.org/8386074/diff/9002/chrome/browser/speech/extens...
chrome/browser/speech/extension_speech_input_api_constants.h:5: #ifndef
CHROME_BROWSER_SPEECH_EXTENSION_SPEECH_INPUT_API_CONSTANTS_H_
On 2011/11/06 22:55:38, Aaron Boodman wrote:
> There's no need for these separate 'constants' files anymore. They are an
> artifact of some requirements we had once upon a time but no longer do.
> 
> Can you move these constants into an anonymous namespace of
> speech_input_extension_api.cc? If you need to expose a few to the manager
class,
> you can expose them in the speech_input_extension_api.h file.

Done.

[Satish, 2011-11-07 18:16:31]

http://codereview.chromium.org/8386074/diff/15001/chrome/app/generated_resour...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/8386074/diff/15001/chrome/app/generated_resour...
chrome/app/generated_resources.grd:14299: <ph name="PRODUCT_NAME">$2<ex>Google
Chrome</ex></ph> extension "<ph name="EXTENSION_NAME">$1<ex>Speech
Recognizer</ex></ph>" has started recording audio for speech recognition. Look
for the microphone tray icon to know when this happens again.
'tray' seems like a windows specific term. Should we use 'notification icon'
instead?

http://codereview.chromium.org/8386074/diff/15001/chrome/browser/speech/speec...
File chrome/browser/speech/speech_input_extension_api.cc (right):

http://codereview.chromium.org/8386074/diff/15001/chrome/browser/speech/speec...
chrome/browser/speech/speech_input_extension_api.cc:21: const char
kDefaultGrammar[] = "builtin:search";
I wonder if extension api should default to a non-search grammar. Can you check?

http://codereview.chromium.org/8386074/diff/15001/chrome/browser/speech/speec...
File chrome/browser/speech/speech_input_extension_manager.h (right):

http://codereview.chromium.org/8386074/diff/15001/chrome/browser/speech/speec...
chrome/browser/speech/speech_input_extension_manager.h:25: class
SpeechExtensionInterface {
should be SpeechInputExtensionInterface ?

http://codereview.chromium.org/8386074/diff/15001/chrome/browser/speech/speec...
File chrome/browser/speech/speech_input_extension_notification_ui.cc (right):

http://codereview.chromium.org/8386074/diff/15001/chrome/browser/speech/speec...
chrome/browser/speech/speech_input_extension_notification_ui.cc:46:
IDR_SPEECH_INPUT_TRAY_WINDOWS_MIC_EMPTY,
any reason why we need 3 different set of IDs? could use the same ID name for
these on all 3 platforms

http://codereview.chromium.org/8386074/diff/15001/chrome/browser/speech/speec...
chrome/browser/speech/speech_input_extension_notification_ui.cc:51: #else
can you make this into an explicit linux & chrome os define and perhaps handle
the #else clause properly, in case there is a new platform in future?

http://codereview.chromium.org/8386074/diff/15001/chrome/browser/speech/speec...
chrome/browser/speech/speech_input_extension_notification_ui.cc:98:
buffer_canvas.save();
since this is a scoped canvas and gets destroyed on return, is a .save() and
.restore() required?

http://codereview.chromium.org/8386074/diff/15001/chrome/browser/speech/speec...
chrome/browser/speech/speech_input_extension_notification_ui.cc:99: const int
kVolumeSteps = 6;
move this to an anonymous namespace constant

http://codereview.chromium.org/8386074/diff/15001/chrome/browser/speech/speec...
File chrome/browser/speech/speech_input_extension_notification_ui.h (right):

http://codereview.chromium.org/8386074/diff/15001/chrome/browser/speech/speec...
chrome/browser/speech/speech_input_extension_notification_ui.h:17: // Manage the
notification UI shown when an extension starts recording audio
Manage -> Manages

http://codereview.chromium.org/8386074/diff/15001/chrome/browser/speech/speec...
chrome/browser/speech/speech_input_extension_notification_ui.h:19: class
SpeechInputExtensionNotificationUI {
could drop the 'UI' from name?

http://codereview.chromium.org/8386074/diff/15001/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

http://codereview.chromium.org/8386074/diff/15001/chrome/common/pref_names.cc...
chrome/common/pref_names.cc:631: // Boolean pref to determine if the tray
notification balloon that shows when
could simplify
  'that shows when using the speech input extension API'
to
  'for speech input extension API'

[Leandro Graciá Gil, 2011-11-07 19:34:42]

http://codereview.chromium.org/8386074/diff/15001/chrome/app/generated_resour...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/8386074/diff/15001/chrome/app/generated_resour...
chrome/app/generated_resources.grd:14299: <ph name="PRODUCT_NAME">$2<ex>Google
Chrome</ex></ph> extension "<ph name="EXTENSION_NAME">$1<ex>Speech
Recognizer</ex></ph>" has started recording audio for speech recognition. Look
for the microphone tray icon to know when this happens again.
On 2011/11/07 18:16:31, Satish wrote:
> 'tray' seems like a windows specific term. Should we use 'notification icon'
> instead?

Done.

http://codereview.chromium.org/8386074/diff/15001/chrome/browser/speech/speec...
File chrome/browser/speech/speech_input_extension_api.cc (right):

http://codereview.chromium.org/8386074/diff/15001/chrome/browser/speech/speec...
chrome/browser/speech/speech_input_extension_api.cc:21: const char
kDefaultGrammar[] = "builtin:search";
On 2011/11/07 18:16:31, Satish wrote:
> I wonder if extension api should default to a non-search grammar. Can you
check?

Checking this. Will change it in a next patch if required.

http://codereview.chromium.org/8386074/diff/15001/chrome/browser/speech/speec...
File chrome/browser/speech/speech_input_extension_manager.h (right):

http://codereview.chromium.org/8386074/diff/15001/chrome/browser/speech/speec...
chrome/browser/speech/speech_input_extension_manager.h:25: class
SpeechExtensionInterface {
On 2011/11/07 18:16:31, Satish wrote:
> should be SpeechInputExtensionInterface ?

Done.

http://codereview.chromium.org/8386074/diff/15001/chrome/browser/speech/speec...
File chrome/browser/speech/speech_input_extension_notification_ui.cc (right):

http://codereview.chromium.org/8386074/diff/15001/chrome/browser/speech/speec...
chrome/browser/speech/speech_input_extension_notification_ui.cc:46:
IDR_SPEECH_INPUT_TRAY_WINDOWS_MIC_EMPTY,
On 2011/11/07 18:16:31, Satish wrote:
> any reason why we need 3 different set of IDs? could use the same ID name for
> these on all 3 platforms

Done.

http://codereview.chromium.org/8386074/diff/15001/chrome/browser/speech/speec...
chrome/browser/speech/speech_input_extension_notification_ui.cc:51: #else
On 2011/11/07 18:16:31, Satish wrote:
> can you make this into an explicit linux & chrome os define and perhaps handle
> the #else clause properly, in case there is a new platform in future?

Done.

http://codereview.chromium.org/8386074/diff/15001/chrome/browser/speech/speec...
chrome/browser/speech/speech_input_extension_notification_ui.cc:98:
buffer_canvas.save();
On 2011/11/07 18:16:31, Satish wrote:
> since this is a scoped canvas and gets destroyed on return, is a .save() and
> .restore() required?

Done.

http://codereview.chromium.org/8386074/diff/15001/chrome/browser/speech/speec...
chrome/browser/speech/speech_input_extension_notification_ui.cc:99: const int
kVolumeSteps = 6;
On 2011/11/07 18:16:31, Satish wrote:
> move this to an anonymous namespace constant

Done.

http://codereview.chromium.org/8386074/diff/15001/chrome/browser/speech/speec...
File chrome/browser/speech/speech_input_extension_notification_ui.h (right):

http://codereview.chromium.org/8386074/diff/15001/chrome/browser/speech/speec...
chrome/browser/speech/speech_input_extension_notification_ui.h:17: // Manage the
notification UI shown when an extension starts recording audio
On 2011/11/07 18:16:31, Satish wrote:
> Manage -> Manages

Done.

http://codereview.chromium.org/8386074/diff/15001/chrome/browser/speech/speec...
chrome/browser/speech/speech_input_extension_notification_ui.h:19: class
SpeechInputExtensionNotificationUI {
On 2011/11/07 18:16:31, Satish wrote:
> could drop the 'UI' from name?

Done.

http://codereview.chromium.org/8386074/diff/15001/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

http://codereview.chromium.org/8386074/diff/15001/chrome/common/pref_names.cc...
chrome/common/pref_names.cc:631: // Boolean pref to determine if the tray
notification balloon that shows when
On 2011/11/07 18:16:31, Satish wrote:
> could simplify
>   'that shows when using the speech input extension API'
> to
>   'for speech input extension API'

Done.

[Leandro Graciá Gil, 2011-11-09 11:35:45]

After checking with Fergus, I'm changing the default grammar to dictation.

[Satish, 2011-11-09 15:10:59]

LGTM

[Aaron Boodman, 2011-11-09 18:23:15]

lgtm

[Leandro Graciá Gil, 2011-11-11 13:39:31]

Creating the notification object dynamically to solve a problem affecting
resource loading in chrome frame net tests.

[Satish, 2011-11-11 13:56:12]

lgtm