Allow speech input feature to be disabled.

Allow speech input feature to be disabled.

BUG=None
TEST=Existing


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

[John Knottenbelt, 2011-10-11 01:03:12]

This is the chromium-side of the patch. WebKit-side forthcoming.

[Satish, 2011-10-11 09:47:34]

Could you add some of the OWNERs in chrome/browser and content/... directories?
I'm interested in how they feel about feature defines like this in chromium.

[John Knottenbelt, 2011-10-11 15:54:45]

Hi John, Miranda,

This change (in combination with a forthcoming webkit change) will allow the
speech input feature to be compiled out. 

Your comments would be much appreciated, especially about the approach of
introducing a flag in common.gypi, corresponding preprocessor define and macros
in the code.

Many thanks

John

[Miranda Callahan, 2011-10-11 17:04:59]

On 2011/10/11 15:54:45, John Knottenbelt wrote:
> Hi John, Miranda,
> 
> This change (in combination with a forthcoming webkit change) will allow the
> speech input feature to be compiled out. 
> 
> Your comments would be much appreciated, especially about the approach of
> introducing a flag in common.gypi, corresponding preprocessor define and
macros
> in the code.
> 
> Many thanks
> 
> John

Thanks, John. If this patch could be combined with refactoring the speech input
feature out of profile_impl.h and into a factory-based model, that might be
better than if-deffing the feature itself in the profile class. See
http://code.google.com/p/chromium/issues/detail?id=77155 -- all external
services will need to be refactored out of Profile sooner or later.

[jam, 2011-10-11 20:32:47]

On 2011/10/11 17:04:59, Miranda Callahan wrote:
> On 2011/10/11 15:54:45, John Knottenbelt wrote:
> > Hi John, Miranda,
> > 
> > This change (in combination with a forthcoming webkit change) will allow the
> > speech input feature to be compiled out. 
> > 
> > Your comments would be much appreciated, especially about the approach of
> > introducing a flag in common.gypi, corresponding preprocessor define and
> macros
> > in the code.
> > 
> > Many thanks
> > 
> > John
> 
> Thanks, John. If this patch could be combined with refactoring the speech
input
> feature out of profile_impl.h and into a factory-based model, that might be
> better than if-deffing the feature itself in the profile class. See
> http://code.google.com/p/chromium/issues/detail?id=77155 -- all external
> services will need to be refactored out of Profile sooner or later.

Miranda's suggestion sounds good, so I'll defer to her.

In general though, for ifdef'ing features out, it's better to have as minimal
number of ifdef's as possible. so adding filters to channel etc would be
ifdef'd, but not member variables or functions (just make the one line that
creates the object).

[John Knottenbelt, 2011-10-11 20:46:33]

Thanks for your comments. I've reduced the change as much as I can, and now it's
not necessary to touch the profile stuff, although I do agree that it would be a
good idea to do the factory refactoring (as a separate cl).

I also disabled the various speech tests from building. 

Tony, please could you review the changes to test_shell?

[tony, 2011-10-11 21:33:35]

test_shell LGTM

[jam, 2011-10-12 03:08:07]

lgtm

http://codereview.chromium.org/8222025/diff/3013/content/renderer/render_view...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/8222025/diff/3013/content/renderer/render_view...
content/renderer/render_view_impl.cc:4385: #if defined(ENABLE_INPUT_SPEECH)
i would even wrap this around 86 and 87 only :)

http://codereview.chromium.org/8222025/diff/3013/webkit/tools/test_shell/test...
File webkit/tools/test_shell/test_shell.cc (right):

http://codereview.chromium.org/8222025/diff/3013/webkit/tools/test_shell/test...
webkit/tools/test_shell/test_shell.cc:608: #if ENABLE_INPUT_SPEECH
nit: around 10 and 11

[commit-bot: I haz the power, 2011-10-14 03:25:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jknotten@chromium.org/8222025/11001

[commit-bot: I haz the power, 2011-10-14 05:08:23]

Change committed as 105463