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

Issue 843851 link

Starred by 1 user

Issue metadata

Status: Assigned
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 1
Type: Bug



Sign in to add a comment

Title bar visible while transitioning to overview mode.

Project Member Reported by sammiequon@chromium.org, May 17 2018

Issue description

We'd want the transition to look like [1] eventually.

But for now it would be sufficient to hide the title bar before transitioning to overview, instead of after.

[1] https://mccanny.users.x20web.corp.google.com/www/wm-motion/index.html#overview-freeform
 
Screenshot 2018-05-16 at 18.22.32.png
450 KB View Download

Comment 1 by wutao@chromium.org, May 17 2018

Hi reveman@ I found the "deferred paint" may not affect the animation smoothness too much. Maybe we do not apply the "deferred paint" in this case?

sammiequon@, could you please compare the animation smoothness with/without "deferred paint"?

There's no way to make this reliable smooth if we rely on raster for those animations. We should design these animations so we're not relying on raster if we want them to be smooth. Also, it looks like these animations use mask layers. That's has a significant cost and will limit how often these animations can be smooth.
wutao@ - I did the some comparison on my worstation:
Without deferred paint:
Enter - max 100 - avg 43
Exit - max 64 - avg 44
With deferred paint:
Enter - max 100 - avg 52
Exit - max 74 - avg 54

reveman@ - Are you referring to the new animations in the spec? UX is aware of the problems but for now we'd just like to remove the title bar so it doesn't appear during animation like in the screenshot.

Comment 4 by wutao@chromium.org, May 18 2018

Thanks sammiequon@, it does have ~20% improvement at workstation. IIRU, on real device, the improvement may be not so big.

Another possibility is that, we set hide the title bar and wait one frame, and then start animation with deferred paint in the next frame.

There are several compositor callback, you can look into this as an example how to use it: https://chromium-review.googlesource.com/c/chromium/src/+/671688
Basically, you call hide title bar first, and then add "this" to the compositor observer, at the the OnCompositingDidCommit(), you start animations.

Will this be too complicated?





Owner: wutao@chromium.org
Status: Assigned (was: Untriaged)
wutao@ please triage by assigning to appropriate owner?

Comment 6 by wutao@chromium.org, Jun 13 2018

Cc: -sammiequon@chromium.org
Owner: sammiequon@chromium.org
Project Member

Comment 7 by bugdroid1@chromium.org, Jul 10

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

commit a663e7fd51488006a133e4115cf306029ea7ee23
Author: Sammie Quon <sammiequon@google.com>
Date: Tue Jul 10 00:28:32 2018

overview: Hide title bars before animating into overview mode.

The title bars should not visible while animating overview mode. This cl
hides the title bar if needed and waits until the next paint before
animating the windows. This will hide the title bar as expected while
keeping the benefits of DeferPaint.

