[webkit-reviews] review denied: [Bug 80417] Speech JavaScript API: add SpeechGrammar and SpeechGrammarList : [Attachment 130613] 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 80417: Speech JavaScript API: add SpeechGrammar and SpeechGrammarList
https://bugs.webkit.org/show_bug.cgi?id=80417

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

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=130613&action=review


> Source/WebCore/ChangeLog:9
> +	   Implement SpeechGrammar and SpeechGrammarList.
> +	   (Spec:
http://speech-javascript-api-spec.googlecode.com/git/speechapi.html)

Thanks for including the link to the spec.

> Source/WebCore/Modules/speech/SpeechGrammar.h:40
> +    static PassRefPtr<SpeechGrammar> create(); // FIXME: The spec is not
clear on what the constructor should look like.
> +    static PassRefPtr<SpeechGrammar> create(const KURL& src, double weight);


I'd update the spec to let you call the second of these constructors.

> Source/WebCore/Modules/speech/SpeechGrammar.h:42
> +    KURL src() { return m_src; }

KURL src() -> const KURL& src() const

> Source/WebCore/Modules/speech/SpeechGrammar.h:43
> +    void setSrc(const String& src) { m_src = KURL(ParsedURLString, src); }

Using ParsedURLString string isn't correct.  ParsedURLString is only for use
when the string in question was created using KURL::string().  It's more likely
you want to resolve this URL relative to some base.  For example, you can add a
[CallWith=ScriptExecutionContext] to the attribute (I think it works for
attributes), and then resolve the URL relative with context->completeURL(src).

You'll also want to add something to there spec about where to get the base
URL.  I believe HTML5 has a notion of a "active script" or something like that,
which matches up with [CallWith=ScriptExecutionContext].

Alternatively, you can just represent this URL as a DOMString and worry about
resolving it when this object gets used.  I'm now sure how this object gets
used, so I don't know whether there's an obvious base URL to use at that time.

> Source/WebCore/Modules/speech/SpeechGrammarList.cpp:50
> +void SpeechGrammarList::addFromUri(const String& src, double weight)
> +{
> +    m_grammars.append(SpeechGrammar::create(KURL(ParsedURLString, src),
weight));
> +}

Here too, you're miss using ParsedURLString.

> Source/WebCore/Modules/speech/SpeechGrammarList.cpp:54
> +    String urlString = String("data:") +
encodeWithURLEscapeSequences(string);

This doesn't look right.  If this is meant to construct a data URL, you'll need
to include a MIME type in a data URL.  Here are some example data URLs:

data:text/html,<h1>hi</h1>

data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAEAAAABACAI
AAAFSDNYfAAAAaklEQVR42u3XQQrAIAwAQeP%2F%2F6wf8CJBJTK9lnQ7FpHGaOurt1
I34nfH9pMMZAZ8BwMGEvvh%2BBsJCAgICLwIOA8EBAQEBAQEBAQEBK79H5RfIQAAAAA
AAAAAAAAAAAAAAAAAAAAAAID%2FABMSqAfj%2FsLmvAAAAABJRU5ErkJggg%3D%3D

> Source/WebCore/Modules/speech/SpeechGrammarList.idl:32
> +	   getter SpeechGrammar item(in unsigned long index);

Unfortunately, I don't think we support the use of getter in this way (although
we'd liked to).  Instead, you should use
https://trac.webkit.org/wiki/WebKitIDL#IndexedGetter

> LayoutTests/fast/speech/scripted/speechgrammar-basics.html:21
> +    shouldBe("g.weight", "1.0");
> +    shouldBe("g.src", "''");

Please add tests for setting these attributes, including to invalid values (and
both relative and absolute URLs).

> LayoutTests/fast/speech/scripted/speechgrammar-basics.html:28
> +    gs.addFromUri("http://example.tld/grammar", 2);

Please add a test with a relative URL.

> LayoutTests/fast/speech/scripted/speechgrammar-basics.html:34
> +    shouldBe("gs.item(1).src", "'data:foo'");

Here we can see that src is an invalid data URLs.  (Every data URL must have a
, character somewhere.)

> LayoutTests/fast/speech/scripted/speechgrammar-basics.html:35
> +    shouldBe("gs.item(1).weight", "42");

Please add a test that uses the indexed getter:

shouldBe("gs[0].weight", "2")

etc.

Also, please add test for out of bounds index (both positive and negative).