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

Issue 827444 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Apr 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 1
Type: Bug



Sign in to add a comment

Browser frame header is not laid out after signout with minimized window

Project Member Reported by warx@chromium.org, Mar 30 2018

Issue description

Repro steps:
(1) Minimize browser window
(2) Signout
(3) Signin
(4) observed browser frame header not laid out

Culprit CL: https://chromium-review.googlesource.com/c/chromium/src/+/967414
 
Screenshot 2018-03-29 at 5.34.43 PM.png
3.2 MB View Download

Comment 1 by warx@chromium.org, Apr 3 2018

Another observation: If in step (1), minimized browser window has gmail as active tab, then do step (2) and (3), it will first has frame header not laid out and then laid out when gmail navigation complete.
Project Member

Comment 2 by bugdroid1@chromium.org, Apr 11 2018

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

commit a34f3f9a59f75adb4b9b26a459813e11704bac5f
Author: Qiang Xu <warx@google.com>
Date: Wed Apr 11 02:37:08 2018

cros: Minimized use pre-minimied show state for caption button update

changes:
Previous CL: crrev.com/c/967414 has bug for session logged in reshowing
minimized browser window. This CL introduces another fix, which is
when window is minimized and update caption buttons, check if there
is pre-minimized window show state and if it exists, apply to that.

