Changing serviceuri param to be url from string in Blink and tagging requests to speech recognizer

Changing serviceuri param in SpeechRecognition to be url rather than string.
Also tagging the requests coming from browser by prepending chromium to client param in serviceURI, if there is any. 
Any malformed strings/urls will be ignored and not sent further to the speech recognizer.

BUG=480516

[kirtia, 2015-05-01 22:09:08]

kirtia@chromium.org changed reviewers:
+ dmazzoni@chromium.org

[kirtia, 2015-05-01 22:09:08]

Please take a look. 

Thanks!!!

[dmazzoni, 2015-05-04 16:23:57]

dmazzoni@chromium.org changed reviewers:
+ jochen@chromium.org

[dmazzoni, 2015-05-04 16:23:58]

lgtm for Source/modules/speech

+jochen for public/web

[jochen (gone - plz use gerrit), 2015-05-05 13:57:54]

can you add a layout test for this please?

please also update the cl description such that the first line is less than 80c
(for better git logs)

https://codereview.chromium.org/1117383002/diff/40001/Source/modules/speech/S...
File Source/modules/speech/SpeechRecognition.cpp (right):

https://codereview.chromium.org/1117383002/diff/40001/Source/modules/speech/S...
Source/modules/speech/SpeechRecognition.cpp:65:
exceptionState.throwDOMException(SyntaxError, "serviceuri is invalid");
the attribute is "serviceURI". also, please add a more helpful message,
something like "is neither empty nor a valid URL"

https://codereview.chromium.org/1117383002/diff/40001/Source/modules/speech/S...
File Source/modules/speech/SpeechRecognitionController.h (right):

https://codereview.chromium.org/1117383002/diff/40001/Source/modules/speech/S...
Source/modules/speech/SpeechRecognitionController.h:31: #include
"platform/weborigin/KURL.h"
should be enough to forward declare KURL, no?

https://codereview.chromium.org/1117383002/diff/40001/Source/web/SpeechRecogn...
File Source/web/SpeechRecognitionClientProxy.cpp (right):

https://codereview.chromium.org/1117383002/diff/40001/Source/web/SpeechRecogn...
Source/web/SpeechRecognitionClientProxy.cpp:37: #include
"platform/weborigin/KURL.h"
probably not needed?

https://codereview.chromium.org/1117383002/diff/40001/Source/web/SpeechRecogn...
File Source/web/SpeechRecognitionClientProxy.h (right):

https://codereview.chromium.org/1117383002/diff/40001/Source/web/SpeechRecogn...
Source/web/SpeechRecognitionClientProxy.h:30: #include
"platform/weborigin/KURL.h"
same here

[kirtia, 2015-05-05 23:12:30]

Thanks so much! PTAL =)

https://codereview.chromium.org/1117383002/diff/40001/Source/modules/speech/S...
File Source/modules/speech/SpeechRecognition.cpp (right):

https://codereview.chromium.org/1117383002/diff/40001/Source/modules/speech/S...
Source/modules/speech/SpeechRecognition.cpp:65:
exceptionState.throwDOMException(SyntaxError, "serviceuri is invalid");
On 2015/05/05 13:57:53, jochen wrote:
> the attribute is "serviceURI". also, please add a more helpful message,
> something like "is neither empty nor a valid URL"

Done.

https://codereview.chromium.org/1117383002/diff/40001/Source/modules/speech/S...
File Source/modules/speech/SpeechRecognitionController.h (right):

https://codereview.chromium.org/1117383002/diff/40001/Source/modules/speech/S...
Source/modules/speech/SpeechRecognitionController.h:31: #include
"platform/weborigin/KURL.h"
On 2015/05/05 13:57:53, jochen wrote:
> should be enough to forward declare KURL, no?

Done. Makes sense!

https://codereview.chromium.org/1117383002/diff/40001/Source/web/SpeechRecogn...
File Source/web/SpeechRecognitionClientProxy.cpp (right):

https://codereview.chromium.org/1117383002/diff/40001/Source/web/SpeechRecogn...
Source/web/SpeechRecognitionClientProxy.cpp:37: #include
"platform/weborigin/KURL.h"
On 2015/05/05 13:57:53, jochen wrote:
> probably not needed?

