1060659 - SpeechGrammarList should both initialize SpeechRecognitionService and set a grammar on it

Status: RESOLVED FIXED

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

Comment 1

[Andre Natal]

Attachment: 0003-This-patch-enables-SpeechGrammarList-who-both-create.patch

Comment 2

[Andre Natal]

This patch both initialize the SpeechRecogntionService as pass the grammar included to it by setGrammarList method.

Comment 3

[Andre Natal]

Attachment: 0004-This-patch-enables-SpeechGrammarList-who-both-create.patch

Comment 4

[Guilherme Gonçalves [:ggp]]

Comment on attachment 8481706 [details] [diff] [review]
0004-This-patch-enables-SpeechGrammarList-who-both-create.patch

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

I'll do a quick first pass now, but I promise I'll make time for a more careful look next week :)

First things first, please update the commit message so it references the bug number and describes your change. I suggest something like:

"Bug 1060659 - Process the grammar on the SpeechRecognitionService when it's set in a SpeechGrammarList. r=smaug"

::: content/media/webspeech/recognition/SpeechGrammarList.cpp
@@ +16,3 @@
>  
>  namespace mozilla {
> +  namespace dom {

Please remove this indentation level.

@@ +43,5 @@
> +        speechRecognitionService = prefValue;
> +      }
> +      speechRecognitionServiceCID =
> +        NS_LITERAL_CSTRING(NS_SPEECH_RECOGNITION_SERVICE_CONTRACTID_PREFIX) +
> +        speechRecognitionService;

Can you move this logic to a function in SpeechRecognition.cpp, and just call it here? Otherwise we end up with this logic duplicated between SpeechRecognition.cpp and SpeechGrammarList.cpp

@@ +119,5 @@
> +          NS_LITERAL_CSTRING(NS_SPEECH_RECOGNITION_SERVICE_CONTRACTID_PREFIX) +
> +          speechRecognitionService;
> +
> +        nsCOMPtr<nsISpeechRecognitionService> mRecognitionService;
> +        mRecognitionService = do_GetService(speechRecognitionServiceCID.get(), &rv);

Same comment about duplication applies here :)

@@ +120,5 @@
> +          speechRecognitionService;
> +
> +        nsCOMPtr<nsISpeechRecognitionService> mRecognitionService;
> +        mRecognitionService = do_GetService(speechRecognitionServiceCID.get(), &rv);
> +        mRecognitionService->SetGrammarList(this);

I don't see an associated change to the nsISpeechRecognitionService interface to add this method, perhaps the patch is incomplete?

::: content/media/webspeech/recognition/SpeechGrammarList.h
@@ +28,5 @@
>  class SpeechGrammar;
>  template<typename> class Optional;
>  
> +class SpeechGrammarList MOZ_FINAL : public nsSupportsWeakReference,
> +                                    public SupportsWeakPtr<SpeechGrammarList>,

Who holds a WeakPtr to SpeechGrammarList now? And why?

::: content/media/webspeech/recognition/SpeechRecognition.cpp
@@ +624,5 @@
> +  GetRecognitionServiceCID(speechRecognitionServiceCID);
> +  mRecognitionService = do_GetService(speechRecognitionServiceCID.get(), &rv);
> +  NS_ENSURE_SUCCESS_VOID(rv);
> +  
> +  rv = mRecognitionService->SetGrammarList( &aArg );

Do you also need to do this from here? Wouldn't the SpeechGrammarList have taken care of it?

Comment 5

[Andre Natal]

