Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2277853003:
Use unions for interface control messages. (Closed)
|
Created:
4 years, 4 months ago by Sam McNally Modified:
4 years, 3 months ago CC:
chromium-reviews, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, darin (slow to review), chrome-apps-syd-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUse unions for interface control messages.
Interface control messages were added before union support was ready so
the structs used for messages manually imitate unions. Now that union
support is ready, this workaround is no longer necessary and the structs
used for interface control messages can contain unions of the possible
message types.
Committed: https://crrev.com/f8af806ea3b60756271f9f3fe56d47965d29337d
Cr-Commit-Position: refs/heads/master@{#414775}
Patch Set 1 : #
Total comments: 6
Patch Set 2 : #Patch Set 3 : #
Total comments: 3
Patch Set 4 : rebase #Patch Set 5 : #
Dependent Patchsets: Messages
Total messages: 51 (39 generated)
The CQ bit was checked by sammc@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: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
The CQ bit was checked by sammc@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 sammc@chromium.org to run a CQ dry run
Patchset #1 (id:1) has been deleted
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 checked by sammc@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...
Patchset #1 (id:20001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by sammc@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 checked by sammc@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...
Patchset #1 (id:40001) has been deleted
sammc@chromium.org changed reviewers: + yzshen@chromium.org
https://codereview.chromium.org/2277853003/diff/60001/mojo/public/cpp/binding... File mojo/public/cpp/bindings/lib/control_message_handler.cc (right): https://codereview.chromium.org/2277853003/diff/60001/mojo/public/cpp/binding... mojo/public/cpp/bindings/lib/control_message_handler.cc:65: return false; If it is an unknown input, we want to return a null output. That is because we want to tolerate newer version of control messages. We have this weird union input/output instead of using separate control message types mostly because of this. (And please change the Java code as well.) As a comparison, RunOrClosePipe() closes the pipe if the input is unknown or the operation fails. https://codereview.chromium.org/2277853003/diff/60001/mojo/public/interfaces/... File mojo/public/interfaces/bindings/interface_control_messages.mojom (right): https://codereview.chromium.org/2277853003/diff/60001/mojo/public/interfaces/... mojo/public/interfaces/bindings/interface_control_messages.mojom:5: [JavaPackage="org.chromium.mojo.bindings"] nit: do you think it is better to change this name to something more consistent? The pipe_control_messages.mojom is using org.chromium.mojo.bindings.pipecontrol https://codereview.chromium.org/2277853003/diff/60001/mojo/public/java/bindin... File mojo/public/java/bindings/src/org/chromium/mojo/bindings/InterfaceControlMessagesHelper.java (right): https://codereview.chromium.org/2277853003/diff/60001/mojo/public/java/bindin... mojo/public/java/bindings/src/org/chromium/mojo/bindings/InterfaceControlMessagesHelper.java:75: return false; We need to respond with a null output.
The CQ bit was checked by sammc@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...
https://codereview.chromium.org/2277853003/diff/60001/mojo/public/cpp/binding... File mojo/public/cpp/bindings/lib/control_message_handler.cc (right): https://codereview.chromium.org/2277853003/diff/60001/mojo/public/cpp/binding... mojo/public/cpp/bindings/lib/control_message_handler.cc:65: return false; On 2016/08/25 21:08:57, yzshen1 wrote: > If it is an unknown input, we want to return a null output. That is because we > want to tolerate newer version of control messages. We have this weird union > input/output instead of using separate control message types mostly because of > this. (And please change the Java code as well.) > > As a comparison, RunOrClosePipe() closes the pipe if the input is unknown or the > operation fails. Done. https://codereview.chromium.org/2277853003/diff/60001/mojo/public/interfaces/... File mojo/public/interfaces/bindings/interface_control_messages.mojom (right): https://codereview.chromium.org/2277853003/diff/60001/mojo/public/interfaces/... mojo/public/interfaces/bindings/interface_control_messages.mojom:5: [JavaPackage="org.chromium.mojo.bindings"] On 2016/08/25 21:08:57, yzshen1 wrote: > nit: do you think it is better to change this name to something more consistent? > > The pipe_control_messages.mojom is using org.chromium.mojo.bindings.pipecontrol Done. https://codereview.chromium.org/2277853003/diff/60001/mojo/public/java/bindin... File mojo/public/java/bindings/src/org/chromium/mojo/bindings/InterfaceControlMessagesHelper.java (right): https://codereview.chromium.org/2277853003/diff/60001/mojo/public/java/bindin... mojo/public/java/bindings/src/org/chromium/mojo/bindings/InterfaceControlMessagesHelper.java:75: return false; On 2016/08/25 21:08:57, yzshen1 wrote: > We need to respond with a null output. Done.
LGTM Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by sammc@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.
sammc@chromium.org changed reviewers: + dcheng@chromium.org
+dcheng for the mojom
Sorry, I'm not really sure what's going on in this CL. Is there a bug I can look at? Or maybe we can make the CL description clearer.
Description was changed from ========== Use unions for interface control messages. ========== to ========== Use unions for interface control messages. Interface control messages were added before union support was ready so the structs used for messages manually imitate unions. Now that union support is ready, this workaround is no longer necessary and the structs used for interface control messages can contain unions of the possible message types. ==========
On 2016/08/26 05:29:53, dcheng wrote: > Sorry, I'm not really sure what's going on in this CL. Is there a bug I can look > at? Or maybe we can make the CL description clearer. Done. This CL is pretty much doing the TODOs in interface_control_messages.mojom.
LGTM https://codereview.chromium.org/2277853003/diff/100001/mojo/public/cpp/bindin... File mojo/public/cpp/bindings/lib/control_message_handler.cc (right): https://codereview.chromium.org/2277853003/diff/100001/mojo/public/cpp/bindin... mojo/public/cpp/bindings/lib/control_message_handler.cc:65: output = nullptr; I personally might find this slightly easier to read as an explicit reset() call, but I suppose that's a judgement call (above, it's not necessarily obvious that auto is capturing an InterfacePtr, unlike something like base::MakeUnique) Alternatively, it would be nice to spell out interface_control::RunOutputPtr above. https://codereview.chromium.org/2277853003/diff/100001/mojo/public/cpp/bindin... File mojo/public/cpp/bindings/lib/control_message_proxy.cc (right): https://codereview.chromium.org/2277853003/diff/100001/mojo/public/cpp/bindin... mojo/public/cpp/bindings/lib/control_message_proxy.cc:63: params_ptr, builder.buffer(), ¶ms, context); I'm not sure if it's possible (nor is it necessary in this CL), but it would be really nice to have some sort of auto-deducing helper for this.
The CQ bit was checked by sammc@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...
Patchset #4 (id:120001) has been deleted
https://codereview.chromium.org/2277853003/diff/100001/mojo/public/cpp/bindin... File mojo/public/cpp/bindings/lib/control_message_handler.cc (right): https://codereview.chromium.org/2277853003/diff/100001/mojo/public/cpp/bindin... mojo/public/cpp/bindings/lib/control_message_handler.cc:65: output = nullptr; On 2016/08/26 17:38:32, dcheng wrote: > I personally might find this slightly easier to read as an explicit reset() > call, but I suppose that's a judgement call (above, it's not necessarily obvious > that auto is capturing an InterfacePtr, unlike something like base::MakeUnique) > > Alternatively, it would be nice to spell out interface_control::RunOutputPtr > above. Done.
The CQ bit was checked by sammc@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yzshen@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2277853003/#ps160001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Use unions for interface control messages. Interface control messages were added before union support was ready so the structs used for messages manually imitate unions. Now that union support is ready, this workaround is no longer necessary and the structs used for interface control messages can contain unions of the possible message types. ========== to ========== Use unions for interface control messages. Interface control messages were added before union support was ready so the structs used for messages manually imitate unions. Now that union support is ready, this workaround is no longer necessary and the structs used for interface control messages can contain unions of the possible message types. ==========
Message was sent while issue was closed.
Committed patchset #5 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Use unions for interface control messages. Interface control messages were added before union support was ready so the structs used for messages manually imitate unions. Now that union support is ready, this workaround is no longer necessary and the structs used for interface control messages can contain unions of the possible message types. ========== to ========== Use unions for interface control messages. Interface control messages were added before union support was ready so the structs used for messages manually imitate unions. Now that union support is ready, this workaround is no longer necessary and the structs used for interface control messages can contain unions of the possible message types. Committed: https://crrev.com/f8af806ea3b60756271f9f3fe56d47965d29337d Cr-Commit-Position: refs/heads/master@{#414775} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/f8af806ea3b60756271f9f3fe56d47965d29337d Cr-Commit-Position: refs/heads/master@{#414775} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
