New issue
Advanced search Search tips

Issue 901555 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Nov 30
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 1
Type: Bug

Blocking:
issue 905388
issue 897387



Sign in to add a comment

Link mirror view's occlusion state to its target window

Project Member Reported by malaykeshav@chromium.org, Nov 2

Issue description

aura::Windows with mirror views have their visibility state permanently set to VISIBLE. This means the target window is set to VISIBLE even if the mirror window is HIDDEN. 

We want to link the mirror window's occlusion state to the target window's to have consistency.

Without this the window's webcontent is re rasterized as soon as a mirror window is attached(Since the occlusion state becomes visible) ignoring the occlusion tracker. This blocks any animation performance improvement that relies on the occlusion tracker to pause re rasterization of the web content.
 
This also introduces an issue of what happens when the mirror window is removed from the target window. From the target window's perspective a window property would be updated. For a WebContentsView this would result in the recomputation  of the web content visibility. Since the target window will most likely be HIDDEN (which is why we are using mirror windows), the web content would be evicted. This code path ignores the pause state of the Occlusion Tracker thus not allowing any control over when the content should be evicted based on occlusion state. Content eviction due to occlusion state change should always be triggered by the occlusion tracker. To enforce this, any other code path that may trigger content eviction related to occlusion should be removed. But it also means there needs to be an API to mark a certain window as dirty in the occlusion tracker.
Sounds like that we should make WindowOccusionTracker to handle the kMirroringEnabledKey property change so that the change triggers occlusion state change under the control of the tracker and is subject to the pause.

For linking the state, WindowMirrorView is a view::View, which does not have an occlusion state. We probably could use View::OnVisibleBoundsChanged + hosting widget's occlusion state to get something. Ideally, this should be part of the occlusion tracker. But not sure how easy/cleanly to integrate the logic in.
I do have a change I have been working on for the past week that handles #0  and is different from the approach in #2. Instead of setting kMirroringEnableKey, I set the occlusion state of the mirror view's widget as a target window's property.
https://chromium-review.googlesource.com/c/chromium/src/+/1313753

However I also like the idea of WindowOcclusionTracker handling this. While updating the occlusion state of a window, it should check the occlusion state of its mirror view's widget. That would also handle #1.
Blocking: 905388
Project Member

Comment 5 by bugdroid1@chromium.org, Nov 30

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

commit e70dc7a2d7b9ff7b89cd2537d65476dbf8087802
Author: Malay Keshav <malaykeshav@chromium.org>
Date: Fri Nov 30 00:15:06 2018

Use mirror window occlusion state to update webcontent visibility

This patch makes WebContentsViewAura manage its web content's visibility
state based on its current mirror window's occlusion state. To achieve
this, the patch makes the following changes:
 - Adds a new callback in WindowObserver to receive occlusion state
   changes.
 - Creates a subclass of aura::WindowObserver to manage mirror window
   occlusion states for the WebContentsViewAura.
 - Replaces a DCHECK with an early return when an already tracked window
   is requested to be tracked again.
 - Replaces the boolean window property of mirror window with a vector
   list of mirror windows.
 - Updates the unittest and adds a new method to WindowTestApi.

Bug:  901555 
Change-Id: I70566c544def6129c7410af84f91e6501a5479d5
Component: Window Mirror View, web contents, Window occlusion tracker
Reviewed-on: https://chromium-review.googlesource.com/c/1313753
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: François Doray <fdoray@chromium.org>
Commit-Queue: Malay Keshav <malaykeshav@chromium.org>
Cr-Commit-Position: refs/heads/master@{#612457}
[modify] https://crrev.com/e70dc7a2d7b9ff7b89cd2537d65476dbf8087802/ash/wm/window_mirror_view.cc
[modify] https://crrev.com/e70dc7a2d7b9ff7b89cd2537d65476dbf8087802/ash/wm/window_mirror_view.h
[modify] https://crrev.com/e70dc7a2d7b9ff7b89cd2537d65476dbf8087802/content/browser/web_contents/web_contents_view_aura.cc
[modify] https://crrev.com/e70dc7a2d7b9ff7b89cd2537d65476dbf8087802/content/browser/web_contents/web_contents_view_aura.h
[modify] https://crrev.com/e70dc7a2d7b9ff7b89cd2537d65476dbf8087802/content/browser/web_contents/web_contents_view_aura_unittest.cc
[modify] https://crrev.com/e70dc7a2d7b9ff7b89cd2537d65476dbf8087802/ui/aura/client/aura_constants.cc
[modify] https://crrev.com/e70dc7a2d7b9ff7b89cd2537d65476dbf8087802/ui/aura/client/aura_constants.h
[modify] https://crrev.com/e70dc7a2d7b9ff7b89cd2537d65476dbf8087802/ui/aura/test/window_test_api.cc
[modify] https://crrev.com/e70dc7a2d7b9ff7b89cd2537d65476dbf8087802/ui/aura/test/window_test_api.h
[modify] https://crrev.com/e70dc7a2d7b9ff7b89cd2537d65476dbf8087802/ui/aura/window.cc
[modify] https://crrev.com/e70dc7a2d7b9ff7b89cd2537d65476dbf8087802/ui/aura/window_observer.h
[modify] https://crrev.com/e70dc7a2d7b9ff7b89cd2537d65476dbf8087802/ui/aura/window_occlusion_tracker.cc

Status: Fixed (was: Assigned)

Sign in to add a comment