Closed Bug 1051148 Opened 5 years ago Closed 4 years ago

Implementation of Webspeech API & PocketSphinx Integration

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
2.2 S14 (12june)
Tracking Status
firefox41 --- fixed

People

(Reporter: anatal, Assigned: kdavis)

References

(Blocks 4 open bugs)

Details

(Whiteboard: [webspeechapi][systemsfe])

User Story

Implement Web Speech API in Firefox OS baseline according to W3C standards

Attachments

(2 files, 19 obsolete files)

33.18 KB, patch
kdavis
: review+
Details | Diff | Splinter Review
1.60 KB, patch
kdavis
: review+
Details | Diff | Splinter Review
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:31.0) Gecko/20100101 Firefox/31.0 (Beta/Release)
Build ID: 20140716183446
Blocks: 1049931
OS: Mac OS X → All
Hardware: x86 → All
Depends on: 1051130
Depends on: 1051146
Depends on: 1051151
Depends on: 1051152
Depends on: 1051153
Depends on: 1051154
Depends on: 1051554
This patch enables WebSpeechAPI with a PocketsphinxService for use with grammars.

Few notes:

1 - Code is multiplatform and tests and builds was made in all platforms (Mac, Linux, B2G, Fennec) less Windows that needs a fix

2 - The patch includes all pocketsphinx sources and models with moz.build and packaging/installer configuration

3 - This patch does not auto set Web Speech API preferences that still should be changed by the user

4 - The decode is made in-memory and on a thread
Attachment #8470537 - Flags: review?(bugs)
Depends on: 1051605
Depends on: 1051606
Depends on: 1051607
Comment on attachment 8470537 [details] [diff] [review]
Patch to enable offline WebSpeech API with a PocketSphinxService

