Validate map keys in C++ Mojo bindings

Validate map keys in C++ Mojo bindings

BUG=616649
TESTED=mojo_public_bindings_unittests

Committed: https://crrev.com/29d59af2f65f5c8929521ba0f09e86ae75f26e63
Cr-Commit-Position: refs/heads/master@{#397955}

[tibell, 2016-06-02 01:45:00]

tibell@chromium.org changed reviewers:
+ yzshen@chromium.org

[tibell, 2016-06-02 01:45:01]

I will rename ArrayValidateParams to ContainerValidateParams in a subsequent CL,
to keep this CL smallish.

[yzshen1, 2016-06-02 19:52:28]

https://codereview.chromium.org/2036623002/diff/20001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/map_data_internal.h (right):

https://codereview.chromium.org/2036623002/diff/20001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/map_data_internal.h:26: //
||element_validate_params| members.
"||" -> "|"

https://codereview.chromium.org/2036623002/diff/20001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/validate_params.h (right):

https://codereview.chromium.org/2036623002/diff/20001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/validate_params.h:20: // Validate a map. A map is
validated as a pair of arrays, one for the keys
style nit: Validate -> Validates. Because the sentence's is "[This method]
Validates ..."

And also line 38

https://codereview.chromium.org/2036623002/diff/20001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/validate_params.h:27: : expected_num_elements(0),
Nit: Please consider doing initialization at their definition (i.e., outside of
constructor). So that each constructor only needs to set those fields that are
interesting.

https://codereview.chromium.org/2036623002/diff/20001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/validate_params.h:74: // instance of
ArrayValidateParams (if keys are arrays or maps), or nullptr.
This comment needs to be revised: the param is for the key array itself, it
doesn't matter whether the keys are arrays or maps (actually, keys cannot be
maps. the only type of array allowed as key is string.)

https://codereview.chromium.org/2036623002/diff/20001/mojo/public/interfaces/...
File
mojo/public/interfaces/bindings/tests/data/validation/conformance_mthd16_uknown_non_extensible_enum_map_entry.data
(right):

https://codereview.chromium.org/2036623002/diff/20001/mojo/public/interfaces/...
mojo/public/interfaces/bindings/tests/data/validation/conformance_mthd16_uknown_non_extensible_enum_map_entry.data:32:
[u4]0x5678                  // Unknown value is not allowed for non-extensible
Please consider splitting this test into two, one with only an illegal key and
one with only an illegal value. That way, we can be sure that both key and value
are validated.

[yzshen1, 2016-06-02 19:59:41]

https://codereview.chromium.org/2036623002/diff/20001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/validate_params.h (right):

https://codereview.chromium.org/2036623002/diff/20001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/validate_params.h:77: // Validation information for
elements. It is either a pointer to another
It seems this comment needs to be updated, too.
In the case of array, this is used to validate each individual element (if
elements are arrays or maps);
In the case of map, this is used to validate the entire value array.

[tibell, 2016-06-02 23:55:02]

https://codereview.chromium.org/2036623002/diff/20001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/map_data_internal.h (right):

https://codereview.chromium.org/2036623002/diff/20001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/map_data_internal.h:26: //
||element_validate_params| members.
On 2016/06/02 19:52:28, yzshen1 wrote:
> "||" -> "|"

Done.

https://codereview.chromium.org/2036623002/diff/20001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/validate_params.h (right):

https://codereview.chromium.org/2036623002/diff/20001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/validate_params.h:20: // Validate a map. A map is
validated as a pair of arrays, one for the keys
On 2016/06/02 19:52:28, yzshen1 wrote:
> style nit: Validate -> Validates. Because the sentence's is "[This method]
> Validates ..."
> 
> And also line 38

Done

(Python has the opposite convention.)

https://codereview.chromium.org/2036623002/diff/20001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/validate_params.h:27: : expected_num_elements(0),
On 2016/06/02 19:52:28, yzshen1 wrote:
> Nit: Please consider doing initialization at their definition (i.e., outside
of
> constructor). So that each constructor only needs to set those fields that are
> interesting.

Done.

https://codereview.chromium.org/2036623002/diff/20001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/validate_params.h:74: // instance of
ArrayValidateParams (if keys are arrays or maps), or nullptr.
On 2016/06/02 19:52:28, yzshen1 wrote:
> This comment needs to be revised: the param is for the key array itself, it
> doesn't matter whether the keys are arrays or maps (actually, keys cannot be
> maps. the only type of array allowed as key is string.)

Done.

https://codereview.chromium.org/2036623002/diff/20001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/validate_params.h:77: // Validation information for
elements. It is either a pointer to another
On 2016/06/02 19:59:41, yzshen1 wrote:
> It seems this comment needs to be updated, too.
> In the case of array, this is used to validate each individual element (if
> elements are arrays or maps);
> In the case of map, this is used to validate the entire value array.

Done.

https://codereview.chromium.org/2036623002/diff/20001/mojo/public/interfaces/...
File
mojo/public/interfaces/bindings/tests/data/validation/conformance_mthd16_uknown_non_extensible_enum_map_entry.data
(right):

https://codereview.chromium.org/2036623002/diff/20001/mojo/public/interfaces/...
mojo/public/interfaces/bindings/tests/data/validation/conformance_mthd16_uknown_non_extensible_enum_map_entry.data:32:
[u4]0x5678                  // Unknown value is not allowed for non-extensible
On 2016/06/02 19:52:28, yzshen1 wrote:
> Please consider splitting this test into two, one with only an illegal key and
> one with only an illegal value. That way, we can be sure that both key and
value
> are validated.

Done.

[yzshen1, 2016-06-02 23:57:05]

LGTM

[tibell, 2016-06-03 00:02:25]

The CQ bit was checked by tibell@chromium.org

[commit-bot: I haz the power, 2016-06-03 00:04:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2036623002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2036623002/40001

[commit-bot: I haz the power, 2016-06-03 03:34:02]

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

[commit-bot: I haz the power, 2016-06-03 03:34:03]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[tibell, 2016-06-05 23:30:44]

The CQ bit was checked by tibell@chromium.org

[commit-bot: I haz the power, 2016-06-05 23:31:01]

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

[commit-bot: I haz the power, 2016-06-06 00:41:38]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-06-06 00:44:16]

Description was changed from

==========
Validate map keys in C++ Mojo bindings

BUG=616649
TESTED=mojo_public_bindings_unittests
==========

to

==========
Validate map keys in C++ Mojo bindings

BUG=616649
TESTED=mojo_public_bindings_unittests

Committed: https://crrev.com/29d59af2f65f5c8929521ba0f09e86ae75f26e63
Cr-Commit-Position: refs/heads/master@{#397955}
==========

[commit-bot: I haz the power, 2016-06-06 00:44:17]

Patchset 3 (id:??) landed as
https://crrev.com/29d59af2f65f5c8929521ba0f09e86ae75f26e63
Cr-Commit-Position: refs/heads/master@{#397955}