Reapply "Gestures: Construct a proper fragment tree for the overscroll pseudos." [chromium/src : main]
AI Code Reviewer (Gerrit)
AI Code Reviewer added 1 comment![]( "Open in Gerrit")
nit: Per the Blink Style Guide (Naming - Precede setters with the word “Set”; use bare words for getters), getters should use bare words unless there's a name collision. Please consider renaming `GetOverscrollAreaName` to `OverscrollAreaName`.
To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **Won't fix**: reason | **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. |
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job mac-m4-mini-perf/motionmark1.3.crossbench complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/17033ddfd10000
Related details
- Vladimir Levin
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. |
Robert Flack (Gerrit)
Robert Flack added 5 comments![]( "Open in Gerrit")
Nice, the overall diff from the original patch looks good, just a few things that I think could improve readability / performance.
MakeGarbageCollected<ScopedCSSName>(pseudo_argument_, &GetDocument());This feels a little awkward to me to assume that any pseudo argument should be tree scoped. I *think* the only reason for the argument to be tree scoped is because it is an ident / name, right? In which case maybe we should only set this for view transition and overscroll pseudo elements (and other future pseudo elements whose argument is an ident).
// TODO(crbug.com/455892921): We need to check the document scope as well.is this checked now that we're checking the ScopedCSSName?
}If uncontained_overscroll_position_descendants_ already contains name you can early return rather than copying it.
auto remove_uncontained_overscroll_position_descendant =I think it's worth checking this early rather than adding the name and removing it at the end to avoid unnecessary copies. I.e. before adding a name check if we should skip that name.
Related details
- Vladimir Levin
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. |
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job mac-m4-mini-perf/motionmark1.3.crossbench complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/15484680310000
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job mac-m4-mini-perf/motionmark1.3.crossbench complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/15e64b10310000
Ian Kilpatrick (Gerrit)
Ian Kilpatrick added 1 comment![]( "Open in Gerrit")
ScopedCSSName* GetTreeScopedPseudoArgument();can all of these be `const ScopedCSSName*`?
Related details
- Vladimir Levin
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. |
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job mac-m4-mini-perf/motionmark1.3.crossbench complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/131fed78310000
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
😿 Job mac-m4-mini-perf/motionmark1.3.crossbench failed.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/15810830310000
Related details
- Vladimir Levin
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. |
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
😿 Job mac-m4-mini-perf/motionmark1.3.crossbench failed.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/12c2fcffd10000
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job mac-m4-mini-perf/motionmark1.3.crossbench complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/14071ccc310000
Related details
- Vladimir Levin
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. |
Vladimir Levin (Gerrit)
Vladimir Levin voted and added 7 comments![]( "Open in Gerrit")
Votes added by Vladimir Levin
| Commit-Queue | +1 |
7 comments
can all of these be `const ScopedCSSName*`?
Done
MakeGarbageCollected<ScopedCSSName>(pseudo_argument_, &GetDocument());This feels a little awkward to me to assume that any pseudo argument should be tree scoped. I *think* the only reason for the argument to be tree scoped is because it is an ident / name, right? In which case maybe we should only set this for view transition and overscroll pseudo elements (and other future pseudo elements whose argument is an ident).
Added a DCHECK
nit: Per the Blink Style Guide (Naming - Precede setters with the word “Set”; use bare words for getters), getters should use bare words unless there's a name collision. Please consider renaming `GetOverscrollAreaName` to `OverscrollAreaName`.
To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **Won't fix**: reason | **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)_
Acknowledged
// TODO(crbug.com/455892921): We need to check the document scope as well.is this checked now that we're checking the ScopedCSSName?
Done
If uncontained_overscroll_position_descendants_ already contains name you can early return rather than copying it.
Done
auto remove_uncontained_overscroll_position_descendant =I think it's worth checking this early rather than adding the name and removing it at the end to avoid unnecessary copies. I.e. before adding a name check if we should skip that name.
- Chris Harrelson
- Ian Kilpatrick
- Robert Flack
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chris Harrelson (Gerrit)
Chris Harrelson added 1 comment![]( "Open in Gerrit")
Is it the same code as it was the first time, or changed?
Related details
- Ian Kilpatrick
- Robert Flack
- Vladimir Levin
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Vladimir Levin (Gerrit)
Vladimir Levin added 1 comment![]( "Open in Gerrit")
Is it the same code as it was the first time, or changed?
It's changed quite a bit. Compare to PS1, which is the original patch
Related details
- Chris Harrelson
- Ian Kilpatrick
- Robert Flack
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Vladimir Levin (Gerrit)
Vladimir Levin added 1 comment![]( "Open in Gerrit")
Vladimir LevinIs it the same code as it was the first time, or changed?
It's changed quite a bit. Compare to PS1, which is the original patch
Summary is on top of the last patch: It implements copy on write for uncontained areas in paint layer, and also switches from atomic string to ScopedCSSName
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job mac-m4-mini-perf/motionmark1.3.crossbench complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/14a360c8310000
Related details
- Chris Harrelson
- Ian Kilpatrick
- Robert Flack
- Vladimir Levin