Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(191)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 2061693003: MacViews: Suppress visibility changes during an asynchronous close.

Created:
4 years, 6 months ago by tapted
Modified:
4 years, 4 months ago
Reviewers:
CC:
chromium-reviews, tfarina, chrome-apps-syd-reviews_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

MacViews: Suppress visibility changes during an asynchronous close. Currently, MD buttons with ripple effects receive a visibility change while a Widget is closing. InkDropHostView::VisibilityChanged(..) has logic that begins a Layer animation when visibility becomes false. Since it's during Widget::Close(), this tickles a codepath when adding the layer to the Button that also attempts to create the parent layer backing the entire widget (which just got cleared out due to the close request). NativeWidgetMac doesn't like this - it only has logic implemented to sync up the Widget's backing layer with the compositor once, and DCHECKs if something tries to do it again. Views generally shouldn't be doing much if their Widget is closing, so fix this by suppressing the Widget visibility change notifications during Widget closure in NativeWidgetMac. Add a test for this. Currently the test reveals that both Windows and Linux still send out visibility change notifications during Widget::Close(). == Open questions == - Is InkDropHostView::VisibilityChanged(..) also trying to start animations during closure on Windows/Linux - If so, is it causing bad stuff (but not bad enough to crash/dcheck)? - Is suppressing visibility changes during Widget::Close the right thing to do? (e.g. do we lose a valid use-case doing this, or just get the benefit of less noise/cycles during Widget Close). - Do we need to clear out the layer during Close() at all? (If we don't there are lifetime issues in WidgetCaptureTest.DestroyWithCapture_Close, but they may be able to be fixed) BUG=619798

Patch Set 1 #

Patch Set 2 : Just simplify ::Close() #

Patch Set 3 : Add a test, try it out on other platforms #

Patch Set 4 : Move things to BridgedNativeWidget #

Patch Set 5 : rebase #

Patch Set 6 : fixes #

Patch Set 7 : Neater, but Linux, Windows disagree on WidgetTest.DesktopNativeWidgetVisibilityAfterCloseTest #

Unified diffs Side-by-side diffs Delta from patch set Stats (+204 lines, -102 lines) Patch
M ui/views/cocoa/bridged_native_widget.h View 1 2 3 4 3 chunks +12 lines, -0 lines 0 comments Download
M ui/views/cocoa/bridged_native_widget.mm View 1 2 3 4 5 6 10 chunks +66 lines, -12 lines 0 comments Download
A ui/views/cocoa/views_nswindow_close_animator.h View 1 2 3 1 chunk +18 lines, -0 lines 0 comments Download
A ui/views/cocoa/views_nswindow_close_animator.mm View 1 2 3 1 chunk +47 lines, -0 lines 0 comments Download
M ui/views/views.gyp View 1 2 3 4 1 chunk +2 lines, -0 lines 0 comments Download
M ui/views/widget/native_widget_mac.h View 1 2 3 4 1 chunk +0 lines, -4 lines 0 comments Download
M ui/views/widget/native_widget_mac.mm View 1 2 3 4 5 5 chunks +1 line, -78 lines 0 comments Download
M ui/views/widget/widget_unittest.cc View 1 2 3 4 4 chunks +58 lines, -8 lines 0 comments Download

Messages

Total messages: 14 (14 generated)
tapted
The CQ bit was checked by tapted@chromium.org to run a CQ dry run
4 years, 4 months ago (2016-08-11 06:34:15 UTC) #1
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2061693003/80001
4 years, 4 months ago (2016-08-11 06:34:24 UTC) #2
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
4 years, 4 months ago (2016-08-11 06:49:31 UTC) #3
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_ng/builds/275933)
4 years, 4 months ago (2016-08-11 06:49:31 UTC) #4
tapted
The CQ bit was checked by tapted@chromium.org to run a CQ dry run
4 years, 4 months ago (2016-08-11 07:24:37 UTC) #5
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2061693003/100001
4 years, 4 months ago (2016-08-11 07:24:47 UTC) #6
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
4 years, 4 months ago (2016-08-11 07:36:46 UTC) #7
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_ng/builds/271739)
4 years, 4 months ago (2016-08-11 07:36:47 UTC) #8
tapted
The CQ bit was checked by tapted@chromium.org to run a CQ dry run
4 years, 4 months ago (2016-08-11 09:30:50 UTC) #9
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2061693003/120001
4 years, 4 months ago (2016-08-11 09:31:03 UTC) #10
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
4 years, 4 months ago (2016-08-11 09:46:21 UTC) #11
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_rel_ng/builds/260066)
4 years, 4 months ago (2016-08-11 09:46:22 UTC) #12
tapted
Description was changed from ========== MacViews: Don't try to clean up before an asynchronous close. ...
4 years, 4 months ago (2016-08-12 06:37:03 UTC) #13
tapted
4 years, 4 months ago (2016-08-12 06:37:24 UTC) #14 
Description was changed from

