Disable the web speech input feature on extension popups.

Disable the web speech input feature on extension popups.

This patch disables the feature as it's currently enabled but broken. Extensions should use the new speech input extension API instead.

BUG=97388
TEST=install extension showing popup with a speech input button, click the mic icon.

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

[Leandro Graciá Gil, 2011-11-11 14:54:12]

Aaron, could you please take a look to the minimal ExtensionHost changes? I'll
need an owner.

Satish, please do a general review of the patch as usual.

[Aaron Boodman, 2011-11-11 17:14:54]

There is already GetRenderViewType() on RenderViewHostDelegate that you can use
for this.

But also, what are you trying to achieve here? If you want to disable this from
being callable in extensions, there are other RVHD you need to consider besides
ExtensionHost. Extension content can also be displayed in TabContents and
BalloonHost.

A better check would be something like:

const Extension* extension =
profile_->GetExtensionService()->GetExtensionByUrl(render_view_host->site_instance()->site(),
false);
if (extension && !extension->is_hosted_app())
  return;

[Leandro Graciá Gil, 2011-11-14 12:52:11]

We're trying to prevent recording for situations where the speech input bubble
will fail because the delegate is not a TabContents object. Not preventing this
leads to speech input recording without any user notification, which is broken
and might have privacy concerns. The main case for this are the speech input
buttons inside extension popups.

So, basically we're not trying to disable the feature for anything called from
extensions but to prevent recording for these already broken cases. In these
situations the experimental speech input extension API should be used instead.


On 2011/11/11 17:14:54, Aaron Boodman wrote:
> There is already GetRenderViewType() on RenderViewHostDelegate that you can
use
> for this.
> 
> But also, what are you trying to achieve here? If you want to disable this
from
> being callable in extensions, there are other RVHD you need to consider
besides
> ExtensionHost. Extension content can also be displayed in TabContents and
> BalloonHost.
> 
> A better check would be something like:
> 
> const Extension* extension =
>
profile_->GetExtensionService()->GetExtensionByUrl(render_view_host->site_instance()->site(),
> false);
> if (extension && !extension->is_hosted_app())
>   return;

[Aaron Boodman, 2011-11-14 19:09:17]

lgtm, thanks for the explanation.

http://codereview.chromium.org/8511062/diff/6001/content/browser/speech/speec...
File content/browser/speech/speech_input_manager.cc (right):

http://codereview.chromium.org/8511062/diff/6001/content/browser/speech/speec...
content/browser/speech/speech_input_manager.cc:116: if
(render_view_host->delegate()->GetRenderViewType() !=
(whitelist instead of blacklist)++. I suggest adding a comment explaining why
this branch exists.

[Leandro Graciá Gil, 2011-11-14 21:15:09]

http://codereview.chromium.org/8511062/diff/6001/content/browser/speech/speec...
File content/browser/speech/speech_input_manager.cc (right):

http://codereview.chromium.org/8511062/diff/6001/content/browser/speech/speec...
content/browser/speech/speech_input_manager.cc:116: if
(render_view_host->delegate()->GetRenderViewType() !=
On 2011/11/14 19:09:18, Aaron Boodman wrote:
> (whitelist instead of blacklist)++. I suggest adding a comment explaining why
> this branch exists.

Done.

[Satish, 2011-11-15 13:03:53]

http://codereview.chromium.org/8511062/diff/9001/content/browser/speech/speec...
File content/browser/speech/speech_input_manager.cc (right):

http://codereview.chromium.org/8511062/diff/9001/content/browser/speech/speec...
content/browser/speech/speech_input_manager.cc:108: void
SpeechInputManager::CheckForExtensionPopup(
can we rename this to CheckRenderViewTypeAndStartRecognition ? The mention of
extension popup won't be known to people who read this code later.

http://codereview.chromium.org/8511062/diff/9001/content/browser/speech/speec...
content/browser/speech/speech_input_manager.cc:117: // For privacy reasons,
prevent from start recording if the speech input
this comment is vague, suggest describing in detail why this is being done, i.e.
mention that for hosts other than tab contents we can't reliably show a popup
(with a reference to any relevant bug) and that extensions should also use the
extension api outside of pages shown in a tabcontents

[Leandro Graciá Gil, 2011-11-16 00:42:01]

http://codereview.chromium.org/8511062/diff/9001/content/browser/speech/speec...
File content/browser/speech/speech_input_manager.cc (right):

http://codereview.chromium.org/8511062/diff/9001/content/browser/speech/speec...
content/browser/speech/speech_input_manager.cc:108: void
SpeechInputManager::CheckForExtensionPopup(
On 2011/11/15 13:03:53, Satish wrote:
> can we rename this to CheckRenderViewTypeAndStartRecognition ? The mention of
> extension popup won't be known to people who read this code later.

Done.

http://codereview.chromium.org/8511062/diff/9001/content/browser/speech/speec...
content/browser/speech/speech_input_manager.cc:117: // For privacy reasons,
prevent from start recording if the speech input
On 2011/11/15 13:03:53, Satish wrote:
> this comment is vague, suggest describing in detail why this is being done,
i.e.
> mention that for hosts other than tab contents we can't reliably show a popup
> (with a reference to any relevant bug) and that extensions should also use the
> extension api outside of pages shown in a tabcontents

Done.

[Satish, 2011-11-16 13:08:55]

LGTM

http://codereview.chromium.org/8511062/diff/14001/content/browser/speech/spee...
File content/browser/speech/speech_input_manager.cc (right):

http://codereview.chromium.org/8511062/diff/14001/content/browser/speech/spee...
content/browser/speech/speech_input_manager.cc:123: 
could remove this extra newline

[Leandro Graciá Gil, 2011-11-16 14:21:09]

Nits fixed. Landing soon if no objections.

http://codereview.chromium.org/8511062/diff/14001/content/browser/speech/spee...
File content/browser/speech/speech_input_manager.cc (right):

http://codereview.chromium.org/8511062/diff/14001/content/browser/speech/spee...
content/browser/speech/speech_input_manager.cc:123: 
On 2011/11/16 13:08:56, Satish wrote:
> could remove this extra newline

Done.