Bug 80417 – Speech JavaScript API: add SpeechGrammar and SpeechGrammarList

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

[Hans Wennborg]

Speech JavaScript API: add SpeechGrammar and SpeechGrammarList

[Hans Wennborg]

Created attachment 130378 [details]
Patch

[Hans Wennborg]

Created attachment 130586 [details]
Patch

Rebase and link to spec in the ChangeLog.

[Satish Sampath]

Comment on attachment 130586 [details]
Patch

> Source/WebCore/Modules/speech/SpeechGrammar.idl: 31
> attribute DOMString src;

should this be "attribute [URL] DOMString src" similar to src in HTMLImageElement.idl ? If you agree, should the 'src()' method in the code return a KURL as it does for image?

> Source/WebCore/Modules/speech/SpeechGrammar.h: 38
> static PassRefPtr<SpeechGrammar> create(); // FIXME:

The spec doesn't mention constructing a SpeechGrammer as a standalone object, instead there are methods in SpeechGrammarList to add from uri and string. So this method could be removed and added later if required. FWIW I think this would be useful but would need SpeechGrammarList to also support add/remove items. That should perhaps be discussed at the spec level.

If you agree, seems like it should also be removed from DOMWindowSpeech.idl

> Source/WebCore/Modules/speech/SpeechGrammarList.cpp: 42
> return 0; // FIXME: Throw exception?

referring to an invalid index typically returns 'undefined'.

> Source/WebCore/Modules/speech/SpeechGrammarList.cpp: 55
> m_grammars.append(SpeechGrammar::create(String("data:") + string, weight));

Do you need to urlencode 'string' when adding it to the data url?

[Hans Wennborg]

(In reply to comment #3)
> (From update of attachment 130586 [details])
> > Source/WebCore/Modules/speech/SpeechGrammar.idl: 31
> > attribute DOMString src;
> 
> should this be "attribute [URL] DOMString src" similar to src in HTMLImageElement.idl ? If you agree, should the 'src()' method in the code return a KURL as it does for image?

Yes, this sounds reasonable. Done.

> > Source/WebCore/Modules/speech/SpeechGrammar.h: 38
> > static PassRefPtr<SpeechGrammar> create(); // FIXME:
> 
> The spec doesn't mention constructing a SpeechGrammer as a standalone object, instead there are methods in SpeechGrammarList to add from uri and string. So this method could be removed and added later if required. FWIW I think this would be useful but would need SpeechGrammarList to also support add/remove items. That should perhaps be discussed at the spec level.

The IDL in the spec does say that SpeechGrammar should have a constructor. But as you say, it doesn't otherwise discuss it, or provide methods to add/remove a SpeechGrammar object to a SpeechGrammarList.

I'd like to keep this with the FIXME and revisit when this is resolved in the spec.

> > Source/WebCore/Modules/speech/SpeechGrammarList.cpp: 42
> > return 0; // FIXME: Throw exception?
> 
> referring to an invalid index typically returns 'undefined'.

Does it? DOMStringList.item, TouchList.item, NamedNodeMap.item and NodeList.item all seem to return null?

Removing the FIXME, though.

> > Source/WebCore/Modules/speech/SpeechGrammarList.cpp: 55
> > m_grammars.append(SpeechGrammar::create(String("data:") + string, weight));
> 
> Do you need to urlencode 'string' when adding it to the data url?

Done.

[Hans Wennborg]

Created attachment 130613 [details]
Patch

[Satish Sampath]

Looks good.

>> referring to an invalid index typically returns 'undefined'.
>
> Does it? DOMStringList.item, TouchList.item, NamedNodeMap.item and NodeList.item all seem to return null?
>
> Removing the FIXME, though.

Yes I meant we can remove the FIXME

[Adam Barth]

Comment on attachment 130613 [details]
Patch

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).

[Hans Wennborg]

(In reply to comment #7)
> (From update of attachment 130613 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=130613&action=review

Thanks very much for the review, Adam!

> > 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.

Sounds reasonable. I'd like to have the spec sort all of this out (the constructor, should we have add/remove on the list object, should the attributes on the grammar be readonly or not). For now, I'd like to just keep the FIXME here.

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

> 
> > 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).

Thanks. I think we should just resolve it relative to the base of the Document when the URLs are passed in. I'll get the spec updated.

> > 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.

The spec is a bit vague (to say the least) about the grammar bit, but if I understand correctly, it should be in the XML format of http://www.w3.org/TR/speech-grammar/. I'll get the spec updated.

So I think "application/xml" should be the appropriate MIME type.

It will be the speech server's responsibility to reject any invalid grammars.

> > 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
Done.

> > 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).
Done.

> > LayoutTests/fast/speech/scripted/speechgrammar-basics.html:28
> > +    gs.addFromUri("http://example.tld/grammar", 2);
> 
> Please add a test with a relative URL.
Done.

> > 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.
Done.

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

[Hans Wennborg]

Created attachment 130814 [details]
Patch

[Adam Barth]

Comment on attachment 130814 [details]
Patch

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

Looks great.  Thanks.

> Source/WebCore/Modules/speech/SpeechGrammarList.idl:29
> +        IndexedGetter

Looks like you're missing a , after IndexedGetter

[Hans Wennborg]

(In reply to comment #10)
> (From update of attachment 130814 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=130814&action=review
> 
> Looks great.  Thanks.
> 
> > Source/WebCore/Modules/speech/SpeechGrammarList.idl:29
> > +        IndexedGetter
> 
> Looks like you're missing a , after IndexedGetter
Done.

[Hans Wennborg]

Committed r110281: <http://trac.webkit.org/changeset/110281>