(In reply to Guilherme Gonçalves [:ggp] from comment #4)
> Comment on attachment 8481706 [details] [diff] [review]
> 0004-This-patch-enables-SpeechGrammarList-who-both-create.patch
> 
> Review of attachment 8481706 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'll do a quick first pass now, but I promise I'll make time for a more
> careful look next week :)
> 
> First things first, please update the commit message so it references the
> bug number and describes your change. I suggest something like:
> 
> "Bug 1060659 - Process the grammar on the SpeechRecognitionService when it's
> set in a SpeechGrammarList. r=smaug"
> 
> ::: content/media/webspeech/recognition/SpeechGrammarList.cpp
> @@ +16,3 @@
> >  
> >  namespace mozilla {
> > +  namespace dom {
> 
> Please remove this indentation level.
> 
> @@ +43,5 @@
> > +        speechRecognitionService = prefValue;
> > +      }
> > +      speechRecognitionServiceCID =
> > +        NS_LITERAL_CSTRING(NS_SPEECH_RECOGNITION_SERVICE_CONTRACTID_PREFIX) +
> > +        speechRecognitionService;
> 
> Can you move this logic to a function in SpeechRecognition.cpp, and just
> call it here? Otherwise we end up with this logic duplicated between
> SpeechRecognition.cpp and SpeechGrammarList.cpp
> 
> @@ +119,5 @@
> > +          NS_LITERAL_CSTRING(NS_SPEECH_RECOGNITION_SERVICE_CONTRACTID_PREFIX) +
> > +          speechRecognitionService;
> > +
> > +        nsCOMPtr<nsISpeechRecognitionService> mRecognitionService;
> > +        mRecognitionService = do_GetService(speechRecognitionServiceCID.get(), &rv);
> 
> Same comment about duplication applies here :)
> 
> @@ +120,5 @@
> > +          speechRecognitionService;
> > +
> > +        nsCOMPtr<nsISpeechRecognitionService> mRecognitionService;
> > +        mRecognitionService = do_GetService(speechRecognitionServiceCID.get(), &rv);
> > +        mRecognitionService->SetGrammarList(this);
> 
> I don't see an associated change to the nsISpeechRecognitionService
> interface to add this method, perhaps the patch is incomplete?
> 
> ::: content/media/webspeech/recognition/SpeechGrammarList.h
> @@ +28,5 @@
> >  class SpeechGrammar;
> >  template<typename> class Optional;
> >  
> > +class SpeechGrammarList MOZ_FINAL : public nsSupportsWeakReference,
> > +                                    public SupportsWeakPtr<SpeechGrammarList>,
> 
> Who holds a WeakPtr to SpeechGrammarList now? And why?
> 
> ::: content/media/webspeech/recognition/SpeechRecognition.cpp
> @@ +624,5 @@
> > +  GetRecognitionServiceCID(speechRecognitionServiceCID);
> > +  mRecognitionService = do_GetService(speechRecognitionServiceCID.get(), &rv);
> > +  NS_ENSURE_SUCCESS_VOID(rv);
> > +  
> > +  rv = mRecognitionService->SetGrammarList( &aArg );
> 
> Do you also need to do this from here? Wouldn't the SpeechGrammarList have
> taken care of it?


When I merged my dev repo with the upstream, my implementation was broked by some recent changes on these classes. I will refactor asap and update a new patch

Comment 6

[Andre Natal]

Attachment: Bug 1060659 - SpeechGrammarList should both initialize SpeechRecognitionService and set a grammar on it

Merged with the upstream and issues fixed

Comment 7

[Guilherme Gonçalves [:ggp]]

Comment on attachment 8489470 [details] [diff] [review]
Bug 1060659 - SpeechGrammarList should both initialize SpeechRecognitionService and set a grammar on it

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

The commit message mentions [PATCH 1/2]. Is there another patch coming up for this bug? If not, please remove that.

