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

Issue 711514 link

Starred by 1 user

Issue metadata

Status: Archived
Owner:
Closed: May 2017
Cc:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 1
Type: Bug



Sign in to add a comment

shadow underlay for maximize/fullscreen should stay visible during the state transition.

Project Member Reported by osh...@chromium.org, Apr 13 2017

Issue description

Shadow underlay for maximize/fullscreen should stay visible during the state transition.
 

Comment 1 by osh...@chromium.org, Apr 13 2017

Cc: mtomasz@chromium.org reve...@chromium.org lpique@chromium.org
chrome side bug for b/36924340
Project Member

Comment 2 by bugdroid1@chromium.org, Apr 14 2017

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

commit ae72f92d7caba6d32ba2a11f32943f29af15c3d8
Author: oshima <oshima@chromium.org>
Date: Fri Apr 14 23:17:20 2017

Do not hide shadow underlay for max/fullscreen state even if the client disables the shadow.

During the state transition, ARC preserves windows being deleted,
however the main window is deleted (thus, window frame is empty) which disables the shadow.

BUG= 711514 
TEST=covered by unit tests.
 manual. Instal & start "Clash Royal", then F4 to toggle fullscreen.

Review-Url: https://codereview.chromium.org/2820493004
Cr-Commit-Position: refs/heads/master@{#464819}

[modify] https://crrev.com/ae72f92d7caba6d32ba2a11f32943f29af15c3d8/components/exo/shell_surface.cc
[modify] https://crrev.com/ae72f92d7caba6d32ba2a11f32943f29af15c3d8/components/exo/shell_surface_unittest.cc

Comment 3 by osh...@chromium.org, Apr 15 2017

Labels: Merge-Request-58 Merge-Request-59
Project Member

Comment 4 by sheriffbot@chromium.org, Apr 15 2017

Labels: -Merge-Request-58 Merge-Review-58 Hotlist-Merge-Review
This bug requires manual review: We are only 9 days from stable.
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), bhthompson@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 5 by osh...@chromium.org, Apr 15 2017

Cc: bthornton@google.com
Project Member

Comment 6 by sheriffbot@chromium.org, Apr 15 2017

Labels: -Merge-Request-59 Hotlist-Merge-Approved Merge-Approved-59
Your change meets the bar and is auto-approved for M59. Please go ahead and merge the CL to branch 3071 manually. Please contact milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), gkihumba@(ChromeOS), Abdul Syed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 7 by osh...@chromium.org, Apr 17 2017

Cc: -bthornton@google.com bhthompson@chromium.org
Labels: -Hotlist-Merge-Review -Merge-Review-58 Merge-Approved-58
Project Member

Comment 9 by bugdroid1@chromium.org, Apr 17 2017

Labels: -merge-approved-58 merge-merged-3029
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/313d467e282244aeecbbf9120be11dd943cef91b

commit 313d467e282244aeecbbf9120be11dd943cef91b
Author: Mitsuru Oshima <oshima@chromium.org>
Date: Mon Apr 17 16:30:32 2017

Do not hide shadow underlay for max/fullscreen state even if the client disables the shadow.

During the state transition, ARC preserves windows being deleted,
however the main window is deleted (thus, window frame is empty) which disables the shadow.

BUG= 711514 
TEST=covered by unit tests.
 manual. Instal & start "Clash Royal", then F4 to toggle fullscreen.

