Closed Bug 1185235 Opened 5 years ago Closed 5 years ago

Implement SpeechRecognition::maxAlternatives

Categories

(Core :: Web Speech, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: kdavis, Assigned: kdavis)

References

Details

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

Attachments

(1 file, 1 obsolete file)

Currently SpeechRecognition::maxAlternatives is not implemented
Assignee: nobody → kdavis
Whiteboard: [webspeechapi][vaani][systemsfe]
Depends on: 1185234
The depends on Bug 1185234 was added simply due to the possibility of merge
conflicts if this bug is landed first.
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)
Blocks: 1177514
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 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+
(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
(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.
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+
> > 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.
(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 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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/487ee69fefe9
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in before you can comment on or make changes to this bug.