Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2944063002:
exo: Fix cursor scaling for 1.25 DSF (Closed)
|
Created:
3 years, 6 months ago by Dominik Laskowski Modified:
3 years, 5 months ago CC:
chromium-reviews Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Descriptionexo: Fix cursor scaling for 1.25 DSF
The cursor was scaled on displays whose DSF did not match the capture
scale. However, no scaling is required between displays with 1.25 and
1.0 DSF, i.e. the 1.25 DSF cursor has the same pixel size but appears
smaller.
BUG=730843
TEST=cave: Cursor is not scaled down on external 1.0 DSF display.
Review-Url: https://codereview.chromium.org/2944063002
Cr-Commit-Position: refs/heads/master@{#482647}
Committed: https://chromium.googlesource.com/chromium/src/+/c2f536007076bd932daa97b0864710eee29ebc1c
Patch Set 1 #Patch Set 2 : Rebase #
Total comments: 2
Patch Set 3 : Rebase #Patch Set 4 : Rebase #Patch Set 5 : Address style nit #Patch Set 6 : Fix exo_unittests #
Total comments: 2
Patch Set 7 : Restore DCHECK #
Messages
Total messages: 43 (20 generated)
domlaskowski@chromium.org changed reviewers: + oshima@chromium.org, reveman@chromium.org
PTAL.
components/exo lgtm
The CQ bit was checked by domlaskowski@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...
Description was changed from ========== exo: Fix cursor scaling for 1.25 DSF The cursor is scaled on displays whose DSF does not match the capture scale. However, no scaling is required between displays with 1.25 and 1.0 DSF, i.e. the 1.25 DSF cursor has the same pixel size but appears smaller. BUG=730843 TEST=cave: Cursor is not scaled down on external 1.0 DSF display. ========== to ========== exo: Fix cursor scaling for 1.25 DSF The cursor was scaled on displays whose DSF did not match the capture scale. However, no scaling is required between displays with 1.25 and 1.0 DSF, i.e. the 1.25 DSF cursor has the same pixel size but appears smaller. BUG=730843 TEST=cave: Cursor is not scaled down on external 1.0 DSF display. ==========
Rebased. For a 1.25 DSF primary display, the |capture_scale_| is 1.25 while the |capture_ratio_| is 1.0, such that the bitmap is only scaled up for 2.0 DSF secondary displays.
https://codereview.chromium.org/2944063002/diff/20001/components/exo/pointer.cc File components/exo/pointer.cc (right): https://codereview.chromium.org/2944063002/diff/20001/components/exo/pointer.... components/exo/pointer.cc:276: std::tie(capture_scale_, capture_ratio_) = std::accumulate( heh, cool; but I think you've crossed the line to where having simple for-loop is easier to read :)
https://codereview.chromium.org/2944063002/diff/20001/components/exo/pointer.cc File components/exo/pointer.cc (right): https://codereview.chromium.org/2944063002/diff/20001/components/exo/pointer.... components/exo/pointer.cc:276: std::tie(capture_scale_, capture_ratio_) = std::accumulate( On 2017/06/19 22:20:39, reveman wrote: > heh, cool; but I think you've crossed the line to where having simple for-loop > is easier to read :) Yeah, even the original was pushing it. Done.
lgtm
oshima: PTAL at ui/display/
lgtm YI: we'll consolidate them to use device scale factor (no ui_scaling, effective xxx), hopefully by 63.
lgtm after changing to GetCaptureDisplayInfo as discussed
The CQ bit was checked by domlaskowski@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by domlaskowski@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...
oshima: PTAL at Use125DSFForUIScaling change.
https://codereview.chromium.org/2944063002/diff/100001/ui/display/manager/man... File ui/display/manager/managed_display_info.cc (right): https://codereview.chromium.org/2944063002/diff/100001/ui/display/manager/man... ui/display/manager/managed_display_info.cc:513: return id_ != kInvalidDisplayId && Display::IsInternalDisplayId(id_); why this is necessary?
https://codereview.chromium.org/2944063002/diff/100001/ui/display/manager/man... File ui/display/manager/managed_display_info.cc (right): https://codereview.chromium.org/2944063002/diff/100001/ui/display/manager/man... ui/display/manager/managed_display_info.cc:513: return id_ != kInvalidDisplayId && Display::IsInternalDisplayId(id_); On 2017/06/23 23:57:14, oshima wrote: > why this is necessary? So GetDensityRatio does not DCHECK on a default-constructed ManagedDisplayInfo.
On 2017/06/23 23:59:13, Dominik Laskowski wrote: > https://codereview.chromium.org/2944063002/diff/100001/ui/display/manager/man... > File ui/display/manager/managed_display_info.cc (right): > > https://codereview.chromium.org/2944063002/diff/100001/ui/display/manager/man... > ui/display/manager/managed_display_info.cc:513: return id_ != kInvalidDisplayId > && Display::IsInternalDisplayId(id_); > On 2017/06/23 23:57:14, oshima wrote: > > why this is necessary? > > So GetDensityRatio does not DCHECK on a default-constructed ManagedDisplayInfo. My question is that why you need to call GetDensityRatio on the invalid displayinfo. Can caller check it first?
On 2017/06/24 00:23:52, oshima wrote:
> My question is that why you need to call GetDensityRatio on the invalid
> displayinfo.
> Can caller check it first?
It simplifies the caller, and I think it makes sense given that
device_scale_factor() can be called. Same applies to GetEffective{DSF,UIScale}
and configured_ui_scale().
On 2017/06/24 00:33:16, Dominik Laskowski wrote:
> On 2017/06/24 00:23:52, oshima wrote:
> > My question is that why you need to call GetDensityRatio on the invalid
> > displayinfo.
> > Can caller check it first?
>
> It simplifies the caller, and I think it makes sense given that
> device_scale_factor() can be called. Same applies to GetEffective{DSF,UIScale}
> and configured_ui_scale().
Sorry I still don't understand why calling any method on the ManageDisplayInfo
with invalid display makes sense.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/06/24 00:36:20, oshima wrote: > Sorry I still don't understand why calling any method on the ManageDisplayInfo > with invalid display makes sense. It allows the caller to handle the headless case uniformly, i.e. by falling back to a default ManagedDisplayInfo.
On 2017/06/24 01:48:50, Dominik Laskowski wrote: > On 2017/06/24 00:36:20, oshima wrote: > > Sorry I still don't understand why calling any method on the ManageDisplayInfo > > with invalid display makes sense. > > It allows the caller to handle the headless case uniformly, i.e. by falling back > to a default ManagedDisplayInfo. ChromeOS creates valid temporary display when there is no display. Can you explain exactly when this is necessary?
On 2017/06/24 02:54:19, oshima wrote: > ChromeOS creates valid temporary display when there is no display. Can you > explain exactly when this is necessary? Including a ManagedDisplayInfo? If so, I could instead replace ">" with ">=" in GetCaptureDisplayInfo. WDYT?
On 2017/06/26 17:06:00, Dominik Laskowski wrote: > On 2017/06/24 02:54:19, oshima wrote: > > ChromeOS creates valid temporary display when there is no display. Can you > > explain exactly when this is necessary? > > Including a ManagedDisplayInfo? If so, I could instead replace ">" with ">=" in > GetCaptureDisplayInfo. WDYT? per offline chat, we agreed to use >= so that it'll always matches at least one display.
The CQ bit was checked by domlaskowski@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...
oshima: PTAL
lgtm
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 domlaskowski@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from reveman@chromium.org Link to the patchset: https://codereview.chromium.org/2944063002/#ps120001 (title: "Restore DCHECK")
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": 120001, "attempt_start_ts": 1498581277770770,
"parent_rev": "3c22f577cbbfb9bb46c690695b800d668f28d5f1", "commit_rev":
"c2f536007076bd932daa97b0864710eee29ebc1c"}
Message was sent while issue was closed.
Description was changed from ========== exo: Fix cursor scaling for 1.25 DSF The cursor was scaled on displays whose DSF did not match the capture scale. However, no scaling is required between displays with 1.25 and 1.0 DSF, i.e. the 1.25 DSF cursor has the same pixel size but appears smaller. BUG=730843 TEST=cave: Cursor is not scaled down on external 1.0 DSF display. ========== to ========== exo: Fix cursor scaling for 1.25 DSF The cursor was scaled on displays whose DSF did not match the capture scale. However, no scaling is required between displays with 1.25 and 1.0 DSF, i.e. the 1.25 DSF cursor has the same pixel size but appears smaller. BUG=730843 TEST=cave: Cursor is not scaled down on external 1.0 DSF display. Review-Url: https://codereview.chromium.org/2944063002 Cr-Commit-Position: refs/heads/master@{#482647} Committed: https://chromium.googlesource.com/chromium/src/+/c2f536007076bd932daa97b08647... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/c2f536007076bd932daa97b08647... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