::: content/media/webspeech/recognition/SpeechGrammarList.cpp
@@ +14,5 @@
>  
>  namespace mozilla {
>  namespace dom {
>  
> +    NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE(SpeechGrammarList, mParent)

It looks like you added an indentation level to the entire file. Please remove that.

@@ +26,5 @@
> +    SpeechGrammarList::SpeechGrammarList(nsISupports* aParent)
> +      : mParent(aParent)
> +    {
> +  		mRecognitionService = GetSpeechRecognitionService();
> +  		SetIsDOMBinding();

There are some tabs in these lines, please remove them.

@@ +40,5 @@
> +    {
> +
> +      nsRefPtr<SpeechGrammarList> speechGrammarList =
> +        new SpeechGrammarList(aGlobal.GetAsSupports());
> +      return speechGrammarList.forget();      

Watch out for trailing whitespace. Also why did this have to be changed? Do constructors have to return already_AddRefed<T> now?

@@ +87,5 @@
> +        if (!mRecognitionService) {
> +          aRv.Throw(NS_ERROR_NOT_IMPLEMENTED);
> +        }
> +        else
> +        {

} else {

@@ +88,5 @@
> +          aRv.Throw(NS_ERROR_NOT_IMPLEMENTED);
> +        }
> +        else
> +        {
> +          mRecognitionService->SetGrammarList(this);    

Trailing whitespace.

@@ +90,5 @@
> +        else
> +        {
> +          mRecognitionService->SetGrammarList(this);    
> +        }
> +      

Whitespace.

::: content/media/webspeech/recognition/SpeechGrammarList.h
@@ +29,3 @@
>  struct JSContext;
>  
> +class nsIDOMWindow;

Are you sure you need all of these declarations here? They should be necessary for implementing recognition services, but please double-check whether they're required here. In particular, nsIDOMWindow seems unused.

@@ +40,4 @@
>  class SpeechGrammar;
>  template<typename> class Optional;
>  
> +class SpeechGrammarList MOZ_FINAL : public nsISupports, 

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

@@ +44,5 @@
>                                      public nsWrapperCache
>  {
>  public:
>    explicit SpeechGrammarList(nsISupports* aParent);
> +  

Extra space.

@@ +49,5 @@
>    NS_DECL_CYCLE_COLLECTING_ISUPPORTS
>    NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS(SpeechGrammarList)
>  
> +  static already_AddRefed<SpeechGrammarList>
> +    Constructor(const GlobalObject& aGlobal, ErrorResult& aRv);

It should be fine to make this into a single line. It's not a big deal to go over 80 columns in header files as far as I know. Also is this really supposed to be static?

@@ +67,5 @@
>    already_AddRefed<SpeechGrammar> IndexedGetter(uint32_t aIndex, bool& aPresent, ErrorResult& aRv);
>  
> +  nsCOMPtr<nsISpeechRecognitionService> mRecognitionService;
> +
> +  const char * mGram;

This looks unused.

::: content/media/webspeech/recognition/SpeechRecognition.cpp
@@ +57,4 @@
>  #define SR_LOG(...)
>  #endif
>  
> +nsCOMPtr<nsISpeechRecognitionService> GetSpeechRecognitionService()

The return value should be in a separate line:

nsCOMPtr<nsISpeechRecognitionService>
GetSpeechRecognitionService()
{
...
}

@@ +74,5 @@
> +      speechRecognitionService;
> +
> +    nsCOMPtr<nsISpeechRecognitionService> mRecognitionService;
> +    mRecognitionService = do_GetService(speechRecognitionServiceCID.get(), &rv); 
> +    

Please remove the extra space on lines 77 and 78.

@@ +75,5 @@
> +
> +    nsCOMPtr<nsISpeechRecognitionService> mRecognitionService;
> +    mRecognitionService = do_GetService(speechRecognitionServiceCID.get(), &rv); 
> +    
> +    return mRecognitionService;

Please also use this function from SpeechRecognition.cpp when grabbing a reference to the service, to avoid duplication.

::: content/media/webspeech/recognition/SpeechRecognition.h
@@ +54,5 @@
>  #endif
>  
> +nsCOMPtr<nsISpeechRecognitionService> GetSpeechRecognitionService();
> +
> +

Nit: remove one of these blank lines.

::: content/media/webspeech/recognition/nsISpeechRecognitionService.idl
@@ +16,5 @@
>  [uuid(857f3fa2-a980-4d3e-a959-a2f53af74232)]
>  interface nsISpeechRecognitionService : nsISupports {
>      void initialize(in SpeechRecognitionWeakPtr aSpeechRecognition);
>      void processAudioSegment(in AudioSegmentPtr aAudioSegment, in long aSampleRate);
> +    void setGrammarList(in SpeechGrammarListPtr aSpeechGramarList);	

Nit: If this is going to process/validate the grammar list, we may want to use a more descriptive name, like validateAndSetGrammarList.

Also please add a comment that this is async, may fail, and will return a Promise soon. Make sure you reference the bug number for switching to Promise.

::: content/media/webspeech/recognition/test/FakeSpeechRecognitionService.cpp
@@ +56,5 @@
> +{
> +  return NS_OK;
> +}
> +
> +

Nit: please leave only one blank line here.

Comment 8

[Andre Natal]

Attachment: Bug 1060659 - SpeechGrammarList should both initialize SpeechRecognitionService and set a grammar on it

Comment 9

[Andre Natal]

Attachment: Bug 1060659 - SpeechGrammarList should both initialize SpeechRecognitionService and set a grammar on it

Comment 10

[Andre Natal]

Comment on attachment 8492498 [details] [diff] [review]
Bug 1060659 - SpeechGrammarList should both initialize SpeechRecognitionService and set a grammar on it

I addressed your requests.

Comment 11

[Andre Natal]

Comment on attachment 8489470 [details] [diff] [review]
Bug 1060659 - SpeechGrammarList should both initialize SpeechRecognitionService and set a grammar on it

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

::: content/media/webspeech/recognition/SpeechGrammarList.cpp
@@ +40,5 @@
> +    {
> +
> +      nsRefPtr<SpeechGrammarList> speechGrammarList =
> +        new SpeechGrammarList(aGlobal.GetAsSupports());
> +      return speechGrammarList.forget();      

Yes, when I merged my last patch with latest m-c, the code broke, so I needed to change it. 

I used SpeechGrammar class [1] as reference since it is newer than SpeechGrammarList [2] and ran ok. Please let me know what is the correct way.

[1] https://github.com/mozilla/gecko-dev/blob/master/content/media/webspeech/recognition/SpeechGrammar.cpp#L33

[2] https://github.com/mozilla/gecko-dev/blob/master/content/media/webspeech/recognition/SpeechGrammarList.cpp#L34

::: content/media/webspeech/recognition/SpeechGrammarList.h
@@ +29,3 @@
>  struct JSContext;
>  
> +class nsIDOMWindow;

The include nsISpeechRecognitionService.h required to add these forward declaration. Without this include I can't reference the nsISpeechRecognitionService on the header on line #69.

@@ +49,5 @@
>    NS_DECL_CYCLE_COLLECTING_ISUPPORTS
>    NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS(SpeechGrammarList)
>  
> +  static already_AddRefed<SpeechGrammarList>
> +    Constructor(const GlobalObject& aGlobal, ErrorResult& aRv);

Yes, I needed to refactor this way to make it run. I used SpeechGrammar as reference:

https://github.com/mozilla/gecko-dev/blob/master/content/media/webspeech/recognition/SpeechGrammar.h#L37

Comment 12

[Guilherme Gonçalves [:ggp]]

(In reply to Andre Natal from comment #11)
> Please let me know what is the correct way.

Fair enough, looks like this was a leftover change from bug 849567. I guess ideally we'll want to split this change into a separate patch (it should be fine to use this bug for it too, no need to file a new one), but I'll leave it to :smaug's discretion when he reviews it.

Comment 13

[Guilherme Gonçalves [:ggp]]

Comment on attachment 8492498 [details] [diff] [review]
Bug 1060659 - SpeechGrammarList should both initialize SpeechRecognitionService and set a grammar on it

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

Please remove [PATCH 3/4] from the commit message.

:smaug, I'd like to hear your thoughts on the changes to SpeechGrammarList::Constructor (see my previous comment) and the change to SpeechGrammarList::SpeechGrammarList (see the beginning of this review), please.

::: content/media/webspeech/recognition/SpeechGrammarList.cpp
@@ +19,5 @@
>  NS_IMPL_CYCLE_COLLECTING_ADDREF(SpeechGrammarList)
>  NS_IMPL_CYCLE_COLLECTING_RELEASE(SpeechGrammarList)
>  NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(SpeechGrammarList)
> +NS_WRAPPERCACHE_INTERFACE_MAP_ENTRY
> +NS_INTERFACE_MAP_ENTRY(nsISupports)

Please don't remove this indentation level.

@@ +26,5 @@
>  SpeechGrammarList::SpeechGrammarList(nsISupports* aParent)
>    : mParent(aParent)
>  {
> +  nsresult aRv;
> +  mRecognitionService = GetSpeechRecognitionService(aRv);

We may want to handle |GetSpeechRecognitionService| returning |nullptr| somehow, or at least log the error.

We can possibly call |GetSpeechRecognitionService| from |SpeechGrammarList::Constructor| instead of here, so we can return nullptr from there if it fails, and pass the nsISpeechRecognitionService * to this constructor. But I'd like to see what :smaug thinks of this.

@@ +82,5 @@
>                                   const Optional<float>& aWeight,
>                                   ErrorResult& aRv)
>  {
> +  if (!mRecognitionService) {
> +   aRv.Throw(NS_ERROR_NOT_IMPLEMENTED);

Catching this here is probably too late, let's try to think of a better approach and remove this conditional.

::: content/media/webspeech/recognition/SpeechGrammarList.h
@@ +22,5 @@
> +    class SpeechGrammarList;
> +  }
> +}
> +
> +#include "nsISpeechRecognitionService.h"

If these declarations are necessary every time you include nsISpeechRecognitionService.h, then can you please move them to the .idl file? Surrounding these lines with %{C++ and %} will cause them to appear in the generated header verbatim.

::: content/media/webspeech/recognition/SpeechRecognition.cpp
@@ +57,4 @@
>  #define SR_LOG(...)
>  #endif
>  
> +nsCOMPtr<nsISpeechRecognitionService> 

Please remove this extra space.

@@ +60,5 @@
> +nsCOMPtr<nsISpeechRecognitionService> 
> +GetSpeechRecognitionService(nsresult& aRv)
> +{
> +  nsAutoCString speechRecognitionServiceCID;
> +   

Please remove this extra space.

::: content/media/webspeech/recognition/nsISpeechRecognitionService.idl
@@ +18,5 @@
>      void initialize(in SpeechRecognitionWeakPtr aSpeechRecognition);
>      void processAudioSegment(in AudioSegmentPtr aAudioSegment, in long aSampleRate);
> +        
> +    // This is async , may fail, and will return a Promise soon. 
> +    // Bug 1051130 - Make SpeechGrammarList's addFromURI/addFromString async with Promise 

Please remove the extra spaces in these three lines.

::: content/media/webspeech/recognition/test/FakeSpeechRecognitionService.cpp
@@ +52,5 @@
>  }
>  
> +// This is async , may fail, and will return a Promise soon. 
> +// Bug 1051130 - Make SpeechGrammarList's addFromURI/addFromString async with Promise 
> +// https://bugzilla.mozilla.org/show_bug.cgi?id=1051130

Commenting on the .idl file is enough, please remove this comment.

Comment 14

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

SpeechGrammarList::Constructor returning already_AddRefed makes much sense, if the question is about that.

And yes, your comment about SpeechGrammarList::SpeechGrammarList makes sense.
Better to do the possibly failing thing before calling the ctor.

Comment 15

[Andre Natal]

Attachment: SpeechGrammarList should both initialize SpeechRecognitionService and set a grammar on it

Comment 16

[Andre Natal]

Attachment: SpeechGrammarList should both initialize SpeechRecognitionService and set a grammar on it

Comment 17

[Guilherme Gonçalves [:ggp]]

Comment on attachment 8493506 [details] [diff] [review]
SpeechGrammarList should both initialize SpeechRecognitionService and set a grammar on it

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

Thanks, looks mostly good to me, except for a few nits. Please r? :smaug for the next version of the patch.

::: content/media/webspeech/recognition/SpeechGrammarList.cpp
@@ +19,5 @@
>  NS_IMPL_CYCLE_COLLECTING_ADDREF(SpeechGrammarList)
>  NS_IMPL_CYCLE_COLLECTING_RELEASE(SpeechGrammarList)
>  NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(SpeechGrammarList)
> + NS_WRAPPERCACHE_INTERFACE_MAP_ENTRY
> + NS_INTERFACE_MAP_ENTRY(nsISupports)

Looks like you accidentally removed one space from the indentation level. You shouldn't need to touch these lines at all.

@@ +24,3 @@
>  NS_INTERFACE_MAP_END
>  
> +SpeechGrammarList::SpeechGrammarList(nsISupports* aParent, nsCOMPtr<nsISpeechRecognitionService> aRecognitionService)

I believe you can just receive nsISpeechRecognitionService* here.

@@ +42,5 @@
> +  nsCOMPtr<nsISpeechRecognitionService> mRecognitionService;
> +  mRecognitionService = GetSpeechRecognitionService(rv);
> +  if (!mRecognitionService) {
> +   aRv.Throw(NS_ERROR_NOT_IMPLEMENTED);
> +   return nullptr;

This should be indented one space further.

@@ +46,5 @@
> +   return nullptr;
> +  } else {
> +   nsRefPtr<SpeechGrammarList> speechGrammarList =
> +      new SpeechGrammarList(aGlobal.GetAsSupports(), mRecognitionService);
> +   return speechGrammarList.forget();

Same here.

@@ +85,4 @@
>    return;
>  }
>  
> +void SpeechGrammarList::AddFromString(const nsAString& aString,

Please undo this change.

Comment 18

[Andre Natal]

Attachment: SpeechGrammarList should both initialize SpeechRecognitionService and set a grammar on it

Comment 19

[Andre Natal]

Attachment: SpeechGrammarList should both initialize SpeechRecognitionService and set a grammar on it

Comment 20

[Andre Natal]

Comment on attachment 8494611 [details] [diff] [review]
SpeechGrammarList should both initialize SpeechRecognitionService and set a grammar on it

I addressed the last comments from ggp

Comment 21

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

Comment on attachment 8494611 [details] [diff] [review]
SpeechGrammarList should both initialize SpeechRecognitionService and set a grammar on it

>+  nsCOMPtr<nsISpeechRecognitionService> mRecognitionService;
>+  mRecognitionService = do_GetService(speechRecognitionServiceCID.get(), &aRv);
>+  return mRecognitionService;

Only member variables should have m-prefix. So drop m, and use lowercase r.

> NS_INTERFACE_MAP_BEGIN(SpeechRecognition)
>-  NS_INTERFACE_MAP_ENTRY(nsIObserver)
>+NS_INTERFACE_MAP_ENTRY(nsIObserver)
No reason for this change

>+namespace mozilla {
>+  class AudioSegment;
>+
>+  namespace dom {
>+    class SpeechRecognition;
>+    class SpeechRecognitionResultList;
>+    class SpeechGrammarList;
>+  }
>+}
Odd indentation.
Should be like http://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabChild.h#40
which doesn't use indentation.


> native SpeechRecognitionWeakPtr(mozilla::WeakPtr<mozilla::dom::SpeechRecognition>);
> [ptr] native AudioSegmentPtr(mozilla::AudioSegment);
>+[ptr] native SpeechGrammarListPtr(mozilla::dom::SpeechGrammarList);
> 
> [uuid(857f3fa2-a980-4d3e-a959-a2f53af74232)]
> interface nsISpeechRecognitionService : nsISupports {
>     void initialize(in SpeechRecognitionWeakPtr aSpeechRecognition);
>     void processAudioSegment(in AudioSegmentPtr aAudioSegment, in long aSampleRate);
>+    // This is async , may fail, and will return a Promise soon.
>+    // Bug 1051130 - Make SpeechGrammarList's addFromURI/addFromString async with Promise
>+    // https://bugzilla.mozilla.org/show_bug.cgi?id=1051130
>+    void validateAndSetGrammarList(in SpeechGrammarListPtr aSpeechGramarList);
This method probably won't return Promise (since return Promise some an idl method is painful and this interface is C++ only, and Promises are mostly for JS to consume).
If we need the async-handling for C++, this method should probably take some callback object as a parameter, and a method of that object would be called when the grammar is
loaded. Since the method isn't even implemented, perhaps you could add such callback interface already now.

Comment 22

[Andre Natal]

Attachment: SpeechGrammarList should both initialize SpeechRecognitionService and set a grammar on it

Comment 23

[Andre Natal]

Attachment: SpeechGrammarList should both initialize SpeechRecognitionService and set a grammar on it

I addressed what we discussed

Comment 24

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

Comment on attachment 8495623 [details] [diff] [review]
SpeechGrammarList should both initialize SpeechRecognitionService and set a grammar on it

>+already_AddRefed<SpeechGrammarList>
> SpeechGrammarList::Constructor(const GlobalObject& aGlobal,
>                                ErrorResult& aRv)
> {
>-  return new SpeechGrammarList(aGlobal.GetAsSupports());
>+  nsresult rv;
>+  nsCOMPtr<nsISpeechRecognitionService> mRecognitionService;
Only member variables should have m-prefix. So recognitionService

>+  mRecognitionService = GetSpeechRecognitionService(rv);
>+  if (!mRecognitionService) {
>+    aRv.Throw(NS_ERROR_NOT_IMPLEMENTED);
NS_ERROR_NOT_AVAILABLE might be a bit better


>+nsCOMPtr<nsISpeechRecognitionService>
>+GetSpeechRecognitionService(nsresult& aRv)
Just drop the aRv param, since you're really use it.
Whenever you use the method, just check that the return value isn't null.

Don't return nsCOMPtr<nsISpeechRecognitionService>, but already_AddRefed<nsISpeechRecognitionService>

>+  nsCOMPtr<nsISpeechRecognitionService> rRecognitionService;
recognitionService


>+  return rRecognitionService;
So assuming already_AddRefed<nsISpeechRecognitionService> would be the return type, 
this would become return recognitionService.forget();
>-  mRecognitionService = do_GetService(speechRecognitionServiceCID.get(), &rv);
>+  mRecognitionService = GetSpeechRecognitionService(rv);
>   NS_ENSURE_SUCCESS_VOID(rv);
...so this would be NS_ENSURE_TRUE_VOID(mRecognitionService);

>+[uuid(374583f0-4507-11e4-a183-164230d1df67)]
>+interface nsICallbackSpeechGrammar : nsISupports {
Perhaps call this nsISpeechGrammarCompilationCallback

>+FakeSpeechRecognitionService::ValidateAndSetGrammarList(mozilla::dom::SpeechGrammarList*, nsICallbackSpeechGrammar* aCallback)
perhaps drop aCallback

Comment 25

[Andre Natal]

Attachment: SpeechGrammarList should both initialize SpeechRecognitionService and set a grammar on it

Comment 26

[Andre Natal]

:ggp, can you please push this patch to try while I do not have permissions for it?

Thanks

Comment 27

[Guilherme Gonçalves [:ggp]]

Try: https://tbpl.mozilla.org/?tree=Try&rev=38fb8b881d15

Comment 28

[Andre Natal]

Attachment: Bug 1060659 - SpeechGrammarList should both initialize SpeechRecognitionService and set a grammar on it r=smaug

:ggp, I fixed that issue in mochitests and all tests passed in my environment. Can you please resubmit to try?

Comment 29

[Guilherme Gonçalves [:ggp]]

Great, it also passes locally for me. https://tbpl.mozilla.org/?tree=Try&rev=7f50e2b8998b

Comment 30

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8b8a64a58627

Comment 31

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8b8a64a58627