Closed
Bug 1060659
Opened 10 years ago
Closed 10 years ago
SpeechGrammarList should both initialize SpeechRecognitionService and set a grammar on it
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla35
People
(Reporter: anatal, Assigned: anatal)
References
Details
Attachments
(1 file, 12 obsolete files)
No description provided.
Assignee | ||
Comment 1•10 years ago
|
||
Assignee: nobody → anatal
Attachment #8481647 -
Flags: review?(ggoncalves)
Assignee | ||
Comment 2•10 years ago
|
||
This patch both initialize the SpeechRecogntionService as pass the grammar included to it by setGrammarList method.
Assignee | ||
Comment 3•10 years ago
|
||
Attachment #8481647 -
Attachment is obsolete: true
Attachment #8481647 -
Flags: review?(ggoncalves)
Attachment #8481706 -
Flags: review?(ggoncalves)
Assignee | ||
Updated•10 years ago
|
Comment 4•10 years ago
|
||
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?
Attachment #8481706 -
Flags: review?(ggoncalves)
Assignee | ||
Updated•10 years ago
|
Assignee | ||
Updated•10 years ago
|
Assignee | ||
Comment 5•10 years ago
|
||
(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
Assignee | ||
Comment 6•10 years ago
|
||
Merged with the upstream and issues fixed
Attachment #8481706 -
Attachment is obsolete: true
Attachment #8489470 -
Flags: review?(ggoncalves)
Assignee | ||
Updated•10 years ago
|
Comment 7•10 years ago
|
||
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.
Attachment #8489470 -
Flags: review?(ggoncalves)
Assignee | ||
Comment 8•10 years ago
|
||
Attachment #8489470 -
Attachment is obsolete: true
Assignee | ||
Comment 9•10 years ago
|
||
Attachment #8492491 -
Attachment is obsolete: true
Assignee | ||
Comment 10•10 years ago
|
||
Comment on attachment 8492498 [details] [diff] [review] Bug 1060659 - SpeechGrammarList should both initialize SpeechRecognitionService and set a grammar on it I addressed your requests.
Attachment #8492498 -
Flags: review?(ggoncalves)
Assignee | ||
Comment 11•10 years ago
|
||
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•10 years ago
|
||
(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•10 years ago
|
||
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.
Attachment #8492498 -
Flags: review?(ggoncalves) → feedback?(bugs)
Comment 14•10 years ago
|
||
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.
Updated•10 years ago
|
Attachment #8492498 -
Flags: feedback?(bugs)
Assignee | ||
Comment 15•10 years ago
|
||
Attachment #8492498 -
Attachment is obsolete: true
Assignee | ||
Comment 16•10 years ago
|
||
Attachment #8493504 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8493506 -
Flags: review?(ggoncalves)
Comment 17•10 years ago
|
||
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.
Attachment #8493506 -
Flags: review?(ggoncalves) → feedback+
Assignee | ||
Comment 18•10 years ago
|
||
Attachment #8493506 -
Attachment is obsolete: true
Assignee | ||
Comment 19•10 years ago
|
||
Attachment #8494605 -
Attachment is obsolete: true
Assignee | ||
Comment 20•10 years ago
|
||
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
Attachment #8494611 -
Flags: review?(bugs)
Comment 21•10 years ago
|
||
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.
Attachment #8494611 -
Flags: review?(bugs) → review-
Assignee | ||
Comment 22•10 years ago
|
||
Attachment #8494611 -
Attachment is obsolete: true
Assignee | ||
Comment 23•10 years ago
|
||
I addressed what we discussed
Attachment #8495621 -
Attachment is obsolete: true
Attachment #8495623 -
Flags: review?(bugs)
Comment 24•10 years ago
|
||
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
Attachment #8495623 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 25•10 years ago
|
||
Attachment #8495623 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8496618 -
Flags: review?(bugs)
Assignee | ||
Updated•10 years ago
|
Attachment #8496618 -
Flags: review?(bugs)
Assignee | ||
Comment 26•10 years ago
|
||
:ggp, can you please push this patch to try while I do not have permissions for it? Thanks
Flags: needinfo?(ggoncalves)
Comment 27•10 years ago
|
||
Try: https://tbpl.mozilla.org/?tree=Try&rev=38fb8b881d15
Flags: needinfo?(ggoncalves)
Assignee | ||
Comment 28•10 years ago
|
||
:ggp, I fixed that issue in mochitests and all tests passed in my environment. Can you please resubmit to try?
Attachment #8496618 -
Attachment is obsolete: true
Flags: needinfo?(ggoncalves)
Comment 29•10 years ago
|
||
Great, it also passes locally for me. https://tbpl.mozilla.org/?tree=Try&rev=7f50e2b8998b
Flags: needinfo?(ggoncalves)
Updated•10 years ago
|
Keywords: checkin-needed
Comment 30•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b8a64a58627
Keywords: checkin-needed
Comment 31•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/8b8a64a58627
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•