Done. Yeah! Sorry about that :-)

https://codereview.chromium.org/1117383002/diff/40001/Source/web/SpeechRecogn...
File Source/web/SpeechRecognitionClientProxy.h (right):

https://codereview.chromium.org/1117383002/diff/40001/Source/web/SpeechRecogn...
Source/web/SpeechRecognitionClientProxy.h:30: #include
"platform/weborigin/KURL.h"
On 2015/05/05 13:57:53, jochen wrote:
> same here

Done.

[jochen (gone - plz use gerrit), 2015-05-06 13:03:58]

jochen@chromium.org changed reviewers:
+ gshires@chromium.org, tommi@chromium.org

[jochen (gone - plz use gerrit), 2015-05-06 13:03:59]

+tommi/gshires for spec question

https://codereview.chromium.org/1117383002/diff/60001/Source/modules/speech/S...
File Source/modules/speech/SpeechRecognition.cpp (right):

https://codereview.chromium.org/1117383002/diff/60001/Source/modules/speech/S...
Source/modules/speech/SpeechRecognition.cpp:63: // Throw an error if serviceuri
is not empty and url is invalid.
nit. that comment doesn't add value, just omit it

https://codereview.chromium.org/1117383002/diff/60001/Source/modules/speech/S...
Source/modules/speech/SpeechRecognition.cpp:65:
exceptionState.throwDOMException(SyntaxError, "serviceURI is neither empty nor a
valid URL.");
i just double-checked the spec, and it doesn't define what should happen here.
Can we update the spec to match this behavior?

[kirtia, 2015-05-06 14:57:13]

Thanks for the quick review! PTAL :-)

I have made a couple of changes in the test as well after consulting with
gshires. :-)

https://codereview.chromium.org/1117383002/diff/60001/Source/modules/speech/S...
File Source/modules/speech/SpeechRecognition.cpp (right):

https://codereview.chromium.org/1117383002/diff/60001/Source/modules/speech/S...
Source/modules/speech/SpeechRecognition.cpp:63: // Throw an error if serviceuri
is not empty and url is invalid.
On 2015/05/06 13:03:58, jochen wrote:
> nit. that comment doesn't add value, just omit it

Done.

[jochen (gone - plz use gerrit), 2015-05-07 11:50:38]

what's gshires' take on the spec question?

[gshires1, 2015-05-07 21:00:39]

On 2015/05/07 11:50:38, jochen wrote:
> what's gshires' take on the spec question?

I agree that the spec doesn't define what should happen if the serviceURI is
neither empty nor a valid URL.

The spec's IDL defines serviceURI as a DOMString.
https://dvcs.w3.org/hg/speech-api/raw-file/tip/webspeechapi.html#speechreco-s...

