Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2834393003:
Re-enable ScrollbarAnimationController::DidRequestShowFromMainThread. (Closed)
|
Created:
3 years, 8 months ago by skobes Modified:
3 years, 8 months ago Reviewers:
aelias_OOO_until_Jul13 CC:
blink-reviews, blink-reviews-frames_chromium.org, blink-reviews-paint_chromium.org, bokan, cc-bugs_chromium.org, chromium-reviews, dshwang, ericrk Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionRe-enable ScrollbarAnimationController::DidRequestShowFromMainThread.
Disabling it didn't fix http://crbug.com/706927, but caused a new regression in
http://crbug.com/715279, while http://crbug.com/712453 recovered for some other
reason.
BUG=606395, 706927, 712453, 715279
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Review-Url: https://codereview.chromium.org/2834393003
Cr-Commit-Position: refs/heads/master@{#467513}
Committed: https://chromium.googlesource.com/chromium/src/+/66aacfa671360982f234c0fbd3b186efe21ee763
Patch Set 1 #Patch Set 2 : rebase #
Total comments: 2
Patch Set 3 : remove Android hacks #
Messages
Total messages: 25 (18 generated)
Description was changed from ========== Re-enable ScrollbarAnimationController::DidRequestShowFromMainThread. Avoid calling ShowOverlayScrollbars for viewport size changes on Android, which seems to regress memory and perf tests. BUG=606395 ========== to ========== Re-enable ScrollbarAnimationController::DidRequestShowFromMainThread. Avoid calling ShowOverlayScrollbars for viewport size changes on Android, which seems to regress memory and perf tests. BUG=606395 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
Description was changed from ========== Re-enable ScrollbarAnimationController::DidRequestShowFromMainThread. Avoid calling ShowOverlayScrollbars for viewport size changes on Android, which seems to regress memory and perf tests. BUG=606395 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Re-enable ScrollbarAnimationController::DidRequestShowFromMainThread. Avoid calling ShowOverlayScrollbars for viewport size changes on Android, which seems to regress memory and perf tests. BUG=606395 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
The CQ bit was checked by skobes@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Re-enable ScrollbarAnimationController::DidRequestShowFromMainThread. Avoid calling ShowOverlayScrollbars for viewport size changes on Android, which seems to regress memory and perf tests. BUG=606395 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Re-enable ScrollbarAnimationController::DidRequestShowFromMainThread. Avoid calling ShowOverlayScrollbars for viewport size changes on Android, which seems to regress memory and perf tests. BUG=606395, 706927, 712453 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by skobes@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
skobes@chromium.org changed reviewers: + aelias@chromium.org
https://codereview.chromium.org/2834393003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/2834393003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameView.cpp:1672: // main on viewport size changes regresses memory and perf tests on Android Hmm, since you traced it to viewport sized changes, it's probably due to top controls hiding in some way. +cc bokan@, any idea what the problem might be? https://codereview.chromium.org/2834393003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameView.cpp:1674: #if !OS(ANDROID) I really don't like OS(ANDROID), among other things because it winds up forcing Android-specific unit tests or unit tests that only go red on Android. If we're going to go with platform-specific behavior, I prefer it to be behind a WebSetting that's only true on Android. A natural one for this purpose would be GetPage()->GetSettings().GetMainFrameResizesAreOrientationChanges()
Since ignoring the entire show-from-main path in http://crrev.com/466712 didn't improve any of the graphs in https://chromeperf.appspot.com/group_report?bug_id=706927, it looks like there's no point in the Android-specific workarounds. This patch is now just a revert of http://crrev.com/466712.
Description was changed from ========== Re-enable ScrollbarAnimationController::DidRequestShowFromMainThread. Avoid calling ShowOverlayScrollbars for viewport size changes on Android, which seems to regress memory and perf tests. BUG=606395, 706927, 712453 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Re-enable ScrollbarAnimationController::DidRequestShowFromMainThread. Avoid calling ShowOverlayScrollbars for viewport size changes on Android, which seems to regress memory and perf tests. BUG=606395, 706927, 712453, 715279 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
lgtm
Please update patch description before landing.
Description was changed from ========== Re-enable ScrollbarAnimationController::DidRequestShowFromMainThread. Avoid calling ShowOverlayScrollbars for viewport size changes on Android, which seems to regress memory and perf tests. BUG=606395, 706927, 712453, 715279 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Re-enable ScrollbarAnimationController::DidRequestShowFromMainThread. BUG=606395, 706927, 712453, 715279 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
Description was changed from ========== Re-enable ScrollbarAnimationController::DidRequestShowFromMainThread. BUG=606395, 706927, 712453, 715279 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Re-enable ScrollbarAnimationController::DidRequestShowFromMainThread. Disabling it didn't fix http://crbug.com/706927, but caused a new regression in http://crbug.com/715279, while http://crbug.com/712453 recovered for some other reason. BUG=606395, 706927, 712453, 715279 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
The CQ bit was checked by skobes@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1493243302391550,
"parent_rev": "80610570ec4327389940e2ce084df9899692f42d", "commit_rev":
"66aacfa671360982f234c0fbd3b186efe21ee763"}
Message was sent while issue was closed.
Description was changed from ========== Re-enable ScrollbarAnimationController::DidRequestShowFromMainThread. Disabling it didn't fix http://crbug.com/706927, but caused a new regression in http://crbug.com/715279, while http://crbug.com/712453 recovered for some other reason. BUG=606395, 706927, 712453, 715279 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Re-enable ScrollbarAnimationController::DidRequestShowFromMainThread. Disabling it didn't fix http://crbug.com/706927, but caused a new regression in http://crbug.com/715279, while http://crbug.com/712453 recovered for some other reason. BUG=606395, 706927, 712453, 715279 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Review-Url: https://codereview.chromium.org/2834393003 Cr-Commit-Position: refs/heads/master@{#467513} Committed: https://chromium.googlesource.com/chromium/src/+/66aacfa671360982f234c0fbd3b1... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/66aacfa671360982f234c0fbd3b1... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
