Bug 69867 – [Chromium] Allow building without speech input enabled.

• Component: New Bugs
• Status: RESOLVED FIXED

[John Knottenbelt]

[Chromium] Allow building without speech input enabled.

[John Knottenbelt]

Created attachment 110569 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[Darin Fisher (:fishd, Google)]

Comment on attachment 110569 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=110569&action=review

> Source/WebKit/chromium/public/WebSpeechInputListener.h:34
> +#if ENABLE_INPUT_SPEECH

we don't put ENABLE_ options in WebKit API headers.  chromium doesn't see config.h.

these should only be used in the .cpp files to protect against using WebCore classes and functions that are only defined under ENABLE_INPUT_SPEECH.

[John Knottenbelt]

Created attachment 110610 [details]
Patch

[John Knottenbelt]

I propose to remove the #if ENABLE(INPUT_SPEECH) guards from the WebCore headers so that WebKit/chromium/src/WebSpeechInputControllerMockImpl.h header file can compile as it inherits from WebCore::SpeechInputListener. 

An alternative would be to put the guards into the WebKit/chromium/src/WebSpeechInputControllerMockImpl.h file (perhaps ok, since it's in the src directory?)

[Darin Fisher (:fishd, Google)]

(In reply to comment #5)
> I propose to remove the #if ENABLE(INPUT_SPEECH) guards from the WebCore headers so that WebKit/chromium/src/WebSpeechInputControllerMockImpl.h header file can compile as it inherits from WebCore::SpeechInputListener. 
> 
> An alternative would be to put the guards into the WebKit/chromium/src/WebSpeechInputControllerMockImpl.h file (perhaps ok, since it's in the src directory?)

Yes, you can use ENABLE(FOO) in chromium/src/.  You just have to hide ENABLE(FOO) from the consumer of the API since they don't get to see config.h.

[Darin Fisher (:fishd, Google)]

Comment on attachment 110610 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=110610&action=review

> Source/WebCore/page/SpeechInputListener.h:-34
> -#if ENABLE(INPUT_SPEECH)

I don't understand why you are removing these guards.  Don't we want
to prevent this code from being seen when people are not interested
in building with INPUT_SPEECH enabled?

> Tools/DumpRenderTree/chromium/LayoutTestController.cpp:90
> +#if ENABLE(INPUT_SPEECH)

I would not expect to see any DumpRenderTree changes since DumpRenderTree
just consumes WebKit via the WebKit API, and the API entrypoints exist
regardless of the value of ENABLE(INPUT_SPEECH).

[John Knottenbelt]

Created attachment 110732 [details]
Patch

[John Knottenbelt]

Thanks again for review, Darin. 

This patch leaves the WebCore files alone. 

I still needed to make changes to DumpRenderTree because when INPUT_SPEECH is disabled, I cannot call WebSpeechInputControllerMock::create to create the mock since this is provided by WebKit/chromium/src/WebSpeechInputControllerMockImpl.cpp which is compiled out. 

I think I can avoid the changes to DumpRenderTree completely if I provide an implementation of WebSpeechInputControllerMock::create (for INPUT_SPEECH disabled) that would yield a dummy mock object that ignores method invocations. Would this be preferable?

[John Knottenbelt]

Another approach would be make WebSpeechInputControllerMock::create return NULL for INPUT_SPEECH disabled.

[Darin Fisher (:fishd, Google)]

(In reply to comment #10)
> Another approach would be make WebSpeechInputControllerMock::create return NULL for INPUT_SPEECH disabled.

Yes, you should do that.

[John Knottenbelt]

Created attachment 110768 [details]
Patch

[Satish Sampath]

Looks good, thanks for this change John.

[WebKit Review Bot]

Comment on attachment 110768 [details]
Patch

Clearing flags on attachment: 110768

Committed r97379: <http://trac.webkit.org/changeset/97379>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.