Add layout tests that test connecting to a system supplied navigator.connect service.

Add layout tests that test connecting to a system supplied navigator.connect service.

BUG=426458
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=191230

[scheib, 2015-02-26 23:02:31]

scheib@chromium.org changed reviewers:
+ scheib@chromium.org

[scheib, 2015-02-26 23:02:32]

These are dense and a bit hard to read. Generally I think looking OK, but some
issues pointed out that confused me.

https://codereview.chromium.org/940423004/diff/1/LayoutTests/http/tests/navig...
File LayoutTests/http/tests/navigatorconnect/system-service.html (right):

https://codereview.chromium.org/940423004/diff/1/LayoutTests/http/tests/navig...
LayoutTests/http/tests/navigatorconnect/system-service.html:16: function
wait_for_reply(t, port) {
Consider using 'test' instead of 't' for helper parameter readability.

https://codereview.chromium.org/940423004/diff/1/LayoutTests/http/tests/navig...
LayoutTests/http/tests/navigatorconnect/system-service.html:30: function
convert_dates(k, v) {
Use more descriptive parameter names than k, v.

https://codereview.chromium.org/940423004/diff/1/LayoutTests/http/tests/navig...
LayoutTests/http/tests/navigatorconnect/system-service.html:33: return
this[k].getTime() / 1000;
I think document more clearly, a Date object converted to a base::Value will be
returned as number of seconds since 1 January 1970 00:00:00 UTC. Cite the
offending code or conversion if possible. Should this be fixed ever? (if so
create an issue)

https://codereview.chromium.org/940423004/diff/1/LayoutTests/http/tests/navig...
LayoutTests/http/tests/navigatorconnect/system-service.html:78:
promise_test(test_sending_messages.bind(undefined, echo_service_url +
'?as-string',
The query strings '?as-string' '?as-value' don't seem to be implemented by
https://codereview.chromium.org/938403005/

https://codereview.chromium.org/940423004/diff/1/LayoutTests/http/tests/navig...
LayoutTests/http/tests/navigatorconnect/system-service.html:79:
compare_as_json.bind(undefined, undefined)),
Isn't the second undefined bound to compare_as_json becoming the the 'port'
parameter?

https://codereview.chromium.org/940423004/diff/1/LayoutTests/http/tests/navig...
LayoutTests/http/tests/navigatorconnect/system-service.html:80: 'Messages can be
sent and received to a system service when sent as strings.');
Did 't' need to be passed in to test_sending_messages?

https://codereview.chromium.org/940423004/diff/1/LayoutTests/http/tests/navig...
LayoutTests/http/tests/navigatorconnect/system-service.html:82:
promise_test(test_sending_messages.bind(undefined, echo_service_url +
'?as-value',
These may be cleaner to read by just using duplicating the 3 lines of
test_sending_messages twice.

https://codereview.chromium.org/940423004/diff/1/LayoutTests/http/tests/navig...
LayoutTests/http/tests/navigatorconnect/system-service.html:119: }, 'Messages
are only send as values when the service asks for it.');
s/send/sent/

[Marijn Kruisselbrink, 2015-02-27 23:04:58]

https://codereview.chromium.org/940423004/diff/1/LayoutTests/http/tests/navig...
File LayoutTests/http/tests/navigatorconnect/system-service.html (right):

https://codereview.chromium.org/940423004/diff/1/LayoutTests/http/tests/navig...
LayoutTests/http/tests/navigatorconnect/system-service.html:16: function
wait_for_reply(t, port) {
On 2015/02/26 23:02:32, scheib wrote:
> Consider using 'test' instead of 't' for helper parameter readability.

Done.

https://codereview.chromium.org/940423004/diff/1/LayoutTests/http/tests/navig...
LayoutTests/http/tests/navigatorconnect/system-service.html:30: function
convert_dates(k, v) {
On 2015/02/26 23:02:32, scheib wrote:
> Use more descriptive parameter names than k, v.

Done.

https://codereview.chromium.org/940423004/diff/1/LayoutTests/http/tests/navig...
LayoutTests/http/tests/navigatorconnect/system-service.html:33: return
this[k].getTime() / 1000;
On 2015/02/26 23:02:32, scheib wrote:
> I think document more clearly, a Date object converted to a base::Value will
be
> returned as number of seconds since 1 January 1970 00:00:00 UTC. Cite the
> offending code or conversion if possible. Should this be fixed ever? (if so
> create an issue)

I made this a bit more explicit. Not sure if directly pointing to the code that
does this conversion makes sense. Currently V8ValueConverterImpl is what does
this, but hopefully in the future the extra roundtrip can be eliminated, and
some newly written piece of code will end up doing this same conversion.

https://codereview.chromium.org/940423004/diff/1/LayoutTests/http/tests/navig...
LayoutTests/http/tests/navigatorconnect/system-service.html:78:
promise_test(test_sending_messages.bind(undefined, echo_service_url +
'?as-string',
On 2015/02/26 23:02:32, scheib wrote:
> The query strings '?as-string' '?as-value' don't seem to be implemented by
> https://codereview.chromium.org/938403005/

The test wouldn't pass if it wouldn't be implemented. See kAsValue, and its use
in 'client.target_url.query() == kAsValue'

https://codereview.chromium.org/940423004/diff/1/LayoutTests/http/tests/navig...
LayoutTests/http/tests/navigatorconnect/system-service.html:79:
compare_as_json.bind(undefined, undefined)),
On 2015/02/26 23:02:32, scheib wrote:
> Isn't the second undefined bound to compare_as_json becoming the the 'port'
> parameter?

the first undefined is the |this| for compare_as_json, the second undefined is
the |replacer| parameter.

https://codereview.chromium.org/940423004/diff/1/LayoutTests/http/tests/navig...
LayoutTests/http/tests/navigatorconnect/system-service.html:80: 'Messages can be
sent and received to a system service when sent as strings.');
On 2015/02/26 23:02:32, scheib wrote:
> Did 't' need to be passed in to test_sending_messages?

promise_test calls its argument with 't', but as you point out maybe there is a
bit too much magic binding going on here, and it's clearer if I just duplicate
the test_sending_messages

https://codereview.chromium.org/940423004/diff/1/LayoutTests/http/tests/navig...
LayoutTests/http/tests/navigatorconnect/system-service.html:82:
promise_test(test_sending_messages.bind(undefined, echo_service_url +
'?as-value',
On 2015/02/26 23:02:32, scheib wrote:
> These may be cleaner to read by just using duplicating the 3 lines of
> test_sending_messages twice.

Done.

https://codereview.chromium.org/940423004/diff/1/LayoutTests/http/tests/navig...
LayoutTests/http/tests/navigatorconnect/system-service.html:119: }, 'Messages
are only send as values when the service asks for it.');
On 2015/02/26 23:02:32, scheib wrote:
> s/send/sent/

You'd think after so many times making the same mistake I'd figure out by now
when to use send and when to use sent....

[scheib, 2015-02-27 23:53:05]

https://codereview.chromium.org/940423004/diff/1/LayoutTests/http/tests/navig...
File LayoutTests/http/tests/navigatorconnect/system-service.html (right):

https://codereview.chromium.org/940423004/diff/1/LayoutTests/http/tests/navig...
LayoutTests/http/tests/navigatorconnect/system-service.html:78:
promise_test(test_sending_messages.bind(undefined, echo_service_url +
'?as-string',
On 2015/02/27 23:04:58, Marijn Kruisselbrink wrote:
> On 2015/02/26 23:02:32, scheib wrote:
> > The query strings '?as-string' '?as-value' don't seem to be implemented by
> > https://codereview.chromium.org/938403005/
> 
> The test wouldn't pass if it wouldn't be implemented. See kAsValue, and its
use
> in 'client.target_url.query() == kAsValue'

Doh, I had searched for as-string.

https://codereview.chromium.org/940423004/diff/1/LayoutTests/http/tests/navig...
LayoutTests/http/tests/navigatorconnect/system-service.html:79:
compare_as_json.bind(undefined, undefined)),
On 2015/02/27 23:04:58, Marijn Kruisselbrink wrote:
> On 2015/02/26 23:02:32, scheib wrote:
> > Isn't the second undefined bound to compare_as_json becoming the the 'port'
> > parameter?
> 
> the first undefined is the |this| for compare_as_json, the second undefined is
> the |replacer| parameter.

:P I was lost in line wrapped ,)(),,0()). Thanks, much clearer now.

https://codereview.chromium.org/940423004/diff/40001/LayoutTests/http/tests/n...
File LayoutTests/http/tests/navigatorconnect/system-service.html (right):

https://codereview.chromium.org/940423004/diff/40001/LayoutTests/http/tests/n...
LayoutTests/http/tests/navigatorconnect/system-service.html:139:
promise_test(function(t) {
This test appears to be identical to above
'Messages are actually understandable by the browser when sent as values.'.

[Marijn Kruisselbrink, 2015-02-27 23:58:10]

https://codereview.chromium.org/940423004/diff/40001/LayoutTests/http/tests/n...
File LayoutTests/http/tests/navigatorconnect/system-service.html (right):

https://codereview.chromium.org/940423004/diff/40001/LayoutTests/http/tests/n...
LayoutTests/http/tests/navigatorconnect/system-service.html:139:
promise_test(function(t) {
On 2015/02/27 23:53:05, scheib wrote:
> This test appears to be identical to above
> 'Messages are actually understandable by the browser when sent as values.'. 

Ah yes, you're right. Now uploaded the correct test.

[scheib, 2015-02-27 23:59:53]

lgtm

[Marijn Kruisselbrink, 2015-03-03 18:42:56]

The CQ bit was checked by mek@chromium.org

[commit-bot: I haz the power, 2015-03-03 18:45:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/940423004/60001

[commit-bot: I haz the power, 2015-03-04 04:13:46]

Committed patchset #4 (id:60001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=191230