Bug 91743 – Speech JavaScript API: Add the SpeechRecognitionResult.emma attribute

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Hans Wennborg]

Speech JavaScript API: Add the SpeechRecognitionResult.emma attribute

[Hans Wennborg]

Created attachment 153261 [details]
Patch

[Hans Wennborg]

Satish: would you like to take an informal look?

[Adam Barth]

Comment on attachment 153261 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=153261&action=review

> Source/WebCore/Modules/speech/SpeechRecognitionResult.cpp:59
> +    StringBuilder sb;

sb -> please use complete words in variable names.  Perhaps emmaString ?

> Source/WebCore/Modules/speech/SpeechRecognitionResult.cpp:60
> +    sb.append("<emma:emma version=\"1.0\" xmlns:emma=\"http://www.w3.org/2003/04/emma\">");

Building XML by string concatenation is error prone.  How can we be sure we've avoided element and attribute splitting bugs?  As an alternative, we have a very nice XML DOM which we can build the document directly.

> Source/WebCore/Modules/speech/SpeechRecognitionResult.idl:34
> +        readonly attribute Document emma;

There's some tricky issues surrounding the lifetime of JavaScript wrappers here.  You need to make sure that the JavaScript wrappers for this Document tree are kept alive by the SpeechRecognitionResult object.  Otherwise, custom properties that folks add to nodes in this document will "fall off" during garbage collection.

[Adam Barth]

+haraken and arv for issues related to JavaScript wrapper lifetime.

[Kentaro Hara]

Comment on attachment 153261 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=153261&action=review

>> Source/WebCore/Modules/speech/SpeechRecognitionResult.idl:34
>> +        readonly attribute Document emma;
> 
> There's some tricky issues surrounding the lifetime of JavaScript wrappers here.  You need to make sure that the JavaScript wrappers for this Document tree are kept alive by the SpeechRecognitionResult object.  Otherwise, custom properties that folks add to nodes in this document will "fall off" during garbage collection.

If you want to keep emma's DOM tree alive, you need to add an implicit reference from SpeechRecognitionResult's wrapper to emma. Specifically, you need to override visitDOMWrapper() and add the implicit reference by AddImplicitReferences(). (e.g. please look at bindings/v8/custom/V8NodeListCustom.cpp)

[Erik Arvidsson]

Here is the test to add to ensure the wrapper is kept alive:

var result = ...
result.emma.customProperty = 42;
gc();
shouldBe('result.emma.customProperty', '42');

For V8 you need to implement V8SpeechRecognitionResult::visitDOMWrapper and add an implicit reference from the SpeechRecognitionResult to the emma Document.

For JSC you need to implement JSSpeechRecognitionResult::visitChildren.

See https://trac.webkit.org/wiki/WebKitIDL for more details.

[Hans Wennborg]

