New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 821987 link

Starred by 1 user
Status: Available
Owner: ----
Cc:
█ fsam...@chromium.org
sadrul@chromium.org
kylec...@chromium.org
sky@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows
Pri: 3
Type: Bug



Sign in to add a comment

aura: WindowTreeHost bounds are inconsistent

Project Member Reported by ccameron@chromium.org, Mar 14 2018 
There exist several mechanisms for updating a WindowTreeHost's bounds
- OnHostMoved/ResizedInPixels
  - gives a message that passes the new bounds
- GetBounds()
  - calls into the sub-class and returns something
  - this may or may not match OnHostMoved/ResizedInPixels
- UpdateRootWindowSizeInPixels
  - this usually given GetBounds as an argument
  - except when called by RootWindowController, where its goal is unclear

This makes it impossible to reason precisely about what is meant by "bounds" for an aura::WindowTreeHost, and makes maintenance of the code very difficult.

I have spent several days trying to introduce CHECKs on LayerTreeHost surface invariants, and my inability to coherently navigate this code has been the primary culprit.

Ideally this should be changed so that
- OnHostMoved/ResizedInPixels
  - is the only way that changes in bounds may be propagated
- GetBounds
  - always returns value most recently sent to OnHostMoved/ResizedInPixels
- UpdateRootWindowSizeInPixels
  - shouldn't exist as a public method

Comment 1 by kylec...@chromium.org, Mar 22 2018

Cc: kylec...@chromium.org
Project Member

Comment 2 by bugdroid1@chromium.org, Apr 11 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/92bbab9c047f50805b8eb57d5526d73e3379b0b1

commit 92bbab9c047f50805b8eb57d5526d73e3379b0b1
Author: Sadrul Habib Chowdhury <sadrul@chromium.org>
Date: Wed Apr 11 19:23:20 2018

aura: de-virtualize WindowTreeHost::UpdateRootWindowSizeInPixels().

De-virtualize UpdateRootWindowSizeInPixels(), and make sure the root-window's
bounds/scale are set only by WindowTreeHost. To allow ash to override how it
applies the transforms, introduce GetTransformedRootWindowBoundsInPixels()
as a const method. This makes it easier to reason about how LocalSurfaceIds
are allocated for a WindowTreeHost (or its root window).

BUG=821987