You need to split the patch to reviewable parts.
Attachment #8470537 - Flags: review?(bugs) → review-
(In reply to Olli Pettay [:smaug] from comment #2)
> Comment on attachment 8470537 [details] [diff] [review]
> Patch to enable offline WebSpeech API with a PocketSphinxService
> 
> You need to split the patch to reviewable parts.

ggp said me that I need to do only one commit. How do you want this to be split ? One patch per bug or  only two: one for pocketsphinx sources and models, and another for PocketSphinxSpeechRecognitionService?
Flags: needinfo?(bugs)
I'm not going review all pocketsphinx source codes, only the code you have written.
(pocketsphinx certainly needs security audit and some fuzz testing before we expose the functionality
to the web.)
Flags: needinfo?(bugs)
Also, is the license of pocketsphinx such that we can distribute it with Gecko?
(I think it is, but better to ask gerv)
(In reply to Olli Pettay [:smaug] from comment #6)
> (I think it is, but better to ask gerv)

Yes
(In reply to Olli Pettay [:smaug] from comment #4)
> I'm not going review all pocketsphinx source codes, only the code you have
> written.
> (pocketsphinx certainly needs security audit and some fuzz testing before we
> expose the functionality
> to the web.)

Ok
(In reply to Andre Natal from comment #7)
> (In reply to Olli Pettay [:smaug] from comment #6)
> > (I think it is, but better to ask gerv)
> 
> Yes

@Sandip has the license and is addressing this.
Flags: needinfo?(skamat)
Reviewing it with Legal this week. (cc: Geoff and Gerv)
Flags: needinfo?(skamat)
There is no problem with the license of PocketSphinx. Please make sure a copy of the COPYING file makes it into whichever directory you import the code into.

Gerv
(In reply to Gervase Markham [:gerv] from comment #11)
> There is no problem with the license of PocketSphinx. Please make sure a
> copy of the COPYING file makes it into whichever directory you import the
> code into.
> 
> Gerv

Thanks Gerv. Andre - can you pls take care of this?
Flags: needinfo?(anatal)
(In reply to Sandip Kamat from comment #12)
> (In reply to Gervase Markham [:gerv] from comment #11)
> > There is no problem with the license of PocketSphinx. Please make sure a
> > copy of the COPYING file makes it into whichever directory you import the
> > code into.
> > 
> > Gerv
> 
> Thanks Gerv. Andre - can you pls take care of this?

Yes, no problem.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Brief benchmark on Flame


> START LISTENING
User 3%, System 4%, IOW 0%, IRQ 0%
User 1 + Nice 20 + Sys 27 + Idle 523 + IOW 0 + IRQ 0 + SIRQ 0 = 571

  PID   TID PR CPU% S     VSS     RSS PCY UID      Thread          Proc
 1597  1643  0   4% S 140868K  70452K     root     opensl_rec_thre /system/b2g/plugin-container
 1597  1627  0   2% S 140868K  70452K     root     AudioGUM        /system/b2g/plugin-container
 1597  1640  0   2% S 140868K  70452K     root     MediaStreamGrph /system/b2g/plugin-container
 1562  1562  1   2% R   1288K    620K     root     top             top
   96    96  1   1% S      0K      0K     root     ksmd            
 1597  1597  0   0% S 140868K  70452K     root     Browser         /system/b2g/plugin-container
 1597  1629  1   0% S 140868K  70452K     root     ProcessThread   /system/b2g/plugin-container
 1597  1642  1   0% S 140868K  70452K     root     AudioRecord     /system/b2g/plugin-container
  279  1641  0   0% S  21696K   6720K  fg media    AudioIn_A       /system/bin/mediaserver
 1574  1574  0   0% D      0K      0K     root     kworker/u:3     


> EXACTLY POINT OF RECOGNITION 
User 15%, System 7%, IOW 0%, IRQ 0%
User 30 + Nice 59 + Sys 44 + Idle 438 + IOW 4 + IRQ 0 + SIRQ 1 = 576

  PID   TID PR CPU% S     VSS     RSS PCY UID      Thread          Proc
 1597  1597  1   6% S 140284K  70456K     root     Browser         /system/b2g/plugin-container
 1597  1643  1   4% S 140284K  70456K     root     opensl_rec_thre /system/b2g/plugin-container
  287   287  0   3% S 188440K  83148K     root     b2g             /system/b2g/b2g
 1597  1627  0   2% S 140284K  70456K     root     AudioGUM        /system/b2g/plugin-container
  287   712  0   2% S 188440K  83148K     root     Compositor      /system/b2g/b2g
 1597  1640  1   1% S 140284K  70456K     root     MediaStreamGrph /system/b2g/plugin-container
 1562  1562  0   1% R   1288K    620K     root     top             top
   96    96  1   1% S      0K      0K     root     ksmd            
 1597  1629  0   0% S 140284K  70456K     root     ProcessThread   /system/b2g/plugin-container
  287  1600  1   0% S 188440K  83148K     root     BufMgrParent#15 /system/b2g/b2g


> STOP LISTENING
User 10%, System 5%, IOW 0%, IRQ 0%
User 6 + Nice 57 + Sys 30 + Idle 478 + IOW 1 + IRQ 0 + SIRQ 2 = 574

  PID   TID PR CPU% S     VSS     RSS PCY UID      Thread          Proc
 1597  1627  0   5% S 140868K  70544K     root     AudioGUM        /system/b2g/plugin-container
 1597  1643  1   5% S 140868K  70544K     root     opensl_rec_thre /system/b2g/plugin-container
 1597  1597  1   3% S 140868K  70544K     root     Browser         /system/b2g/plugin-container
 1597  1640  1   2% S 140868K  70544K     root     MediaStreamGrph /system/b2g/plugin-container
 1562  1562  0   2% R   1288K    620K     root     top             top
   96    96  0   1% S      0K      0K     root     ksmd            
  287   712  1   0% S 189024K  83148K     root     Compositor      /system/b2g/b2g
 1597  1629  1   0% S 140868K  70544K     root     ProcessThread   /system/b2g/plugin-container
 1597  1642  1   0% S 140868K  70544K     root     AudioRecord     /system/b2g/plugin-container
  279  1641  1   0% S  21696K   6720K  fg media    AudioIn_A       /system/bin/mediaserver
OK
Flags: needinfo?(anatal)
The patch was split in two and here is only the new code while bug 1051146 have the build system patch.
Attachment #8470537 - Attachment is obsolete: true
Attachment #8474364 - Flags: review?(bugs)
Comment on attachment 8474364 [details] [diff] [review]
This patch creates a SpeechRecognitionService using PocketSphinx as a backend decoder to Web Speech API

ggp, could you take the first round of reviews here.


(random comments, remove printfs, new code should use gecko coding style
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style)
Attachment #8474364 - Flags: review?(bugs) → review?(ggoncalves)
Duplicate of this bug: 1051118
No longer depends on: 1051146
Depends on: 967896
Comment on attachment 8474364 [details] [diff] [review]
This patch creates a SpeechRecognitionService using PocketSphinx as a backend decoder to Web Speech API

Andre and I agreed today on splitting the patches for ease of reviewing -- basically one patch for the scaffolding changes that had to be made in the current code, and another patch adding the service itself.

Let's cancel the review for now and have another look when the patches are split, but in the meantime, Andre, please make sure you address :smaug's comments about coding style and removing the printf's.
Attachment #8474364 - Flags: review?(ggoncalves)
Assignee: nobody → anatal
This patch covers only the include of PocketSphinxSpeechRecognitionService.
Attachment #8474364 - Attachment is obsolete: true
Attachment #8481643 - Flags: review?(ggoncalves)
Attachment #8481643 - Attachment is obsolete: true
Attachment #8481643 - Flags: review?(ggoncalves)
Attachment #8481705 - Flags: review?(ggoncalves)
Blocks: 1051146
No longer blocks: 1049931
Depends on: 1060659
Blocks: 1051118
No longer blocks: 1051146
No longer depends on: 1060659
Blocks: 1060659
No longer blocks: 1051118
No longer depends on: 1051607
Blocks: 1049931
No longer blocks: 1060659
No longer depends on: 1051554
Attachment #8481705 - Attachment is obsolete: true
Attachment #8481705 - Flags: review?(ggoncalves)
Attachment #8489735 - Flags: review?(ggoncalves)
Summary: Create SpeechRecognitionService for Pocketsphinx and integrate with the decoder to enable offline WebSpeechAPI → Development of PocketSphinxSpeechRecognitionService to use pocketsphinx and enable Webspeech API
Blocks: 1067689
No longer blocks: 1067689
Blocks: 1067689
No longer blocks: 1049931
No longer depends on: 967896
No longer depends on: 1051606
No longer depends on: 1051130
Attachment #8489735 - Attachment is obsolete: true
Attachment #8489735 - Flags: review?(ggoncalves)
Attachment #8489750 - Flags: review?(ggoncalves)
Attachment #8489750 - Attachment is obsolete: true
Attachment #8489750 - Flags: review?(ggoncalves)
Attachment #8493507 - Flags: review?(ggoncalves)
Depends on: 1051146
No longer depends on: 1051146
Depends on: 1051146
Attachment #8493507 - Attachment is obsolete: true
Attachment #8493507 - Flags: review?(ggoncalves)
Attachment #8496617 - Flags: review?(ggoncalves)
If you ggp are busy with other stuff, feel free to move this to my review queue.
(though, can't promise very fast review.)
(In reply to Olli Pettay [:smaug] from comment #26)
> If you ggp are busy with other stuff, feel free to move this to my review
> queue.
> (though, can't promise very fast review.)

I'll have time to do a first pass tomorrow. As with the other patches, I'll make sure all the obvious issues and style nits are fixed before moving it to you :)
Comment on attachment 8496617 [details] [diff] [review]
Development of PocketSphinxSpeechRecognitionService to use pocketsphinx and enable Webspeech API

Review of attachment 8496617 [details] [diff] [review]:
-----------------------------------------------------------------

I've only taken a cursory look at PocketSphinxSpeechRecognitionService.{h,cpp} to find style issues,
but the other changes look good.

The one worrying thing I've found is the usage of mGram in SpeechGrammarList to compensate for the
fact that we haven't implemented SpeechGrammarList and SpeechGrammar yet. As much as I hate do delay
this patch, I think we should file a separate bug and implement these first. It shouldn't be too hard,
as a SpeechGrammarList basically wraps an array of SpeechGrammar objects.

Andre, would you be available for working on SpeechGrammar/SpeechGrammarList?

::: content/media/webspeech/recognition/PocketSphinxSpeechRecognitionService.cpp
@@ +94,5 @@
> +    rv = ps_start_utt(mPs, "goforward");
> +    rv = ps_process_raw(mPs, &mAudiovector[0], mAudiovector.size(), FALSE, FALSE);
> +
> +    rv = ps_end_utt(mPs);
> +    if (rv >= 0)

if (rv >= 0) {, here and in the rest of the file.

@@ +132,5 @@
> +    nsString aStringDictPath; // dict folder
> +
> +    NS_GetSpecialDirectory(NS_GRE_DIR, getter_AddRefs(tmpFile));
> +    #if  defined(XP_WIN) // for some reason, on windows NS_GRE_DIR is not bin root, but bin/browser
> +    tmpFile->AppendRelativePath(NS_LITERAL_STRING("..") );    

Extra whitespace at the end of the line. Please watch out for other occurrences of this in the same method.

::: content/media/webspeech/recognition/PocketSphinxSpeechRecognitionService.h
@@ +45,5 @@
> +  cmd_ln_t *config;
> +  nsCOMPtr<nsIThread> decoderthread;
> +  bool decodersane; // flag to verify if decoder is sane
> +  bool grammarsane; // flag to verify if grammar is sane
> +  std::vector<int16_t> audiovector;

Please fix these declarations according to our coding style. For example,

ps_decoder_t* mPSHandle;
nsCOMPtr<nsIThread> mDecoderThread;

and so on.
Also, what does it mean for a decoder or a grammar to be sane? Please rename the
member variables if you can make the meaning more explicit, or explain in a comment.

::: content/media/webspeech/recognition/SpeechGrammarList.cpp
@@ +89,4 @@
>                                   const Optional<float>& aWeight,
>                                   ErrorResult& aRv)
>  {
> +  mGram = ToNewUTF8String(aString);

Hmm, I feel like we should finish the implementation of SpeechGrammar and SpeechGrammarList instead of resorting to stuff like this.
If we had them properly implemented, then inside ValidateAndSetGrammarList you could just call SpeechGrammarList::Item to get a SpeechGrammar, then call SpeechGrammar::GetSrc to get the source as a string.

::: content/media/webspeech/recognition/SpeechGrammarList.h
@@ +52,4 @@
>  
>    nsCOMPtr<nsISpeechRecognitionService> mRecognitionService;
>  
> +   const char * mGram;

I guess you don't need this; please see my comment on SpeechGrammarList.cpp. Please remember to also remove the blank line you added.

::: content/media/webspeech/recognition/SpeechRecognition.cpp
@@ +21,5 @@
>  #include "nsIObserverService.h"
>  #include "nsServiceManagerUtils.h"
>  
> +#include "nsWeakPtr.h"
> +#include "nsIWeakReferenceUtils.h"

I guess you don't need this, correct? Please remember to also remove the blank line you added.

::: content/media/webspeech/recognition/moz.build
@@ -19,4 @@
>      'SpeechRecognitionResult.h',
>      'SpeechRecognitionResultList.h',
>      'SpeechStreamListener.h',
> -    'test/FakeSpeechRecognitionService.h',

No need to remove this.

@@ -33,4 @@
>      'SpeechRecognitionResult.cpp',
>      'SpeechRecognitionResultList.cpp',
>      'SpeechStreamListener.cpp',
> -    'test/FakeSpeechRecognitionService.cpp',

No need to remove this either.

@@ +38,5 @@
>  
>  LOCAL_INCLUDES += [
>      '/dom/base',
> +    '/media/pocketsphinx',
> +    '/media/sphinxbase',    

Please remove the extra whitespace at the end of the line.

::: layout/build/nsLayoutModule.cpp
@@ +96,4 @@
>  #include "mozilla/Services.h"
>  
>  #ifdef MOZ_WEBSPEECH
> +#include "mozilla/dom/PocketSphinxSpeechRecognitionService.h"

Please don't replace the lines referring to FakeSpeechRecognitionService, but rather add yours after them. This goes for all changes in this file.
Attachment #8496617 - Flags: review?(ggoncalves)
Comment on attachment 8496617 [details] [diff] [review]
Development of PocketSphinxSpeechRecognitionService to use pocketsphinx and enable Webspeech API

Review of attachment 8496617 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/media/webspeech/recognition/SpeechGrammarList.cpp
@@ +89,4 @@
>                                   const Optional<float>& aWeight,
>                                   ErrorResult& aRv)
>  {
> +  mGram = ToNewUTF8String(aString);

Makes sense, but well, actually w3c specification it is not well suited for grammar recogntion. 

In this case for example, SpeechGrammar's src attribute does not expect the grammar string, but an URI that points to that grammar.

So you confirm that it is OK to fill this attribute with the grammar string instead the URI? 

https://dvcs.w3.org/hg/speech-api/raw-file/tip/speechapi.html#speechreco-speechgrammar

src attribute
    The required src attribute is the URI for the grammar. Note some services may support builtin grammars that can be specified using a builtin URI scheme.
Flags: needinfo?(ggoncalves)
(In reply to Andre Natal from comment #29)
> Note some services may support builtin grammars that can be specified using a builtin
> URI scheme.

I think this snippet answers your question, actually. We should be free to define builtin URI schemes referrring to the PocketSphinx grammars we support.

Note that implementing SpeechGrammar/SpeechGrammarList doesn't really depend on specifying such schemes -- a SpeechGrammar will just hold an arbitrary string as its src, and it's up to the recognition service to interpret that string. So once we have these objects implemented, we can bikeshed on this bug about what URI scheme we can use to expose the grammars we'll ship (and we probably want to look at what other implementations are doing in this regard, too).
Flags: needinfo?(ggoncalves)
(In reply to Guilherme Gonçalves [:ggp] from comment #30)
> (In reply to Andre Natal from comment #29)
> > Note some services may support builtin grammars that can be specified using a builtin
> > URI scheme.
> 
> I think this snippet answers your question, actually. We should be free to
> define builtin URI schemes referrring to the PocketSphinx grammars we
> support.
> 
> Note that implementing SpeechGrammar/SpeechGrammarList doesn't really depend
> on specifying such schemes -- a SpeechGrammar will just hold an arbitrary
> string as its src, and it's up to the recognition service to interpret that
> string. So once we have these objects implemented, we can bikeshed on this
> bug about what URI scheme we can use to expose the grammars we'll ship (and
> we probably want to look at what other implementations are doing in this
> regard, too).

Thanks. 

I am working on it.
Attachment #8496617 - Attachment is obsolete: true
Attachment #8510685 - Flags: review?(ggoncalves)
Attachment #8510685 - Flags: review?(bugs)
(In reply to Guilherme Gonçalves [:ggp] from comment #30)
> (In reply to Andre Natal from comment #29)
> > Note some services may support builtin grammars that can be specified using a builtin
> > URI scheme.
> 
> I think this snippet answers your question, actually. We should be free to
> define builtin URI schemes referrring to the PocketSphinx grammars we
> support.
> 
> Note that implementing SpeechGrammar/SpeechGrammarList doesn't really depend
> on specifying such schemes -- a SpeechGrammar will just hold an arbitrary
> string as its src, and it's up to the recognition service to interpret that
> string. So once we have these objects implemented, we can bikeshed on this
> bug about what URI scheme we can use to expose the grammars we'll ship (and
> we probably want to look at what other implementations are doing in this
> regard, too).

I did the fixed that you asked for and created another bug about SpeechGrammar/SpeechGrammarList approach and also submit the patch
Comment on attachment 8510685 [details] [diff] [review]
Development of PocketSphinxSpeechRecognitionService to use pocketsphinx and enable Webspeech API

As I said on IRC, we shouldn't enable pocketsphinx just if MOZ_WEBSPEECH is set.
We need something similar what pico does for tts.
And perhaps put PocketSphinx related files to 
content/media/webspeech/recognition/pocketsphinx/

>+  DecodeResultTask( const nsString& hypstring , WeakPtr<dom::SpeechRecognition> recognition)
aHypstring, aRecognition...
Please use aFoo naming for arguments, same issue also elsewhere in the patch


>+  NS_IMETHOD Run() {
{ should be in the next line

>+public:
>+  DecodeTask(WeakPtr<dom::SpeechRecognition> recogntion , std::vector<int16_t>   audiovector , ps_decoder_t * ps)
extra space before *

>+  NS_IMETHOD Run() {
{ should be on its own line

>+
>+
>+    char const *hyp, *uttid;

* goes with the type, 
char const* hyp;
char const* uttid;

(But do you really want char const* or const char* ?)



>+    int rv;
>+    int32 score;
>+    nsString hypoValue;
>+
>+    rv = ps_start_utt(mPs, "goforward");
>+    rv = ps_process_raw(mPs, &mAudiovector[0], mAudiovector.size(), FALSE, FALSE);
>+
>+    rv = ps_end_utt(mPs);
>+    if (rv >= 0) {
I don't understand rv value usage here. You assign to rv several times, but check the value only once.

>+private:
>+  WeakPtr<dom::SpeechRecognition> mRecognition;
>+  ps_decoder_t * mPs;
Extra space before*.
mPs needs also comment about what keeps the object alive.

>+  std::vector<int16_t>  mAudiovector;
Any particular reason to use std::vector here?
Why not nsTArray<int16_t> and call SetCapacity before the mAudioVector.push_back calls

>+  PocketSphinxSpeechRecognitionService::PocketSphinxSpeechRecognitionService()
>+  {
>+    mSpeexState = nullptr;
>+
>+    // get root folder
>+    nsCOMPtr<nsIFile> tmpFile;
>+    nsString aStringAMPath; // am folder
>+    nsString aStringDictPath; // dict folder
nsAutoString for local string variables.


>+    NS_GetSpecialDirectory(NS_GRE_DIR, getter_AddRefs(tmpFile));
>+    #if  defined(XP_WIN) // for some reason, on windows NS_GRE_DIR is not bin root, but bin/browser
>+    tmpFile->AppendRelativePath(NS_LITERAL_STRING("..") );
>+    #endif
>+    tmpFile->AppendRelativePath(NS_LITERAL_STRING("models") );
>+    tmpFile->AppendRelativePath(NS_LITERAL_STRING("en-us-semi") );
>+    tmpFile->GetPath(aStringAMPath);
>+
>+    NS_GetSpecialDirectory(NS_GRE_DIR, getter_AddRefs(tmpFile));
>+    #if  defined(XP_WIN)  // for some reason, on windows NS_GRE_DIR is not bin root, but bin/browser
>+    tmpFile->AppendRelativePath(NS_LITERAL_STRING("..") );
Does something else in the platform need to use this kind of hack?


>+     if (mPSConfig == NULL) {
nullptr

>+  PocketSphinxSpeechRecognitionService::~PocketSphinxSpeechRecognitionService()
>+  {
>+    if (mPSConfig)
>+      free(mPSConfig);
Always {} with if
if (mPSConfig) {
  free(mPSConfig);
}

>+  PocketSphinxSpeechRecognitionService::ProcessAudioSegment(AudioSegment* aAudioSegment, int32_t aSampleRate)
>+  {
>+    if (!mSpeexState) {
>+        mSpeexState = speex_resampler_init(1,  aSampleRate, 16000,  SPEEX_RESAMPLER_QUALITY_MAX  ,  nullptr);
2 spaces for indentation. Same issue also elsewhere.
And you have some odd extra spaces between the parameters

>+    }
>+    else {
      } else {
Attachment #8510685 - Flags: review?(bugs) → review-
Attachment #8510685 - Flags: review?(ggoncalves)
Whiteboard: [webspeechapi]
Assignee: anatal → kdavis
Part 1 of 2:  Patch that introduces WebSpeech API implementation

This is the part 1 of 2 for this bug.

This patch introduces the Pocketsphinx/Sphinxbase implementation of the WebSpeech API. This implementation depends upon all of the patches for Bug 1051146.

The try for this patch is running here https://treeherder.mozilla.org/#/jobs?repo=try&revision=d932008c76cd
Attachment #8606749 - Flags: review?(bugs)
Part 2 of 2:  Patch that turns off the WebSpeech API implementation using flags

This is the part 2 of 2 for this bug.

This patch turns off the WebSpeech API implementation using flags. This implementation depends upon all of the patches for Bug 1051146.

The try for this patch is running here https://treeherder.mozilla.org/#/jobs?repo=try&revision=cae7e9ed4c66
Attachment #8510685 - Attachment is obsolete: true
Attachment #8606767 - Flags: review?(bugs)
The patches above will have to be revised when the flags suggested in Bug 1051146, comment 50 are added.
Part 2 of 2:  Patch that turns off the WebSpeech API implementation using flags

This is the part 2 of 2 for this bug.

This patch turns off the WebSpeech API implementation using flags. This patch fixes a typo in the previous patch. Also, this implementation depends upon all of the patches for Bug 1051146.

The try for this patch is running here https://treeherder.mozilla.org/#/jobs?repo=try&revision=ffea7548ae17
Attachment #8606767 - Attachment is obsolete: true
Attachment #8606767 - Flags: review?(bugs)
Attachment #8606916 - Flags: review?(bugs)
Part 2 of 2:  Patch that turns off the WebSpeech API implementation using various flags

This is the part 2 of 2 for this bug.

This patch turns off the WebSpeech API implementation using various flags. In addition this patch fixes some tests that did not respect the MOZ_WEBSPEECH flag. Also, this patch depends upon all of the patches for Bug 1051146.

The try for this patch is running here https://treeherder.mozilla.org/#/jobs?repo=try&revision=6186a5fa2359
Attachment #8606916 - Attachment is obsolete: true
Attachment #8606916 - Flags: review?(bugs)
Attachment #8607113 - Flags: review?(bugs)
Comment on attachment 8606749 [details] [diff] [review]
Part 1 of 2:  Patch that introduces WebSpeech API implementation

So this has lots of unrelated changes to Synthesis, and that all partially makes
the patch size massive.
Please include only the part which implements the recognition service.
(feel free to file a separate bug to fix whatever issues you see in Synthesis code)

Btw, since this all makes recognizer run on child process
(I like that approach, safer), do we need to worry about sandboxing?
*sphinx tries to open files, and if process is sandboxed, can it?
Attachment #8606749 - Flags: review?(bugs) → review-
Comment on attachment 8607113 [details] [diff] [review]
Part 2 of 2:  Patch that turns off the WebSpeech API implementation using various flags

I think we should enable the media.webspeech.recognition.enable/media.webspeech.service.default
only on nightlies first.
there is  NIGHTLY_BUILD flag for it.
That way we don't accidentally end up exposing still in-development code.

test_interfaces.html has become more strict and tries to test what we actually ship, so trying to live without pref() checks.
Wouldn't nightly: true, b2g: true work?
Attachment #8607113 - Flags: review?(bugs) → review-
(In reply to Olli Pettay [:smaug] from comment #40)
> Comment on attachment 8606749 [details] [diff] [review]
> Part 1 of 2:  Patch that introduces WebSpeech API implementation
> 
> So this has lots of unrelated changes to Synthesis, and that all partially
> makes
> the patch size massive.
> Please include only the part which implements the recognition service.
> (feel free to file a separate bug to fix whatever issues you see in
> Synthesis code)

OK. I will not submit my fixes for the Synthesis code.

I have filed bugs against most of the issues I have fixed in the
Synthesis code, but will not submit. Here are the associated bugs:

* Bug 1167538
* Bug 1167539
* Bug 1167540
* Bug 1167541
* Bug 1167542
* Bug 1167543
Part 1 of 2:  Patch that introduces WebSpeech API implementation

This is the part 1 of 2 for this bug.

This patch introduces the Pocketsphinx/Sphinxbase implementation of the WebSpeech API. This implementation depends upon all of the patches for Bug 1051146.

The try for this patch is running here https://treeherder.mozilla.org/#/jobs?repo=try&revision=880674e0a840
Attachment #8606749 - Attachment is obsolete: true
Attachment #8607113 - Attachment is obsolete: true
Attachment #8610065 - Flags: review?(bugs)
Part 2 of 2:  Patch that turns off the WebSpeech API implementation using flags

This is the part 2 of 2 for this bug.

This patch turns off the WebSpeech API implementation using flags. This implementation depends upon all of the patches for Bug 1051146.

The try for this patch is running here https://treeherder.mozilla.org/#/jobs?repo=try&revision=37f1f8a35dfe
Attachment #8610066 - Flags: review?(bugs)
Comment on attachment 8610066 [details] [diff] [review]
Part 2 of 2:  Patch that turns off the WebSpeech API implementation using flags

ah, I guess we can do it this way.
One thing to check is that whether lots of code makes bug 1073399 kind of cases worse.
Attachment #8610066 - Flags: review?(bugs) → review+
Comment on attachment 8610065 [details] [diff] [review]
Part 1 of 2:  Patch that introduces WebSpeech API implementation

>+  mPSConfig = cmd_ln_init(NULL, ps_args(), TRUE, "-hmm",
>+                          ToNewUTF8String(aStringAMPath), // acoustic model
>+                          "-dict", ToNewUTF8String(aStringDictPath), NULL);
>+  if (mPSConfig == NULL) {
nullptr, not NULL. Also elsewhere.


>+PocketSphinxSpeechRecognitionService::~PocketSphinxSpeechRecognitionService()
>+{
>+  if (mPSConfig)
>+    free(mPSConfig);
>+  if (mPSHandle)
>+    free(mPSHandle);
Always {} with if. Also elsewhere

>+
>+    NS_ConvertUTF16toUTF8 utf8Str(grammar);
>+    const nsACString& utf8AStr = utf8Str;
>+    int result = ps_set_jsgf_string(mPSHandle, "name",
>+                                    PromiseFlatCString(utf8AStr).get());
You could just do 
NS_ConvertUTF16toUTF8(grammar).get()

>+NS_IMETHODIMP
>+PocketSphinxSpeechRecognitionService::Observe(nsISupports* aSubject,
>+                                              const char* aTopic,
>+                                              const char16_t* aData)
>+{
>+  MOZ_ASSERT(mRecognition->mTestConfig.mFakeRecognitionService,
>+             "Got request to fake recognition service event, "
>+             "but " TEST_PREFERENCE_FAKE_RECOGNITION_SERVICE " is not set");
>+
>+  if (!strcmp(aTopic, SPEECH_RECOGNITION_TEST_END_TOPIC)) {
>+    nsCOMPtr<nsIObserverService> obs = services::GetObserverService();
>+    obs->RemoveObserver(this, SPEECH_RECOGNITION_TEST_EVENT_REQUEST_TOPIC);
>+    obs->RemoveObserver(this, SPEECH_RECOGNITION_TEST_END_TOPIC);
>+
>+    return NS_OK;
>+  }
>+
>+  const nsDependentString eventName = nsDependentString(aData);
>+
>+  if (eventName.EqualsLiteral("EVENT_RECOGNITIONSERVICE_ERROR")) {
>+    mRecognition->DispatchError(
>+        SpeechRecognition::EVENT_RECOGNITIONSERVICE_ERROR,
>+        SpeechRecognitionErrorCode::Network, // TODO different codes?
>+        NS_LITERAL_STRING("RECOGNITIONSERVICE_ERROR test event"));
>+
>+  } else if (eventName.EqualsLiteral("EVENT_RECOGNITIONSERVICE_FINAL_RESULT")) {
>+    nsRefPtr<SpeechEvent> event = new SpeechEvent(
>+        mRecognition, SpeechRecognition::EVENT_RECOGNITIONSERVICE_FINAL_RESULT);
>+
>+    event->mRecognitionResultList = BuildMockResultList();
>+    NS_DispatchToMainThread(event);
>+  }
>+
>+  return NS_OK;
>+}
>+
>+SpeechRecognitionResultList*
>+PocketSphinxSpeechRecognitionService::BuildMockResultList()
>+{
>+  SpeechRecognitionResultList* resultList =
>+      new SpeechRecognitionResultList(mRecognition);
>+  SpeechRecognitionResult* result = new SpeechRecognitionResult(mRecognition);
>+  SpeechRecognitionAlternative* alternative =
>+      new SpeechRecognitionAlternative(mRecognition);
>+
>+  alternative->mTranscript = NS_LITERAL_STRING("Mock final result");
>+  alternative->mConfidence = 0.0f;
>+
>+  result->mItems.AppendElement(alternative);
>+  resultList->mItems.AppendElement(result);
>+
>+  return resultList;
>+}
Please use 4 spaces for indentation.
But I'm now here lost with the "mock" setup. Why BuildMockResultList?


>+  /**
>+   * Builds a mock SpeechRecognitionResultList
>+   */
>+  dom::SpeechRecognitionResultList *BuildMockResultList();
* goes with the type

>+
>+  /** Speex state */
>+  SpeexResamplerState *mSpeexState;
ditto. SpeexResamplerState* mSpeexState;

>+
>+  /** Pocksphix decoder */
>+  ps_decoder_t *mPSHandle;
ditto

>+
>+  /** Sphinxbase parsed command line arguments */
>+  cmd_ln_t *mPSConfig;

ditto
>+  /** Flag to verify if decoder was created */
>+  bool ISDecoderCreated;
>+
>+  /** Flag to verify if grammar was compiled */
>+  bool ISGrammarCompiled;

member variables should be name mFoo

>+
>+  /** Audio data */
>+  std::vector<int16_t> mAudioVector;
I'd prefer using nsTArray, of if we know some expected size for the array, perhaps even nsAutoTArray (that might reduce memory churn quite a bit).



>+++ b/dom/media/webspeech/recognition/SpeechGrammar.h
>@@ -49,6 +49,8 @@ private:
>   ~SpeechGrammar();
> 
>   nsCOMPtr<nsISupports> mParent;
>+
>+  nsString mGram;
Could you call this mSrc;




>+++ b/dom/media/webspeech/recognition/SpeechGrammarList.h
>@@ -56,6 +56,8 @@ private:
>   ~SpeechGrammarList();
> 
>   nsCOMPtr<nsISupports> mParent;
>+
>+  nsTArray<nsRefPtr<SpeechGrammar> > mItems;
drop space between > and >. These days all the compilers support >>, in the old days
> > had to be used.



>@@ -535,7 +535,9 @@ SpeechRecognition::StartRecording(DOMMediaStream* aDOMStream)
>   // doesn't get Destroy()'ed
>   mDOMStream = aDOMStream;
> 
>-  NS_ENSURE_STATE(mDOMStream->GetStream());
>+  if (NS_WARN_IF(!mDOMStream->GetStream())) {
>+    return NS_ERROR_UNEXPECTED;
>+  }
>   mSpeechListener = new SpeechStreamListener(this);
>   mDOMStream->GetStream()->AddListener(mSpeechListener);
> 
>@@ -698,11 +700,15 @@ SpeechRecognition::Start(const Optional<NonNull<DOMMediaStream>>& aStream, Error
>   }
> 
>   mRecognitionService = GetSpeechRecognitionService();
>-  NS_ENSURE_TRUE_VOID(mRecognitionService);
>+  if (NS_WARN_IF(!mRecognitionService)) {
>+    return;
>+  }
> 
>   nsresult rv;
>   rv = mRecognitionService->Initialize(this);
>-  NS_ENSURE_SUCCESS_VOID(rv);
>+  if (NS_WARN_IF(NS_FAILED(rv))) {
>+    return;
>+  }
I prefer use of NS_ENSURE_* macros over if (NS_WARN_IF())




> SpeechRecognitionAlternative::SpeechRecognitionAlternative(SpeechRecognition* aParent)
>-  : mTranscript(NS_LITERAL_STRING(""))
>+  : mTranscript(EmptyString())
The right fix would be to drop explicit construction here. Strings are empty by default





> interface SpeechRecognitionResult {
>     readonly attribute unsigned long length;
>     getter SpeechRecognitionAlternative item(unsigned long index);
>-    readonly attribute boolean final;
>+    readonly attribute boolean isFinal;
Why this change?
The spec has 
    interface SpeechRecognitionResult {
        readonly attribute unsigned long length;
        getter SpeechRecognitionAlternative item(in unsigned long index);
        readonly attribute boolean final;
    };
please undo, or explain.
Attachment #8610065 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #46)
> Comment on attachment 8610065 [details] [diff] [review]
> Part 1 of 2:  Patch that introduces WebSpeech API implementation
> 
> >+  mPSConfig = cmd_ln_init(NULL, ps_args(), TRUE, "-hmm",
> >+                          ToNewUTF8String(aStringAMPath), // acoustic model
> >+                          "-dict", ToNewUTF8String(aStringDictPath), NULL);
> >+  if (mPSConfig == NULL) {
> nullptr, not NULL. Also elsewhere.


OK. I'll make these changes.
(In reply to Olli Pettay [:smaug] from comment #46)
> Comment on attachment 8610065 [details] [diff] [review]
> Part 1 of 2:  Patch that introduces WebSpeech API implementation
> 
> >+PocketSphinxSpeechRecognitionService::~PocketSphinxSpeechRecognitionService()
> >+{
> >+  if (mPSConfig)
> >+    free(mPSConfig);
> >+  if (mPSHandle)
> >+    free(mPSHandle);
> Always {} with if. Also elsewhere

OK. I'll make these changes.
(In reply to Olli Pettay [:smaug] from comment #46)
> Comment on attachment 8610065 [details] [diff] [review]
> Part 1 of 2:  Patch that introduces WebSpeech API implementation
> 
> 
> >+
> >+    NS_ConvertUTF16toUTF8 utf8Str(grammar);
> >+    const nsACString& utf8AStr = utf8Str;
> >+    int result = ps_set_jsgf_string(mPSHandle, "name",
> >+                                    PromiseFlatCString(utf8AStr).get());
> You could just do 
> NS_ConvertUTF16toUTF8(grammar).get()

OK. I can change this.
(In reply to Olli Pettay [:smaug] from comment #46)
> Comment on attachment 8610065 [details] [diff] [review]
> Part 1 of 2:  Patch that introduces WebSpeech API implementation
> >+NS_IMETHODIMP
> >+PocketSphinxSpeechRecognitionService::Observe(nsISupports* aSubject,
> >+                                              const char* aTopic,
> >+                                              const char16_t* aData)
> >+{
> >+  MOZ_ASSERT(mRecognition->mTestConfig.mFakeRecognitionService,
> >+             "Got request to fake recognition service event, "
> >+             "but " TEST_PREFERENCE_FAKE_RECOGNITION_SERVICE " is not set");
> >+
> >+  if (!strcmp(aTopic, SPEECH_RECOGNITION_TEST_END_TOPIC)) {
> >+    nsCOMPtr<nsIObserverService> obs = services::GetObserverService();
> >+    obs->RemoveObserver(this, SPEECH_RECOGNITION_TEST_EVENT_REQUEST_TOPIC);
> >+    obs->RemoveObserver(this, SPEECH_RECOGNITION_TEST_END_TOPIC);
> >+
> >+    return NS_OK;
> >+  }
> >+
> >+  const nsDependentString eventName = nsDependentString(aData);
> >+
> >+  if (eventName.EqualsLiteral("EVENT_RECOGNITIONSERVICE_ERROR")) {
> >+    mRecognition->DispatchError(
> >+        SpeechRecognition::EVENT_RECOGNITIONSERVICE_ERROR,
> >+        SpeechRecognitionErrorCode::Network, // TODO different codes?
> >+        NS_LITERAL_STRING("RECOGNITIONSERVICE_ERROR test event"));
> >+
> >+  } else if (eventName.EqualsLiteral("EVENT_RECOGNITIONSERVICE_FINAL_RESULT")) {
> >+    nsRefPtr<SpeechEvent> event = new SpeechEvent(
> >+        mRecognition, SpeechRecognition::EVENT_RECOGNITIONSERVICE_FINAL_RESULT);
> >+
> >+    event->mRecognitionResultList = BuildMockResultList();
> >+    NS_DispatchToMainThread(event);
> >+  }
> >+
> >+  return NS_OK;
> >+}
> >+
> >+SpeechRecognitionResultList*
> >+PocketSphinxSpeechRecognitionService::BuildMockResultList()
> >+{
> >+  SpeechRecognitionResultList* resultList =
> >+      new SpeechRecognitionResultList(mRecognition);
> >+  SpeechRecognitionResult* result = new SpeechRecognitionResult(mRecognition);
> >+  SpeechRecognitionAlternative* alternative =
> >+      new SpeechRecognitionAlternative(mRecognition);
> >+
> >+  alternative->mTranscript = NS_LITERAL_STRING("Mock final result");
> >+  alternative->mConfidence = 0.0f;
> >+
> >+  result->mItems.AppendElement(alternative);
> >+  resultList->mItems.AppendElement(result);
> >+
> >+  return resultList;
> >+}
> Please use 4 spaces for indentation.

OK. I'll change this.
(In reply to Olli Pettay [:smaug] from comment #46)
> Comment on attachment 8610065 [details] [diff] [review]
> Part 1 of 2:  Patch that introduces WebSpeech API implementation
>  
> >+  /**
> >+   * Builds a mock SpeechRecognitionResultList
> >+   */
> >+  dom::SpeechRecognitionResultList *BuildMockResultList();
> * goes with the type

OK. I'll change here and other places too.
(In reply to Olli Pettay [:smaug] from comment #46)
> Comment on attachment 8610065 [details] [diff] [review]
> Part 1 of 2:  Patch that introduces WebSpeech API implementation
> 
> >+  /** Flag to verify if decoder was created */
> >+  bool ISDecoderCreated;
> >+
> >+  /** Flag to verify if grammar was compiled */
> >+  bool ISGrammarCompiled;
> 
> member variables should be name mFoo
> 

Ok will change this.
(In reply to Olli Pettay [:smaug] from comment #46)
> Comment on attachment 8610065 [details] [diff] [review]
> Part 1 of 2:  Patch that introduces WebSpeech API implementation
> 
> >+
> >+  /** Audio data */
> >+  std::vector<int16_t> mAudioVector;
> I'd prefer using nsTArray, of if we know some expected size for the array,
> perhaps even nsAutoTArray (that might reduce memory churn quite a bit).
> 

The size of this is not known at compile time. In particular, its size is
dependent upon the size of the AudioSegment which isn't known at compile
time as it's dependent upon what the user says. (See the implementation of
the method PocketSphinxSpeechRecognitionService::ProcessAudioSegment).

That being said, I don't see the advantage. Maybe there is something I am
missing?
(In reply to Olli Pettay [:smaug] from comment #46)
> Comment on attachment 8610065 [details] [diff] [review]
> Part 1 of 2:  Patch that introduces WebSpeech API implementation
> 
> >+++ b/dom/media/webspeech/recognition/SpeechGrammar.h
> >@@ -49,6 +49,8 @@ private:
> >   ~SpeechGrammar();
> > 
> >   nsCOMPtr<nsISupports> mParent;
> >+
> >+  nsString mGram;
> Could you call this mSrc;

OK. I'll make this change.
(In reply to Olli Pettay [:smaug] from comment #46)
> Comment on attachment 8610065 [details] [diff] [review]
> Part 1 of 2:  Patch that introduces WebSpeech API implementation
> 
> >+++ b/dom/media/webspeech/recognition/SpeechGrammarList.h
> >@@ -56,6 +56,8 @@ private:
> >   ~SpeechGrammarList();
> > 
> >   nsCOMPtr<nsISupports> mParent;
> >+
> >+  nsTArray<nsRefPtr<SpeechGrammar> > mItems;
> drop space between > and >. These days all the compilers support >>, in the
> old days
> > > had to be used.

OK. I'll make this change.
(In reply to Olli Pettay [:smaug] from comment #46)
> Comment on attachment 8610065 [details] [diff] [review]
> Part 1 of 2:  Patch that introduces WebSpeech API implementation
> 
> >@@ -535,7 +535,9 @@ SpeechRecognition::StartRecording(DOMMediaStream* aDOMStream)
> >   // doesn't get Destroy()'ed
> >   mDOMStream = aDOMStream;
> > 
> >-  NS_ENSURE_STATE(mDOMStream->GetStream());
> >+  if (NS_WARN_IF(!mDOMStream->GetStream())) {
> >+    return NS_ERROR_UNEXPECTED;
> >+  }
> >   mSpeechListener = new SpeechStreamListener(this);
> >   mDOMStream->GetStream()->AddListener(mSpeechListener);
> > 
> >@@ -698,11 +700,15 @@ SpeechRecognition::Start(const Optional<NonNull<DOMMediaStream>>& aStream, Error
> >   }
> > 
> >   mRecognitionService = GetSpeechRecognitionService();
> >-  NS_ENSURE_TRUE_VOID(mRecognitionService);
> >+  if (NS_WARN_IF(!mRecognitionService)) {
> >+    return;
> >+  }
> > 
> >   nsresult rv;
> >   rv = mRecognitionService->Initialize(this);
> >-  NS_ENSURE_SUCCESS_VOID(rv);
> >+  if (NS_WARN_IF(NS_FAILED(rv))) {
> >+    return;
> >+  }
> I prefer use of NS_ENSURE_* macros over if (NS_WARN_IF())
> 

The Mozilla coding standard[1] mandates use of NS_WARN_IF in new code. I quote

"Previously the NS_ENSURE_* macros were used for this purpose, but those macros
hide return statements and should not be used in new code."

So, I'd like to keep it as it is.

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Use_the_nice_macros
(In reply to Olli Pettay [:smaug] from comment #46)
> Comment on attachment 8610065 [details] [diff] [review]
> Part 1 of 2:  Patch that introduces WebSpeech API implementation
> 
> >+  mPSConfig = cmd_ln_init(NULL, ps_args(), TRUE, "-hmm",
> >+                          ToNewUTF8String(aStringAMPath), // acoustic model
> >+                          "-dict", ToNewUTF8String(aStringDictPath), NULL);
> >+  if (mPSConfig == NULL) {
> nullptr, not NULL. Also elsewhere.
> 
> 
> >+PocketSphinxSpeechRecognitionService::~PocketSphinxSpeechRecognitionService()
> >+{
> >+  if (mPSConfig)
> >+    free(mPSConfig);
> >+  if (mPSHandle)
> >+    free(mPSHandle);
> Always {} with if. Also elsewhere
> 
> >+
> >+    NS_ConvertUTF16toUTF8 utf8Str(grammar);
> >+    const nsACString& utf8AStr = utf8Str;
> >+    int result = ps_set_jsgf_string(mPSHandle, "name",
> >+                                    PromiseFlatCString(utf8AStr).get());
> You could just do 
> NS_ConvertUTF16toUTF8(grammar).get()
> 
> >+NS_IMETHODIMP
> >+PocketSphinxSpeechRecognitionService::Observe(nsISupports* aSubject,
> >+                                              const char* aTopic,
> >+                                              const char16_t* aData)
> >+{
> >+  MOZ_ASSERT(mRecognition->mTestConfig.mFakeRecognitionService,
> >+             "Got request to fake recognition service event, "
> >+             "but " TEST_PREFERENCE_FAKE_RECOGNITION_SERVICE " is not set");
> >+
> >+  if (!strcmp(aTopic, SPEECH_RECOGNITION_TEST_END_TOPIC)) {
> >+    nsCOMPtr<nsIObserverService> obs = services::GetObserverService();
> >+    obs->RemoveObserver(this, SPEECH_RECOGNITION_TEST_EVENT_REQUEST_TOPIC);
> >+    obs->RemoveObserver(this, SPEECH_RECOGNITION_TEST_END_TOPIC);
> >+
> >+    return NS_OK;
> >+  }
> >+
> >+  const nsDependentString eventName = nsDependentString(aData);
> >+
> >+  if (eventName.EqualsLiteral("EVENT_RECOGNITIONSERVICE_ERROR")) {
> >+    mRecognition->DispatchError(
> >+        SpeechRecognition::EVENT_RECOGNITIONSERVICE_ERROR,
> >+        SpeechRecognitionErrorCode::Network, // TODO different codes?
> >+        NS_LITERAL_STRING("RECOGNITIONSERVICE_ERROR test event"));
> >+
> >+  } else if (eventName.EqualsLiteral("EVENT_RECOGNITIONSERVICE_FINAL_RESULT")) {
> >+    nsRefPtr<SpeechEvent> event = new SpeechEvent(
> >+        mRecognition, SpeechRecognition::EVENT_RECOGNITIONSERVICE_FINAL_RESULT);
> >+
> >+    event->mRecognitionResultList = BuildMockResultList();
> >+    NS_DispatchToMainThread(event);
> >+  }
> >+
> >+  return NS_OK;
> >+}
> >+
> >+SpeechRecognitionResultList*
> >+PocketSphinxSpeechRecognitionService::BuildMockResultList()
> >+{
> >+  SpeechRecognitionResultList* resultList =
> >+      new SpeechRecognitionResultList(mRecognition);
> >+  SpeechRecognitionResult* result = new SpeechRecognitionResult(mRecognition);
> >+  SpeechRecognitionAlternative* alternative =
> >+      new SpeechRecognitionAlternative(mRecognition);
> >+
> >+  alternative->mTranscript = NS_LITERAL_STRING("Mock final result");
> >+  alternative->mConfidence = 0.0f;
> >+
> >+  result->mItems.AppendElement(alternative);
> >+  resultList->mItems.AppendElement(result);
> >+
> >+  return resultList;
> >+}
> Please use 4 spaces for indentation.
> But I'm now here lost with the "mock" setup. Why BuildMockResultList?
> 
> 
> >+  /**
> >+   * Builds a mock SpeechRecognitionResultList
> >+   */
> >+  dom::SpeechRecognitionResultList *BuildMockResultList();
> * goes with the type
> 
> >+
> >+  /** Speex state */
> >+  SpeexResamplerState *mSpeexState;
> ditto. SpeexResamplerState* mSpeexState;
> 
> >+
> >+  /** Pocksphix decoder */
> >+  ps_decoder_t *mPSHandle;
> ditto
> 
> >+
> >+  /** Sphinxbase parsed command line arguments */
> >+  cmd_ln_t *mPSConfig;
> 
> ditto
> >+  /** Flag to verify if decoder was created */
> >+  bool ISDecoderCreated;
> >+
> >+  /** Flag to verify if grammar was compiled */
> >+  bool ISGrammarCompiled;
> 
> member variables should be name mFoo
> 
> >+
> >+  /** Audio data */
> >+  std::vector<int16_t> mAudioVector;
> I'd prefer using nsTArray, of if we know some expected size for the array,
> perhaps even nsAutoTArray (that might reduce memory churn quite a bit).
> 
> 
> 
> >+++ b/dom/media/webspeech/recognition/SpeechGrammar.h
> >@@ -49,6 +49,8 @@ private:
> >   ~SpeechGrammar();
> > 
> >   nsCOMPtr<nsISupports> mParent;
> >+
> >+  nsString mGram;
> Could you call this mSrc;
> 
> 
> 
> 
> >+++ b/dom/media/webspeech/recognition/SpeechGrammarList.h
> >@@ -56,6 +56,8 @@ private:
> >   ~SpeechGrammarList();
> > 
> >   nsCOMPtr<nsISupports> mParent;
> >+
> >+  nsTArray<nsRefPtr<SpeechGrammar> > mItems;
> drop space between > and >. These days all the compilers support >>, in the
> old days
> > > had to be used.
> 
> 
> 
> >@@ -535,7 +535,9 @@ SpeechRecognition::StartRecording(DOMMediaStream* aDOMStream)
> >   // doesn't get Destroy()'ed
> >   mDOMStream = aDOMStream;
> > 
> >-  NS_ENSURE_STATE(mDOMStream->GetStream());
> >+  if (NS_WARN_IF(!mDOMStream->GetStream())) {
> >+    return NS_ERROR_UNEXPECTED;
> >+  }
> >   mSpeechListener = new SpeechStreamListener(this);
> >   mDOMStream->GetStream()->AddListener(mSpeechListener);
> > 
> >@@ -698,11 +700,15 @@ SpeechRecognition::Start(const Optional<NonNull<DOMMediaStream>>& aStream, Error
> >   }
> > 
> >   mRecognitionService = GetSpeechRecognitionService();
> >-  NS_ENSURE_TRUE_VOID(mRecognitionService);
> >+  if (NS_WARN_IF(!mRecognitionService)) {
> >+    return;
> >+  }
> > 
> >   nsresult rv;
> >   rv = mRecognitionService->Initialize(this);
> >-  NS_ENSURE_SUCCESS_VOID(rv);
> >+  if (NS_WARN_IF(NS_FAILED(rv))) {
> >+    return;
> >+  }
> I prefer use of NS_ENSURE_* macros over if (NS_WARN_IF())
> 
> 
> 
> 
> > SpeechRecognitionAlternative::SpeechRecognitionAlternative(SpeechRecognition* aParent)
> >-  : mTranscript(NS_LITERAL_STRING(""))
> >+  : mTranscript(EmptyString())
> The right fix would be to drop explicit construction here. Strings are empty
> by default
> 

s/NS_LITERAL_STRING("")/EmptyString()/g doesn't usually make such changes. But, I can remove
the explicit member construction here.
(In reply to Olli Pettay [:smaug] from comment #46)
> Comment on attachment 8610065 [details] [diff] [review]
> Part 1 of 2:  Patch that introduces WebSpeech API implementation
> 
> 
> > interface SpeechRecognitionResult {
> >     readonly attribute unsigned long length;
> >     getter SpeechRecognitionAlternative item(unsigned long index);
> >-    readonly attribute boolean final;
> >+    readonly attribute boolean isFinal;
> Why this change?
> The spec has 
>     interface SpeechRecognitionResult {
>         readonly attribute unsigned long length;
>         getter SpeechRecognitionAlternative item(in unsigned long index);
>         readonly attribute boolean final;
>     };
> please undo, or explain.

There are two versions of the spec the 19 October 2012 version [3] and the
6 June 2014 version [4]. The speech synthesis portion of the API targets
the newer version of the spec; thus, the recognition portion of the API
should also target this newer version of the spec.

The spec you are referring to is the older version of the spec [1]. The newer
version of the spec [2] changed this member name from final to isFinal. Thus
I have also changed this member name to adhere to the newer spec.

[1] https://dvcs.w3.org/hg/speech-api/raw-file/tip/speechapi.html#dfn-final
[2] https://dvcs.w3.org/hg/speech-api/raw-file/tip/webspeechapi.html#dfn-isFinal
[3] https://dvcs.w3.org/hg/speech-api/raw-file/tip/speechapi.html
[4] https://dvcs.w3.org/hg/speech-api/raw-file/tip/webspeechapi.html
(In reply to Olli Pettay [:smaug] from comment #46)
> Comment on attachment 8610065 [details] [diff] [review]
> Part 1 of 2:  Patch that introduces WebSpeech API implementation
> 
> >+NS_IMETHODIMP
> >+PocketSphinxSpeechRecognitionService::Observe(nsISupports* aSubject,
> >+                                              const char* aTopic,
> >+                                              const char16_t* aData)
> >+{
> >+  MOZ_ASSERT(mRecognition->mTestConfig.mFakeRecognitionService,
> >+             "Got request to fake recognition service event, "
> >+             "but " TEST_PREFERENCE_FAKE_RECOGNITION_SERVICE " is not set");
> >+
> >+  if (!strcmp(aTopic, SPEECH_RECOGNITION_TEST_END_TOPIC)) {
> >+    nsCOMPtr<nsIObserverService> obs = services::GetObserverService();
> >+    obs->RemoveObserver(this, SPEECH_RECOGNITION_TEST_EVENT_REQUEST_TOPIC);
> >+    obs->RemoveObserver(this, SPEECH_RECOGNITION_TEST_END_TOPIC);
> >+
> >+    return NS_OK;
> >+  }
> >+
> >+  const nsDependentString eventName = nsDependentString(aData);
> >+
> >+  if (eventName.EqualsLiteral("EVENT_RECOGNITIONSERVICE_ERROR")) {
> >+    mRecognition->DispatchError(
> >+        SpeechRecognition::EVENT_RECOGNITIONSERVICE_ERROR,
> >+        SpeechRecognitionErrorCode::Network, // TODO different codes?
> >+        NS_LITERAL_STRING("RECOGNITIONSERVICE_ERROR test event"));
> >+
> >+  } else if (eventName.EqualsLiteral("EVENT_RECOGNITIONSERVICE_FINAL_RESULT")) {
> >+    nsRefPtr<SpeechEvent> event = new SpeechEvent(
> >+        mRecognition, SpeechRecognition::EVENT_RECOGNITIONSERVICE_FINAL_RESULT);
> >+
> >+    event->mRecognitionResultList = BuildMockResultList();
> >+    NS_DispatchToMainThread(event);
> >+  }
> >+
> >+  return NS_OK;
> >+}
> >+
> >+SpeechRecognitionResultList*
> >+PocketSphinxSpeechRecognitionService::BuildMockResultList()
> >+{
> >+  SpeechRecognitionResultList* resultList =
> >+      new SpeechRecognitionResultList(mRecognition);
> >+  SpeechRecognitionResult* result = new SpeechRecognitionResult(mRecognition);
> >+  SpeechRecognitionAlternative* alternative =
> >+      new SpeechRecognitionAlternative(mRecognition);
> >+
> >+  alternative->mTranscript = NS_LITERAL_STRING("Mock final result");
> >+  alternative->mConfidence = 0.0f;
> >+
> >+  result->mItems.AppendElement(alternative);
> >+  resultList->mItems.AppendElement(result);
> >+
> >+  return resultList;
> >+}
> Please use 4 spaces for indentation.

I've a question....

The Mozilla Coding Convention [1] states of indentation:

"Two spaces per logic level, or four spaces in Python code."

My reading of this is that indentation should be two spaces.

Also, the code examples given by the Mozilla Coding Convention
use two spaces for indentation. So it seems as if two spaces is
the Mozilla coding convention.

Is this not so?

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Indentation
(In reply to kdavis from comment #59)

> > >+  return resultList;
> > >+}
> > Please use 4 spaces for indentation.
> 
> I've a question....
> 
> The Mozilla Coding Convention [1] states of indentation:
> 
> "Two spaces per logic level, or four spaces in Python code."
> 
> My reading of this is that indentation should be two spaces.
YES. What did I write. 2 space. 2 2 2. I must have missing "don't"
(In reply to kdavis from comment #58)
> (In reply to Olli Pettay [:smaug] from comment #46)
> > Comment on attachment 8610065 [details] [diff] [review]
> > Part 1 of 2:  Patch that introduces WebSpeech API implementation
> > 
> > 
> > > interface SpeechRecognitionResult {
> > >     readonly attribute unsigned long length;
> > >     getter SpeechRecognitionAlternative item(unsigned long index);
> > >-    readonly attribute boolean final;
> > >+    readonly attribute boolean isFinal;
> > Why this change?
> > The spec has 
> >     interface SpeechRecognitionResult {
> >         readonly attribute unsigned long length;
> >         getter SpeechRecognitionAlternative item(in unsigned long index);
> >         readonly attribute boolean final;
> >     };
> > please undo, or explain.
> 
> There are two versions of the spec the 19 October 2012 version [3] and the
> 6 June 2014 version [4]. The speech synthesis portion of the API targets
> the newer version of the spec; thus, the recognition portion of the API
> should also target this newer version of the spec.
> 
> The spec you are referring to is the older version of the spec [1]. The newer
> version of the spec [2] changed this member name from final to isFinal. Thus
> I have also changed this member name to adhere to the newer spec.
> 
> [1] https://dvcs.w3.org/hg/speech-api/raw-file/tip/speechapi.html#dfn-final
> [2]
> https://dvcs.w3.org/hg/speech-api/raw-file/tip/webspeechapi.html#dfn-isFinal
> [3] https://dvcs.w3.org/hg/speech-api/raw-file/tip/speechapi.html
> [4] https://dvcs.w3.org/hg/speech-api/raw-file/tip/webspeechapi.html

I see. It is possible that looked at https://dvcs.w3.org/hg/speech-api/raw-file/tip/speechapi.html last night. "tip" in the url is very misleading.
(In reply to kdavis from comment #56)
> 
> The Mozilla coding standard[1] mandates use of NS_WARN_IF in new code. I
> quote
> 
> "Previously the NS_ENSURE_* macros were used for this purpose, but those
> macros
> hide return statements and should not be used in new code."
> 
Hmm, who added that to the coding style page. It is not agreed.
But fine.
(In reply to kdavis from comment #53)
> 
> That being said, I don't see the advantage. Maybe there is something I am
> missing?
If we can estimate what the needed size is, using nsAutoTArray would reduce malloc/free calls possibly significantly. And nsTArray vs std::vector: we know how nsTArray behaves from performance point of view. We can't have similar guarantees for vector.
See also https://developer.mozilla.org/en-US/docs/Using_CXX_in_Mozilla_code#C.2B.2B_and_Mozilla_standard_libraries
(In reply to Olli Pettay [:smaug] from comment #63)
> (In reply to kdavis from comment #53)
> If we can estimate what the needed size is, using nsAutoTArray would reduce
> malloc/free calls possibly significantly. And nsTArray vs std::vector: we
> know how nsTArray behaves from performance point of view. We can't have
> similar guarantees for vector.
> See also
> https://developer.mozilla.org/en-US/docs/Using_CXX_in_Mozilla_code#C.2B.
> 2B_and_Mozilla_standard_libraries

We can't estimate the needed size as we don't know what the user will say
at compile time. So, the initial capacity of a nsAutoTArray, or for that
matter std::vector, is not going to help us much.

Also, I'm not sure what you mean by...

> we know how nsTArray behaves from performance point of view. We can't
> have similar guarantees for vector

The C++ standard makes performance guarantees for its containers.

For example for std::vector see sections 23.3.6.2-23.3.6.5 of the current working
draft [1] of the C++ standard. The complexity of each method and constructor is
specified.

However, I can use a nsTArray, but I don't think the stated reasons justify its use.

[1] http://open-std.org/JTC1/SC22/WG21/docs/papers/2015/n4431.pdf
(In reply to kdavis from comment #64)
> We can't estimate the needed size as we don't know what the user will say
> at compile time. So, the initial capacity of a nsAutoTArray, or for that
> matter std::vector, is not going to help us much.
Well, I was thinking if we could estimate how much some shorter speech commands would usually take
memory and just use nsAutoTArray at least that size. But perhaps not worth.


> > we know how nsTArray behaves from performance point of view. We can't
> > have similar guarantees for vector
> 
> The C++ standard makes performance guarantees for its containers.
It does? Does it guarantee in which way the buffer inside vector is increased?
(In reply to Olli Pettay [:smaug] from comment #65)
> (In reply to kdavis from comment #64)
>
> > The C++ standard makes performance guarantees for its containers.
> It does? Does it guarantee in which way the buffer inside vector is
> increased?

The listed complexities of 23.3.6.2-23.3.6.5 are for the most part time
complexities not space complexities. Space complexities are indeed under
specified by the standard. However, time complexities are, more-or-less,
fully specified.

Be that as it may, I'll use nsTArray. Thus, the question of std::vector
space complexity will not arise.
Comment on attachment 8610066 [details] [diff] [review]
Part 2 of 2:  Patch that turns off the WebSpeech API implementation using flags

Flagging  gps for review of build changes
Attachment #8610066 - Flags: superreview?(gps)
Part 1 of 2:  Patch that introduces WebSpeech API implementation

This is the part 1 of 2 for this bug.

This patch introduces the Pocketsphinx/Sphinxbase implementation of the WebSpeech API. This patch addresses the review suggestions of Comment 46. This implementation depends upon all of the patches for Bug 1051146.

The try for this patch is running here https://treeherder.mozilla.org/#/jobs?repo=try&revision=68e31ec78735
Attachment #8610065 - Attachment is obsolete: true
Attachment #8612098 - Flags: review+
(In reply to kdavis from comment #67)
> Comment on attachment 8610066 [details] [diff] [review]
> Part 2 of 2:  Patch that turns off the WebSpeech API implementation using
> flags
> 
> Flagging  gps for review of build changes

gps is this alright as is?
Flags: needinfo?(gps)
Blocks: 1115604
Whiteboard: [webspeechapi] → [webspeechapi][systemsfe]
Target Milestone: --- → 2.2 S14 (12june)
Duplicate of this bug: 1161447
Comment on attachment 8610066 [details] [diff] [review]
Part 2 of 2:  Patch that turns off the WebSpeech API implementation using flags

Review of attachment 8610066 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not sure why you are changing configure.in: you need the AC_SUBST bits for this to work.

::: configure.in
@@ -5191,5 @@
> -if test -n "$MOZ_WEBSPEECH_POCKETSPHINX"; then
> -    AC_DEFINE(MOZ_WEBSPEECH_POCKETSPHINX)
> -fi
> -
> -AC_SUBST(MOZ_WEBSPEECH_POCKETSPHINX)

Removing MOZ_WEBSPEECH_POCKETSPHINX from AC_SUBST will not expose this variable to moz.build files, causing the check for feature presence in the moz.build added in part 1 to never return true.
Attachment #8610066 - Flags: superreview?(gps) → superreview-
Flags: needinfo?(gps)
(In reply to Gregory Szorc [:gps] from comment #71)
> Comment on attachment 8610066 [details] [diff] [review]
> Part 2 of 2:  Patch that turns off the WebSpeech API implementation using
> flags
> 
> Review of attachment 8610066 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm not sure why you are changing configure.in: you need the AC_SUBST bits
> for this to work.
> 
> ::: configure.in
> @@ -5191,5 @@
> > -if test -n "$MOZ_WEBSPEECH_POCKETSPHINX"; then
> > -    AC_DEFINE(MOZ_WEBSPEECH_POCKETSPHINX)
> > -fi
> > -
> > -AC_SUBST(MOZ_WEBSPEECH_POCKETSPHINX)
> 
> Removing MOZ_WEBSPEECH_POCKETSPHINX from AC_SUBST will not expose this
> variable to moz.build files, causing the check for feature presence in the
> moz.build added in part 1 to never return true.

I think only the MOZ_ARG_DISABLE_BOOL should be removed so the flag is not on
by default. So, I'd think the diff should be something like...

 dnl ========================================================
 dnl = Disable Speech API models
 dnl ========================================================
-MOZ_ARG_DISABLE_BOOL(webspeechmodels,
-[  --disable-webspeechmodels        Disable support for HTML Speech API Models],
-    MOZ_WEBSPEECH_MODELS=,
-    MOZ_WEBSPEECH_MODELS=1)
 
 if test -n "$MOZ_WEBSPEECH_MODELS"; then
     AC_DEFINE(MOZ_WEBSPEECH_MODELS)
 fi
 
 AC_SUBST(MOZ_WEBSPEECH_MODELS)

and a similar thing for the other similar flags.

Seem reasonable?
Flags: needinfo?(gps)
Part 2 of 2:  Patch that turns off the WebSpeech API implementation using flags

This is the part 2 of 2 for this bug.

This patch turns off the WebSpeech API implementation using flags. This implementation depends upon all of the patches for Bug 1051146.

This patch obsoletes attachment 8612098 [details] [diff] [review], its previous version, and
implements the suggestions of the build peer review Comment 71

The try for this patch is running here https://treeherder.mozilla.org/#/jobs?repo=try&revision=365759599600
Attachment #8612098 - Attachment is obsolete: true
Attachment #8614016 - Flags: review?(gps)
Comment on attachment 8614016 [details] [diff] [review]
Part 2 of 2:  Patch that turns off the WebSpeech API implementation using flags

Review of attachment 8614016 [details] [diff] [review]:
-----------------------------------------------------------------

This looks better.

It's kinda weird not having a way to enable a feature via a configure argument. You could switch this to MOZ_ARG_ENABLE_BOOL and make it opt in by default. But this is your feature and I don't care too much to enforce configure flag presence.
Attachment #8614016 - Flags: review?(gps) → review+
Flags: needinfo?(gps)
Comment on attachment 8612098 [details] [diff] [review]
Part 1 of 2:  Patch that introduces WebSpeech API implementation

Clicked wrong box
Attachment #8612098 - Attachment is obsolete: false
Comment on attachment 8610066 [details] [diff] [review]
Part 2 of 2:  Patch that turns off the WebSpeech API implementation using flags

Clicked wrong box
Attachment #8610066 - Attachment is obsolete: true
Keywords: checkin-needed
Backed out for the rooting failures identified in bug 1051146 comment 139.
"init" needs to become a RootedDictionary<SpeechRecognitionEventInit>.
(In reply to Not doing reviews right now from comment #80)
> "init" needs to become a RootedDictionary<SpeechRecognitionEventInit>.

Thanks!
Depends on: 1171249
Depends on: 1171850
Part 1 of 2:  Patch that introduces WebSpeech API implementation

This is the part 1 of 2 for this bug.

This patch introduces the Pocketsphinx/Sphinxbase implementation of the WebSpeech API. Relative to the patch it obsoletes this patch introduces
only a single change on a single line, that suggested in Comment 80.

To run properly this patch depends upon Bug 1171249, which is currently
tagged checkin-needed, and Bug 1171850, a one line change, that is waiting
on build peer review. 

The try for this patch is running here https://treeherder.mozilla.org/#/jobs?repo=try&revision=6e1382c3709d
Attachment #8612098 - Attachment is obsolete: true
Attachment #8615907 - Flags: review+
Part 2 of 2:  Patch that turns off the WebSpeech API implementation using flags

This is the part 2 of 2 for this bug.

This patch turns off the WebSpeech API implementation using flags.
This implementation depends upon Bug 1171249, which is currently
tagged checkin-needed.

Relative to the patch it obsoletes this patch make no material
changes. It only compensates for the line number changes that
are needed as a result of Bug 1171249.

The try for this patch is running here https://treeherder.mozilla.org/#/jobs?repo=try&revision=372d7aa2b034
Attachment #8614016 - Attachment is obsolete: true
Attachment #8615913 - Flags: review+
Attachment #8615907 - Flags: review+ → review?(bugs)
Attachment #8615913 - Flags: review+ → review?(bugs)
Attachment #8615913 - Flags: review?(bugs) → review?(gps)
Comment on attachment 8615907 [details] [diff] [review]
Part 1 of 2:  Patch that introduces WebSpeech API implementation

The diff between the new and old patches is due to the change...

SpeechRecognitionEventInit init;

to

RootedDictionary<SpeechRecognitionEventInit> init(nsContentUtils::RootingCx());

as suggested in Comment 80. The rest of the diff between patches
is simply a result of changes in the "index f311ac2" values. 

============================================================================
8c8
< index 3cf84cf..9d2761c4 100644
---
> index 7f150fe..8cf1d76 100644
11c11
< @@ -1003,6 +1003,10 @@ pref("network.proxy.browsing.app_origins", "app://system.gaiamobile.org");
---
> @@ -1001,6 +1001,10 @@ pref("network.proxy.browsing.app_origins", "app://system.gaiamobile.org");
586c586
< index 2001e2e..002612a 100644
---
> index 0666153..fbb9c44 100644
610,616c610
< @@ -472,12 +472,12 @@ SpeechRecognition::NotifyFinalResult(SpeechEvent* aEvent)
<  {
<    ResetAndEnd();
<  
< -  SpeechRecognitionEventInit init;
< +  RootedDictionary<SpeechRecognitionEventInit> init(nsContentUtils::RootingCx());
<    init.mBubbles = true;
---
> @@ -477,7 +477,7 @@ SpeechRecognition::NotifyFinalResult(SpeechEvent* aEvent)
731c725
< index 14cb375..8002a2f 100644
---
> index f311ac2..ee3ac68 100644
734c728
< @@ -26,6 +26,11 @@ if CONFIG['MOZ_WEBSPEECH_TEST_BACKEND']:
---
> @@ -29,6 +29,11 @@ if CONFIG['MOZ_WEBSPEECH_TEST_BACKEND']:
746c740
< @@ -44,10 +49,21 @@ if CONFIG['MOZ_WEBSPEECH_TEST_BACKEND']:
---
> @@ -47,10 +52,21 @@ if CONFIG['MOZ_WEBSPEECH_TEST_BACKEND']:
Comment on attachment 8615907 [details] [diff] [review]
Part 1 of 2:  Patch that introduces WebSpeech API implementation

Even more easily understood is the interdiff between the new and old patches:

diff -u b/dom/media/webspeech/recognition/SpeechRecognition.cpp b/dom/media/webspeech/recognition/SpeechRecognition.cpp
--- b/dom/media/webspeech/recognition/SpeechRecognition.cpp
+++ b/dom/media/webspeech/recognition/SpeechRecognition.cpp
@@ -472,7 +472,7 @@
 {
   ResetAndEnd();
 
-  RootedDictionary<SpeechRecognitionEventInit> init(nsContentUtils::RootingCx());
+  SpeechRecognitionEventInit init;
   init.mBubbles = true;
   init.mCancelable = false;
   // init.mResultIndex = 0;
Comment on attachment 8615913 [details] [diff] [review]
Part 2 of 2:  Patch that turns off the WebSpeech API implementation using flags

The interdiff between the new and old patches here is simple it's the empty file.
Comment on attachment 8615913 [details] [diff] [review]
Part 2 of 2:  Patch that turns off the WebSpeech API implementation using flags

Review of attachment 8615913 [details] [diff] [review]:
-----------------------------------------------------------------

Haven't I reviewed this already?
Attachment #8615913 - Flags: review?(gps) → review+
(In reply to Gregory Szorc [:gps] from comment #87)
> Comment on attachment 8615913 [details] [diff] [review]
> Part 2 of 2:  Patch that turns off the WebSpeech API implementation using
> flags
> 
> Review of attachment 8615913 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Haven't I reviewed this already?

Yeah. The diff here shifts line numbers of the changes, other that that
it's the same. The line numbers had to shift as a result of Bug 1171249. 

Gregor just wanted me run it by everyone again.
(In reply to kdavis from comment #88)
> (In reply to Gregory Szorc [:gps] from comment #87)
> > Comment on attachment 8615913 [details] [diff] [review]
> > Part 2 of 2:  Patch that turns off the WebSpeech API implementation using
> > flags
> > 
> > Review of attachment 8615913 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Haven't I reviewed this already?
> 
> Yeah. The diff here shifts line numbers of the changes, other that that
> it's the same. The line numbers had to shift as a result of Bug 1171249. 
> 
> Gregor just wanted me run it by everyone again.

Oh you don't have to ask for review if only the line numbers change. I was talking about the other patch where you actually made a code change.
(In reply to Gregor Wagner [:gwagner] from comment #89)
> (In reply to kdavis from comment #88)
> > (In reply to Gregory Szorc [:gps] from comment #87)
> > > Comment on attachment 8615913 [details] [diff] [review]
> > > Part 2 of 2:  Patch that turns off the WebSpeech API implementation using
> > > flags
> > > 
> > > Review of attachment 8615913 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > Haven't I reviewed this already?
> > 
> > Yeah. The diff here shifts line numbers of the changes, other that that
> > it's the same. The line numbers had to shift as a result of Bug 1171249. 
> > 
> > Gregor just wanted me run it by everyone again.
> 
> Oh you don't have to ask for review if only the line numbers change. I was
> talking about the other patch where you actually made a code change.

I was just looking at the line that causes the problem SpeechRecognition.cpp:475

https://github.com/mozilla/gecko-dev/blob/master/dom/media/webspeech/recognition/SpeechRecognition.cpp#L475

and, strangely enough, my original patch doesn't change that line at all. My
patch just left the line as was.

Maybe some new commit hook was added between when SpeechRecognition.cpp:475 was
originally written and now?

Either way, the new patch should fix it.
Attachment #8615907 - Flags: review?(bugs) → review+
Blocks: 1167875
Depends on: 1172925
Part 1 of 2:  Patch that introduces WebSpeech API implementation

This is the part 1 of 2 for this bug.

This patch introduces the Pocketsphinx/Sphinxbase implementation of the WebSpeech API. Relative to the patch it obsoletes this patch makes *no*
changes other than compensating for line number changes that have occurred
in the intervening time period.

To run properly this patch depends upon Bug 1172925, which is currently
waiting on build peer review. 

The try for this patch is running here https://treeherder.mozilla.org/#/jobs?repo=try&revision=5b35b8d10b7d
Attachment #8615907 - Attachment is obsolete: true
Attachment #8617745 - Flags: review+
Part 2 of 2:  Patch that turns off the WebSpeech API implementation using flags

This is the part 2 of 2 for this bug.

This patch turns off the WebSpeech API implementation using flags.
This implementation depends upon Bug 1172925, which is currently
waiting on build peer review.

Relative to the patch it obsoletes this patch make *no* changes
other than compensating for line number changes that have occurred
in the intervening time period.

The try for this patch is running here https://treeherder.mozilla.org/#/jobs?repo=try&revision=e64cefa5f4ca
Attachment #8615913 - Attachment is obsolete: true
Attachment #8617747 - Flags: review+
Blocks: 1169653
Keywords: checkin-needed
Duplicate of this bug: 1067700
Duplicate of this bug: 1088336
Duplicate of this bug: 1067700
User Story: (updated)
Summary: Development of PocketSphinxSpeechRecognitionService to use pocketsphinx and enable Webspeech API → Implementation of Webspeech API & PocketSphinx Integration
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.