unit_tests FramedBrowserWindowTest.WindowWidgetLocation fails on 10.13 |
|||||
Issue descriptionChrome 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.
,
Aug 1 2017
,
Aug 2 2017
sdy@ - will you please take a look at this and at least figure out why the test is failing?
,
Aug 4 2017
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.)
,
Aug 4 2017
,
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
,
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
,
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
,
Aug 14 2017
|
|||||
►
Sign in to add a comment |
|||||
Comment 1 by tapted@chromium.org
, Jul 28 2017