Add JS bindings validation logic for non-nullable types

Add JS bindings validation logic for non-nullable types

Added codec types: NullableString, NullableHandle,
NullableArrayOf, NullablePointerTo and removed the
unit test code that disabled the null validation tests.

BUG=405072
R=mpcomplete@chromium.org, yzshen@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=290960

[hansmuller, 2014-08-19 21:13:22]

Added support for validating "nullable" pointers and handles.

[Matt Perry, 2014-08-20 01:23:48]

lgtm

[yzshen1, 2014-08-20 17:04:21]

https://codereview.chromium.org/490623003/diff/40001/mojo/public/js/bindings/...
File mojo/public/js/bindings/validator.js (right):

https://codereview.chromium.org/490623003/diff/40001/mojo/public/js/bindings/...
mojo/public/js/bindings/validator.js:7: ], function(codec) {
I think the indent is a little unusual.
Maybe the previous is correct?

https://codereview.chromium.org/490623003/diff/40001/mojo/public/js/bindings/...
mojo/public/js/bindings/validator.js:37: type instanceof codec.NullableArrayOf
||
wrong indent. (37 - 39)

https://codereview.chromium.org/490623003/diff/40001/mojo/public/js/bindings/...
mojo/public/js/bindings/validator.js:92: Validator.prototype.validateHandle =
function(offset, nullable) {
One thing may worth considering:
Passing nullable at runtime may be unnecessary: we could have two separate
functions:
  validateHandle()
  isHandleValid()

For nullable handle field, we generate code to call validateHandle().
For non-nullable handle field, we generate code to call validateHandle() &&
isHandleValid().

We could consider similar things for structs/arrays, too.

In C++, I did such things because validation is on the performance critical
path. But I don't think I know about javascript performance very well. Maybe it
doesn't matter.

https://codereview.chromium.org/490623003/diff/40001/mojo/public/js/bindings/...
mojo/public/js/bindings/validator.js:267: elementOffset,
elementClass.encodedSize, 0, elementClass, nullable);
wrong indent.

https://codereview.chromium.org/490623003/diff/40001/mojo/public/tools/bindin...
File mojo/public/tools/bindings/generators/mojom_js_generator.py (right):

https://codereview.chromium.org/490623003/diff/40001/mojo/public/tools/bindin...
mojo/public/tools/bindings/generators/mojom_js_generator.py:99: ptrType =
"NullablePointerTo" if mojom.IsNullableKind(kind) else "PointerTo"
please use naming style ptr_type. (here and below)

https://codereview.chromium.org/490623003/diff/40001/mojo/public/tools/bindin...
mojo/public/tools/bindings/generators/mojom_js_generator.py:101: if
mojom.IsAnyArrayKind(kind) and mojom.IsBoolKind(kind.kind):
nit: maybe we could merge this if-block into the next one:
if IsAnyArrayKind():
  array_type = ...
  return "..." % ... IsBoolKind() ...

https://codereview.chromium.org/490623003/diff/40001/mojo/public/tools/bindin...
mojo/public/tools/bindings/generators/mojom_js_generator.py:148: def
JavaScriptNullableParam(pf):
packed_field may be a better name than pf. (like line 144)

https://codereview.chromium.org/490623003/diff/40001/mojo/public/tools/bindin...
mojo/public/tools/bindings/generators/mojom_js_generator.py:154: eltKind =
pf.field.kind.kind
element_kind is actually better than elt_kind according to style guide.

https://codereview.chromium.org/490623003/diff/40001/mojo/public/tools/bindin...
mojo/public/tools/bindings/generators/mojom_js_generator.py:160:
(JavaScriptFieldOffset(pf), eltSize, eltCount, eltType, nullable)
wrong indent.

[hansmuller, 2014-08-20 17:46:31]

Addressed Yuzhu's feedback.

https://codereview.chromium.org/490623003/diff/40001/mojo/public/js/bindings/...
File mojo/public/js/bindings/validator.js (right):

https://codereview.chromium.org/490623003/diff/40001/mojo/public/js/bindings/...
mojo/public/js/bindings/validator.js:7: ], function(codec) {
On 2014/08/20 17:04:20, yzshen1 wrote:
> I think the indent is a little unusual.
> Maybe the previous is correct?

Done.

https://codereview.chromium.org/490623003/diff/40001/mojo/public/js/bindings/...
mojo/public/js/bindings/validator.js:37: type instanceof codec.NullableArrayOf
||
On 2014/08/20 17:04:20, yzshen1 wrote:
> wrong indent. (37 - 39)

Done.

https://codereview.chromium.org/490623003/diff/40001/mojo/public/js/bindings/...
mojo/public/js/bindings/validator.js:92: Validator.prototype.validateHandle =
function(offset, nullable) {
On 2014/08/20 17:04:20, yzshen1 wrote:
> One thing may worth considering:
> Passing nullable at runtime may be unnecessary: we could have two separate
> functions:
>   validateHandle()
>   isHandleValid()
> 
> For nullable handle field, we generate code to call validateHandle().
> For non-nullable handle field, we generate code to call validateHandle() &&
> isHandleValid().
> 
> We could consider similar things for structs/arrays, too.
> 
> In C++, I did such things because validation is on the performance critical
> path. But I don't think I know about javascript performance very well. Maybe
it
> doesn't matter.

I don't know what the performance impact of an additional boolean function
parameter to these functions is for JS either. If it's OK with you, I'd like to
defer JS bindings performance tuning until it can be informed by benchmarks.

https://codereview.chromium.org/490623003/diff/40001/mojo/public/js/bindings/...
mojo/public/js/bindings/validator.js:267: elementOffset,
elementClass.encodedSize, 0, elementClass, nullable);
On 2014/08/20 17:04:20, yzshen1 wrote:
> wrong indent.

Done.

https://codereview.chromium.org/490623003/diff/40001/mojo/public/tools/bindin...
File mojo/public/tools/bindings/generators/mojom_js_generator.py (right):

https://codereview.chromium.org/490623003/diff/40001/mojo/public/tools/bindin...
mojo/public/tools/bindings/generators/mojom_js_generator.py:99: ptrType =
"NullablePointerTo" if mojom.IsNullableKind(kind) else "PointerTo"
On 2014/08/20 17:04:20, yzshen1 wrote:
> please use naming style ptr_type. (here and below)

Done.

https://codereview.chromium.org/490623003/diff/40001/mojo/public/tools/bindin...
mojo/public/tools/bindings/generators/mojom_js_generator.py:101: if
mojom.IsAnyArrayKind(kind) and mojom.IsBoolKind(kind.kind):
On 2014/08/20 17:04:21, yzshen1 wrote:
> nit: maybe we could merge this if-block into the next one:
> if IsAnyArrayKind():
>   array_type = ...
>   return "..." % ... IsBoolKind() ...
>   

Done.

https://codereview.chromium.org/490623003/diff/40001/mojo/public/tools/bindin...
mojo/public/tools/bindings/generators/mojom_js_generator.py:148: def
JavaScriptNullableParam(pf):
On 2014/08/20 17:04:20, yzshen1 wrote:
> packed_field may be a better name than pf. (like line 144)

Done.

https://codereview.chromium.org/490623003/diff/40001/mojo/public/tools/bindin...
mojo/public/tools/bindings/generators/mojom_js_generator.py:154: eltKind =
pf.field.kind.kind
On 2014/08/20 17:04:20, yzshen1 wrote:
> element_kind is actually better than elt_kind according to style guide.

Done.

https://codereview.chromium.org/490623003/diff/40001/mojo/public/tools/bindin...
mojo/public/tools/bindings/generators/mojom_js_generator.py:160:
(JavaScriptFieldOffset(pf), eltSize, eltCount, eltType, nullable)
On 2014/08/20 17:04:20, yzshen1 wrote:
> wrong indent.

Done.

[yzshen1, 2014-08-20 17:54:08]

LGTM

https://codereview.chromium.org/490623003/diff/40001/mojo/public/js/bindings/...
File mojo/public/js/bindings/validator.js (right):

https://codereview.chromium.org/490623003/diff/40001/mojo/public/js/bindings/...
mojo/public/js/bindings/validator.js:92: Validator.prototype.validateHandle =
function(offset, nullable) {
On 2014/08/20 17:46:31, hansmuller wrote:
> On 2014/08/20 17:04:20, yzshen1 wrote:
> > One thing may worth considering:
> > Passing nullable at runtime may be unnecessary: we could have two separate
> > functions:
> >   validateHandle()
> >   isHandleValid()
> > 
> > For nullable handle field, we generate code to call validateHandle().
> > For non-nullable handle field, we generate code to call validateHandle() &&
> > isHandleValid().
> > 
> > We could consider similar things for structs/arrays, too.
> > 
> > In C++, I did such things because validation is on the performance critical
> > path. But I don't think I know about javascript performance very well. Maybe
> it
> > doesn't matter.
> 
> I don't know what the performance impact of an additional boolean function
> parameter to these functions is for JS either. If it's OK with you, I'd like
to
> defer JS bindings performance tuning until it can be informed by benchmarks.

Sounds good.

[hansmuller, 2014-08-20 22:05:49]

The CQ bit was checked by hansmuller@chromium.org

[commit-bot: I haz the power, 2014-08-20 22:07:09]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hansmuller@chromium.org/490623003/80001

[commit-bot: I haz the power, 2014-08-20 23:25:25]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  android_clang_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)

[commit-bot: I haz the power, 2014-08-20 23:27:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-20 23:27:38]

Try jobs failed on following builders:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)

[hansmuller, 2014-08-20 23:57:40]

The CQ bit was checked by hansmuller@chromium.org

[commit-bot: I haz the power, 2014-08-21 00:00:21]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hansmuller@chromium.org/490623003/80001

[commit-bot: I haz the power, 2014-08-21 00:05:45]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  android_clang_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)

[commit-bot: I haz the power, 2014-08-21 00:11:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-21 00:11:12]

Try jobs failed on following builders:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  android_clang_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)

[hansmuller, 2014-08-21 00:37:25]

Committed patchset #3 manually as 290960 (presubmit successful).