Bug 80424 - Speech JavaScript API: SpeechRecognitionAlternative, Result and ResultList
Summary: Speech JavaScript API: SpeechRecognitionAlternative, Result and ResultList
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: Hans Wennborg
URL:
Keywords:
Depends on: 80410
Blocks: 80513
  Show dependency treegraph
 
Reported: 2012-03-06 08:16 PST by Hans Wennborg
Modified: 2012-03-08 03:22 PST (History)
4 users (show)

See Also:


Attachments
Patch (24.51 KB, patch)
2012-03-06 08:21 PST, Hans Wennborg
no flags Details | Formatted Diff | Diff
Patch (24.57 KB, patch)
2012-03-07 03:37 PST, Hans Wennborg
no flags Details | Formatted Diff | Diff
Patch (24.51 KB, patch)
2012-03-07 09:00 PST, Hans Wennborg
abarth: review+
abarth: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hans Wennborg 2012-03-06 08:16:07 PST
Speech JavaScript API: SpeechRecognitionAlternative, Result and ResultList
Comment 1 Hans Wennborg 2012-03-06 08:21:31 PST
Created attachment 130389 [details]
Patch
Comment 2 Hans Wennborg 2012-03-07 03:37:23 PST
Created attachment 130589 [details]
Patch

Rebase and link to spec in the ChangeLog.
Comment 3 Satish Sampath 2012-03-07 08:46:06 PST
Comment on attachment 130589 [details]
Patch

looks good
Comment 4 Hans Wennborg 2012-03-07 09:00:42 PST
Created attachment 130633 [details]
Patch

Remove FIXME as per offline discussion.
Comment 5 Adam Barth 2012-03-07 10:32:47 PST
Comment on attachment 130633 [details]
Patch

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

> Source/WebCore/Modules/speech/SpeechRecognitionAlternative.h:42
> +    String transcript() { return m_transcript; }

String transcript() -> const String& transcript() const

> Source/WebCore/Modules/speech/SpeechRecognitionAlternative.h:43
> +    double confidence() { return m_confidence; }

double confidence() -> double confidence() const

> Source/WebCore/Modules/speech/SpeechRecognitionResult.h:42
> +    PassRefPtr<SpeechRecognitionAlternative> item(unsigned long index);

Why a PassRefPtr ?  We're not transferring ownership.  This should return a SpeechRecognitionAlternative*.  Please see http://www.webkit.org/coding/RefPtr.html for more information about RefPtr and friends.

> Source/WebCore/Modules/speech/SpeechRecognitionResult.idl:31
> +        getter SpeechRecognitionAlternative item(in unsigned long index);

As I mentioned in the other patch, we don't support the "getter" syntax (yet!).  Instead you should use https://trac.webkit.org/wiki/WebKitIDL#IndexedGetter

> Source/WebCore/Modules/speech/SpeechRecognitionResultList.h:42
> +    PassRefPtr<SpeechRecognitionResult> item(unsigned long index);

Same issue here.  This should be a SpeechRecognitionResult*

> Source/WebCore/Modules/speech/SpeechRecognitionResultList.idl:31
> +        getter SpeechRecognitionResult item(in unsigned long index);

Same issue with "getter"
Comment 6 Hans Wennborg 2012-03-08 03:18:00 PST
Thanks very much for the review!


(In reply to comment #5)
> (From update of attachment 130633 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=130633&action=review
> 
> > Source/WebCore/Modules/speech/SpeechRecognitionAlternative.h:42
> > +    String transcript() { return m_transcript; }
> 
> String transcript() -> const String& transcript() const
Done.

> > Source/WebCore/Modules/speech/SpeechRecognitionAlternative.h:43
> > +    double confidence() { return m_confidence; }
> 
> double confidence() -> double confidence() const
Done.

> > Source/WebCore/Modules/speech/SpeechRecognitionResult.h:42
> > +    PassRefPtr<SpeechRecognitionAlternative> item(unsigned long index);
> 
> Why a PassRefPtr ?  We're not transferring ownership.  This should return a SpeechRecognitionAlternative*.  Please see http://www.webkit.org/coding/RefPtr.html for more information about RefPtr and friends.
Done.

I was thinking that we're passing ownership to the V8 wrapper object, but I see now that PassRefPtr is not used in this situation. Thanks for catching it.

> > Source/WebCore/Modules/speech/SpeechRecognitionResult.idl:31
> > +        getter SpeechRecognitionAlternative item(in unsigned long index);
> 
> As I mentioned in the other patch, we don't support the "getter" syntax (yet!).  Instead you should use https://trac.webkit.org/wiki/WebKitIDL#IndexedGetter
Done.

> > Source/WebCore/Modules/speech/SpeechRecognitionResultList.h:42
> > +    PassRefPtr<SpeechRecognitionResult> item(unsigned long index);
> 
> Same issue here.  This should be a SpeechRecognitionResult*
Done.

> > Source/WebCore/Modules/speech/SpeechRecognitionResultList.idl:31
> > +        getter SpeechRecognitionResult item(in unsigned long index);
> 
> Same issue with "getter"
Done.
Comment 7 Hans Wennborg 2012-03-08 03:22:49 PST
Committed r110160: <http://trac.webkit.org/changeset/110160>