The default value for NullableString should be null (was "")

The default value for NullableString should be null (was ""). Mojo struct fields whose type is string? should be initialized to null, not "".

BUG=417039
Committed: https://crrev.com/ae685fa448164fef86b0b97f519ac2f81cc8b310
Cr-Commit-Position: refs/heads/master@{#296612}

[hansmuller, 2014-09-24 19:18:42]

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

[hansmuller, 2014-09-24 19:27:56]

This turned out to be a smaller fix than I'd expected. PTAL when you get a
chance.

[yzshen1, 2014-09-24 19:41:56]

https://codereview.chromium.org/605443002/diff/1/mojo/public/tools/bindings/g...
File mojo/public/tools/bindings/generators/mojom_js_generator.py (right):

https://codereview.chromium.org/605443002/diff/1/mojo/public/tools/bindings/g...
mojo/public/tools/bindings/generators/mojom_js_generator.py:34: mojom.STRING:   
            '""',
I think if a default value is unspecified, this should be initialized to null,
too. (according to the current design proposal.)

Nullable or not is purely for validation purpose. It has nothing to do with
initialization. So a string/struct/array/handle is non-nullable doesn't mean it
is initialized to non-null, intead it means that the user needs to set a value
for it before sending it on a pipe.

What do you think? Thanks!

[hansmuller, 2014-09-24 22:12:07]

I added some simple struct tests to verify that all field types are initialized
to their expected default values and that all of the types that can specify an
explicit field value handle that correctly as well. Doing so turned up a small
bug in the C++ bindings: the float "f" suffix was being added to decimal numbers
and the result would not compile. I fixed that as well.

https://codereview.chromium.org/605443002/diff/1/mojo/public/tools/bindings/g...
File mojo/public/tools/bindings/generators/mojom_js_generator.py (right):

https://codereview.chromium.org/605443002/diff/1/mojo/public/tools/bindings/g...
mojo/public/tools/bindings/generators/mojom_js_generator.py:34: mojom.STRING:   
            '""',
On 2014/09/24 19:41:56, yzshen1 wrote:
> I think if a default value is unspecified, this should be initialized to null,
> too. (according to the current design proposal.)
> 
> Nullable or not is purely for validation purpose. It has nothing to do with
> initialization. So a string/struct/array/handle is non-nullable doesn't mean
it
> is initialized to non-null, intead it means that the user needs to set a value
> for it before sending it on a pipe.
> 
> What do you think? Thanks!

OK, I agree with this. Both kinds of String fields are now null by default.

[yzshen1, 2014-09-24 22:49:25]

LGTM with a few nits


Thanks! :)

https://codereview.chromium.org/605443002/diff/40001/mojo/public/interfaces/b...
File mojo/public/interfaces/bindings/tests/test_structs.mojom (right):

https://codereview.chromium.org/605443002/diff/40001/mojo/public/interfaces/b...
mojo/public/interfaces/bindings/tests/test_structs.mojom:27: int16 f3;
uint16 :)

https://codereview.chromium.org/605443002/diff/40001/mojo/public/interfaces/b...
mojo/public/interfaces/bindings/tests/test_structs.mojom:41:
handle<data_pipe_producer>? f17;
(optional; if you want to)

handle, handle?, handle<shared_buffer>, handle<shared_buffer>?

https://codereview.chromium.org/605443002/diff/40001/mojo/public/interfaces/b...
mojo/public/interfaces/bindings/tests/test_structs.mojom:55: int16 f3 = 100;
uint16 :)

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

https://codereview.chromium.org/605443002/diff/40001/mojo/public/js/bindings/...
mojo/public/js/bindings/struct_unittests.js:57: function
testExplicitFieldValues() {
nit: because we usually refer to "explicit field value" as "default value". In
order to avoid confusion, maybe we could consider "default" here and use
something such as "unspecified" for line 44?

https://codereview.chromium.org/605443002/diff/40001/mojo/public/tools/bindin...
File mojo/public/tools/bindings/pylib/mojom/generate/module.py (right):

https://codereview.chromium.org/605443002/diff/40001/mojo/public/tools/bindin...
mojo/public/tools/bindings/pylib/mojom/generate/module.py:338: return kind is
not None and kind.spec == FLOAT.spec
nit: All surrounding functions don't test kind is not None. It might make
readers wonder why this one is special.

[hansmuller, 2014-09-24 23:33:11]

Thanks for the additional feedback and for catching the missing types!

https://codereview.chromium.org/605443002/diff/40001/mojo/public/interfaces/b...
File mojo/public/interfaces/bindings/tests/test_structs.mojom (right):

https://codereview.chromium.org/605443002/diff/40001/mojo/public/interfaces/b...
mojo/public/interfaces/bindings/tests/test_structs.mojom:27: int16 f3;
On 2014/09/24 22:49:25, yzshen1 wrote:
> uint16 :)

Done.

https://codereview.chromium.org/605443002/diff/40001/mojo/public/interfaces/b...
mojo/public/interfaces/bindings/tests/test_structs.mojom:41:
handle<data_pipe_producer>? f17;
On 2014/09/24 22:49:25, yzshen1 wrote:
> (optional; if you want to)
> 
> handle, handle?, handle<shared_buffer>, handle<shared_buffer>?

I'd wanted to include all of the types, so yes. Thanks for pointing those out.

https://codereview.chromium.org/605443002/diff/40001/mojo/public/interfaces/b...
mojo/public/interfaces/bindings/tests/test_structs.mojom:55: int16 f3 = 100;
On 2014/09/24 22:49:25, yzshen1 wrote:
> uint16 :)

Done.

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

https://codereview.chromium.org/605443002/diff/40001/mojo/public/js/bindings/...
mojo/public/js/bindings/struct_unittests.js:57: function
testExplicitFieldValues() {
On 2014/09/24 22:49:25, yzshen1 wrote:
> nit: because we usually refer to "explicit field value" as "default value". In
> order to avoid confusion, maybe we could consider "default" here and use
> something such as "unspecified" for line 44?

OK. I've called the two structs and test functions: NoDefaultValues and
DefaultValues.

https://codereview.chromium.org/605443002/diff/40001/mojo/public/tools/bindin...
File mojo/public/tools/bindings/pylib/mojom/generate/module.py (right):

https://codereview.chromium.org/605443002/diff/40001/mojo/public/tools/bindin...
mojo/public/tools/bindings/pylib/mojom/generate/module.py:338: return kind is
not None and kind.spec == FLOAT.spec
On 2014/09/24 22:49:25, yzshen1 wrote:
> nit: All surrounding functions don't test kind is not None. It might make
> readers wonder why this one is special.

I've moved to the "is not None" test to the call site.

[hansmuller, 2014-09-24 23:33:17]

The CQ bit was checked by hansmuller@chromium.org

[yzshen1, 2014-09-24 23:34:49]

still LGTM

Thanks!

[commit-bot: I haz the power, 2014-09-24 23:35:46]

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

[commit-bot: I haz the power, 2014-09-24 23:51:57]

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

[commit-bot: I haz the power, 2014-09-24 23:51:58]

Try jobs failed on following builders:
  linux_chromium_gn_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[hansmuller, 2014-09-25 00:05:43]

The CQ bit was checked by hansmuller@chromium.org

[commit-bot: I haz the power, 2014-09-25 00:08:27]

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

[commit-bot: I haz the power, 2014-09-25 01:35:08]

Committed patchset #4 (id:60001) as 4c02746c495ea2352509cec0a8ae1f2829672c58

[commit-bot: I haz the power, 2014-09-25 01:35:47]

Patchset 4 (id:??) landed as
https://crrev.com/ae685fa448164fef86b0b97f519ac2f81cc8b310
Cr-Commit-Position: refs/heads/master@{#296612}