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

Issue 898222 link

Starred by 0 users

Issue metadata

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

Blocking:
issue 897378



Sign in to add a comment

Introduce OnOverviewStartAnimationComplete observer method.

Project Member Reported by sammiequon@chromium.org, Oct 23

Issue description

This is to remove unneeded animations while transitioning into overview mode. The logic used to determine when animations are done will also be used by overview mode to apply mask and shadow and maybe others.

The first usage will be to animate the overview tray button after window animation.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Oct 26

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

commit 77cbc4f0fa1c446950859be5859071b9689208ae
Author: Sammie Quon <sammiequon@google.com>
Date: Fri Oct 26 00:12:42 2018

overview: Introduce OnOverviewStartAnimationComplete observer.

Introdue a new overview observer which notifies observers when the start
animation for overview is different.

Uses will be both in and out of overview - for example adding mask, shadow
and animating tray, shelf.

Also modify CleanupObserverTest to use some new stuff.

Test: ash_unittests StartAnimationObserverTest.Basic
Bug:  898222 
Change-Id: I86bdd253f0a4a4dd81ad6ba03780baf07923d256
Reviewed-on: https://chromium-review.googlesource.com/c/1297405
Commit-Queue: Sammie Quon <sammiequon@chromium.org>
Reviewed-by: Mitsuru Oshima <oshima@chromium.org>
Cr-Commit-Position: refs/heads/master@{#602941}
[modify] https://crrev.com/77cbc4f0fa1c446950859be5859071b9689208ae/ash/BUILD.gn
[modify] https://crrev.com/77cbc4f0fa1c446950859be5859071b9689208ae/ash/app_list/app_list_controller_impl.cc
[modify] https://crrev.com/77cbc4f0fa1c446950859be5859071b9689208ae/ash/app_list/app_list_controller_impl.h
[modify] https://crrev.com/77cbc4f0fa1c446950859be5859071b9689208ae/ash/shell.cc
[modify] https://crrev.com/77cbc4f0fa1c446950859be5859071b9689208ae/ash/shell.h
[modify] https://crrev.com/77cbc4f0fa1c446950859be5859071b9689208ae/ash/shell_observer.h
[modify] https://crrev.com/77cbc4f0fa1c446950859be5859071b9689208ae/ash/wm/overview/cleanup_animation_observer.h
[modify] https://crrev.com/77cbc4f0fa1c446950859be5859071b9689208ae/ash/wm/overview/cleanup_animation_observer_unittest.cc
[modify] https://crrev.com/77cbc4f0fa1c446950859be5859071b9689208ae/ash/wm/overview/scoped_transform_overview_window.cc
[add] https://crrev.com/77cbc4f0fa1c446950859be5859071b9689208ae/ash/wm/overview/start_animation_observer.cc
[add] https://crrev.com/77cbc4f0fa1c446950859be5859071b9689208ae/ash/wm/overview/start_animation_observer.h
[add] https://crrev.com/77cbc4f0fa1c446950859be5859071b9689208ae/ash/wm/overview/start_animation_observer_unittest.cc
[modify] https://crrev.com/77cbc4f0fa1c446950859be5859071b9689208ae/ash/wm/overview/window_selector_controller.cc
[modify] https://crrev.com/77cbc4f0fa1c446950859be5859071b9689208ae/ash/wm/overview/window_selector_controller.h
[modify] https://crrev.com/77cbc4f0fa1c446950859be5859071b9689208ae/ash/wm/overview/window_selector_delegate.h
[modify] https://crrev.com/77cbc4f0fa1c446950859be5859071b9689208ae/ash/wm/overview/window_selector_item.cc
[modify] https://crrev.com/77cbc4f0fa1c446950859be5859071b9689208ae/ash/wm/overview/window_selector_unittest.cc

Labels: Merge-Request-71
Project Member

Comment 3 by bugdroid1@chromium.org, Oct 26

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

commit aa89084324fd9fc9d3d665bf7fefa2863bff8089
Author: Takashi Sakamoto <tasak@google.com>
Date: Fri Oct 26 02:10:35 2018

Revert "overview: Introduce OnOverviewStartAnimationComplete observer."

This reverts commit 77cbc4f0fa1c446950859be5859071b9689208ae.

Reason for revert:
Suspect compile failure on linux-chromeos-dbg.

Example:
https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/linux-chromeos-dbg/8511

Log:
FAILED: ash_unittests 
../../buildtools/third_party/libc++/trunk/include/memory:5091: error: undefined reference to 'ash::StartAnimationObserver::StartAnimationObserver()'
clang: error: linker command failed with exit code 1 (use -v to see invocation)

Original change's description:
> overview: Introduce OnOverviewStartAnimationComplete observer.
> 
> Introdue a new overview observer which notifies observers when the start
> animation for overview is different.
> 
> Uses will be both in and out of overview - for example adding mask, shadow
> and animating tray, shelf.
> 
> Also modify CleanupObserverTest to use some new stuff.
> 
> Test: ash_unittests StartAnimationObserverTest.Basic
> Bug:  898222 
> Change-Id: I86bdd253f0a4a4dd81ad6ba03780baf07923d256
> Reviewed-on: https://chromium-review.googlesource.com/c/1297405
> Commit-Queue: Sammie Quon <sammiequon@chromium.org>
> Reviewed-by: Mitsuru Oshima <oshima@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#602941}

TBR=oshima@chromium.org,sammiequon@chromium.org

Change-Id: I5038dbe64ae02fb0f71a7c4ffcc36bcfaaca13ca
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  898222 
Reviewed-on: https://chromium-review.googlesource.com/c/1300815
Reviewed-by: Takashi Sakamoto <tasak@google.com>
Commit-Queue: Takashi Sakamoto <tasak@google.com>
Cr-Commit-Position: refs/heads/master@{#602972}
[modify] https://crrev.com/aa89084324fd9fc9d3d665bf7fefa2863bff8089/ash/BUILD.gn
[modify] https://crrev.com/aa89084324fd9fc9d3d665bf7fefa2863bff8089/ash/app_list/app_list_controller_impl.cc
[modify] https://crrev.com/aa89084324fd9fc9d3d665bf7fefa2863bff8089/ash/app_list/app_list_controller_impl.h
[modify] https://crrev.com/aa89084324fd9fc9d3d665bf7fefa2863bff8089/ash/shell.cc
[modify] https://crrev.com/aa89084324fd9fc9d3d665bf7fefa2863bff8089/ash/shell.h
[modify] https://crrev.com/aa89084324fd9fc9d3d665bf7fefa2863bff8089/ash/shell_observer.h
[modify] https://crrev.com/aa89084324fd9fc9d3d665bf7fefa2863bff8089/ash/wm/overview/cleanup_animation_observer.h
[modify] https://crrev.com/aa89084324fd9fc9d3d665bf7fefa2863bff8089/ash/wm/overview/cleanup_animation_observer_unittest.cc
[modify] https://crrev.com/aa89084324fd9fc9d3d665bf7fefa2863bff8089/ash/wm/overview/scoped_transform_overview_window.cc
[delete] https://crrev.com/bf48cde887e7610b35b09dc38a9594dba992287c/ash/wm/overview/start_animation_observer.cc
[delete] https://crrev.com/bf48cde887e7610b35b09dc38a9594dba992287c/ash/wm/overview/start_animation_observer.h
[delete] https://crrev.com/bf48cde887e7610b35b09dc38a9594dba992287c/ash/wm/overview/start_animation_observer_unittest.cc
[modify] https://crrev.com/aa89084324fd9fc9d3d665bf7fefa2863bff8089/ash/wm/overview/window_selector_controller.cc
[modify] https://crrev.com/aa89084324fd9fc9d3d665bf7fefa2863bff8089/ash/wm/overview/window_selector_controller.h
[modify] https://crrev.com/aa89084324fd9fc9d3d665bf7fefa2863bff8089/ash/wm/overview/window_selector_delegate.h
[modify] https://crrev.com/aa89084324fd9fc9d3d665bf7fefa2863bff8089/ash/wm/overview/window_selector_item.cc
[modify] https://crrev.com/aa89084324fd9fc9d3d665bf7fefa2863bff8089/ash/wm/overview/window_selector_unittest.cc

Project Member

Comment 4 by bugdroid1@chromium.org, Oct 26

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

commit be1860675191a799acb2e758f2ead655ec5803a0
Author: Sammie Quon <sammiequon@google.com>
Date: Fri Oct 26 06:40:34 2018

Reland "overview: Introduce OnOverviewStartAnimationComplete observer."

This is a reland of 77cbc4f0fa1c446950859be5859071b9689208ae.
Added a missing ASH_EXPORT.

TBR=oshima@chromium.org

Original change's description:
> overview: Introduce OnOverviewStartAnimationComplete observer.
>
> Introdue a new overview observer which notifies observers when the start
> animation for overview is different.
>
> Uses will be both in and out of overview - for example adding mask, shadow
> and animating tray, shelf.
>
> Also modify CleanupObserverTest to use some new stuff.
>
> Test: ash_unittests StartAnimationObserverTest.Basic
> Bug:  898222 
> Change-Id: I86bdd253f0a4a4dd81ad6ba03780baf07923d256
> Reviewed-on: https://chromium-review.googlesource.com/c/1297405
> Commit-Queue: Sammie Quon <sammiequon@chromium.org>
> Reviewed-by: Mitsuru Oshima <oshima@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#602941}

Bug:  898222 
Change-Id: Idc7b84a5d47f4c872549ea55600b295f46e5127d
Reviewed-on: https://chromium-review.googlesource.com/c/1301073
Reviewed-by: Sammie Quon <sammiequon@chromium.org>
Commit-Queue: Sammie Quon <sammiequon@chromium.org>
Cr-Commit-Position: refs/heads/master@{#603010}
[modify] https://crrev.com/be1860675191a799acb2e758f2ead655ec5803a0/ash/BUILD.gn
[modify] https://crrev.com/be1860675191a799acb2e758f2ead655ec5803a0/ash/app_list/app_list_controller_impl.cc
[modify] https://crrev.com/be1860675191a799acb2e758f2ead655ec5803a0/ash/app_list/app_list_controller_impl.h
[modify] https://crrev.com/be1860675191a799acb2e758f2ead655ec5803a0/ash/shell.cc
[modify] https://crrev.com/be1860675191a799acb2e758f2ead655ec5803a0/ash/shell.h
[modify] https://crrev.com/be1860675191a799acb2e758f2ead655ec5803a0/ash/shell_observer.h
[modify] https://crrev.com/be1860675191a799acb2e758f2ead655ec5803a0/ash/wm/overview/cleanup_animation_observer.h
[modify] https://crrev.com/be1860675191a799acb2e758f2ead655ec5803a0/ash/wm/overview/cleanup_animation_observer_unittest.cc
[modify] https://crrev.com/be1860675191a799acb2e758f2ead655ec5803a0/ash/wm/overview/scoped_transform_overview_window.cc
[add] https://crrev.com/be1860675191a799acb2e758f2ead655ec5803a0/ash/wm/overview/start_animation_observer.cc
[add] https://crrev.com/be1860675191a799acb2e758f2ead655ec5803a0/ash/wm/overview/start_animation_observer.h
[add] https://crrev.com/be1860675191a799acb2e758f2ead655ec5803a0/ash/wm/overview/start_animation_observer_unittest.cc
[modify] https://crrev.com/be1860675191a799acb2e758f2ead655ec5803a0/ash/wm/overview/window_selector_controller.cc
[modify] https://crrev.com/be1860675191a799acb2e758f2ead655ec5803a0/ash/wm/overview/window_selector_controller.h
[modify] https://crrev.com/be1860675191a799acb2e758f2ead655ec5803a0/ash/wm/overview/window_selector_delegate.h
[modify] https://crrev.com/be1860675191a799acb2e758f2ead655ec5803a0/ash/wm/overview/window_selector_item.cc
[modify] https://crrev.com/be1860675191a799acb2e758f2ead655ec5803a0/ash/wm/overview/window_selector_unittest.cc

This change impacts a number of areas.  Can you add context regarding testing on ToT/M72 or otherwise prior to a merge?  I'd like to understand the merge risk.  

Is this needed to resolve an issue, or is it a feature that's being introduced?  We're way past feature freeze if this is a new feature..

This change only impacts overview code. The bits to ash/shell* and app_list are very negligible.

This is needed to help reduce all the jank we've been seeing entering overview mode. Its been tested manually , and new tests have been written for it.
Project Member

Comment 7 by sheriffbot@chromium.org, Oct 27

Labels: -Merge-Request-71 Hotlist-Merge-Review Merge-Review-71
This bug requires manual review: Reverts referenced in bugdroid comments after merge request.
Please contact the milestone owner if you have questions.
Owners: benmason@(Android), kariahda@(iOS), kbleicher@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Cc: kbleicher@chromium.org
+kbleicher for permissions
Cc: -kbleicher@chromium.org kbleicher@google.com
Labels: -Merge-Review-71 Merge-Approved-71
TPMs review requests regularly; no need for the CC.

Approving merge to M71 Chrome OS.

Project Member

Comment 11 by bugdroid1@chromium.org, Oct 30

Labels: -merge-approved-71 merge-merged-3578
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/de83f4bf8e9aa120356b1a585e7f697f106be93c

commit de83f4bf8e9aa120356b1a585e7f697f106be93c
Author: Sammie Quon <sammiequon@google.com>
Date: Tue Oct 30 19:31:42 2018

[merge to 71] Reland "overview: Introduce OnOverviewStartAnimationComplete observer."

This is a reland of 77cbc4f0fa1c446950859be5859071b9689208ae.
Added a missing ASH_EXPORT.

TBR=oshima@chromium.org, sammiequon@google.com

Original change's description:
> overview: Introduce OnOverviewStartAnimationComplete observer.
>
> Introdue a new overview observer which notifies observers when the start
> animation for overview is different.
>
> Uses will be both in and out of overview - for example adding mask, shadow
> and animating tray, shelf.
>
> Also modify CleanupObserverTest to use some new stuff.
>
> Test: ash_unittests StartAnimationObserverTest.Basic
> Bug:  898222 
> Change-Id: I86bdd253f0a4a4dd81ad6ba03780baf07923d256
> Reviewed-on: https://chromium-review.googlesource.com/c/1297405
> Commit-Queue: Sammie Quon <sammiequon@chromium.org>
> Reviewed-by: Mitsuru Oshima <oshima@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#602941}

(cherry picked from commit be1860675191a799acb2e758f2ead655ec5803a0)

Bug:  898222 
Change-Id: Idc7b84a5d47f4c872549ea55600b295f46e5127d
Reviewed-on: https://chromium-review.googlesource.com/c/1301073
Reviewed-by: Sammie Quon <sammiequon@chromium.org>
Commit-Queue: Sammie Quon <sammiequon@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#603010}
Reviewed-on: https://chromium-review.googlesource.com/c/1308660
Cr-Commit-Position: refs/branch-heads/3578@{#411}
Cr-Branched-From: 4226ddf99103e493d7afb23a4c7902ee496108b6-refs/heads/master@{#599034}
[modify] https://crrev.com/de83f4bf8e9aa120356b1a585e7f697f106be93c/ash/BUILD.gn
[modify] https://crrev.com/de83f4bf8e9aa120356b1a585e7f697f106be93c/ash/app_list/app_list_controller_impl.cc
[modify] https://crrev.com/de83f4bf8e9aa120356b1a585e7f697f106be93c/ash/app_list/app_list_controller_impl.h
[modify] https://crrev.com/de83f4bf8e9aa120356b1a585e7f697f106be93c/ash/shell.cc
[modify] https://crrev.com/de83f4bf8e9aa120356b1a585e7f697f106be93c/ash/shell.h
[modify] https://crrev.com/de83f4bf8e9aa120356b1a585e7f697f106be93c/ash/shell_observer.h
[modify] https://crrev.com/de83f4bf8e9aa120356b1a585e7f697f106be93c/ash/wm/overview/cleanup_animation_observer.h
[modify] https://crrev.com/de83f4bf8e9aa120356b1a585e7f697f106be93c/ash/wm/overview/cleanup_animation_observer_unittest.cc
[modify] https://crrev.com/de83f4bf8e9aa120356b1a585e7f697f106be93c/ash/wm/overview/scoped_transform_overview_window.cc
[add] https://crrev.com/de83f4bf8e9aa120356b1a585e7f697f106be93c/ash/wm/overview/start_animation_observer.cc
[add] https://crrev.com/de83f4bf8e9aa120356b1a585e7f697f106be93c/ash/wm/overview/start_animation_observer.h
[add] https://crrev.com/de83f4bf8e9aa120356b1a585e7f697f106be93c/ash/wm/overview/start_animation_observer_unittest.cc
[modify] https://crrev.com/de83f4bf8e9aa120356b1a585e7f697f106be93c/ash/wm/overview/window_selector_controller.cc
[modify] https://crrev.com/de83f4bf8e9aa120356b1a585e7f697f106be93c/ash/wm/overview/window_selector_controller.h
[modify] https://crrev.com/de83f4bf8e9aa120356b1a585e7f697f106be93c/ash/wm/overview/window_selector_delegate.h
[modify] https://crrev.com/de83f4bf8e9aa120356b1a585e7f697f106be93c/ash/wm/overview/window_selector_item.cc
[modify] https://crrev.com/de83f4bf8e9aa120356b1a585e7f697f106be93c/ash/wm/overview/window_selector_unittest.cc

Labels: Merge-Merged-71-3578
The following revision refers to this bug: 
https://chromium.googlesource.com/chromium/src.git/+/de83f4bf8e9aa120356b1a585e7f697f106be93c

Commit: de83f4bf8e9aa120356b1a585e7f697f106be93c
Author: sammiequon@google.com
Commiter: sammiequon@chromium.org
Date: 2018-10-30 19:31:42 +0000 UTC

[merge to 71] Reland "overview: Introduce OnOverviewStartAnimationComplete observer."

This is a reland of 77cbc4f0fa1c446950859be5859071b9689208ae.
Added a missing ASH_EXPORT.

TBR=oshima@chromium.org, sammiequon@google.com

Original change's description:
> overview: Introduce OnOverviewStartAnimationComplete observer.
>
> Introdue a new overview observer which notifies observers when the start
> animation for overview is different.
>
> Uses will be both in and out of overview - for example adding mask, shadow
> and animating tray, shelf.
>
> Also modify CleanupObserverTest to use some new stuff.
>
> Test: ash_unittests StartAnimationObserverTest.Basic
> Bug:  898222 
> Change-Id: I86bdd253f0a4a4dd81ad6ba03780baf07923d256
> Reviewed-on: https://chromium-review.googlesource.com/c/1297405
> Commit-Queue: Sammie Quon <sammiequon@chromium.org>
> Reviewed-by: Mitsuru Oshima <oshima@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#602941}

(cherry picked from commit be1860675191a799acb2e758f2ead655ec5803a0)

Bug:  898222 
Change-Id: Idc7b84a5d47f4c872549ea55600b295f46e5127d
Reviewed-on: https://chromium-review.googlesource.com/c/1301073
Reviewed-by: Sammie Quon <sammiequon@chromium.org>
Commit-Queue: Sammie Quon <sammiequon@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#603010}
Reviewed-on: https://chromium-review.googlesource.com/c/1308660
Cr-Commit-Position: refs/branch-heads/3578@{#411}
Cr-Branched-From: 4226ddf99103e493d7afb23a4c7902ee496108b6-refs/heads/master@{#599034}
Status: Fixed (was: Assigned)

Sign in to add a comment