The spec's text does state that "the serviceURI is a generic URI" but doesn't
define what happens when the serviceURI is not a legally formatted URI.  (It
does imply that the interpretation of some URIs are user-agent dependent,
implying that unrecognized URIs should be ignored: "URN with meaning to the user
agent or by specifying a URL that the user agent recognizes as a local
service").
https://dvcs.w3.org/hg/speech-api/raw-file/tip/webspeechapi.html#dfn-serviceuri

How is this typically handled for HTML5 APIs? Is an exception typically thrown
when a string that should be a URI is not a valid URI?

[domenic, 2015-05-07 21:22:51]

On 2015/05/07 at 21:00:39, gshires wrote:
> On 2015/05/07 11:50:38, jochen wrote:
> > what's gshires' take on the spec question?
> 
> I agree that the spec doesn't define what should happen if the serviceURI is
neither empty nor a valid URL.

It does; it's just a DOMString, so nothing happens.

> How is this typically handled for HTML5 APIs? Is an exception typically thrown
when a string that should be a URI is not a valid URI?

Typically no. If it were to be handled that way, the setter definition for
serviceURI would need to call it out. But there's not really any concept of
"valid URI" in the platform; pretty much any string can be parsed as a URL.

[esprehn, 2015-05-08 19:41:05]

Please bring this to the standards body first. We shouldn't be inventing
vendor:// urls for modules. How will Firefox use this service? Is the protocol
for talking to the service standardized?

[gshires1, 2015-05-20 20:52:23]

domenic #12: So instead of throwing an exception, do you recommend that it
ignore any string that is not parseable? (And perform the recognition as
normal.)

esprehn #13: In the Web Speech API spec, the interpretation of the serviceURI is
user-agent dependent. Thus we can/should use a "chrome://" url (and we should
not invent a "vendor://" url.)

[gshires1, 2015-05-20 20:55:09]

https://codereview.chromium.org/1117383002/diff/80001/LayoutTests/fast/speech...
File LayoutTests/fast/speech/scripted/speechrecognition-basics.html (right):

https://codereview.chromium.org/1117383002/diff/80001/LayoutTests/fast/speech...
LayoutTests/fast/speech/scripted/speechrecognition-basics.html:33:
evalAndLog("r.serviceURI =
'vendor://com.google?foo=bar&client=chromium/com.example'");
Change this to "chrome://speech-recognition?foo=bar&client=com.example"
(Please also change corresponding instances elsewhere in this patch.)

[kirtia, 2015-05-26 21:42:14]

On 2015/05/20 20:52:23, gshires1 wrote:
> domenic #12: So instead of throwing an exception, do you recommend that it
> ignore any string that is not parseable? (And perform the recognition as
> normal.)
> 
> esprehn #13: In the Web Speech API spec, the interpretation of the serviceURI
is
> user-agent dependent. Thus we can/should use a "chrome://" url (and we should
> not invent a "vendor://" url.)

@domenic, friendly ping. =)

[jochen (gone - plz use gerrit), 2015-05-27 15:01:28]

On 2015/05/26 at 21:42:14, kirtia wrote:
> On 2015/05/20 20:52:23, gshires1 wrote:
> > domenic #12: So instead of throwing an exception, do you recommend that it
> > ignore any string that is not parseable? (And perform the recognition as
> > normal.)
> > 
> > esprehn #13: In the Web Speech API spec, the interpretation of the
serviceURI is
> > user-agent dependent. Thus we can/should use a "chrome://" url (and we
should
> > not invent a "vendor://" url.)
> 
> @domenic, friendly ping. =)

since it's changing web facing behavior nevertheless (at least the exception is
not specified), this change should be reflected in the spec, and it should have
an "intent to ship" mail on blink-dev
http://www.chromium.org/blink#TOC-Web-Platform-Changes:-Process

[kirtia, 2015-06-02 20:36:24]

Patchset #6 (id:100001) has been deleted

[kirtia, 2015-06-02 20:56:58]

Please take another look.

Thanks!
K

https://codereview.chromium.org/1117383002/diff/60001/Source/modules/speech/S...
File Source/modules/speech/SpeechRecognition.cpp (right):

https://codereview.chromium.org/1117383002/diff/60001/Source/modules/speech/S...
Source/modules/speech/SpeechRecognition.cpp:65:
exceptionState.throwDOMException(SyntaxError, "serviceURI is neither empty nor a
valid URL.");
On 2015/05/06 13:03:58, jochen wrote:
> i just double-checked the spec, and it doesn't define what should happen here.
> Can we update the spec to match this behavior?

Ack. It has been decided that we ignore malformed urls so the web observable
behavior does not change.

https://codereview.chromium.org/1117383002/diff/80001/LayoutTests/fast/speech...
File LayoutTests/fast/speech/scripted/speechrecognition-basics.html (right):

https://codereview.chromium.org/1117383002/diff/80001/LayoutTests/fast/speech...
LayoutTests/fast/speech/scripted/speechrecognition-basics.html:33:
evalAndLog("r.serviceURI =
'vendor://com.google?foo=bar&client=chromium/com.example'");
On 2015/05/20 20:55:09, gshires1 wrote:
> Change this to "chrome://speech-recognition?foo=bar&client=com.example"
> (Please also change corresponding instances elsewhere in this patch.)

Done.

[jochen (gone - plz use gerrit), 2015-06-05 12:08:03]

(waiting for the result on the blink-dev thread)