Bug 91743

Summary: Speech JavaScript API: Add the SpeechRecognitionResult.emma attribute
Product: WebKit Reporter: Hans Wennborg <hans>
Component: New BugsAssignee: Hans Wennborg <hans>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, arv, haraken, japhet, jochen, ojan, satish, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 80261, 92232    
Attachments:
Description Flags
Patch
none
Patch abarth: review+, abarth: commit-queue-

Description Hans Wennborg 2012-07-19 07:26:11 PDT
Speech JavaScript API: Add the SpeechRecognitionResult.emma attribute
Comment 1 Hans Wennborg 2012-07-19 07:46:33 PDT
Created attachment 153261 [details]
Patch
Comment 2 Hans Wennborg 2012-07-19 07:47:12 PDT
Satish: would you like to take an informal look?
Comment 3 Adam Barth 2012-07-19 09:17:46 PDT
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.
Comment 4 Adam Barth 2012-07-19 09:18:20 PDT
+haraken and arv for issues related to JavaScript wrapper lifetime.
Comment 5 Kentaro Hara 2012-07-19 10:15:43 PDT
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)
Comment 6 Erik Arvidsson 2012-07-19 10:31:20 PDT
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.
Comment 7 Hans Wennborg 2012-07-19 10:49:31 PDT
(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.
Comment 8 Hans Wennborg 2012-07-23 06:32:14 PDT
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.
Comment 9 Hans Wennborg 2012-07-23 06:32:38 PDT
Created attachment 153788 [details]
Patch
Comment 10 Adam Barth 2012-07-23 11:33:59 PDT
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.
Comment 11 Adam Barth 2012-07-23 11:34:22 PDT
Comment on attachment 153788 [details]
Patch

Please address the minor nits above before landing.  Thanks for iterating on the patch.
Comment 12 Hans Wennborg 2012-07-24 04:26:30 PDT
(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.
Comment 13 Hans Wennborg 2012-07-24 05:25:44 PDT
Committed r123461: <http://trac.webkit.org/changeset/123461>
Comment 14 Adam Barth 2012-07-24 10:28:35 PDT
> 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.
Comment 15 Hans Wennborg 2012-07-25 02:54:02 PDT
(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.
Comment 16 Adam Barth 2012-07-25 08:36:58 PDT
> Yes, it's covered. If I put it in a namespace, the test fails.

Awesome.