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

Issue 749186 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Aug 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Mac
Pri: 1
Type: Bug

Blocking:
issue 748242



Sign in to add a comment

unit_tests FramedBrowserWindowTest.WindowWidgetLocation fails on 10.13

Project Member Reported by rsesek@chromium.org, Jul 26 2017

Issue description

Chrome Version: 5f9df04869c13d09182e36ad8f1c7192e0389214
OS: macOS 10.13 High Sierra

What steps will reproduce the problem?
(1) Run unit_tests on 10.13
(2) FramedBrowserWindowRTLTest.WindowWidgetLocation and FramedBrowserWindowTest.WindowWidgetLocation fail

FramedBrowserWindowTest.WindowWidgetLocation (run #1):
[ RUN      ] FramedBrowserWindowTest.WindowWidgetLocation
2017-07-25 22:34:52.927 unit_tests[72078:1632040] *** WARNING: Textured window <FramedBrowserWindow: 0x7fd42b592740> is getting an implicitly transparent titlebar. This will break when linking against newer SDKs. Use NSWindow's -titlebarAppearsTransparent=YES instead.
../../chrome/browser/ui/cocoa/framed_browser_window_unittest.mm:127: Failure
      Expected: NSMaxY(closeBoxFrame)
      Which is: 1174
To be equal to: NSMaxY(windowBounds) - kFramedWindowButtonsWithoutTabStripOffsetFromTop
      Which is: 596
../../chrome/browser/ui/cocoa/framed_browser_window_unittest.mm:140: Failure
      Expected: NSMaxY(miniaturizeFrame)
      Which is: 1174
To be equal to: NSMaxY(windowBounds) - kFramedWindowButtonsWithoutTabStripOffsetFromTop
      Which is: 596
[  FAILED  ] FramedBrowserWindowTest.WindowWidgetLocation (17 ms)


What is the expected result?
Test passes

What happens instead?
Test fails

Please use labels and text to provide additional information.

Job: https://luci-milo.appspot.com/buildbot/chromium.fyi/Chromium%20Mac%2010.13/6
Logs: https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Fchromium.fyi%2FChromium_Mac_10.13%2F6%2F%2B%2Frecipes%2Fsteps%2Funit_tests%2F0%2Fstdout


For graphics-related bugs, please copy/paste the contents of the about:gpu
page at the end of this report.

 

Comment 1 by tapted@chromium.org, Jul 28 2017

That warning is interesting too

*** WARNING: Textured window <FramedBrowserWindow: 0x7fd42b592740> is getting an implicitly transparent titlebar. This will break when linking against newer SDKs. Use NSWindow's -titlebarAppearsTransparent=YES instead.

I see it for most tests involving NativeWidgetMacNSWindow as well
Labels: M-62
Labels: -Pri-2 Pri-1
sdy@ - will you please take a look at this and at least figure out why the test is failing?

Comment 4 by sdy@chromium.org, Aug 4 2017

Cc: shrike@chromium.org
Status: Started (was: Assigned)
This test exercises code that moves the window buttons in non-tabbed browser windows:

https://chromium.googlesource.com/chromium/src/+/2ae3701ed1f5331e0cbc69bb2bd375c4d3375fe9/chrome/browser/ui/cocoa/framed_browser_window.mm#167

This seems to be a holdover from when Chrome instantiated its own window buttons, and the constants are supposed to put them in the same place as AppKit (see  issue 24539  / r28823) but actually put them 1pt off. The code path hasn't been functional since at least 10.12, when the window controls started to ignore attempts to move them reentrantly (e.g. from a frame changed notification, like Chrome does). A workaround was added in r424609, but it only affects tabbed windows, so non-tabbed windows get their buttons moved at init time, but AppKit steps in and puts them back as soon as the window's size changes. This always happens to browser windows before they're shown:

https://chromium.googlesource.com/chromium/src/+/2ae3701ed1f5331e0cbc69bb2bd375c4d3375fe9/chrome/browser/ui/cocoa/browser_window_controller.mm#337

…but, since the test creates a window in isolation and never resizes it, the buttons are where the button-tweaking code put them, and the test passes. 10.13 changed the structure of titled windows a little bit. 10.12:

    [ D A  O  P  U ] h=--- v=--- NSThemeFrame 0x7fb3c1f5ced0 f=(0,0,800,600) b=(-) TIME drawRect: min/mean/max 0.00/0.00/0.00 ms
      [ D AF       U ] h=--- v=--- _NSThemeCloseWidget 0x7fb3c1f5d470 "Button" f=(8,580,14,16) b=(-) TIME drawRect: min/mean/max 0.00/0.00/0.00 ms
      [ D AF       U ] h=--- v=--- _NSThemeZoomWidget 0x7fb3c1f5e150 "Button" f=(48,580,14,16) b=(-) TIME drawRect: min/mean/max 0.00/0.00/0.00 ms
      [ D AF       U ] h=--- v=--- _NSThemeWidget 0x7fb3c1f5e800 "Button" f=(28,580,14,16) b=(-) TIME drawRect: min/mean/max 0.00/0.00/0.00 ms
      [ D A        U ] h=--- v=--- NSView 0x7fb3c1f5f2c0 f=(0,0,800,578) b=(-) TIME drawRect: min/mean/max 0.00/0.00/0.00 ms
    A=autoresizesSubviews, C=canDrawConcurrently, D=needsDisplay, F=flipped, G=gstate, H=hidden (h=by ancestor), L=needsLayout (l=child needsLayout), U=needsUpdateConstraints (u=child needsUpdateConstraints), O=opaque, P=preservesContentDuringLiveResize, S=scaled/rotated, W=wantsLayer (w=ancestor wantsLayer), V=needsVibrancy (v=allowsVibrancy), #=has surface

10.13:

   [ D A  O  P  U ] h=--- v=--- NSThemeFrame 0x7fd3af0abdb0 f=(0,0,800,600) b=(-)
     [ D A        U ] h=--- v=--- NSView 0x7fd3af135f70 f=(0,0,800,578) b=(-)
     [ D A      W U ] h=-&- v=--- NSTitlebarContainerView 0x7fd3af0ac330 f=(0,578,800,22) b=(-) => <_NSViewBackingLayer: 0x7fd3af0ac5b0>
       [ D A      W U ] h=-&- v=-&- NSTitlebarView 0x7fd3af0ac900 f=(0,0,800,22) b=(-) => <_NSViewBackingLayer: 0x7fd3af0acde0>
         [   AF       U ] h=--- v=--- _NSThemeCloseWidget 0x7fd3af133ef0 "Button" f=(8,580,14,16) b=(-)
         [   AF       U ] h=--- v=--- _NSThemeZoomWidget 0x7fd3af12a130 "Button" f=(48,580,14,16) b=(-)
         [   AF       U ] h=--- v=--- _NSThemeWidget 0x7fd3af122550 "Button" f=(28,580,14,16) b=(-)
         [ D AF   V W U ] h=--- v=&-- NSTextField 0x7fd3af128130 f=(395,3,10,17) b=(-) => <NSTextLayer: 0x7fd3af125ec0>
   A=autoresizesSubviews, C=canDrawConcurrently, D=needsDisplay, F=flipped, G=gstate, H=hidden (h=by ancestor), L=needsLayout (l=child needsLayout), U=needsUpdateConstraints (u=child needsUpdateConstraints), O=opaque, P=preservesContentDuringLiveResize, S=scaled/rotated, W=wantsLayer (w=ancestor wantsLayer), V=needsVibrancy (v=allowsVibrancy), #=has surface

The button tweaking for non-tabbed windows doesn't account for the NSTitlebarContainerView superview, so the y offsets are based on the size of the window and the buttons are placed totally outside the bounds of the window. Users never see this (because AppKit moves the buttons back when the window is resized by BWC), but it makes the test fail. I'm about to upload a CL which:

- Stops trying to position the buttons in non-tabbed windows.
- Adjusts the constants by 1pt, so that the tests expects the buttons to be where AppKit puts them (i.e. where users see them now).

(I'm not doing any more cleanup because crrev/c/562603 removes all of this code and I want to try to land it soon.)
Project Member

Comment 6 by bugdroid1@chromium.org, Aug 4 2017

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

commit 4681197f97a2e4fedd7e160563b888f71c2adc17
Author: Sidney San Martín <sdy@chromium.org>
Date: Fri Aug 04 16:26:51 2017

Don't try to move non-tabbed windows' buttons.

unit_tests/FramedBrowserWindowTest.WindowWidgetLocation was failing on
10.13. It turns out that the button positioning code produces the wrong
results on 10.13 (see bug for details), but only affected the test
because at some point window buttons started ignoring frame changes in
NSViewFrameDidChangeNotification handlers, which this code does.

The constants were 1pt off from the stock positions, and  crbug.com/24539 
suggests that they matched in older macOS. Since users already see the
buttons where AppKit puts them instead of where these constants would
put them, this CL:

- Changes the constants which the test checks to match AppKit.
- Stops trying to position non-tabbed windows' buttons.

Bug:  749186 
Change-Id: I54a29659ba85b4e14ebd0fcd5eab47cd88071a9c
Reviewed-on: https://chromium-review.googlesource.com/602370
Reviewed-by: Robert Sesek <rsesek@chromium.org>
Commit-Queue: Sidney San Martín <sdy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#492049}
[modify] https://crrev.com/4681197f97a2e4fedd7e160563b888f71c2adc17/chrome/browser/ui/cocoa/framed_browser_window.h
[modify] https://crrev.com/4681197f97a2e4fedd7e160563b888f71c2adc17/chrome/browser/ui/cocoa/framed_browser_window.mm

Project Member

Comment 7 by bugdroid1@chromium.org, Aug 4 2017

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

commit ed16704c186cb6b7ec873100ddbdee02a676b1a9
Author: Joe Downing <joedow@chromium.org>
Date: Fri Aug 04 18:34:39 2017

Revert "Don't try to move non-tabbed windows' buttons."

This reverts commit 4681197f97a2e4fedd7e160563b888f71c2adc17.

Reason for revert: Culprit CL for Mac 10.12 browser_side_navigation_tests failures:
https://uberchromegw.corp.google.com/i/chromium.mac/builders/Mac10.12%20Tests

Original change's description:
> Don't try to move non-tabbed windows' buttons.
> 
> unit_tests/FramedBrowserWindowTest.WindowWidgetLocation was failing on
> 10.13. It turns out that the button positioning code produces the wrong
> results on 10.13 (see bug for details), but only affected the test
> because at some point window buttons started ignoring frame changes in
> NSViewFrameDidChangeNotification handlers, which this code does.
> 
> The constants were 1pt off from the stock positions, and  crbug.com/24539 
> suggests that they matched in older macOS. Since users already see the
> buttons where AppKit puts them instead of where these constants would
> put them, this CL:
> 
> - Changes the constants which the test checks to match AppKit.
> - Stops trying to position non-tabbed windows' buttons.
> 
> Bug:  749186 
> Change-Id: I54a29659ba85b4e14ebd0fcd5eab47cd88071a9c
> Reviewed-on: https://chromium-review.googlesource.com/602370
> Reviewed-by: Robert Sesek <rsesek@chromium.org>
> Commit-Queue: Sidney San Martín <sdy@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#492049}

TBR=rsesek@chromium.org,sdy@chromium.org

Change-Id: I5d1681378dd7986f666843178dda4972c35acb22
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  749186 
Reviewed-on: https://chromium-review.googlesource.com/602667
Commit-Queue: Joe Downing <joedow@chromium.org>
Reviewed-by: Joe Downing <joedow@chromium.org>
Cr-Commit-Position: refs/heads/master@{#492081}
[modify] https://crrev.com/ed16704c186cb6b7ec873100ddbdee02a676b1a9/chrome/browser/ui/cocoa/framed_browser_window.h
[modify] https://crrev.com/ed16704c186cb6b7ec873100ddbdee02a676b1a9/chrome/browser/ui/cocoa/framed_browser_window.mm

Project Member

Comment 8 by bugdroid1@chromium.org, Aug 8 2017

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

commit b78af9629d8f565557c125297b7b47603dcb89d4
Author: Sidney San Martín <sdy@chromium.org>
Date: Tue Aug 08 01:19:54 2017

Don't override the positions of non-tabbed browser windows' buttons.

unit_tests/FramedBrowserWindowTest.WindowWidgetLocation was failing on
10.13. It turns out that the button positioning code produces the wrong
results on 10.13 (see bug for details), but only impacted the test
because at some point window buttons started ignoring frame changes in
NSViewFrameDidChangeNotification handlers, which this code does.

The constants were 1pt off from the default positions, and
 crbug.com/24539  suggests that they did match in older macOS. Since users
already see the buttons where AppKit puts them instead of where these
constants would put them, I'm changing the constants (which are now only
used by the test).

Also don't allow browser window buttons to flip when RTL is disabled; this was
breaking a test and currently behaves wrong (buttons are on the right for
non-tabbed windows, on the left for tabbed windows).

This re-lands r492049.

Bug:  749186 
Change-Id: Iad9738b592645597f1e6e5cd4cfbaf687b1ba23d
Reviewed-on: https://chromium-review.googlesource.com/604692
Reviewed-by: Trent Apted <tapted@chromium.org>
Commit-Queue: Sidney San Martín <sdy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#492489}
[modify] https://crrev.com/b78af9629d8f565557c125297b7b47603dcb89d4/chrome/browser/ui/cocoa/framed_browser_window.h
[modify] https://crrev.com/b78af9629d8f565557c125297b7b47603dcb89d4/chrome/browser/ui/cocoa/framed_browser_window.mm

Comment 9 by sdy@chromium.org, Aug 14 2017

Status: Fixed (was: Started)

Sign in to add a comment