Bug 48068 - Pass on all the speech recognition results to the input element.
Summary: Pass on all the speech recognition results to the input element.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 39485
  Show dependency treegraph
 
Reported: 2010-10-21 07:52 PDT by Satish Sampath
Modified: 2010-10-25 14:10 PDT (History)
3 users (show)

See Also:


Attachments
Patch (29.94 KB, patch)
2010-10-21 08:26 PDT, Satish Sampath
no flags Details | Formatted Diff | Diff
Patch (30.61 KB, patch)
2010-10-25 09:59 PDT, Satish Sampath
jorlow: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Satish Sampath 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.
Comment 1 Satish Sampath 2010-10-21 08:26:28 PDT
Created attachment 71440 [details]
Patch
Comment 2 Jeremy Orlow 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.
Comment 3 Satish Sampath 2010-10-25 09:59:55 PDT
Created attachment 71765 [details]
Patch

Addressed all review comments.
Comment 4 Satish Sampath 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
Comment 5 Jeremy Orlow 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?
Comment 6 Satish Sampath 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.
Comment 7 Eric Seidel (no email) 2010-10-25 13:17:11 PDT
Attachment 71765 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4779001
Comment 8 Satish Sampath 2010-10-25 14:10:54 PDT
Committed r70490: <http://trac.webkit.org/changeset/70490>