Change-Id: I61f4fd54dbe2ec0034fdb1697deb2b4abd65a41c
Reviewed-on: https://chromium-review.googlesource.com/1005566
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Daniel Nicoara <dnicoara@chromium.org>
Commit-Queue: Sadrul Chowdhury <sadrul@chromium.org>
Cr-Commit-Position: refs/heads/master@{#549941}
[modify] https://crrev.com/92bbab9c047f50805b8eb57d5526d73e3379b0b1/ash/host/ash_window_tree_host_mus.cc
[modify] https://crrev.com/92bbab9c047f50805b8eb57d5526d73e3379b0b1/ash/host/ash_window_tree_host_mus.h
[modify] https://crrev.com/92bbab9c047f50805b8eb57d5526d73e3379b0b1/ash/host/ash_window_tree_host_platform.cc
[modify] https://crrev.com/92bbab9c047f50805b8eb57d5526d73e3379b0b1/ash/host/ash_window_tree_host_platform.h
[modify] https://crrev.com/92bbab9c047f50805b8eb57d5526d73e3379b0b1/ash/host/transformer_helper.cc
[modify] https://crrev.com/92bbab9c047f50805b8eb57d5526d73e3379b0b1/ash/host/transformer_helper.h
[modify] https://crrev.com/92bbab9c047f50805b8eb57d5526d73e3379b0b1/chromecast/graphics/cast_window_manager_aura.cc
[modify] https://crrev.com/92bbab9c047f50805b8eb57d5526d73e3379b0b1/ui/aura/window_tree_host.cc
[modify] https://crrev.com/92bbab9c047f50805b8eb57d5526d73e3379b0b1/ui/aura/window_tree_host.h
Project Member

Comment 3 by bugdroid1@chromium.org, Apr 12 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f1d68ed3d35afb2b3275b6dda216fb2d13e21620

commit f1d68ed3d35afb2b3275b6dda216fb2d13e21620
Author: Sadrul Habib Chowdhury <sadrul@chromium.org>
Date: Thu Apr 12 05:51:17 2018

views/win: Re-enable Display swap without touching cc.

Reset the size of the viz::Display directly to the original size, instead
of triggering a size-change in the layer-compositor (cc) too, when a resize
operation ends without actually resizing the window on Windows.

BUG=821987

Change-Id: I97c6e76fd52b46d782a172fc54bb7cf9a0e0a1d6
Reviewed-on: https://chromium-review.googlesource.com/1006518
Reviewed-by: Antoine Labour <piman@chromium.org>
Commit-Queue: Sadrul Chowdhury <sadrul@chromium.org>
Cr-Commit-Position: refs/heads/master@{#550031}
[modify] https://crrev.com/f1d68ed3d35afb2b3275b6dda216fb2d13e21620/ui/compositor/compositor.cc
[modify] https://crrev.com/f1d68ed3d35afb2b3275b6dda216fb2d13e21620/ui/compositor/compositor.h
[modify] https://crrev.com/f1d68ed3d35afb2b3275b6dda216fb2d13e21620/ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc
Project Member

Comment 4 by bugdroid1@chromium.org, Apr 12 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a824472ce809c4435cf42f460d893751708caf73

commit a824472ce809c4435cf42f460d893751708caf73
Author: Sadrul Habib Chowdhury <sadrul@chromium.org>
Date: Thu Apr 12 06:01:01 2018

aura: Update UpdateRootWindowSizeInPixels() not take any input.

UpdateRootWindowSizeInPixels() should always use the pixel-bounds it
already knows about. So it does not need to take any input params. Fix
extension-shell to use SetBoundsInPixels() instead, because that's the
correct way to set the size of the host (which would automatically
update the size of the root-window etc.).

BUG=821987

Change-Id: I31d0e5fa21471192aab5c462f79a1b1906ec23ac
Reviewed-on: https://chromium-review.googlesource.com/1006491
Commit-Queue: Sadrul Chowdhury <sadrul@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/master@{#550037}
[modify] https://crrev.com/a824472ce809c4435cf42f460d893751708caf73/ash/host/transformer_helper.cc
[modify] https://crrev.com/a824472ce809c4435cf42f460d893751708caf73/extensions/shell/browser/root_window_controller.cc
[modify] https://crrev.com/a824472ce809c4435cf42f460d893751708caf73/ui/aura/window_tree_host.cc
[modify] https://crrev.com/a824472ce809c4435cf42f460d893751708caf73/ui/aura/window_tree_host.h
Project Member

Comment 5 by bugdroid1@chromium.org, Apr 17 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/effe28870c6d2559dd0b2a2eb1f2efd372c1678d

commit effe28870c6d2559dd0b2a2eb1f2efd372c1678d
Author: Sadrul Habib Chowdhury <sadrul@chromium.org>
Date: Tue Apr 17 00:19:34 2018

aura: Special-case allocation of LocalSurfaceId for root.

A LocalSurfaceId allocation/renewal for an aura::Window happens in
response to a bounds or dsf-change to its corresponding ui::Layer.
The root window is special in this regard, since its state can be
changed only by the WindowTreeHost, and should happen before the
ui::Compositor is updated for the new bounds/dsf (since it needs
to provide the LocalSurfaceId to the ui::Compositor).

Window::SetDeviceScaleFactor() is no longer needed after this. So
remove that.

BUG=821987

Change-Id: I3eb674b1c59a2728315125b6eb14b610033f6bf4
Reviewed-on: https://chromium-review.googlesource.com/1012932
Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Sadrul Chowdhury <sadrul@chromium.org>
Cr-Commit-Position: refs/heads/master@{#551180}
[modify] https://crrev.com/effe28870c6d2559dd0b2a2eb1f2efd372c1678d/ui/aura/local/window_port_local.cc
[modify] https://crrev.com/effe28870c6d2559dd0b2a2eb1f2efd372c1678d/ui/aura/mus/window_port_mus.cc
[modify] https://crrev.com/effe28870c6d2559dd0b2a2eb1f2efd372c1678d/ui/aura/mus/window_tree_client.cc
[modify] https://crrev.com/effe28870c6d2559dd0b2a2eb1f2efd372c1678d/ui/aura/window.cc
[modify] https://crrev.com/effe28870c6d2559dd0b2a2eb1f2efd372c1678d/ui/aura/window.h
[modify] https://crrev.com/effe28870c6d2559dd0b2a2eb1f2efd372c1678d/ui/aura/window_tree_host.cc
Project Member

Comment 6 by bugdroid1@chromium.org, Apr 18 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3c7fdca95ba99cc9abc7cf10b1a77e339dc4ebcb

commit 3c7fdca95ba99cc9abc7cf10b1a77e339dc4ebcb
Author: Sadrul Habib Chowdhury <sadrul@chromium.org>
Date: Wed Apr 18 05:08:40 2018

aura: Sanitize LocalSurfaceId allocation for WindowTreeHost.

Notable changes:
. Use the regular aura-path for setting the LocalSurfaceId for the
  compositor, instead of specially setting it from WindowTreeClient,
  when the window-server sets the LocalSurfaceId with a bounds-change.
  WindowTreeHost::SetBoundsInPixels() is changed to take LocalSurfaceId
  as an input-param to facilitate this.
. When the client itself changes the size, do not set the LocalSurfaceId
  on the compositor multiple times for the same size from various places
  (e.g. WindowTreeHostMus, WindowTreeClient etc.).
. Add validation checks for ui::Compositor that a new LocalSurfaceId is
  assigned whenever the size changes. This makes it easier to catch
  errors earlier in the pipeline.

BUG=821987

Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel
Change-Id: Ic9703c57b7a7f9ef2b65b34fa38c8ec7de9e9f36
Reviewed-on: https://chromium-review.googlesource.com/1014568
Commit-Queue: Sadrul Chowdhury <sadrul@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/master@{#551588}
[modify] https://crrev.com/3c7fdca95ba99cc9abc7cf10b1a77e339dc4ebcb/ash/host/ash_window_tree_host_mus_unified.cc
[modify] https://crrev.com/3c7fdca95ba99cc9abc7cf10b1a77e339dc4ebcb/ash/host/ash_window_tree_host_mus_unified.h
[modify] https://crrev.com/3c7fdca95ba99cc9abc7cf10b1a77e339dc4ebcb/ash/host/ash_window_tree_host_platform.cc
[modify] https://crrev.com/3c7fdca95ba99cc9abc7cf10b1a77e339dc4ebcb/ash/host/ash_window_tree_host_platform.h
[modify] https://crrev.com/3c7fdca95ba99cc9abc7cf10b1a77e339dc4ebcb/ash/host/ash_window_tree_host_unified.cc
[modify] https://crrev.com/3c7fdca95ba99cc9abc7cf10b1a77e339dc4ebcb/ash/host/ash_window_tree_host_unified.h
[modify] https://crrev.com/3c7fdca95ba99cc9abc7cf10b1a77e339dc4ebcb/components/viz/client/client_layer_tree_frame_sink.cc
[modify] https://crrev.com/3c7fdca95ba99cc9abc7cf10b1a77e339dc4ebcb/headless/lib/browser/headless_browser_impl_aura.cc
[modify] https://crrev.com/3c7fdca95ba99cc9abc7cf10b1a77e339dc4ebcb/headless/lib/browser/headless_window_tree_host.cc
[modify] https://crrev.com/3c7fdca95ba99cc9abc7cf10b1a77e339dc4ebcb/headless/lib/browser/headless_window_tree_host.h
[modify] https://crrev.com/3c7fdca95ba99cc9abc7cf10b1a77e339dc4ebcb/ui/aura/mus/window_port_mus.h
[modify] https://crrev.com/3c7fdca95ba99cc9abc7cf10b1a77e339dc4ebcb/ui/aura/mus/window_tree_client.cc
[modify] https://crrev.com/3c7fdca95ba99cc9abc7cf10b1a77e339dc4ebcb/ui/aura/mus/window_tree_host_mus.cc
[modify] https://crrev.com/3c7fdca95ba99cc9abc7cf10b1a77e339dc4ebcb/ui/aura/mus/window_tree_host_mus.h
[modify] https://crrev.com/3c7fdca95ba99cc9abc7cf10b1a77e339dc4ebcb/ui/aura/window_tree_host.cc
[modify] https://crrev.com/3c7fdca95ba99cc9abc7cf10b1a77e339dc4ebcb/ui/aura/window_tree_host.h
[modify] https://crrev.com/3c7fdca95ba99cc9abc7cf10b1a77e339dc4ebcb/ui/aura/window_tree_host_platform.cc
[modify] https://crrev.com/3c7fdca95ba99cc9abc7cf10b1a77e339dc4ebcb/ui/aura/window_tree_host_platform.h
[modify] https://crrev.com/3c7fdca95ba99cc9abc7cf10b1a77e339dc4ebcb/ui/compositor/compositor.cc
[modify] https://crrev.com/3c7fdca95ba99cc9abc7cf10b1a77e339dc4ebcb/ui/views/mus/desktop_window_tree_host_mus.cc
[modify] https://crrev.com/3c7fdca95ba99cc9abc7cf10b1a77e339dc4ebcb/ui/views/mus/desktop_window_tree_host_mus.h
[modify] https://crrev.com/3c7fdca95ba99cc9abc7cf10b1a77e339dc4ebcb/ui/views/widget/desktop_aura/desktop_window_tree_host_platform.cc
[modify] https://crrev.com/3c7fdca95ba99cc9abc7cf10b1a77e339dc4ebcb/ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc
[modify] https://crrev.com/3c7fdca95ba99cc9abc7cf10b1a77e339dc4ebcb/ui/views/widget/desktop_aura/desktop_window_tree_host_win.h
[modify] https://crrev.com/3c7fdca95ba99cc9abc7cf10b1a77e339dc4ebcb/ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc
[modify] https://crrev.com/3c7fdca95ba99cc9abc7cf10b1a77e339dc4ebcb/ui/views/widget/desktop_aura/desktop_window_tree_host_x11.h

Comment 7 by ccameron@chromium.org, Jun 28 2018

Owner: ----
Status: Available (was: Assigned)
Marking available, if anyone wants to grab it. Otherwise feel free to close -- we've fixed the surface invariants issues.
►

Sign in to add a comment