SpeechGrammarList should use SpeechGrammar to store grammars

RESOLVED DUPLICATE of bug 1051148

Status

()

Core
DOM
RESOLVED DUPLICATE of bug 1051148
4 years ago
3 years ago

People

(Reporter: Andre Natal, Assigned: Andre Natal)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [webspeechapi])

User Story

SpeechGrammarList should store the grammars input by the user in a SpeechGrammar array

Attachments

(1 attachment)

Comment hidden (empty)
(Assignee)

Updated

4 years ago
Blocks: 1067689
(Assignee)

Comment 1

4 years ago
Created attachment 8510679 [details] [diff] [review]
SpeechGrammarList should use SpeechGrammar to store grammars
Assignee: nobody → anatal
Attachment #8510679 - Flags: review?(ggoncalves)
Attachment #8510679 - Flags: review?(bugs)
(Assignee)

Updated

4 years ago
User Story: (updated)
(Assignee)

Updated

4 years ago
User Story: (updated)
Comment on attachment 8510679 [details] [diff] [review]
SpeechGrammarList should use SpeechGrammar to store grammars

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

Thanks! This is very much on the right track.

::: content/media/webspeech/recognition/SpeechGrammar.cpp
@@ +60,5 @@
>  
>  void
>  SpeechGrammar::SetSrc(const nsAString& aArg, ErrorResult& aRv)
>  {
> +  mGram = ToNewUTF8String(aArg);

Note that ToNewUTF8String copies aArg, and you're leaking the copy. Hence my suggestion to use one of our string classes for mGram.

::: content/media/webspeech/recognition/SpeechGrammar.h
@@ +14,5 @@
> +
> +#include "nsISpeechRecognitionService.h"
> +
> +struct JSContext;
> +

Looks like you don't need this.

@@ +54,5 @@
>    ~SpeechGrammar();
>  
>    nsCOMPtr<nsISupports> mParent;
> +
> +  char * mGram;

This should probably be one of our string classes instead of a raw pointer, most likely a nsString (some docs: https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Guide/Internal_strings). I'm not really sure who will own the memory associated with this string, please ask :smaug.

::: content/media/webspeech/recognition/SpeechGrammarList.cpp
@@ +105,5 @@
> +    return nullptr;
> +  }
> +  ErrorResult rv;
> +  aPresent = true;
> +  return Item(aIndex, rv);

A bit of a nit, but I think you can use nsTArray::SafeElementAt to make this a bit simpler. So you'd basically do |nsRefPtr ret = mItems.SafeElementAt(aIndex, nullptr)|, set |aPresent = (ret == nullptr)| and return |ret.forget()|.

Also I'm not really sure what we're supposed to do with aRv here -- is it supposed to be set to an error code when aPresent is false?

::: content/media/webspeech/recognition/SpeechGrammarList.h
@@ +56,5 @@
>    ~SpeechGrammarList();
>  
>    nsCOMPtr<nsISupports> mParent;
> +
> +  nsTArray<nsRefPtr<SpeechGrammar>> mItems;

Nit: mGrammars would be more descriptive.

::: content/media/webspeech/recognition/nsISpeechRecognitionService.idl
@@ +24,3 @@
>  [ptr] native SpeechGrammarPtr(mozilla::dom::SpeechGrammar);
> +[ptr] native SpeechGrammarListPtr(mozilla::dom::SpeechGrammarList);
> +

Do you really need this change?

@@ +34,4 @@
>  interface nsISpeechRecognitionService : nsISupports {
>      void initialize(in SpeechRecognitionWeakPtr aSpeechRecognition);
>      void processAudioSegment(in AudioSegmentPtr aAudioSegment, in long aSampleRate);
> +    void validateAndSetGrammarList(in SpeechGrammarPtr aSpeechGrammar, in nsISpeechGrammarCompilationCallback aCallback);

Why do you no longer want to receive a SpeechGrammarList here? A recognition service could conceivably work with a list of grammars at once, right?

If you have a good reason for this change, please also update the method name (validateAndSetGrammar), otherwise please undo it.
Attachment #8510679 - Flags: review?(ggoncalves) → feedback+
Oh, and you only really need r=smaug in the commit message, please update it :)
Comment on attachment 8510679 [details] [diff] [review]
SpeechGrammarList should use SpeechGrammar to store grammars


>   nsCOMPtr<nsISupports> mParent;
>+
>+  char * mGram;
Why not just nsString mGram;

> already_AddRefed<SpeechGrammar>
> SpeechGrammarList::Item(uint32_t aIndex, ErrorResult& aRv)
> {
>-  aRv.Throw(NS_ERROR_NOT_IMPLEMENTED);
>-  return nullptr;
>+  nsRefPtr<SpeechGrammar> result = mItems.ElementAt(aIndex);
Please check that aIndex points to a valid entry. 
Otherwise you hit http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsTArray.h?rev=c984a4104674&mark=916-916#910
in debug builds, and in optimized builds you just have a security bug.

> SpeechGrammarList::IndexedGetter(uint32_t aIndex, bool& aPresent,
>                                  ErrorResult& aRv)
> {
>-  aRv.Throw(NS_ERROR_NOT_IMPLEMENTED);
>-  return nullptr;
>+  if (aIndex >= Length()) {
>+    aPresent = false;
>+    return nullptr;
>+  }
>+  ErrorResult rv;
>+  aPresent = true;
>+  return Item(aIndex, rv);
... and I guess then you wouldn't need to do if (aIndex >= Length()) { check here since Item() would be safe.
Attachment #8510679 - Flags: review?(bugs) → review-

Updated

3 years ago
Whiteboard: [webspeechapi]

Updated

3 years ago
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1051148
You need to log in before you can comment on or make changes to this bug.