(In reply to comment #5)
> If you want to keep emma's DOM tree alive, you need to add an implicit reference from SpeechRecognitionResult's wrapper to emma. Specifically, you need to override visitDOMWrapper() and add the implicit reference by AddImplicitReferences(). (e.g. please look at bindings/v8/custom/V8NodeListCustom.cpp)

(In reply to comment #6)
> Here is the test to add to ensure the wrapper is kept alive:
> 
> var result = ...
> result.emma.customProperty = 42;
> gc();
> shouldBe('result.emma.customProperty', '42');

Awesome! Thanks for the pointers.

[Hans Wennborg]

Thanks everyone for the review and comments!

(In reply to comment #3)
> (From update of attachment 153261 [details])
> > Source/WebCore/Modules/speech/SpeechRecognitionResult.cpp:60
> > +    sb.append("<emma:emma version=\"1.0\" xmlns:emma=\"http://www.w3.org/2003/04/emma\">");
> 
> Building XML by string concatenation is error prone.  How can we be sure we've avoided element and attribute splitting bugs?  As an alternative, we have a very nice XML DOM which we can build the document directly.

Done. I'm not very familiar with these interfaces, so please take a look.

> > Source/WebCore/Modules/speech/SpeechRecognitionResult.idl:34
> > +        readonly attribute Document emma;
> 
> There's some tricky issues surrounding the lifetime of JavaScript wrappers here.  You need to make sure that the JavaScript wrappers for this Document tree are kept alive by the SpeechRecognitionResult object.  Otherwise, custom properties that folks add to nodes in this document will "fall off" during garbage collection.

Thanks for catching this! I've added the test that arv suggested, and added a custom visitDOMWrapper function. One thing I'm not sure about is which IDL attributes to add. I looked at arv's change from Bug 90194, and added CustomIsReachable and V8DependentLifetime based on that, but since I don't really know what those mean, it would be great if one of you could verify.

[Hans Wennborg]

Created attachment 153788 [details]
Patch

[Adam Barth]

Comment on attachment 153788 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=153788&action=review

> Source/WebCore/Modules/speech/SpeechRecognitionResult.cpp:62
> +    const char emmaUrl[] = "http://www.w3.org/2003/04/emma";

This looks like a namespace.  I wonder if we should have something like http://trac.webkit.org/browser/trunk/Source/WebCore/html/HTMLTagNames.in to create AtomicStrings for the namespace and the qualified names below...

Do you expect to use these constants in any other functions?

> Source/WebCore/Modules/speech/SpeechRecognitionResult.cpp:65
> +    emma->setAttribute("version", "1.0", ec);

I notice that below you're using a setAttribute function that takes a QualifiedName.  Should we be doing that for this attribute as well?

> Source/WebCore/Modules/speech/SpeechRecognitionResult.cpp:76
> +        RefPtr<SpeechRecognitionAlternative> alternative = m_alternatives[i];

Why a RefPtr?  I would have expected m_alternatives to hold a reference for you.

> Source/WebCore/Modules/speech/SpeechRecognitionResult.cpp:84
> +        interpretation->appendChild(literal);

Can we use literal.release() here to save a ref()/deref() pair?

> Source/WebCore/Modules/speech/SpeechRecognitionResult.cpp:85
> +        oneOf->appendChild(interpretation);

Same with interpretation.release() here.

> Source/WebCore/Modules/speech/SpeechRecognitionResult.cpp:91
> +    m_emma = document;

It's slightly odd that m_emma and emma don't correspond.  I wonder if we should rename document to emma and the existing emma to emmaElement.

> Source/WebCore/bindings/v8/custom/V8SpeechRecognitionResultCustom.cpp:43
> +        v8::V8::AddImplicitReferences(wrapper, &emmaWrapper, 1);

We'll probably need something similar for JSC.

[Adam Barth]

Comment on attachment 153788 [details]
Patch

Please address the minor nits above before landing.  Thanks for iterating on the patch.

[Hans Wennborg]

(In reply to comment #10)
> (From update of attachment 153788 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=153788&action=review
> 
> > Source/WebCore/Modules/speech/SpeechRecognitionResult.cpp:62
> > +    const char emmaUrl[] = "http://www.w3.org/2003/04/emma";
> 
> This looks like a namespace.  I wonder if we should have something like http://trac.webkit.org/browser/trunk/Source/WebCore/html/HTMLTagNames.in to create AtomicStrings for the namespace and the qualified names below...
> 
> Do you expect to use these constants in any other functions?

It is indeed a namespace (renaming it to emmaNamespaceUrl to make it even more clear). I looked at HTMLTagNames.in, but it feels like overkill for this situation. And I don't expect these constant to be used anywhere else in the code.

One thing I could do would be to break out the making of these QualifiedNames into something like emmaQualifiedName(), so the code would become:

  oneOf->setAttribute(emmaQualifiedName("medium"), "acoustic");

Do you think that would be better?

> > Source/WebCore/Modules/speech/SpeechRecognitionResult.cpp:65
> > +    emma->setAttribute("version", "1.0", ec);
> 
> I notice that below you're using a setAttribute function that takes a QualifiedName.  Should we be doing that for this attribute as well?

No, the EMMA spec (http://www.w3.org/TR/emma/#s3.1) doesn't say that the version attribute should be in the emma namespace, so I'm using an unqualified name.

> > Source/WebCore/Modules/speech/SpeechRecognitionResult.cpp:76
> > +        RefPtr<SpeechRecognitionAlternative> alternative = m_alternatives[i];
> 
> Why a RefPtr?  I would have expected m_alternatives to hold a reference for you.
Done.

> > Source/WebCore/Modules/speech/SpeechRecognitionResult.cpp:84
> > +        interpretation->appendChild(literal);
> 
> Can we use literal.release() here to save a ref()/deref() pair?
Done.

> > Source/WebCore/Modules/speech/SpeechRecognitionResult.cpp:85
> > +        oneOf->appendChild(interpretation);
> 
> Same with interpretation.release() here.
Done.

> > Source/WebCore/Modules/speech/SpeechRecognitionResult.cpp:91
> > +    m_emma = document;
> 
> It's slightly odd that m_emma and emma don't correspond.  I wonder if we should rename document to emma and the existing emma to emmaElement.
emmaElement sounds good. Done.

> > Source/WebCore/bindings/v8/custom/V8SpeechRecognitionResultCustom.cpp:43
> > +        v8::V8::AddImplicitReferences(wrapper, &emmaWrapper, 1);
> 
> We'll probably need something similar for JSC.
Right, once a port using JSC picks this up.


(In reply to comment #11)
> (From update of attachment 153788 [details])
> Please address the minor nits above before landing.  Thanks for iterating on the patch.

I've addressed them above. If you think I missed something, I'll be happy to follow up with another patch.

[Hans Wennborg]

Committed r123461: <http://trac.webkit.org/changeset/123461>

[Adam Barth]

> It is indeed a namespace (renaming it to emmaNamespaceUrl to make it even more clear). I looked at HTMLTagNames.in, but it feels like overkill for this situation. And I don't expect these constant to be used anywhere else in the code.

Ok.  Thanks.

> One thing I could do would be to break out the making of these QualifiedNames into something like emmaQualifiedName(), so the code would become:
> 
>   oneOf->setAttribute(emmaQualifiedName("medium"), "acoustic");
> 
> Do you think that would be better?

Yes, that would be great.

> > > Source/WebCore/Modules/speech/SpeechRecognitionResult.cpp:65
> > > +    emma->setAttribute("version", "1.0", ec);
> > 
> > I notice that below you're using a setAttribute function that takes a QualifiedName.  Should we be doing that for this attribute as well?
> 
> No, the EMMA spec (http://www.w3.org/TR/emma/#s3.1) doesn't say that the version attribute should be in the emma namespace, so I'm using an unqualified name.

Great.  Is the lack of namespace covered by your tests?  That seems like something we could easily mess up in the future.

[Hans Wennborg]

(In reply to comment #14)
> > One thing I could do would be to break out the making of these QualifiedNames into something like emmaQualifiedName(), so the code would become:
> > 
> >   oneOf->setAttribute(emmaQualifiedName("medium"), "acoustic");
> > 
> > Do you think that would be better?
> 
> Yes, that would be great.

Doing this in Bug 92232.

> > > > Source/WebCore/Modules/speech/SpeechRecognitionResult.cpp:65
> > > > +    emma->setAttribute("version", "1.0", ec);
> > > 
> > > I notice that below you're using a setAttribute function that takes a QualifiedName.  Should we be doing that for this attribute as well?
> > 
> > No, the EMMA spec (http://www.w3.org/TR/emma/#s3.1) doesn't say that the version attribute should be in the emma namespace, so I'm using an unqualified name.
> 
> Great.  Is the lack of namespace covered by your tests?  That seems like something we could easily mess up in the future.

Yes, it's covered. If I put it in a namespace, the test fails.

[Adam Barth]

> Yes, it's covered. If I put it in a namespace, the test fails.

Awesome.