Issue metadata
Sign in to add a comment
|
34.9%-170.5% regression in page_cycler_v2.tough_layout_cases at 424588:424644 |
||||||||||||||||||||
Issue descriptionSee the link to graphs below.
,
Oct 13 2016
Started bisect job https://chromeperf.appspot.com/buildbucket_job_status/8998903767501482768
,
Oct 13 2016
,
Oct 13 2016
=== Auto-CCing suspected CL author eugenebut@chromium.org === Hi eugenebut@chromium.org, the bisect results pointed to your CL below as possibly causing a regression. Please have a look at this info and see whether your CL be related. ===== BISECT JOB RESULTS ===== Status: completed ===== SUSPECTED CL(s) ===== Subject : [Mac] Avoid "adding unknown subview" warning. Author : eugenebut Commit description: Chrome Mac has added a subview above the window's content view, which the Appkit warns about at runtime. Presumably, doing so may break in a future macOS release. This cl adds Chrome's top-level subview to the window's content view, which is what the Appkit wants. The change is done only for macOS 10.11 and later. Using correct view hierarchy breaks a few things which had to be fixed: 1.) Window buttons are now placed in NSToolbarContainerView which height is 22 points and to move buttons down the whole NSToolbarContainerView should be moved down. NSToolbarContainerView is not movable via NSLayoutConstraints and simply resized every time its bounds changed. 2.) Window buttons are not movable anymore by changing their frame. Their position is controlled by layout constraints received from autoresizing mask and buttons can be moved via NSLayoutConstraints. 3.) The hight of browserFrameViewPaintHeight should be the same as tab strip height in order to paint content area correctly. 4.) NSVisualEffect view for titlebar should be hidden in fullscreen. With old view hierarchy NSVisualEffectView was moved offscreen for some reason. Now it is always shown on the titlebar. 5.) Window does not update tracking area which highlights window buttons when those buttons are moved (rdar://28535344). As a workaround addTrackingArea: method is swizzled to prevent unnecessarily adding an extra tracking area. Swizzling is necessary because OS does not provide any signals that tracking area was updated ( NSViewDidUpdateTrackingAreasNotification is called before adding a tracking area, not after). 6.) Windows w/o tab strip should not use NSFullSizeContentViewWindowMask style mask, otherwise they will show up without toolbar. 7.) Tab should not allow moving window when dragged. This is done by overriding _opaqueRectForWindowMoveWhenInTitlebar (AppKit follows the same approach for NSButton, to prevent it from moving the window). BUG= 605219 Review-Url: https://codereview.chromium.org/2404783002 Cr-Commit-Position: refs/heads/master@{#424609} Commit : 42c6683729d8e049fdcc89e552bdcff5fb337227 Date : Wed Oct 12 00:19:27 2016 ===== TESTED REVISIONS ===== Revision Mean Std Dev N Good? chromium@424601 407.559 298.164 12 good chromium@424605 352.393 201.501 8 good chromium@424607 371.928 249.043 8 good chromium@424608 372.608 254.222 8 good chromium@424609 819.294 83.9567 8 bad <-- chromium@424617 786.903 191.815 12 bad Bisect job ran on: mac_retina_perf_bisect Bug ID: 655665 Test Command: src/tools/perf/run_benchmark -v --browser=release --output-format=chartjson --upload-results --pageset-repeat=1 --also-run-disabled-tests page_cycler_v2.tough_layout_cases Test Metric: timeToFirstContentfulPaint_avg/pcv1-cold/http___natunkantha.com Relative Change: 62.88% Score: 99.8 Buildbot stdio: http://build.chromium.org/p/tryserver.chromium.perf/builders/mac_retina_perf_bisect/builds/1739 Job details: https://chromeperf.appspot.com/buildbucket_job_status/8998903767501482768 Not what you expected? We'll investigate and get back to you! https://chromeperf.appspot.com/bad_bisect?try_job_id=5579681833680896 | O O | Visit http://www.chromium.org/developers/speed-infra/perf-bug-faq | X | for more information addressing perf regression bugs. For feedback, | / \ | file a bug with component Tests>AutoBisect. Thank you!
,
Oct 13 2016
My CL changes view hierarchy, and it can potentially affect window resizing performance. lanwei@, do you know if we do any window resizing in this test?
,
Oct 14 2016
Trent, could you please find a new owner for this bug as I'm switching back to iOS. Looks like viewport is actually resized, even if window frame remains unchanged. Could be the same root cause as in BrowserTest.GetSizeForNewRenderView failure ( crbug.com/655112 ).
,
Oct 21 2016
I suspect https://codereview.chromium.org/2442573003/ will fix this. See related Issue 655112
,
Oct 25 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/ba268401a975c321f1e15ae3a4776fd3c60e268e commit ba268401a975c321f1e15ae3a4776fd3c60e268e Author: tapted <tapted@chromium.org> Date: Tue Oct 25 06:41:26 2016 Mac: Don't allow RenderWidgetHostViewCocoa to participate in autolayout. WebContentsViewCocoa has a hook to resize all subviews to match its size. The problem is that the size of a RenderWidgetHostViewCocoa is allowed to get out of sync with its superview while waiting for a WebContents paint to be committed. If an autolayout is triggered while waiting for that commit, the WebContents thinks it's been resized and spawns a new paint. Since r424609, for 10.11+, Chrome stopped replacing the NSThemeFrame (which AppKit does not support) and instead started using NSFullSizeContentViewWindowMask. This seems to opt the window into additional autolayout triggers, coming from CoreAnimation. This can engage the code to "re-sync" the sizes of the RenderWidgetHostViewCocoa and WebContentsViewCocoa when it wasn't done previously. To fix, "re-sync" sizes in an override of -setFrameSize: rather than -resizeSubviewsWithOldSize:. This ensures a re-sync only occurs when the size of the WebContentsViewCocoa changes. BUG= 655112 , 655665 , 264207 TBR=sky@chromium.org Review-Url: https://codereview.chromium.org/2442573003 Cr-Commit-Position: refs/heads/master@{#427290} [modify] https://crrev.com/ba268401a975c321f1e15ae3a4776fd3c60e268e/chrome/browser/ui/browser_browsertest.cc [modify] https://crrev.com/ba268401a975c321f1e15ae3a4776fd3c60e268e/content/browser/web_contents/web_contents_view_mac.mm
,
Oct 25 2016
Looks like that's doing the trick, but still waiting on some of the graphs to catch up.
,
Oct 26 2016
Note one of the graphs was a Linux regression which couldn't have been attributed to r424609 since it is Mac only. I've bumped that graph to Issue 659510 . |
|||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||
Comment 1 by lanwei@chromium.org
, Oct 13 2016