exo: Synchronize multi-display window positioning

exo: Synchronize multi-display window positioning

This CL enables the compositor to synchronize window positions across
display configuration changes when bounds are controlled by the client.
In that case, the client uses screen coordinates for geometry updates,
and acknowledges when the compositor changes the origin of the screen
coordinate system. The compositor compensates for in-flight geometry
updates until it receives the acknowledgement, at which point it
switches to the new coordinate system.

For ARC 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.

Note that ShellSurface::SetOrigin will be used in a follow-up CL to
implement support for multiple displays in ARC.

BUG=642894
TEST=samus: Window positions are updated atomically when changing the
     display configuration, e.g. closing the lid to enter docked mode.

Review-Url: https://codereview.chromium.org/2706543002
Cr-Commit-Position: refs/heads/master@{#452913}
Committed: https://chromium.googlesource.com/chromium/src/+/908ff1bf9511fe0a66bea48737a898ca71c81440

[Dominik Laskowski, 2017-02-17 21:35:29]

The CQ bit was checked by domlaskowski@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-17 21:36:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Dominik Laskowski, 2017-02-17 21:40:40]

domlaskowski@chromium.org changed reviewers:
+ oshima@chromium.org, reveman@chromium.org

[Dominik Laskowski, 2017-02-17 21:40:40]

PTAL. Split from https://codereview.chromium.org/2645663004/

[commit-bot: I haz the power, 2017-02-17 23:39:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-17 23:39:53]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)

[reveman, 2017-02-18 00:32:39]

https://codereview.chromium.org/2706543002/diff/1/components/exo/shell_surfac...
File components/exo/shell_surface.cc (right):

https://codereview.chromium.org/2706543002/diff/1/components/exo/shell_surfac...
components/exo/shell_surface.cc:634: const gfx::Point old_origin = origin_;
nit: exo and chromium code typically doesn't use const for local variables
unless it's compile time constant

https://codereview.chromium.org/2706543002/diff/1/components/exo/shell_surfac...
components/exo/shell_surface.cc:637: if (bounds_mode_ != BoundsMode::CLIENT ||
origin == old_origin)
nit: can you move the "origin == old_origin" check above as "(origin == origin_)
return" and remove the need for the old_origin variable?

https://codereview.chromium.org/2706543002/diff/1/components/exo/shell_surfac...
components/exo/shell_surface.cc:931: if (bounds_mode_ == BoundsMode::CLIENT ||
!widget_ || !surface_ ||
nit: is this needed as part of this patch? in that case at least move
"bounds_mode_ == BoundsMode::CLIENT" into a separate check? I prefer that as I
think it's cleaner and not sure we should be doing this long term

https://codereview.chromium.org/2706543002/diff/1/components/exo/shell_surfac...
components/exo/shell_surface.cc:1323: // the surface with the widget. Note that
the widget position must have been
hm, I'd expect CLIENT bounds to behave just like SHELL when in TOPLEFT resize
mode. See comment below.

https://codereview.chromium.org/2706543002/diff/1/components/exo/shell_surfac...
components/exo/shell_surface.cc:1387: // Position is relative to the latest
origin acknowledged by the client.
Can we have this behave as the SHELL when "resize_component_ != HTCAPTION"?
That's how I expected this to work with minimal extra logic for the CLIENT case.

[Dominik Laskowski, 2017-02-23 03:39:00]

The CQ bit was checked by domlaskowski@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-23 03:39:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Dominik Laskowski, 2017-02-23 03:43:48]

oshima: PTAL at the latest patch, which fixes a shadow regression after
rebasing.

reveman: FYI, I've noticed a regression for XDG clients. The initial position of
non-popup windows is incorrect. I haven't looked into it yet, but I've verified
that the multi-display CLs are innocent.

https://codereview.chromium.org/2706543002/diff/1/components/exo/shell_surfac...
File components/exo/shell_surface.cc (right):

https://codereview.chromium.org/2706543002/diff/1/components/exo/shell_surfac...
components/exo/shell_surface.cc:634: const gfx::Point old_origin = origin_;
On 2017/02/18 00:32:39, reveman wrote:
> nit: exo and chromium code typically doesn't use const for local variables
> unless it's compile time constant

Done.

https://codereview.chromium.org/2706543002/diff/1/components/exo/shell_surfac...
components/exo/shell_surface.cc:637: if (bounds_mode_ != BoundsMode::CLIENT ||
origin == old_origin)
On 2017/02/18 00:32:39, reveman wrote:
> nit: can you move the "origin == old_origin" check above as "(origin ==
origin_)
> return" and remove the need for the old_origin variable?

Done.

