Bug 69867 - [Chromium] Allow building without speech input enabled.
Summary: [Chromium] Allow building without speech input enabled.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: John Knottenbelt
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-10-11 13:51 PDT by John Knottenbelt
Modified: 2011-10-13 12:05 PDT (History)
4 users (show)

See Also:


Attachments
Patch (10.63 KB, patch)
2011-10-11 13:51 PDT, John Knottenbelt
no flags Details | Formatted Diff | Diff
Patch (9.74 KB, patch)
2011-10-11 17:05 PDT, John Knottenbelt
no flags Details | Formatted Diff | Diff
Patch (6.40 KB, patch)
2011-10-12 13:06 PDT, John Knottenbelt
no flags Details | Formatted Diff | Diff
Patch (5.82 KB, patch)
2011-10-12 16:17 PDT, John Knottenbelt
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description John Knottenbelt 2011-10-11 13:51:16 PDT
[Chromium] Allow building without speech input enabled.
Comment 1 John Knottenbelt 2011-10-11 13:51:40 PDT
Created attachment 110569 [details]
Patch
Comment 2 WebKit Review Bot 2011-10-11 13:53:32 PDT
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
Comment 3 Darin Fisher (:fishd, Google) 2011-10-11 15:50:12 PDT
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.
Comment 4 John Knottenbelt 2011-10-11 17:05:29 PDT
Created attachment 110610 [details]
Patch
Comment 5 John Knottenbelt 2011-10-11 17:10:25 PDT
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?)
Comment 6 Darin Fisher (:fishd, Google) 2011-10-11 21:33:19 PDT
(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.
Comment 7 Darin Fisher (:fishd, Google) 2011-10-11 21:37:20 PDT
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).
Comment 8 John Knottenbelt 2011-10-12 13:06:48 PDT
Created attachment 110732 [details]
Patch
Comment 9 John Knottenbelt 2011-10-12 13:17:51 PDT
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?
Comment 10 John Knottenbelt 2011-10-12 13:19:55 PDT
Another approach would be make WebSpeechInputControllerMock::create return NULL for INPUT_SPEECH disabled.
Comment 11 Darin Fisher (:fishd, Google) 2011-10-12 14:11:14 PDT
(In reply to comment #10)
> Another approach would be make WebSpeechInputControllerMock::create return NULL for INPUT_SPEECH disabled.

Yes, you should do that.
Comment 12 John Knottenbelt 2011-10-12 16:17:11 PDT
Created attachment 110768 [details]
Patch
Comment 13 Satish Sampath 2011-10-13 01:39:25 PDT
Looks good, thanks for this change John.
Comment 14 WebKit Review Bot 2011-10-13 12:05:51 PDT
Comment on attachment 110768 [details]
Patch

Clearing flags on attachment: 110768

Committed r97379: <http://trac.webkit.org/changeset/97379>
Comment 15 WebKit Review Bot 2011-10-13 12:05:56 PDT
All reviewed patches have been landed.  Closing bug.