Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11140006:
Fix code around display overscan settings. (Closed)
|
Created:
8 years, 2 months ago by Jun Mukai Modified:
8 years, 2 months ago CC:
chromium-reviews, sadrul, ben+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionFix code around display overscan settings.
The current logic is wrong actually in case of updating overscan insets several times.
To replace the existing insets by new one, the existing insets has to be reimbursed beforehand.
BUG=139419
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=162570
Patch Set 1 #
Total comments: 1
Patch Set 2 : #
Total comments: 6
Patch Set 3 : #Patch Set 4 : #
Total comments: 7
Patch Set 5 : #Patch Set 6 : #
Total comments: 2
Patch Set 7 : #Messages
Total messages: 18 (0 generated)
oshima-san, could you review this CL?
I think it's better to store native bounds in MultiDisplayManager. Let's discuss offline. https://codereview.chromium.org/11140006/diff/1/ui/gfx/display.cc File ui/gfx/display.cc (right): https://codereview.chromium.org/11140006/diff/1/ui/gfx/display.cc#newcode115 ui/gfx/display.cc:115: SetScaleAndBounds(device_scale_factor_, bounds_in_pixel); this updates bounds_in_pixel, so it will keep shrinking as you call.
moved the all code to multi display manager as we talked offline.
http://codereview.chromium.org/11140006/diff/6001/ash/display/multi_display_m... File ash/display/multi_display_manager.cc (right): http://codereview.chromium.org/11140006/diff/6001/ash/display/multi_display_m... ash/display/multi_display_manager.cc:155: for (DisplayList::iterator iter = displays_.begin(); FindDisplayForId http://codereview.chromium.org/11140006/diff/6001/ash/display/multi_display_m... ash/display/multi_display_manager.cc:161: bounds.Inset(negated_insets.Scale(iter->device_scale_factor())); Scale(- iter->device_scale_factor()); (or -1 *)
https://codereview.chromium.org/11140006/diff/6001/ash/display/multi_display_... File ash/display/multi_display_manager.cc (right): https://codereview.chromium.org/11140006/diff/6001/ash/display/multi_display_... ash/display/multi_display_manager.cc:155: for (DisplayList::iterator iter = displays_.begin(); On 2012/10/16 23:54:17, oshima wrote: > FindDisplayForId Thanks. But as far as I see, it returns a const reference, so the code to reimburse the existing insets could be awkward -- I want to edit an existing display in the list. Also it raises DLOG(FATAL) in case no displays are found, but it wouldn't be exceptional here. By the way, I found GetDisplayForId() too, and both are almost identical. Why do we have both? https://codereview.chromium.org/11140006/diff/6001/ash/display/multi_display_... ash/display/multi_display_manager.cc:161: bounds.Inset(negated_insets.Scale(iter->device_scale_factor())); On 2012/10/16 23:54:17, oshima wrote: > Scale(- iter->device_scale_factor()); (or -1 *) Done.
https://codereview.chromium.org/11140006/diff/6001/ash/display/multi_display_... File ash/display/multi_display_manager.cc (right): https://codereview.chromium.org/11140006/diff/6001/ash/display/multi_display_... ash/display/multi_display_manager.cc:155: for (DisplayList::iterator iter = displays_.begin(); On 2012/10/17 00:07:49, Jun Mukai wrote: > On 2012/10/16 23:54:17, oshima wrote: > > FindDisplayForId > > Thanks. But as far as I see, it returns a const reference, so the code to > reimburse the existing insets could be awkward -- I want to edit an existing > display in the list. Also it raises DLOG(FATAL) in case no displays are found, > but it wouldn't be exceptional here. > > By the way, I found GetDisplayForId() too, and both are almost identical. Why > do we have both? FindDisplayId returns non const reference. GetDisplayId is const versino of it. I think it's just mistake that const/non const versions have different name and one of them should be renamed. (it'd be outside of this scope)
https://codereview.chromium.org/11140006/diff/6001/ash/display/multi_display_... File ash/display/multi_display_manager.cc (right): https://codereview.chromium.org/11140006/diff/6001/ash/display/multi_display_... ash/display/multi_display_manager.cc:155: for (DisplayList::iterator iter = displays_.begin(); On 2012/10/17 01:09:40, oshima wrote: > On 2012/10/17 00:07:49, Jun Mukai wrote: > > On 2012/10/16 23:54:17, oshima wrote: > > > FindDisplayForId > > > > Thanks. But as far as I see, it returns a const reference, so the code to > > reimburse the existing insets could be awkward -- I want to edit an existing > > display in the list. Also it raises DLOG(FATAL) in case no displays are > found, > > but it wouldn't be exceptional here. > > > > By the way, I found GetDisplayForId() too, and both are almost identical. Why > > do we have both? > > FindDisplayId returns non const reference. GetDisplayId is const versino of it. > I think it's just mistake that const/non const versions have different name and > one of them should be renamed. (it'd be outside of this scope) Done.
l gtm if you make these change, and you can send it to sky. http://codereview.chromium.org/11140006/diff/4002/ash/display/multi_display_m... File ash/display/multi_display_manager.cc (right): http://codereview.chromium.org/11140006/diff/4002/ash/display/multi_display_m... ash/display/multi_display_manager.cc:135: return GetInvalidDisplay(); can you changet hsi to use FindDisplayForId() ? it's ok to use const_cast here to call private method. http://codereview.chromium.org/11140006/diff/4002/ash/display/multi_display_m... ash/display/multi_display_manager.cc:156: if (display.id() != gfx::Display::kInvalidDisplayID) { Can you add Display::is_valid() and use here? http://codereview.chromium.org/11140006/diff/4002/ash/display/multi_display_m... ash/display/multi_display_manager.cc:452: return FindDisplayForId(id); gfx::Display& display = FindDisplayFor(id); DCHECK(display.is_valid()); return display;
https://codereview.chromium.org/11140006/diff/4002/ash/display/multi_display_... File ash/display/multi_display_manager.cc (right): https://codereview.chromium.org/11140006/diff/4002/ash/display/multi_display_... ash/display/multi_display_manager.cc:135: return GetInvalidDisplay(); On 2012/10/17 16:34:09, oshima wrote: > can you changet hsi to use FindDisplayForId() ? > it's ok to use const_cast here to call private method. Done but rather changed FindDisplayForId to use this... you know, const method cannot call non-const, but non-const can (with const_cast). https://codereview.chromium.org/11140006/diff/4002/ash/display/multi_display_... ash/display/multi_display_manager.cc:156: if (display.id() != gfx::Display::kInvalidDisplayID) { On 2012/10/17 16:34:09, oshima wrote: > Can you add Display::is_valid() and use here? Done. https://codereview.chromium.org/11140006/diff/4002/ash/display/multi_display_... ash/display/multi_display_manager.cc:452: return FindDisplayForId(id); On 2012/10/17 16:34:09, oshima wrote: > gfx::Display& display = FindDisplayFor(id); > DCHECK(display.is_valid()); > return display; Done.
sky, can you review this?
https://codereview.chromium.org/11140006/diff/4002/ash/display/multi_display_... File ash/display/multi_display_manager.cc (right): https://codereview.chromium.org/11140006/diff/4002/ash/display/multi_display_... ash/display/multi_display_manager.cc:135: return GetInvalidDisplay(); On 2012/10/17 16:53:40, Jun Mukai wrote: > On 2012/10/17 16:34:09, oshima wrote: > > can you changet hsi to use FindDisplayForId() ? > > it's ok to use const_cast here to call private method. > > Done but rather changed FindDisplayForId to use this... you know, const method > cannot call non-const, but non-const can (with const_cast). talked offline and changed to edit GetDisplayForId part.
derat, can you review this? I'd like to reduce sky's review load...
lgtm http://codereview.chromium.org/11140006/diff/7006/ash/display/multi_display_m... File ash/display/multi_display_manager.cc (right): http://codereview.chromium.org/11140006/diff/7006/ash/display/multi_display_m... ash/display/multi_display_manager.cc:151: // Reimburse the existing insets before applying the new insets. "Undo" might be clearer than "Reimburse" here
https://codereview.chromium.org/11140006/diff/7006/ash/display/multi_display_... File ash/display/multi_display_manager.cc (right): https://codereview.chromium.org/11140006/diff/7006/ash/display/multi_display_... ash/display/multi_display_manager.cc:151: // Reimburse the existing insets before applying the new insets. On 2012/10/17 18:46:21, Daniel Erat wrote: > "Undo" might be clearer than "Reimburse" here Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mukai@chromium.org/11140006/17002
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mukai@chromium.org/11140006/17002
Change committed as 162570 |
