New issue
Advanced search Search tips

Issue 655665 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Oct 2016
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug-Regression



Sign in to add a comment

34.9%-170.5% regression in page_cycler_v2.tough_layout_cases at 424588:424644

Project Member Reported by lanwei@chromium.org, Oct 13 2016

Issue description

See the link to graphs below.
 

Comment 3 by lanwei@chromium.org, Oct 13 2016

Cc: -lanwei@google.com lanwei@chromium.org
Project Member

Comment 4 by 42576172...@developer.gserviceaccount.com, Oct 13 2016

Cc: eugene...@chromium.org
Owner: eugene...@chromium.org

=== 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!
Cc: tapted@chromium.org
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?
Owner: tapted@chromium.org
Status: Assigned (was: Untriaged)
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 ).

Comment 7 by tapted@chromium.org, Oct 21 2016

Status: Started (was: Assigned)
I suspect https://codereview.chromium.org/2442573003/ will fix this. See related  Issue 655112 
Project Member

Comment 8 by bugdroid1@chromium.org, 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

Comment 9 by tapted@chromium.org, Oct 25 2016

Status: Fixed (was: Started)
Looks like that's doing the trick, but still waiting on some of the graphs to catch up.
Screen Shot 2016-10-26 at 10.32.47 AM.png
296 KB View Download
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