Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(920)

Issue 2689513003: Add field-initializing constructors to generated mojo structs. (Closed)

Created:
3 years, 10 months ago by Sam McNally
Modified:
3 years, 9 months ago
CC:
Aaron Boodman, abarth-chromium, cc-bugs_chromium.org, chrome-apps-syd-reviews_chromium.org, chromium-reviews, darin (slow to review), qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Add field-initializing constructors to generated mojo structs. Currently, when creating mojo-generated struct instances, it requires a multi-step process, e.g.: FooPtr foo = Foo::New(); foo->bar = bar; foo->baz = baz; This leads to helper factory functions of the form: FooPtr MakeFoo(int bar, int baz) { FooPtr foo = Foo::New(); foo->bar = bar; foo->baz = baz; return foo; } with call sites of the form: FooPtr foo = MakeFoo(bar, baz); This boilerplate should not be necessary. This CL adds constructors to generated mojo structs that initialize struct fields, allowing the simpler: FooPtr foo = Foo::New(bar, baz); CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2689513003 Cr-Commit-Position: refs/heads/master@{#453494} Committed: https://chromium.googlesource.com/chromium/src/+/138493453d9928b3b467939da9d19691831e02ba

Patch Set 1 : #

Total comments: 13

Patch Set 2 : #

Total comments: 6

Patch Set 3 : rebase #

Patch Set 4 : #

Patch Set 5 : rebase #

Patch Set 6 : rebase #

Patch Set 7 : rebase #

Patch Set 8 : #

Patch Set 9 : #

Total comments: 2

Patch Set 10 : #

Total comments: 2

Patch Set 11 : #

Patch Set 12 : rebase #

Unified diffs Side-by-side diffs Delta from patch set Stats (+275 lines, -316 lines) Patch
M cc/ipc/compositor_frame_metadata.typemap View 1 2 3 4 1 chunk +1 line, -2 lines 0 comments Download
M cc/ipc/struct_traits_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +3 lines, -3 lines 0 comments Download
M mojo/common/OWNERS View 1 1 chunk +2 lines, -0 lines 0 comments Download
M mojo/common/values.typemap View 1 2 3 1 chunk +1 line, -1 line 0 comments Download
M mojo/public/cpp/bindings/lib/native_struct.cc View 1 1 chunk +1 line, -3 lines 0 comments Download
M mojo/public/cpp/bindings/struct_ptr.h View 1 7 chunks +19 lines, -34 lines 0 comments Download
A mojo/public/cpp/bindings/tests/OWNERS View 1 chunk +6 lines, -0 lines 0 comments Download
M mojo/public/cpp/bindings/tests/equals_unittest.cc View 1 2 3 4 5 6 7 3 chunks +5 lines, -13 lines 0 comments Download
M mojo/public/cpp/bindings/tests/handle_passing_unittest.cc View 1 2 3 4 5 6 7 3 chunks +6 lines, -9 lines 0 comments Download
M mojo/public/cpp/bindings/tests/hash_unittest.cc View 1 chunk +2 lines, -14 lines 0 comments Download
M mojo/public/cpp/bindings/tests/pickle_unittest.cc View 7 chunks +32 lines, -37 lines 0 comments Download
M mojo/public/cpp/bindings/tests/pickled_types_blink.h View 1 chunk +5 lines, -0 lines 0 comments Download
M mojo/public/cpp/bindings/tests/sample_service_unittest.cc View 1 2 3 4 5 6 7 2 chunks +7 lines, -27 lines 0 comments Download
M mojo/public/cpp/bindings/tests/struct_unittest.cc View 1 2 3 4 5 6 7 8 9 10 16 chunks +66 lines, -108 lines 0 comments Download
M mojo/public/cpp/bindings/tests/test_native_types_blink.typemap View 1 chunk +1 line, -1 line 0 comments Download
M mojo/public/cpp/bindings/tests/test_native_types_chromium.typemap View 2 chunks +2 lines, -3 lines 0 comments Download
M mojo/public/cpp/bindings/tests/type_conversion_unittest.cc View 3 chunks +5 lines, -15 lines 0 comments Download
M mojo/public/cpp/bindings/tests/wtf_hash_unittest.cc View 1 chunk +6 lines, -18 lines 0 comments Download
M mojo/public/cpp/bindings/tests/wtf_types_unittest.cc View 1 chunk +1 line, -3 lines 0 comments Download
M mojo/public/interfaces/bindings/tests/test_structs.mojom View 1 2 3 4 5 6 7 8 9 10 1 chunk +10 lines, -0 lines 0 comments Download
M mojo/public/tools/bindings/generate_type_mappings.py View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M mojo/public/tools/bindings/generators/cpp_templates/wrapper_class_declaration.tmpl View 1 2 3 4 5 6 7 8 2 chunks +15 lines, -2 lines 0 comments Download
M mojo/public/tools/bindings/generators/cpp_templates/wrapper_class_definition.tmpl View 1 2 3 4 5 6 7 8 9 1 chunk +20 lines, -15 lines 0 comments Download
M mojo/public/tools/bindings/generators/cpp_templates/wrapper_class_template_definition.tmpl View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +4 lines, -5 lines 0 comments Download
M mojo/public/tools/bindings/generators/cpp_templates/wrapper_union_class_definition.tmpl View 1 1 chunk +1 line, -3 lines 0 comments Download
M mojo/public/tools/bindings/generators/mojom_cpp_generator.py View 1 2 3 4 5 6 7 8 9 2 chunks +53 lines, -0 lines 0 comments Download

Messages

Total messages: 97 (78 generated)
Sam McNally
3 years, 10 months ago (2017-02-13 08:46:56 UTC) #39
yzshen1
This is nice. Thanks! https://codereview.chromium.org/2689513003/diff/160001/mojo/public/cpp/bindings/struct_ptr.h File mojo/public/cpp/bindings/struct_ptr.h (right): https://codereview.chromium.org/2689513003/diff/160001/mojo/public/cpp/bindings/struct_ptr.h#newcode214 mojo/public/cpp/bindings/struct_ptr.h:214: value_.~Struct(); It seems we always ...
3 years, 10 months ago (2017-02-13 17:45:53 UTC) #40
Sam McNally
https://codereview.chromium.org/2689513003/diff/160001/mojo/public/cpp/bindings/struct_ptr.h File mojo/public/cpp/bindings/struct_ptr.h (right): https://codereview.chromium.org/2689513003/diff/160001/mojo/public/cpp/bindings/struct_ptr.h#newcode214 mojo/public/cpp/bindings/struct_ptr.h:214: value_.~Struct(); On 2017/02/13 17:45:53, yzshen1 wrote: > It seems ...
3 years, 10 months ago (2017-02-14 02:32:24 UTC) #43
yzshen1
https://codereview.chromium.org/2689513003/diff/160001/mojo/public/tools/bindings/generators/cpp_templates/wrapper_class_declaration.tmpl File mojo/public/tools/bindings/generators/cpp_templates/wrapper_class_declaration.tmpl (right): https://codereview.chromium.org/2689513003/diff/160001/mojo/public/tools/bindings/generators/cpp_templates/wrapper_class_declaration.tmpl#newcode34 mojo/public/tools/bindings/generators/cpp_templates/wrapper_class_declaration.tmpl:34: {% for num_params in range(struct|num_constructor_params) %} On 2017/02/14 02:32:24, ...
3 years, 10 months ago (2017-02-14 17:21:53 UTC) #46
Sam McNally
https://codereview.chromium.org/2689513003/diff/160001/mojo/public/tools/bindings/generators/cpp_templates/wrapper_class_declaration.tmpl File mojo/public/tools/bindings/generators/cpp_templates/wrapper_class_declaration.tmpl (right): https://codereview.chromium.org/2689513003/diff/160001/mojo/public/tools/bindings/generators/cpp_templates/wrapper_class_declaration.tmpl#newcode34 mojo/public/tools/bindings/generators/cpp_templates/wrapper_class_declaration.tmpl:34: {% for num_params in range(struct|num_constructor_params) %} On 2017/02/14 17:21:52, ...
3 years, 10 months ago (2017-02-15 06:35:49 UTC) #52
yzshen1
https://codereview.chromium.org/2689513003/diff/160001/mojo/public/tools/bindings/generators/cpp_templates/wrapper_class_declaration.tmpl File mojo/public/tools/bindings/generators/cpp_templates/wrapper_class_declaration.tmpl (right): https://codereview.chromium.org/2689513003/diff/160001/mojo/public/tools/bindings/generators/cpp_templates/wrapper_class_declaration.tmpl#newcode34 mojo/public/tools/bindings/generators/cpp_templates/wrapper_class_declaration.tmpl:34: {% for num_params in range(struct|num_constructor_params) %} On 2017/02/15 06:35:49, ...
3 years, 10 months ago (2017-02-15 17:25:42 UTC) #55
Sam McNally
https://codereview.chromium.org/2689513003/diff/160001/mojo/public/tools/bindings/generators/cpp_templates/wrapper_class_declaration.tmpl File mojo/public/tools/bindings/generators/cpp_templates/wrapper_class_declaration.tmpl (right): https://codereview.chromium.org/2689513003/diff/160001/mojo/public/tools/bindings/generators/cpp_templates/wrapper_class_declaration.tmpl#newcode34 mojo/public/tools/bindings/generators/cpp_templates/wrapper_class_declaration.tmpl:34: {% for num_params in range(struct|num_constructor_params) %} On 2017/02/15 17:25:42, ...
3 years, 10 months ago (2017-02-20 09:08:58 UTC) #62
yzshen1
https://codereview.chromium.org/2689513003/diff/340001/mojo/public/tools/bindings/generators/mojom_cpp_generator.py File mojo/public/tools/bindings/generators/mojom_cpp_generator.py (right): https://codereview.chromium.org/2689513003/diff/340001/mojo/public/tools/bindings/generators/mojom_cpp_generator.py#newcode519 mojo/public/tools/bindings/generators/mojom_cpp_generator.py:519: return (StructConstructor(struct.fields[:param_count]) I think the fields are ordered by ...
3 years, 10 months ago (2017-02-20 16:42:33 UTC) #63
Sam McNally
https://codereview.chromium.org/2689513003/diff/340001/mojo/public/tools/bindings/generators/mojom_cpp_generator.py File mojo/public/tools/bindings/generators/mojom_cpp_generator.py (right): https://codereview.chromium.org/2689513003/diff/340001/mojo/public/tools/bindings/generators/mojom_cpp_generator.py#newcode519 mojo/public/tools/bindings/generators/mojom_cpp_generator.py:519: return (StructConstructor(struct.fields[:param_count]) On 2017/02/20 16:42:32, yzshen1 wrote: > I ...
3 years, 10 months ago (2017-02-21 04:05:27 UTC) #68
yzshen1
LGTM Thanks! https://codereview.chromium.org/2689513003/diff/350001/mojo/public/cpp/bindings/tests/struct_unittest.cc File mojo/public/cpp/bindings/tests/struct_unittest.cc (right): https://codereview.chromium.org/2689513003/diff/350001/mojo/public/cpp/bindings/tests/struct_unittest.cc#newcode499 mojo/public/cpp/bindings/tests/struct_unittest.cc:499: EXPECT_EQ(6U, reordered->f_uint32); nit: The test is a ...
3 years, 10 months ago (2017-02-21 16:38:33 UTC) #69
Sam McNally
+vmpstr for //cc +dcheng for typemaps https://codereview.chromium.org/2689513003/diff/350001/mojo/public/cpp/bindings/tests/struct_unittest.cc File mojo/public/cpp/bindings/tests/struct_unittest.cc (right): https://codereview.chromium.org/2689513003/diff/350001/mojo/public/cpp/bindings/tests/struct_unittest.cc#newcode499 mojo/public/cpp/bindings/tests/struct_unittest.cc:499: EXPECT_EQ(6U, reordered->f_uint32); On ...
3 years, 10 months ago (2017-02-21 23:13:10 UTC) #73
dcheng
Do you mind updating the CL description to explain more of the motivation behind this ...
3 years, 10 months ago (2017-02-21 23:35:33 UTC) #74
Sam McNally
On 2017/02/21 23:35:33, dcheng wrote: > Do you mind updating the CL description to explain ...
3 years, 10 months ago (2017-02-22 01:34:17 UTC) #78
dcheng
LGTM
3 years, 10 months ago (2017-02-22 02:09:02 UTC) #79
Sam McNally
vmpstr: ping
3 years, 10 months ago (2017-02-23 22:32:44 UTC) #80
Sam McNally
+enne for //cc
3 years, 9 months ago (2017-02-27 23:17:32 UTC) #90
enne (OOO)
cc lgtm
3 years, 9 months ago (2017-02-28 02:51:42 UTC) #91
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2689513003/390001
3 years, 9 months ago (2017-02-28 03:02:06 UTC) #94
commit-bot: I haz the power
3 years, 9 months ago (2017-02-28 04:35:22 UTC) #97
Message was sent while issue was closed.
Committed patchset #12 (id:390001) as
https://chromium.googlesource.com/chromium/src/+/138493453d9928b3b467939da9d1...

Powered by Google App Engine
This is Rietveld 408576698