Summary: | Pass on all the speech recognition results to the input element. | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Satish Sampath <satish> | ||||||
Component: | Forms | Assignee: | Nobody <webkit-unassigned> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | dglazkov, eric, jorlow | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | All | ||||||||
OS: | All | ||||||||
Bug Depends on: | |||||||||
Bug Blocks: | 39485 | ||||||||
Attachments: |
|
Description
Satish Sampath
2010-10-21 07:52:00 PDT
Created attachment 71440 [details]
Patch
Comment on attachment 71440 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=71440&action=review > WebCore/page/SpeechInputListener.h:37 > + no newline > WebCore/page/SpeechInputListener.h:60 > + virtual void setRecognitionResult(int requestId, const SpeechInputResultArray& results) = 0; results not needed > WebCore/page/SpeechInputResult.cpp:37 > + extra new line > WebCore/page/SpeechInputResult.cpp:40 > + : m_confidence(0) What uses this? > WebCore/page/SpeechInputResult.h:32 > + no newline > WebCore/page/SpeechInputResult.h:48 > + SpeechInputResult(); What's this needed for? > WebCore/platform/mock/SpeechInputClientMock.cpp:96 > + results.append(SpeechInputResult::create(m_recognitionResult, 1.0)); Would we ever see a 1.0 in the real world? I guess it doesn't matter that much, though.. > WebKit/chromium/public/WebSpeechInputListener.h:58 > + virtual void setRecognitionResult(int, const WebSpeechInputResultArray&) = 0; None of these should be purely virtual. Use WEBKIT_ASSERT_NOT_REACHED. And, to avoid breaking the build, you should have the legacy version call the new version. > WebKit/chromium/public/WebSpeechInputResult.h:46 > +class WebSpeechInputResult { This should just wrap the WebCore version and not actually store stuff itself. Created attachment 71765 [details]
Patch
Addressed all review comments.
(In reply to comment #2) > (From update of attachment 71440 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=71440&action=review > > > WebCore/page/SpeechInputResult.cpp:40 > > + : m_confidence(0) > > What uses this? Apparently no one, removed. > > WebCore/page/SpeechInputResult.h:48 > > + SpeechInputResult(); > > What's this needed for? Not needed, removed. > > WebCore/platform/mock/SpeechInputClientMock.cpp:96 > > + results.append(SpeechInputResult::create(m_recognitionResult, 1.0)); > > Would we ever see a 1.0 in the real world? I guess it doesn't matter that much, though.. It would depend on the recognizer itself but 1.0 is a legal value per the proposed spec. > > WebKit/chromium/public/WebSpeechInputListener.h:58 > > + virtual void setRecognitionResult(int, const WebSpeechInputResultArray&) = 0; > > None of these should be purely virtual. Use WEBKIT_ASSERT_NOT_REACHED. Done > And, to avoid breaking the build, you should have the legacy version call the new version. Done > > WebKit/chromium/public/WebSpeechInputResult.h:46 > > +class WebSpeechInputResult { > > This should just wrap the WebCore version and not actually store stuff itself. Done Comment on attachment 71765 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=71765&action=review r=me > WebKit/chromium/public/WebSpeechInputResult.h:34 > +#if WEBKIT_IMPLEMENTATION Do we need to hide this behind WEBKIT_IMPLEMENTATION? What do other files do? (In reply to comment #5) > (From update of attachment 71765 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=71765&action=review > > > WebKit/chromium/public/WebSpeechInputResult.h:34 > > +#if WEBKIT_IMPLEMENTATION > > Do we need to hide this behind WEBKIT_IMPLEMENTATION? What do other files do? This is now not needed since I have included WebPrivatePtr.h which has this declaration. Hence removed. Attachment 71765 [details] did not build on chromium: Build output: http://queues.webkit.org/results/4779001 Committed r70490: <http://trac.webkit.org/changeset/70490> |