[webkit-reviews] review granted: [Bug 91743] Speech JavaScript API: Add the SpeechRecognitionResult.emma attribute : [Attachment 153788] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has granted Hans Wennborg <hans at chromium.org>'s
request for review:
Bug 91743: Speech JavaScript API: Add the SpeechRecognitionResult.emma
attribute
https://bugs.webkit.org/show_bug.cgi?id=91743

Attachment 153788: Patch
https://bugs.webkit.org/attachment.cgi?id=153788&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
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.