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

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied 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 153261: Patch
https://bugs.webkit.org/attachment.cgi?id=153261&action=review

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