Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2297743002:
Process DisplaySnapshots in PlatformScreen. (Closed)
DescriptionProcess DisplaySnapshots in PlatformScreen.
Add logic to process DisplaySnapshots from the DisplayConfigurator.
Process the contents of the snapshots to find new, removed or modified
displays.
Add unit tests to test that the correct calls are made to the
PlatformScreenDelegate in response to receiving an updated list of
display snapshots.
BUG=641012
Committed: https://crrev.com/80f206eec2f4ba6ec5485d6a40ac5a9af968ab38
Cr-Commit-Position: refs/heads/master@{#416657}
Patch Set 1 #Patch Set 2 : Formatting. #Patch Set 3 : Fix test. #
Total comments: 25
Patch Set 4 : Rebase and address comments. #Patch Set 5 : Update tests. #
Total comments: 29
Patch Set 6 : Address comments. #
Total comments: 7
Patch Set 7 : More fixes. #
Total comments: 2
Patch Set 8 : Add comment. #
Dependent Patchsets: Messages
Total messages: 24 (10 generated)
kylechar@chromium.org changed reviewers: + rjkroege@chromium.org
rjkroege: any thoughts before I send it off to sky for OWNERS review?
https://codereview.chromium.org/2297743002/diff/40001/services/ui/display/pla... File services/ui/display/platform_screen_impl_ozone.cc (right): https://codereview.chromium.org/2297743002/diff/40001/services/ui/display/pla... services/ui/display/platform_screen_impl_ozone.cc:26: class DisplayIdPredicate { what is this for? https://codereview.chromium.org/2297743002/diff/40001/services/ui/display/pla... services/ui/display/platform_screen_impl_ozone.cc:29: bool operator()(const PlatformScreenImplOzone::DisplayInfo& display) const { why not operator== for a comparison op? Or perhaps I'm not understanding. https://codereview.chromium.org/2297743002/diff/40001/services/ui/display/pla... services/ui/display/platform_screen_impl_ozone.cc:85: // Find cached displays with no matching snapshot and mark as removed. we should be replacing this code with the guts of display_change_observer_chromeos.cc as soon as it's pushed into ui yes? i.e.: we will refactor both appropriately? https://codereview.chromium.org/2297743002/diff/40001/services/ui/display/pla... services/ui/display/platform_screen_impl_ozone.cc:109: auto iter = std::find_if(cached_displays_.begin(), cached_displays_.end(), I'm probably in the minority but I think that a for loop is easier to read than find_if. https://codereview.chromium.org/2297743002/diff/40001/services/ui/display/pla... services/ui/display/platform_screen_impl_ozone.cc:127: // removed or modified displays. This ensures that we only send one update per per displays? This is not clear to me. https://codereview.chromium.org/2297743002/diff/40001/services/ui/display/pla... services/ui/display/platform_screen_impl_ozone.cc:135: delegate_->OnDisplayRemoved(display_info.id); we should have a discussion about the delegate_ api https://codereview.chromium.org/2297743002/diff/40001/services/ui/display/pla... File services/ui/display/platform_screen_impl_ozone.h (right): https://codereview.chromium.org/2297743002/diff/40001/services/ui/display/pla... services/ui/display/platform_screen_impl_ozone.h:26: // TODO(kylechar): Replace with something like ash::DisplayInfo after it's fyi: is in the commit queue You need to say what a DisplayInfo is. https://codereview.chromium.org/2297743002/diff/40001/services/ui/display/pla... File services/ui/display/platform_screen_ozone_unittests.cc (right): https://codereview.chromium.org/2297743002/diff/40001/services/ui/display/pla... services/ui/display/platform_screen_ozone_unittests.cc:29: const int64_t kDefaultDisplayId = 36028797018963969; reasoning for number? https://codereview.chromium.org/2297743002/diff/40001/services/ui/display/pla... services/ui/display/platform_screen_ozone_unittests.cc:40: class DisplayIdPredicate { can't you just have a "FindForID()" method instead? https://codereview.chromium.org/2297743002/diff/40001/services/ui/display/pla... services/ui/display/platform_screen_ozone_unittests.cc:52: class TestPlatformScreenDelegate : public PlatformScreenDelegate { seems like mocks would be handy? Is mus allowed to use mocks? https://codereview.chromium.org/2297743002/diff/40001/services/ui/ws/display_... File services/ui/ws/display_manager.cc (right): https://codereview.chromium.org/2297743002/diff/40001/services/ui/ws/display_... services/ui/ws/display_manager.cc:107: Display* DisplayManager::GetDisplayById(int64_t display_id) { So: this display_id is a an id of a ui::Display right? which as we know is not the same as the ws::Display. Right? So... this feels like a layering violation to me. https://codereview.chromium.org/2297743002/diff/40001/services/ui/ws/display_... services/ui/ws/display_manager.cc:186: Display* display = GetDisplayById(id); The PlatformDisplay should be the delegate and get told to destroy itself? It will manage its storage yes?
https://codereview.chromium.org/2297743002/diff/40001/services/ui/display/pla... File services/ui/display/platform_screen_impl_ozone.cc (right): https://codereview.chromium.org/2297743002/diff/40001/services/ui/display/pla... services/ui/display/platform_screen_impl_ozone.cc:26: class DisplayIdPredicate { On 2016/08/30 18:41:59, rjkroege wrote: > what is this for? It's for find_if. We only have a int64_t id and we want to match on a DisplayInfo object with the same int64_t id (but the other attributes are unknown). You would have to implement == that only looks at the id field for it to be equivalent. I've removed the find_ifs though. https://codereview.chromium.org/2297743002/diff/40001/services/ui/display/pla... services/ui/display/platform_screen_impl_ozone.cc:29: bool operator()(const PlatformScreenImplOzone::DisplayInfo& display) const { On 2016/08/30 18:41:59, rjkroege wrote: > why not operator== for a comparison op? Or perhaps I'm not understanding. See above. https://codereview.chromium.org/2297743002/diff/40001/services/ui/display/pla... services/ui/display/platform_screen_impl_ozone.cc:85: // Find cached displays with no matching snapshot and mark as removed. On 2016/08/30 18:41:59, rjkroege wrote: > we should be replacing this code with the guts of > display_change_observer_chromeos.cc as soon as it's pushed into ui yes? i.e.: we > will refactor both appropriately? Yep something like that. I need some business logic here to test that display <-> ws <-> ash interaction works (which it definitely doesn't right now). https://codereview.chromium.org/2297743002/diff/40001/services/ui/display/pla... services/ui/display/platform_screen_impl_ozone.cc:109: auto iter = std::find_if(cached_displays_.begin(), cached_displays_.end(), On 2016/08/30 18:41:59, rjkroege wrote: > I'm probably in the minority but I think that a for loop is easier to read than > find_if. I think you're right here. I already had DisplayIdPredicate so I reused it but it's much harder to read than a loop. https://codereview.chromium.org/2297743002/diff/40001/services/ui/display/pla... services/ui/display/platform_screen_impl_ozone.cc:127: // removed or modified displays. This ensures that we only send one update per On 2016/08/30 18:41:59, rjkroege wrote: > per displays? This is not clear to me. It should be per display, typo. Fixed. https://codereview.chromium.org/2297743002/diff/40001/services/ui/display/pla... services/ui/display/platform_screen_impl_ozone.cc:135: delegate_->OnDisplayRemoved(display_info.id); On 2016/08/30 18:41:59, rjkroege wrote: > we should have a discussion about the delegate_ api As per our offline discussion there are a bunch of things that can be changed about the API. They're outside the scope of this CL though. https://codereview.chromium.org/2297743002/diff/40001/services/ui/display/pla... File services/ui/display/platform_screen_impl_ozone.h (right): https://codereview.chromium.org/2297743002/diff/40001/services/ui/display/pla... services/ui/display/platform_screen_impl_ozone.h:26: // TODO(kylechar): Replace with something like ash::DisplayInfo after it's On 2016/08/30 18:41:59, rjkroege wrote: > fyi: is in the commit queue > > You need to say what a DisplayInfo is. I've updated the comment. I see display::ManagedDisplayInfo exists now but I guess that's not totally usable yet without the rest of the display management code. https://codereview.chromium.org/2297743002/diff/40001/services/ui/display/pla... File services/ui/display/platform_screen_ozone_unittests.cc (right): https://codereview.chromium.org/2297743002/diff/40001/services/ui/display/pla... services/ui/display/platform_screen_ozone_unittests.cc:29: const int64_t kDefaultDisplayId = 36028797018963969; On 2016/08/30 18:41:59, rjkroege wrote: > reasoning for number? Added comment. https://codereview.chromium.org/2297743002/diff/40001/services/ui/display/pla... services/ui/display/platform_screen_ozone_unittests.cc:40: class DisplayIdPredicate { On 2016/08/30 18:41:59, rjkroege wrote: > can't you just have a "FindForID()" method instead? I added that where it's clearer. I've also just used a lambda for the remove part where I actually need an iterator instead of wanting a pointer. https://codereview.chromium.org/2297743002/diff/40001/services/ui/display/pla... services/ui/display/platform_screen_ozone_unittests.cc:52: class TestPlatformScreenDelegate : public PlatformScreenDelegate { On 2016/08/30 18:41:59, rjkroege wrote: > seems like mocks would be handy? Is mus allowed to use mocks? I'm not sure I understand the mocks bit since PlatformScreenDelegate is pure virtual. If you mean using gmock matchers then yes, that would be nicer and I've switched it the tests to use them. https://codereview.chromium.org/2297743002/diff/40001/services/ui/ws/display_... File services/ui/ws/display_manager.cc (right): https://codereview.chromium.org/2297743002/diff/40001/services/ui/ws/display_... services/ui/ws/display_manager.cc:107: Display* DisplayManager::GetDisplayById(int64_t display_id) { On 2016/08/30 18:41:59, rjkroege wrote: > So: this display_id is a an id of a ui::Display right? which as we know is not > the same as the ws::Display. Right? So... this feels like a layering violation > to me. We're using the same id in all places. https://codereview.chromium.org/2297743002/diff/40001/services/ui/ws/display_... services/ui/ws/display_manager.cc:186: Display* display = GetDisplayById(id); On 2016/08/30 18:41:59, rjkroege wrote: > The PlatformDisplay should be the delegate and get told to destroy itself? It > will manage its storage yes? I'm not sure I agree/understand. Let's discuss at the meeting today. I also copied and pasted this in from service.cc and wasn't paying enough attention because I'm getting the display manager.. from the window server.. from the display manager. Fixed.
https://codereview.chromium.org/2297743002/diff/40001/services/ui/ws/display_... File services/ui/ws/display_manager.cc (right): https://codereview.chromium.org/2297743002/diff/40001/services/ui/ws/display_... services/ui/ws/display_manager.cc:186: Display* display = GetDisplayById(id); On 2016/08/31 15:58:01, kylechar wrote: > On 2016/08/30 18:41:59, rjkroege wrote: > > The PlatformDisplay should be the delegate and get told to destroy itself? It > > will manage its storage yes? > > I'm not sure I agree/understand. Let's discuss at the meeting today. > > I also copied and pasted this in from service.cc and wasn't paying enough > attention because I'm getting the display manager.. from the window server.. > from the display manager. Fixed. Should have changed that comment. As per the discussion offline I understand what you were saying.
kylechar@chromium.org changed reviewers: + sky@chromium.org
+sky for OWNERS review.
https://codereview.chromium.org/2297743002/diff/80001/services/ui/display/pla... File services/ui/display/platform_screen_impl_ozone.cc (right): https://codereview.chromium.org/2297743002/diff/80001/services/ui/display/pla... services/ui/display/platform_screen_impl_ozone.cc:70: for (ui::DisplaySnapshot* snapshot : snapshots) { no {} https://codereview.chromium.org/2297743002/diff/80001/services/ui/display/pla... services/ui/display/platform_screen_impl_ozone.cc:106: if (display_info) { Move inside look on 101 so you don't need local. https://codereview.chromium.org/2297743002/diff/80001/services/ui/display/pla... services/ui/display/platform_screen_impl_ozone.cc:121: while (iter != cached_displays_.end()) { Use a for loop, that way iter stays local to the loop. https://codereview.chromium.org/2297743002/diff/80001/services/ui/display/pla... services/ui/display/platform_screen_impl_ozone.cc:125: delegate_->OnDisplayRemoved(display_info.id); Did you consider updating cached_displays_ and then notifying the delegate? Otherwise if the delegate calls back cached_displays_ is in a wierd state. https://codereview.chromium.org/2297743002/diff/80001/services/ui/display/pla... services/ui/display/platform_screen_impl_ozone.cc:129: if (next_display_origin_ != display_info.bounds.origin()) { no {} https://codereview.chromium.org/2297743002/diff/80001/services/ui/display/pla... services/ui/display/platform_screen_impl_ozone.cc:139: iter++; ++iter. https://codereview.chromium.org/2297743002/diff/80001/services/ui/display/pla... services/ui/display/platform_screen_impl_ozone.cc:150: if (std::find_if(cached_displays_.begin(), cached_displays_.end(), It seems worth a function for this, given the loop on 99-100 is effectively doing the same thing. I'm specifically suggesting something like: FindCachedDisplayIteratorByDisplayId() that returns an iterator into cached_displays_ based on supplied id. That name is wordy, but you get the idea. https://codereview.chromium.org/2297743002/diff/80001/services/ui/display/pla... File services/ui/display/platform_screen_impl_ozone.h (right): https://codereview.chromium.org/2297743002/diff/80001/services/ui/display/pla... services/ui/display/platform_screen_impl_ozone.h:28: struct DisplayInfo { Does this need to be public? https://codereview.chromium.org/2297743002/diff/80001/services/ui/display/pla... services/ui/display/platform_screen_impl_ozone.h:29: DisplayInfo(int64_t new_id, gfx::Rect new_bounds) const gfx::rect& https://codereview.chromium.org/2297743002/diff/80001/services/ui/display/pla... services/ui/display/platform_screen_impl_ozone.h:35: // The display bounds that the delegate knows about. clarify what 'knows about' means here. That said, why do you need this? It seems like in the one place you use this you could copy the bounds to a local before updating the bounds and then compare the two there. https://codereview.chromium.org/2297743002/diff/80001/services/ui/display/pla... services/ui/display/platform_screen_impl_ozone.h:48: // Process display snapshots and mark removed displays as removed. Does not Document what 'process' means here. I'm suggesting something like "Sets |removed| on any displays that have been removed. Updates |primary_display_id_| if the primary display was removed. https://codereview.chromium.org/2297743002/diff/80001/services/ui/display/pla... services/ui/display/platform_screen_impl_ozone.h:53: // Processes display snapshots and updates the bounds of an existing displays. remove 'an' https://codereview.chromium.org/2297743002/diff/80001/services/ui/display/pla... File services/ui/display/platform_screen_ozone_unittests.cc (right): https://codereview.chromium.org/2297743002/diff/80001/services/ui/display/pla... services/ui/display/platform_screen_ozone_unittests.cc:1: // Copyright 2016 The Chromium Authors. All rights reserved. Should this filename by platform_screen_ozone_impl_unittests to match name of file it's a test for? https://codereview.chromium.org/2297743002/diff/80001/services/ui/display/pla... services/ui/display/platform_screen_ozone_unittests.cc:118: ASSERT_FALSE(GetSnapshotById(id)); If you're going to have ASSERTS in function like this, then you you need to wrap call sites in ASSERT_NO_FATAL_FAILURE, otherwise the test continues on after this call fails.
Patchset #6 (id:100001) has been deleted
https://codereview.chromium.org/2297743002/diff/80001/services/ui/display/pla... File services/ui/display/platform_screen_impl_ozone.cc (right): https://codereview.chromium.org/2297743002/diff/80001/services/ui/display/pla... services/ui/display/platform_screen_impl_ozone.cc:70: for (ui::DisplaySnapshot* snapshot : snapshots) { On 2016/09/01 19:25:08, sky wrote: > no {} Done. https://codereview.chromium.org/2297743002/diff/80001/services/ui/display/pla... services/ui/display/platform_screen_impl_ozone.cc:106: if (display_info) { On 2016/09/01 19:25:08, sky wrote: > Move inside look on 101 so you don't need local. Done. https://codereview.chromium.org/2297743002/diff/80001/services/ui/display/pla... services/ui/display/platform_screen_impl_ozone.cc:121: while (iter != cached_displays_.end()) { On 2016/09/01 19:25:08, sky wrote: > Use a for loop, that way iter stays local to the loop. Done. https://codereview.chromium.org/2297743002/diff/80001/services/ui/display/pla... services/ui/display/platform_screen_impl_ozone.cc:125: delegate_->OnDisplayRemoved(display_info.id); On 2016/09/01 19:25:07, sky wrote: > Did you consider updating cached_displays_ and then notifying the delegate? > Otherwise if the delegate calls back cached_displays_ is in a wierd state. Done. https://codereview.chromium.org/2297743002/diff/80001/services/ui/display/pla... services/ui/display/platform_screen_impl_ozone.cc:129: if (next_display_origin_ != display_info.bounds.origin()) { On 2016/09/01 19:25:08, sky wrote: > no {} Done. (Java styleguide habits are hard to kick.) https://codereview.chromium.org/2297743002/diff/80001/services/ui/display/pla... services/ui/display/platform_screen_impl_ozone.cc:139: iter++; On 2016/09/01 19:25:08, sky wrote: > ++iter. Done. https://codereview.chromium.org/2297743002/diff/80001/services/ui/display/pla... services/ui/display/platform_screen_impl_ozone.cc:150: if (std::find_if(cached_displays_.begin(), cached_displays_.end(), On 2016/09/01 19:25:07, sky wrote: > It seems worth a function for this, given the loop on 99-100 is effectively > doing the same thing. I'm specifically suggesting something like: > FindCachedDisplayIteratorByDisplayId() that returns an iterator into > cached_displays_ based on supplied id. That name is wordy, but you get the idea. I had something similar in patch 3 but removed it. Added back. https://codereview.chromium.org/2297743002/diff/80001/services/ui/display/pla... File services/ui/display/platform_screen_impl_ozone.h (right): https://codereview.chromium.org/2297743002/diff/80001/services/ui/display/pla... services/ui/display/platform_screen_impl_ozone.h:28: struct DisplayInfo { On 2016/09/01 19:25:08, sky wrote: > Does this need to be public? Not anymore (it did at a previous revision). Move to private. https://codereview.chromium.org/2297743002/diff/80001/services/ui/display/pla... services/ui/display/platform_screen_impl_ozone.h:29: DisplayInfo(int64_t new_id, gfx::Rect new_bounds) On 2016/09/01 19:25:08, sky wrote: > const gfx::rect& Done. https://codereview.chromium.org/2297743002/diff/80001/services/ui/display/pla... services/ui/display/platform_screen_impl_ozone.h:35: // The display bounds that the delegate knows about. On 2016/09/01 19:25:08, sky wrote: > clarify what 'knows about' means here. > That said, why do you need this? It seems like in the one place you use this you > could copy the bounds to a local before updating the bounds and then compare the > two there. Clarified comment. As to why, the bounds can get changed in both ProcessModifiedDisplays() and UpdateCachedDisplays() so a local won't work. If either are different then I call OnDisplayModified(). I'm doing it this way so that DisplayManager gets the absolutely minimum number of updates. https://codereview.chromium.org/2297743002/diff/80001/services/ui/display/pla... services/ui/display/platform_screen_impl_ozone.h:48: // Process display snapshots and mark removed displays as removed. Does not On 2016/09/01 19:25:08, sky wrote: > Document what 'process' means here. I'm suggesting something like "Sets > |removed| on any displays that have been removed. Updates |primary_display_id_| > if the primary display was removed. Done. https://codereview.chromium.org/2297743002/diff/80001/services/ui/display/pla... services/ui/display/platform_screen_impl_ozone.h:53: // Processes display snapshots and updates the bounds of an existing displays. On 2016/09/01 19:25:08, sky wrote: > remove 'an' Done. I've updated the method descriptions to be more explanatory in general. https://codereview.chromium.org/2297743002/diff/80001/services/ui/display/pla... File services/ui/display/platform_screen_ozone_unittests.cc (right): https://codereview.chromium.org/2297743002/diff/80001/services/ui/display/pla... services/ui/display/platform_screen_ozone_unittests.cc:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2016/09/01 19:25:08, sky wrote: > Should this filename by platform_screen_ozone_impl_unittests to match name of > file it's a test for? I'm planning on renaming the others to PlatformScreenStub and PlatformScreenOzone soon. Want me to change the name here to match the current files and then update it later or just leave it like this? https://codereview.chromium.org/2297743002/diff/80001/services/ui/display/pla... services/ui/display/platform_screen_ozone_unittests.cc:118: ASSERT_FALSE(GetSnapshotById(id)); On 2016/09/01 19:25:08, sky wrote: > If you're going to have ASSERTS in function like this, then you you need to wrap > call sites in ASSERT_NO_FATAL_FAILURE, otherwise the test continues on after > this call fails. Oh, I didn't realize it worked that way with gtest. Hmm. Reading through go/unit-test-practices they seem to discourage ASSERTS in the "arrange" stage so I'll just leave them out. The tests will fail if snapshots aren't setup correctly. I could also have these methods return false if something is wrong. https://codereview.chromium.org/2297743002/diff/120001/services/ui/display/pl... File services/ui/display/platform_screen_impl_ozone.cc (right): https://codereview.chromium.org/2297743002/diff/120001/services/ui/display/pl... services/ui/display/platform_screen_impl_ozone.cc:159: auto PlatformScreenImplOzone::GetCachedDisplayIterator(int64_t display_id) std::vector<PlatformScreenImplOzone::DisplayInfo>::iterator seems less readable than just using a trailing return type to me but I can change it if desired.
https://codereview.chromium.org/2297743002/diff/80001/services/ui/display/pla... File services/ui/display/platform_screen_impl_ozone.h (right): https://codereview.chromium.org/2297743002/diff/80001/services/ui/display/pla... services/ui/display/platform_screen_impl_ozone.h:35: // The display bounds that the delegate knows about. On 2016/09/02 15:18:34, kylechar wrote: > On 2016/09/01 19:25:08, sky wrote: > > clarify what 'knows about' means here. > > That said, why do you need this? It seems like in the one place you use this > you > > could copy the bounds to a local before updating the bounds and then compare > the > > two there. > > Clarified comment. As to why, the bounds can get changed in both > ProcessModifiedDisplays() and UpdateCachedDisplays() so a local won't work. If > either are different then I call OnDisplayModified(). I'm doing it this way so > that DisplayManager gets the absolutely minimum number of updates. I'm likely missing something. See my other comment. https://codereview.chromium.org/2297743002/diff/120001/services/ui/display/pl... File services/ui/display/platform_screen_impl_ozone.cc (right): https://codereview.chromium.org/2297743002/diff/120001/services/ui/display/pl... services/ui/display/platform_screen_impl_ozone.cc:101: if (current_mode->size() != display_info.bounds.size()) { no {} https://codereview.chromium.org/2297743002/diff/120001/services/ui/display/pl... services/ui/display/platform_screen_impl_ozone.cc:128: delegate_->OnDisplayModified(display_info.id, display_info.bounds); This is the only place I see you checking/setting known_bounds, where it seems like a local would work. Am I missing something? https://codereview.chromium.org/2297743002/diff/120001/services/ui/display/pl... services/ui/display/platform_screen_impl_ozone.cc:159: auto PlatformScreenImplOzone::GetCachedDisplayIterator(int64_t display_id) On 2016/09/02 15:18:34, kylechar wrote: > std::vector<PlatformScreenImplOzone::DisplayInfo>::iterator seems less readable > than just using a trailing return type to me but I can change it if desired. I have to admit I'm surprised this actually compiles. I prefer something close to what you mention: PlatformScreenImplOzone::CachedDisplayIterator ...
https://codereview.chromium.org/2297743002/diff/120001/services/ui/display/pl... File services/ui/display/platform_screen_impl_ozone.cc (right): https://codereview.chromium.org/2297743002/diff/120001/services/ui/display/pl... services/ui/display/platform_screen_impl_ozone.cc:101: if (current_mode->size() != display_info.bounds.size()) { On 2016/09/02 16:03:21, sky wrote: > no {} Done. (Sorry, Java style guide habits are hard to break.) https://codereview.chromium.org/2297743002/diff/120001/services/ui/display/pl... services/ui/display/platform_screen_impl_ozone.cc:128: delegate_->OnDisplayModified(display_info.id, display_info.bounds); On 2016/09/02 16:03:21, sky wrote: > This is the only place I see you checking/setting known_bounds, where it seems > like a local would work. Am I missing something? Hmm, right. I'm setting bounds in two places and comparing known_bounds here. You're right though, I don't actually need the old bounds I just need to know it's modified and a bool will do the same job. Changed. https://codereview.chromium.org/2297743002/diff/120001/services/ui/display/pl... services/ui/display/platform_screen_impl_ozone.cc:159: auto PlatformScreenImplOzone::GetCachedDisplayIterator(int64_t display_id) On 2016/09/02 16:03:21, sky wrote: > On 2016/09/02 15:18:34, kylechar wrote: > > std::vector<PlatformScreenImplOzone::DisplayInfo>::iterator seems less > readable > > than just using a trailing return type to me but I can change it if desired. > > I have to admit I'm surprised this actually compiles. I prefer something close > to what you mention: > > PlatformScreenImplOzone::CachedDisplayIterator ... Changed. (I was surprised trailing return types were allowed in the Chromium style guide!)
LGTM https://codereview.chromium.org/2297743002/diff/140001/services/ui/display/pl... File services/ui/display/platform_screen_impl_ozone.h (right): https://codereview.chromium.org/2297743002/diff/140001/services/ui/display/pl... services/ui/display/platform_screen_impl_ozone.h:43: bool modified = false; Add a description as to what this means and intended uses.
Thanks sky! https://codereview.chromium.org/2297743002/diff/140001/services/ui/display/pl... File services/ui/display/platform_screen_impl_ozone.h (right): https://codereview.chromium.org/2297743002/diff/140001/services/ui/display/pl... services/ui/display/platform_screen_impl_ozone.h:43: bool modified = false; On 2016/09/02 18:29:28, sky wrote: > Add a description as to what this means and intended uses. Done.
The CQ bit was checked by kylechar@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 kylechar@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org Link to the patchset: https://codereview.chromium.org/2297743002/#ps160001 (title: "Add comment.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #8 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Process DisplaySnapshots in PlatformScreen. Add logic to process DisplaySnapshots from the DisplayConfigurator. Process the contents of the snapshots to find new, removed or modified displays. Add unit tests to test that the correct calls are made to the PlatformScreenDelegate in response to receiving an updated list of display snapshots. BUG=641012 ========== to ========== Process DisplaySnapshots in PlatformScreen. Add logic to process DisplaySnapshots from the DisplayConfigurator. Process the contents of the snapshots to find new, removed or modified displays. Add unit tests to test that the correct calls are made to the PlatformScreenDelegate in response to receiving an updated list of display snapshots. BUG=641012 Committed: https://crrev.com/80f206eec2f4ba6ec5485d6a40ac5a9af968ab38 Cr-Commit-Position: refs/heads/master@{#416657} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/80f206eec2f4ba6ec5485d6a40ac5a9af968ab38 Cr-Commit-Position: refs/heads/master@{#416657} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
