Add more type validation to codec.js

Add more type validation to codec.js

Also fixed array validation to use
instanceof per Hans' request in
https://codereview.chromium.org/798163002/

R=hansmuller@chromium.org
BUG=

Committed: https://chromium.googlesource.com/external/mojo/+/1a12b3f231de825a06be026528cc43c2049be7d6

[eseidel, 2014-12-15 23:49:46]

Hopefully my test case is not too complex.  I was trying to avoid copy/pasta but
I'm not sure what I wrote is any clearer.

[hansmuller, 2014-12-16 01:10:41]

LGTM with a nit.

https://codereview.chromium.org/803173003/diff/1/mojo/public/js/codec.js
File mojo/public/js/codec.js (right):

https://codereview.chromium.org/803173003/diff/1/mojo/public/js/codec.js#newc...
mojo/public/js/codec.js:375: if (typeof(val) !== "string") {
Don't you mean primitive strings?

[eseidel, 2014-12-16 01:13:04]

https://codereview.chromium.org/803173003/diff/1/mojo/public/js/codec.js
File mojo/public/js/codec.js (right):

https://codereview.chromium.org/803173003/diff/1/mojo/public/js/codec.js#newc...
mojo/public/js/codec.js:375: if (typeof(val) !== "string") {
On 2014/12/16 01:10:41, hansmuller wrote:
> Don't you mean primitive strings? 
> 

I'm not sure what you mean.

I mean string literals, like "foo" vs. new String("foo").

I should probably should have written "string literals" isntead of "literal
strings" if that's what you mean?

[eseidel, 2014-12-16 01:18:16]

Committed patchset #2 (id:20001) manually as
1a12b3f231de825a06be026528cc43c2049be7d6 (presubmit successful).

[hansmuller, 2014-12-16 15:38:27]

On 2014/12/16 01:13:04, eseidel wrote:
> https://codereview.chromium.org/803173003/diff/1/mojo/public/js/codec.js
> File mojo/public/js/codec.js (right):
> 
>
https://codereview.chromium.org/803173003/diff/1/mojo/public/js/codec.js#newc...
> mojo/public/js/codec.js:375: if (typeof(val) !== "string") {
> On 2014/12/16 01:10:41, hansmuller wrote:
> > Don't you mean primitive strings? 
> > 
> 
> I'm not sure what you mean.
> 
> I mean string literals, like "foo" vs. new String("foo").
> 
> I should probably should have written "string literals" isntead of "literal
> strings" if that's what you mean?

I'm not passing a string literal in Mojo method call like
foo("foo".substring(0)). 

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Obje...

String literals (denoted by double or single quotes) and strings returned from
String calls in a non-constructor context (i.e., without using the new keyword)
are primitive strings.

[eseidel, 2014-12-16 17:09:45]

OK.  Will fix the comment.  Thanks!

[eseidel, 2014-12-16 17:13:37]

Done:
https://codereview.chromium.org/807873002

[Ryan Sleevi, 2014-12-30 04:54:16]

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

[Ryan Sleevi, 2014-12-30 04:54:17]

+dcheng, since this blocked a Chromium roll of Mojo (which as depending on this
behaviour)

In this case, the broken test was doing
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/test/da...


This worked in previous iterations because ArrayBufferView is an array-like
object, but fails now due to the more stringent requirements.

https://codereview.chromium.org/803173003/diff/20001/mojo/public/js/codec.js
File mojo/public/js/codec.js (right):

https://codereview.chromium.org/803173003/diff/20001/mojo/public/js/codec.js#...
mojo/public/js/codec.js:360: }
The original code had the benefit that it properly handled ES6 array-like
objects, such as the ArrayBufferView family. This change loses this property,
which has prevented rolling an updated Mojo into Chrome (see
https://codereview.chromium.org/795593004 )

This may be what you desired, but it's certainly not intuitive when using JS
bindings.

https://people.mozilla.org/~jorendorff/es6-draft.html#sec-createlistfromarray...
is the canonical ES6 algorithm for handling array-like objects into sequenced
lists, and it seems useful here when talking about IDL bindings.

https://codereview.chromium.org/803173003/diff/20001/mojo/public/js/codec.js#...
mojo/public/js/codec.js:403: if (!(val instanceof Map)) {
This is another surprising change, as it prevents map-like objects and iterables
from working. Again, this may be desirable, but it certainly strikes as
surprising.

[hansmuller1, 2014-12-30 16:08:18]

On 2014/12/30 04:54:17, Ryan Sleevi wrote:
> +dcheng, since this blocked a Chromium roll of Mojo (which as depending on
this
> behaviour)
> 
> In this case, the broken test was doing
>
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/test/da...
> 
> 
> This worked in previous iterations because ArrayBufferView is an array-like
> object, but fails now due to the more stringent requirements.
> 
> https://codereview.chromium.org/803173003/diff/20001/mojo/public/js/codec.js
> File mojo/public/js/codec.js (right):
> 
>
https://codereview.chromium.org/803173003/diff/20001/mojo/public/js/codec.js#...
> mojo/public/js/codec.js:360: }
> The original code had the benefit that it properly handled ES6 array-like
> objects, such as the ArrayBufferView family. This change loses this property,
> which has prevented rolling an updated Mojo into Chrome (see
> https://codereview.chromium.org/795593004 )
> 
> This may be what you desired, but it's certainly not intuitive when using JS
> bindings.
> 
>
https://people.mozilla.org/~jorendorff/es6-draft.html#sec-createlistfromarray...
> is the canonical ES6 algorithm for handling array-like objects into sequenced
> lists, and it seems useful here when talking about IDL bindings.
> 
>
https://codereview.chromium.org/803173003/diff/20001/mojo/public/js/codec.js#...
> mojo/public/js/codec.js:403: if (!(val instanceof Map)) {
> This is another surprising change, as it prevents map-like objects and
iterables
> from working. Again, this may be desirable, but it certainly strikes as
> surprising.

The surprising changes you're referring to were made because the failures caused
by providing map/array values that were insufficiently map or array "like" were
difficult to diagnose. Surprising even.

I agree that it would be useful to loosen the requirements for map and array
values. 

This CL wasn't supposed to be backwards incompatible, so for now I'll just
weaken the type checking a little.