Test: manual
Bug: 843851
Change-Id: I99cf7ca39fd1ae14314ac406abd0fec999e2c024
Reviewed-on: https://chromium-review.googlesource.com/1112774
Reviewed-by: Mitsuru Oshima <oshima@chromium.org>
Reviewed-by: Tao Wu <wutao@chromium.org>
Commit-Queue: Sammie Quon <sammiequon@chromium.org>
Cr-Commit-Position: refs/heads/master@{#573552}
[modify] https://crrev.com/a663e7fd51488006a133e4115cf306029ea7ee23/ash/system/overview/overview_button_tray_unittest.cc
[modify] https://crrev.com/a663e7fd51488006a133e4115cf306029ea7ee23/ash/test/ash_test_helper.cc
[modify] https://crrev.com/a663e7fd51488006a133e4115cf306029ea7ee23/ash/wm/overview/scoped_transform_overview_window.cc
[modify] https://crrev.com/a663e7fd51488006a133e4115cf306029ea7ee23/ash/wm/overview/scoped_transform_overview_window.h
[modify] https://crrev.com/a663e7fd51488006a133e4115cf306029ea7ee23/ash/wm/overview/window_selector_item.cc
[modify] https://crrev.com/a663e7fd51488006a133e4115cf306029ea7ee23/ash/wm/overview/window_selector_item.h

Project Member

Comment 8 by bugdroid1@chromium.org, Jul 10

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

commit 90e3ae20e0e37d8dfc0284f2e4974a7a3861b409
Author: Joshua Pawlicki <waffles@chromium.org>
Date: Tue Jul 10 16:10:55 2018

Revert "overview: Hide title bars before animating into overview mode."

This reverts commit a663e7fd51488006a133e4115cf306029ea7ee23.

Reason for revert: Causes flaky thread-checker failure in debug builds; see  crbug.com/862165 

Original change's description:
> overview: Hide title bars before animating into overview mode.
> 
> The title bars should not visible while animating overview mode. This cl
> hides the title bar if needed and waits until the next paint before
> animating the windows. This will hide the title bar as expected while
> keeping the benefits of DeferPaint.
> 
> Test: manual
> Bug: 843851
> Change-Id: I99cf7ca39fd1ae14314ac406abd0fec999e2c024
> Reviewed-on: https://chromium-review.googlesource.com/1112774
> Reviewed-by: Mitsuru Oshima <oshima@chromium.org>
> Reviewed-by: Tao Wu <wutao@chromium.org>
> Commit-Queue: Sammie Quon <sammiequon@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#573552}

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

Change-Id: I0878300bd4d9a5935d00776442f68b85a46981f2
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 843851
Reviewed-on: https://chromium-review.googlesource.com/1131714
Reviewed-by: Joshua Pawlicki <waffles@chromium.org>
Commit-Queue: Joshua Pawlicki <waffles@chromium.org>
Cr-Commit-Position: refs/heads/master@{#573759}
[modify] https://crrev.com/90e3ae20e0e37d8dfc0284f2e4974a7a3861b409/ash/system/overview/overview_button_tray_unittest.cc
[modify] https://crrev.com/90e3ae20e0e37d8dfc0284f2e4974a7a3861b409/ash/test/ash_test_helper.cc
[modify] https://crrev.com/90e3ae20e0e37d8dfc0284f2e4974a7a3861b409/ash/wm/overview/scoped_transform_overview_window.cc
[modify] https://crrev.com/90e3ae20e0e37d8dfc0284f2e4974a7a3861b409/ash/wm/overview/scoped_transform_overview_window.h
[modify] https://crrev.com/90e3ae20e0e37d8dfc0284f2e4974a7a3861b409/ash/wm/overview/window_selector_item.cc
[modify] https://crrev.com/90e3ae20e0e37d8dfc0284f2e4974a7a3861b409/ash/wm/overview/window_selector_item.h

Project Member

Comment 9 by bugdroid1@chromium.org, Jul 11

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

commit f00acd85de088eb905005c5bc0c144361caaa814
Author: Sammie Quon <sammiequon@google.com>
Date: Wed Jul 11 17:21:12 2018

Reland "overview: Hide title bars before animating into overview mode."

This is a reland of a663e7fd51488006a133e4115cf306029ea7ee23

The original cl causes some flaky tests with stack trace:

[16202:16202:0709/211202.910243:FATAL:single_thread_proxy.cc(237)] Check failed: task_runner_provider_->IsMainThread().
#0 0x7f9b36f8672d base::debug::StackTrace::StackTrace()
#1 0x7f9b36c9d3fc base::debug::StackTrace::StackTrace()
#2 0x7f9b36d06dfd logging::LogMessage::~LogMessage()
#3 0x7f9b2b5692e8 cc::SingleThreadProxy::SetNeedsCommit()
#4 0x7f9b2b45ecef cc::LayerTreeHost::SetNeedsCommit()
#5 0x7f9b2b467426 cc::LayerTreeHost::SetMutatorsNeedCommit()
#6 0x7f9b0fd29981 cc::AnimationHost::SetNeedsCommit()
#7 0x7f9b0fd1962d cc::Animation::SetNeedsCommit()
#8 0x7f9b0fd41151 cc::KeyframeEffect::KeyframeModelAdded()
#9 0x7f9b0fd44e6c cc::KeyframeEffect::AddKeyframeModel()
#10 0x7f9b0fd17fbc cc::Animation::AddKeyframeModelForKeyframeEffect()
#11 0x7f9b0fd71a6d cc::SingleKeyframeEffectAnimation::AddKeyframeModel()
#12 0x7f9b28c83a95 ui::LayerAnimator::AddThreadedAnimation()
#13 0x7f9b28c7498e ui::(anonymous namespace)::ThreadedLayerAnimationElement::RequestEffectiveStart()
#14 0x7f9b28c71113 ui::LayerAnimationElement::Start()
#15 0x7f9b28c7cbe3 ui::LayerAnimationSequence::Start()
#16 0x7f9b28c84287 ui::LayerAnimator::StartSequenceImmediately()
#17 0x7f9b28c81251 ui::LayerAnimator::StartAnimation()
#18 0x7f9b28c80eb6 ui::LayerAnimator::SetTransform()
#19 0x7f9b28c6122e ui::Layer::SetTransform()
#20 0x7f9b28ded831 aura::Window::SetTransform()
#21 0x7f9b22b732df ash::ScopedTransformOverviewWindow::SetTransform()
#22 0x7f9b22b73f69 ash::ScopedTransformOverviewWindow::OnCompositingStarted()
#23 0x7f9b28c4d734 ui::Compositor::DidSubmitCompositorFrame()
#24 0x7f9b2b56cd05 cc::SingleThreadProxy::DoComposite()
#25 0x7f9b2b56e835 cc::SingleThreadProxy::ScheduledActionDrawIfPossible()
#26 0x7f9b2b3736fd cc::Scheduler::DrawIfPossible()
#27 0x7f9b2b36dbdc cc::Scheduler::ProcessScheduledActions()
#28 0x7f9b2b36d5fd cc::Scheduler::OnBeginImplFrameDeadline()

This cl makes that test skip waiting for composit when entering overview, like the tests
in ash.

Original change's description:
> overview: Hide title bars before animating into overview mode.
>
> The title bars should not visible while animating overview mode. This cl
> hides the title bar if needed and waits until the next paint before
> animating the windows. This will hide the title bar as expected while
> keeping the benefits of DeferPaint.
>
> Test: manual
> Bug: 843851
> Change-Id: I99cf7ca39fd1ae14314ac406abd0fec999e2c024
> Reviewed-on: https://chromium-review.googlesource.com/1112774
> Reviewed-by: Mitsuru Oshima <oshima@chromium.org>
> Reviewed-by: Tao Wu <wutao@chromium.org>
> Commit-Queue: Sammie Quon <sammiequon@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#573552}

TBR=oshima@chromium.org

Bug: 843851
Change-Id: I3a538250d5f6d42d6c73e218445080af533b54dc
Reviewed-on: https://chromium-review.googlesource.com/1131874
Commit-Queue: Sammie Quon <sammiequon@chromium.org>
Reviewed-by: Sammie Quon <sammiequon@chromium.org>
Reviewed-by: Michael Wasserman <msw@chromium.org>
Cr-Commit-Position: refs/heads/master@{#574209}
[modify] https://crrev.com/f00acd85de088eb905005c5bc0c144361caaa814/ash/system/overview/overview_button_tray_unittest.cc
[modify] https://crrev.com/f00acd85de088eb905005c5bc0c144361caaa814/ash/test/ash_test_helper.cc
[modify] https://crrev.com/f00acd85de088eb905005c5bc0c144361caaa814/ash/wm/overview/scoped_transform_overview_window.cc
[modify] https://crrev.com/f00acd85de088eb905005c5bc0c144361caaa814/ash/wm/overview/scoped_transform_overview_window.h
[modify] https://crrev.com/f00acd85de088eb905005c5bc0c144361caaa814/ash/wm/overview/window_selector_item.cc
[modify] https://crrev.com/f00acd85de088eb905005c5bc0c144361caaa814/ash/wm/overview/window_selector_item.h
[modify] https://crrev.com/f00acd85de088eb905005c5bc0c144361caaa814/chrome/browser/ui/views/frame/browser_non_client_frame_view_ash_browsertest.cc

It seems this cl does not work with OnCompositingStarted anymore (issue 863795). Uploaded cl to fix the crash in the meantime, but this will need investigation why it started failing.
Project Member

Comment 11 by bugdroid1@chromium.org, Jul 16

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

commit b87381369ce222524f52343c2bd4f0387574191e
Author: Sammie Quon <sammiequon@google.com>
Date: Mon Jul 16 22:28:03 2018

overview: Fix crash with hiding title bars on entry.

Using OnCompositorStarted does not work for overview anymore. See stack
trace in bug.

Test: manual
Bug: 863795, 843851
Change-Id: I3dc8965cbece03ac36f1dc404c9b19b7dcac5b5e
Reviewed-on: https://chromium-review.googlesource.com/1138737
Commit-Queue: Sammie Quon <sammiequon@chromium.org>
Reviewed-by: Mitsuru Oshima <oshima@chromium.org>
Cr-Commit-Position: refs/heads/master@{#575447}
[modify] https://crrev.com/b87381369ce222524f52343c2bd4f0387574191e/ash/wm/overview/scoped_transform_overview_window.cc
[modify] https://crrev.com/b87381369ce222524f52343c2bd4f0387574191e/ash/wm/overview/scoped_transform_overview_window.h

Chrome now crashes (hits a dcheck) when entering overview. This code snippet is not valid because the cast breaks for browser windows (among other window types):


  CustomFrameViewAsh* frame =
      static_cast<CustomFrameViewAsh*>(widget->non_client_view()->frame_view());
  if (!frame)
    return false;
Labels: -Pri-2 Pri-1
Hi Sammie,

are you aware of the above issue and/or working on a fix? It causes a crash in debug and undefined behavior in release, so I think we should fix it very soon or revert the offending change.
Hi estade - sorry i  meant to reply yesterday but got sidetracked.

There is a cl associated with issue 863795 submitted that should fix the crash on ToT. The cause was something else other than the invalid cast (which I will investigate).

That should work for now in release ill investigate the debug crash more thoroughly.
Project Member

Comment 15 by bugdroid1@chromium.org, Jul 18

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

commit c89a63613279a3ebe0a0397c31f48949b55a4a6f
Author: Sammie Quon <sammiequon@chromium.org>
Date: Wed Jul 18 18:06:19 2018

Revert "overview: Fix crash with hiding title bars on entry."

This reverts commit b87381369ce222524f52343c2bd4f0387574191e.

Reason for revert: crbug.com/863795

Original change's description:
> overview: Fix crash with hiding title bars on entry.
> 
> Using OnCompositorStarted does not work for overview anymore. See stack
> trace in bug.
> 
> Test: manual
> Bug: 863795, 843851
> Change-Id: I3dc8965cbece03ac36f1dc404c9b19b7dcac5b5e
> Reviewed-on: https://chromium-review.googlesource.com/1138737
> Commit-Queue: Sammie Quon <sammiequon@chromium.org>
> Reviewed-by: Mitsuru Oshima <oshima@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#575447}

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

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 863795, 843851
Change-Id: Ibc5965bc51a62e08b44d034a81f6954b8e799cad
Reviewed-on: https://chromium-review.googlesource.com/1142166
Reviewed-by: Sammie Quon <sammiequon@chromium.org>
Commit-Queue: Sammie Quon <sammiequon@chromium.org>
Cr-Commit-Position: refs/heads/master@{#576135}
[modify] https://crrev.com/c89a63613279a3ebe0a0397c31f48949b55a4a6f/ash/wm/overview/scoped_transform_overview_window.cc
[modify] https://crrev.com/c89a63613279a3ebe0a0397c31f48949b55a4a6f/ash/wm/overview/scoped_transform_overview_window.h

Project Member

Comment 16 by bugdroid1@chromium.org, Jul 18

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

commit fe17281dcd0cdcbd10524383b0eca16bd1d468c8
Author: Sammie Quon <sammiequon@chromium.org>
Date: Wed Jul 18 23:25:38 2018

Revert "Reland "overview: Hide title bars before animating into overview mode.""

This reverts commit f00acd85de088eb905005c5bc0c144361caaa814.

Reason for revert: crbug.com/863795

Original change's description:
> Reland "overview: Hide title bars before animating into overview mode."
> 
> This is a reland of a663e7fd51488006a133e4115cf306029ea7ee23
> 
> The original cl causes some flaky tests with stack trace:
> 
> [16202:16202:0709/211202.910243:FATAL:single_thread_proxy.cc(237)] Check failed: task_runner_provider_->IsMainThread().
> #0 0x7f9b36f8672d base::debug::StackTrace::StackTrace()
> #1 0x7f9b36c9d3fc base::debug::StackTrace::StackTrace()
> #2 0x7f9b36d06dfd logging::LogMessage::~LogMessage()
> #3 0x7f9b2b5692e8 cc::SingleThreadProxy::SetNeedsCommit()
> #4 0x7f9b2b45ecef cc::LayerTreeHost::SetNeedsCommit()
> #5 0x7f9b2b467426 cc::LayerTreeHost::SetMutatorsNeedCommit()
> #6 0x7f9b0fd29981 cc::AnimationHost::SetNeedsCommit()
> #7 0x7f9b0fd1962d cc::Animation::SetNeedsCommit()
> #8 0x7f9b0fd41151 cc::KeyframeEffect::KeyframeModelAdded()
> #9 0x7f9b0fd44e6c cc::KeyframeEffect::AddKeyframeModel()
> #10 0x7f9b0fd17fbc cc::Animation::AddKeyframeModelForKeyframeEffect()
> #11 0x7f9b0fd71a6d cc::SingleKeyframeEffectAnimation::AddKeyframeModel()
> #12 0x7f9b28c83a95 ui::LayerAnimator::AddThreadedAnimation()
> #13 0x7f9b28c7498e ui::(anonymous namespace)::ThreadedLayerAnimationElement::RequestEffectiveStart()
> #14 0x7f9b28c71113 ui::LayerAnimationElement::Start()
> #15 0x7f9b28c7cbe3 ui::LayerAnimationSequence::Start()
> #16 0x7f9b28c84287 ui::LayerAnimator::StartSequenceImmediately()
> #17 0x7f9b28c81251 ui::LayerAnimator::StartAnimation()
> #18 0x7f9b28c80eb6 ui::LayerAnimator::SetTransform()
> #19 0x7f9b28c6122e ui::Layer::SetTransform()
> #20 0x7f9b28ded831 aura::Window::SetTransform()
> #21 0x7f9b22b732df ash::ScopedTransformOverviewWindow::SetTransform()
> #22 0x7f9b22b73f69 ash::ScopedTransformOverviewWindow::OnCompositingStarted()
> #23 0x7f9b28c4d734 ui::Compositor::DidSubmitCompositorFrame()
> #24 0x7f9b2b56cd05 cc::SingleThreadProxy::DoComposite()
> #25 0x7f9b2b56e835 cc::SingleThreadProxy::ScheduledActionDrawIfPossible()
> #26 0x7f9b2b3736fd cc::Scheduler::DrawIfPossible()
> #27 0x7f9b2b36dbdc cc::Scheduler::ProcessScheduledActions()
> #28 0x7f9b2b36d5fd cc::Scheduler::OnBeginImplFrameDeadline()
> 
> This cl makes that test skip waiting for composit when entering overview, like the tests
> in ash.
> 
> Original change's description:
> > overview: Hide title bars before animating into overview mode.
> >
> > The title bars should not visible while animating overview mode. This cl
> > hides the title bar if needed and waits until the next paint before
> > animating the windows. This will hide the title bar as expected while
> > keeping the benefits of DeferPaint.
> >
> > Test: manual
> > Bug: 843851
> > Change-Id: I99cf7ca39fd1ae14314ac406abd0fec999e2c024
> > Reviewed-on: https://chromium-review.googlesource.com/1112774
> > Reviewed-by: Mitsuru Oshima <oshima@chromium.org>
> > Reviewed-by: Tao Wu <wutao@chromium.org>
> > Commit-Queue: Sammie Quon <sammiequon@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#573552}
> 
> TBR=oshima@chromium.org
> 
> Bug: 843851
> Change-Id: I3a538250d5f6d42d6c73e218445080af533b54dc
> Reviewed-on: https://chromium-review.googlesource.com/1131874
> Commit-Queue: Sammie Quon <sammiequon@chromium.org>
> Reviewed-by: Sammie Quon <sammiequon@chromium.org>
> Reviewed-by: Michael Wasserman <msw@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#574209}

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

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 843851
Change-Id: Ib2744c7f677be40004b70a714cebe0a337f7c60b
Reviewed-on: https://chromium-review.googlesource.com/1142304
Reviewed-by: Sammie Quon <sammiequon@chromium.org>
Reviewed-by: Mitsuru Oshima <oshima@chromium.org>
Reviewed-by: Michael Wasserman <msw@chromium.org>
Commit-Queue: Sammie Quon <sammiequon@chromium.org>
Cr-Commit-Position: refs/heads/master@{#576250}
[modify] https://crrev.com/fe17281dcd0cdcbd10524383b0eca16bd1d468c8/ash/system/overview/overview_button_tray_unittest.cc
[modify] https://crrev.com/fe17281dcd0cdcbd10524383b0eca16bd1d468c8/ash/test/ash_test_helper.cc
[modify] https://crrev.com/fe17281dcd0cdcbd10524383b0eca16bd1d468c8/ash/wm/overview/scoped_transform_overview_window.cc
[modify] https://crrev.com/fe17281dcd0cdcbd10524383b0eca16bd1d468c8/ash/wm/overview/scoped_transform_overview_window.h
[modify] https://crrev.com/fe17281dcd0cdcbd10524383b0eca16bd1d468c8/ash/wm/overview/window_selector_item.cc
[modify] https://crrev.com/fe17281dcd0cdcbd10524383b0eca16bd1d468c8/ash/wm/overview/window_selector_item.h
[modify] https://crrev.com/fe17281dcd0cdcbd10524383b0eca16bd1d468c8/chrome/browser/ui/views/frame/browser_non_client_frame_view_ash_browsertest.cc

Sign in to add a comment