Views: switching a tab causes 12 toolbar layouts |
||||||||||||||||
Issue descriptionWhich cascades into ~600 total view layouts. Each browser view layout causes 3 toolbar layouts. Switching a tab lays out the browser view 3 times: Once, because we want to (I think?) Twice to change the window title (which, at least on Mac ends up being a no-op for tabbed browsers) Add 3 more layouts caused by a browser actions container animation, which fires no matter what because it doesn't include the separator width in its desired width when deciding whether to animate or not, and therefore always animates. From a MacViews perspective, we can: - Fix the browser actions animation - Don't try to update the window title on tabbed browsers (on Mac-only? or all platforms?) - Find out if we can get around laying out the toolbar three times when the browser lays out - Find out if we can avoid laying out invisible decorations in the Omnibox (search keyword, keyword hint) - Probably some other things
,
Apr 24 2018
Re: browser actions container, this is actually due to the resize area not being accounted for. It goes away with the refreshed style (probably due to the addition of the avatar and separator?), so I'm not going to bother with it.
,
Apr 26 2018
,
Apr 26 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/5e0ca406d7893fa97928f8d013ac2bccefef21ff commit 5e0ca406d7893fa97928f8d013ac2bccefef21ff Author: Leonard Grey <lgrey@chromium.org> Date: Thu Apr 26 15:52:10 2018 Don't try to update window title on non-titled browsers This causes two unnecessary layouts of BrowserView, each of which causes three layouts of ToolbarView, each of which causes a whole bunch of things to happen. ChromeOS is excluded since even on a tabbed browser, it relies on the title to be updated to set the accessibility title. Bug: 835983 Change-Id: I4a62579f8fae394a6c11eb7c8632da176452b00d Reviewed-on: https://chromium-review.googlesource.com/1024828 Commit-Queue: Leonard Grey <lgrey@chromium.org> Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org> Cr-Commit-Position: refs/heads/master@{#554040} [modify] https://crrev.com/5e0ca406d7893fa97928f8d013ac2bccefef21ff/chrome/browser/ui/views/frame/browser_view.cc
,
Apr 30 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/1117510707037f759a77d17e797d734e32e29836 commit 1117510707037f759a77d17e797d734e32e29836 Author: Leonard Grey <lgrey@chromium.org> Date: Mon Apr 30 14:18:56 2018 Revert "Don't try to update window title on non-titled browsers" This reverts commit 5e0ca406d7893fa97928f8d013ac2bccefef21ff. Reason for revert: https://crbug.com/837825 Original change's description: > Don't try to update window title on non-titled browsers > > This causes two unnecessary layouts of BrowserView, each of which causes > three layouts of ToolbarView, each of which causes a whole bunch of > things to happen. > > ChromeOS is excluded since even on a tabbed browser, it relies on > the title to be updated to set the accessibility title. > > Bug: 835983 > Change-Id: I4a62579f8fae394a6c11eb7c8632da176452b00d > Reviewed-on: https://chromium-review.googlesource.com/1024828 > Commit-Queue: Leonard Grey <lgrey@chromium.org> > Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org> > Cr-Commit-Position: refs/heads/master@{#554040} TBR=ellyjones@chromium.org,lgrey@chromium.org # Not skipping CQ checks because original CL landed > 1 day ago. Bug: 835983 Change-Id: I238505c04e1943ce45d69e46c18f633c376acc02 Reviewed-on: https://chromium-review.googlesource.com/1034762 Reviewed-by: Leonard Grey <lgrey@chromium.org> Commit-Queue: Leonard Grey <lgrey@chromium.org> Cr-Commit-Position: refs/heads/master@{#554743} [modify] https://crrev.com/1117510707037f759a77d17e797d734e32e29836/chrome/browser/ui/views/frame/browser_view.cc
,
Apr 30 2018
I'll see if I can do a minimal refactor to separate notifying the window that the title has changed from performing a layout. If not, I'll probably reland this as Mac-only.
,
May 2 2018
FYI: in the Sampling Profiler data we see small increases in work done at startup[1] and during steady-state execution[2] with the revert in comment 7. [1] https://uma.googleplex.com/p/chrome/callstacks?sid=ea65e64bd2f4ca04477d8fac9908c0a9 [2] https://uma.googleplex.com/p/chrome/callstacks?sid=d119aa0267333f519a285d1dfdef3a68
,
Jun 20 2018
,
Jun 22 2018
,
Jun 25 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f70f003f8d4c89129c923238170e194f87dd0d16 commit f70f003f8d4c89129c923238170e194f87dd0d16 Author: Leonard Grey <lgrey@chromium.org> Date: Mon Jun 25 16:43:37 2018 Views: more accurate layout tracing Consider two views: A and its child view B. The "Views::Layout" trace event for B is currently emitted when A calls B's Layout as part of A's Layout. This is less intuitive than calling it in B's Layout, but is better because it can capture an event for a B which doesn't call through to the base class Layout implementation in its Layout. Unfortunately, this still leaves many gaps: 1) Layout managers 2) Views whose *parents* don't call the base class implementation 3) Layouts triggered by bounds changes 4) Probably some more I can't think of right now. This change adds extra trace events to address #3 from the list above. Bug: 850128 , 835983 Change-Id: I135ac0e8b2c6b9e277dce6da06e9d0aeae7dfbd1 Reviewed-on: https://chromium-review.googlesource.com/1112466 Commit-Queue: Leonard Grey <lgrey@chromium.org> Reviewed-by: Robert Liao <robliao@chromium.org> Reviewed-by: Trent Apted <tapted@chromium.org> Cr-Commit-Position: refs/heads/master@{#570065} [modify] https://crrev.com/f70f003f8d4c89129c923238170e194f87dd0d16/ui/views/view.cc
,
Jul 2
,
Jul 6
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/bd83c7a7b4e1b157b99c79139b54fb6437fe5c17 commit bd83c7a7b4e1b157b99c79139b54fb6437fe5c17 Author: Leonard Grey <lgrey@chromium.org> Date: Fri Jul 06 16:19:21 2018 Views: reduce non-client view frame/client view relayouts Currently, the NonClient view forces layout on its frame and client views after setting their bounds. Since setting bounds triggers a layout, this causes us to do twice the work each time (and since this is essentially the root, this is a lot of work). The original intention appears to be to trigger a paint for any changes that weren't covered by the bounds change. The second layout was mitigated against by no-oping OnBoundsChanged, but this is no longer effective, since the layout is triggered directly in SetBoundsRect and OnBoundsChanged is now just a subclass hook. This change removes the extra layout calls in favor of scheduling a paint. Bug: 850128 , 835983 Change-Id: Id041373d14d9ba88affd81dfa1863aad8e4b5ba0 Reviewed-on: https://chromium-review.googlesource.com/1102479 Commit-Queue: Leonard Grey <lgrey@chromium.org> Reviewed-by: Trent Apted <tapted@chromium.org> Reviewed-by: Robert Liao <robliao@chromium.org> Cr-Commit-Position: refs/heads/master@{#572985} [modify] https://crrev.com/bd83c7a7b4e1b157b99c79139b54fb6437fe5c17/ui/views/BUILD.gn [modify] https://crrev.com/bd83c7a7b4e1b157b99c79139b54fb6437fe5c17/ui/views/window/non_client_view.cc [modify] https://crrev.com/bd83c7a7b4e1b157b99c79139b54fb6437fe5c17/ui/views/window/non_client_view.h [add] https://crrev.com/bd83c7a7b4e1b157b99c79139b54fb6437fe5c17/ui/views/window/non_client_view_unittest.cc
,
Jul 10
macviews triage: is this Fixed now, or close to?
,
Jul 10
It's "only" 6 now, so it's an improvement. Since this bug was filed, it has come to light that excessive layout is the rule and not the exception, so I have not been actively working on this specific issue. I still think we can do 0 layouts here if we implement the idea in c#8.
,
Jul 12
,
Jul 12
,
Jul 13
,
Jul 20
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/02b33c1f9c7145169cfda0caf4b5eb0c50e471b5 commit 02b33c1f9c7145169cfda0caf4b5eb0c50e471b5 Author: Leonard Grey <lgrey@chromium.org> Date: Fri Jul 20 15:36:31 2018 Views: Don't unnecessarily invalidate layout when setting app menu button margin Currently, we invalidate the app menu button's layout unconditionally when setting its margin. Since invalidating layout walks up the view hierarchy, this essentially keeps ToolbarView's layout dirty all the time. This change exits early if the new margin is the same as the current margin. Bug: 858944, 835983 Change-Id: If94d3c7d29cb1476fdb268a8ece35187d0b43f4f Reviewed-on: https://chromium-review.googlesource.com/1144222 Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org> Commit-Queue: Leonard Grey <lgrey@chromium.org> Cr-Commit-Position: refs/heads/master@{#576873} [modify] https://crrev.com/02b33c1f9c7145169cfda0caf4b5eb0c50e471b5/chrome/browser/ui/views/toolbar/browser_app_menu_button.cc
,
Jul 20
Requesting a merge to 69 for the above CL since it just missed branch and helps MacViews performance quite a bit. It's close to zero risk
,
Jul 21
This bug requires manual review: Reverts referenced in bugdroid comments after merge request. Please contact the milestone owner if you have questions. Owners: amineer@(Android), kariahda@(iOS), cindyb@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jul 23
pls apply appropriate OSs label. Thank you.
,
Jul 23
,
Jul 23
Approving merge to M69 branch 3497 for cl listed at #20 based on comment #21. Pls merge ASAP so we can pick it up for this week M69 Dev release. Thank you.
,
Jul 23
Thanks,govind@! Done
,
Jul 23
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b80a747d547adbc6c364808d7596377a458a3533 commit b80a747d547adbc6c364808d7596377a458a3533 Author: Leonard Grey <lgrey@chromium.org> Date: Mon Jul 23 18:33:29 2018 Views: Don't unnecessarily invalidate layout when setting app menu button margin Currently, we invalidate the app menu button's layout unconditionally when setting its margin. Since invalidating layout walks up the view hierarchy, this essentially keeps ToolbarView's layout dirty all the time. This change exits early if the new margin is the same as the current margin. Bug: 858944, 835983 Change-Id: If94d3c7d29cb1476fdb268a8ece35187d0b43f4f Reviewed-on: https://chromium-review.googlesource.com/1144222 Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org> Commit-Queue: Leonard Grey <lgrey@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#576873}(cherry picked from commit 02b33c1f9c7145169cfda0caf4b5eb0c50e471b5) Reviewed-on: https://chromium-review.googlesource.com/1147181 Reviewed-by: Leonard Grey <lgrey@chromium.org> Cr-Commit-Position: refs/branch-heads/3497@{#20} Cr-Branched-From: 271eaf50594eb818c9295dc78d364aea18c82ea8-refs/heads/master@{#576753} [modify] https://crrev.com/b80a747d547adbc6c364808d7596377a458a3533/chrome/browser/ui/views/toolbar/browser_app_menu_button.cc
,
Jul 25
lgrey@ - Could you please provide manual repro steps to verify the issue from TE-end. Thanks...!!
,
Jul 25
There are no manual steps, it requires debugging, adding code, or tracing to verify. There's still one more change in flight so not marking fixed yet.
,
Jul 25
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/081f7dc4810ce63a3c976a077576bc0ad8ed6f6a commit 081f7dc4810ce63a3c976a077576bc0ad8ed6f6a Author: Leonard Grey <lgrey@chromium.org> Date: Wed Jul 25 14:48:57 2018 Views: Don't layout frame on title change if window title isn't displayed Bug: 858944, 835983 Change-Id: Ia7908a1a199a1e537bf7ba7668116e018cbd4e61 Reviewed-on: https://chromium-review.googlesource.com/1144138 Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org> Reviewed-by: Sadrul Chowdhury <sadrul@chromium.org> Commit-Queue: Leonard Grey <lgrey@chromium.org> Cr-Commit-Position: refs/heads/master@{#577884} [modify] https://crrev.com/081f7dc4810ce63a3c976a077576bc0ad8ed6f6a/chrome/browser/ui/views/frame/glass_browser_frame_view.cc [modify] https://crrev.com/081f7dc4810ce63a3c976a077576bc0ad8ed6f6a/chrome/browser/ui/views/frame/opaque_browser_frame_view.cc [modify] https://crrev.com/081f7dc4810ce63a3c976a077576bc0ad8ed6f6a/ui/views/bubble/bubble_frame_view.cc [modify] https://crrev.com/081f7dc4810ce63a3c976a077576bc0ad8ed6f6a/ui/views/widget/widget.cc [modify] https://crrev.com/081f7dc4810ce63a3c976a077576bc0ad8ed6f6a/ui/views/window/custom_frame_view.cc
,
Jul 26
,
Jul 26
This is down to 2 in Canary, will declare that good enough for now |
||||||||||||||||
►
Sign in to add a comment |
||||||||||||||||
Comment 1 by lgrey@chromium.org
, Apr 23 2018