1185235 - Implement SpeechRecognition::maxAlternatives

Status: RESOLVED FIXED

(Core :: Web Speech, defect)

Description

[kdavis]

Currently SpeechRecognition::maxAlternatives is not implemented

Comment 1

[kdavis]

The depends on Bug 1185234 was added simply due to the possibility of merge
conflicts if this bug is landed first.

Comment 2

[kdavis]

Attachment: 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

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

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

[Olli Pettay [:smaug][[e-mail]]]

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+

Comment 5

[kdavis]

(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

Comment 6

[kdavis]

(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.

Comment 7

[kdavis]

Attachment: 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

Comment 8

[kdavis]

> > 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.

Comment 9

[kdavis]

(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.

Comment 10

[kdavis]

(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

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/487ee69fefe9

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/487ee69fefe9