Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2646213002:
Write mojom and StructTraits for DisplaySnapshot. (Closed)
DescriptionWrite mojoms and StructTraits for DisplaySnapshot.
We need to enable communication between the mus window server and mus
gpu in the new split architecture of window server and gpu.
DisplaySnapShot is abstract so DisplaySnapshotMojo is created to
support mojom serialization/deserialization. Enum mojom and struct
traits are also added for DisplayConnectionType.
BUG=649366
Review-Url: https://codereview.chromium.org/2646213002
Cr-Commit-Position: refs/heads/master@{#455466}
Committed: https://chromium.googlesource.com/chromium/src/+/e29136a35e8282ad38b3504915837eb26425918c
Patch Set 1 #Patch Set 2 : Write mojom and StructTraits for DisplaySnapshot #Patch Set 3 : Change function order. Clean up format. #Patch Set 4 : Merge with ToT #Patch Set 5 : try to deserialize modes(). #Patch Set 6 : DisplaySnapshot struct traits #Patch Set 7 : rebase #Patch Set 8 : initalize empty vector. #Patch Set 9 : fix base::FilePath return type #Patch Set 10 : use StringPieceType to create base::FilePath #Patch Set 11 : use base::FilePath::StringType instead of std::string in unittest #Patch Set 12 : use FILE_PATH_LITERAL to create base::FilePath::StringType #
Total comments: 67
Patch Set 13 : Clean up codes #Patch Set 14 : use filepath mojom #
Total comments: 34
Patch Set 15 : fix format/comments. Refactor structure/code. #
Total comments: 55
Patch Set 16 : Create more unit tests. Refactor/Enhance code readility/format. #
Total comments: 28
Patch Set 17 : Fix nits. #Patch Set 18 : use std::numeric_limits<uint64_t>::max() instead of -1 #Patch Set 19 : Add Clone() for DisplaySnapShotMojo and refactor unit test. #
Total comments: 30
Patch Set 20 : Fix nits. #
Total comments: 6
Patch Set 21 : fix nits. #
Total comments: 2
Patch Set 22 : fix nits. #
Total comments: 8
Patch Set 23 : Add back helper function CompareModes and revert changes in display_mode.(cc|h) #
Total comments: 10
Patch Set 24 : Fix nits. #Patch Set 25 : Rebase. #Messages
Total messages: 145 (107 generated)
Description was changed from ========== Create mojom and StructTraits for display_snapshot.cc BUG=649366 ========== to ========== Write mojom and StructTraits for DisplaySnapshot BUG=649366 ==========
Description was changed from ========== Write mojom and StructTraits for DisplaySnapshot BUG=649366 ========== to ========== Write mojom and StructTraits for DisplaySnapshot. BUG=649366 ==========
The CQ bit was checked by thanhph@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...
thanhph@google.com changed reviewers: + kylechar@chromium.org
@kylechar: Please review my cl. Thanks!
The CQ bit was checked by thanhph@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.
The CQ bit was checked by thanhph@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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) 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 thanhph@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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) 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 thanhph@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: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by thanhph@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: 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 thanhph@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: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
Patchset #11 (id:200001) has been deleted
The CQ bit was checked by thanhph@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.
On 2017/01/20 23:45:20, thanhph1 wrote: > @kylechar: Please review my cl. Thanks! Hi Kyle, Can you review this? CQ bots are happy. Thanks, Thanh.
There are some duplicate implementations of the same mojom structs and StructTraits. I'm not really sure what is going on here but please deduplicate the implementations and fix the comments I left. I'll finish reviewing the files I didn't look at after that. https://codereview.chromium.org/2646213002/diff/240001/ui/display/mojo/displa... File ui/display/mojo/display_constants.mojom (right): https://codereview.chromium.org/2646213002/diff/240001/ui/display/mojo/displa... ui/display/mojo/display_constants.mojom:7: // Video output types. Say what non-mojom enum type this corresponds to. https://codereview.chromium.org/2646213002/diff/240001/ui/display/mojo/displa... ui/display/mojo/display_constants.mojom:20: DISPLAY_CONNECTION_TYPE_LAST = DISPLAY_CONNECTION_TYPE_VIRTUAL This isn't necessary/wanted here. This type is only used as a wire format, so you're not going to comparing things against display::mojom::DISPLAY_CONNECTION_TYPE_LAST. https://codereview.chromium.org/2646213002/diff/240001/ui/display/mojo/displa... File ui/display/mojo/display_constants_struct_traits.cc (right): https://codereview.chromium.org/2646213002/diff/240001/ui/display/mojo/displa... ui/display/mojo/display_constants_struct_traits.cc:5: #ifndef UI_DISPLAY_MOJO_DISPLAY_CONSTANTS_STRUCT_TRAITS Why is this in a cc file? https://codereview.chromium.org/2646213002/diff/240001/ui/display/mojo/displa... ui/display/mojo/display_constants_struct_traits.cc:10: #include "ui/display/mojo/display_constants.mojom-shared.h" You already included the mojom.h in the your header. You also don't need mojom-shared.h and mojom.h included. https://codereview.chromium.org/2646213002/diff/240001/ui/display/mojo/displa... ui/display/mojo/display_constants_struct_traits.cc:12: #include "ui/display/types/display_constants.h" This is included in the header already. https://codereview.chromium.org/2646213002/diff/240001/ui/display/mojo/displa... ui/display/mojo/display_constants_struct_traits.cc:18: type) { Did clang-format do this? https://codereview.chromium.org/2646213002/diff/240001/ui/display/mojo/displa... ui/display/mojo/display_constants_struct_traits.cc:102: NOTREACHED(); You can drop the NOTREACHED() here, this will already fail in a visible way. https://codereview.chromium.org/2646213002/diff/240001/ui/display/mojo/displa... File ui/display/mojo/display_mode_struct_traits.cc (right): https://codereview.chromium.org/2646213002/diff/240001/ui/display/mojo/displa... ui/display/mojo/display_mode_struct_traits.cc:5: #ifndef UI_DISPLAY_MOJO_DISPLAY_MODE_STRUCT_TRAITS_CC_ Again, remove this. https://codereview.chromium.org/2646213002/diff/240001/ui/display/mojo/displa... ui/display/mojo/display_mode_struct_traits.cc:8: #include "ui/display/mojo/display_mode_struct_traits.h" Please read the style guide section on includes and fix all problems here. https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes https://codereview.chromium.org/2646213002/diff/240001/ui/display/mojo/displa... File ui/display/mojo/display_snapshot.mojom (right): https://codereview.chromium.org/2646213002/diff/240001/ui/display/mojo/displa... ui/display/mojo/display_snapshot.mojom:25: display.mojom.DisplayMode current_mode; bool + DisplayMode takes more memory than an int32. Please fix. https://codereview.chromium.org/2646213002/diff/240001/ui/display/mojo/displa... ui/display/mojo/display_snapshot.mojom:29: string string_representation; This isn't part of display::DispalySnapshot and isn't necessary. https://codereview.chromium.org/2646213002/diff/240001/ui/display/mojo/displa... File ui/display/mojo/display_snapshot_mojo.cc (right): https://codereview.chromium.org/2646213002/diff/240001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo.cc:10: #include "ui/display/types/display_constants.h" You use DisplayConnectionType in the header, move the include there. https://codereview.chromium.org/2646213002/diff/240001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo.cc:66: return "DisplaySnapshotMojo"; This isn't super useful. https://codereview.chromium.org/2646213002/diff/240001/ui/display/mojo/displa... File ui/display/mojo/display_snapshot_mojo.h (right): https://codereview.chromium.org/2646213002/diff/240001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo.h:8: #include "ui/display/display_export.h" Missing include for macros. https://codereview.chromium.org/2646213002/diff/240001/ui/display/mojo/displa... File ui/display/mojo/display_snapshot_mojo.mojom (right): https://codereview.chromium.org/2646213002/diff/240001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo.mojom:1: // Copyright 2017 The Chromium Authors. All rights reserved. Huh? I don't understand why there are two mojom definitions for both DisplayConnectionType and DisplaySnapshotMojo. https://codereview.chromium.org/2646213002/diff/240001/ui/display/mojo/displa... File ui/display/mojo/display_snapshot_mojo_struct_traits.cc (right): https://codereview.chromium.org/2646213002/diff/240001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo_struct_traits.cc:5: #ifndef UI_DISPLAY_MOJO_DISPLAY_SNAPSHOT_MOJO_STRUCT_TRAITS_CC_ Remove. https://codereview.chromium.org/2646213002/diff/240001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo_struct_traits.cc:10: #include "ui/display/mojo/display_mode.mojom.h" Please deduplicate header and source file includes. https://codereview.chromium.org/2646213002/diff/240001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo_struct_traits.cc:19: display::mojom::DisplayConnectionType EnumTraits< Deduplicate. https://codereview.chromium.org/2646213002/diff/240001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo_struct_traits.cc:116: int64_t display_id = data.display_id(); You don't really need a temporary variable if there is a normal accessor method, right? https://codereview.chromium.org/2646213002/diff/240001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo_struct_traits.cc:149: for (int i = 0; i < (int)modes1.size(); i++) { This would be clearer as a for each style loop. Also C style casts aren't allowed anyways. https://codereview.chromium.org/2646213002/diff/240001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo_struct_traits.cc:151: modes.push_back(std::move(temp)); Why clone then move? Why not just clone? https://codereview.chromium.org/2646213002/diff/240001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo_struct_traits.cc:155: if ((current_mode_index < 0) || (current_mode_index >= (int)modes1.size())) What happens if current_mode == null when you are serializing? Also C style casts are banned. https://codereview.chromium.org/2646213002/diff/240001/ui/display/mojo/displa... File ui/display/mojo/display_snapshot_mojo_struct_traits.h (right): https://codereview.chromium.org/2646213002/diff/240001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo_struct_traits.h:10: #include "ui/display/mojo/display_snapshot_mojo.mojom.h" You are missing an import for DisplaySnapshotMojo. https://codereview.chromium.org/2646213002/diff/240001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo_struct_traits.h:15: struct EnumTraits<display::mojom::DisplayConnectionType, Why is the defined twice? https://codereview.chromium.org/2646213002/diff/240001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo_struct_traits.h:66: static const base::FilePath::StringType sys_path( I think you can just use FilePath in your mojom. There is a type and StructTraits for it. If not, you're passing a string on some platform and wstring on others. You'll need to double check that is okay, I'm not sure how it works. https://codereview.chromium.org/2646213002/diff/240001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo_struct_traits.h:71: static bool compareDisplayMode(const display::DisplayMode* display_mode_a, Many problems here. 1. This doesn't belong in the header, it's an implementation detail and shouldn't be exposed. 2. The method naming style is wrong. 3. Don't pass constant points, they need to be references. 3. nit: Name the variables left/right, lhs/rhs, a/b, etc. Those are much more conventional names for an equality check. https://codereview.chromium.org/2646213002/diff/240001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo_struct_traits.h:73: if ((display_mode_a->size() != display_mode_b->size()) || return lhs.size() == rhs.size() && ...; https://codereview.chromium.org/2646213002/diff/240001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo_struct_traits.h:87: std::vector<std::unique_ptr<display::DisplayMode>> display_mode_list; You know the size, tell the constructor so it can allocate the correct size initially. https://codereview.chromium.org/2646213002/diff/240001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo_struct_traits.h:94: const std::unique_ptr<display::DisplaySnapshotMojo>& display_snapshot) { Only one liner methods should go in the header. Move to source file. https://codereview.chromium.org/2646213002/diff/240001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo_struct_traits.h:95: for (int i = 0; i < (int)display_snapshot->modes().size(); i++) { You aren't allowed to use C style casts. https://google.github.io/styleguide/cppguide.html#Casting https://codereview.chromium.org/2646213002/diff/240001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo_struct_traits.h:96: if (compareDisplayMode(display_snapshot->modes()[i].get(), It would help for readability if you had done something like: auto& modes = display_snapshot->modes(); for (size_t i = 0; i < modes.size(); ++i) { https://codereview.chromium.org/2646213002/diff/240001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo_struct_traits.h:104: const std::unique_ptr<display::DisplaySnapshotMojo>& display_snapshot) { Same as above. https://codereview.chromium.org/2646213002/diff/240001/ui/display/mojo/displa... File ui/display/mojo/display_snapshot_struct_traits.h (right): https://codereview.chromium.org/2646213002/diff/240001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_struct_traits.h:1: // Copyright 2017 The Chromium Authors. All rights reserved. I don't understand why this file exists again? Please fix duplication, I don't know if I'm even reviewing the right files. I'll wait until you've fixed your patch before doing anymore review.
Thanks Kyle. Please review my new cl. Thanh. https://codereview.chromium.org/2646213002/diff/240001/ui/display/mojo/displa... File ui/display/mojo/display_constants.mojom (right): https://codereview.chromium.org/2646213002/diff/240001/ui/display/mojo/displa... ui/display/mojo/display_constants.mojom:7: // Video output types. On 2017/02/09 15:05:53, kylechar wrote: > Say what non-mojom enum type this corresponds to. Done, I put a comment in. https://codereview.chromium.org/2646213002/diff/240001/ui/display/mojo/displa... ui/display/mojo/display_constants.mojom:20: DISPLAY_CONNECTION_TYPE_LAST = DISPLAY_CONNECTION_TYPE_VIRTUAL On 2017/02/09 15:05:53, kylechar wrote: > This isn't necessary/wanted here. This type is only used as a wire format, so > you're not going to comparing things against > display::mojom::DISPLAY_CONNECTION_TYPE_LAST. Done. https://codereview.chromium.org/2646213002/diff/240001/ui/display/mojo/displa... File ui/display/mojo/display_constants_struct_traits.cc (right): https://codereview.chromium.org/2646213002/diff/240001/ui/display/mojo/displa... ui/display/mojo/display_constants_struct_traits.cc:5: #ifndef UI_DISPLAY_MOJO_DISPLAY_CONSTANTS_STRUCT_TRAITS On 2017/02/09 15:05:53, kylechar wrote: > Why is this in a cc file? Done. Removed! https://codereview.chromium.org/2646213002/diff/240001/ui/display/mojo/displa... ui/display/mojo/display_constants_struct_traits.cc:10: #include "ui/display/mojo/display_constants.mojom-shared.h" On 2017/02/09 15:05:53, kylechar wrote: > You already included the mojom.h in the your header. You also don't need > mojom-shared.h and mojom.h included. Done. https://codereview.chromium.org/2646213002/diff/240001/ui/display/mojo/displa... ui/display/mojo/display_constants_struct_traits.cc:12: #include "ui/display/types/display_constants.h" On 2017/02/09 15:05:53, kylechar wrote: > This is included in the header already. Done. https://codereview.chromium.org/2646213002/diff/240001/ui/display/mojo/displa... ui/display/mojo/display_constants_struct_traits.cc:18: type) { On 2017/02/09 15:05:53, kylechar wrote: > Did clang-format do this? Yes. I just redid it and got the same line. https://codereview.chromium.org/2646213002/diff/240001/ui/display/mojo/displa... ui/display/mojo/display_constants_struct_traits.cc:102: NOTREACHED(); On 2017/02/09 15:05:53, kylechar wrote: > You can drop the NOTREACHED() here, this will already fail in a visible way. Done. https://codereview.chromium.org/2646213002/diff/240001/ui/display/mojo/displa... File ui/display/mojo/display_mode_struct_traits.cc (right): https://codereview.chromium.org/2646213002/diff/240001/ui/display/mojo/displa... ui/display/mojo/display_mode_struct_traits.cc:5: #ifndef UI_DISPLAY_MOJO_DISPLAY_MODE_STRUCT_TRAITS_CC_ On 2017/02/09 15:05:53, kylechar wrote: > Again, remove this. Done. https://codereview.chromium.org/2646213002/diff/240001/ui/display/mojo/displa... ui/display/mojo/display_mode_struct_traits.cc:8: #include "ui/display/mojo/display_mode_struct_traits.h" On 2017/02/09 15:05:53, kylechar wrote: > Please read the style guide section on includes and fix all problems here. > > https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes I removed a duplicated include size.h https://codereview.chromium.org/2646213002/diff/240001/ui/display/mojo/displa... File ui/display/mojo/display_snapshot.mojom (right): https://codereview.chromium.org/2646213002/diff/240001/ui/display/mojo/displa... ui/display/mojo/display_snapshot.mojom:25: display.mojom.DisplayMode current_mode; On 2017/02/09 15:05:53, kylechar wrote: > bool + DisplayMode takes more memory than an int32. Please fix. I removed this file. https://codereview.chromium.org/2646213002/diff/240001/ui/display/mojo/displa... ui/display/mojo/display_snapshot.mojom:29: string string_representation; On 2017/02/09 15:05:53, kylechar wrote: > This isn't part of display::DispalySnapshot and isn't necessary. Done, I removed maximum_cursor_size also. https://codereview.chromium.org/2646213002/diff/240001/ui/display/mojo/displa... File ui/display/mojo/display_snapshot_mojo.cc (right): https://codereview.chromium.org/2646213002/diff/240001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo.cc:10: #include "ui/display/types/display_constants.h" On 2017/02/09 15:05:53, kylechar wrote: > You use DisplayConnectionType in the header, move the include there. Done. https://codereview.chromium.org/2646213002/diff/240001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo.cc:66: return "DisplaySnapshotMojo"; On 2017/02/09 15:05:54, kylechar wrote: > This isn't super useful. I put return "". https://codereview.chromium.org/2646213002/diff/240001/ui/display/mojo/displa... File ui/display/mojo/display_snapshot_mojo.h (right): https://codereview.chromium.org/2646213002/diff/240001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo.h:8: #include "ui/display/display_export.h" On 2017/02/09 15:05:54, kylechar wrote: > Missing include for macros. Done. https://codereview.chromium.org/2646213002/diff/240001/ui/display/mojo/displa... File ui/display/mojo/display_snapshot_mojo.mojom (right): https://codereview.chromium.org/2646213002/diff/240001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo.mojom:1: // Copyright 2017 The Chromium Authors. All rights reserved. On 2017/02/09 15:05:54, kylechar wrote: > Huh? I don't understand why there are two mojom definitions for both > DisplayConnectionType and DisplaySnapshotMojo. I removed this enum here. Check the display_constants.mojom for enum DisplayConnectionType https://codereview.chromium.org/2646213002/diff/240001/ui/display/mojo/displa... File ui/display/mojo/display_snapshot_mojo_struct_traits.cc (right): https://codereview.chromium.org/2646213002/diff/240001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo_struct_traits.cc:5: #ifndef UI_DISPLAY_MOJO_DISPLAY_SNAPSHOT_MOJO_STRUCT_TRAITS_CC_ On 2017/02/09 15:05:54, kylechar wrote: > Remove. Done. https://codereview.chromium.org/2646213002/diff/240001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo_struct_traits.cc:10: #include "ui/display/mojo/display_mode.mojom.h" On 2017/02/09 15:05:54, kylechar wrote: > Please deduplicate header and source file includes. Done. Removed a few. https://codereview.chromium.org/2646213002/diff/240001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo_struct_traits.cc:19: display::mojom::DisplayConnectionType EnumTraits< On 2017/02/09 15:05:54, kylechar wrote: > Deduplicate. Removed. https://codereview.chromium.org/2646213002/diff/240001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo_struct_traits.cc:116: int64_t display_id = data.display_id(); On 2017/02/09 15:05:54, kylechar wrote: > You don't really need a temporary variable if there is a normal accessor method, > right? Done. https://codereview.chromium.org/2646213002/diff/240001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo_struct_traits.cc:149: for (int i = 0; i < (int)modes1.size(); i++) { On 2017/02/09 15:05:54, kylechar wrote: > This would be clearer as a for each style loop. Also C style casts aren't > allowed anyways. Cool, done! https://codereview.chromium.org/2646213002/diff/240001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo_struct_traits.cc:151: modes.push_back(std::move(temp)); On 2017/02/09 15:05:54, kylechar wrote: > Why clone then move? Why not just clone? Done. https://codereview.chromium.org/2646213002/diff/240001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo_struct_traits.cc:155: if ((current_mode_index < 0) || (current_mode_index >= (int)modes1.size())) On 2017/02/09 15:05:54, kylechar wrote: > What happens if current_mode == null when you are serializing? Also C style > casts are banned. Refactored. current_mode will be null if it doesn't find a match in the vector. https://codereview.chromium.org/2646213002/diff/240001/ui/display/mojo/displa... File ui/display/mojo/display_snapshot_mojo_struct_traits.h (right): https://codereview.chromium.org/2646213002/diff/240001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo_struct_traits.h:10: #include "ui/display/mojo/display_snapshot_mojo.mojom.h" On 2017/02/09 15:05:54, kylechar wrote: > You are missing an import for DisplaySnapshotMojo. Done. https://codereview.chromium.org/2646213002/diff/240001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo_struct_traits.h:15: struct EnumTraits<display::mojom::DisplayConnectionType, On 2017/02/09 15:05:54, kylechar wrote: > Why is the defined twice? Removed. https://codereview.chromium.org/2646213002/diff/240001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo_struct_traits.h:66: static const base::FilePath::StringType sys_path( On 2017/02/09 15:05:54, kylechar wrote: > I think you can just use FilePath in your mojom. There is a type and > StructTraits for it. If not, you're passing a string on some platform and > wstring on others. You'll need to double check that is okay, I'm not sure how it > works. Nice, thanks. I didn't this exists. Updated! https://codereview.chromium.org/2646213002/diff/240001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo_struct_traits.h:71: static bool compareDisplayMode(const display::DisplayMode* display_mode_a, On 2017/02/09 15:05:54, kylechar wrote: > Many problems here. > 1. This doesn't belong in the header, it's an implementation detail and > shouldn't be exposed. > 2. The method naming style is wrong. > 3. Don't pass constant points, they need to be references. > 3. nit: Name the variables left/right, lhs/rhs, a/b, etc. Those are much more > conventional names for an equality check. Nice, thanks. I use the name display_mode_lhs and display_mode_rhs. https://codereview.chromium.org/2646213002/diff/240001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo_struct_traits.h:73: if ((display_mode_a->size() != display_mode_b->size()) || On 2017/02/09 15:05:54, kylechar wrote: > return lhs.size() == rhs.size() && ...; Done. https://codereview.chromium.org/2646213002/diff/240001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo_struct_traits.h:87: std::vector<std::unique_ptr<display::DisplayMode>> display_mode_list; On 2017/02/09 15:05:54, kylechar wrote: > You know the size, tell the constructor so it can allocate the correct size > initially. Done. https://codereview.chromium.org/2646213002/diff/240001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo_struct_traits.h:94: const std::unique_ptr<display::DisplaySnapshotMojo>& display_snapshot) { On 2017/02/09 15:05:54, kylechar wrote: > Only one liner methods should go in the header. Move to source file. Done. https://codereview.chromium.org/2646213002/diff/240001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo_struct_traits.h:95: for (int i = 0; i < (int)display_snapshot->modes().size(); i++) { On 2017/02/09 15:05:54, kylechar wrote: > You aren't allowed to use C style casts. > > https://google.github.io/styleguide/cppguide.html#Casting Cool, thanks! https://codereview.chromium.org/2646213002/diff/240001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo_struct_traits.h:96: if (compareDisplayMode(display_snapshot->modes()[i].get(), On 2017/02/09 15:05:54, kylechar wrote: > It would help for readability if you had done something like: > > auto& modes = display_snapshot->modes(); > for (size_t i = 0; i < modes.size(); ++i) { nice, done! https://codereview.chromium.org/2646213002/diff/240001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo_struct_traits.h:104: const std::unique_ptr<display::DisplaySnapshotMojo>& display_snapshot) { On 2017/02/09 15:05:54, kylechar wrote: > Same as above. Done. https://codereview.chromium.org/2646213002/diff/240001/ui/display/mojo/displa... File ui/display/mojo/display_snapshot_struct_traits.h (right): https://codereview.chromium.org/2646213002/diff/240001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_struct_traits.h:1: // Copyright 2017 The Chromium Authors. All rights reserved. On 2017/02/09 15:05:54, kylechar wrote: > I don't understand why this file exists again? Please fix duplication, I don't > know if I'm even reviewing the right files. I'll wait until you've fixed your > patch before doing anymore review. Removed.
The CQ bit was checked by thanhph@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...
https://codereview.chromium.org/2646213002/diff/280001/ui/display/BUILD.gn File ui/display/BUILD.gn (right): https://codereview.chromium.org/2646213002/diff/280001/ui/display/BUILD.gn#ne... ui/display/BUILD.gn:33: "mojo/display_snapshot_mojo.cc", I would move these files so they are src/ui/display/display_snapshot_mojo.[cc|h]. Usually only the mojom and StructTrait definitions go in the mojo subdir. https://codereview.chromium.org/2646213002/diff/280001/ui/display/mojo/displa... File ui/display/mojo/display_constants.mojom (right): https://codereview.chromium.org/2646213002/diff/280001/ui/display/mojo/displa... ui/display/mojo/display_constants.mojom:7: // Video output types. The video output types comment isn't useful. https://codereview.chromium.org/2646213002/diff/280001/ui/display/mojo/displa... File ui/display/mojo/display_snapshot_mojo.cc (right): https://codereview.chromium.org/2646213002/diff/280001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo.cc:65: std::string DisplaySnapshotMojo::ToString() const { This isn't ideal, although I'd be fine if you just added a TODO here. There is a decent implementation of this FakeDisplaySnapshot. You could move the implementation to a static function in DisplaySnapshot and then use the static function in FakeDisplaySnapshot and DisplaySnapshotMojo. That would be better to happen in a follow up CL though. https://codereview.chromium.org/2646213002/diff/280001/ui/display/mojo/displa... File ui/display/mojo/display_snapshot_mojo.h (right): https://codereview.chromium.org/2646213002/diff/280001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo.h:17: DisplaySnapshotMojo(); I don't think you need a default constructor? https://codereview.chromium.org/2646213002/diff/280001/ui/display/mojo/displa... File ui/display/mojo/display_snapshot_mojo.mojom (right): https://codereview.chromium.org/2646213002/diff/280001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo.mojom:22: mojo.common.mojom.FilePath sys_path; Very cool! https://codereview.chromium.org/2646213002/diff/280001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo.mojom:25: int64 current_mode_index; uint64? is probably a more appropriate type. See comments in StructTraits. https://codereview.chromium.org/2646213002/diff/280001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo.mojom:27: int64 product_id; You need all the fields in DisplaySnapshot, maximum_cursor_size_ is definitely used. https://cs.chromium.org/chromium/src/ui/display/manager/chromeos/display_chan... https://codereview.chromium.org/2646213002/diff/280001/ui/display/mojo/displa... File ui/display/mojo/display_snapshot_mojo_struct_traits.cc (right): https://codereview.chromium.org/2646213002/diff/280001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo_struct_traits.cc:34: display_mode_list.reserve(display_snapshot->modes().size()); You are creating a vector with the default size then immediately resizing it. Just provide it the correct size initially. https://codereview.chromium.org/2646213002/diff/280001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo_struct_traits.cc:45: const std::unique_ptr<display::DisplaySnapshotMojo>& display_snapshot) { if (!display_snapshot->current_mode()) return base::Optional<uint64_t>(); https://codereview.chromium.org/2646213002/diff/280001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo_struct_traits.cc:50: return i; You'll need to change this to work with base::Optional<uint64_t>. That should help guard against any overflow problems. https://codereview.chromium.org/2646213002/diff/280001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo_struct_traits.cc:52: return -1; NOTREACHED(); return base::Optional<uint64_t>(); https://codereview.chromium.org/2646213002/diff/280001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo_struct_traits.cc:60: auto& modes = display_snapshot->modes(); Adding a helper function in an anonymous namespace to share for current_mode and native_mode would help. https://codereview.chromium.org/2646213002/diff/280001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo_struct_traits.cc:92: std::vector<std::unique_ptr<display::DisplayMode>> modes1; Please change the name, modes1 doesn't tell us anything, why is there modes and modes1? Also, does this compile if modes1 is std::vector<std::unique_ptr<const display::DisplayMode>>. https://codereview.chromium.org/2646213002/diff/280001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo_struct_traits.cc:97: for_each(modes1.begin(), modes1.end(), A few things: 1. Sometimes it makes sense to do things using STL functions but not here. A loop would take two lines vs four lines this used. 2. Did you mean std::for_each? I wonder what function you're actually using here. 3. Assuming you can't just pass |modes| into ReadModes() and avoid this entirely, you don't need to call Clone(). What exactly are you doing with the contents of |modes1| when this function returns? It goes out of scope so the contents get destroyed. You should be able to just move each individual std::unique_ptr<DisplayMode> from |modes1| to |modes|. https://codereview.chromium.org/2646213002/diff/280001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo_struct_traits.cc:103: int current_mode_index = data.current_mode_index(); You are converting int64_t to int here which could cause problems. Just FYI. You'll need to change this to work the base::Optional<uint64_t> anyways. https://codereview.chromium.org/2646213002/diff/280001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo_struct_traits.cc:104: if (current_mode_index >= (int)modes1.size()) https://google.github.io/styleguide/cppguide.html#Casting https://codereview.chromium.org/2646213002/diff/280001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo_struct_traits.cc:110: // Get native_mode pointer from modes1 array. Helper function again. https://codereview.chromium.org/2646213002/diff/280001/ui/display/mojo/displa... File ui/display/mojo/display_snapshot_mojo_struct_traits.h (right): https://codereview.chromium.org/2646213002/diff/280001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo_struct_traits.h:62: static bool compare_display_mode( I meant move the the whole function to the .cc file, not just the implementation. This shouldn't be part of the StructTraits public API, although adding operator== might be a better choice if this is used multiple places.
https://codereview.chromium.org/2646213002/diff/240001/ui/display/mojo/displa... File ui/display/mojo/display_snapshot_mojo_struct_traits.cc (right): https://codereview.chromium.org/2646213002/diff/240001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo_struct_traits.cc:149: for (int i = 0; i < (int)modes1.size(); i++) { On 2017/02/10 19:54:32, thanhph1 wrote: > On 2017/02/09 15:05:54, kylechar wrote: > > This would be clearer as a for each style loop. Also C style casts aren't > > allowed anyways. > > Cool, done! Oh I see where you got the for_each thing for. Sorry, this is what "for each" loop refers to. The std::for_each function is a bit different. http://en.cppreference.com/w/cpp/language/range-for
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
> > > https://codereview.chromium.org/2646213002/diff/280001/ui/ > display/mojo/display_snapshot_mojo.mojom#newcode25 > ui/display/mojo/display_snapshot_mojo.mojom:25: int64 > current_mode_index; > uint64? is probably a more appropriate type. See comments in > StructTraits. > > > https://codereview.chromium.org/2646213002/diff/280001/ui/ > display/mojo/display_snapshot_mojo_struct_traits.cc#newcode45 > ui/display/mojo/display_snapshot_mojo_struct_traits.cc:45: const > std::unique_ptr<display::DisplaySnapshotMojo>& display_snapshot) { > if (!display_snapshot->current_mode()) > return base::Optional<uint64_t>(); > > https://codereview.chromium.org/2646213002/diff/280001/ui/ > display/mojo/display_snapshot_mojo_struct_traits.cc#newcode50 > ui/display/mojo/display_snapshot_mojo_struct_traits.cc:50: return i; > You'll need to change this to work with base::Optional<uint64_t>. That > should help guard against any overflow problems. > > https://codereview.chromium.org/2646213002/diff/280001/ui/ > display/mojo/display_snapshot_mojo_struct_traits.cc#newcode52 > ui/display/mojo/display_snapshot_mojo_struct_traits.cc:52: return -1; > NOTREACHED(); > return base::Optional<uint64_t>(); > > https://codereview.chromium.org/2646213002/diff/280001/ui/ > display/mojo/display_snapshot_mojo_struct_traits.cc#newcode60 > ui/display/mojo/display_snapshot_mojo_struct_traits.cc:60: auto& modes = > display_snapshot->modes(); > Adding a helper function in an anonymous namespace to share for > current_mode and native_mode would help. > > https://codereview.chromium.org/2646213002/diff/280001/ui/ > display/mojo/display_snapshot_mojo_struct_traits.cc#newcode103 > ui/display/mojo/display_snapshot_mojo_struct_traits.cc:103: int > current_mode_index = data.current_mode_index(); > You are converting int64_t to int here which could cause problems. Just > FYI. You'll need to change this to work the base::Optional<uint64_t> > anyways. > > > https://codereview.chromium.org/2646213002/diff/280001/ui/ > display/mojo/display_snapshot_mojo_struct_traits.cc#newcode110 > ui/display/mojo/display_snapshot_mojo_struct_traits.cc:110: // Get > native_mode pointer from modes1 array. > Helper function again. > > https://codereview.chromium.org/2646213002/diff/280001/ui/ > display/mojo/display_snapshot_mojo_struct_traits.h > File ui/display/mojo/display_snapshot_mojo_struct_traits.h (right): > > > Hi Kyle, I'm now sure how to use base::Optional<uint64_t> properly here. Basically I refactored lots of code in this approach but stuck in this line ui/display/mojo/display_snapshot_mojo_struct_traits.cc <https://codereview.chromium.org/2646213002/diff/280001/ui/display/mojo/displa...> *103* int current_mode_index = data.current_mode_index(); Ssince in the display_snapshot_mojo.mojom can't declare (with the question mark after variable) uint64? current_mode_index; uint64? native_mode_index; The data.current_mode_index() can't return base::Optional<uint64_t>. Let me know if you understand my blockage. Thanks, Thanh. -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Patchset #15 (id:300001) has been deleted
Thanks Kyle. Please review my new cl. Thanh. https://codereview.chromium.org/2646213002/diff/280001/ui/display/mojo/displa... File ui/display/mojo/display_constants.mojom (right): https://codereview.chromium.org/2646213002/diff/280001/ui/display/mojo/displa... ui/display/mojo/display_constants.mojom:7: // Video output types. On 2017/02/10 21:40:29, kylechar wrote: > The video output types comment isn't useful. Done. https://codereview.chromium.org/2646213002/diff/280001/ui/display/mojo/displa... File ui/display/mojo/display_snapshot_mojo.cc (right): https://codereview.chromium.org/2646213002/diff/280001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo.cc:65: std::string DisplaySnapshotMojo::ToString() const { On 2017/02/10 21:40:29, kylechar wrote: > This isn't ideal, although I'd be fine if you just added a TODO here. There is a > decent implementation of this FakeDisplaySnapshot. You could move the > implementation to a static function in DisplaySnapshot and then use the static > function in FakeDisplaySnapshot and DisplaySnapshotMojo. That would be better to > happen in a follow up CL though. I'll put a TODO here. I can refactor if necessary in a follow up CL. https://codereview.chromium.org/2646213002/diff/280001/ui/display/mojo/displa... File ui/display/mojo/display_snapshot_mojo.h (right): https://codereview.chromium.org/2646213002/diff/280001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo.h:17: DisplaySnapshotMojo(); On 2017/02/10 21:40:29, kylechar wrote: > I don't think you need a default constructor? Done. https://codereview.chromium.org/2646213002/diff/280001/ui/display/mojo/displa... File ui/display/mojo/display_snapshot_mojo.mojom (right): https://codereview.chromium.org/2646213002/diff/280001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo.mojom:25: int64 current_mode_index; On 2017/02/10 21:40:29, kylechar wrote: > uint64? is probably a more appropriate type. See comments in StructTraits. Done. https://codereview.chromium.org/2646213002/diff/280001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo.mojom:27: int64 product_id; On 2017/02/10 21:40:29, kylechar wrote: > You need all the fields in DisplaySnapshot, maximum_cursor_size_ is definitely > used. > > https://cs.chromium.org/chromium/src/ui/display/manager/chromeos/display_chan... Done. https://codereview.chromium.org/2646213002/diff/280001/ui/display/mojo/displa... File ui/display/mojo/display_snapshot_mojo_struct_traits.cc (right): https://codereview.chromium.org/2646213002/diff/280001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo_struct_traits.cc:34: display_mode_list.reserve(display_snapshot->modes().size()); On 2017/02/10 21:40:29, kylechar wrote: > You are creating a vector with the default size then immediately resizing it. > Just provide it the correct size initially. Done. https://codereview.chromium.org/2646213002/diff/280001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo_struct_traits.cc:45: const std::unique_ptr<display::DisplaySnapshotMojo>& display_snapshot) { On 2017/02/10 21:40:29, kylechar wrote: > if (!display_snapshot->current_mode()) > return base::Optional<uint64_t>(); Done, cool! Since we won't use base::Optional<uint64_t>() after discussion, I use !mode. https://codereview.chromium.org/2646213002/diff/280001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo_struct_traits.cc:50: return i; On 2017/02/10 21:40:29, kylechar wrote: > You'll need to change this to work with base::Optional<uint64_t>. That should > help guard against any overflow problems. As discussed, I'll use uint64_t and a bool to replace base::Optional<uint64_t> because mojom doesn't allow uint64? https://codereview.chromium.org/2646213002/diff/280001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo_struct_traits.cc:52: return -1; On 2017/02/10 21:40:29, kylechar wrote: > NOTREACHED(); > return base::Optional<uint64_t>(); Done. https://codereview.chromium.org/2646213002/diff/280001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo_struct_traits.cc:60: auto& modes = display_snapshot->modes(); On 2017/02/10 21:40:29, kylechar wrote: > Adding a helper function in an anonymous namespace to share for current_mode and > native_mode would help. Done. https://codereview.chromium.org/2646213002/diff/280001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo_struct_traits.cc:92: std::vector<std::unique_ptr<display::DisplayMode>> modes1; On 2017/02/10 21:40:29, kylechar wrote: > Please change the name, modes1 doesn't tell us anything, why is there modes and > modes1? Also, does this compile if modes1 is std::vector<std::unique_ptr<const > display::DisplayMode>>. |modes1| contains non-const display::DisplayMode while |modes| contains const display::DisplayMode. Mojom doesn't allow const variable so I need to use non-const variable in |modes1| then later on convert this variable to const in |modes|. https://codereview.chromium.org/2646213002/diff/280001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo_struct_traits.cc:97: for_each(modes1.begin(), modes1.end(), On 2017/02/10 21:40:29, kylechar wrote: > A few things: > 1. Sometimes it makes sense to do things using STL functions but not here. A > loop would take two lines vs four lines this used. > 2. Did you mean std::for_each? I wonder what function you're actually using > here. > 3. Assuming you can't just pass |modes| into ReadModes() and avoid this > entirely, you don't need to call Clone(). What exactly are you doing with the > contents of |modes1| when this function returns? It goes out of scope so the > contents get destroyed. You should be able to just move each individual > std::unique_ptr<DisplayMode> from |modes1| to |modes|. Thanks, cool! mojom doesn't allow to use const variable so |modes1| contains non-const unique pointers of DisplayMode which will be moved and auto-casted to const in |modes|. This comment is also added in the code. https://codereview.chromium.org/2646213002/diff/280001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo_struct_traits.cc:103: int current_mode_index = data.current_mode_index(); On 2017/02/10 21:40:29, kylechar wrote: > You are converting int64_t to int here which could cause problems. Just FYI. > You'll need to change this to work the base::Optional<uint64_t> anyways. Good catch, thanks! I'll change to int64_t instead of int. base::Optional<uint64_t> won't work for mojom built-in type as discussed. https://codereview.chromium.org/2646213002/diff/280001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo_struct_traits.cc:104: if (current_mode_index >= (int)modes1.size()) On 2017/02/10 21:40:29, kylechar wrote: > https://google.github.io/styleguide/cppguide.html#Casting Removed this line. https://codereview.chromium.org/2646213002/diff/280001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo_struct_traits.cc:110: // Get native_mode pointer from modes1 array. On 2017/02/10 21:40:30, kylechar wrote: > Helper function again. Done. https://codereview.chromium.org/2646213002/diff/280001/ui/display/mojo/displa... File ui/display/mojo/display_snapshot_mojo_struct_traits.h (right): https://codereview.chromium.org/2646213002/diff/280001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo_struct_traits.h:62: static bool compare_display_mode( On 2017/02/10 21:40:30, kylechar wrote: > I meant move the the whole function to the .cc file, not just the > implementation. This shouldn't be part of the StructTraits public API, although > adding operator== might be a better choice if this is used multiple places. This makes sense. I use operator==. Thanks!
The CQ bit was checked by thanhph@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_...)
There are a number of things happening in this CL, so a good CL description is need. Please add one. https://codereview.chromium.org/2646213002/diff/320001/ui/display/display_sna... File ui/display/display_snapshot_mojo.cc (right): https://codereview.chromium.org/2646213002/diff/320001/ui/display/display_sna... ui/display/display_snapshot_mojo.cc:52: // TODO: This should be in the following format: TODO(name): Description of what needs to be done. https://codereview.chromium.org/2646213002/diff/320001/ui/display/display_sna... File ui/display/display_snapshot_mojo.h (right): https://codereview.chromium.org/2646213002/diff/320001/ui/display/display_sna... ui/display/display_snapshot_mojo.h:36: const bool& current_mode_exist() const { return current_mode_exist_; } These two methods shouldn't be necessary, see comment below. https://codereview.chromium.org/2646213002/diff/320001/ui/display/display_sna... ui/display/display_snapshot_mojo.h:40: bool current_mode_exist_; Ditto for these members, this is just current_mode_ == nullptr, except you only set it in the constructor. When you change the current mode this will not be the right value. https://codereview.chromium.org/2646213002/diff/320001/ui/display/mojo/displa... File ui/display/mojo/display_snapshot_mojo.h (right): https://codereview.chromium.org/2646213002/diff/320001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo.h:1: // Copyright 2017 The Chromium Authors. All rights reserved. You have two copies of this header in different dirs. https://codereview.chromium.org/2646213002/diff/320001/ui/display/mojo/displa... File ui/display/mojo/display_snapshot_mojo.mojom (right): https://codereview.chromium.org/2646213002/diff/320001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo.mojom:12: // Corresponds to display::DisplaySnapshotMojo Comments should have proper punctuation, for example end with a period. Please fix other comments too :) https://codereview.chromium.org/2646213002/diff/320001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo.mojom:26: bool current_mode_exist; nit: has_current_mode https://codereview.chromium.org/2646213002/diff/320001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo.mojom:28: bool native_mode_exist; nit: has_native_mode https://codereview.chromium.org/2646213002/diff/320001/ui/display/mojo/displa... File ui/display/mojo/display_snapshot_mojo_struct_traits.cc (right): https://codereview.chromium.org/2646213002/diff/320001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo_struct_traits.cc:18: static int64_t get_mode_index( https://google.github.io/styleguide/cppguide.html#Function_Names https://codereview.chromium.org/2646213002/diff/320001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo_struct_traits.cc:24: for (size_t i = 0; i < modes.size(); ++i) The for loop body is more than one line so add {}. https://codereview.chromium.org/2646213002/diff/320001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo_struct_traits.cc:53: return get_mode_index(display_snapshot->modes(), get_mode_index returns an int64_t which you then return as uint64_t. This isn't quite right. You'll either want two helper functions (uint64_t GetModeIndex() vs bool HasMode()) or one method that returns a base::Optional<uint64_t>. https://codereview.chromium.org/2646213002/diff/320001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo_struct_traits.cc:87: base::FilePath filePath; file_path https://codereview.chromium.org/2646213002/diff/320001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo_struct_traits.cc:91: // mojom doesn't allow to use const variable so |modes1| contains non-const Maybe this would be clearer? // There is a type mismatch between vectors containing unique_ptr<T> vs unique_ptr<const T>. We deserialize into a vector of unique_ptr<T> then create a vector of unique_ptr<const T> after. https://codereview.chromium.org/2646213002/diff/320001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo_struct_traits.cc:94: std::vector<std::unique_ptr<display::DisplayMode>> modes1; The same comment as last time, |modes1| isn't a good name. https://codereview.chromium.org/2646213002/diff/320001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo_struct_traits.cc:98: for (auto& it : modes1) The variable |it| has nothing to do with an iterator so it's not an appropriate name. https://codereview.chromium.org/2646213002/diff/320001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo_struct_traits.cc:104: if (current_mode_exist) { Your DCHECK won't be there in release builds so there isn't actually any guarantee that current_mode_index isn't bigger than the modes vector. Read() function should return false for bad data, so do that instead. Here is how I would write this check. if (data.has_current_mode()) { size_t current_mode_index = data.current_mode_index(); if (current_mode_index >= modes.size()) return false; current_mode = modes[current_mode_index].get(); } https://codereview.chromium.org/2646213002/diff/320001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo_struct_traits.cc:131: data.product_id(), std::move(modes), edid, current_mode, std::move(edid) https://codereview.chromium.org/2646213002/diff/320001/ui/display/mojo/displa... File ui/display/mojo/display_snapshot_mojo_struct_traits.h (right): https://codereview.chromium.org/2646213002/diff/320001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo_struct_traits.h:22: static gfx::Point origin(const std::unique_ptr<display::DisplaySnapshotMojo>& Return const references if possible, which should be possible here. This will create a temporary copy needlessly. This applies to all places where you're returning from an accessor and it returns a class/struct by const reference. There are a couple of others. https://codereview.chromium.org/2646213002/diff/320001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo_struct_traits.h:82: static bool current_mode_exist( has_current_mode https://codereview.chromium.org/2646213002/diff/320001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo_struct_traits.h:92: static bool native_mode_exist( has_native_mode https://codereview.chromium.org/2646213002/diff/320001/ui/display/mojo/displa... File ui/display/mojo/display_struct_traits_unittest.cc (left): https://codereview.chromium.org/2646213002/diff/320001/ui/display/mojo/displa... ui/display/mojo/display_struct_traits_unittest.cc:116: Undo this change? https://codereview.chromium.org/2646213002/diff/320001/ui/display/mojo/displa... File ui/display/mojo/display_struct_traits_unittest.cc (right): https://codereview.chromium.org/2646213002/diff/320001/ui/display/mojo/displa... ui/display/mojo/display_struct_traits_unittest.cc:14: #include "ui/display/mojo/display_mode_struct_traits.h" You shouldn't need to include the StructTraits I don't think. https://codereview.chromium.org/2646213002/diff/320001/ui/display/mojo/displa... ui/display/mojo/display_struct_traits_unittest.cc:123: // Prepare sample input with random values Undo this change? https://codereview.chromium.org/2646213002/diff/320001/ui/display/mojo/displa... ui/display/mojo/display_struct_traits_unittest.cc:234: TEST_F(DisplayStructTraitsTest, DefaultDisplaySnapshotMojo) { This test is pretty massive and doesn't hit all the important cases. I'd say you need at least the following tests: (1) One display mode, current and native mode nullptr. (2) One display mode that is the native mode and no current mode. (3) Multiple display modes, both native and current mode set. (4) Tests for both internal display and external display. An internal display will only have one display mode and type=DISPLAY_CONNECT_TYPE_INTERNAL FYI. You'll probably want some helper functions to make this more manageable. Also you want to check a few different values in the important fields. A person preference of mine is to use realistic looking data but that is less important. https://codereview.chromium.org/2646213002/diff/320001/ui/display/mojo/displa... ui/display/mojo/display_struct_traits_unittest.cc:298: EXPECT_EQ(4, (int)output->modes().size()); You shouldn't downcast unless you absolutely need to, which isn't required here, just use the proper value to compare against. EXPECT_EQ(4u, output->modes().size()); You're not allowed to use C style casts anyways in any piece of code either. https://codereview.chromium.org/2646213002/diff/320001/ui/display/mojo/displa... ui/display/mojo/display_struct_traits_unittest.cc:301: EXPECT_EQ(*expected_modes[i].get(), *output->modes()[i].get()); unique_ptr defines *, so no need to call .get() first. https://codereview.chromium.org/2646213002/diff/320001/ui/display/types/displ... File ui/display/types/display_mode.cc (right): https://codereview.chromium.org/2646213002/diff/320001/ui/display/types/displ... ui/display/types/display_mode.cc:28: return (this->size() == display_mode.size()) && You don't need () around each == statement. https://codereview.chromium.org/2646213002/diff/320001/ui/display/types/displ... File ui/display/types/display_mode.h (right): https://codereview.chromium.org/2646213002/diff/320001/ui/display/types/displ... ui/display/types/display_mode.h:29: bool operator==(const DisplayMode& display_mode) const; You should always define operator=! if you define operator==. It's easy thankfully, bool DisplayMode::operator!=(const DisplayMode& other) const { return !operator==(other); } https://codereview.chromium.org/2646213002/diff/320001/ui/display/types/displ... ui/display/types/display_mode.h:29: bool operator==(const DisplayMode& display_mode) const; The parameter is typically named |other| here, in both the header and cc.
Thanks Kyle. Please review my new cl. Thanh. https://codereview.chromium.org/2646213002/diff/320001/ui/display/display_sna... File ui/display/display_snapshot_mojo.cc (right): https://codereview.chromium.org/2646213002/diff/320001/ui/display/display_sna... ui/display/display_snapshot_mojo.cc:52: // TODO: On 2017/02/13 22:01:23, kylechar wrote: > This should be in the following format: > > TODO(name): Description of what needs to be done. Done. https://codereview.chromium.org/2646213002/diff/320001/ui/display/display_sna... File ui/display/display_snapshot_mojo.h (right): https://codereview.chromium.org/2646213002/diff/320001/ui/display/display_sna... ui/display/display_snapshot_mojo.h:36: const bool& current_mode_exist() const { return current_mode_exist_; } On 2017/02/13 22:01:23, kylechar wrote: > These two methods shouldn't be necessary, see comment below. Removed! https://codereview.chromium.org/2646213002/diff/320001/ui/display/display_sna... ui/display/display_snapshot_mojo.h:40: bool current_mode_exist_; On 2017/02/13 22:01:23, kylechar wrote: > Ditto for these members, this is just current_mode_ == nullptr, except you only > set it in the constructor. When you change the current mode this will not be the > right value. Removed! https://codereview.chromium.org/2646213002/diff/320001/ui/display/mojo/displa... File ui/display/mojo/display_snapshot_mojo.h (right): https://codereview.chromium.org/2646213002/diff/320001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo.h:1: // Copyright 2017 The Chromium Authors. All rights reserved. On 2017/02/13 22:01:23, kylechar wrote: > You have two copies of this header in different dirs. Removed. https://codereview.chromium.org/2646213002/diff/320001/ui/display/mojo/displa... File ui/display/mojo/display_snapshot_mojo.mojom (right): https://codereview.chromium.org/2646213002/diff/320001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo.mojom:12: // Corresponds to display::DisplaySnapshotMojo On 2017/02/13 22:01:23, kylechar wrote: > Comments should have proper punctuation, for example end with a period. Please > fix other comments too :) Done. https://codereview.chromium.org/2646213002/diff/320001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo.mojom:26: bool current_mode_exist; On 2017/02/13 22:01:23, kylechar wrote: > nit: has_current_mode Done. https://codereview.chromium.org/2646213002/diff/320001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo.mojom:28: bool native_mode_exist; On 2017/02/13 22:01:23, kylechar wrote: > nit: has_native_mode Done. https://codereview.chromium.org/2646213002/diff/320001/ui/display/mojo/displa... File ui/display/mojo/display_snapshot_mojo_struct_traits.cc (right): https://codereview.chromium.org/2646213002/diff/320001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo_struct_traits.cc:18: static int64_t get_mode_index( On 2017/02/13 22:01:23, kylechar wrote: > https://google.github.io/styleguide/cppguide.html#Function_Names Oh cool, thanks! https://codereview.chromium.org/2646213002/diff/320001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo_struct_traits.cc:24: for (size_t i = 0; i < modes.size(); ++i) On 2017/02/13 22:01:23, kylechar wrote: > The for loop body is more than one line so add {}. Done. https://codereview.chromium.org/2646213002/diff/320001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo_struct_traits.cc:53: return get_mode_index(display_snapshot->modes(), On 2017/02/13 22:01:24, kylechar wrote: > get_mode_index returns an int64_t which you then return as uint64_t. This isn't > quite right. You'll either want two helper functions (uint64_t GetModeIndex() vs > bool HasMode()) or one method that returns a base::Optional<uint64_t>. I change return value from int64_t to uint64_t in get_mode_index(). https://codereview.chromium.org/2646213002/diff/320001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo_struct_traits.cc:87: base::FilePath filePath; On 2017/02/13 22:01:24, kylechar wrote: > file_path Done. https://codereview.chromium.org/2646213002/diff/320001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo_struct_traits.cc:91: // mojom doesn't allow to use const variable so |modes1| contains non-const On 2017/02/13 22:01:24, kylechar wrote: > Maybe this would be clearer? > > // There is a type mismatch between vectors containing unique_ptr<T> vs > unique_ptr<const T>. We deserialize into a vector of unique_ptr<T> then create a > vector of unique_ptr<const T> after. Done. https://codereview.chromium.org/2646213002/diff/320001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo_struct_traits.cc:94: std::vector<std::unique_ptr<display::DisplayMode>> modes1; On 2017/02/13 22:01:24, kylechar wrote: > The same comment as last time, |modes1| isn't a good name. Done. Changed to non_const_modes. https://codereview.chromium.org/2646213002/diff/320001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo_struct_traits.cc:98: for (auto& it : modes1) On 2017/02/13 22:01:23, kylechar wrote: > The variable |it| has nothing to do with an iterator so it's not an appropriate > name. Done, change to it |mode|. https://codereview.chromium.org/2646213002/diff/320001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo_struct_traits.cc:104: if (current_mode_exist) { On 2017/02/13 22:01:24, kylechar wrote: > Your DCHECK won't be there in release builds so there isn't actually any > guarantee that current_mode_index isn't bigger than the modes vector. Read() > function should return false for bad data, so do that instead. Here is how I > would write this check. > > if (data.has_current_mode()) { > size_t current_mode_index = data.current_mode_index(); > if (current_mode_index >= modes.size()) > return false; > current_mode = modes[current_mode_index].get(); > } Good to know DCHECK won't be in release builds, thanks! https://codereview.chromium.org/2646213002/diff/320001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo_struct_traits.cc:131: data.product_id(), std::move(modes), edid, current_mode, On 2017/02/13 22:01:23, kylechar wrote: > std::move(edid) Done. https://codereview.chromium.org/2646213002/diff/320001/ui/display/mojo/displa... File ui/display/mojo/display_snapshot_mojo_struct_traits.h (right): https://codereview.chromium.org/2646213002/diff/320001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo_struct_traits.h:22: static gfx::Point origin(const std::unique_ptr<display::DisplaySnapshotMojo>& On 2017/02/13 22:01:24, kylechar wrote: > Return const references if possible, which should be possible here. This will > create a temporary copy needlessly. > > > This applies to all places where you're returning from an accessor and it > returns a class/struct by const reference. There are a couple of others. Done. https://codereview.chromium.org/2646213002/diff/320001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo_struct_traits.h:82: static bool current_mode_exist( On 2017/02/13 22:01:24, kylechar wrote: > has_current_mode Done. https://codereview.chromium.org/2646213002/diff/320001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo_struct_traits.h:92: static bool native_mode_exist( On 2017/02/13 22:01:24, kylechar wrote: > has_native_mode Done. https://codereview.chromium.org/2646213002/diff/320001/ui/display/mojo/displa... File ui/display/mojo/display_struct_traits_unittest.cc (right): https://codereview.chromium.org/2646213002/diff/320001/ui/display/mojo/displa... ui/display/mojo/display_struct_traits_unittest.cc:14: #include "ui/display/mojo/display_mode_struct_traits.h" On 2017/02/13 22:01:24, kylechar wrote: > You shouldn't need to include the StructTraits I don't think. I remove display_mode_struct_traits.h. display_struct_traits_test.mojom.h is still needed. https://codereview.chromium.org/2646213002/diff/320001/ui/display/mojo/displa... ui/display/mojo/display_struct_traits_unittest.cc:123: // Prepare sample input with random values On 2017/02/13 22:01:24, kylechar wrote: > Undo this change? Done. https://codereview.chromium.org/2646213002/diff/320001/ui/display/mojo/displa... ui/display/mojo/display_struct_traits_unittest.cc:234: TEST_F(DisplayStructTraitsTest, DefaultDisplaySnapshotMojo) { On 2017/02/13 22:01:24, kylechar wrote: > This test is pretty massive and doesn't hit all the important cases. I'd say you > need at least the following tests: > > (1) One display mode, current and native mode nullptr. > (2) One display mode that is the native mode and no current mode. > (3) Multiple display modes, both native and current mode set. > (4) Tests for both internal display and external display. An internal display > will only have one display mode and type=DISPLAY_CONNECT_TYPE_INTERNAL FYI. > > You'll probably want some helper functions to make this more manageable. Also > you want to check a few different values in the important fields. A person > preference of mine is to use realistic looking data but that is less important. Thanks! I added helper function CheckDisplaySnapShotMojoEqual() to compare input, output. The test data input preparation/creation will be left in the same unit test function for readability. https://codereview.chromium.org/2646213002/diff/320001/ui/display/mojo/displa... ui/display/mojo/display_struct_traits_unittest.cc:298: EXPECT_EQ(4, (int)output->modes().size()); On 2017/02/13 22:01:24, kylechar wrote: > You shouldn't downcast unless you absolutely need to, which isn't required here, > just use the proper value to compare against. > > EXPECT_EQ(4u, output->modes().size()); > > You're not allowed to use C style casts anyways in any piece of code either. Thanks, changed to EXPECT_EQ(input.modes().size(), output.modes().size()); https://codereview.chromium.org/2646213002/diff/320001/ui/display/mojo/displa... ui/display/mojo/display_struct_traits_unittest.cc:301: EXPECT_EQ(*expected_modes[i].get(), *output->modes()[i].get()); On 2017/02/13 22:01:24, kylechar wrote: > unique_ptr defines *, so no need to call .get() first. Nice, done! https://codereview.chromium.org/2646213002/diff/320001/ui/display/types/displ... File ui/display/types/display_mode.cc (right): https://codereview.chromium.org/2646213002/diff/320001/ui/display/types/displ... ui/display/types/display_mode.cc:28: return (this->size() == display_mode.size()) && On 2017/02/13 22:01:24, kylechar wrote: > You don't need () around each == statement. Done. https://codereview.chromium.org/2646213002/diff/320001/ui/display/types/displ... File ui/display/types/display_mode.h (right): https://codereview.chromium.org/2646213002/diff/320001/ui/display/types/displ... ui/display/types/display_mode.h:29: bool operator==(const DisplayMode& display_mode) const; On 2017/02/13 22:01:24, kylechar wrote: > You should always define operator=! if you define operator==. It's easy > thankfully, > > bool DisplayMode::operator!=(const DisplayMode& other) const { > return !operator==(other); > } Nice, thanks! https://codereview.chromium.org/2646213002/diff/320001/ui/display/types/displ... ui/display/types/display_mode.h:29: bool operator==(const DisplayMode& display_mode) const; On 2017/02/13 22:01:24, kylechar wrote: > The parameter is typically named |other| here, in both the header and cc. Done.
The CQ bit was checked by thanhph@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: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
kylechar@chromium.org changed reviewers: + rjkroege@chromium.org
+rjkroege https://codereview.chromium.org/2646213002/diff/340001/ui/display/display_sna... File ui/display/display_snapshot_mojo.cc (right): https://codereview.chromium.org/2646213002/diff/340001/ui/display/display_sna... ui/display/display_snapshot_mojo.cc:45: // TODO(thanhph): What should ToString() output? Maybe for debugging purposes. The description of a TODO should be an action to do, eg. TODO(thanhph): Implement ToString() for debugging purposes. https://codereview.chromium.org/2646213002/diff/340001/ui/display/display_sna... File ui/display/display_snapshot_mojo.h (right): https://codereview.chromium.org/2646213002/diff/340001/ui/display/display_sna... ui/display/display_snapshot_mojo.h:14: // This class is used to create DisplaySnapshot IPC struct traits. I don't know if I would say this class exists to create DisplaySnapshot StructTraits. That is what you are doing but it's not the "why". You need to be able to send DisplaySnapshot over Mojo IPC, hence the class. // DisplaySnapshot implementation that can be used with Mojo IPC. https://codereview.chromium.org/2646213002/diff/340001/ui/display/display_sna... ui/display/display_snapshot_mojo.h:33: std::string ToString() const override; You need to list what class you're overriding from before a block of methods. This doesn't apply to the destructor. // DisplaySnapshot: std::string ToString() const override; https://codereview.chromium.org/2646213002/diff/340001/ui/display/mojo/displa... File ui/display/mojo/display_constants.mojom (right): https://codereview.chromium.org/2646213002/diff/340001/ui/display/mojo/displa... ui/display/mojo/display_constants.mojom:7: // Corresponding to display::DisplayConnectionType nit: Comment sentence ends with a period. https://codereview.chromium.org/2646213002/diff/340001/ui/display/mojo/displa... File ui/display/mojo/display_snapshot_mojo_struct_traits.cc (right): https://codereview.chromium.org/2646213002/diff/340001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo_struct_traits.cc:7: #include "ipc/ipc_message_utils.h" I'm not sure what this is doing here? This is a header with helper functions for the old style of Chrome IPC. https://codereview.chromium.org/2646213002/diff/340001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo_struct_traits.cc:22: return -1; You are returning a negative number as an unsigned integer. That will be converted to std::numeric_limits<uint64_t>::max() I think, which isn't a bad value to serialize for nullptr, but it's not really correct to just return -1. https://codereview.chromium.org/2646213002/diff/340001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo_struct_traits.cc:97: display::DisplaySnapshot::DisplayModeList modes; display::DisplaySnapshot::DisplayModeList modes(non_const_modes.size()); https://codereview.chromium.org/2646213002/diff/340001/ui/display/mojo/displa... File ui/display/mojo/display_snapshot_mojo_struct_traits.h (right): https://codereview.chromium.org/2646213002/diff/340001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo_struct_traits.h:18: display_snapshot_mojo) { optional nit: If you named your parameter |snapshot| you could fit more on one line. https://codereview.chromium.org/2646213002/diff/340001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo_struct_traits.h:87: return display_snapshot_mojo->current_mode(); Instad of your comment you could just do the following and it would be self documenting. return display_snapshot_mojo->current_mode() != nullptr; What happens if display_snapshot_mojo->current_mode() isn't null but it points to something that isn't contained in display_snapshot->modes() though? https://codereview.chromium.org/2646213002/diff/340001/ui/display/mojo/displa... File ui/display/mojo/display_struct_traits_unittest.cc (right): https://codereview.chromium.org/2646213002/diff/340001/ui/display/mojo/displa... ui/display/mojo/display_struct_traits_unittest.cc:116: EXPECT_EQ(true, (nullptr == output.current_mode()) && So in general if you want to expect something is true you do the following: EXPECT_TRUE(<code>); Even with that change this would be difficult to read. Maybe something like: if (!input.current_mode()) EXPECT_EQ(nullptr, output.current_mode(); else EXPECT_EQ(*input.current_mode(), *output.current_mode()); https://codereview.chromium.org/2646213002/diff/340001/ui/display/mojo/displa... ui/display/mojo/display_struct_traits_unittest.cc:269: mojom::DisplayStructTraitsTestPtr proxy = GetTraitsTestProxy(); You should create this near where you use it. Although, it's easy enough to just do: GetTraitsTestProxy()->EchoDisplaySnapshotMojo(...); https://codereview.chromium.org/2646213002/diff/340001/ui/display/mojo/displa... ui/display/mojo/display_struct_traits_unittest.cc:284: const base::FilePath sys_path(string_piece_path); Are you just trying to create a FilePath object with these three lines? You can just do something like: base::FilePath sys_path = base::FilePath::FromUTF8Unsafe("/a/b/c"); ASCII is valid UTF8 so it should work fine. https://codereview.chromium.org/2646213002/diff/340001/ui/display/mojo/displa... ui/display/mojo/display_struct_traits_unittest.cc:311: std::unique_ptr<DisplaySnapshotMojo> expected_input = I'm not a big fan of this. https://codereview.chromium.org/2646213002/diff/340001/ui/display/types/displ... File ui/display/types/display_mode.h (right): https://codereview.chromium.org/2646213002/diff/340001/ui/display/types/displ... ui/display/types/display_mode.h:29: bool operator==(const DisplayMode& other) const; https://google.github.io/styleguide/cppguide.html#Declaration_Order nit: Personal preference but the style guide says "group similar declarations together". So I would either put the operators before any non constructor/destructor public methods or after all non-operator public methods others. Mixing user defined methods and operators isn't ideal.
Oh ya, please add a CL description!
Oh ya, please add a CL description!
The CQ bit was checked by thanhph@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...
Description was changed from ========== Write mojom and StructTraits for DisplaySnapshot. BUG=649366 ========== to ========== Adding mojoms and StructTraits for DisplaySnapshot to support serialization/deserialization via unique_ptr. Enum display_constants struct traits is also added. BUG=649366 ==========
Patchset #17 (id:360001) has been deleted
The CQ bit was checked by thanhph@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: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by thanhph@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...
Thanks Kyle. Please review my new cl. Thanh. https://codereview.chromium.org/2646213002/diff/340001/ui/display/display_sna... File ui/display/display_snapshot_mojo.cc (right): https://codereview.chromium.org/2646213002/diff/340001/ui/display/display_sna... ui/display/display_snapshot_mojo.cc:45: // TODO(thanhph): What should ToString() output? Maybe for debugging purposes. On 2017/02/14 21:35:28, kylechar wrote: > The description of a TODO should be an action to do, eg. > > TODO(thanhph): Implement ToString() for debugging purposes. Done. https://codereview.chromium.org/2646213002/diff/340001/ui/display/display_sna... File ui/display/display_snapshot_mojo.h (right): https://codereview.chromium.org/2646213002/diff/340001/ui/display/display_sna... ui/display/display_snapshot_mojo.h:14: // This class is used to create DisplaySnapshot IPC struct traits. On 2017/02/14 21:35:28, kylechar wrote: > I don't know if I would say this class exists to create DisplaySnapshot > StructTraits. That is what you are doing but it's not the "why". You need to be > able to send DisplaySnapshot over Mojo IPC, hence the class. > > // DisplaySnapshot implementation that can be used with Mojo IPC. Done. https://codereview.chromium.org/2646213002/diff/340001/ui/display/display_sna... ui/display/display_snapshot_mojo.h:33: std::string ToString() const override; On 2017/02/14 21:35:28, kylechar wrote: > You need to list what class you're overriding from before a block of methods. > This doesn't apply to the destructor. > > // DisplaySnapshot: > std::string ToString() const override; Done. https://codereview.chromium.org/2646213002/diff/340001/ui/display/mojo/displa... File ui/display/mojo/display_constants.mojom (right): https://codereview.chromium.org/2646213002/diff/340001/ui/display/mojo/displa... ui/display/mojo/display_constants.mojom:7: // Corresponding to display::DisplayConnectionType On 2017/02/14 21:35:28, kylechar wrote: > nit: Comment sentence ends with a period. Done. https://codereview.chromium.org/2646213002/diff/340001/ui/display/mojo/displa... File ui/display/mojo/display_snapshot_mojo_struct_traits.cc (right): https://codereview.chromium.org/2646213002/diff/340001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo_struct_traits.cc:7: #include "ipc/ipc_message_utils.h" On 2017/02/14 21:35:28, kylechar wrote: > I'm not sure what this is doing here? This is a header with helper functions for > the old style of Chrome IPC. |sys_path| mojom serialization and de-serialization use this. https://codereview.chromium.org/2646213002/diff/340001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo_struct_traits.cc:22: return -1; On 2017/02/14 21:35:28, kylechar wrote: > You are returning a negative number as an unsigned integer. That will be > converted to std::numeric_limits<uint64_t>::max() I think, which isn't a bad > value to serialize for nullptr, but it's not really correct to just return -1. Agree. I will use std::numeric_limits<uint64_t>::max() then. https://codereview.chromium.org/2646213002/diff/340001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo_struct_traits.cc:97: display::DisplaySnapshot::DisplayModeList modes; On 2017/02/14 21:35:28, kylechar wrote: > display::DisplaySnapshot::DisplayModeList modes(non_const_modes.size()); This will fail. I'll keep it as it is. https://codereview.chromium.org/2646213002/diff/340001/ui/display/mojo/displa... File ui/display/mojo/display_snapshot_mojo_struct_traits.h (right): https://codereview.chromium.org/2646213002/diff/340001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo_struct_traits.h:18: display_snapshot_mojo) { On 2017/02/14 21:35:28, kylechar wrote: > optional nit: If you named your parameter |snapshot| you could fit more on one > line. Done. https://codereview.chromium.org/2646213002/diff/340001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo_struct_traits.h:87: return display_snapshot_mojo->current_mode(); On 2017/02/14 21:35:28, kylechar wrote: > Instad of your comment you could just do the following and it would be self > documenting. > > return display_snapshot_mojo->current_mode() != nullptr; > > What happens if display_snapshot_mojo->current_mode() isn't null but it points > to something that isn't contained in display_snapshot->modes() though? Done. https://codereview.chromium.org/2646213002/diff/340001/ui/display/mojo/displa... File ui/display/mojo/display_struct_traits_unittest.cc (right): https://codereview.chromium.org/2646213002/diff/340001/ui/display/mojo/displa... ui/display/mojo/display_struct_traits_unittest.cc:116: EXPECT_EQ(true, (nullptr == output.current_mode()) && On 2017/02/14 21:35:28, kylechar wrote: > So in general if you want to expect something is true you do the following: > > EXPECT_TRUE(<code>); > > Even with that change this would be difficult to read. Maybe something like: > > if (!input.current_mode()) > EXPECT_EQ(nullptr, output.current_mode(); > else > EXPECT_EQ(*input.current_mode(), *output.current_mode()); Done. https://codereview.chromium.org/2646213002/diff/340001/ui/display/mojo/displa... ui/display/mojo/display_struct_traits_unittest.cc:269: mojom::DisplayStructTraitsTestPtr proxy = GetTraitsTestProxy(); On 2017/02/14 21:35:28, kylechar wrote: > You should create this near where you use it. Although, it's easy enough to just > do: > > GetTraitsTestProxy()->EchoDisplaySnapshotMojo(...); Done. https://codereview.chromium.org/2646213002/diff/340001/ui/display/mojo/displa... ui/display/mojo/display_struct_traits_unittest.cc:284: const base::FilePath sys_path(string_piece_path); On 2017/02/14 21:35:28, kylechar wrote: > Are you just trying to create a FilePath object with these three lines? You can > just do something like: > > base::FilePath sys_path = base::FilePath::FromUTF8Unsafe("/a/b/c"); > > ASCII is valid UTF8 so it should work fine. Done. https://codereview.chromium.org/2646213002/diff/340001/ui/display/mojo/displa... ui/display/mojo/display_struct_traits_unittest.cc:311: std::unique_ptr<DisplaySnapshotMojo> expected_input = On 2017/02/14 21:35:28, kylechar wrote: > I'm not a big fan of this. I added a Clone() for DisplaySnapshotMojo and able to cut down some lines here. https://codereview.chromium.org/2646213002/diff/340001/ui/display/types/displ... File ui/display/types/display_mode.h (right): https://codereview.chromium.org/2646213002/diff/340001/ui/display/types/displ... ui/display/types/display_mode.h:29: bool operator==(const DisplayMode& other) const; On 2017/02/14 21:35:28, kylechar wrote: > https://google.github.io/styleguide/cppguide.html#Declaration_Order > > nit: Personal preference but the style guide says "group similar declarations > together". So I would either put the operators before any non > constructor/destructor public methods or after all non-operator public methods > others. Mixing user defined methods and operators isn't ideal. Done.
Looks pretty good! A couple last small things and some comments on your test data. https://codereview.chromium.org/2646213002/diff/420001/ui/display/display_sna... File ui/display/display_snapshot_mojo.h (right): https://codereview.chromium.org/2646213002/diff/420001/ui/display/display_sna... ui/display/display_snapshot_mojo.h:7: Add include for unique_ptr. #include <memory> https://codereview.chromium.org/2646213002/diff/420001/ui/display/display_sna... ui/display/display_snapshot_mojo.h:33: virtual std::unique_ptr<DisplaySnapshotMojo> Clone() const; Why is this virtual? https://codereview.chromium.org/2646213002/diff/420001/ui/display/mojo/displa... File ui/display/mojo/display_snapshot_mojo_struct_traits.cc (right): https://codereview.chromium.org/2646213002/diff/420001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo_struct_traits.cc:18: // Return std::numeric_limits<uint64_t>::max() if no index is found. Please read through the style guide section on comments: https://google.github.io/styleguide/cppguide.html#Function_Comments "comments should be descriptive ("Opens the file") rather than imperative ("Open the file")" "the comment describes the function, it does not tell the function what to do" "What the inputs and outputs are" Something like the following would be a better description. // Returns the index of |mode| in |modes| or uint64_t max if |mode| is null. https://codereview.chromium.org/2646213002/diff/420001/ui/display/mojo/displa... File ui/display/mojo/display_struct_traits_unittest.cc (right): https://codereview.chromium.org/2646213002/diff/420001/ui/display/mojo/displa... ui/display/mojo/display_struct_traits_unittest.cc:271: TEST_F(DisplayStructTraitsTest, DisplaySnapshotMojoNullBothIndexPointer) { DisplaySnapshotCurrentAndNativeModesNull https://codereview.chromium.org/2646213002/diff/420001/ui/display/mojo/displa... ui/display/mojo/display_struct_traits_unittest.cc:286: const DisplayMode display_mode(gfx::Size(13, 11), true, 1211.0); I don't think any displays have refresh rates that high, just from a nit picky perspective. https://codereview.chromium.org/2646213002/diff/420001/ui/display/mojo/displa... ui/display/mojo/display_struct_traits_unittest.cc:289: modes.push_back(display_mode.Clone()); If you think this helps with readability, leave it as is, but you can also do modes.push_back(base::MakeUnique<DisplayMode>(...)); https://codereview.chromium.org/2646213002/diff/420001/ui/display/mojo/displa... ui/display/mojo/display_struct_traits_unittest.cc:309: TEST_F(DisplayStructTraitsTest, DisplaySnapshotMojoNullOneIndexPointer) { DisplaySnapshotCurrentModeNull https://codereview.chromium.org/2646213002/diff/420001/ui/display/mojo/displa... ui/display/mojo/display_struct_traits_unittest.cc:328: const DisplayMode* current_mode = modes[0].get(); The description says one native mode and no current mode but you're doing the opposite here. https://codereview.chromium.org/2646213002/diff/420001/ui/display/mojo/displa... ui/display/mojo/display_struct_traits_unittest.cc:346: TEST_F(DisplayStructTraitsTest, DisplaySnapshotMojoDefault) { What does DisplaySnapshotMojoDefault mean exactly? Maybe DisplaySnapshotExternal https://codereview.chromium.org/2646213002/diff/420001/ui/display/mojo/displa... ui/display/mojo/display_struct_traits_unittest.cc:347: // prepare sample input with random values Here is some sample data from an external monitor: id:9834293210466051 origin: 0,1760 physical_size:520x320 has_overscan: false is_aspect_perserving_scaling: false has_color_correction_matrix: false display_name: "HP Z24i" modes: (1024x768, false, 60.00), (1440x900/59.89), (1920x1200, false, 59.89f) *this is the native mode https://codereview.chromium.org/2646213002/diff/420001/ui/display/mojo/displa... ui/display/mojo/display_struct_traits_unittest.cc:388: // Tests for both internal display and external display. This display is an internal display, as per the display type, so this comment doesn't make sense. https://codereview.chromium.org/2646213002/diff/420001/ui/display/mojo/displa... ui/display/mojo/display_struct_traits_unittest.cc:389: TEST_F(DisplayStructTraitsTest, DisplaySnapshotMojoInternalDisplay) { DisplaySnapshotInternal maybe? https://codereview.chromium.org/2646213002/diff/420001/ui/display/mojo/displa... ui/display/mojo/display_struct_traits_unittest.cc:390: // prepare sample input with random values I'll provide you with some sample data from Pixel 1, which is better than random data. id:13761487533244416 origin:0,0 physical_size:270x180 has_overscan=false is_aspect_perserving_scaling=true has_color_correction_matrix=false display_name:"" sys_path: modes=(2560x1700, false, 95.96) this is the native mode maximum_cursor_size=64x64 https://codereview.chromium.org/2646213002/diff/420001/ui/display/types/displ... File ui/display/types/display_mode.cc (right): https://codereview.chromium.org/2646213002/diff/420001/ui/display/types/displ... ui/display/types/display_mode.cc:39: std::unique_ptr<DisplayMode> DisplayMode::Clone() const { Please keep the order consistent between header and source files. You moved Clone() under ToString() for some reason in this patch?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by thanhph@chromium.org to run a CQ dry run
Patchset #20 (id:440001) 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 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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Patchset #20 (id:460001) has been deleted
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: This issue passed the CQ dry run.
Thanks Kyle. Please review my new patch. Thanh. https://codereview.chromium.org/2646213002/diff/420001/ui/display/display_sna... File ui/display/display_snapshot_mojo.h (right): https://codereview.chromium.org/2646213002/diff/420001/ui/display/display_sna... ui/display/display_snapshot_mojo.h:7: On 2017/02/15 16:48:45, kylechar wrote: > Add include for unique_ptr. > > #include <memory> Done. https://codereview.chromium.org/2646213002/diff/420001/ui/display/display_sna... ui/display/display_snapshot_mojo.h:33: virtual std::unique_ptr<DisplaySnapshotMojo> Clone() const; On 2017/02/15 16:48:45, kylechar wrote: > Why is this virtual? Is there other class that will implement DisplaySnapShot? I notice DisplayMode->Clone() is also virtual. (http://stackoverflow.com/questions/17842594/virtual-function-inheritance) https://codereview.chromium.org/2646213002/diff/420001/ui/display/mojo/displa... File ui/display/mojo/display_snapshot_mojo_struct_traits.cc (right): https://codereview.chromium.org/2646213002/diff/420001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo_struct_traits.cc:18: // Return std::numeric_limits<uint64_t>::max() if no index is found. On 2017/02/15 16:48:45, kylechar wrote: > Please read through the style guide section on comments: > https://google.github.io/styleguide/cppguide.html#Function_Comments > > "comments should be descriptive ("Opens the file") rather than imperative ("Open > the file")" > "the comment describes the function, it does not tell the function what to do" > "What the inputs and outputs are" > > Something like the following would be a better description. > > // Returns the index of |mode| in |modes| or uint64_t max if |mode| is null. Cool, thanks! https://codereview.chromium.org/2646213002/diff/420001/ui/display/mojo/displa... File ui/display/mojo/display_struct_traits_unittest.cc (right): https://codereview.chromium.org/2646213002/diff/420001/ui/display/mojo/displa... ui/display/mojo/display_struct_traits_unittest.cc:271: TEST_F(DisplayStructTraitsTest, DisplaySnapshotMojoNullBothIndexPointer) { On 2017/02/15 16:48:45, kylechar wrote: > DisplaySnapshotCurrentAndNativeModesNull Done. https://codereview.chromium.org/2646213002/diff/420001/ui/display/mojo/displa... ui/display/mojo/display_struct_traits_unittest.cc:286: const DisplayMode display_mode(gfx::Size(13, 11), true, 1211.0); On 2017/02/15 16:48:45, kylechar wrote: > I don't think any displays have refresh rates that high, just from a nit picky > perspective. I changed to 40.0, 50.0 & 60.0. https://codereview.chromium.org/2646213002/diff/420001/ui/display/mojo/displa... ui/display/mojo/display_struct_traits_unittest.cc:289: modes.push_back(display_mode.Clone()); On 2017/02/15 16:48:45, kylechar wrote: > If you think this helps with readability, leave it as is, but you can also do > > modes.push_back(base::MakeUnique<DisplayMode>(...)); I'll leave it as it is since it's shorter. Thanks for explicit explanation. https://codereview.chromium.org/2646213002/diff/420001/ui/display/mojo/displa... ui/display/mojo/display_struct_traits_unittest.cc:309: TEST_F(DisplayStructTraitsTest, DisplaySnapshotMojoNullOneIndexPointer) { On 2017/02/15 16:48:45, kylechar wrote: > DisplaySnapshotCurrentModeNull Done. https://codereview.chromium.org/2646213002/diff/420001/ui/display/mojo/displa... ui/display/mojo/display_struct_traits_unittest.cc:328: const DisplayMode* current_mode = modes[0].get(); On 2017/02/15 16:48:45, kylechar wrote: > The description says one native mode and no current mode but you're doing the > opposite here. Done, I swapped current_mode to nullptr. https://codereview.chromium.org/2646213002/diff/420001/ui/display/mojo/displa... ui/display/mojo/display_struct_traits_unittest.cc:346: TEST_F(DisplayStructTraitsTest, DisplaySnapshotMojoDefault) { On 2017/02/15 16:48:45, kylechar wrote: > What does DisplaySnapshotMojoDefault mean exactly? > > Maybe DisplaySnapshotExternal Done. https://codereview.chromium.org/2646213002/diff/420001/ui/display/mojo/displa... ui/display/mojo/display_struct_traits_unittest.cc:347: // prepare sample input with random values On 2017/02/15 16:48:45, kylechar wrote: > Here is some sample data from an external monitor: > > id:9834293210466051 > origin: 0,1760 > physical_size:520x320 > has_overscan: false > is_aspect_perserving_scaling: false > has_color_correction_matrix: false > display_name: "HP Z24i" > modes: (1024x768, false, 60.00), > (1440x900/59.89), > (1920x1200, false, 59.89f) *this is the native mode > Thanks, I added these data in. How do you get these data? https://codereview.chromium.org/2646213002/diff/420001/ui/display/mojo/displa... ui/display/mojo/display_struct_traits_unittest.cc:388: // Tests for both internal display and external display. On 2017/02/15 16:48:45, kylechar wrote: > This display is an internal display, as per the display type, so this comment > doesn't make sense. Removed this comment since the test name seems meaningful enough. https://codereview.chromium.org/2646213002/diff/420001/ui/display/mojo/displa... ui/display/mojo/display_struct_traits_unittest.cc:389: TEST_F(DisplayStructTraitsTest, DisplaySnapshotMojoInternalDisplay) { On 2017/02/15 16:48:45, kylechar wrote: > DisplaySnapshotInternal maybe? Done. https://codereview.chromium.org/2646213002/diff/420001/ui/display/mojo/displa... ui/display/mojo/display_struct_traits_unittest.cc:390: // prepare sample input with random values On 2017/02/15 16:48:45, kylechar wrote: > I'll provide you with some sample data from Pixel 1, which is better than random > data. > > > id:13761487533244416 > origin:0,0 > physical_size:270x180 > has_overscan=false > is_aspect_perserving_scaling=true > has_color_correction_matrix=false > display_name:"" > sys_path: > modes=(2560x1700, false, 95.96) this is the native mode > maximum_cursor_size=64x64 nice, thanks! https://codereview.chromium.org/2646213002/diff/420001/ui/display/types/displ... File ui/display/types/display_mode.cc (right): https://codereview.chromium.org/2646213002/diff/420001/ui/display/types/displ... ui/display/types/display_mode.cc:39: std::unique_ptr<DisplayMode> DisplayMode::Clone() const { On 2017/02/15 16:48:45, kylechar wrote: > Please keep the order consistent between header and source files. You moved > Clone() under ToString() for some reason in this patch? Nice catch. Thanks!
https://codereview.chromium.org/2646213002/diff/420001/ui/display/display_sna... File ui/display/display_snapshot_mojo.h (right): https://codereview.chromium.org/2646213002/diff/420001/ui/display/display_sna... ui/display/display_snapshot_mojo.h:33: virtual std::unique_ptr<DisplaySnapshotMojo> Clone() const; On 2017/02/16 11:48:20, thanhph wrote: > On 2017/02/15 16:48:45, kylechar wrote: > > Why is this virtual? > > Is there other class that will implement DisplaySnapShot? I notice > DisplayMode->Clone() is also virtual. > (http://stackoverflow.com/questions/17842594/virtual-function-inheritance) Virtual only works for subclasses. Putting a virtual method in DisplaySnapshotMojo has no effect on the parent class DisplaySnapshot. You'd have to be planning on using virtual dispatch in subclasses of DisplaySnapshotMojo, which AFAIK there will be none? https://codereview.chromium.org/2646213002/diff/420001/ui/display/mojo/displa... File ui/display/mojo/display_struct_traits_unittest.cc (right): https://codereview.chromium.org/2646213002/diff/420001/ui/display/mojo/displa... ui/display/mojo/display_struct_traits_unittest.cc:347: // prepare sample input with random values On 2017/02/16 11:48:21, thanhph wrote: > On 2017/02/15 16:48:45, kylechar wrote: > > Here is some sample data from an external monitor: > > > > id:9834293210466051 > > origin: 0,1760 > > physical_size:520x320 > > has_overscan: false > > is_aspect_perserving_scaling: false > > has_color_correction_matrix: false > > display_name: "HP Z24i" > > modes: (1024x768, false, 60.00), > > (1440x900/59.89), > > (1920x1200, false, 59.89f) *this is the native mode > > > > Thanks, I added these data in. How do you get these data? I have this monitor on my desk and I printed the contents of DisplaySnapshot when I connected it to a Chromebook. https://codereview.chromium.org/2646213002/diff/480001/ui/display/mojo/displa... File ui/display/mojo/display_struct_traits_unittest.cc (right): https://codereview.chromium.org/2646213002/diff/480001/ui/display/mojo/displa... ui/display/mojo/display_struct_traits_unittest.cc:272: // prepare sample input with random values Please make the comments a proper sentence (eg capitalize start and period at the end). Here and all other comments you've added. https://codereview.chromium.org/2646213002/diff/480001/ui/display/mojo/displa... ui/display/mojo/display_struct_traits_unittest.cc:347: // prepare sample input with random values These aren't random values, please fix the comment to say they are sample values from an external display. https://codereview.chromium.org/2646213002/diff/480001/ui/display/mojo/displa... ui/display/mojo/display_struct_traits_unittest.cc:387: // prepare sample input with random values Ditto above.
Description was changed from ========== Adding mojoms and StructTraits for DisplaySnapshot to support serialization/deserialization via unique_ptr. Enum display_constants struct traits is also added. BUG=649366 ========== to ========== Adding mojoms and StructTraits for DisplaySnapshot. DisplaySnapShot can't be instantiated so DisplaySnapshotMojo is created to support mojom serialization/deserialization. Enum display_constants mojom struct traits is also added to serialize/deserialize DisplayConnectionType. This mojom and struct traits enable the communication between the mus window server and mus gpu in the new split architecture of window server and gpu. BUG=649366 ==========
Patchset #21 (id:500001) has been deleted
Description was changed from ========== Adding mojoms and StructTraits for DisplaySnapshot. DisplaySnapShot can't be instantiated so DisplaySnapshotMojo is created to support mojom serialization/deserialization. Enum display_constants mojom struct traits is also added to serialize/deserialize DisplayConnectionType. This mojom and struct traits enable the communication between the mus window server and mus gpu in the new split architecture of window server and gpu. BUG=649366 ========== to ========== Adding mojoms and StructTraits for DisplaySnapshot. We need to enable communication between the mus window server and mus gpu in the new split architecture of window server and gpu. DisplaySnapShot can't be instantiated so DisplaySnapshotMojo is created to support mojom serialization/deserialization. Enum mojom and struct traits are also added for DisplayConnectionType. Serialization/Deserialization facilitates unique_ptr to move a vector of const DisplayMode between processes. BUG=649366 ==========
Description was changed from ========== Adding mojoms and StructTraits for DisplaySnapshot. We need to enable communication between the mus window server and mus gpu in the new split architecture of window server and gpu. DisplaySnapShot can't be instantiated so DisplaySnapshotMojo is created to support mojom serialization/deserialization. Enum mojom and struct traits are also added for DisplayConnectionType. Serialization/Deserialization facilitates unique_ptr to move a vector of const DisplayMode between processes. BUG=649366 ========== to ========== Adding mojoms and StructTraits for DisplaySnapshot. We need to enable communication between the mus window server and mus gpu in the new split architecture of window server and gpu. DisplaySnapShot can't be instantiated so DisplaySnapshotMojo is created to support mojom serialization/deserialization. Enum mojom and struct traits are also added for DisplayConnectionType. Serialization/Deserialization facilitates unique_ptr to move a vector of const DisplayMode between processes. BUG=649366 ==========
Thanks Kyle. Please review my new cl. Thanh. https://codereview.chromium.org/2646213002/diff/480001/ui/display/mojo/displa... File ui/display/mojo/display_struct_traits_unittest.cc (right): https://codereview.chromium.org/2646213002/diff/480001/ui/display/mojo/displa... ui/display/mojo/display_struct_traits_unittest.cc:272: // prepare sample input with random values On 2017/02/16 14:05:42, kylechar wrote: > Please make the comments a proper sentence (eg capitalize start and period at > the end). Here and all other comments you've added. Done. https://codereview.chromium.org/2646213002/diff/480001/ui/display/mojo/displa... ui/display/mojo/display_struct_traits_unittest.cc:347: // prepare sample input with random values On 2017/02/16 14:05:42, kylechar wrote: > These aren't random values, please fix the comment to say they are sample values > from an external display. Done. https://codereview.chromium.org/2646213002/diff/480001/ui/display/mojo/displa... ui/display/mojo/display_struct_traits_unittest.cc:387: // prepare sample input with random values On 2017/02/16 14:05:42, kylechar wrote: > Ditto above. Done.
The CQ bit was checked by thanhph@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: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Your commit message line length is way over 72 characters. Change "DisplaySnapShot can't be instantiated" => "DisplaySnapshot is abstract" Probably just delete this sentence. "Serialization/Deserialization facilitates unique_ptr to move a vector of const DisplayMode between processes." https://codereview.chromium.org/2646213002/diff/520001/ui/display/display_sna... File ui/display/display_snapshot_mojo.h (right): https://codereview.chromium.org/2646213002/diff/520001/ui/display/display_sna... ui/display/display_snapshot_mojo.h:35: virtual std::unique_ptr<DisplaySnapshotMojo> Clone() const; You missed the comment on this line from patch set 19.
Description was changed from ========== Adding mojoms and StructTraits for DisplaySnapshot. We need to enable communication between the mus window server and mus gpu in the new split architecture of window server and gpu. DisplaySnapShot can't be instantiated so DisplaySnapshotMojo is created to support mojom serialization/deserialization. Enum mojom and struct traits are also added for DisplayConnectionType. Serialization/Deserialization facilitates unique_ptr to move a vector of const DisplayMode between processes. BUG=649366 ========== to ========== Write mojoms and StructTraits for DisplaySnapshot. We need to enable communication between the mus window server and mus gpu in the new split architecture of window server and gpu. DisplaySnapShot is abstract so DisplaySnapshotMojo is created to support mojom serialization/deserialization. Enum mojom and struct traits are also added for DisplayConnectionType. ==========
https://codereview.chromium.org/2646213002/diff/520001/ui/display/display_sna... File ui/display/display_snapshot_mojo.h (right): https://codereview.chromium.org/2646213002/diff/520001/ui/display/display_sna... ui/display/display_snapshot_mojo.h:35: virtual std::unique_ptr<DisplaySnapshotMojo> Clone() const; On 2017/02/16 19:56:46, kylechar wrote: > You missed the comment on this line from patch set 19. I meant if there is any class that implements DisplaySnapshotMojo in the future. If there's none as you mention, I remove virtual here.
The CQ bit was checked by thanhph@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...
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2646213002/diff/540001/ui/display/display_sna... File ui/display/display_snapshot_mojo.cc (right): https://codereview.chromium.org/2646213002/diff/540001/ui/display/display_sna... ui/display/display_snapshot_mojo.cc:60: std::string DisplaySnapshotMojo::ToString() const { My recollection is that this method is used for more than debugging purposes. Please confirm this and implement as necessary in a subsequent CL. https://codereview.chromium.org/2646213002/diff/540001/ui/display/mojo/displa... File ui/display/mojo/display_constants_struct_traits.cc (right): https://codereview.chromium.org/2646213002/diff/540001/ui/display/mojo/displa... ui/display/mojo/display_constants_struct_traits.cc:9: display::mojom::DisplayConnectionType EnumTraits< non-actionable aside: surely the mojo will provide a better way to deal with this someday. https://codereview.chromium.org/2646213002/diff/540001/ui/display/mojo/displa... File ui/display/mojo/display_snapshot_mojo.mojom (right): https://codereview.chromium.org/2646213002/diff/540001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo.mojom:13: struct DisplaySnapshotMojo { you will tear out the innards of display::DisplaySnapshot and replace with an instance of this in a future CL? Or something that accomplishes the same goal? https://codereview.chromium.org/2646213002/diff/540001/ui/display/types/displ... File ui/display/types/display_mode.h (right): https://codereview.chromium.org/2646213002/diff/540001/ui/display/types/displ... ui/display/types/display_mode.h:24: bool operator==(const DisplayMode& other) const; Why do you need this? I'd prefer that it would be unnecessary.
The CQ bit was checked by thanhph@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...
Patchset #23 (id:560001) has been deleted
The CQ bit was checked by thanhph@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.
Thanks Rob, I made some changes below. Please review my cl. Thanh. https://codereview.chromium.org/2646213002/diff/540001/ui/display/display_sna... File ui/display/display_snapshot_mojo.cc (right): https://codereview.chromium.org/2646213002/diff/540001/ui/display/display_sna... ui/display/display_snapshot_mojo.cc:60: std::string DisplaySnapshotMojo::ToString() const { On 2017/02/17 03:03:51, rjkroege wrote: > My recollection is that this method is used for more than debugging purposes. > Please confirm this and implement as necessary in a subsequent CL. I checked a few references to ToString() on code search and all of them are used in debugging purposes. I can implement this method as necessary for subsequent CLs. https://codereview.chromium.org/2646213002/diff/540001/ui/display/mojo/displa... File ui/display/mojo/display_constants_struct_traits.cc (right): https://codereview.chromium.org/2646213002/diff/540001/ui/display/mojo/displa... ui/display/mojo/display_constants_struct_traits.cc:9: display::mojom::DisplayConnectionType EnumTraits< On 2017/02/17 03:03:51, rjkroege wrote: > non-actionable aside: surely the mojo will provide a better way to deal with > this someday. That will be nice. https://codereview.chromium.org/2646213002/diff/540001/ui/display/mojo/displa... File ui/display/mojo/display_snapshot_mojo.mojom (right): https://codereview.chromium.org/2646213002/diff/540001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo.mojom:13: struct DisplaySnapshotMojo { On 2017/02/17 03:03:51, rjkroege wrote: > you will tear out the innards of display::DisplaySnapshot and replace with an > instance of this in a future CL? Or something that accomplishes the same goal? That sounds good to me! https://codereview.chromium.org/2646213002/diff/540001/ui/display/types/displ... File ui/display/types/display_mode.h (right): https://codereview.chromium.org/2646213002/diff/540001/ui/display/types/displ... ui/display/types/display_mode.h:24: bool operator==(const DisplayMode& other) const; On 2017/02/17 03:03:51, rjkroege wrote: > Why do you need this? I'd prefer that it would be unnecessary. I removed this and added back a helper function in files display_struct_traits_unittest.cc and display_snapshot_mojo_struct_traits.cc.
thanhph@google.com changed reviewers: + tsepez@chromium.org
@tsepez: Hi Tom, please review my mojom IPC struct traits. Thanks, Thanh.
https://codereview.chromium.org/2646213002/diff/580001/ui/display/mojo/displa... File ui/display/mojo/display_constants_struct_traits.cc (right): https://codereview.chromium.org/2646213002/diff/580001/ui/display/mojo/displa... ui/display/mojo/display_constants_struct_traits.cc:14: case display::DisplayConnectionType::DISPLAY_CONNECTION_TYPE_NONE: So these values are defined as successive powers of two, suggesting that they might be a bitmask. Do we expect to see a value with more than one of these set?
https://codereview.chromium.org/2646213002/diff/580001/ui/display/mojo/displa... File ui/display/mojo/display_constants_struct_traits.cc (right): https://codereview.chromium.org/2646213002/diff/580001/ui/display/mojo/displa... ui/display/mojo/display_constants_struct_traits.cc:14: case display::DisplayConnectionType::DISPLAY_CONNECTION_TYPE_NONE: On 2017/02/24 17:05:21, Tom Sepez wrote: > So these values are defined as successive powers of two, suggesting that they > might be a bitmask. Do we expect to see a value with more than one of these > set? https://cs.chromium.org/chromium/src/ui/display/types/display_constants.h?q=D... I'm not sure on the specifics for why the enums look like a bitmask, it has something to do with content protection, but the existing code assumes that only one value is set.
OK, LGTM, given that you have full-up switch statements, the mojo side constants could be consecutive, but no matter.
On 2017/02/24 18:22:23, Tom Sepez wrote: > OK, LGTM, given that you have full-up switch statements, the mojo side constants > could be consecutive, but no matter. Great, thanks Tom!
lgtm https://codereview.chromium.org/2646213002/diff/580001/ui/display/display_sna... File ui/display/display_snapshot_mojo.h (right): https://codereview.chromium.org/2646213002/diff/580001/ui/display/display_sna... ui/display/display_snapshot_mojo.h:16: // DisplaySnapshot implementation that can be used with Mojo IPC. This exists because we have a refactoring that's not done right? Because really we could use the mojo struct everywhere in the future? In which case, could you note/record this in a TODO and/or a bug before landing so that this code cleanup actually happens.
Thanks Rob. I added a clean up bug. Thanh. https://codereview.chromium.org/2646213002/diff/580001/ui/display/display_sna... File ui/display/display_snapshot_mojo.h (right): https://codereview.chromium.org/2646213002/diff/580001/ui/display/display_sna... ui/display/display_snapshot_mojo.h:16: // DisplaySnapshot implementation that can be used with Mojo IPC. On 2017/03/01 15:46:27, rjkroege wrote: > This exists because we have a refactoring that's not done right? Because really > we could use the mojo struct everywhere in the future? In which case, could you > note/record this in a TODO and/or a bug before landing so that this code cleanup > actually happens. Sounds good! We have this class because we can't instantiate DisplaySnapShot due to ToString() method. I added https://bugs.chromium.org/p/chromium/issues/detail?id=697475 to easily track our cleanup efforts in the future.
thanhph@google.com changed reviewers: + oshima@chromium.org
oshima@chromium.org: Hi Mitsuru, please review changes in ui/display tsepez@chromium.org: Hi Tom, please review changes in mojom struct traits. Thanks folks, Thanh.
thanhph@chromium.org changed reviewers: + derat@chromium.org - oshima@chromium.org
Hi Dan, Mitsuru is OOO. Could you review my CL? Thanks, Thanh.
lgtm for ui/display/ https://codereview.chromium.org/2646213002/diff/580001/ui/display/mojo/displa... File ui/display/mojo/display_constants_struct_traits.cc (right): https://codereview.chromium.org/2646213002/diff/580001/ui/display/mojo/displa... ui/display/mojo/display_constants_struct_traits.cc:14: case display::DisplayConnectionType::DISPLAY_CONNECTION_TYPE_NONE: On 2017/02/24 17:51:20, kylechar wrote: > On 2017/02/24 17:05:21, Tom Sepez wrote: > > So these values are defined as successive powers of two, suggesting that they > > might be a bitmask. Do we expect to see a value with more than one of these > > set? > > https://cs.chromium.org/chromium/src/ui/display/types/display_constants.h?q=D... > > I'm not sure on the specifics for why the enums look like a bitmask, it has > something to do with content protection, but the existing code assumes that only > one value is set. i don't know/remember the background here either, but it looks like it may be for some content-protection-specific code. see QueryProtectionCallback in ui/display/manager/chromeos/display_configurator.h, for instance. https://codereview.chromium.org/2646213002/diff/580001/ui/display/mojo/displa... File ui/display/mojo/display_snapshot_mojo_struct_traits.cc (right): https://codereview.chromium.org/2646213002/diff/580001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo_struct_traits.cc:18: // Returns the index of |mode| in |modes| or uint64_t max if |mode| is null. "... or not found." https://codereview.chromium.org/2646213002/diff/580001/ui/display/mojo/displa... File ui/display/mojo/display_struct_traits_test.mojom (right): https://codereview.chromium.org/2646213002/diff/580001/ui/display/mojo/displa... ui/display/mojo/display_struct_traits_test.mojom:28: }; i'm not sure what rietveld's highlighting of this line means. did the old file lack a trailing newline?
Thanks for the review Dan! I updated the comment and remove the white space and sending the patch to CQ. Thanks, Thanh. https://codereview.chromium.org/2646213002/diff/580001/ui/display/mojo/displa... File ui/display/mojo/display_constants_struct_traits.cc (right): https://codereview.chromium.org/2646213002/diff/580001/ui/display/mojo/displa... ui/display/mojo/display_constants_struct_traits.cc:14: case display::DisplayConnectionType::DISPLAY_CONNECTION_TYPE_NONE: On 2017/03/07 23:47:01, Daniel Erat wrote: > On 2017/02/24 17:51:20, kylechar wrote: > > On 2017/02/24 17:05:21, Tom Sepez wrote: > > > So these values are defined as successive powers of two, suggesting that > they > > > might be a bitmask. Do we expect to see a value with more than one of these > > > set? > > > > > https://cs.chromium.org/chromium/src/ui/display/types/display_constants.h?q=D... > > > > I'm not sure on the specifics for why the enums look like a bitmask, it has > > something to do with content protection, but the existing code assumes that > only > > one value is set. > > i don't know/remember the background here either, but it looks like it may be > for some content-protection-specific code. see QueryProtectionCallback in > ui/display/manager/chromeos/display_configurator.h, for instance. Acknowledged. Thanks for the link! https://codereview.chromium.org/2646213002/diff/580001/ui/display/mojo/displa... File ui/display/mojo/display_snapshot_mojo_struct_traits.cc (right): https://codereview.chromium.org/2646213002/diff/580001/ui/display/mojo/displa... ui/display/mojo/display_snapshot_mojo_struct_traits.cc:18: // Returns the index of |mode| in |modes| or uint64_t max if |mode| is null. On 2017/03/07 23:47:01, Daniel Erat wrote: > "... or not found." Done. https://codereview.chromium.org/2646213002/diff/580001/ui/display/mojo/displa... File ui/display/mojo/display_struct_traits_test.mojom (right): https://codereview.chromium.org/2646213002/diff/580001/ui/display/mojo/displa... ui/display/mojo/display_struct_traits_test.mojom:28: }; On 2017/03/07 23:47:01, Daniel Erat wrote: > i'm not sure what rietveld's highlighting of this line means. did the old file > lack a trailing newline? The old file doesn't have this newline so I removed it here. Thanks!
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, rjkroege@chromium.org, tsepez@chromium.org, derat@chromium.org Link to the patchset: https://codereview.chromium.org/2646213002/#ps600001 (title: "Fix nits.")
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: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Description was changed from ========== Write mojoms and StructTraits for DisplaySnapshot. We need to enable communication between the mus window server and mus gpu in the new split architecture of window server and gpu. DisplaySnapShot is abstract so DisplaySnapshotMojo is created to support mojom serialization/deserialization. Enum mojom and struct traits are also added for DisplayConnectionType. ========== to ========== Write mojoms and StructTraits for DisplaySnapshot. We need to enable communication between the mus window server and mus gpu in the new split architecture of window server and gpu. DisplaySnapShot is abstract so DisplaySnapshotMojo is created to support mojom serialization/deserialization. Enum mojom and struct traits are also added for DisplayConnectionType. BUG=649366 ==========
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: 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 rjkroege@chromium.org, tsepez@chromium.org, kylechar@chromium.org, derat@chromium.org Link to the patchset: https://codereview.chromium.org/2646213002/#ps620001 (title: "Rebase.")
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": 620001, "attempt_start_ts": 1488989924541490,
"parent_rev": "fc0a791f782a8963a46425367140ac5ed86b3b83", "commit_rev":
"e29136a35e8282ad38b3504915837eb26425918c"}
Message was sent while issue was closed.
Description was changed from ========== Write mojoms and StructTraits for DisplaySnapshot. We need to enable communication between the mus window server and mus gpu in the new split architecture of window server and gpu. DisplaySnapShot is abstract so DisplaySnapshotMojo is created to support mojom serialization/deserialization. Enum mojom and struct traits are also added for DisplayConnectionType. BUG=649366 ========== to ========== Write mojoms and StructTraits for DisplaySnapshot. We need to enable communication between the mus window server and mus gpu in the new split architecture of window server and gpu. DisplaySnapShot is abstract so DisplaySnapshotMojo is created to support mojom serialization/deserialization. Enum mojom and struct traits are also added for DisplayConnectionType. BUG=649366 Review-Url: https://codereview.chromium.org/2646213002 Cr-Commit-Position: refs/heads/master@{#455466} Committed: https://chromium.googlesource.com/chromium/src/+/e29136a35e8282ad38b350491583... ==========
Message was sent while issue was closed.
Committed patchset #25 (id:620001) as https://chromium.googlesource.com/chromium/src/+/e29136a35e8282ad38b350491583... |
