Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2645663004:
exo: Initial support for multiple displays in ARC (Closed)
|
Created:
3 years, 11 months ago by Dominik Laskowski Modified:
3 years, 9 months ago CC:
chromium-reviews, Mr4D (OOO till 08-26) Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionexo: Initial support for multiple displays in ARC
This CL adds the ability to drag ARC windows to external displays,
which is achieved by mapping window positions from a single virtual
display in ARC to multiple physical displays in Chrome OS. The new
version of the Wayland protocol enables the ARC window manager to
perform window layout on multiple workspaces, which are mapped to
server-side displays. The compositor also handles events from ARC
to differentiate between dragging and other geometry updates. This
enables it to render phantom windows during dragging, and transfer
the window to the target root window when the drag ends.
BUG=642894
TEST=samus,minnie: ARC apps can be dragged to external displays using
different display settings, e.g. resolution, layout, orientation.
Review-Url: https://codereview.chromium.org/2645663004
Cr-Commit-Position: refs/heads/master@{#457249}
Committed: https://chromium.googlesource.com/chromium/src/+/623c02906151795123d5b40dfa3b931f98a916cc
Patch Set 1 #Patch Set 2 : Rebase #Patch Set 3 : Fix race and refactor #
Total comments: 24
Patch Set 4 : Address comments #Patch Set 5 : Rebase #Patch Set 6 : Fix bad merge in test #
Total comments: 4
Patch Set 7 : Split #Patch Set 8 : Rebase #Patch Set 9 : Fix bad merge #Patch Set 10 : Rebase #
Total comments: 1
Patch Set 11 : Rebase #Patch Set 12 : Split #Patch Set 13 : Remove ShellSurface tracking #Patch Set 14 : Rebase #
Total comments: 30
Patch Set 15 : Refactor #
Total comments: 8
Patch Set 16 : Address nits #Patch Set 17 : Move coordinate conversion to client #Patch Set 18 : Rebase #
Total comments: 4
Patch Set 19 : Address nits #
Depends on Patchset: Messages
Total messages: 76 (53 generated)
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...
domlaskowski@chromium.org changed reviewers: + oshima@chromium.org, reveman@chromium.org
PTAL. Dragging edge cases were split out: https://codereview.chromium.org/2396883003/
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Ping.
Description was changed from
==========
exo: Initial support for multiple displays in ARC
This CL adds the ability to drag ARC windows to external displays,
which is achieved by mapping window positions from a single virtual
display in ARC to multiple physical displays in Chrome OS. However,
multiple workspaces are not yet supported in ARC, so certain window
management operations leak this implementation detail. For example,
windows are maximized to the size of the virtual display, so their
content is partially off-screen. The compositor handles events from
ARC to differentiate between dragging and other geometry updates.
It also aborts the drag on deactivation, e.g. due to system dialogs.
BUG=642894
TEST=samus,minnie: ARC apps can be dragged to external displays using
different display settings, e.g. resolution, layout, orientation.
==========
to
==========
exo: Initial support for multiple displays in ARC
This CL adds the ability to drag ARC windows to external displays,
which is achieved by mapping window positions from a single virtual
display in ARC to multiple physical displays in Chrome OS. However,
multiple workspaces are not yet supported in ARC, so certain window
management operations leak this implementation detail. For example,
windows are maximized to the size of the virtual display, so their
content is partially off-screen. The compositor handles events from
ARC to differentiate between dragging and other geometry updates.
It also aborts the drag on deactivation, e.g. due to system dialogs.
In addition, the compositor now synchronizes window positions across
display configuration changes. In extended desktop mode, this prevents
a race that causes windows to briefly appear at an incorrect location
when a display is connected or disconnected.
BUG=642894
TEST=samus,minnie: ARC apps can be dragged to external displays using
different display settings, e.g. resolution, layout, orientation.
TEST=samus: Window positions are updated atomically when changing the
display configuration, e.g. closing the lid to enter docked mode.
==========
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: This issue passed the CQ dry run.
PTAL. As discussed offline, the client now uses screen coordinates for geometry updates, and acknowledges when the server changes the origin. The server compensates by accumulating origin offsets while geometry updates are in flight. The latest patch also includes refactoring we talked about, with some caveats: 1) |initial_bounds_| was abused to tell different shell surfaces apart. I've replaced it with an |origin_|, and additional state that determines how bounds are managed. Unfortunately, we need an enum rather than a flag because remote and popup shell surfaces can no longer be treated as a single case, i.e. the widget must be offset differently. 2) I considered moving the origin computation to ShellSurface by making it a display observer, but IMO this logic belongs to WaylandRemoteShell because the virtual display is specific to remote shell surfaces. We would also be repeating the union computation for each shell surface. 3) Dragging is still driven by the client, because this CL is complex enough.
Some early feedback. I haven't looked at server.cc code yet and need to review the other code again after interactive move part has been removed. https://codereview.chromium.org/2645663004/diff/40001/components/exo/shell_su... File components/exo/shell_surface.cc (right): https://codereview.chromium.org/2645663004/diff/40001/components/exo/shell_su... components/exo/shell_surface.cc:17: #include "ash/wm/drag_window_resizer.h" please remove from this patch https://codereview.chromium.org/2645663004/diff/40001/components/exo/shell_su... components/exo/shell_surface.cc:192: class CustomWindowResizer : public ash::WindowResizer { please remove from this patch https://codereview.chromium.org/2645663004/diff/40001/components/exo/shell_su... components/exo/shell_surface.cc:969: // TODO(domlaskowski): For shell surfaces whose bounds are controlled by the Please remove this as it shouldn't hurt to trigger the callback with a 0 origin delta. You can filter it out in the remote shell code. https://codereview.chromium.org/2645663004/diff/40001/components/exo/shell_su... components/exo/shell_surface.cc:1003: if (bounds_mode_ != BoundsMode::SHELL || !widget_ || !surface_ || why is this needed. should ignore_window_bounds_changes_ be set correctly when this happens instead? https://codereview.chromium.org/2645663004/diff/40001/components/exo/shell_su... components/exo/shell_surface.cc:1018: UpdateSurfaceBounds(); is this change needed? https://codereview.chromium.org/2645663004/diff/40001/components/exo/shell_su... components/exo/shell_surface.cc:1203: widget_->set_movement_disabled(bounds_mode_ != BoundsMode::SHELL); nit: consider "bool shell_controls_bounds = bounds_mode_ != BoundsMode::SHELL;" before this line to avoid doing '!=' twice https://codereview.chromium.org/2645663004/diff/40001/components/exo/shell_su... components/exo/shell_surface.cc:1441: if (bounds_mode_ == BoundsMode::SHELL && please split this into multiple lines as before. the bounds mode check as a separate statement with a "3) ..." comment https://codereview.chromium.org/2645663004/diff/40001/components/exo/shell_su... components/exo/shell_surface.cc:1456: if (bounds_mode_ == BoundsMode::CLIENT) { nit: use a switch statement https://codereview.chromium.org/2645663004/diff/40001/components/exo/shell_su... File components/exo/shell_surface.h (right): https://codereview.chromium.org/2645663004/diff/40001/components/exo/shell_su... components/exo/shell_surface.h:133: void SetMoving(); Please remove all functionality related to this from the patch. As discussed, this type of interactive move is bad from a security pov. https://codereview.chromium.org/2645663004/diff/40001/components/exo/shell_su... components/exo/shell_surface.h:336: base::Closure destroyed_callback_; why is this needed? please remove if related to interactive move. https://codereview.chromium.org/2645663004/diff/40001/components/exo/shell_su... components/exo/shell_surface.h:344: gfx::Vector2d pending_origin_offset_accumulator_; I guess this is just a cleanup? Can you keep it as before to limit the size of the patch or land this change separately before this patch? https://codereview.chromium.org/2645663004/diff/40001/components/exo/shell_su... components/exo/shell_surface.h:354: std::unique_ptr<ash::WindowResizer> move_resizer_; please remove from this patch
PTAL. Instead of waiting for a client request, the compositor now aborts dragging when the mouse is released. This is a stepping stone to server-driven dragging where the compositor instructs the client to initiate/abort dragging. However, as discussed offline, window state transitions must first be synchronized with the client, which is beyond the scope of this CL. Note that the dependency CL changes the remote shell protocol in anticipation of server-driven dragging. https://codereview.chromium.org/2645663004/diff/40001/components/exo/shell_su... File components/exo/shell_surface.cc (right): https://codereview.chromium.org/2645663004/diff/40001/components/exo/shell_su... components/exo/shell_surface.cc:17: #include "ash/wm/drag_window_resizer.h" On 2017/02/06 19:49:34, reveman wrote: > please remove from this patch See comment below. https://codereview.chromium.org/2645663004/diff/40001/components/exo/shell_su... components/exo/shell_surface.cc:192: class CustomWindowResizer : public ash::WindowResizer { On 2017/02/06 19:49:34, reveman wrote: > please remove from this patch Dragging and multi-display positioning are interdependent. https://codereview.chromium.org/2645663004/diff/40001/components/exo/shell_su... components/exo/shell_surface.cc:969: // TODO(domlaskowski): For shell surfaces whose bounds are controlled by the On 2017/02/06 19:49:34, reveman wrote: > Please remove this as it shouldn't hurt to trigger the callback with a 0 origin > delta. You can filter it out in the remote shell code. The TODO alludes to the above comment, i.e. animations should be disabled even though we have a configure callback. https://codereview.chromium.org/2645663004/diff/40001/components/exo/shell_su... components/exo/shell_surface.cc:1003: if (bounds_mode_ != BoundsMode::SHELL || !widget_ || !surface_ || On 2017/02/06 19:49:34, reveman wrote: > why is this needed. should ignore_window_bounds_changes_ be set correctly when > this happens instead? The origin offset should not be modified for size changes when bounds are controlled by the client, because the configure protocol does not support resizing. I've moved this check to where the shell surface is registered as a WindowObserver, because the other handler is also inapplicable. https://codereview.chromium.org/2645663004/diff/40001/components/exo/shell_su... components/exo/shell_surface.cc:1018: UpdateSurfaceBounds(); On 2017/02/06 19:49:33, reveman wrote: > is this change needed? Yes, UpdateSurfaceBounds is called from several places. https://codereview.chromium.org/2645663004/diff/40001/components/exo/shell_su... components/exo/shell_surface.cc:1203: widget_->set_movement_disabled(bounds_mode_ != BoundsMode::SHELL); On 2017/02/06 19:49:33, reveman wrote: > nit: consider "bool shell_controls_bounds = bounds_mode_ != BoundsMode::SHELL;" > before this line to avoid doing '!=' twice Done. https://codereview.chromium.org/2645663004/diff/40001/components/exo/shell_su... components/exo/shell_surface.cc:1441: if (bounds_mode_ == BoundsMode::SHELL && On 2017/02/06 19:49:33, reveman wrote: > please split this into multiple lines as before. the bounds mode check as a > separate statement with a "3) ..." comment Done, but it's "&&" so there's no third case. https://codereview.chromium.org/2645663004/diff/40001/components/exo/shell_su... components/exo/shell_surface.cc:1456: if (bounds_mode_ == BoundsMode::CLIENT) { On 2017/02/06 19:49:33, reveman wrote: > nit: use a switch statement Done. I've also merged the case below so it's clear that it only applies when bounds are controlled by the shell. https://codereview.chromium.org/2645663004/diff/40001/components/exo/shell_su... File components/exo/shell_surface.h (right): https://codereview.chromium.org/2645663004/diff/40001/components/exo/shell_su... components/exo/shell_surface.h:133: void SetMoving(); On 2017/02/06 19:49:34, reveman wrote: > Please remove all functionality related to this from the patch. As discussed, > this type of interactive move is bad from a security pov. Done. https://codereview.chromium.org/2645663004/diff/40001/components/exo/shell_su... components/exo/shell_surface.h:336: base::Closure destroyed_callback_; On 2017/02/06 19:49:34, reveman wrote: > why is this needed? please remove if related to interactive move. WaylandRemoteShell updates the origin for each shell surface, and uses this callback to observe shell surface lifetime. https://codereview.chromium.org/2645663004/diff/40001/components/exo/shell_su... components/exo/shell_surface.h:344: gfx::Vector2d pending_origin_offset_accumulator_; On 2017/02/06 19:49:34, reveman wrote: > I guess this is just a cleanup? Can you keep it as before to limit the size of > the patch or land this change separately before this patch? The diff for this rename is small, and the change is justified by the context of this patch, e.g. the new meaning of |origin_|. https://codereview.chromium.org/2645663004/diff/40001/components/exo/shell_su... components/exo/shell_surface.h:354: std::unique_ptr<ash::WindowResizer> move_resizer_; On 2017/02/06 19:49:34, reveman wrote: > please remove from this patch Done.
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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
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: Initial support for multiple displays in ARC
This CL adds the ability to drag ARC windows to external displays,
which is achieved by mapping window positions from a single virtual
display in ARC to multiple physical displays in Chrome OS. However,
multiple workspaces are not yet supported in ARC, so certain window
management operations leak this implementation detail. For example,
windows are maximized to the size of the virtual display, so their
content is partially off-screen. The compositor handles events from
ARC to differentiate between dragging and other geometry updates.
It also aborts the drag on deactivation, e.g. due to system dialogs.
In addition, the compositor now synchronizes window positions across
display configuration changes. In extended desktop mode, this prevents
a race that causes windows to briefly appear at an incorrect location
when a display is connected or disconnected.
BUG=642894
TEST=samus,minnie: ARC apps can be dragged to external displays using
different display settings, e.g. resolution, layout, orientation.
TEST=samus: Window positions are updated atomically when changing the
display configuration, e.g. closing the lid to enter docked mode.
==========
to
==========
exo: Initial support for multiple displays in ARC
This CL adds the ability to drag ARC windows to external displays,
which is achieved by mapping window positions from a single virtual
display in ARC to multiple physical displays in Chrome OS. However,
multiple workspaces are not yet supported in ARC, so certain window
management operations leak this implementation detail. For example,
windows are maximized to the size of the virtual display, so their
content is partially off-screen. The compositor initiates server-
side dragging when it receives a request from ARC. It aborts the
drag on mouse release and lost capture, e.g. due to system dialogs.
In addition, the compositor now synchronizes window positions across
display configuration changes. In extended desktop mode, this prevents
a race that causes windows to briefly appear at an incorrect location
when a display is connected or disconnected.
BUG=642894
TEST=samus,minnie: ARC apps can be dragged to external displays using
different display settings, e.g. resolution, layout, orientation.
TEST=samus: Window positions are updated atomically when changing the
display configuration, e.g. closing the lid to enter docked mode.
==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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: This issue passed the CQ dry run.
I haven't had time to review this yet but can you start by pulling out the changes mentioned below as I'd like to see them done separately as it will make the real logic easier to review. https://codereview.chromium.org/2645663004/diff/100001/components/exo/shell_s... File components/exo/shell_surface.h (right): https://codereview.chromium.org/2645663004/diff/100001/components/exo/shell_s... components/exo/shell_surface.h:50: enum class BoundsMode { SHELL, CLIENT, FIXED }; Can you start by pulling out the origin and BoundsMode change into a separate patch? https://codereview.chromium.org/2645663004/diff/100001/components/exo/shell_s... components/exo/shell_surface.h:349: gfx::Vector2d pending_origin_offset_accumulator_; I'd like to see these member variable name changes in a separate patch too.
Split out unrelated refactoring: https://codereview.chromium.org/2688483003/ https://codereview.chromium.org/2645663004/diff/100001/components/exo/shell_s... File components/exo/shell_surface.h (right): https://codereview.chromium.org/2645663004/diff/100001/components/exo/shell_s... components/exo/shell_surface.h:50: enum class BoundsMode { SHELL, CLIENT, FIXED }; On 2017/02/08 22:04:43, reveman wrote: > Can you start by pulling out the origin and BoundsMode change into a separate > patch? Done. https://codereview.chromium.org/2645663004/diff/100001/components/exo/shell_s... components/exo/shell_surface.h:349: gfx::Vector2d pending_origin_offset_accumulator_; On 2017/02/08 22:04:43, reveman wrote: > I'd like to see these member variable name changes in a separate patch too. Done.
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_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
Ping.
I'm not sure about the resizer related code. E.g. I don't think we should include the notion of a client side drag. It should still be a server side drag, just that the client is able to synchronize contents updates. ie. widget bounds should be change immediately in the server side but contents should not until acked. Can you move the resizer code to a follow up patch so we can discuss and review that code in isolation? https://codereview.chromium.org/2645663004/diff/180001/components/exo/wayland... File components/exo/wayland/server.cc (right): https://codereview.chromium.org/2645663004/diff/180001/components/exo/wayland... components/exo/wayland/server.cc:2235: std::unordered_set<ShellSurface*> shell_surfaces_; // Unowned. As discussed. Please refactor ShellSurface so this is not needed.
On 2017/02/17 01:29:34, reveman wrote: > I'm not sure about the resizer related code. E.g. I don't think we should > include the notion of a client side drag. It should still be a server side drag, > just that the client is able to synchronize contents updates. ie. widget bounds > should be change immediately in the server side but contents should not until > acked. Can you move the resizer code to a follow up patch so we can discuss and > review that code in isolation? > > https://codereview.chromium.org/2645663004/diff/180001/components/exo/wayland... > File components/exo/wayland/server.cc (right): > > https://codereview.chromium.org/2645663004/diff/180001/components/exo/wayland... > components/exo/wayland/server.cc:2235: std::unordered_set<ShellSurface*> > shell_surfaces_; // Unowned. > As discussed. Please refactor ShellSurface so this is not needed. The bounds/contents synchronization when moving/resizing is a separate issue. Currently, the client updates the bounds asynchronously when moving/resizing, so I'd like to keep it that way for now. The only difference is that the server keeps the widget in the same root window during dragging. Note that the resizer is needed to transfer the widget to the root window where the mouse is located when the window is dropped, so it cannot be split out. The virtual origin is an implementation detail of WaylandRemoteShell. ShellSurface should not know or care about virtual coordinate translation, and it appropriately exposes a public SetOrigin API.
On 2017/02/17 at 04:15:11, domlaskowski wrote: > On 2017/02/17 01:29:34, reveman wrote: > > I'm not sure about the resizer related code. E.g. I don't think we should > > include the notion of a client side drag. It should still be a server side drag, > > just that the client is able to synchronize contents updates. ie. widget bounds > > should be change immediately in the server side but contents should not until > > acked. Can you move the resizer code to a follow up patch so we can discuss and > > review that code in isolation? > > > > https://codereview.chromium.org/2645663004/diff/180001/components/exo/wayland... > > File components/exo/wayland/server.cc (right): > > > > https://codereview.chromium.org/2645663004/diff/180001/components/exo/wayland... > > components/exo/wayland/server.cc:2235: std::unordered_set<ShellSurface*> > > shell_surfaces_; // Unowned. > > As discussed. Please refactor ShellSurface so this is not needed. > > The bounds/contents synchronization when moving/resizing is a separate issue. Currently, the client updates the bounds asynchronously when moving/resizing, so I'd like to keep it that way for now. The only difference is that the server keeps the widget in the same root window during dragging. Note that the resizer is needed to transfer the widget to the root window where the mouse is located when the window is dropped, so it cannot be split out. Can you leave ShellSurface::Move() not implemented for the CLIENT case (same as FIXED) in this first patch and land the other changes first? > > The virtual origin is an implementation detail of WaylandRemoteShell. ShellSurface should not know or care about virtual coordinate translation, and it appropriately exposes a public SetOrigin API. I don't want logic that tracks instances using some container in wayland binding code. That type of logic should be unit tested and generally be avoided at all cost. With the introduction of CLIENT bounds in ShellSurface it also makes sense for ShellSurface code to be aware of DisplayMetrics changes imo. Also, why are we using virtual coordinates for the client? Why not use the same coordinates for the client as we use internally in chrome?
Description was changed from
==========
exo: Initial support for multiple displays in ARC
This CL adds the ability to drag ARC windows to external displays,
which is achieved by mapping window positions from a single virtual
display in ARC to multiple physical displays in Chrome OS. However,
multiple workspaces are not yet supported in ARC, so certain window
management operations leak this implementation detail. For example,
windows are maximized to the size of the virtual display, so their
content is partially off-screen. The compositor initiates server-
side dragging when it receives a request from ARC. It aborts the
drag on mouse release and lost capture, e.g. due to system dialogs.
In addition, the compositor now synchronizes window positions across
display configuration changes. In extended desktop mode, this prevents
a race that causes windows to briefly appear at an incorrect location
when a display is connected or disconnected.
BUG=642894
TEST=samus,minnie: ARC apps can be dragged to external displays using
different display settings, e.g. resolution, layout, orientation.
TEST=samus: Window positions are updated atomically when changing the
display configuration, e.g. closing the lid to enter docked mode.
==========
to
==========
exo: Initial support for multiple displays in ARC
This CL adds the ability to drag ARC windows to external displays,
which is achieved by mapping window positions from a single virtual
display in ARC to multiple physical displays in Chrome OS. However,
multiple workspaces are not yet supported in ARC, so certain window
management operations leak this implementation detail. For example,
windows are maximized to the size of the virtual display, so their
content is partially off-screen. The compositor initiates server-
side dragging when it receives a request from ARC. It aborts the
drag on mouse release and lost capture, e.g. due to system dialogs.
BUG=642894
TEST=samus,minnie: ARC apps can be dragged to external displays using
different display settings, e.g. resolution, layout, orientation.
==========
On 2017/02/17 17:50:13, reveman wrote: > Can you leave ShellSurface::Move() not implemented for the CLIENT case (same as > FIXED) in this first patch and land the other changes first? Done: https://codereview.chromium.org/2706543002/ > I don't want logic that tracks instances using some container in wayland binding > code. That type of logic should be unit tested and generally be avoided at all > cost. With the introduction of CLIENT bounds in ShellSurface it also makes sense > for ShellSurface code to be aware of DisplayMetrics changes imo. > > Also, why are we using virtual coordinates for the client? Why not use the same > coordinates for the client as we use internally in chrome? Done. ShellSurface now observes display configuration changes. However, I still think that WaylandRemoteShell should be responsible for the virtual display computation, so I've added a callback to ShellSurface. We are using virtual coordinates to match the coordinate system of the single client-side display. Introducing screen coordinates across the Android graphics stack is a large and complex change, most of which would have to be scrapped once proper multi-display support is implemented upstream.
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: This issue passed the CQ dry run.
Ping.
https://codereview.chromium.org/2645663004/diff/260001/components/exo/shell_s... File components/exo/shell_surface.cc (left): https://codereview.chromium.org/2645663004/diff/260001/components/exo/shell_s... components/exo/shell_surface.cc:1216: // Cannot start another drag if one is already taking place. why remove this? https://codereview.chromium.org/2645663004/diff/260001/components/exo/shell_s... components/exo/shell_surface.cc:1303: why not keep this the way it is? https://codereview.chromium.org/2645663004/diff/260001/components/exo/shell_s... File components/exo/shell_surface.cc (right): https://codereview.chromium.org/2645663004/diff/260001/components/exo/shell_s... components/exo/shell_surface.cc:154: nit: remove this blank line https://codereview.chromium.org/2645663004/diff/260001/components/exo/shell_s... components/exo/shell_surface.cc:157: // ash::WindowResizer: nit: // Overridden from ash::WindowResizer: https://codereview.chromium.org/2645663004/diff/260001/components/exo/shell_s... components/exo/shell_surface.cc:551: if (!widget_ || resizer_) why is this needed now? https://codereview.chromium.org/2645663004/diff/260001/components/exo/shell_s... components/exo/shell_surface.cc:559: AttemptToStartClientDrag(); nit: please move switch statement into AttemptToStartDrag instead so AttemptToStartClientDrag is not needed. I'm not a fan of the "ClientDrag" name in general https://codereview.chromium.org/2645663004/diff/260001/components/exo/shell_s... components/exo/shell_surface.cc:571: if (!widget_ || resizer_) why is this needed now? https://codereview.chromium.org/2645663004/diff/260001/components/exo/shell_s... components/exo/shell_surface.cc:1031: display_config_changed_callback_.Run(); why not send a configure request and adjust origin here directly for BoundsMode::CLIENT? I thought the purpose of bounds_mode_ was so we could do that. https://codereview.chromium.org/2645663004/diff/260001/components/exo/shell_s... components/exo/shell_surface.cc:1043: if (bounds_mode_ == BoundsMode::SHELL && We should be allowed to cancel a client drag too. Why add this special case? https://codereview.chromium.org/2645663004/diff/260001/components/exo/shell_s... components/exo/shell_surface.cc:1356: EndDrag(surface_->window(), revert); why are we using surface_->window() here instead of widget_->GetNativeWindow()? what if surface_ is gone? https://codereview.chromium.org/2645663004/diff/260001/components/exo/wayland... File components/exo/wayland/server.cc (right): https://codereview.chromium.org/2645663004/diff/260001/components/exo/wayland... components/exo/wayland/server.cc:2028: // This class is also responsible for conversion between server-side screen I don't understand why we need to create another coordinate space for the client. Why can't we just use chrome coordinates?
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...
https://codereview.chromium.org/2645663004/diff/260001/components/exo/shell_s... File components/exo/shell_surface.cc (left): https://codereview.chromium.org/2645663004/diff/260001/components/exo/shell_s... components/exo/shell_surface.cc:1216: // Cannot start another drag if one is already taking place. On 2017/03/02 02:00:38, reveman wrote: > why remove this? See above. https://codereview.chromium.org/2645663004/diff/260001/components/exo/shell_s... components/exo/shell_surface.cc:1303: On 2017/03/02 02:00:38, reveman wrote: > why not keep this the way it is? Refactored to be consistent with AttemptToStartDrag. https://codereview.chromium.org/2645663004/diff/260001/components/exo/shell_s... File components/exo/shell_surface.cc (right): https://codereview.chromium.org/2645663004/diff/260001/components/exo/shell_s... components/exo/shell_surface.cc:154: On 2017/03/02 02:00:38, reveman wrote: > nit: remove this blank line Done. https://codereview.chromium.org/2645663004/diff/260001/components/exo/shell_s... components/exo/shell_surface.cc:157: // ash::WindowResizer: On 2017/03/02 02:00:38, reveman wrote: > nit: // Overridden from ash::WindowResizer: Done. https://codereview.chromium.org/2645663004/diff/260001/components/exo/shell_s... components/exo/shell_surface.cc:551: if (!widget_ || resizer_) On 2017/03/02 02:00:38, reveman wrote: > why is this needed now? This check was pulled out from AttemptToStartDrag to be shared with AttemptToStartClientDrag, but I've merged those in the latest patch. https://codereview.chromium.org/2645663004/diff/260001/components/exo/shell_s... components/exo/shell_surface.cc:559: AttemptToStartClientDrag(); On 2017/03/02 02:00:38, reveman wrote: > nit: please move switch statement into AttemptToStartDrag instead so > AttemptToStartClientDrag is not needed. I'm not a fan of the "ClientDrag" name > in general Merged AttemptToStartClientDrag into AttemptToStartDrag. https://codereview.chromium.org/2645663004/diff/260001/components/exo/shell_s... components/exo/shell_surface.cc:571: if (!widget_ || resizer_) On 2017/03/02 02:00:38, reveman wrote: > why is this needed now? See above. https://codereview.chromium.org/2645663004/diff/260001/components/exo/shell_s... components/exo/shell_surface.cc:1031: display_config_changed_callback_.Run(); On 2017/03/02 02:00:38, reveman wrote: > why not send a configure request and adjust origin here directly for > BoundsMode::CLIENT? I thought the purpose of bounds_mode_ was so we could do > that. Given that the origin is part of the public ShellSurface API (constructor and SetOrigin), I think it's cleaner for the origin to be computed by the user of ShellSurface, i.e. WaylandRemoteShell. https://codereview.chromium.org/2645663004/diff/260001/components/exo/shell_s... components/exo/shell_surface.cc:1043: if (bounds_mode_ == BoundsMode::SHELL && On 2017/03/02 02:00:38, reveman wrote: > We should be allowed to cancel a client drag too. Why add this special case? We need to sync window states to revert dragging properly in the CLIENT case. I figured that we should disable this until then (even though it's not an issue because the client currently destroys the window on VKEY_ESCAPE). https://codereview.chromium.org/2645663004/diff/260001/components/exo/shell_s... components/exo/shell_surface.cc:1356: EndDrag(surface_->window(), revert); On 2017/03/02 02:00:38, reveman wrote: > why are we using surface_->window() here instead of widget_->GetNativeWindow()? > what if surface_ is gone? The surface window needs to have capture, because it's the target for mouse events that get forwarded to the client. Good point, I've added a null check in AttemptToStartDrag and a DCHECK here as a precaution against a bad client that sends a "move" request after the surface is destroyed. https://codereview.chromium.org/2645663004/diff/260001/components/exo/wayland... File components/exo/wayland/server.cc (right): https://codereview.chromium.org/2645663004/diff/260001/components/exo/wayland... components/exo/wayland/server.cc:2028: // This class is also responsible for conversion between server-side screen On 2017/03/02 02:00:38, reveman wrote: > I don't understand why we need to create another coordinate space for the > client. Why can't we just use chrome coordinates? Because from the perspective of SF and DMS/WMS/AMS, there is a single display and coordinates are relative to its top-left corner. Converting from that coordinate space at the Wayland boundary is a simple solution that avoids drastic non-upstreamable changes across the Android stack.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lg after addressing latest set of comments https://codereview.chromium.org/2645663004/diff/260001/components/exo/shell_s... File components/exo/shell_surface.cc (right): https://codereview.chromium.org/2645663004/diff/260001/components/exo/shell_s... components/exo/shell_surface.cc:1031: display_config_changed_callback_.Run(); On 2017/03/03 at 13:50:40, Dominik Laskowski wrote: > On 2017/03/02 02:00:38, reveman wrote: > > why not send a configure request and adjust origin here directly for > > BoundsMode::CLIENT? I thought the purpose of bounds_mode_ was so we could do > > that. > > Given that the origin is part of the public ShellSurface API (constructor and SetOrigin), I think it's cleaner for the origin to be computed by the user of ShellSurface, i.e. WaylandRemoteShell. As other changes to origin happen internally and result in configure events I much prefer if that was also the case for display configuration changes. If this means we can remove SetOrigin API then that would be great but maybe we still need that.. https://codereview.chromium.org/2645663004/diff/260001/components/exo/shell_s... components/exo/shell_surface.cc:1043: if (bounds_mode_ == BoundsMode::SHELL && On 2017/03/03 at 13:50:40, Dominik Laskowski wrote: > On 2017/03/02 02:00:38, reveman wrote: > > We should be allowed to cancel a client drag too. Why add this special case? > > We need to sync window states to revert dragging properly in the CLIENT case. I figured that we should disable this until then (even though it's not an issue because the client currently destroys the window on VKEY_ESCAPE). Please leave this the way it's supposed to work long term of that's not causing problems. Temporary code that is doing something else on the Chrome side should at a minimal have a TODO statement with description and a bug number to make it clear that the code is should updated when possible. https://codereview.chromium.org/2645663004/diff/260001/components/exo/shell_s... components/exo/shell_surface.cc:1356: EndDrag(surface_->window(), revert); On 2017/03/03 at 13:50:40, Dominik Laskowski wrote: > On 2017/03/02 02:00:38, reveman wrote: > > why are we using surface_->window() here instead of widget_->GetNativeWindow()? > > what if surface_ is gone? > > The surface window needs to have capture, because it's the target for mouse events that get forwarded to the client. Good point, I've added a null check in AttemptToStartDrag and a DCHECK here as a precaution against a bad client that sends a "move" request after the surface is destroyed. The general idea of doing a capture on the surface window to have events still sent to the client seems like a bit of a hack and doesn't work with MUS support as only the top-level ShellSurface will have a window in the future. I think it would be best to have these changes to movement sent back to the client as origin changes in configure events. This would also allow features like window snapping to work properly. I'm OK saving this as a follow up but please file bugs and add appropriate TODO's where needed in the code. https://codereview.chromium.org/2645663004/diff/260001/components/exo/wayland... File components/exo/wayland/server.cc (right): https://codereview.chromium.org/2645663004/diff/260001/components/exo/wayland... components/exo/wayland/server.cc:2028: // This class is also responsible for conversion between server-side screen On 2017/03/03 at 13:50:41, Dominik Laskowski wrote: > On 2017/03/02 02:00:38, reveman wrote: > > I don't understand why we need to create another coordinate space for the > > client. Why can't we just use chrome coordinates? > > Because from the perspective of SF and DMS/WMS/AMS, there is a single display and coordinates are relative to its top-left corner. Converting from that coordinate space at the Wayland boundary is a simple solution that avoids drastic non-upstreamable changes across the Android stack. This is android specific so very confusing to see in Chromium code that is supposed to be client agnostic. I'm not suggesting that we modify the Android framework. I'm suggesting that we just modify the wayland code on the client side that process these events instead of the code here that is not Android specific. https://codereview.chromium.org/2645663004/diff/280001/components/exo/shell_s... File components/exo/shell_surface.cc (right): https://codereview.chromium.org/2645663004/diff/280001/components/exo/shell_s... components/exo/shell_surface.cc:148: class CustomWindowResizer : public ash::WindowResizer { Please add a class description that explains how this resizer is different from the default one. You can explain why it's needed in the client bounds case where you instantiate it. https://codereview.chromium.org/2645663004/diff/280001/components/exo/shell_s... components/exo/shell_surface.cc:1331: // The resizer renders phantom windows, but does not move the window. Is it not also used for moving windows between displays? https://codereview.chromium.org/2645663004/diff/280001/components/exo/shell_s... components/exo/shell_surface.cc:1481: LOG_IF(ERROR, widget_bounds.size() != new_widget_bounds.size()) nit: DLOG_IF if this is something a malicious client can produce https://codereview.chromium.org/2645663004/diff/280001/components/exo/shell_s... File components/exo/shell_surface.h (right): https://codereview.chromium.org/2645663004/diff/280001/components/exo/shell_s... components/exo/shell_surface.h:293: aura::Window* GetDragWindow() const; Can this be private? nit: please add a comment explaining what this returns
https://codereview.chromium.org/2645663004/diff/260001/components/exo/shell_s... File components/exo/shell_surface.cc (right): https://codereview.chromium.org/2645663004/diff/260001/components/exo/shell_s... components/exo/shell_surface.cc:1031: display_config_changed_callback_.Run(); On 2017/03/06 19:02:13, reveman wrote: > On 2017/03/03 at 13:50:40, Dominik Laskowski wrote: > > On 2017/03/02 02:00:38, reveman wrote: > > > why not send a configure request and adjust origin here directly for > > > BoundsMode::CLIENT? I thought the purpose of bounds_mode_ was so we could do > > > that. > > > > Given that the origin is part of the public ShellSurface API (constructor and > SetOrigin), I think it's cleaner for the origin to be computed by the user of > ShellSurface, i.e. WaylandRemoteShell. > > As other changes to origin happen internally and result in configure events I > much prefer if that was also the case for display configuration changes. If this > means we can remove SetOrigin API then that would be great but maybe we still > need that.. But then ShellSurface would contain logic specific to Android. With this injection, the virtual display concept is at least contained in WaylandRemoteShell. https://codereview.chromium.org/2645663004/diff/260001/components/exo/shell_s... components/exo/shell_surface.cc:1043: if (bounds_mode_ == BoundsMode::SHELL && On 2017/03/06 19:02:13, reveman wrote: > On 2017/03/03 at 13:50:40, Dominik Laskowski wrote: > > On 2017/03/02 02:00:38, reveman wrote: > > > We should be allowed to cancel a client drag too. Why add this special case? > > > > We need to sync window states to revert dragging properly in the CLIENT case. > I figured that we should disable this until then (even though it's not an issue > because the client currently destroys the window on VKEY_ESCAPE). > > Please leave this the way it's supposed to work long term of that's not causing > problems. Temporary code that is doing something else on the Chrome side should > at a minimal have a TODO statement with description and a bug number to make it > clear that the code is should updated when possible. Reverted and added TODO. https://codereview.chromium.org/2645663004/diff/260001/components/exo/shell_s... components/exo/shell_surface.cc:1356: EndDrag(surface_->window(), revert); On 2017/03/06 19:02:13, reveman wrote: > On 2017/03/03 at 13:50:40, Dominik Laskowski wrote: > > On 2017/03/02 02:00:38, reveman wrote: > > > why are we using surface_->window() here instead of > widget_->GetNativeWindow()? > > > what if surface_ is gone? > > > > The surface window needs to have capture, because it's the target for mouse > events that get forwarded to the client. Good point, I've added a null check in > AttemptToStartDrag and a DCHECK here as a precaution against a bad client that > sends a "move" request after the surface is destroyed. > > The general idea of doing a capture on the surface window to have events still > sent to the client seems like a bit of a hack and doesn't work with MUS support > as only the top-level ShellSurface will have a window in the future. I think it > would be best to have these changes to movement sent back to the client as > origin changes in configure events. This would also allow features like window > snapping to work properly. > > I'm OK saving this as a follow up but please file bugs and add appropriate > TODO's where needed in the code. Filed crbug.com/699746 and added TODOs. https://codereview.chromium.org/2645663004/diff/260001/components/exo/wayland... File components/exo/wayland/server.cc (right): https://codereview.chromium.org/2645663004/diff/260001/components/exo/wayland... components/exo/wayland/server.cc:2028: // This class is also responsible for conversion between server-side screen On 2017/03/06 19:02:13, reveman wrote: > This is android specific so very confusing to see in Chromium code that is > supposed to be client agnostic. I'm not suggesting that we modify the Android > framework. I'm suggesting that we just modify the wayland code on the client > side that process these events instead of the code here that is not Android > specific. But the server needs to know about the virtual display for synchronization, i.e. to compensate when the screen position of the virtual origin changes. https://codereview.chromium.org/2645663004/diff/280001/components/exo/shell_s... File components/exo/shell_surface.cc (right): https://codereview.chromium.org/2645663004/diff/280001/components/exo/shell_s... components/exo/shell_surface.cc:148: class CustomWindowResizer : public ash::WindowResizer { On 2017/03/06 19:02:13, reveman wrote: > Please add a class description that explains how this resizer is different from > the default one. You can explain why it's needed in the client bounds case where > you instantiate it. Done. https://codereview.chromium.org/2645663004/diff/280001/components/exo/shell_s... components/exo/shell_surface.cc:1331: // The resizer renders phantom windows, but does not move the window. On 2017/03/06 19:02:13, reveman wrote: > Is it not also used for moving windows between displays? True, amended. https://codereview.chromium.org/2645663004/diff/280001/components/exo/shell_s... components/exo/shell_surface.cc:1481: LOG_IF(ERROR, widget_bounds.size() != new_widget_bounds.size()) On 2017/03/06 19:02:13, reveman wrote: > nit: DLOG_IF if this is something a malicious client can produce Done. https://codereview.chromium.org/2645663004/diff/280001/components/exo/shell_s... File components/exo/shell_surface.h (right): https://codereview.chromium.org/2645663004/diff/280001/components/exo/shell_s... components/exo/shell_surface.h:293: aura::Window* GetDragWindow() const; On 2017/03/06 19:02:13, reveman wrote: > Can this be private? nit: please add a comment explaining what this returns Already private. Done.
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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
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: Initial support for multiple displays in ARC
This CL adds the ability to drag ARC windows to external displays,
which is achieved by mapping window positions from a single virtual
display in ARC to multiple physical displays in Chrome OS. However,
multiple workspaces are not yet supported in ARC, so certain window
management operations leak this implementation detail. For example,
windows are maximized to the size of the virtual display, so their
content is partially off-screen. The compositor initiates server-
side dragging when it receives a request from ARC. It aborts the
drag on mouse release and lost capture, e.g. due to system dialogs.
BUG=642894
TEST=samus,minnie: ARC apps can be dragged to external displays using
different display settings, e.g. resolution, layout, orientation.
==========
to
==========
exo: Initial support for multiple displays in ARC
This CL adds the ability to drag ARC windows to external displays,
which is achieved by mapping window positions from a single virtual
display in ARC to multiple physical displays in Chrome OS. The new
version of the Wayland protocol enables the ARC window manager to
perform window layout on multiple workspaces, which are mapped to
server-side displays. The compositor also handles events from ARC
to differentiate between dragging and other geometry updates. This
enables it to render phantom windows during dragging, and transfer
the window to the target root window when the drag ends.
BUG=642894
TEST=samus,minnie: ARC apps can be dragged to external displays using
different display settings, e.g. resolution, layout, orientation.
==========
PTAL. As discussed offline, the client is now responsible for converting between screen and virtual coordinates.
lgtm https://codereview.chromium.org/2645663004/diff/340001/components/exo/shell_s... File components/exo/shell_surface.cc (right): https://codereview.chromium.org/2645663004/diff/340001/components/exo/shell_s... components/exo/shell_surface.cc:1034: display::Display primary_display; nit: maybe s/primary_display/old_primary_display/ to make the code below a bit more clear https://codereview.chromium.org/2645663004/diff/340001/components/exo/shell_s... components/exo/shell_surface.cc:1274: if (bounds_mode_ == BoundsMode::SHELL) nit: switch statement here without a "default" case would be nice as it makes it more clear what we do if mode is neither SHELL nor CLIENT
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2645663004/diff/340001/components/exo/shell_s... File components/exo/shell_surface.cc (right): https://codereview.chromium.org/2645663004/diff/340001/components/exo/shell_s... components/exo/shell_surface.cc:1034: display::Display primary_display; On 2017/03/15 21:26:14, reveman wrote: > nit: maybe s/primary_display/old_primary_display/ to make the code below a bit > more clear Done. https://codereview.chromium.org/2645663004/diff/340001/components/exo/shell_s... components/exo/shell_surface.cc:1274: if (bounds_mode_ == BoundsMode::SHELL) On 2017/03/15 21:26:14, reveman wrote: > nit: switch statement here without a "default" case would be nice as it makes it > more clear what we do if mode is neither SHELL nor CLIENT Done.
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: 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/2645663004/#ps360001 (title: "Address nits")
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": 360001, "attempt_start_ts": 1489618366276690,
"parent_rev": "844158bb22c9b0008e11689bf3370f14295cfe65", "commit_rev":
"623c02906151795123d5b40dfa3b931f98a916cc"}
Message was sent while issue was closed.
Description was changed from
==========
exo: Initial support for multiple displays in ARC
This CL adds the ability to drag ARC windows to external displays,
which is achieved by mapping window positions from a single virtual
display in ARC to multiple physical displays in Chrome OS. The new
version of the Wayland protocol enables the ARC window manager to
perform window layout on multiple workspaces, which are mapped to
server-side displays. The compositor also handles events from ARC
to differentiate between dragging and other geometry updates. This
enables it to render phantom windows during dragging, and transfer
the window to the target root window when the drag ends.
BUG=642894
TEST=samus,minnie: ARC apps can be dragged to external displays using
different display settings, e.g. resolution, layout, orientation.
==========
to
==========
exo: Initial support for multiple displays in ARC
This CL adds the ability to drag ARC windows to external displays,
which is achieved by mapping window positions from a single virtual
display in ARC to multiple physical displays in Chrome OS. The new
version of the Wayland protocol enables the ARC window manager to
perform window layout on multiple workspaces, which are mapped to
server-side displays. The compositor also handles events from ARC
to differentiate between dragging and other geometry updates. This
enables it to render phantom windows during dragging, and transfer
the window to the target root window when the drag ends.
BUG=642894
TEST=samus,minnie: ARC apps can be dragged to external displays using
different display settings, e.g. resolution, layout, orientation.
Review-Url: https://codereview.chromium.org/2645663004
Cr-Commit-Position: refs/heads/master@{#457249}
Committed:
https://chromium.googlesource.com/chromium/src/+/623c02906151795123d5b40dfa3b...
==========
Message was sent while issue was closed.
Committed patchset #19 (id:360001) as https://chromium.googlesource.com/chromium/src/+/623c02906151795123d5b40dfa3b... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
