Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2396883003:
exo: Fix dragging edge cases (Closed)
|
Created:
4 years, 2 months ago by Dominik Laskowski Modified:
3 years, 10 months ago CC:
chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionexo: Fix dragging edge cases
This CL fixes two edge cases that affect both ARC and XDG clients,
namely aborting the drag when entering immersive or overview mode.
Note that this is only a partial server-side fix. For immersive mode,
there is a client-side race between the drag bounds updates and the
immersive bounds update, which sometimes results in an inconsistent
state where the window is maximized but the content is not. For
overview mode, there is a mouse input bug that resets the cursor's
position to the origin, moving the window along with it.
BUG=642894
BUG=631136
TEST=Entering immersive mode while dragging across displays does not
result in an inconsistent state where the window is full screen
but moving the mouse keeps updating its opacity.
TEST=Entering overview mode while dragging across displays does not
result in a crash.
Patch Set 1 #Patch Set 2 : Rebase and format #Patch Set 3 : Fix unit tests #
Total comments: 19
Patch Set 4 : Rebase #Patch Set 5 : Address comments #Patch Set 6 : Fix presubmit errors #
Total comments: 2
Patch Set 7 : Split #Patch Set 8 : Rebase #Patch Set 9 : Move coordinate translation #Patch Set 10 : Remove useless header #Patch Set 11 : Split #Patch Set 12 : Rebase #
Total comments: 9
Depends on Patchset: Messages
Total messages: 46 (27 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.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
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_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.
sorry for the delay. I started reviewing this but it's large and has a few moving parts so I think it's best to split it up if possible. can we start by landing the set/unset_moving parts first?
https://codereview.chromium.org/2396883003/diff/40001/components/exo/display.h File components/exo/display.h (right): https://codereview.chromium.org/2396883003/diff/40001/components/exo/display.... components/exo/display.h:38: // displayable output. can you add documentation how coordinates work between ARC++ and Chrome? https://codereview.chromium.org/2396883003/diff/40001/components/exo/shell_su... File components/exo/shell_surface.cc (right): https://codereview.chromium.org/2396883003/diff/40001/components/exo/shell_su... components/exo/shell_surface.cc:201: // ash::WindowResizer: https://codereview.chromium.org/2396883003/diff/40001/components/exo/shell_su... components/exo/shell_surface.cc:207: ash::WmShell* const shell_; DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/2396883003/diff/40001/components/exo/shell_su... components/exo/shell_surface.cc:986: } Can you test what happens when display configuration changes? (add/remove display and/or switching to/from mirror mode) https://codereview.chromium.org/2396883003/diff/40001/components/exo/shell_su... File components/exo/shell_surface.h (right): https://codereview.chromium.org/2396883003/diff/40001/components/exo/shell_su... components/exo/shell_surface.h:291: const Display& display_; can you use const Display* ? https://codereview.chromium.org/2396883003/diff/40001/components/exo/shell_su... components/exo/shell_surface.h:324: std::unique_ptr<ash::WindowResizer> move_resizer_; the original resizer_ is also used to drag. How about phantom_window_resizer? I also wonder if we really need two resizer. If we just want to create a phantom window, you may not need resizer.
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...
Most of the ShellSurface changes (including set_moving/unset_moving) depend on coordinate conversion, which must land along with the WaylandRemoteShell changes. The remaining code is for handling edge cases, and I don't think splitting it up is worthwhile because it's comparatively simple. https://codereview.chromium.org/2396883003/diff/40001/components/exo/display.h File components/exo/display.h (right): https://codereview.chromium.org/2396883003/diff/40001/components/exo/display.... components/exo/display.h:38: // displayable output. On 2016/10/12 01:56:54, oshima wrote: > can you add documentation how coordinates work between ARC++ and Chrome? Done. https://codereview.chromium.org/2396883003/diff/40001/components/exo/shell_su... File components/exo/shell_surface.cc (right): https://codereview.chromium.org/2396883003/diff/40001/components/exo/shell_su... components/exo/shell_surface.cc:201: On 2016/10/12 01:56:54, oshima wrote: > // ash::WindowResizer: Done. https://codereview.chromium.org/2396883003/diff/40001/components/exo/shell_su... components/exo/shell_surface.cc:207: ash::WmShell* const shell_; On 2016/10/12 01:56:54, oshima wrote: > DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/2396883003/diff/40001/components/exo/shell_su... components/exo/shell_surface.cc:986: } On 2016/10/12 01:56:54, oshima wrote: > Can you test what happens when display configuration changes? (add/remove > display and/or switching to/from mirror mode) In general, this edge case is broken even for Chrome windows. Plugging a display in overview mode results in an invalid state where only one display is in overview mode. Unplugging a display containing windows in overview mode results in a crash. The code above is needed to prevent a crash when entering overview mode while dragging. By the way, this case (i.e. entering/exiting overview mode while dragging) is somewhat broken for Chrome windows. It's possible for a window to become translucent after exiting overview mode. I have another small patch to fix this. https://codereview.chromium.org/2396883003/diff/40001/components/exo/shell_su... File components/exo/shell_surface.h (right): https://codereview.chromium.org/2396883003/diff/40001/components/exo/shell_su... components/exo/shell_surface.h:291: const Display& display_; On 2016/10/12 01:56:54, oshima wrote: > can you use const Display* ? It should never be null, and the reference makes that clear. https://codereview.chromium.org/2396883003/diff/40001/components/exo/shell_su... components/exo/shell_surface.h:324: std::unique_ptr<ash::WindowResizer> move_resizer_; On 2016/10/12 01:56:54, oshima wrote: > the original resizer_ is also used to drag. How about > > phantom_window_resizer? > > I also wonder if we really need two resizer. If we just want to create a phantom > window, you may not need resizer. The pointer is also used to check whether a client-driven move is in progress, so that name would be confusing. We need all the logic in DragWindowResizer except resizer chaining. We could factor it out into another class, but chaining a dummy resizer is simpler.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Ok, I'll do a thorough review of this tomorrow then.
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.
Again, sorry for the delay. I'm still trying to understand the details of this CL. The part I find confusing is that I expected the notion of a virtual screen to be completely constrained to ARC clients and layered on top of proper multi-display support for non-ARC clients but I'm seeing a lot of general code in this CL that seems to affect non-ARC clients and I'm trying to understand why that is needed. I'm going to be in MTV Tue-Thu. Let's work together then to make sure this functionality can land asap. https://codereview.chromium.org/2396883003/diff/100001/components/exo/display.h File components/exo/display.h (right): https://codereview.chromium.org/2396883003/diff/100001/components/exo/display... components/exo/display.h:68: template <typename Point> why the template argument? what are the different point types that are passed to this function?
https://codereview.chromium.org/2396883003/diff/40001/components/exo/shell_su... File components/exo/shell_surface.cc (right): https://codereview.chromium.org/2396883003/diff/40001/components/exo/shell_su... components/exo/shell_surface.cc:986: } On 2016/10/13 03:21:17, Dominik Laskowski wrote: > On 2016/10/12 01:56:54, oshima wrote: > > Can you test what happens when display configuration changes? (add/remove > > display and/or switching to/from mirror mode) > > In general, this edge case is broken even for Chrome windows. Plugging a display > in overview mode results in an invalid state where only one display is in > overview mode. Unplugging a display containing windows in overview mode results > in a crash. That's a separate issue. When display configuraiton changes, it should drop the window in the correct display. > > The code above is needed to prevent a crash when entering overview mode while > dragging. By the way, this case (i.e. entering/exiting overview mode while > dragging) is somewhat broken for Chrome windows. It's possible for a window to > become translucent after exiting overview mode. I have another small patch to > fix this. https://codereview.chromium.org/2396883003/diff/40001/components/exo/shell_su... File components/exo/shell_surface.h (right): https://codereview.chromium.org/2396883003/diff/40001/components/exo/shell_su... components/exo/shell_surface.h:291: const Display& display_; On 2016/10/13 03:21:17, Dominik Laskowski wrote: > On 2016/10/12 01:56:54, oshima wrote: > > can you use const Display* ? > > It should never be null, and the reference makes that clear. It doesn't prevent from passing null reference. assertion (DCHECK/DCHECK) is better choice IMO, but I'll leave this to the owner. https://codereview.chromium.org/2396883003/diff/40001/components/exo/shell_su... components/exo/shell_surface.h:324: std::unique_ptr<ash::WindowResizer> move_resizer_; On 2016/10/13 03:21:17, Dominik Laskowski wrote: > On 2016/10/12 01:56:54, oshima wrote: > > the original resizer_ is also used to drag. How about > > > > phantom_window_resizer? > > > > I also wonder if we really need two resizer. If we just want to create a > phantom > > window, you may not need resizer. > > The pointer is also used to check whether a client-driven move is in progress, > so that name would be confusing. > > We need all the logic in DragWindowResizer except resizer chaining. We could > factor it out into another class, but chaining a dummy resizer is simpler. Hmm, then I may be reading this wrong. Can you explain why we need two resizer?
https://codereview.chromium.org/2396883003/diff/40001/components/exo/shell_su... File components/exo/shell_surface.cc (right): https://codereview.chromium.org/2396883003/diff/40001/components/exo/shell_su... components/exo/shell_surface.cc:986: } On 2016/10/17 17:22:58, oshima (OOO Oct 17) wrote: > On 2016/10/13 03:21:17, Dominik Laskowski wrote: > > On 2016/10/12 01:56:54, oshima wrote: > > > Can you test what happens when display configuration changes? (add/remove > > > display and/or switching to/from mirror mode) > > > > In general, this edge case is broken even for Chrome windows. Plugging a > display > > in overview mode results in an invalid state where only one display is in > > overview mode. Unplugging a display containing windows in overview mode > results > > in a crash. > > That's a separate issue. When display configuraiton changes, it should > drop the window in the correct display. > > > > > > The code above is needed to prevent a crash when entering overview mode while > > dragging. By the way, this case (i.e. entering/exiting overview mode while > > dragging) is somewhat broken for Chrome windows. It's possible for a window to > > become translucent after exiting overview mode. I have another small patch to > > fix this. > Sorry, I misread that as changing the display configuration while in overview mode. Reverting on display reconfiguration will be handled in Android, but support for multiple workspaces is a prerequisite, without which Android can't detect certain display layout changes, i.e. when the size of the virtual screen stays the same. Currently, display configuration changes seem to reset the Android mouse position to the origin, so the dragged window follows along. Moving the mouse after the display has reconfigured resumes the drag at the mouse's actual position. https://codereview.chromium.org/2396883003/diff/40001/components/exo/shell_su... File components/exo/shell_surface.h (right): https://codereview.chromium.org/2396883003/diff/40001/components/exo/shell_su... components/exo/shell_surface.h:291: const Display& display_; On 2016/10/17 17:22:58, oshima (OOO Oct 17) wrote: > On 2016/10/13 03:21:17, Dominik Laskowski wrote: > > On 2016/10/12 01:56:54, oshima wrote: > > > can you use const Display* ? > > > > It should never be null, and the reference makes that clear. > > It doesn't prevent from passing null reference. assertion (DCHECK/DCHECK) is > better choice IMO, but I'll leave this to the owner. The caller would have to dereference a null pointer though. I don't mind changing it to a pointer, but I feel this is self-documenting. reveman: let me know what style you prefer. https://codereview.chromium.org/2396883003/diff/40001/components/exo/shell_su... components/exo/shell_surface.h:324: std::unique_ptr<ash::WindowResizer> move_resizer_; On 2016/10/17 17:22:58, oshima (OOO Oct 17) wrote: > On 2016/10/13 03:21:17, Dominik Laskowski wrote: > > On 2016/10/12 01:56:54, oshima wrote: > > > the original resizer_ is also used to drag. How about > > > > > > phantom_window_resizer? > > > > > > I also wonder if we really need two resizer. If we just want to create a > > phantom > > > window, you may not need resizer. > > > > The pointer is also used to check whether a client-driven move is in progress, > > so that name would be confusing. > > > > We need all the logic in DragWindowResizer except resizer chaining. We could > > factor it out into another class, but chaining a dummy resizer is simpler. > > Hmm, then I may be reading this wrong. Can you explain why we need two resizer? There are three states we want to detect: No drag: !resizer_ && !move_resizer_ Client-side drag: !resizer_ && move_resizer_ Server-side drag: resizer_ && !move_resizer_ https://codereview.chromium.org/2396883003/diff/100001/components/exo/display.h File components/exo/display.h (right): https://codereview.chromium.org/2396883003/diff/100001/components/exo/display... components/exo/display.h:68: template <typename Point> On 2016/10/16 23:16:02, reveman wrote: > why the template argument? what are the different point types that are passed to > this function? gfx::Point and gfx::Rect. Would overloads be preferable?
https://codereview.chromium.org/2396883003/diff/40001/components/exo/shell_su... File components/exo/shell_surface.h (right): https://codereview.chromium.org/2396883003/diff/40001/components/exo/shell_su... components/exo/shell_surface.h:324: std::unique_ptr<ash::WindowResizer> move_resizer_; On 2016/10/18 21:22:56, Dominik Laskowski wrote: > On 2016/10/17 17:22:58, oshima (OOO Oct 17) wrote: > > On 2016/10/13 03:21:17, Dominik Laskowski wrote: > > > On 2016/10/12 01:56:54, oshima wrote: > > > > the original resizer_ is also used to drag. How about > > > > > > > > phantom_window_resizer? > > > > > > > > I also wonder if we really need two resizer. If we just want to create a > > > phantom > > > > window, you may not need resizer. > > > > > > The pointer is also used to check whether a client-driven move is in > progress, > > > so that name would be confusing. > > > > > > We need all the logic in DragWindowResizer except resizer chaining. We could > > > factor it out into another class, but chaining a dummy resizer is simpler. > > > > Hmm, then I may be reading this wrong. Can you explain why we need two > resizer? > > There are three states we want to detect: > > No drag: !resizer_ && !move_resizer_ > Client-side drag: !resizer_ && move_resizer_ > Server-side drag: resizer_ && !move_resizer_ After an offline chat with oshima, the suggestion was to use a single resizer, and detect the above states using other member variables. However, this would complicate the code, because the only way to distinguish a remote shell surface is: !initial_bounds_.IsEmpty() && !parent_ reveman: do you prefer the two resizers? If not, could we introduce explicit state to tell shell surfaces apart, e.g. an enum for regular/pop-up/remote? That would be contradictory with the idea of moving concepts specific to the remote shell (e.g. virtual coordinates) out of ShellSurface. On the other hand, it would simplify a lot of conditions that implicitly depend on otherwise unused initial state, e.g. gfx::Size(1, 1).
Ping? Also, it seems like much of the shell surface code will have to be restructured once we migrate to layered Wayland interfaces, so could we postpone the coordinate conversion refactoring until then?
On 2016/10/26 22:10:53, Dominik Laskowski wrote: > Ping? Also, it seems like much of the shell surface code will have to be > restructured once we migrate to layered Wayland interfaces, so could we postpone > the coordinate conversion refactoring until then? I believe you're waiting for review from reveman right?
On 2016/10/26 at 22:19:11, oshima wrote: > On 2016/10/26 22:10:53, Dominik Laskowski wrote: > > Ping? Also, it seems like much of the shell surface code will have to be > > restructured once we migrate to layered Wayland interfaces, so could we postpone > > the coordinate conversion refactoring until then? > > I believe you're waiting for review from reveman right? At this time, with the initial version of ARC++ already having been released, I'd like to make sure we don't make any rushed design decisions but instead try to get it right the first time and not make exo code more complicated than it has to be. For ARC++ I think we should try hard to align with upstream wayland interfaces as that makes maintaining this code much easier. For window dragging, that means not having android control that anymore. For virtual screen size management, that means having it handled as much as possible on the client side. The TEST= line for this patch refers to weston-terminal but most of the changes of this patch seem completely unrelated to that. Can we land the changes needed for weston-ternminal and regular xdg-shell clients to work with multiple displays first as that should be much less complicated and then layer arc++ client support on top of that? When everything is done in one patch it's hard for me to understand what is needed and if the current approach is a good long term design.
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.
This CL also fixes two edge cases that affect both ARC and XDG clients,
namely aborting the drag when entering immersive or overview mode.
The new positioning code also fixes pop-up windows for XDG clients
on secondary displays, which was included in a previous CL but
reverted due to a regression in overview mode.
BUG=642894
BUG=631136
TEST=samus: ARC apps can be dragged to external displays using different
display settings, e.g. resolution, layout, orientation.
TEST=weston-terminal: Right-click > Open Terminal on secondary display.
==========
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.
This CL also fixes two edge cases that affect both ARC and XDG clients,
namely aborting the drag when entering immersive or overview mode.
BUG=642894
BUG=631136
TEST=samus,minnie: ARC apps can be dragged to external displays using
different display settings, e.g. resolution, layout, orientation.
==========
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...
On 2016/10/31 03:43:46, reveman wrote: > On 2016/10/26 at 22:19:11, oshima wrote: > > On 2016/10/26 22:10:53, Dominik Laskowski wrote: > > > Ping? Also, it seems like much of the shell surface code will have to be > > > restructured once we migrate to layered Wayland interfaces, so could we > postpone > > > the coordinate conversion refactoring until then? > > > > I believe you're waiting for review from reveman right? > > At this time, with the initial version of ARC++ already having been released, > I'd like to make sure we don't make any rushed design decisions but instead try > to get it right the first time and not make exo code more complicated than it > has to be. For ARC++ I think we should try hard to align with upstream wayland > interfaces as that makes maintaining this code much easier. For window dragging, > that means not having android control that anymore. For virtual screen size > management, that means having it handled as much as possible on the client side. > > The TEST= line for this patch refers to weston-terminal but most of the changes > of this patch seem completely unrelated to that. Can we land the changes needed > for weston-ternminal and regular xdg-shell clients to work with multiple > displays first as that should be much less complicated and then layer arc++ > client support on top of that? When everything is done in one patch it's hard > for me to understand what is needed and if the current approach is a good long > term design. Sorry for the delay. Had to port the Android side to N. PTAL. I've moved virtual coordinate translation from Display to WaylandRemoteShell. Is that more in line with what you had in mind? I believe we've agreed that window dragging will continue to be driven by Android for the time being.
Still a lot going on in this patch. Can you split it into smaller chunks? Everything related to phantom window drawing is something I really like to see in a follow up patch as I consider that just UI improvement that is not needed for the core multi-display functionality.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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.
This CL also fixes two edge cases that affect both ARC and XDG clients,
namely aborting the drag when entering immersive or overview mode.
BUG=642894
BUG=631136
TEST=samus,minnie: ARC apps can be dragged to external displays using
different display settings, e.g. resolution, layout, orientation.
==========
to
==========
exo: Fix dragging edge cases
This CL fixes two edge cases that affect both ARC and XDG clients,
namely aborting the drag when entering immersive or overview mode.
Note that this is only a partial server-side fix. For immersive mode,
there is a client-side race between the drag bounds updates and the
immersive bounds update, which sometimes results in an inconsistent
state where the window is maximized but the content is not. For
overview mode, there is a mouse input bug that resets the cursor's
position to the origin, moving the window along with it.
BUG=642894
BUG=631136
TEST=Entering immersive mode while dragging across displays does not
result in an inconsistent state where the window is full screen
but moving the mouse keeps updating its opacity.
TEST=Entering overview mode while dragging across displays does not
result in a crash.
==========
On 2017/01/19 00:31:45, reveman wrote: > Still a lot going on in this patch. Can you split it into smaller chunks? > Everything related to phantom window drawing is something I really like to see > in a follow up patch as I consider that just UI improvement that is not needed > for the core multi-display functionality. Phantom window drawing can't be split out, because it's done in DragWindowResizer::Drag, without which DragWindowResizer::CompleteDrag can't pick the destination display. As I mentioned above, the edge cases are the only part that can be split out, which I've done in the latest patch.
On 2017/01/19 at 18:38:36, domlaskowski wrote: > On 2017/01/19 00:31:45, reveman wrote: > > Still a lot going on in this patch. Can you split it into smaller chunks? > > Everything related to phantom window drawing is something I really like to see > > in a follow up patch as I consider that just UI improvement that is not needed > > for the core multi-display functionality. > > Phantom window drawing can't be split out, because it's done in DragWindowResizer::Drag, without which DragWindowResizer::CompleteDrag can't pick the destination display. As I mentioned above, the edge cases are the only part that can be split out, which I've done in the latest patch. The WindowResizer code is supposed to be used to drive interactive movement/resize of windows but it seems like we're now planning to use it for client side driven movement. I'm not sure that's a good idea. Drag completion is expected to be controlled by trusted code but we're now moving that into an untrusted client. I think it's very important that an untrusted client can't have chrome enter into a state that the user can't easily escaping from. Two questions: 1. How can we make sure that the user can always leave the interactive move? ie. so it's not possible to create a malicious exo client that prevents it. 2. If we force dragging to end for security reasons, how does the client handle that? https://codereview.chromium.org/2396883003/diff/220001/components/exo/shell_s... File components/exo/shell_surface.cc (right): https://codereview.chromium.org/2396883003/diff/220001/components/exo/shell_s... components/exo/shell_surface.cc:939: EndDragOrMove(false /* revert */); how is other chrome windows handling this? https://codereview.chromium.org/2396883003/diff/220001/components/exo/shell_s... components/exo/shell_surface.cc:1038: EndDragOrMove(false /* revert */); is this also a problem for non-exo window dragging? or are they already doing this? https://codereview.chromium.org/2396883003/diff/220001/components/exo/shell_s... components/exo/shell_surface.cc:1039: ignore_widget_bounds_changes_ = true; why is this needed? seems fragile to rely on us detecting when we enter this specific mode..
I don't think it's possible to prevent this unless we switch to server-side movement. The assumption here is that both sides can detect drag completion independently, and behave accordingly. https://codereview.chromium.org/2396883003/diff/220001/components/exo/shell_s... File components/exo/shell_surface.cc (right): https://codereview.chromium.org/2396883003/diff/220001/components/exo/shell_s... components/exo/shell_surface.cc:939: EndDragOrMove(false /* revert */); On 2017/01/23 20:23:48, reveman wrote: > how is other chrome windows handling this? It's not handled. If you enter full screen after dragging (but not dropping) a Chrome window to another display, its content disappears and the full screen key stops working. https://codereview.chromium.org/2396883003/diff/220001/components/exo/shell_s... components/exo/shell_surface.cc:1038: EndDragOrMove(false /* revert */); On 2017/01/23 20:23:48, reveman wrote: > is this also a problem for non-exo window dragging? or are they already doing > this? Yes, entering/exiting overview mode while dragging is broken for Chrome windows too. You can keep dragging in overview mode, leading to inconsistent state. I have another small patch that does the same for Ash. https://codereview.chromium.org/2396883003/diff/220001/components/exo/shell_s... components/exo/shell_surface.cc:1039: ignore_widget_bounds_changes_ = true; On 2017/01/23 20:23:48, reveman wrote: > why is this needed? seems fragile to rely on us detecting when we enter this > specific mode.. When entering overview mode, a lost focus event causes Android to abort the drag, but it's possible for geometry updates to be in flight at that time. This flag is needed to discard those updates.
On 2017/01/23 at 23:01:36, domlaskowski wrote: > I don't think it's possible to prevent this unless we switch to server-side movement. The assumption here is that both sides can detect drag completion independently, and behave accordingly. What does behave accordingly mean? As discussed, I think we need remote shell clients to either be in control of movement completely (including display placement) or chrome is in control but the client provides contents placement and we have an ack mechanism to prevent a race between contents updates and display changes. https://codereview.chromium.org/2396883003/diff/220001/components/exo/shell_s... File components/exo/shell_surface.cc (right): https://codereview.chromium.org/2396883003/diff/220001/components/exo/shell_s... components/exo/shell_surface.cc:939: EndDragOrMove(false /* revert */); On 2017/01/23 at 23:01:36, Dominik Laskowski wrote: > On 2017/01/23 20:23:48, reveman wrote: > > how is other chrome windows handling this? > > It's not handled. If you enter full screen after dragging (but not dropping) a Chrome window to another display, its content disappears and the full screen key stops working. That sounds like a bug. Is there a more general way to fix this than just making exo/ handle it correctly? https://codereview.chromium.org/2396883003/diff/220001/components/exo/shell_s... components/exo/shell_surface.cc:1038: EndDragOrMove(false /* revert */); On 2017/01/23 at 23:01:36, Dominik Laskowski wrote: > On 2017/01/23 20:23:48, reveman wrote: > > is this also a problem for non-exo window dragging? or are they already doing > > this? > > Yes, entering/exiting overview mode while dragging is broken for Chrome windows too. You can keep dragging in overview mode, leading to inconsistent state. I have another small patch that does the same for Ash. Ok, let's do Ash first and once that lands we'll make a consistent change here. https://codereview.chromium.org/2396883003/diff/220001/components/exo/shell_s... components/exo/shell_surface.cc:1039: ignore_widget_bounds_changes_ = true; On 2017/01/23 at 23:01:36, Dominik Laskowski wrote: > On 2017/01/23 20:23:48, reveman wrote: > > why is this needed? seems fragile to rely on us detecting when we enter this > > specific mode.. > > When entering overview mode, a lost focus event causes Android to abort the drag, but it's possible for geometry updates to be in flight at that time. This flag is needed to discard those updates. Why do they need to be discarded? Doesn't this leave us in a state where chrome is not in sync with the android state?
Abandoning this CL. As discussed offline, the non-hacky solution is to synchronize window state transitions with the client, and abort client-side dragging on transition to immersive or overview mode. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
