Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2710193003:
Adding a new message type to the Mojo channel. (Closed)
|
Created:
3 years, 10 months ago by Jay Civelli Modified:
3 years, 9 months ago CC:
Aaron Boodman, abarth-chromium, 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. |
DescriptionAdding a new message type to the Mojo channel.
Adding a new Channel::Message type and changing Channel so it
supports the old and the new messages.
This will help when modules using different version of Mojo
communicate.
BUG=695510
Review-Url: https://codereview.chromium.org/2710193003
Cr-Commit-Position: refs/heads/master@{#453814}
Committed: https://chromium.googlesource.com/chromium/src/+/823ec06c330538e88cf641061ea42cd607254e57
Patch Set 1 #Patch Set 2 : Adding versioned messages to the Mojo channel. #Patch Set 3 : Clean-up. #Patch Set 4 : Fix Mac build. #
Total comments: 16
Patch Set 5 : Fix bots. #Patch Set 6 : Addressed comments #Patch Set 7 : Fixed Mac bots #Patch Set 8 : Synced #Patch Set 9 : Changed comment #Patch Set 10 : Removed versioning from Header. #
Total comments: 11
Patch Set 11 : Addresed latest comments #
Total comments: 2
Patch Set 12 : Addressed more comments. #Patch Set 13 : Fixed Mac tests + sync #
Messages
Total messages: 50 (37 generated)
Description was changed from ========== Adding versioned messages to the Mojo channel. Adding a new Channel::Message type which has a version number and changing Channel so it still supports the old and the new messages. This will help when modules using different version of Mojo communicate. BUG= ========== to ========== Adding versioned messages to the Mojo channel. Adding a new Channel::Message type which has a version number and changing Channel so it still supports the old and the new messages. This will help when modules using different version of Mojo communicate. BUG=695510 ==========
jcivelli@chromium.org changed reviewers: + rockot@chromium.org
The CQ bit was checked by jcivelli@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: ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by jcivelli@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...)
The CQ bit was checked by jcivelli@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...
Looks decent overall but it has one fundamental flaw in that it assumes in some places that "use legacy message format" == "MOJO_EDK_LEGACY_PROTOCOL" which is totally not true. After some thought, I believe the easiest solution to that problem is to make it true by doing what we discussed earlier, i.e. making versioned messages the default (say, as an argument to Channel::Create) whenever !MOJO_EDK_LEGACY_PROTOCOL. That relegates usage of the build macro to NodeChannel (and maybe BrokerHost?) only, instead of having it entangled with all the other internal runtime logic in Message. https://codereview.chromium.org/2710193003/diff/60001/mojo/edk/system/channel.cc File mojo/edk/system/channel.cc (right): https://codereview.chromium.org/2710193003/diff/60001/mojo/edk/system/channel... mojo/edk/system/channel.cc:31: const uint16_t kMessageCurrentVersion = 1; nit: s/const/constexpr/ https://codereview.chromium.org/2710193003/diff/60001/mojo/edk/system/channel.h File mojo/edk/system/channel.h (right): https://codereview.chromium.org/2710193003/diff/60001/mojo/edk/system/channel... mojo/edk/system/channel.h:19: const size_t kChannelMessageAlignment = 8; nit: We could also now add a utility like: constexpr bool IsAlignedForChannelMessage(size_t n) { return n % kChannelMessageAlignment == 0; } which can then be used to make the static_assert sites more readable. https://codereview.chromium.org/2710193003/diff/60001/mojo/edk/system/channel... mojo/edk/system/channel.h:32: struct Message { You also need an EXPORT macro on the nested types you reference in tests. https://codereview.chromium.org/2710193003/diff/60001/mojo/edk/system/channel... mojo/edk/system/channel.h:34: // A normal message. nit: update comment? https://codereview.chromium.org/2710193003/diff/60001/mojo/edk/system/channel... mojo/edk/system/channel.h:42: NORMAL_VERSIONED, nit: NORMAL is probably fine if we call the old one NORMAL_LEGACY nit: add comment? https://codereview.chromium.org/2710193003/diff/60001/mojo/edk/system/channel... mojo/edk/system/channel.h:61: // message_type field should be at the same offset than in LegacyHeader. nit: s/should be/must be/ Also please add a notice to increment kMessageCurrentVersion in channel.cc when changing this structure, and note that such changes may only augment the data structure (i.e. we can never delete or rearrange existing fields.) We may someday wish to violate that policy but until we need to, this grants us a reasonable level of flexibility while keeping backwards-compatibility relatively simple. https://codereview.chromium.org/2710193003/diff/60001/mojo/edk/system/channel... mojo/edk/system/channel.h:121: MessageType message_type = MessageType::NORMAL_LEGACY); nit: I'd prefer to use a delegating constructor rather than a default argument https://codereview.chromium.org/2710193003/diff/60001/mojo/edk/system/channel... mojo/edk/system/channel.h:172: LegacyHeader* legacy_header_ = nullptr; nit: use an anonymous union for legacy_header_ and versioned_header_ storage?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by jcivelli@google.com 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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by jcivelli@google.com 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: This issue passed the CQ dry run.
https://codereview.chromium.org/2710193003/diff/60001/mojo/edk/system/channel.cc File mojo/edk/system/channel.cc (right): https://codereview.chromium.org/2710193003/diff/60001/mojo/edk/system/channel... mojo/edk/system/channel.cc:31: const uint16_t kMessageCurrentVersion = 1; On 2017/02/23 22:07:04, Ken Rockot wrote: > nit: s/const/constexpr/ Done. https://codereview.chromium.org/2710193003/diff/60001/mojo/edk/system/channel.h File mojo/edk/system/channel.h (right): https://codereview.chromium.org/2710193003/diff/60001/mojo/edk/system/channel... mojo/edk/system/channel.h:19: const size_t kChannelMessageAlignment = 8; On 2017/02/23 22:07:05, Ken Rockot wrote: > nit: We could also now add a utility like: > > constexpr bool IsAlignedForChannelMessage(size_t n) { > return n % kChannelMessageAlignment == 0; > } > > which can then be used to make the static_assert sites more readable. Good idea, done. https://codereview.chromium.org/2710193003/diff/60001/mojo/edk/system/channel... mojo/edk/system/channel.h:32: struct Message { On 2017/02/23 22:07:05, Ken Rockot wrote: > You also need an EXPORT macro on the nested types you reference in tests. Yes, that's the only other one used in the test I think. https://codereview.chromium.org/2710193003/diff/60001/mojo/edk/system/channel... mojo/edk/system/channel.h:34: // A normal message. On 2017/02/23 22:07:05, Ken Rockot wrote: > nit: update comment? Done. https://codereview.chromium.org/2710193003/diff/60001/mojo/edk/system/channel... mojo/edk/system/channel.h:42: NORMAL_VERSIONED, On 2017/02/23 22:07:05, Ken Rockot wrote: > nit: NORMAL is probably fine if we call the old one NORMAL_LEGACY > nit: add comment? Done. https://codereview.chromium.org/2710193003/diff/60001/mojo/edk/system/channel... mojo/edk/system/channel.h:61: // message_type field should be at the same offset than in LegacyHeader. On 2017/02/23 22:07:05, Ken Rockot wrote: > nit: s/should be/must be/ > > Also please add a notice to increment kMessageCurrentVersion in channel.cc when > changing this structure, and note that such changes may only augment the data > structure (i.e. we can never delete or rearrange existing fields.) We may > someday wish to violate that policy but until we need to, this grants us a > reasonable level of flexibility while keeping backwards-compatibility relatively > simple. Done. https://codereview.chromium.org/2710193003/diff/60001/mojo/edk/system/channel... mojo/edk/system/channel.h:121: MessageType message_type = MessageType::NORMAL_LEGACY); On 2017/02/23 22:07:05, Ken Rockot wrote: > nit: I'd prefer to use a delegating constructor rather than a default argument Done. https://codereview.chromium.org/2710193003/diff/60001/mojo/edk/system/channel... mojo/edk/system/channel.h:172: LegacyHeader* legacy_header_ = nullptr; On 2017/02/23 22:07:05, Ken Rockot wrote: > nit: use an anonymous union for legacy_header_ and versioned_header_ storage? I use the fact that versioned_header_ is not null to figure out if I am in legacy or not mode. I could not do it if this was unionized, right?
The CQ bit was checked by jcivelli@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: This issue passed the CQ dry run.
Description was changed from ========== Adding versioned messages to the Mojo channel. Adding a new Channel::Message type which has a version number and changing Channel so it still supports the old and the new messages. This will help when modules using different version of Mojo communicate. BUG=695510 ========== to ========== Adding new message types to the Mojo channel. Adding a new Channel::Message type and changing Channel so it still supports the old and the new messages. This will help when modules using different version of Mojo communicate. BUG=695510 ==========
Description was changed from ========== Adding new message types to the Mojo channel. Adding a new Channel::Message type and changing Channel so it still supports the old and the new messages. This will help when modules using different version of Mojo communicate. BUG=695510 ========== to ========== Adding a new message type to the Mojo channel. Adding a new Channel::Message type and changing Channel so it supports the old and the new messages. This will help when modules using different version of Mojo communicate. BUG=695510 ==========
looks good, a few comments https://codereview.chromium.org/2710193003/diff/180001/mojo/edk/system/channe... File mojo/edk/system/channel.cc (right): https://codereview.chromium.org/2710193003/diff/180001/mojo/edk/system/channe... mojo/edk/system/channel.cc:82: size_t header_size; Let's maybe make header_size a constexpr and use a conditional expression? Then it's clear that it's constant, and we can separately do DCHECK(extra_header_size == 0 || !is_legacy_message); https://codereview.chromium.org/2710193003/diff/180001/mojo/edk/system/channe... mojo/edk/system/channel.cc:176: if (extra_header_size > 0 && I don't think this should ever be zero https://codereview.chromium.org/2710193003/diff/180001/mojo/edk/system/channe... mojo/edk/system/channel.cc:183: extra_header_size == 0 same here https://codereview.chromium.org/2710193003/diff/180001/mojo/edk/system/channe... mojo/edk/system/channel.cc:580: // We have at least enough data available for a MessageHeader. nit: "LegacyHeader" https://codereview.chromium.org/2710193003/diff/180001/mojo/edk/system/channel.h File mojo/edk/system/channel.h (right): https://codereview.chromium.org/2710193003/diff/180001/mojo/edk/system/channe... mojo/edk/system/channel.h:40: // TODO(jcivelli): remove legacy support when Arc++ has updated to Mojo nit: No need for this TODO since I think this will all be replaced https://codereview.chromium.org/2710193003/diff/180001/mojo/edk/system/channe... File mojo/edk/system/channel_unittest.cc (right): https://codereview.chromium.org/2710193003/diff/180001/mojo/edk/system/channe... mojo/edk/system/channel_unittest.cc:41: // Not using GMock as I don't think it supports movable types. nit: I think this comment is unnecessary - it's pretty common to avoid gmock in the face of move-only stuff. Also is it really a mock? TestChannelDelegate seems fine. https://codereview.chromium.org/2710193003/diff/180001/mojo/edk/system/channe... mojo/edk/system/channel_unittest.cc:59: // Notify that an error has occured and the Channel will cease operation. nit: empty override probably doesn't need a comment?
https://codereview.chromium.org/2710193003/diff/180001/mojo/edk/system/channe... File mojo/edk/system/channel.cc (right): https://codereview.chromium.org/2710193003/diff/180001/mojo/edk/system/channe... mojo/edk/system/channel.cc:82: size_t header_size; On 2017/02/28 16:58:32, Ken Rockot wrote: > Let's maybe make header_size a constexpr and use a conditional expression? Then > it's clear that it's constant, and we can separately do > > DCHECK(extra_header_size == 0 || !is_legacy_message); It can be const but not constexpr since it depends on the value of is_legacy_message which is not constexpr. I can make it const though. https://codereview.chromium.org/2710193003/diff/180001/mojo/edk/system/channe... mojo/edk/system/channel.cc:176: if (extra_header_size > 0 && On 2017/02/28 16:58:32, Ken Rockot wrote: > I don't think this should ever be zero Yes, removed. https://codereview.chromium.org/2710193003/diff/180001/mojo/edk/system/channe... mojo/edk/system/channel.cc:183: extra_header_size == 0 On 2017/02/28 16:58:32, Ken Rockot wrote: > same here Removed. https://codereview.chromium.org/2710193003/diff/180001/mojo/edk/system/channe... mojo/edk/system/channel.cc:580: // We have at least enough data available for a MessageHeader. On 2017/02/28 16:58:32, Ken Rockot wrote: > nit: "LegacyHeader" Done.
LGTM - Please update the title of the CL too, and you'll need a security review (tsepez or ochang?)
jcivelli@chromium.org changed reviewers: + tsepez@chromium.org
Adding tsepez@ for security review.
https://codereview.chromium.org/2710193003/diff/200001/mojo/edk/system/channel.h File mojo/edk/system/channel.h (right): https://codereview.chromium.org/2710193003/diff/200001/mojo/edk/system/channe... mojo/edk/system/channel.h:68: // message_type field must be at the same offset than in LegacyHeader. can we write static assert(s) for this using offesetof() from stddef.h ? Nit: "as in" sounds better to me than "than in".
https://codereview.chromium.org/2710193003/diff/200001/mojo/edk/system/channel.h File mojo/edk/system/channel.h (right): https://codereview.chromium.org/2710193003/diff/200001/mojo/edk/system/channe... mojo/edk/system/channel.h:68: // message_type field must be at the same offset than in LegacyHeader. On 2017/02/28 19:15:49, Tom Sepez wrote: > can we write static assert(s) for this using offesetof() from stddef.h ? > Nit: "as in" sounds better to me than "than in". Good idea, done.
The CQ bit was checked by jcivelli@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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
lgtm
The CQ bit was checked by jcivelli@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org, tsepez@chromium.org Link to the patchset: https://codereview.chromium.org/2710193003/#ps240001 (title: "Fixed Mac tests + sync")
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": 240001, "attempt_start_ts": 1488329932809060,
"parent_rev": "c110fd587e6e7b835f51d1305bdf0d9ac51ac99e", "commit_rev":
"823ec06c330538e88cf641061ea42cd607254e57"}
Message was sent while issue was closed.
Description was changed from ========== Adding a new message type to the Mojo channel. Adding a new Channel::Message type and changing Channel so it supports the old and the new messages. This will help when modules using different version of Mojo communicate. BUG=695510 ========== to ========== Adding a new message type to the Mojo channel. Adding a new Channel::Message type and changing Channel so it supports the old and the new messages. This will help when modules using different version of Mojo communicate. BUG=695510 Review-Url: https://codereview.chromium.org/2710193003 Cr-Commit-Position: refs/heads/master@{#453814} Committed: https://chromium.googlesource.com/chromium/src/+/823ec06c330538e88cf641061ea4... ==========
Message was sent while issue was closed.
Committed patchset #13 (id:240001) as https://chromium.googlesource.com/chromium/src/+/823ec06c330538e88cf641061ea4... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