==========
MacViews: Suppress visibility changes during an asynchronous close.

Currently, MD buttons with ripple effects receive a visibility change
while a Widget is closing. InkDropHostView::VisibilityChanged(..) has
logic that begins a Layer animation when visibility becomes false.

Since it's during Widget::Close(), this tickles a codepath when adding
the layer to the Button that also attempts to create the parent layer
backing the entire widget (which just got cleared out due to the close
request). NativeWidgetMac doesn't like this - it only has logic
implemented to sync up the Widget's backing layer with the compositor
once, and DCHECKs if something tries to do it again.

Views generally shouldn't be doing much if their Widget is closing, so
fix this by suppressing the Widget visibility change notifications
during Widget closure in NativeWidgetMac. Add a test for this.

Currently the test reveals that both Windows and Linux still send out
visibility change notifications during Widget::Close().

Open questions:  - Is InkDropHostView::VisibilityChanged(..) also trying
to start animations during closure on Windows/Linux

- If so, is it causing bad stuff (but not bad enough to crash/dcheck)?

- Is suppressing visibility changes during Widget::Close the right thing
to do? (e.g. do we lose a valid use-case doing this, or just get the
benefit of less noise/cycles during Widget Close).

- Do we need to clear out the layer during Close() at all? (If we don't
there are lifetime issues in WidgetCaptureTest.DestroyWithCapture_Close,
but they may be able to be fixed)

BUG=619798
==========

to

==========
MacViews: Suppress visibility changes during an asynchronous close.

Currently, MD buttons with ripple effects receive a visibility change
while a Widget is closing. InkDropHostView::VisibilityChanged(..) has
logic that begins a Layer animation when visibility becomes false.

Since it's during Widget::Close(), this tickles a codepath when adding
the layer to the Button that also attempts to create the parent layer
backing the entire widget (which just got cleared out due to the close
request). NativeWidgetMac doesn't like this - it only has logic
implemented to sync up the Widget's backing layer with the compositor
once, and DCHECKs if something tries to do it again.

Views generally shouldn't be doing much if their Widget is closing, so
fix this by suppressing the Widget visibility change notifications
during Widget closure in NativeWidgetMac. Add a test for this.

Currently the test reveals that both Windows and Linux still send out
visibility change notifications during Widget::Close().

== Open questions ==

- Is InkDropHostView::VisibilityChanged(..) also trying to start
animations during closure on Windows/Linux

- If so, is it causing bad stuff (but not bad enough to crash/dcheck)?

- Is suppressing visibility changes during Widget::Close the right thing
to do? (e.g. do we lose a valid use-case doing this, or just get the
benefit of less noise/cycles during Widget Close).

- Do we need to clear out the layer during Close() at all? (If we don't
there are lifetime issues in WidgetCaptureTest.DestroyWithCapture_Close,
but they may be able to be fixed)

BUG=619798
==========

Powered by Google App Engine
This is Rietveld 408576698