Implement SpeechRecognition::maxAlternatives

RESOLVED FIXED in Firefox 42

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: kdavis, Assigned: kdavis)

Tracking

unspecified
mozilla42
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox42 fixed)

Details

(Whiteboard: [webspeechapi][vaani][systemsfe])

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

3 years ago
Currently SpeechRecognition::maxAlternatives is not implemented
(Assignee)

Updated

3 years ago
Assignee: nobody → kdavis
Whiteboard: [webspeechapi][vaani][systemsfe]
(Assignee)

Updated

3 years ago
Depends on: 1185234
(Assignee)

Comment 1

3 years ago
The depends on Bug 1185234 was added simply due to the possibility of merge
conflicts if this bug is landed first.
(Assignee)

Comment 2

3 years ago
Created attachment 8638130 [details] [diff] [review]
Part 1 of 1: Implemented SpeechRecognition::maxAlternatives

Part 1 of 1: Implemented SpeechRecognition::maxAlternatives
                                                                                
Part 1 of 1 for this bug.                                                       
                                                                                
This patch implements SpeechRecognition::maxAlternatives described in the Web    
Speech API specification                                                        
                                                                                
https://dvcs.w3.org/hg/speech-api/raw-file/tip/webspeechapi.html#dfn-maxalternatives
                                                                                
This implementation is particularly simple as the Web Speech API requires of    
the attribute SpeechRecognition::maxAlternatives that                            
                                                                                
1. Default value is 1                     
2. Subsequent value is the "maximum number of SpeechRecognitionAlternatives per result"                               
                                                                                
As Pocketsphinx can only return at maximum a single SpeechRecognitionAlternative
per SpeechRecognitionResult, we can trivially implement this attribute in a conforming
manner by

1. Defaulting mMaxAlternatives to 1
2. Ignore all non-zero values of mMaxAlternatives, ie return, as normal, only a single
   SpeechRecognitionAlternative per SpeechRecognitionResult
3. If mMaxAlternatives is 0 return no SpeechRecognitionAlternative per SpeechRecognitionResult                         
                                                                                
The try for this patch is here https://treeherder.mozilla.org/#/jobs?repo=try&revision=e5c7bbc9e7f9
Attachment #8638130 - Flags: review?(bugs)
(Assignee)

Updated

3 years ago
Blocks: 1177514

Comment 3

3 years ago
For future patches, please generate them with some more context, 8 lines is the common one.
I have
[defaults]
diff = -p -U 8
in my .hgrc.

Comment 4

3 years ago
Comment on attachment 8638130 [details] [diff] [review]
Part 1 of 1: Implemented SpeechRecognition::maxAlternatives