Review-Url: https://codereview.chromium.org/2820493004
Cr-Commit-Position: refs/heads/master@{#464819}
(cherry picked from commit ae72f92d7caba6d32ba2a11f32943f29af15c3d8)

Review-Url: https://codereview.chromium.org/2818413003 .
Cr-Commit-Position: refs/branch-heads/3029@{#733}
Cr-Branched-From: 939b32ee5ba05c396eef3fd992822fcca9a2e262-refs/heads/master@{#454471}

[modify] https://crrev.com/313d467e282244aeecbbf9120be11dd943cef91b/components/exo/shell_surface.cc
[modify] https://crrev.com/313d467e282244aeecbbf9120be11dd943cef91b/components/exo/shell_surface_unittest.cc

Status: Fixed (was: Started)
Project Member

Comment 11 by sheriffbot@chromium.org, Apr 19 2017

This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible!

If all merges have been completed, please remove any remaining Merge-Approved labels from this issue.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 12 by bugdroid1@chromium.org, Apr 19 2017

Labels: -merge-approved-59 merge-merged-3071
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4d5dad8b29ff50856c5565610dedc7d17aaf0080

commit 4d5dad8b29ff50856c5565610dedc7d17aaf0080
Author: Mitsuru Oshima <oshima@chromium.org>
Date: Wed Apr 19 16:47:55 2017

Do not hide shadow underlay for max/fullscreen state even if the client disables the shadow.

During the state transition, ARC preserves windows being deleted,
however the main window is deleted (thus, window frame is empty) which disables the shadow.

BUG= 711514 
TEST=covered by unit tests.
 manual. Instal & start "Clash Royal", then F4 to toggle fullscreen.

Review-Url: https://codereview.chromium.org/2820493004
Cr-Commit-Position: refs/heads/master@{#464819}
(cherry picked from commit ae72f92d7caba6d32ba2a11f32943f29af15c3d8)

Review-Url: https://codereview.chromium.org/2821963002 .
Cr-Commit-Position: refs/branch-heads/3071@{#52}
Cr-Branched-From: a106f0abbf69dad349d4aaf4bcc4f5d376dd2377-refs/heads/master@{#464641}

[modify] https://crrev.com/4d5dad8b29ff50856c5565610dedc7d17aaf0080/components/exo/shell_surface.cc
[modify] https://crrev.com/4d5dad8b29ff50856c5565610dedc7d17aaf0080/components/exo/shell_surface_unittest.cc

Project Member

Comment 13 by bugdroid1@chromium.org, Apr 20 2017

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

commit b650cb773adbf72f3f731e0bc56e22d55c521391
Author: nya <nya@chromium.org>
Date: Thu Apr 20 17:55:22 2017

Revert of Do not hide shadow underlay for max/fullscreen state even if the client disables the shadow. (patchset #3 id:40001 of https://codereview.chromium.org/2820493004/ )

Reason for revert:
This change broke basic WM interaction of ARC and caused browser crash.  https://crbug.com/713560 

Original issue's description:
> Do not hide shadow underlay for max/fullscreen state even if the client disables the shadow.
>
> During the state transition, ARC preserves windows being deleted,
> however the main window is deleted (thus, window frame is empty) which disables the shadow.
>
> BUG= 711514 
> TEST=covered by unit tests.
>  manual. Instal & start "Clash Royal", then F4 to toggle fullscreen.
>
> Review-Url: https://codereview.chromium.org/2820493004
> Cr-Commit-Position: refs/heads/master@{#464819}
> Committed: https://chromium.googlesource.com/chromium/src/+/ae72f92d7caba6d32ba2a11f32943f29af15c3d8

TBR=reveman@chromium.org,oshima@chromium.org
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG= 713560 

Review-Url: https://codereview.chromium.org/2827233002
Cr-Commit-Position: refs/heads/master@{#466063}

[modify] https://crrev.com/b650cb773adbf72f3f731e0bc56e22d55c521391/components/exo/shell_surface.cc
[modify] https://crrev.com/b650cb773adbf72f3f731e0bc56e22d55c521391/components/exo/shell_surface_unittest.cc

Project Member

Comment 14 by bugdroid1@chromium.org, Apr 20 2017

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

commit 29b3bfe7c7371600d717d66502307b1e676d7c2c
Author: Victor Hsieh <victorhsieh@google.com>
Date: Thu Apr 20 18:23:49 2017

Revert of Do not hide shadow underlay for max/fullscreen state even if the client disables the shadow. (patchset #3 id:40001 of https://codereview.chromium.org/2820493004/ )

Reason for revert:
This change broke basic WM interaction of ARC and caused browser crash.  https://crbug.com/713560 

Original issue's description:
> Do not hide shadow underlay for max/fullscreen state even if the client disables the shadow.
>
> During the state transition, ARC preserves windows being deleted,
> however the main window is deleted (thus, window frame is empty) which disables the shadow.
>
> BUG= 711514 
> TEST=covered by unit tests.
>  manual. Instal & start "Clash Royal", then F4 to toggle fullscreen.
>
> Review-Url: https://codereview.chromium.org/2820493004
> Cr-Commit-Position: refs/heads/master@{#464819}
> Committed: https://chromium.googlesource.com/chromium/src/+/ae72f92d7caba6d32ba2a11f32943f29af15c3d8

TBR=reveman@chromium.org,oshima@chromium.org
BUG= 713560 

Review-Url: https://codereview.chromium.org/2827233002
Cr-Commit-Position: refs/heads/master@{#466063}
(cherry picked from commit b650cb773adbf72f3f731e0bc56e22d55c521391)

Review-Url: https://codereview.chromium.org/2829723004 .
Cr-Commit-Position: refs/branch-heads/3071@{#90}
Cr-Branched-From: a106f0abbf69dad349d4aaf4bcc4f5d376dd2377-refs/heads/master@{#464641}

[modify] https://crrev.com/29b3bfe7c7371600d717d66502307b1e676d7c2c/components/exo/shell_surface.cc
[modify] https://crrev.com/29b3bfe7c7371600d717d66502307b1e676d7c2c/components/exo/shell_surface_unittest.cc

Project Member

Comment 15 by bugdroid1@chromium.org, Apr 20 2017

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

commit f053a5346cc243578179e26b0331f5e0f6fe4316
Author: Victor Hsieh <victorhsieh@google.com>
Date: Thu Apr 20 19:59:40 2017

Revert of Do not hide shadow underlay for max/fullscreen state even if the client disables the shadow. (patchset #3 id:40001 of https://codereview.chromium.org/2820493004/ )

Reason for revert:
This change broke basic WM interaction of ARC and caused browser crash.  https://crbug.com/713560 

Original issue's description:
> Do not hide shadow underlay for max/fullscreen state even if the client disables the shadow.
>
> During the state transition, ARC preserves windows being deleted,
> however the main window is deleted (thus, window frame is empty) which disables the shadow.
>
> BUG= 711514 
> TEST=covered by unit tests.
>  manual. Instal & start "Clash Royal", then F4 to toggle fullscreen.
>
> Review-Url: https://codereview.chromium.org/2820493004
> Cr-Commit-Position: refs/heads/master@{#464819}
> Committed: https://chromium.googlesource.com/chromium/src/+/ae72f92d7caba6d32ba2a11f32943f29af15c3d8

TBR=reveman@chromium.org,oshima@chromium.org
BUG= 713560 

Review-Url: https://codereview.chromium.org/2827233002
Cr-Commit-Position: refs/heads/master@{#466063}
(cherry picked from commit b650cb773adbf72f3f731e0bc56e22d55c521391)

Review-Url: https://codereview.chromium.org/2836433002 .
Cr-Commit-Position: refs/branch-heads/3029@{#758}
Cr-Branched-From: 939b32ee5ba05c396eef3fd992822fcca9a2e262-refs/heads/master@{#454471}

[modify] https://crrev.com/f053a5346cc243578179e26b0331f5e0f6fe4316/components/exo/shell_surface.cc
[modify] https://crrev.com/f053a5346cc243578179e26b0331f5e0f6fe4316/components/exo/shell_surface_unittest.cc

Labels: -Hotlist-Merge-Approved -merge-merged-3029 -merge-merged-3071 Merge-Request-58 Merge-Request-59
Status: Started (was: Fixed)
Project Member

Comment 17 by sheriffbot@chromium.org, Apr 27 2017

Labels: -Merge-Request-59 Merge-Review-59 Hotlist-Merge-Review
This bug requires manual review: Reverts referenced in bugdroid comments after merge request.
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), gkihumba@(ChromeOS), Abdul Syed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 18 by sheriffbot@chromium.org, Apr 27 2017

Labels: -Merge-Request-58 Merge-Review-58
This bug requires manual review: Request affecting a post-stable build
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), bhthompson@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
bhthompson@, I tested this with MNC arc, and worked. I'll test this on m58 and update the bug.
Labels: -Hotlist-Merge-Review -Merge-Review-58 Merge-Approved-58
Consider this merge approved for 58 if you are going to verify it.
Tested & verified this with m58 + NYC on caroline and m58 + MNC on minnie.
Project Member

Comment 22 by bugdroid1@chromium.org, Apr 28 2017

Labels: -merge-approved-58 merge-merged-3029
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c6c1349fb73e21bef5db5f9d87fb4026b9f1bfdf

commit c6c1349fb73e21bef5db5f9d87fb4026b9f1bfdf
Author: Mitsuru Oshima <oshima@chromium.org>
Date: Fri Apr 28 15:34:14 2017

Do not hide shadow underlay for max/fullscreen state even if the client disables the shadow.

* Make sure resets all shadow layers when the switching between deprecated shadow.

* Don't try to show the window that is already visible.

This is reland of crrev.com/2820493004 with a fix for  crbug.com/713560 

R=reveman@chromium.org
BUG= 711514 
TEST=covered by unit tests.
 manual. Instal & start "Clash Royal", then F4 to toggle fullscreen.
 Tested on MNC and NYC devices as well.

Review-Url: https://codereview.chromium.org/2837903002
Cr-Original-Commit-Position: refs/heads/master@{#467485}
Committed: https://chromium.googlesource.com/chromium/src/+/27b41b0b6fb0317aa944f2bdf7e4879628aab83c
Review-Url: https://codereview.chromium.org/2845053004 .
Cr-Commit-Position: refs/branch-heads/3029@{#780}
Cr-Branched-From: 939b32ee5ba05c396eef3fd992822fcca9a2e262-refs/heads/master@{#454471}

[modify] https://crrev.com/c6c1349fb73e21bef5db5f9d87fb4026b9f1bfdf/components/exo/shell_surface.cc
[modify] https://crrev.com/c6c1349fb73e21bef5db5f9d87fb4026b9f1bfdf/components/exo/shell_surface.h
[modify] https://crrev.com/c6c1349fb73e21bef5db5f9d87fb4026b9f1bfdf/components/exo/shell_surface_unittest.cc

Cc: gkihumba@chromium.org
Labels: M-59
gkihumba@ for m59
ping?
Labels: Hotlist-Merge-Review
Labels: Merge-Approved-59
Status: Fixed (was: Started)
Project Member

Comment 28 by sheriffbot@chromium.org, May 8 2017

Cc: bhthompson@google.com gkihumba@google.com
This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible!

If all merges have been completed, please remove any remaining Merge-Approved labels from this issue.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Labels: -M58 M-58
Cc: -gkihumba@chromium.org
Labels: -Merge-Review-59
Labels: -Hotlist-Merge-Review

Comment 32 by dchan@chromium.org, Jan 22 2018

Status: Archived (was: Fixed)
Labels: -Merge-Approved-59

Sign in to add a comment