Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2575503003:
Getting rid of frame_size_in_dip in Layer::SetShowSurface (Closed)
|
Created:
4 years ago by Saman Sami Modified:
4 years ago CC:
cc-bugs_chromium.org, chromium-reviews, danakj+watch_chromium.org, darin-cc_chromium.org, jam, jbauman+watch_chromium.org, kalyank, miu+watch_chromium.org, piman+watch_chromium.org, Ian Vollick, xjz+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionGetting rid of frame_size_in_dip in Layer::SetShowSurface
frame_size_in_dip is a function of surface_size and scale, so in order
to simplify the API, this CL removes it from the parameter list of
Layer::SetShowSurface
Committed: https://crrev.com/47fb891dcc80611ff6d28db61440d17699fd5e48
Cr-Commit-Position: refs/heads/master@{#438651}
Patch Set 1 #Patch Set 2 : up #
Total comments: 7
Patch Set 3 : up #
Total comments: 2
Patch Set 4 : test #
Total comments: 1
Patch Set 5 : x #
Messages
Total messages: 38 (24 generated)
The CQ bit was checked by samans@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 ========== Getting rid of frame_size_in_dip in Layer::SetShowSurface frame_size_in_dip is a function of surface_size and scale, so in order to simplify the API, this CL removes it from the parameter list. ========== to ========== Getting rid of frame_size_in_dip in Layer::SetShowSurface frame_size_in_dip is a function of surface_size and scale, so in order to simplify the API, this CL removes it from the parameter list. ==========
samans@chromium.org changed reviewers: + fsamuel@chromium.org
Description was changed from ========== Getting rid of frame_size_in_dip in Layer::SetShowSurface frame_size_in_dip is a function of surface_size and scale, so in order to simplify the API, this CL removes it from the parameter list. ========== to ========== Getting rid of frame_size_in_dip in Layer::SetShowSurface frame_size_in_dip is a function of surface_size and scale, so in order to simplify the API, this CL removes it from the parameter list of Layer::SetShowSurface ==========
The CQ bit was checked by samans@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...
samans@chromium.org changed reviewers: + reveman@chromium.org
samans@chromium.org changed reviewers: + sadrul@chromium.org
https://codereview.chromium.org/2575503003/diff/20001/components/exo/surface.cc File components/exo/surface.cc (right): https://codereview.chromium.org/2575503003/diff/20001/components/exo/surface.... components/exo/surface.cc:426: content_size_, contents_surface_to_layer_scale); Doesn't this mean we're changing behavior? Both DIPS and physical pixels are the same? That's very weird. https://codereview.chromium.org/2575503003/diff/20001/content/browser/rendere... File content/browser/renderer_host/delegated_frame_host.cc (left): https://codereview.chromium.org/2575503003/diff/20001/content/browser/rendere... content/browser/renderer_host/delegated_frame_host.cc:517: frame_device_scale_factor, frame_size_in_dip); Do we still need frame_size_in_dip? Can you delete that?
https://codereview.chromium.org/2575503003/diff/20001/components/exo/surface.cc File components/exo/surface.cc (right): https://codereview.chromium.org/2575503003/diff/20001/components/exo/surface.... components/exo/surface.cc:426: content_size_, contents_surface_to_layer_scale); On 2016/12/14 00:33:16, Fady Samuel wrote: > Doesn't this mean we're changing behavior? Both DIPS and physical pixels are the > same? That's very weird. The scale is always one (see line 414), so it's fine. https://codereview.chromium.org/2575503003/diff/20001/content/browser/rendere... File content/browser/renderer_host/delegated_frame_host.cc (left): https://codereview.chromium.org/2575503003/diff/20001/content/browser/rendere... content/browser/renderer_host/delegated_frame_host.cc:517: frame_device_scale_factor, frame_size_in_dip); On 2016/12/14 00:33:16, Fady Samuel wrote: > Do we still need frame_size_in_dip? Can you delete that? It's used in a few if conditions above and then it's assigned to current_frame_size_in_dip_ below
OK, LGTM!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2575503003/diff/20001/ui/compositor/layer.h File ui/compositor/layer.h (right): https://codereview.chromium.org/2575503003/diff/20001/ui/compositor/layer.h#n... ui/compositor/layer.h:299: gfx::Size surface_size, Can you rename this to be |surface_size_in_pixels| to make it obvious that this is not in DIP?
https://codereview.chromium.org/2575503003/diff/20001/ui/compositor/layer.h File ui/compositor/layer.h (right): https://codereview.chromium.org/2575503003/diff/20001/ui/compositor/layer.h#n... ui/compositor/layer.h:299: gfx::Size surface_size, On 2016/12/14 01:06:25, sadrul wrote: > Can you rename this to be |surface_size_in_pixels| to make it obvious that this > is not in DIP? While you're at it, we tend to prefer pass by const ref for non-primitive types. const gfx::Size& surface_size_in_pixels
The CQ bit was checked by samans@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...
PTAL https://codereview.chromium.org/2575503003/diff/20001/ui/compositor/layer.h File ui/compositor/layer.h (right): https://codereview.chromium.org/2575503003/diff/20001/ui/compositor/layer.h#n... ui/compositor/layer.h:299: gfx::Size surface_size, On 2016/12/14 01:11:44, Fady Samuel wrote: > On 2016/12/14 01:06:25, sadrul wrote: > > Can you rename this to be |surface_size_in_pixels| to make it obvious that > this > > is not in DIP? > > While you're at it, we tend to prefer pass by const ref for non-primitive types. > > const gfx::Size& surface_size_in_pixels Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2575503003/diff/40001/ui/compositor/layer_uni... File ui/compositor/layer_unittest.cc (right): https://codereview.chromium.org/2575503003/diff/40001/ui/compositor/layer_uni... ui/compositor/layer_unittest.cc:1876: gfx::Size(20, 20), 2.0f); The numbers here don't match: frame-size in the old code is 20x20, whereas in DIP after this change, it would become 10x10 (since DSF=2). Can you add an expectation below to make sure the frame-size is correct (after line ~1885, for example)?
lgtm
The CQ bit was checked by samans@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...
https://codereview.chromium.org/2575503003/diff/40001/ui/compositor/layer_uni... File ui/compositor/layer_unittest.cc (right): https://codereview.chromium.org/2575503003/diff/40001/ui/compositor/layer_uni... ui/compositor/layer_unittest.cc:1876: gfx::Size(20, 20), 2.0f); On 2016/12/14 05:13:28, sadrul wrote: > The numbers here don't match: frame-size in the old code is 20x20, whereas in > DIP after this change, it would become 10x10 (since DSF=2). Can you add an > expectation below to make sure the frame-size is correct (after line ~1885, for > example)? It doesn't matter in this test, because it's simply trying to see if the values are passed correctly to the mirrors, regardless of whether or not they're consistent. I'm gonna add a new test for checking frame size in dip.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
lgtm https://codereview.chromium.org/2575503003/diff/60001/ui/compositor/layer.h File ui/compositor/layer.h (right): https://codereview.chromium.org/2575503003/diff/60001/ui/compositor/layer.h#n... ui/compositor/layer.h:390: const gfx::Size& frame_size_in_dip() const { return frame_size_in_dip_; } Like the one above: add a _for_testing() at the end?
The CQ bit was checked by samans@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from reveman@chromium.org, fsamuel@chromium.org, sadrul@chromium.org Link to the patchset: https://codereview.chromium.org/2575503003/#ps80001 (title: "x")
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": 80001, "attempt_start_ts": 1481747287325980,
"parent_rev": "e68aada22427cd98f8596c098e74c26a1848ed76", "commit_rev":
"7a66a6b9541a95bd0cc9bb25eda8675a7bd98ed8"}
Message was sent while issue was closed.
Description was changed from ========== Getting rid of frame_size_in_dip in Layer::SetShowSurface frame_size_in_dip is a function of surface_size and scale, so in order to simplify the API, this CL removes it from the parameter list of Layer::SetShowSurface ========== to ========== Getting rid of frame_size_in_dip in Layer::SetShowSurface frame_size_in_dip is a function of surface_size and scale, so in order to simplify the API, this CL removes it from the parameter list of Layer::SetShowSurface Review-Url: https://codereview.chromium.org/2575503003 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Getting rid of frame_size_in_dip in Layer::SetShowSurface frame_size_in_dip is a function of surface_size and scale, so in order to simplify the API, this CL removes it from the parameter list of Layer::SetShowSurface Review-Url: https://codereview.chromium.org/2575503003 ========== to ========== Getting rid of frame_size_in_dip in Layer::SetShowSurface frame_size_in_dip is a function of surface_size and scale, so in order to simplify the API, this CL removes it from the parameter list of Layer::SetShowSurface Committed: https://crrev.com/47fb891dcc80611ff6d28db61440d17699fd5e48 Cr-Commit-Position: refs/heads/master@{#438651} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/47fb891dcc80611ff6d28db61440d17699fd5e48 Cr-Commit-Position: refs/heads/master@{#438651} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
