Reland "Fix toolbar snap-back and stale bitmap capture" [chromium/src : main]
Alejandro Romano (Gerrit)
Alejandro Romano added 1 comment![]( "Open in Gerrit")
Some additional notes on this reland:
In local testing across SPA and MPA navigations, scroll sequences, and route changes, the pre-draw listener approach reduces toolbar bitmap captures by ~60–75% compared to my previous fix. The reduction comes from coalescing multiple invalidations per frame into a single capture, and the listener self-deregistering once the toolbar starts revealing.
Memory: the bisect flagged ~1 MB regression (crbug.com/504007917) in the previous fix. Fewer unnecessary captures could bring it down. Or it could stay the same if the remaining captures are all necessary, which would mean the bitmap cache is working as intended (I believe). Either way it shouldn't be worse.
Also tested manually on a Samsung S21 (ARM64, Android 15). No UI/UX regressions. Done some additional Android x64 tests that would fail in previous CL/patches. All good.
The NullPointerException was a pre-existing latent bug. Our increased capture rate was the first time it was reliably triggered in CI.
Ready for review/CQ Dry Run whenever you want!
Related details
- Patrick Noland
- Peilin Wang
- Sky Malice
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alejandro Romano (Gerrit)
Alejandro Romano added 1 comment![]( "Open in Gerrit")
Some additional notes on this reland:
In local testing across SPA and MPA navigations, scroll sequences, and route changes, the pre-draw listener approach reduces toolbar bitmap captures by ~60–75% compared to my previous fix. The reduction comes from coalescing multiple invalidations per frame into a single capture, and the listener self-deregistering once the toolbar starts revealing.
Memory: the bisect flagged ~1 MB regression (crbug.com/504007917) in the previous fix. Fewer unnecessary captures could bring it down. Or it could stay the same if the remaining captures are all necessary, which would mean the bitmap cache is working as intended (I believe). Either way it shouldn't be worse.
Also tested manually on a Samsung S21 (ARM64, Android 15). No UI/UX regressions. Done some additional Android x64 tests that would fail in previous CL/patches. All good.
The NullPointerException was a pre-existing latent bug. Our increased capture rate was the first time it was reliably triggered in CI.
Ready for review/CQ Dry Run whenever you want!
Oh and please, if I'm speaking non-sense about the memory regression and it's important to address (if this keeps being a problem), please let me know. I don't have the sufficient knowledge to debug it properly and verify the nature of it, but I'm willing to learn of course
Related details
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Peilin Wang (Gerrit)
Peilin Wang added 1 comment![]( "Open in Gerrit")
if (dirtyRect != null) {sorry you're gonna get a merge conflict here, I landed another patch to fix this first cause it was causing crashes in canary/dev
Related details
- Alejandro Romano
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Peilin Wang (Gerrit)
Peilin Wang added 1 comment![]( "Open in Gerrit")
Alejandro RomanoSome additional notes on this reland:
In local testing across SPA and MPA navigations, scroll sequences, and route changes, the pre-draw listener approach reduces toolbar bitmap captures by ~60–75% compared to my previous fix. The reduction comes from coalescing multiple invalidations per frame into a single capture, and the listener self-deregistering once the toolbar starts revealing.
Memory: the bisect flagged ~1 MB regression (crbug.com/504007917) in the previous fix. Fewer unnecessary captures could bring it down. Or it could stay the same if the remaining captures are all necessary, which would mean the bitmap cache is working as intended (I believe). Either way it shouldn't be worse.
Also tested manually on a Samsung S21 (ARM64, Android 15). No UI/UX regressions. Done some additional Android x64 tests that would fail in previous CL/patches. All good.
The NullPointerException was a pre-existing latent bug. Our increased capture rate was the first time it was reliably triggered in CI.
Ready for review/CQ Dry Run whenever you want!
Oh and please, if I'm speaking non-sense about the memory regression and it's important to address (if this keeps being a problem), please let me know. I don't have the sufficient knowledge to debug it properly and verify the nature of it, but I'm willing to learn of course
The memory regression can be real. In the past we reduced memory by capturing less often. If we run an A/B test we can see how big/real the regression is in the real world. You can land this patch first and I can run an A/B test after.
Alejandro Romano (Gerrit)
Alejandro Romano added 2 comments![]( "Open in Gerrit")
Alejandro RomanoSome additional notes on this reland:
In local testing across SPA and MPA navigations, scroll sequences, and route changes, the pre-draw listener approach reduces toolbar bitmap captures by ~60–75% compared to my previous fix. The reduction comes from coalescing multiple invalidations per frame into a single capture, and the listener self-deregistering once the toolbar starts revealing.
Memory: the bisect flagged ~1 MB regression (crbug.com/504007917) in the previous fix. Fewer unnecessary captures could bring it down. Or it could stay the same if the remaining captures are all necessary, which would mean the bitmap cache is working as intended (I believe). Either way it shouldn't be worse.
Also tested manually on a Samsung S21 (ARM64, Android 15). No UI/UX regressions. Done some additional Android x64 tests that would fail in previous CL/patches. All good.
The NullPointerException was a pre-existing latent bug. Our increased capture rate was the first time it was reliably triggered in CI.
Ready for review/CQ Dry Run whenever you want!
Peilin WangOh and please, if I'm speaking non-sense about the memory regression and it's important to address (if this keeps being a problem), please let me know. I don't have the sufficient knowledge to debug it properly and verify the nature of it, but I'm willing to learn of course
The memory regression can be real. In the past we reduced memory by capturing less often. If we run an A/B test we can see how big/real the regression is in the real world. You can land this patch first and I can run an A/B test after.
Got it. Thanks!
if (dirtyRect != null) {sorry you're gonna get a merge conflict here, I landed another patch to fix this first cause it was causing crashes in canary/dev
Got it. Awesome that it's already been taken care of. Thanks!
Related details
- Peilin Wang
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alejandro Romano (Gerrit)
Alejandro Romano added 1 comment![]( "Open in Gerrit")
New patchset. Ready for review and a CQ Dry Run.
Rebased, removed dirtyRect workaround and test in favor of upstream fix.
Related details
- Patrick Noland
- Peilin Wang
- Sky Malice
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alejandro Romano (Gerrit)
Alejandro Romano removed Jerome Jiang from this change![]( "Open in Gerrit")
- Jerome Jiang
Related details
- Patrick Noland
- Peilin Wang
- Sky Malice
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alejandro Romano (Gerrit)
Alejandro Romano removed Mirko Bonadei from this change![]( "Open in Gerrit")
- Mirko Bonadei
Alejandro Romano (Gerrit)
Alejandro Romano removed net-r...@chromium.org from this change![]( "Open in Gerrit")
Alejandro Romano (Gerrit)
Alejandro Romano removed mar...@chromium.org from this change![]( "Open in Gerrit")
Alejandro Romano (Gerrit)
Alejandro Romano removed cblume...@chromium.org from this change![]( "Open in Gerrit")
Alejandro Romano (Gerrit)
Alejandro Romano removed chrome-intell...@chromium.org from this change![]( "Open in Gerrit")
Alejandro Romano (Gerrit)
Alejandro Romano removed devtools...@chromium.org from this change![]( "Open in Gerrit")
Alejandro Romano (Gerrit)
Alejandro Romano removed chrome-intelligence-te...@google.com from this change![]( "Open in Gerrit")
Alejandro Romano (Gerrit)
Alejandro Romano removed feature-me...@chromium.org from this change![]( "Open in Gerrit")
Alejandro Romano (Gerrit)
Alejandro Romano removed hanxi...@chromium.org from this change![]( "Open in Gerrit")
Alejandro Romano (Gerrit)
Alejandro Romano removed jz...@chromium.org from this change![]( "Open in Gerrit")
Alejandro Romano (Gerrit)
Alejandro Romano removed fgal...@chromium.org from this change![]( "Open in Gerrit")
Alejandro Romano (Gerrit)
Alejandro Romano removed peilinwa...@google.com from this change![]( "Open in Gerrit")
Alejandro Romano (Gerrit)
Alejandro Romano removed penghuan...@chromium.org from this change![]( "Open in Gerrit")
Alejandro Romano (Gerrit)
Alejandro Romano added 2 comments![]( "Open in Gerrit")
New patchset. Ready for review and a CQ Dry Run.
Rebased, removed dirtyRect workaround and test in favor of upstream fix.
Done
Patchset 7. Rebased. Please trigger a CQ Run when ready!
Related details
- Patrick Noland
- Peilin Wang
- Sky Malice
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alejandro Romano (Gerrit)
Alejandro Romano added 1 comment![]( "Open in Gerrit")
Patchset 7. Rebased. Please trigger a CQ Run when ready!
All tests passing! Resolving this comment
Related details
- Patrick Noland
- Peilin Wang
- Sky Malice
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Peilin Wang (Gerrit)
Peilin Wang added 1 comment![]( "Open in Gerrit")
private boolean onHiddenCapturePreDraw() {does this get called from anywhere?
Related details
- Alejandro Romano
- Patrick Noland
- Sky Malice
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alejandro Romano (Gerrit)
Alejandro Romano added 1 comment![]( "Open in Gerrit")
private boolean onHiddenCapturePreDraw() {does this get called from anywhere?
not really, only from the line above it, mHiddenCaptureOnPreDraw. I can turn it into a simpler = () -> {} inline
Related details
- Peilin Wang
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Peilin Wang (Gerrit)
Peilin Wang added 1 comment![]( "Open in Gerrit")
private boolean onHiddenCapturePreDraw() {Alejandro Romanodoes this get called from anywhere?
not really, only from the line above it, mHiddenCaptureOnPreDraw. I can turn it into a simpler = () -> {} inline
Declaring+initializing OnPreDrawListener will call this function?
Related details
- Alejandro Romano
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alejandro Romano (Gerrit)
Alejandro Romano added 1 comment![]( "Open in Gerrit")
private boolean onHiddenCapturePreDraw() {Alejandro Romanodoes this get called from anywhere?
Peilin Wangnot really, only from the line above it, mHiddenCaptureOnPreDraw. I can turn it into a simpler = () -> {} inline
Declaring+initializing OnPreDrawListener will call this function?
No, it just stores a reference to `onHiddenCapturePreDraw()`. It won't initialize it right away.
We do that to pass the same `mHiddenCaptureOnPreDraw` object to both `addOnPreDrawListener()` and `removeOnPreDrawListener()`. If we passed `this::onHiddenCapturePreDraw` directly each time, each call would create a new object with no way to unregister it, if that makes sense.
Related details
- Peilin Wang
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Peilin Wang (Gerrit)
Peilin Wang added 1 comment![]( "Open in Gerrit")
private boolean onHiddenCapturePreDraw() {Alejandro Romanodoes this get called from anywhere?
Peilin Wangnot really, only from the line above it, mHiddenCaptureOnPreDraw. I can turn it into a simpler = () -> {} inline
Alejandro RomanoDeclaring+initializing OnPreDrawListener will call this function?
No, it just stores a reference to `onHiddenCapturePreDraw()`. It won't initialize it right away.
We do that to pass the same `mHiddenCaptureOnPreDraw` object to both `addOnPreDrawListener()` and `removeOnPreDrawListener()`. If we passed `this::onHiddenCapturePreDraw` directly each time, each call would create a new object with no way to unregister it, if that makes sense.
Ah sorry I didn't see you're adding the listener on line 802...
Related details
- Alejandro Romano
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Peilin Wang (Gerrit)
Peilin Wang voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Alejandro Romano
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Patrick Noland (Gerrit)
Patrick Noland voted and added 1 comment![]( "Open in Gerrit")
Votes added by Patrick Noland
| Code-Review | +1 |
1 comment
private boolean shouldCaptureWhileHidden() {Since this is only used once, you could fold the `!mHiddenCaptureRegistered` check into the method and rename to `shouldRegisterHiddenCapturePreDrawListener` . up to you
Related details
- Alejandro Romano
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alejandro Romano (Gerrit)
Alejandro Romano added 1 comment![]( "Open in Gerrit")
private boolean shouldCaptureWhileHidden() {Since this is only used once, you could fold the `!mHiddenCaptureRegistered` check into the method and rename to `shouldRegisterHiddenCapturePreDrawListener` . up to you
You mean `shouldCaptureWhileHidden` being used once? It is used in three places
- Line 706: inside onHiddenCapturePreDraw() to decide whether to deregister
- Line 715: inside the posted runnable to re-check before capturing
- Line 798: at registration time, combined with !mHiddenCaptureRegistered
That's why it exists as its own function
Related details
- Patrick Noland
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Patrick Noland (Gerrit)
Patrick Noland added 1 comment![]( "Open in Gerrit")
private boolean shouldCaptureWhileHidden() {Alejandro RomanoSince this is only used once, you could fold the `!mHiddenCaptureRegistered` check into the method and rename to `shouldRegisterHiddenCapturePreDrawListener` . up to you
You mean `shouldCaptureWhileHidden` being used once? It is used in three places
- Line 706: inside onHiddenCapturePreDraw() to decide whether to deregister
- Line 715: inside the posted runnable to re-check before capturing
- Line 798: at registration time, combined with !mHiddenCaptureRegistered
That's why it exists as its own function
- Alejandro Romano
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alejandro Romano (Gerrit)
Alejandro Romano voted Commit-Queue+2![]( "Open in Gerrit")
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alejandro Romano (Gerrit)
Alejandro Romano added 1 comment![]( "Open in Gerrit")
- Patrick Noland
- Peilin Wang
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alejandro Romano (Gerrit)
Alejandro Romano added 1 comment![]( "Open in Gerrit")
Sorry, previous rebase was missing some changes. PTAL!
Related details
- Patrick Noland
- Peilin Wang
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Patrick Noland (Gerrit)
Patrick Noland voted Code-Review+1![]( "Open in Gerrit")
- Alejandro Romano
- Peilin Wang
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |