Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2664403004:
Use pixel coordinates for shadow underlay bounds (Closed)
DescriptionUse pixel coordinates for shadow underlay bounds
* Introduced new request set_rectangular_surface_shadow.
Reprecated old one.
* Added placeholder for set/unset moving introduced in remote_surface version 2.
This only switches the underlay to pixel and that looks good enough to me.
I'll work on the shadow itself separately if necessary.
BUG=687742
TEST=tested on Elm.
Review-Url: https://codereview.chromium.org/2664403004
Cr-Commit-Position: refs/heads/master@{#448549}
Committed: https://chromium.googlesource.com/chromium/src/+/ba48072770ed46f3328f09d8147f42c8cf4a1677
Patch Set 1 : . #
Total comments: 1
Patch Set 2 : add new request. mark old one as deprecated #Patch Set 3 : flip the default #Patch Set 4 : update #
Total comments: 8
Patch Set 5 : update names etc #
Total comments: 6
Patch Set 6 : rename,rebase,etc #Patch Set 7 : set version 2 #
Total comments: 12
Patch Set 8 : address comments #
Total comments: 2
Patch Set 9 : addressed comment #Messages
Total messages: 48 (29 generated)
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
Patchset #1 (id:40001) has been deleted
Description was changed from ========== Pixel aligned shadow underlay BUG= ========== to ========== Pixel aligned shadow underlay BUG=687742 TEST=tested on Elm. ==========
oshima@chromium.org changed reviewers: + reveman@chromium.org
Description was changed from ========== Pixel aligned shadow underlay BUG=687742 TEST=tested on Elm. ========== to ========== Use pixel coordinates for shadow underlay bounds 1) If client (ARC++) sends negative scale, the rectangular shadow bounds becomes pixel coordinates. 2) In this mode, the shadow underlay is added to the shell surface at the bottom 3) Old M impl sends positive scale, so it will use the DIP. I'll remove the old path once NYC transition is complete. BUG=687742 TEST=tested on Elm. ==========
oshima@chromium.org changed reviewers: + lpique@chromium.org
Patchset #2 (id:80001) has been deleted
Patchset #1 (id:60001) has been deleted
The CQ bit was checked by oshima@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/2664403004/diff/100001/components/exo/shell_s... File components/exo/shell_surface.cc (right): https://codereview.chromium.org/2664403004/diff/100001/components/exo/shell_s... components/exo/shell_surface.cc:630: pending_scale_ = -scale; I'm using negative scale during the transition. If you have a better idea how to distinguish between M vs N, please let me know.
Description was changed from ========== Use pixel coordinates for shadow underlay bounds 1) If client (ARC++) sends negative scale, the rectangular shadow bounds becomes pixel coordinates. 2) In this mode, the shadow underlay is added to the shell surface at the bottom 3) Old M impl sends positive scale, so it will use the DIP. I'll remove the old path once NYC transition is complete. BUG=687742 TEST=tested on Elm. ========== to ========== Use pixel coordinates for shadow underlay bounds 1) If client (ARC++) sends negative scale, the rectangular shadow bounds becomes pixel coordinates. 2) In this mode, the shadow underlay is added to the shell surface at the bottom 3) Old M impl sends positive scale, so it will use the DIP. I'll remove the old path once NYC transition is complete. This only switches the underlay to pixel and that looks good enough for now. I'll work on the shadow itself separately if necessary. BUG=687742 TEST=tested on Elm. ==========
Description was changed from ========== Use pixel coordinates for shadow underlay bounds 1) If client (ARC++) sends negative scale, the rectangular shadow bounds becomes pixel coordinates. 2) In this mode, the shadow underlay is added to the shell surface at the bottom 3) Old M impl sends positive scale, so it will use the DIP. I'll remove the old path once NYC transition is complete. This only switches the underlay to pixel and that looks good enough for now. I'll work on the shadow itself separately if necessary. BUG=687742 TEST=tested on Elm. ========== to ========== Use pixel coordinates for shadow underlay bounds 1) If client (ARC++) sends negative scale, the rectangular shadow bounds becomes pixel coordinates. 2) In this mode, the shadow underlay is added to the shell surface at the bottom 3) Old M impl sends positive scale, so it will use the DIP. I'll remove the old path once NYC transition is complete. This only switches the underlay to pixel and that looks good enough to me. I'll work on the shadow itself separately if necessary. BUG=687742 TEST=tested on Elm. ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Can we add another request to the wayland interface for this instead? Easier to review that way I think.
On 2017/02/02 at 02:52:16, reveman wrote: > Can we add another request to the wayland interface for this instead? Easier to review that way I think. I mean new request instead of the negative coordinate hack.
On 2017/02/02 02:53:09, reveman wrote: > On 2017/02/02 at 02:52:16, reveman wrote: > > Can we add another request to the wayland interface for this instead? Easier > to review that way I think. > > I mean new request instead of the negative coordinate hack. I'm happy to add new one but just want to make sure we're on the same page. This is temporary and negative scale (not coordinate) will be removed once we moved to NYC. Do you still want to add new one?
Patchset #3 (id:140001) has been deleted
Patchset #2 (id:120001) has been deleted
Patchset #2 (id:160001) has been deleted
Patchset #2 (id:180001) has been deleted
Patchset #2 (id:200001) has been deleted
added new request in the interface. ptal.
Description was changed from ========== Use pixel coordinates for shadow underlay bounds 1) If client (ARC++) sends negative scale, the rectangular shadow bounds becomes pixel coordinates. 2) In this mode, the shadow underlay is added to the shell surface at the bottom 3) Old M impl sends positive scale, so it will use the DIP. I'll remove the old path once NYC transition is complete. This only switches the underlay to pixel and that looks good enough to me. I'll work on the shadow itself separately if necessary. BUG=687742 TEST=tested on Elm. ========== to ========== Use pixel coordinates for shadow underlay bounds * Introduced new request set_rectangular_surface_shadow. Reprecated old one. * Added placeholder for requests introduced for remote_surface version 2. This only switches the underlay to pixel and that looks good enough to me. I'll work on the shadow itself separately if necessary. BUG=687742 TEST=tested on Elm. ==========
Description was changed from ========== Use pixel coordinates for shadow underlay bounds * Introduced new request set_rectangular_surface_shadow. Reprecated old one. * Added placeholder for requests introduced for remote_surface version 2. This only switches the underlay to pixel and that looks good enough to me. I'll work on the shadow itself separately if necessary. BUG=687742 TEST=tested on Elm. ========== to ========== Use pixel coordinates for shadow underlay bounds * Introduced new request set_rectangular_surface_shadow. Reprecated old one. * Added placeholder for set/unset moving introduced in remote_surface version 2. This only switches the underlay to pixel and that looks good enough to me. I'll work on the shadow itself separately if necessary. BUG=687742 TEST=tested on Elm. ==========
Patchset #4 (id:260001) has been deleted
https://codereview.chromium.org/2664403004/diff/280001/components/exo/wayland... File components/exo/wayland/server.cc (right): https://codereview.chromium.org/2664403004/diff/280001/components/exo/wayland... components/exo/wayland/server.cc:1512: shell_surface->set_shadow_underlay_in_shell_surface(false); We usually try to have the exo interface (ShellSurface in this case) match the wayland interface as closely as possible unless there's a very good reason not to do so. Makes the code easier to understand as names will be consistent and as much logic as possible will live in exo/ rather than exo/wayland which makes it easier to unit test. So can you add ShellSurface::SetRectangularSurfaceShadow instead of this setter? It's fine to keep a flag inside the ShellSurface until that can be removed. Fyi, we should remove ShellSurface::SetRectangularShadow too for this same kind of consistency. Feel free to do that as part of this change if you like. https://codereview.chromium.org/2664403004/diff/280001/components/exo/wayland... components/exo/wayland/server.cc:1595: void remote_surface_empty_noarg(wl_client* client, Can you add stub functions with the correct names but NOTIMPLEMENTED() statements in them instead? https://codereview.chromium.org/2664403004/diff/280001/components/exo/wayland... components/exo/wayland/server.cc:1604: remote_surface_set_rectangular_shadow, Please change the name of remote_surface_set_rectangular_shadow function to remote_surface_set_rectangular_shadow_DEPRECATED. That's a pattern we've used in the past to make it very clear that some functions are deprecated. https://codereview.chromium.org/2664403004/diff/280001/third_party/wayland-pr... File third_party/wayland-protocols/unstable/remote-shell/remote-shell-unstable-v1.xml (right): https://codereview.chromium.org/2664403004/diff/280001/third_party/wayland-pr... third_party/wayland-protocols/unstable/remote-shell/remote-shell-unstable-v1.xml:413: The arguments are given in the remote surface coordinate space. nit: you already mention this above
Patchset #5 (id:300001) has been deleted
https://codereview.chromium.org/2664403004/diff/280001/components/exo/wayland... File components/exo/wayland/server.cc (right): https://codereview.chromium.org/2664403004/diff/280001/components/exo/wayland... components/exo/wayland/server.cc:1512: shell_surface->set_shadow_underlay_in_shell_surface(false); On 2017/02/03 02:22:18, reveman wrote: > We usually try to have the exo interface (ShellSurface in this case) match the > wayland interface as closely as possible unless there's a very good reason not > to do so. Makes the code easier to understand as names will be consistent and as > much logic as possible will live in exo/ rather than exo/wayland which makes it > easier to unit test. > > So can you add ShellSurface::SetRectangularSurfaceShadow instead of this setter? > It's fine to keep a flag inside the ShellSurface until that can be removed. > > Fyi, we should remove ShellSurface::SetRectangularShadow too for this same kind > of consistency. Feel free to do that as part of this change if you like. Done. https://codereview.chromium.org/2664403004/diff/280001/components/exo/wayland... components/exo/wayland/server.cc:1595: void remote_surface_empty_noarg(wl_client* client, On 2017/02/03 02:22:18, reveman wrote: > Can you add stub functions with the correct names but NOTIMPLEMENTED() > statements in them instead? Done. https://codereview.chromium.org/2664403004/diff/280001/components/exo/wayland... components/exo/wayland/server.cc:1604: remote_surface_set_rectangular_shadow, On 2017/02/03 02:22:18, reveman wrote: > Please change the name of remote_surface_set_rectangular_shadow function to > remote_surface_set_rectangular_shadow_DEPRECATED. That's a pattern we've used in > the past to make it very clear that some functions are deprecated. Done. https://codereview.chromium.org/2664403004/diff/280001/third_party/wayland-pr... File third_party/wayland-protocols/unstable/remote-shell/remote-shell-unstable-v1.xml (right): https://codereview.chromium.org/2664403004/diff/280001/third_party/wayland-pr... third_party/wayland-protocols/unstable/remote-shell/remote-shell-unstable-v1.xml:413: The arguments are given in the remote surface coordinate space. On 2017/02/03 02:22:18, reveman wrote: > nit: you already mention this above Done.
reveman@chromium.org changed reviewers: + domlaskowski@chromium.org
https://codereview.chromium.org/2664403004/diff/320001/components/exo/shell_s... File components/exo/shell_surface.h (right): https://codereview.chromium.org/2664403004/diff/320001/components/exo/shell_s... components/exo/shell_surface.h:153: void SetRectangularShadow_DEPRECATED(const gfx::Rect& content_bounds); Remove _DEPRECATED here. See my comment in server.cc. https://codereview.chromium.org/2664403004/diff/320001/components/exo/shell_s... components/exo/shell_surface.h:247: void set_shadow_enabled(bool enabled) { shadow_enabled_ = enabled; } Remove this. See my comment in server.cc. https://codereview.chromium.org/2664403004/diff/320001/components/exo/shell_s... components/exo/shell_surface.h:329: // TODO(oshima): Remove this once NYC migration is complete. I think we still need this for non arc clients. Please move this to the deprecated function in server.cc. https://codereview.chromium.org/2664403004/diff/320001/components/exo/wayland... File components/exo/wayland/server.cc (right): https://codereview.chromium.org/2664403004/diff/320001/components/exo/wayland... components/exo/wayland/server.cc:882: shell_surface->set_shadow_enabled(true); Ah, sorry! I forgot that we use this here. Let's keep ShellSurface::SetRectangularShadow as before and remove set_shadow_enabled. The remote shell wayland requests can still be considered deprecated so remote_surface_set_rectangular_shadow_DEPRECATED below is good. https://codereview.chromium.org/2664403004/diff/320001/third_party/wayland-pr... File third_party/wayland-protocols/unstable/remote-shell/remote-shell-unstable-v1.xml (right): https://codereview.chromium.org/2664403004/diff/320001/third_party/wayland-pr... third_party/wayland-protocols/unstable/remote-shell/remote-shell-unstable-v1.xml:385: <request name="set_moving" since="2"> Actually, we never used version 2 and we decided that these functions should be removed so we can just amend version 2 instead. ie. remove set_moving/unset_moving and make set_rectangular_surface_shadow a version 2 request. +domlaskowski
https://codereview.chromium.org/2664403004/diff/320001/third_party/wayland-pr... File third_party/wayland-protocols/unstable/remote-shell/remote-shell-unstable-v1.xml (right): https://codereview.chromium.org/2664403004/diff/320001/third_party/wayland-pr... third_party/wayland-protocols/unstable/remote-shell/remote-shell-unstable-v1.xml:385: <request name="set_moving" since="2"> On 2017/02/03 17:47:01, reveman wrote: > Actually, we never used version 2 and we decided that these functions should be > removed so we can just amend version 2 instead. ie. remove > set_moving/unset_moving and make set_rectangular_surface_shadow a version 2 > request. > > +domlaskowski set_moving/unset_moving is still needed for the moment. The multi-display CL is complex enough with the extra configure/ack_configure logic, so I'd like to postpone the migration to server-side movement for another CL. Instead of adding stubs and bumping to version 3, could we swap the versions and bump to version 2?
On 2017/02/03 18:55:14, Dominik Laskowski wrote: > https://codereview.chromium.org/2664403004/diff/320001/third_party/wayland-pr... > File > third_party/wayland-protocols/unstable/remote-shell/remote-shell-unstable-v1.xml > (right): > > https://codereview.chromium.org/2664403004/diff/320001/third_party/wayland-pr... > third_party/wayland-protocols/unstable/remote-shell/remote-shell-unstable-v1.xml:385: > <request name="set_moving" since="2"> > On 2017/02/03 17:47:01, reveman wrote: > > Actually, we never used version 2 and we decided that these functions should > be > > removed so we can just amend version 2 instead. ie. remove > > set_moving/unset_moving and make set_rectangular_surface_shadow a version 2 > > request. > > > > +domlaskowski > > set_moving/unset_moving is still needed for the moment. The multi-display CL is > complex enough with the extra configure/ack_configure logic, so I'd like to > postpone the migration to server-side movement for another CL. > > Instead of adding stubs and bumping to version 3, could we swap the versions and > bump to version 2? Will do
Patchset #6 (id:340001) has been deleted
* Renamed methods * Rebased * Changed version to 2 PTAL
https://codereview.chromium.org/2664403004/diff/380001/components/exo/shell_s... File components/exo/shell_surface.cc (right): https://codereview.chromium.org/2664403004/diff/380001/components/exo/shell_s... components/exo/shell_surface.cc:1435: delete shadow_underlay_; I think this would be best to do as a separate patch if you don't mind. https://codereview.chromium.org/2664403004/diff/380001/components/exo/shell_s... components/exo/shell_surface.cc:1482: if (shadow_underlay_in_surface_) { why do we need this? https://codereview.chromium.org/2664403004/diff/380001/components/exo/shell_s... components/exo/shell_surface.cc:1546: if (!shadow_underlay_in_surface_) { nit: remove "!" and flip the contents of each clause https://codereview.chromium.org/2664403004/diff/380001/components/exo/shell_s... File components/exo/shell_surface.h (right): https://codereview.chromium.org/2664403004/diff/380001/components/exo/shell_s... components/exo/shell_surface.h:152: void SetEnableRectangularShadow(bool enabled); nit: SetRectangularShadowEnabled https://codereview.chromium.org/2664403004/diff/380001/components/exo/shell_s... components/exo/shell_surface.h:249: Surface* surface() { return surface_; } nit: surface_for_testing() and blank line between shadow_underlay() and this.
On 2017/02/04 01:00:36, oshima wrote: > * Renamed methods > * Rebased > * Changed version to 2 > > PTAL friendly ping reveman@ -> all domlaskowski@ -> remote-shell-unstable-v1.xml lpique@ -> fyi
https://codereview.chromium.org/2664403004/diff/380001/components/exo/shell_s... File components/exo/shell_surface.cc (right): https://codereview.chromium.org/2664403004/diff/380001/components/exo/shell_s... components/exo/shell_surface.cc:1435: delete shadow_underlay_; On 2017/02/06 22:38:03, reveman wrote: > I think this would be best to do as a separate patch if you don't mind. This was to cover the test. Done. https://codereview.chromium.org/2664403004/diff/380001/components/exo/shell_s... components/exo/shell_surface.cc:1482: if (shadow_underlay_in_surface_) { On 2017/02/06 22:38:03, reveman wrote: > why do we need this? This is to make sure the underlay is at the bottom of the surface layers. I made more strict. I did this because the tree structure under surface_ can change, but I can remove if it's guaranteed that it will stay at the bottom once it's moved. https://codereview.chromium.org/2664403004/diff/380001/components/exo/shell_s... components/exo/shell_surface.cc:1546: if (!shadow_underlay_in_surface_) { On 2017/02/06 22:38:03, reveman wrote: > nit: remove "!" and flip the contents of each clause Done. https://codereview.chromium.org/2664403004/diff/380001/components/exo/shell_s... File components/exo/shell_surface.h (right): https://codereview.chromium.org/2664403004/diff/380001/components/exo/shell_s... components/exo/shell_surface.h:152: void SetEnableRectangularShadow(bool enabled); On 2017/02/06 22:38:03, reveman wrote: > nit: SetRectangularShadowEnabled Done. https://codereview.chromium.org/2664403004/diff/380001/components/exo/shell_s... components/exo/shell_surface.h:249: Surface* surface() { return surface_; } On 2017/02/06 22:38:03, reveman wrote: > nit: surface_for_testing() and blank line between shadow_underlay() and this. Done.
https://codereview.chromium.org/2664403004/diff/380001/components/exo/shell_s... File components/exo/shell_surface.cc (right): https://codereview.chromium.org/2664403004/diff/380001/components/exo/shell_s... components/exo/shell_surface.cc:1482: if (shadow_underlay_in_surface_) { On 2017/02/07 at 02:21:45, oshima wrote: > On 2017/02/06 22:38:03, reveman wrote: > > why do we need this? > > This is to make sure the underlay is at the bottom of the surface layers. I made more strict. > > I did this because the tree structure under surface_ can change, but > I can remove if it's guaranteed that it will stay at the bottom > once it's moved. I don't think we're doing anything that requires us to restack the underlay and if we do then I'm not sure UpdateShadow is called to fix it anyhow so best to remove unless we know a case where it's needed. https://codereview.chromium.org/2664403004/diff/400001/components/exo/shell_s... File components/exo/shell_surface.cc (right): https://codereview.chromium.org/2664403004/diff/400001/components/exo/shell_s... components/exo/shell_surface.cc:1479: if (shadow_underlay_in_surface_ && shadow_underlay_ != nullptr && nit: s/shadow_underlay_ != nullptr/shadow_underlay_/ https://codereview.chromium.org/2664403004/diff/400001/components/exo/shell_s... components/exo/shell_surface.cc:1480: *(surface_->window()->children().begin()) != shadow_underlay_) { is this really needed? what results in this not being the case?
https://codereview.chromium.org/2664403004/diff/380001/components/exo/shell_s... File components/exo/shell_surface.cc (right): https://codereview.chromium.org/2664403004/diff/380001/components/exo/shell_s... components/exo/shell_surface.cc:1482: if (shadow_underlay_in_surface_) { On 2017/02/07 03:16:53, reveman wrote: > On 2017/02/07 at 02:21:45, oshima wrote: > > On 2017/02/06 22:38:03, reveman wrote: > > > why do we need this? > > > > This is to make sure the underlay is at the bottom of the surface layers. I > made more strict. > > > > I did this because the tree structure under surface_ can change, but > > I can remove if it's guaranteed that it will stay at the bottom > > once it's moved. > > I don't think we're doing anything that requires us to restack the underlay and > if we do then I'm not sure UpdateShadow is called to fix it anyhow so best to > remove unless we know a case where it's needed. Ok, move the stacking code into to above if block.
lgtm
The CQ bit was checked by oshima@chromium.org
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": 420001, "attempt_start_ts": 1486440638979610,
"parent_rev": "6914300d5e9019f1d4256883615e50d698df7bbd", "commit_rev":
"ba48072770ed46f3328f09d8147f42c8cf4a1677"}
Message was sent while issue was closed.
Description was changed from ========== Use pixel coordinates for shadow underlay bounds * Introduced new request set_rectangular_surface_shadow. Reprecated old one. * Added placeholder for set/unset moving introduced in remote_surface version 2. This only switches the underlay to pixel and that looks good enough to me. I'll work on the shadow itself separately if necessary. BUG=687742 TEST=tested on Elm. ========== to ========== Use pixel coordinates for shadow underlay bounds * Introduced new request set_rectangular_surface_shadow. Reprecated old one. * Added placeholder for set/unset moving introduced in remote_surface version 2. This only switches the underlay to pixel and that looks good enough to me. I'll work on the shadow itself separately if necessary. BUG=687742 TEST=tested on Elm. Review-Url: https://codereview.chromium.org/2664403004 Cr-Commit-Position: refs/heads/master@{#448549} Committed: https://chromium.googlesource.com/chromium/src/+/ba48072770ed46f3328f09d8147f... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:420001) as https://chromium.googlesource.com/chromium/src/+/ba48072770ed46f3328f09d8147f... |
