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

Issue 156982 link

Starred by 1 user

Issue metadata

Status: Verified
Owner:
Closed: Nov 2012
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 1
Type: Bug-Regression



Sign in to add a comment

Black line in the tab bar when you reset to default theme

Project Member Reported by sebmarchand@chromium.org, Oct 20 2012

Issue description

Version: 23.0.1271.40 beta-m
OS: Windows 7 64 bits SP1 - Aero activated

What steps will reproduce the problem?
1. Open Chrome.
2. Change the theme to one with a dark tab bar.
3. Return to settings and choose "Reset to default theme".

What is the expected output? What do you see instead?

The tab bar should be the default one but a black line is present (see the screenshot). Switching to another windows and come back to this one fix the problem.
 
UI_Tab_Bar_Line.png
14.6 KB View Download
Labels: -Pri-2 Pri-3
Labels: Feature-Themes
Labels: ReleaseBlock-Beta
I'm not able to reproduce it on Chrome 22 (official channel). 

Comment 4 by dharani@google.com, Oct 23 2012

Labels: Mstone-23 Action-BisectNeeded
I'm not sure if its a release blocker. I'll leave it to Karen.

Comment 5 by laforge@google.com, Oct 23 2012

Labels: -Type-Bug -Pri-3 -ReleaseBlock-Beta -Mstone-23 Type-Regression Pri-1 Mstone-24
We're not going to block beta/stable for a P3.  If there is a reasonable fix, will consider merge for 23.  

Comment 6 Deleted

Cc: karen@chromium.org ligim...@chromium.org
Labels: -Mstone-24 Mstone-23 ReleaseBlock-Stable
Cc: ashej...@chromium.org
Labels: -Pri-1 -ReleaseBlock-Stable Pri-3
Anant.. The bisect seems to be incorrect . Pls try again . Also provide the screenshot. Not happening for me in the canary .
I still get it in the canary: 24.0.1307.0 (Official Build 164023) canary
screenshot_canary.png
126 KB View Download
Hello All,

Apologies for the initial confusion below is the correct bisect info:-
Here are the Good and bad builds details:-
Last good build:-23.0.1247.1
First bad build:-23.0.1251.0.
P.S:- There are no Window builds available between these two builds on official builds and chrome signing.


You are probably looking for a change made after 153998 (known good), but no lat
er than 154008 (first known bad).
CHANGELOG URL:
  http://build.chromium.org/f/chromium/perf/dashboard/ui/changelog.html?url=/tru
nk/src&range=153998%3A154008

Do let me know if any more information is required from my side.

Thanks & Regards,
Anant

Comment 12 by kareng@google.com, Oct 29 2012

Labels: -Pri-3 Pri-1

Comment 13 by msw@chromium.org, Oct 29 2012

Cc: sky@chromium.org ben@chromium.org
Owner: msw@chromium.org
Status: Assigned
I can repro on Canary, I'll take a look; but feel free to grab this, Ben or Scott.

Comment 14 by msw@chromium.org, Oct 29 2012

Note that repro may require re-sizing the browser window while the theme is installed.

Comment 15 by msw@chromium.org, Oct 29 2012

Here's an example theme that seems to repro consistently with that extra step:
https://chrome.google.com/webstore/detail/colorfull-sun-set/iknflcjkkahjgichcidlfcalplplegii

Comment 16 by msw@chromium.org, Oct 30 2012

Status: Started
I have a local workaround by calling BrowserFrameWin::UpdateDWMFrame() after (instead of, or in addition to the call before) NativeWidgetWin::HandleFrameChanged() in BrowserFrameWin::HandleFrameChanged().
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/ui/views/frame/browser_frame_win.cc&l=179

Ben reversed the call order and added a comment indicating the necessity of that order in http://crrev.com/29164 (though the CL description notes some uncertainty).

I'll see if there's a bug in the underlying calls. Otherwise, duplicating or swapping the UpdateDWMFrame() order may be a suitable workaround (but may also introduce regressions).

Track progress at https://codereview.chromium.org/11273107/
Ben, if you have any thoughts, I'm all ears!

Comment 17 by msw@chromium.org, Oct 31 2012

So, the problem is that when switching from an opaque frame to a glass frame: In BrowserFrameWin::HandleFrameChanged() the call to UpdateDWMFrame() uses stale tabstrip bounds from OpaqueBrowserFrameView to set the glass inset (to 45 pixels on top). Then, the NativeWidgetWin::HandleFrameChanged() call actually swaps in the new GlassBrowserFrameView with a larger top glass inset (49 pixels) but never actually updates the glass bounds.

