Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2749853003:
Making the Mojo Channel Messages legacy mode dynamic. (Closed)
|
Created:
3 years, 9 months ago by Jay Civelli Modified:
3 years, 9 months ago Reviewers:
Ken Rockot(use gerrit already) CC:
Aaron Boodman, abarth-chromium, chromium-reviews, darin (slow to review), darin-cc_chromium.org, jam, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMaking the Mojo Channel Messages legacy mode dynamic.
The mojo::edk::Channel::Message class uses a compile flag to determine
whether to use the legacy or normal mode (legacy mode is used to keep
backward compatibility between Arc++ and ChromeOS).
Making that flag dynamically set on the embedder so that the default is
now normal mode on Android. This is going to be used for adding
Parcelable support on mojo Android (where we'll need the normal
Message's header to include the parcelables).
BUG=699311
Review-Url: https://codereview.chromium.org/2749853003
Cr-Commit-Position: refs/heads/master@{#457549}
Committed: https://chromium.googlesource.com/chromium/src/+/f2be81a465a7b2ef07b3667f83de1009206acace
Patch Set 1 : Making the Mojo Channel Messages legacy mode dynamic. #
Total comments: 4
Patch Set 2 : Addressed rockot@'s comments. #Patch Set 3 : Fixed tests. #Patch Set 4 : Fixed NaCl browser test failures. #
Messages
Total messages: 32 (23 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Description was changed from ========== Making the Mojo Channel Messages legacy mode dynamic. The mojo::edk::Channel::Message class uses a compile flag to determine whether to use the legacy or normal mode (legacy mode is used to keep backward compatibility between Arc++ and ChromeOS). Making that flag dynamically set on the embedder so that the default is now normal mode on Android. This is going to be used for adding Parcelable support on mojo Android (where we'll need the normal Message's header to include the parcelables). BUG= ========== to ========== Making the Mojo Channel Messages legacy mode dynamic. The mojo::edk::Channel::Message class uses a compile flag to determine whether to use the legacy or normal mode (legacy mode is used to keep backward compatibility between Arc++ and ChromeOS). Making that flag dynamically set on the embedder so that the default is now normal mode on Android. This is going to be used for adding Parcelable support on mojo Android (where we'll need the normal Message's header to include the parcelables). BUG=699311 ==========
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: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2749853003/diff/60001/mojo/edk/system/channel.cc File mojo/edk/system/channel.cc (right): https://codereview.chromium.org/2749853003/diff/60001/mojo/edk/system/channel... mojo/edk/system/channel.cc:399: g_use_legacy_protocol = use_legacy_protocol; nit: Let's add a DCHECK(!internal::g_core) to ensure that nobody calls this after mojo::edk::Init(). It's in mojo/edk/embedder/embedder_internal.h https://codereview.chromium.org/2749853003/diff/60001/mojo/edk/system/channel... File mojo/edk/system/channel_unittest.cc (right): https://codereview.chromium.org/2749853003/diff/60001/mojo/edk/system/channel... mojo/edk/system/channel_unittest.cc:69: Channel::Message::SetUseLegacyTransportProtocol(legacy_message); Hmm. This isn't really safe. Messages can be created at any time from any thread (for e.g. propagating internal ports events). Might be OK for now the way these tests work, but it seems too subtle, and it also makes it impossible to enforce the otherwise useful DCHECK in my other comment. Can we just leave this code alone since the constructor which takes a message type is still usable?
The CQ bit was checked by jcivelli@chromium.org to run a CQ dry run
https://codereview.chromium.org/2749853003/diff/60001/mojo/edk/system/channel.cc File mojo/edk/system/channel.cc (right): https://codereview.chromium.org/2749853003/diff/60001/mojo/edk/system/channel... mojo/edk/system/channel.cc:399: g_use_legacy_protocol = use_legacy_protocol; On 2017/03/15 17:59:31, Ken Rockot wrote: > nit: Let's add a DCHECK(!internal::g_core) to ensure that nobody calls this > after mojo::edk::Init(). > > It's in mojo/edk/embedder/embedder_internal.h Good idea, done. https://codereview.chromium.org/2749853003/diff/60001/mojo/edk/system/channel... File mojo/edk/system/channel_unittest.cc (right): https://codereview.chromium.org/2749853003/diff/60001/mojo/edk/system/channel... mojo/edk/system/channel_unittest.cc:69: Channel::Message::SetUseLegacyTransportProtocol(legacy_message); On 2017/03/15 17:59:31, Ken Rockot wrote: > Hmm. This isn't really safe. Messages can be created at any time from any thread > (for e.g. propagating internal ports events). Might be OK for now the way these > tests work, but it seems too subtle, and it also makes it impossible to enforce > the otherwise useful DCHECK in my other comment. > > Can we just leave this code alone since the constructor which takes a message > type is still usable? Sure, I was actually hesitant with that changed (I wanted to check that SetUseLegacyTransportProtocol really propagates to the message).
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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 Link to the patchset: https://codereview.chromium.org/2749853003/#ps120001 (title: "Fixed NaCl browser test failures.")
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
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by jcivelli@chromium.org
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": 120001, "attempt_start_ts": 1489694495188960,
"parent_rev": "0b3940796390cda644f435497a35e07b8a330ff3", "commit_rev":
"f2be81a465a7b2ef07b3667f83de1009206acace"}
Message was sent while issue was closed.
Description was changed from ========== Making the Mojo Channel Messages legacy mode dynamic. The mojo::edk::Channel::Message class uses a compile flag to determine whether to use the legacy or normal mode (legacy mode is used to keep backward compatibility between Arc++ and ChromeOS). Making that flag dynamically set on the embedder so that the default is now normal mode on Android. This is going to be used for adding Parcelable support on mojo Android (where we'll need the normal Message's header to include the parcelables). BUG=699311 ========== to ========== Making the Mojo Channel Messages legacy mode dynamic. The mojo::edk::Channel::Message class uses a compile flag to determine whether to use the legacy or normal mode (legacy mode is used to keep backward compatibility between Arc++ and ChromeOS). Making that flag dynamically set on the embedder so that the default is now normal mode on Android. This is going to be used for adding Parcelable support on mojo Android (where we'll need the normal Message's header to include the parcelables). BUG=699311 Review-Url: https://codereview.chromium.org/2749853003 Cr-Commit-Position: refs/heads/master@{#457549} Committed: https://chromium.googlesource.com/chromium/src/+/f2be81a465a7b2ef07b3667f83de... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:120001) as https://chromium.googlesource.com/chromium/src/+/f2be81a465a7b2ef07b3667f83de...
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:120001) has been created in https://codereview.chromium.org/2765673002/ by jcivelli@chromium.org. The reason for reverting is: Breaks the Chrome PFQ bot. Will need to sync with a change to the Arc++ code before landing this.. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