>+    if (0 != mRecognition->GetMaxAlternatives(rv)) { // GetMaxAlternatives can't fail
I would probably write that
+    if (mRecognition->GetMaxAlternatives(rv) > 0) { // GetMaxAlternatives can't fail
but just because of personally I think that is easier to read.
Either way.

But, since getter/setter of maxAlternatives can't actually throw any exception anymore, remove
http://mxr.mozilla.org/mozilla-central/source/dom/webidl/SpeechRecognition.webidl?rev=417b50770873&mark=26-26#27
and then ErrorResult& aRv params from the methods, and then you don't need to have these dummy ErrorResult rv; variables.
With that, r+
Attachment #8638130 - Flags: review?(bugs) → review+
(Assignee)

Comment 5

3 years ago
(In reply to Olli Pettay [:smaug] (away-ish until July 26, expect slower than usual reviews) from comment #3)
> For future patches, please generate them with some more context, 8 lines is
> the common one.
> I have
> [defaults]
> diff = -p -U 8
> in my .hgrc.

Ok will do.

I am using git, then git-patch-to-hg-patch, but for anyone that finds this
in the future, including myself, it seems like the way to configure this
for git to to issue

git config --global diff.context 8

which adds the lines

[diff]
        context = 8

to .gitconfig
(Assignee)

Comment 6

3 years ago
(In reply to Olli Pettay [:smaug] (away-ish until July 26, expect slower than usual reviews) from comment #4)
> Comment on attachment 8638130 [details] [diff] [review]
> Part 1 of 1: Implemented SpeechRecognition::maxAlternatives
> 
> >+    if (0 != mRecognition->GetMaxAlternatives(rv)) { // GetMaxAlternatives can't fail
> I would probably write that
> +    if (mRecognition->GetMaxAlternatives(rv) > 0) { // GetMaxAlternatives
> can't fail
> but just because of personally I think that is easier to read.
> Either way.

Ok. I'll switch this

> But, since getter/setter of maxAlternatives can't actually throw any
> exception anymore, remove
> http://mxr.mozilla.org/mozilla-central/source/dom/webidl/SpeechRecognition.
> webidl?rev=417b50770873&mark=26-26#27
> and then ErrorResult& aRv params from the methods, and then you don't need
> to have these dummy ErrorResult rv; variables.
> With that, r+

Ok. I'll do this too, but I'll put it in another patch/bug to separate this
issue from the issue of SpeechRecognition::maxAlternatives implementation,
as the two are orthogonal.
(Assignee)

Comment 7

3 years ago
Created attachment 8639143 [details] [diff] [review]
Part 1 of 1: Implemented SpeechRecognition::maxAlternatives

Part 1 of 1: Implemented SpeechRecognition::maxAlternatives

Part 1 of 1 for this bug.

This patch has the same functionality as the previous patch of Comment 2.
However, this patch incorporates changes suggested in Comment 4.

In particular lines of the form:

  if (0 != mRecognition->GetMaxAlternatives(rv)) { // GetMaxAlternatives can't fail

were replaced with lines of the form:

  if (0 < mRecognition->GetMaxAlternatives(rv)) { // GetMaxAlternatives can't fail

The full interdiff between this patch and the previous patch is as follows:

diff -u b/dom/media/webspeech/recognition/PocketSphinxSpeechRecognitionService.cpp b/dom/media/webspeech/recognition/PocketSphinxSpeechRecognitionService.cpp
--- b/dom/media/webspeech/recognition/PocketSphinxSpeechRecognitionService.cpp
+++ b/dom/media/webspeech/recognition/PocketSphinxSpeechRecognitionService.cpp
@@ -55,7 +55,7 @@
       new SpeechRecognitionResultList(mRecognition);
     SpeechRecognitionResult* result = new SpeechRecognitionResult(mRecognition);
     ErrorResult rv;
-    if (0 != mRecognition->GetMaxAlternatives(rv)) { // GetMaxAlternatives can't fail
+    if (0 < mRecognition->GetMaxAlternatives(rv)) { // GetMaxAlternatives can't fail
       SpeechRecognitionAlternative* alternative =
         new SpeechRecognitionAlternative(mRecognition);
 
@@ -334,7 +334,7 @@
     new SpeechRecognitionResultList(mRecognition);
   SpeechRecognitionResult* result = new SpeechRecognitionResult(mRecognition);
   ErrorResult rv;
-  if (0 != mRecognition->GetMaxAlternatives(rv)) { // GetMaxAlternatives can't fail
+  if (0 < mRecognition->GetMaxAlternatives(rv)) { // GetMaxAlternatives can't fail
     SpeechRecognitionAlternative* alternative =
       new SpeechRecognitionAlternative(mRecognition);
 
diff -u b/dom/media/webspeech/recognition/test/FakeSpeechRecognitionService.cpp b/dom/media/webspeech/recognition/test/FakeSpeechRecognitionService.cpp
--- b/dom/media/webspeech/recognition/test/FakeSpeechRecognitionService.cpp
+++ b/dom/media/webspeech/recognition/test/FakeSpeechRecognitionService.cpp
@@ -103,7 +103,7 @@
   SpeechRecognitionResultList* resultList = new SpeechRecognitionResultList(mRecognition);
   SpeechRecognitionResult* result = new SpeechRecognitionResult(mRecognition);
   ErrorResult rv;
-  if (0 != mRecognition->GetMaxAlternatives(rv)) { // GetMaxAlternatives can't fail
+  if (0 < mRecognition->GetMaxAlternatives(rv)) { // GetMaxAlternatives can't fail
     SpeechRecognitionAlternative* alternative = new SpeechRecognitionAlternative(mRecognition);
 
     alternative->mTranscript = NS_LITERAL_STRING("Mock final result");


The try for this patch is here https://treeherder.mozilla.org/#/jobs?repo=try&revision=dc0c3f0988a8
Attachment #8638130 - Attachment is obsolete: true
Attachment #8639143 - Flags: review+
(Assignee)

Comment 8

3 years ago
> > But, since getter/setter of maxAlternatives can't actually throw any
> > exception anymore, remove
> > http://mxr.mozilla.org/mozilla-central/source/dom/webidl/SpeechRecognition.
> > webidl?rev=417b50770873&mark=26-26#27
> > and then ErrorResult& aRv params from the methods, and then you don't need
> > to have these dummy ErrorResult rv; variables.
> > With that, r+
> 
> Ok. I'll do this too, but I'll put it in another patch/bug to separate this
> issue from the issue of SpeechRecognition::maxAlternatives implementation,
> as the two are orthogonal.

I've introduced Bug 1187791 to track this issue.
(Assignee)

Comment 9

3 years ago
(In reply to kdavis from comment #7)
> Created attachment 8639143 [details] [diff] [review]
> Part 1 of 1: Implemented SpeechRecognition::maxAlternatives
> 
> The try for this patch is here
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=dc0c3f0988a8

Unfortunately it looks like with my rebase I am now sitting on some broken code.
So, I will rebase again and post another try hoping the broken stuff is backed
out.
(Assignee)

Comment 10

3 years ago
(In reply to kdavis from comment #9)
> (In reply to kdavis from comment #7)
> > Created attachment 8639143 [details] [diff] [review]
> > Part 1 of 1: Implemented SpeechRecognition::maxAlternatives
> > 
> > The try for this patch is here
> > https://treeherder.mozilla.org/#/jobs?repo=try&revision=dc0c3f0988a8
> 
> Unfortunately it looks like with my rebase I am now sitting on some broken
> code.
> So, I will rebase again and post another try hoping the broken stuff is
> backed
> out.

In the interm there was a backout. So here is a try rebased over code which
is itself not broken https://treeherder.mozilla.org/#/jobs?repo=try&revision=fc4372a65840
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/487ee69fefe9
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox42: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in before you can comment on or make changes to this bug.