Swapping the call order and adding an extra Layout+SchedulePaint works without any immediately apparent regressions.
That said, this is a work in progress, and I'll need to do more thorough testing of the issues mentioned in http://crrev.com/29164

Additionally, this code is pretty convoluted/scattered; it seems like BrowserView::UserChangedTheme() should immediately swap the BrowserFrameView, call InvalidateLayout(), then propagate FrameTypeChanged/HandleFrameChanged to adjust the window's glass insets, etc.
Project Member

Comment 18 by bugdroid1@chromium.org, Nov 1 2012

The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=165265

------------------------------------------------------------------------
r165265 | msw@google.com | 2012-10-31T23:57:32.195785Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/ui/views/frame/browser_frame_win.cc?r1=165265&r2=165264&pathrev=165265

Fix BrowserFrameWin artifact on opaque to glass frame changes.

The problem when switching from an opaque frame to a glass frame:
Calling UpdateDWMFrame() first uses stale glass inset values.
(from the old OpaqueBrowserFrameView's tabstrip bounds, yielding 45px on top)
HandleFrameChanged() then sets the new GlassBrowserFrameView with a larger top inset.
(but doesn't update the dwm glass inset until the next UpdateDWMFrame, for 49px on top)

Swapping the call order works without any apparent regressions.
(this puts the new frame in place before updating the dwm glass insets)
See http://crrev.com/29164 for a contradictory change that may be outdated.

Also, use the more convenient IsFullscreen() call, no behavior change.

BUG= 156982 
TEST=No black bar switching from theme->glass chrome on Windows; no regressions.
R=ben@chromium.org

Review URL: https://codereview.chromium.org/11273107
------------------------------------------------------------------------

Comment 19 by msw@chromium.org, Nov 1 2012

Labels: -Mstone-23 Mstone-24 Merge-Requested
That CL fixes the issue for me, please help verify once it hits canary.
The bug is too low in severity (I'd say P2) for me to recommend merge to M23 this late.
I'd consider merging to M24, (more time to verify, etc.), but even that isn't critical.
Labels: -Merge-Requested Merge-Approved
Please merge it in 1312 branch.
Project Member

Comment 21 by bugdroid1@chromium.org, Nov 2 2012

Labels: -Merge-Approved merge-merged-1312
The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=165559

------------------------------------------------------------------------
r165559 | msw@chromium.org | 2012-11-01T23:59:24.508421Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/branches/1312/src/chrome/browser/ui/views/frame/browser_frame_win.cc?r1=165559&r2=165558&pathrev=165559

Merge 165265 - Fix BrowserFrameWin artifact on opaque to glass frame changes.

The problem when switching from an opaque frame to a glass frame:
Calling UpdateDWMFrame() first uses stale glass inset values.
(from the old OpaqueBrowserFrameView's tabstrip bounds, yielding 45px on top)
HandleFrameChanged() then sets the new GlassBrowserFrameView with a larger top inset.
(but doesn't update the dwm glass inset until the next UpdateDWMFrame, for 49px on top)

Swapping the call order works without any apparent regressions.
(this puts the new frame in place before updating the dwm glass insets)
See http://crrev.com/29164 for a contradictory change that may be outdated.

Also, use the more convenient IsFullscreen() call, no behavior change.

BUG= 156982 
TEST=No black bar switching from theme->glass chrome on Windows; no regressions.
R=ben@chromium.org

Review URL: https://codereview.chromium.org/11273107

TBR=msw@google.com
Review URL: https://codereview.chromium.org/11366054
------------------------------------------------------------------------

Comment 22 by msw@chromium.org, Nov 2 2012

Status: Fixed
Status: Verified
Hello All,

Verified this issue on latest canary version "25.0.1314.1" and not able to reproduce the same. Hence marking as Verified.
Attached are the screen shot fyr.
Thanks,
Anant
156928_Retest1.jpg
102 KB View Download
156928_Retest2.jpg
90.1 KB View Download
Project Member

Comment 24 by bugdroid1@chromium.org, Mar 9 2013

Labels: -Type-Regression -Area-UI -Feature-Themes -Mstone-24 Type-Bug-Regression Cr-UI-Browser-Themes M-24 Cr-UI

Sign in to add a comment