Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(181)

Issue 2669183002: Seems incorrect to se device_scale_factor in SetLocalSurfaceId.

Created:
3 years, 10 months ago by k.devara
Modified:
3 years, 10 months ago
Reviewers:
Fady Samuel
CC:
chromium-reviews, cc-bugs_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Seems incorrect to se device_scale_factor in SetLocalSurfaceId. Added new function SetDeviceScaleFactor to set device_scale_factor. BUG= R=fsamuel@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Patch Set 1 #

Total comments: 3

Patch Set 2 : Seems incorrect to set device_scale_factor in SetLocalSurfaceId. Added new function SetDeviceScaleF… #

Patch Set 3 : Rebase #

Patch Set 4 : Rebase #

Patch Set 5 : Seems incorrect to set device_scale_factor in SetLocalSurfaceId. Added new function SetDeviceScaleF… #

Unified diffs Side-by-side diffs Delta from patch set Stats (+34 lines, -22 lines) Patch
M cc/ipc/display_compositor.mojom View 1 2 3 4 1 chunk +2 lines, -2 lines 0 comments Download
M cc/surfaces/direct_compositor_frame_sink.cc View 1 2 3 4 1 chunk +2 lines, -2 lines 0 comments Download
M cc/surfaces/display.h View 2 3 4 1 chunk +2 lines, -1 line 0 comments Download
M cc/surfaces/display.cc View 1 2 3 4 1 chunk +10 lines, -6 lines 0 comments Download
M cc/surfaces/display_unittest.cc View 1 2 3 4 2 chunks +4 lines, -2 lines 0 comments Download
M cc/test/test_compositor_frame_sink.cc View 1 2 3 4 1 chunk +2 lines, -2 lines 0 comments Download
M components/display_compositor/gpu_root_compositor_frame_sink.h View 1 2 3 4 1 chunk +2 lines, -2 lines 0 comments Download
M components/display_compositor/gpu_root_compositor_frame_sink.cc View 1 2 3 4 1 chunk +6 lines, -2 lines 0 comments Download
M content/renderer/android/synchronous_compositor_frame_sink.cc View 1 2 3 4 1 chunk +2 lines, -2 lines 0 comments Download
M services/ui/ws/frame_generator.cc View 1 2 3 4 1 chunk +2 lines, -1 line 0 comments Download

Messages

Total messages: 31 (25 generated)
Fady Samuel
https://codereview.chromium.org/2669183002/diff/1/cc/surfaces/display.cc File cc/surfaces/display.cc (right): https://codereview.chromium.org/2669183002/diff/1/cc/surfaces/display.cc#newcode108 cc/surfaces/display.cc:108: if (current_surface_id_.local_surface_id() == id) { drop the braces { ...
3 years, 10 months ago (2017-02-02 16:34:29 UTC) #2
weiliangc
When we get a new device_scale_factor, do we need a new local surface ID? Changing ...
3 years, 10 months ago (2017-02-02 18:26:16 UTC) #3
Fady Samuel
That's a good point, Wei. That got me thinking: should SetLocalSurfaceId be instead SetSurfaceInfo which ...
3 years, 10 months ago (2017-02-02 18:27:44 UTC) #4
weiliangc
On 2017/02/02 18:27:44, Fady Samuel wrote: > That's a good point, Wei. That got me ...
3 years, 10 months ago (2017-02-02 18:34:10 UTC) #5
k.devara
https://codereview.chromium.org/2669183002/diff/1/cc/surfaces/display.cc File cc/surfaces/display.cc (right): https://codereview.chromium.org/2669183002/diff/1/cc/surfaces/display.cc#newcode126 cc/surfaces/display.cc:126: device_scale_factor_ = device_scale_factor; On 2017/02/02 16:34:29, Fady Samuel wrote: ...
3 years, 10 months ago (2017-02-03 11:35:38 UTC) #6
k.devara
3 years, 10 months ago (2017-02-03 11:36:45 UTC) #7
On 2017/02/02 18:34:10, weiliangc wrote:
> On 2017/02/02 18:27:44, Fady Samuel wrote:
> > That's a good point, Wei. That got me thinking: should SetLocalSurfaceId be
> > instead SetSurfaceInfo which includes a device_scale_factor, frame_size,
> > and surface ID?
> > 
> > Fady
> > 
> > On Thu, Feb 2, 2017 at 10:26 AM mailto:weiliangc@chromium.org via
> > http://codereview.chromium.org
> <mailto:reply@chromiumcodereview-hr.appspotmail.com> wrote:
> > 
> > > When we get a new device_scale_factor, do we need a new local surface ID?
> > > Changing the code this way would remove this guarantee.
> > >
> > > https://codereview.chromium.org/2669183002/
> > >
> > 
> > -- 
> > 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 mailto:chromium-reviews+unsubscribe@chromium.org.
> 
> Yeah that sounds good.

Also, I was wondering if SetLocalSurfaceId may be renamed SetCurrentSurfaceId, 
meaning the current_surface associated with the display.

Powered by Google App Engine
This is Rietveld 408576698