Bug:  827444 
Test: manual test, see bug fixed.
Change-Id: I0fe7d3a72fba87b2f5d93cd763e406f065e5ae8b
Reviewed-on: https://chromium-review.googlesource.com/987405
Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Qiang Xu <warx@google.com>
Cr-Commit-Position: refs/heads/master@{#549737}
[modify] https://crrev.com/a34f3f9a59f75adb4b9b26a459813e11704bac5f/ash/frame/custom_frame_view_ash.cc
[modify] https://crrev.com/a34f3f9a59f75adb4b9b26a459813e11704bac5f/ash/frame/default_frame_header.cc
[modify] https://crrev.com/a34f3f9a59f75adb4b9b26a459813e11704bac5f/ash/frame/default_frame_header.h
[modify] https://crrev.com/a34f3f9a59f75adb4b9b26a459813e11704bac5f/ash/frame/frame_header.h
[modify] https://crrev.com/a34f3f9a59f75adb4b9b26a459813e11704bac5f/ash/frame/header_view.cc
[modify] https://crrev.com/a34f3f9a59f75adb4b9b26a459813e11704bac5f/ash/frame/header_view.h
[modify] https://crrev.com/a34f3f9a59f75adb4b9b26a459813e11704bac5f/chrome/browser/ui/views/frame/browser_frame_header_ash.cc
[modify] https://crrev.com/a34f3f9a59f75adb4b9b26a459813e11704bac5f/chrome/browser/ui/views/frame/browser_frame_header_ash.h
[modify] https://crrev.com/a34f3f9a59f75adb4b9b26a459813e11704bac5f/chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc
[modify] https://crrev.com/a34f3f9a59f75adb4b9b26a459813e11704bac5f/chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.h
[modify] https://crrev.com/a34f3f9a59f75adb4b9b26a459813e11704bac5f/chrome/browser/ui/views/frame/browser_non_client_frame_view_ash_browsertest.cc

Comment 3 by warx@chromium.org, Apr 11 2018

Labels: Merge-Request-66
Project Member

Comment 4 by sheriffbot@chromium.org, Apr 11 2018

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

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

Comment 5 by josa...@google.com, Apr 11 2018

Labels: -Merge-Review-66 Merge-Approved-66

Comment 6 by warx@chromium.org, Apr 11 2018

Cc: osh...@chromium.org josa...@chromium.org
To merge the CL in #2, I had to merge https://chromium-review.googlesource.com/c/chromium/src/+/972677 first. The CL owner Oshima agrees the backporting.

Josafat@, can you approve the merge for https://chromium-review.googlesource.com/c/chromium/src/+/972677? Thanks!
hmm ok, let's confirm these both changes in canary and then merge if no issues/ side effects 
Project Member

Comment 8 by bugdroid1@chromium.org, Apr 11 2018

Labels: -merge-approved-66 merge-merged-3359
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/54c6313e1044fa708f9dc0d7c7a0c55b4249c052

commit 54c6313e1044fa708f9dc0d7c7a0c55b4249c052
Author: Qiang Xu <warx@google.com>
Date: Wed Apr 11 19:39:28 2018

m66 merge: Move the caption color handling code from WindowStateDelegate to CustomFrameViewAsh.

A client may set custom WindowStateDelegate, so the code should be
in the CustomFrameViewAsh.

BUG=b/33693796
BUG= 827444 
TEST=covered by unit tests.

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

(cherry picked from commit 7a0308de962197c75a33c52dbd6a584f044ea4c1)

Change-Id: Iccacddf8a0bce01ed1a3819c959e73d591a8d8ef
Reviewed-on: https://chromium-review.googlesource.com/972677
Commit-Queue: Mitsuru Oshima <oshima@chromium.org>
Reviewed-by: Tao Wu <wutao@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#545092}
Reviewed-on: https://chromium-review.googlesource.com/1007978
Reviewed-by: Qiang Xu <warx@google.com>
Cr-Commit-Position: refs/branch-heads/3359@{#686}
Cr-Branched-From: 66afc5e5d10127546cc4b98b9117aff588b5e66b-refs/heads/master@{#540276}
[modify] https://crrev.com/54c6313e1044fa708f9dc0d7c7a0c55b4249c052/ash/frame/caption_buttons/frame_caption_button_container_view.cc
[modify] https://crrev.com/54c6313e1044fa708f9dc0d7c7a0c55b4249c052/ash/frame/caption_buttons/frame_caption_button_container_view.h
[modify] https://crrev.com/54c6313e1044fa708f9dc0d7c7a0c55b4249c052/ash/frame/custom_frame_view_ash.cc
[modify] https://crrev.com/54c6313e1044fa708f9dc0d7c7a0c55b4249c052/ash/frame/custom_frame_view_ash.h
[modify] https://crrev.com/54c6313e1044fa708f9dc0d7c7a0c55b4249c052/ash/frame/custom_frame_view_ash_unittest.cc
[modify] https://crrev.com/54c6313e1044fa708f9dc0d7c7a0c55b4249c052/ash/frame/default_frame_header.cc
[modify] https://crrev.com/54c6313e1044fa708f9dc0d7c7a0c55b4249c052/ash/frame/header_view.cc

Project Member

Comment 9 by bugdroid1@chromium.org, Apr 13 2018

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

commit 209ba0cf9d1cd078a7e76456ae9b7645f013b442
Author: Qiang Xu <warx@google.com>
Date: Fri Apr 13 22:05:15 2018

Revert "m66 merge: Move the caption color handling code from WindowStateDelegate to CustomFrameViewAsh."

This reverts commit 54c6313e1044fa708f9dc0d7c7a0c55b4249c052.

Reason for revert: break m66 build

Original change's description:
> m66 merge: Move the caption color handling code from WindowStateDelegate to CustomFrameViewAsh.
> 
> A client may set custom WindowStateDelegate, so the code should be
> in the CustomFrameViewAsh.
> 
> BUG=b/33693796
> BUG= 827444 
> TEST=covered by unit tests.
> 
> TBR=oshima@chromium.org, wutao@chromium.org
> 
> (cherry picked from commit 7a0308de962197c75a33c52dbd6a584f044ea4c1)
> 
> Change-Id: Iccacddf8a0bce01ed1a3819c959e73d591a8d8ef
> Reviewed-on: https://chromium-review.googlesource.com/972677
> Commit-Queue: Mitsuru Oshima <oshima@chromium.org>
> Reviewed-by: Tao Wu <wutao@chromium.org>
> Cr-Original-Commit-Position: refs/heads/master@{#545092}
> Reviewed-on: https://chromium-review.googlesource.com/1007978
> Reviewed-by: Qiang Xu <warx@google.com>
> Cr-Commit-Position: refs/branch-heads/3359@{#686}
> Cr-Branched-From: 66afc5e5d10127546cc4b98b9117aff588b5e66b-refs/heads/master@{#540276}

TBR=oshima@chromium.org,wutao@chromium.org,warx@google.com

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

Bug: b/33693796, 827444
Change-Id: I1d3df56513c18bbf739e8490f0fe0fb395a6cfee
Reviewed-on: https://chromium-review.googlesource.com/1012861
Reviewed-by: Qiang Xu <warx@google.com>
Cr-Commit-Position: refs/branch-heads/3359@{#708}
Cr-Branched-From: 66afc5e5d10127546cc4b98b9117aff588b5e66b-refs/heads/master@{#540276}
[modify] https://crrev.com/209ba0cf9d1cd078a7e76456ae9b7645f013b442/ash/frame/caption_buttons/frame_caption_button_container_view.cc
[modify] https://crrev.com/209ba0cf9d1cd078a7e76456ae9b7645f013b442/ash/frame/caption_buttons/frame_caption_button_container_view.h
[modify] https://crrev.com/209ba0cf9d1cd078a7e76456ae9b7645f013b442/ash/frame/custom_frame_view_ash.cc
[modify] https://crrev.com/209ba0cf9d1cd078a7e76456ae9b7645f013b442/ash/frame/custom_frame_view_ash.h
[modify] https://crrev.com/209ba0cf9d1cd078a7e76456ae9b7645f013b442/ash/frame/custom_frame_view_ash_unittest.cc
[modify] https://crrev.com/209ba0cf9d1cd078a7e76456ae9b7645f013b442/ash/frame/default_frame_header.cc
[modify] https://crrev.com/209ba0cf9d1cd078a7e76456ae9b7645f013b442/ash/frame/header_view.cc

Project Member

Comment 10 by bugdroid1@chromium.org, Apr 17 2018

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

commit 4f232bb1a4fa032f807708b2f09abf5d9a3c630f
Author: Qiang Xu <warx@google.com>
Date: Tue Apr 17 05:24:16 2018

m66 merge: cros: Minimized use pre-minimied show state for caption button update

changes:
Previous CL: crrev.com/c/967414 has bug for session logged in reshowing
minimized browser window. This CL introduces another fix, which is
when window is minimized and update caption buttons, check if there
is pre-minimized window show state and if it exists, apply to that.

TBR=sky@chromium.org

(cherry picked from commit a34f3f9a59f75adb4b9b26a459813e11704bac5f)

Bug:  827444 
Test: manual test, see bug fixed.
Change-Id: I0fe7d3a72fba87b2f5d93cd763e406f065e5ae8b
Reviewed-on: https://chromium-review.googlesource.com/987405
Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Qiang Xu <warx@google.com>
Cr-Original-Commit-Position: refs/heads/master@{#549737}
Reviewed-on: https://chromium-review.googlesource.com/1014745
Reviewed-by: Qiang Xu <warx@google.com>
Cr-Commit-Position: refs/branch-heads/3359@{#726}
Cr-Branched-From: 66afc5e5d10127546cc4b98b9117aff588b5e66b-refs/heads/master@{#540276}
[modify] https://crrev.com/4f232bb1a4fa032f807708b2f09abf5d9a3c630f/ash/frame/custom_frame_view_ash.cc
[modify] https://crrev.com/4f232bb1a4fa032f807708b2f09abf5d9a3c630f/ash/frame/default_frame_header.cc
[modify] https://crrev.com/4f232bb1a4fa032f807708b2f09abf5d9a3c630f/ash/frame/default_frame_header.h
[modify] https://crrev.com/4f232bb1a4fa032f807708b2f09abf5d9a3c630f/ash/frame/frame_header.h
[modify] https://crrev.com/4f232bb1a4fa032f807708b2f09abf5d9a3c630f/ash/frame/header_view.cc
[modify] https://crrev.com/4f232bb1a4fa032f807708b2f09abf5d9a3c630f/ash/frame/header_view.h
[modify] https://crrev.com/4f232bb1a4fa032f807708b2f09abf5d9a3c630f/chrome/browser/ui/views/frame/browser_frame_header_ash.cc
[modify] https://crrev.com/4f232bb1a4fa032f807708b2f09abf5d9a3c630f/chrome/browser/ui/views/frame/browser_frame_header_ash.h
[modify] https://crrev.com/4f232bb1a4fa032f807708b2f09abf5d9a3c630f/chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc
[modify] https://crrev.com/4f232bb1a4fa032f807708b2f09abf5d9a3c630f/chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.h
[modify] https://crrev.com/4f232bb1a4fa032f807708b2f09abf5d9a3c630f/chrome/browser/ui/views/frame/browser_non_client_frame_view_ash_browsertest.cc

Comment 11 by warx@chromium.org, Apr 17 2018

Status: Fixed (was: Assigned)

Sign in to add a comment