https://codereview.chromium.org/2706543002/diff/1/components/exo/shell_surfac...
components/exo/shell_surface.cc:931: if (bounds_mode_ == BoundsMode::CLIENT ||
!widget_ || !surface_ ||
On 2017/02/18 00:32:39, reveman wrote:
> nit: is this needed as part of this patch? in that case at least move
> "bounds_mode_ == BoundsMode::CLIENT" into a separate check? I prefer that as I
> think it's cleaner and not sure we should be doing this long term

Yes, but it will be removed once resizing is driven by the server. Until then,
the origin offset should not be modified when the window bounds change.

Moved into a separate check and added a comment.

https://codereview.chromium.org/2706543002/diff/1/components/exo/shell_surfac...
components/exo/shell_surface.cc:1323: // the surface with the widget. Note that
the widget position must have been
On 2017/02/18 00:32:39, reveman wrote:
> hm, I'd expect CLIENT bounds to behave just like SHELL when in TOPLEFT resize
> mode. See comment below.

I've removed the special case below to avoid depending on widget state.

https://codereview.chromium.org/2706543002/diff/1/components/exo/shell_surfac...
components/exo/shell_surface.cc:1387: // Position is relative to the latest
origin acknowledged by the client.
On 2017/02/18 00:32:39, reveman wrote:
> Can we have this behave as the SHELL when "resize_component_ != HTCAPTION"?
> That's how I expected this to work with minimal extra logic for the CLIENT
case.

I've simplified this a bit, but I don't think unifying these cases is possible
because the meaning of origin/geometry is different.

[commit-bot: I haz the power, 2017-02-23 04:11:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-23 04:11:29]

Dry run: This issue passed the CQ dry run.

[reveman, 2017-02-24 16:50:01]

lgtm with minor style nit

https://codereview.chromium.org/2706543002/diff/20001/components/exo/shell_su...
File components/exo/shell_surface.cc (right):

https://codereview.chromium.org/2706543002/diff/20001/components/exo/shell_su...
components/exo/shell_surface.cc:1336: origin_offset_ -
visible_bounds.OffsetFromOrigin();
nit: split into multiple lines to make it easier to read. maybe just add:

if (bounds_mode_ == BoundsMode::CLIENT)
  return origin_ + origin_offset_ - visible_bounds.OffsetFromOrigin();

above existing line 1308 as an early out

[Dominik Laskowski, 2017-02-24 18:51:30]

https://codereview.chromium.org/2706543002/diff/20001/components/exo/shell_su...
File components/exo/shell_surface.cc (right):

https://codereview.chromium.org/2706543002/diff/20001/components/exo/shell_su...
components/exo/shell_surface.cc:1336: origin_offset_ -
visible_bounds.OffsetFromOrigin();
On 2017/02/24 16:50:00, reveman wrote:
> nit: split into multiple lines to make it easier to read. maybe just add:
> 
> if (bounds_mode_ == BoundsMode::CLIENT)
>   return origin_ + origin_offset_ - visible_bounds.OffsetFromOrigin();
> 
> above existing line 1308 as an early out

Done.

[Dominik Laskowski, 2017-02-24 19:10:47]

The CQ bit was checked by domlaskowski@chromium.org

[Dominik Laskowski, 2017-02-24 19:10:47]

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/2706543002/#ps40001
(title: "Address nit")

[commit-bot: I haz the power, 2017-02-24 19:11:23]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-24 21:07:47]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1487963447143970,
"parent_rev": "b87e5b0b9505543cc4270479af08e60da11cd507", "commit_rev":
"908ff1bf9511fe0a66bea48737a898ca71c81440"}

[commit-bot: I haz the power, 2017-02-24 21:08:48]

Description was changed from

==========
exo: Synchronize multi-display window positioning

This CL enables the compositor to synchronize window positions across
display configuration changes when bounds are controlled by the client.
In that case, the client uses screen coordinates for geometry updates,
and acknowledges when the compositor changes the origin of the screen
coordinate system. The compositor compensates for in-flight geometry
updates until it receives the acknowledgement, at which point it
switches to the new coordinate system.

For ARC 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.

Note that ShellSurface::SetOrigin will be used in a follow-up CL to
implement support for multiple displays in ARC.

BUG=642894
TEST=samus: Window positions are updated atomically when changing the
     display configuration, e.g. closing the lid to enter docked mode.
==========

to

==========
exo: Synchronize multi-display window positioning

This CL enables the compositor to synchronize window positions across
display configuration changes when bounds are controlled by the client.
In that case, the client uses screen coordinates for geometry updates,
and acknowledges when the compositor changes the origin of the screen
coordinate system. The compositor compensates for in-flight geometry
updates until it receives the acknowledgement, at which point it
switches to the new coordinate system.

For ARC 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.

Note that ShellSurface::SetOrigin will be used in a follow-up CL to
implement support for multiple displays in ARC.

BUG=642894
TEST=samus: Window positions are updated atomically when changing the
     display configuration, e.g. closing the lid to enter docked mode.

Review-Url: https://codereview.chromium.org/2706543002
Cr-Commit-Position: refs/heads/master@{#452913}
Committed:
https://chromium.googlesource.com/chromium/src/+/908ff1bf9511fe0a66bea48737a8...
==========

[commit-bot: I haz the power, 2017-02-24 21:08:49]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/908ff1bf9511fe0a66bea48737a8...