Bug 48068

Summary: Pass on all the speech recognition results to the input element.
Product: WebKit Reporter: Satish Sampath <satish>
Component: FormsAssignee: 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 Flags
Patch
none
Patch jorlow: review+

Satish Sampath
Reported 2010-10-21 07:52:00 PDT
The speech input implementation currently gives the input element only a single string when speech recognition completes. Instead we want to give all the matching results with a confidence value for each result. This list will be exposed as an attribute in a future CL and scripts can make use of it.
Attachments
Patch (29.94 KB, patch)
2010-10-21 08:26 PDT, Satish Sampath 		no flags
DetailsFormatted DiffDiff
Patch (30.61 KB, patch)
2010-10-25 09:59 PDT, Satish Sampath 		jorlow: review+
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Satish Sampath
Comment 1 2010-10-21 08:26:28 PDT
Created attachment 71440 [details] Patch
Jeremy Orlow
Comment 2 2010-10-25 07:07:40 PDT
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.
Satish Sampath
Comment 3 2010-10-25 09:59:55 PDT
Created attachment 71765 [details] Patch Addressed all review comments.
Satish Sampath
Comment 4 2010-10-25 10:02:09 PDT
(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
Jeremy Orlow
Comment 5 2010-10-25 10:08:09 PDT
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?
Satish Sampath
Comment 6 2010-10-25 12:58:36 PDT
(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.
Eric Seidel (no email)
Comment 7 2010-10-25 13:17:11 PDT
Attachment 71765 [details] did not build on chromium: Build output: http://queues.webkit.org/results/4779001
Satish Sampath
Comment 8 2010-10-25 14:10:54 PDT
Committed r70490: <http://trac.webkit.org/changeset/70490>
Note You need to log in before you can comment on or make changes to this bug.