Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2732093006:
Write mojom and EnumTraits for display::HDCPState. (Closed)
DescriptionWrite mojom and EnumTraits for display::HDCPState.
We need a mojom and StructTraits for GammaRampRGBEntry for communication
between the gpu process to the mus-ws process for Ozone GBM.
BUG=697026
Review-Url: https://codereview.chromium.org/2732093006
Cr-Commit-Position: refs/heads/master@{#455803}
Committed: https://chromium.googlesource.com/chromium/src/+/e217f94c36f61d376d83539bc1a4fe181a755f78
Patch Set 1 #
Total comments: 4
Patch Set 2 : Fix nits. #
Total comments: 2
Patch Set 3 : . #
Messages
Total messages: 37 (21 generated)
The CQ bit was checked by thanhph@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_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
thanhph@chromium.org changed reviewers: + kylechar@chromium.org, rjkroege@chromium.org
Hi Kyle, Can you review my CL? Thanks, Thanh.
lgtm
thanhph@chromium.org changed reviewers: + tsepez@chromium.org
Thanks Kyle. Rob: Could you review my CL? Tom: Could you review my mojom struct traits? Thanks, Thanh.
thanhph@chromium.org changed reviewers: + derat@google.com - rjkroege@chromium.org
-Rob Hi Daniel, Could you review my CL? Thanks, Thanh.
Description was changed from ========== Write mojom and EnumTraits for display::HDCPState. We need a mojom and StructTraits for GammaRampRGBEntry for communication between the gpu process to the mus-ws process for Ozone GBM. BUG=697026 ========== to ========== Write mojom and EnumTraits for display::HDCPState. We need a mojom and StructTraits for GammaRampRGBEntry for communication between the gpu process to the mus-ws process for Ozone GBM. BUG=697026 ==========
thanhph@chromium.org changed reviewers: + derat@chromium.org - derat@google.com
lgtm https://codereview.chromium.org/2732093006/diff/1/ui/display/mojo/display_con... File ui/display/mojo/display_constants_struct_traits.cc (right): https://codereview.chromium.org/2732093006/diff/1/ui/display/mojo/display_con... ui/display/mojo/display_constants_struct_traits.cc:106: i know you're just being consistent with the existing style here, but i'm curious about why this file has blank lines between cases (which i don't think i've seen elsewhere in switch statements). https://codereview.chromium.org/2732093006/diff/1/ui/display/mojo/display_con... ui/display/mojo/display_constants_struct_traits.cc:133: } nit: should this have a NOTREACHED() too?
Thanks Daniel, I upload the new patch. Please review. Thanks, Thanh. https://codereview.chromium.org/2732093006/diff/1/ui/display/mojo/display_con... File ui/display/mojo/display_constants_struct_traits.cc (right): https://codereview.chromium.org/2732093006/diff/1/ui/display/mojo/display_con... ui/display/mojo/display_constants_struct_traits.cc:106: On 2017/03/09 14:46:32, Daniel Erat wrote: > i know you're just being consistent with the existing style here, but i'm > curious about why this file has blank lines between cases (which i don't think > i've seen elsewhere in switch statements). Done, I remove new lines between 2 switch cases in this file. https://codereview.chromium.org/2732093006/diff/1/ui/display/mojo/display_con... ui/display/mojo/display_constants_struct_traits.cc:133: } On 2017/03/09 14:46:32, Daniel Erat wrote: > nit: should this have a NOTREACHED() too? The deserialization should fail if the code reaches here. Returning false here seems to be sufficient and let the caller address the failure.
The CQ bit was checked by thanhph@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/2732093006/diff/20001/ui/display/mojo/display... File ui/display/mojo/display_constants_struct_traits.cc (right): https://codereview.chromium.org/2732093006/diff/20001/ui/display/mojo/display... ui/display/mojo/display_constants_struct_traits.cc:90: i think you missed the blank lines in this new function. :-)
https://codereview.chromium.org/2732093006/diff/20001/ui/display/mojo/display... File ui/display/mojo/display_constants_struct_traits.cc (right): https://codereview.chromium.org/2732093006/diff/20001/ui/display/mojo/display... ui/display/mojo/display_constants_struct_traits.cc:90: On 2017/03/09 15:35:40, Daniel Erat wrote: > i think you missed the blank lines in this new function. :-) Oops, thanks! Done.
The CQ bit was checked by thanhph@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...
lgtm thanks!
On 2017/03/09 15:41:18, Daniel Erat wrote: > lgtm > > thanks! Thanks Daniel!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by thanhph@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kylechar@chromium.org Link to the patchset: https://codereview.chromium.org/2732093006/#ps40001 (title: ".")
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
lgtm
The CQ bit was checked by thanhph@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": 40001, "attempt_start_ts": 1489083973099480,
"parent_rev": "f8e4017f550a8553187afb08de1835efe5114445", "commit_rev":
"e217f94c36f61d376d83539bc1a4fe181a755f78"}
Message was sent while issue was closed.
Description was changed from ========== Write mojom and EnumTraits for display::HDCPState. We need a mojom and StructTraits for GammaRampRGBEntry for communication between the gpu process to the mus-ws process for Ozone GBM. BUG=697026 ========== to ========== Write mojom and EnumTraits for display::HDCPState. We need a mojom and StructTraits for GammaRampRGBEntry for communication between the gpu process to the mus-ws process for Ozone GBM. BUG=697026 Review-Url: https://codereview.chromium.org/2732093006 Cr-Commit-Position: refs/heads/master@{#455803} Committed: https://chromium.googlesource.com/chromium/src/+/e217f94c36f61d376d83539bc1a4... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/e217f94c36f61d376d83539bc1a4...
Message was sent while issue was closed.
On 2017/03/09 17:34:46, Tom Sepez wrote: > lgtm Thanks Tom. Thanh. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
