Ensure propagation of PropertyChangeForcesCommit to compositor [chromium/src : main]
AI Code Reviewer (Gerrit)
AI Code Reviewer added 2 comments![]( "Open in Gerrit")
cc::PropertyChangeForcesCommitCriteria criteria,Blink Style Guide: Prefer enums or StrongAliases to bare bools for function parameters. To improve type safety and readability, consider using a base::StrongAlias<class ForcePropagationTag, bool> for the 'force_propagation' parameter instead of a bare bool.
To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **OK But Won't Fix**: reason | **Later**: b/<bug_id> | **Invalid:** reason
_This comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent)._
_AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve._
_[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
cc::PropertyChangeForcesCommitCriteria criteria,Blink Style Guide: Prefer enums or StrongAliases to bare bools for function parameters. To improve type safety and readability, consider using a base::StrongAlias<class ForcePropagationTag, bool> for the 'force_propagation' parameter instead of a bare bool.
To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **OK But Won't Fix**: reason | **Later**: b/<bug_id> | **Invalid:** reason
_This comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent)._
_AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve._
_[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
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. |
Stefan Zager (Gerrit)
Stefan Zager added 1 comment![]( "Open in Gerrit")
I can't figure out how to make a test that fails :(, any suggestions?
Related details
- Morten Stenshorne
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Morten Stenshorne (Gerrit)
Morten Stenshorne added 1 comment![]( "Open in Gerrit")
I can't figure out how to make a test that fails :(, any suggestions?
We need to make a couple of changes in the anchor positioning code for it to work correctly there, so that's not an option for now.
But how about an animation that does nothing until it has spent, say, 50% of the time, and then use intersection observer?
Related details
- Stefan Zager
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stefan Zager (Gerrit)
Stefan Zager added 1 comment![]( "Open in Gerrit")
Morten StenshorneI can't figure out how to make a test that fails :(, any suggestions?
We need to make a couple of changes in the anchor positioning code for it to work correctly there, so that's not an option for now.
But how about an animation that does nothing until it has spent, say, 50% of the time, and then use intersection observer?
I was able to tweak the existing test so that the virtual/threaded version fails without the patch, passes without it; PTAL.
Related details
- Morten Stenshorne
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Morten Stenshorne (Gerrit)
Morten Stenshorne added 1 comment![]( "Open in Gerrit")
force_commit_for_propagation_ |= force_propagation;Won't this get stuck forever, once turned on?
```html
<!DOCTYPE html>
<style>
#anchor {
animation: 2s anim1;
will-change: transform;
anchor-name: --a;
width: 100px;
height: 100px;
background: yellow;
}
#anchored {
position: absolute;
position-anchor: --a;
top: anchor(bottom);
width: 40px;
height: 40px;
background: blue;
} #spinner {
position: absolute;
right: 200px;
bottom: 200px;
width: 100px;
height: 100px;
animation: 2s anim1 infinite linear;
background: hotpink;
} @keyframes anim1 {
from {
transform: rotate(0deg);
}
to {
transform: rotate(360deg);
}
}
</style>
<div id="anchor"></div>
<div id="anchored"></div>
<div id="spinner"></div>
```After 2 seconds (when `#anchor` is done animating), we should stop forcing, but I can see that LocalFrameView::UpdateLifecyclePhasesInternal() is called for each animation frame forever.
This isn't the case with a local fix I wrote based on your sketches a few weeks ago. Like this: https://chromium-review.googlesource.com/c/chromium/src/+/7247219
Maybe the problem isn't this line of code though, because I see that "my" patch also does this.
Related details
- Stefan Zager
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. |
Stefan Zager (Gerrit)
Stefan Zager added 1 comment![]( "Open in Gerrit")
force_commit_for_propagation_ |= force_propagation;Good catch! I fixed this in LocalFrameView; it now stops running BeginMainFrame after the anchor-affecting animation stops.
Related details
- Morten Stenshorne
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Morten Stenshorne (Gerrit)
Morten Stenshorne voted Code-Review+1![]( "Open in Gerrit")
- Stefan Zager
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stefan Zager (Gerrit)
Blink W3C Test Autoroller (Gerrit)
Message from Blink W3C Test Autoroller![]( "Open in Gerrit")
Exportable changes to web-platform-tests were detected in this CL and a pull request in the upstream repo has been made: https://github.com/web-platform-tests/wpt/pull/56718.
When this CL lands, the bot will automatically merge the PR on GitHub if the required GitHub checks pass; otherwise, ecosystem-infra@ team will triage the failures and may contact you.
WPT Export docs:
https://chromium.googlesource.com/chromium/src/+/main/docs/testing/web_platform_tests.md#Automatic-export-process
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stefan Zager (Gerrit)
Stefan Zager voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Ensure propagation of PropertyChangeForcesCommit to compositor
Prior to this change, a new value for PropertyChangeForcesCommit
might not make it to the compositor thread right away if
BeginMainFrame resulted in no visual update and returned early. If a
page is idle, it could take an arbitrary amount of time for the new
value to be propagated.
With this CL, we force a commit to be sent to the compositor when a
new value occurs.
- M cc/trees/layer_tree_host.cc
- M cc/trees/layer_tree_host.h
- M cc/trees/proxy_main.cc
- M third_party/blink/renderer/core/frame/local_frame_view.cc
- M third_party/blink/renderer/core/frame/local_frame_view.h
- M third_party/blink/renderer/core/frame/web_frame_widget_impl.cc
- M third_party/blink/renderer/core/frame/web_frame_widget_impl.h
- M third_party/blink/renderer/core/loader/empty_clients.h
- M third_party/blink/renderer/core/page/chrome_client.h
- M third_party/blink/renderer/core/page/chrome_client_impl.cc
- M third_party/blink/renderer/core/page/chrome_client_impl.h
- M third_party/blink/web_tests/external/wpt/intersection-observer/animating.html
Code-Review: +1 by Morten Stenshorne
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Blink W3C Test Autoroller (Gerrit)
Message from Blink W3C Test Autoroller![]( "Open in Gerrit")
The WPT PR for this CL has been merged upstream! https://github.com/web-platform-tests/wpt/pull/56718
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |