Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2594203002:
[mojo] Expose interface method min_versions (Closed)
|
Created:
4 years ago by Luis Héctor Chávez Modified:
3 years, 12 months ago Reviewers:
yzshen1 CC:
chromium-reviews, elijahtaylor+arcwatch_chromium.org, cbentzel+watch_chromium.org, yusukes+watch_chromium.org, abhishekbh_chromium.org, viettrungluu+watch_chromium.org, hidehiko+watch_chromium.org, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, lhchavez+watch_chromium.org, darin (slow to review), Kevin Cernekee, Sameer Nanda, qsr+mojo_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[mojo] Expose interface method min_versions
This change adds constants Interface::kMethodNameMinVersion to be able
to identify the minimum version the interface must support to be able to
call that method.
BUG=649782
TEST=git try
Committed: https://crrev.com/5b863cf97b57423bddb6b2c3167dd6411b257b33
Cr-Commit-Position: refs/heads/master@{#440560}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Only expose min_version #
Total comments: 3
Patch Set 3 : Removed a k in the enum name #
Dependent Patchsets: Messages
Total messages: 23 (13 generated)
The CQ bit was checked by lhchavez@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
yzshen@chromium.org changed reviewers: + yzshen@chromium.org
https://codereview.chromium.org/2594203002/diff/1/components/arc/instance_hol... File components/arc/instance_holder.h (right): https://codereview.chromium.org/2594203002/diff/1/components/arc/instance_hol... components/arc/instance_holder.h:69: using Method = mojo::MethodTraits<T, MethodIndex>; I feel that mojo::MethodTraits is not needed, we could simply do: const auto& kMethodMetaData = T::kMethodMetadata[MethodIndex]; And then use kMethodMetaData.{name, min_version} below. WDYT? https://codereview.chromium.org/2594203002/diff/1/mojo/public/cpp/bindings/in... File mojo/public/cpp/bindings/interface_traits.h (right): https://codereview.chromium.org/2594203002/diff/1/mojo/public/cpp/bindings/in... mojo/public/cpp/bindings/interface_traits.h:16: uint32_t min_version; At the moment, it seems that min_version is what is really needed. Besides, there is an internal conversation (background: crbug.com/673417, we could chat in person if necessary). So storing mapping to ordinal/name may not be a good idea. Without |name|, we won't be able to do something like GetInstanceForMethod<mojom::Foo::kBarMethodIndex>() and let it logs a string name. Instead we would have to do either: GetInstanceForMethod<mojom::Foo::kBarMethodIndex>("Bar") or if we really want to avoid duplication of "Bar" in the call above, use macro: GET_INSTANCE_FOR_METHOD(mojom::Foo, Bar); Getting rid of |name| probably also helps to avoid binary size increase. WDYT? https://codereview.chromium.org/2594203002/diff/1/mojo/public/tools/bindings/... File mojo/public/tools/bindings/generators/cpp_templates/interface_declaration.tmpl (right): https://codereview.chromium.org/2594203002/diff/1/mojo/public/tools/bindings/... mojo/public/tools/bindings/generators/cpp_templates/interface_declaration.tmpl:33: enum kMethodIndices : uint32_t { For Foo.Bar(), we already have "const uint32_t internal::kFoo_Bar_Name" defined. Can we merge the two?
https://codereview.chromium.org/2594203002/diff/1/components/arc/instance_hol... File components/arc/instance_holder.h (right): https://codereview.chromium.org/2594203002/diff/1/components/arc/instance_hol... components/arc/instance_holder.h:69: using Method = mojo::MethodTraits<T, MethodIndex>; On 2016/12/22 18:53:28, yzshen1 wrote: > I feel that mojo::MethodTraits is not needed, we could simply do: > const auto& kMethodMetaData = T::kMethodMetadata[MethodIndex]; > > And then use kMethodMetaData.{name, min_version} below. WDYT? But please see my comments in the other file. If we only have min_version as meta data, we could direct do: x->GetInstanceForVersion(kFooMinVersion, "Foo" /* for logging */); or macro GET_INSTANCE_FOR_VERSION(x, Foo);
Description was changed from ========== [mojo] Expose method metadata This change adds a struct mojo::MethodTraits<Interface, MethodIndex>, that can be used to get information about a method in an interface. BUG=649782 TEST=git try ========== to ========== [mojo] Expose interface method min_versions This change adds constants Interface::kMethodNameMinVersion to be able to identify the minimum version the interface must support to be able to call that method. BUG=649782 TEST=git try ==========
The CQ bit was checked by lhchavez@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
now I'm only exposing the min version. Will actually consume it in the next change to avoid having to add tons of reviewers. PTAAL.
https://codereview.chromium.org/2594203002/diff/20001/mojo/public/tools/bindi... File mojo/public/tools/bindings/generators/cpp_templates/interface_declaration.tmpl (right): https://codereview.chromium.org/2594203002/diff/20001/mojo/public/tools/bindi... mojo/public/tools/bindings/generators/cpp_templates/interface_declaration.tmpl:33: enum kMethodMinVersions : uint32_t { Does it make sense to use static const uint32_t k{{method.name}}MinVersion = xx; ...
https://codereview.chromium.org/2594203002/diff/20001/mojo/public/tools/bindi... File mojo/public/tools/bindings/generators/cpp_templates/interface_declaration.tmpl (right): https://codereview.chromium.org/2594203002/diff/20001/mojo/public/tools/bindi... mojo/public/tools/bindings/generators/cpp_templates/interface_declaration.tmpl:33: enum kMethodMinVersions : uint32_t { On 2016/12/22 21:52:19, yzshen1 wrote: > Does it make sense to use > static const uint32_t k{{method.name}}MinVersion = xx; > ... > I'm following the advice on this thread: https://groups.google.com/a/chromium.org/forum/#!searchin/chromium-dev/ODR%7C...
LGTM with one nit: https://codereview.chromium.org/2594203002/diff/20001/mojo/public/tools/bindi... File mojo/public/tools/bindings/generators/cpp_templates/interface_declaration.tmpl (right): https://codereview.chromium.org/2594203002/diff/20001/mojo/public/tools/bindi... mojo/public/tools/bindings/generators/cpp_templates/interface_declaration.tmpl:33: enum kMethodMinVersions : uint32_t { On 2016/12/22 21:54:03, Luis Héctor Chávez wrote: > On 2016/12/22 21:52:19, yzshen1 wrote: > > Does it make sense to use > > static const uint32_t k{{method.name}}MinVersion = xx; > > ... > > > > I'm following the advice on this thread: > https://groups.google.com/a/chromium.org/forum/#!searchin/chromium-dev/ODR%7C... Thanks for pointing me there. Yeah, ODR... :) Please change the name of the enum to MethodMinVersions (i.e., removing "k").
On 2016/12/22 22:19:08, yzshen1 wrote: > LGTM with one nit: > > https://codereview.chromium.org/2594203002/diff/20001/mojo/public/tools/bindi... > File > mojo/public/tools/bindings/generators/cpp_templates/interface_declaration.tmpl > (right): > > https://codereview.chromium.org/2594203002/diff/20001/mojo/public/tools/bindi... > mojo/public/tools/bindings/generators/cpp_templates/interface_declaration.tmpl:33: > enum kMethodMinVersions : uint32_t { > On 2016/12/22 21:54:03, Luis Héctor Chávez wrote: > > On 2016/12/22 21:52:19, yzshen1 wrote: > > > Does it make sense to use > > > static const uint32_t k{{method.name}}MinVersion = xx; > > > ... > > > > > > > I'm following the advice on this thread: > > > https://groups.google.com/a/chromium.org/forum/#!searchin/chromium-dev/ODR%7C... > > Thanks for pointing me there. Yeah, ODR... :) > > Please change the name of the enum to MethodMinVersions (i.e., removing "k"). Done! Thanks!
The CQ bit was checked by lhchavez@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yzshen@chromium.org Link to the patchset: https://codereview.chromium.org/2594203002/#ps40001 (title: "Removed a k in the enum name")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1482445390954890,
"parent_rev": "27a1144e7ca3ed285ba3accc2eb08697649e84fa", "commit_rev":
"e21ec03f99a74faa16c3cec4beb7193bf4153ac1"}
Message was sent while issue was closed.
Description was changed from ========== [mojo] Expose interface method min_versions This change adds constants Interface::kMethodNameMinVersion to be able to identify the minimum version the interface must support to be able to call that method. BUG=649782 TEST=git try ========== to ========== [mojo] Expose interface method min_versions This change adds constants Interface::kMethodNameMinVersion to be able to identify the minimum version the interface must support to be able to call that method. BUG=649782 TEST=git try Review-Url: https://codereview.chromium.org/2594203002 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== [mojo] Expose interface method min_versions This change adds constants Interface::kMethodNameMinVersion to be able to identify the minimum version the interface must support to be able to call that method. BUG=649782 TEST=git try Review-Url: https://codereview.chromium.org/2594203002 ========== to ========== [mojo] Expose interface method min_versions This change adds constants Interface::kMethodNameMinVersion to be able to identify the minimum version the interface must support to be able to call that method. BUG=649782 TEST=git try Committed: https://crrev.com/5b863cf97b57423bddb6b2c3167dd6411b257b33 Cr-Commit-Position: refs/heads/master@{#440560} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/5b863cf97b57423bddb6b2c3167dd6411b257b33 Cr-Commit-Position: refs/heads/master@{#440560} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
