Refactor ElasticOverscrollController to store entries in a base::flat_map [chromium/src : main]
Free Debreuil (Gerrit)
Free Debreuil added 2 comments![]( "Open in Gerrit")
Free DebreuilBlink Style Guide: Prefer blink:: types over STL and base types. Since ordering of entries does not appear to be a requirement, please use `WTF::HashMap` instead of `base::flat_map`.
_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:** reasonThis 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)_
Done
return helper_->ScrollBounds(element_id);Free DebreuilBlink Style Guide: Naming - Use 'CamelCase' for all function names. Please rename `scroll_velocity` to `ScrollVelocity`. This also applies to the other new getters on lines 118 and 121.
_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:** reasonThis 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)_
Done
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Free Debreuil (Gerrit)
Free Debreuil added 1 comment![]( "Open in Gerrit")
Related details
- 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 4 comments![]( "Open in Gerrit")
OverscrollEntry& EntryMutable();nit: I'd name this something like GetOrCreateEntry()
gfx::Size ScrollBounds() const;I'd add a TODO to add element id into these (and below as well I think)
// called and momentum_animation_start_time. Using this information, thenit if you wrap the variable names in backticks (`) in comments, then they will get linkified in codesearch as well
fabs(ScrollVelocity().x()) > kIgnoreForwardBounceVelocityThresholdThis presumably would do a find on a map for each invocation. In general, all of the getters are likely not going to be used in isolation (ie if I get Foo(element_id) then I'll also get Bar(element_id)).
I think a better pattern would be similar to what you have for EntryMutable(), but named something like const Entry& GetEntry(ElementId). Then all of these functions can get the entry once and use whatever they need from it
Related details
- Free Debreuil
- Robert Flack
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 2 comments![]( "Open in Gerrit")
OverscrollEntry& EntryMutable();nit: I'd name this something like GetOrCreateEntry()
Or EnsureEntry is common elsewhere.
gfx::Size ScrollBounds() const;I'd add a TODO to add element id into these (and below as well I think)
Rather than parameterize all of the methods it may be easier and less verbose if OverscrollEntry was a class with the relevant methods on it. Then consumers could get the entry for the scroller they care about and update it accordingly.
Related details
- Free Debreuil
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. |
Free Debreuil (Gerrit)
Free Debreuil voted and added 2 comments![]( "Open in Gerrit")
Votes added by Free Debreuil
| Commit-Queue | +1 |
2 comments
gfx::Size ScrollBounds() const;Robert FlackI'd add a TODO to add element id into these (and below as well I think)
Rather than parameterize all of the methods it may be easier and less verbose if OverscrollEntry was a class with the relevant methods on it. Then consumers could get the entry for the scroller they care about and update it accordingly.
That could work yes - there is currently a split where some of the functionality is accessed through an entry in the `ElasticOverscrollController` and some through `ScrollElasticityHelper`.
Would it be reasonable to add a method on `ElasticOverscrollController`:
```
const OverscrollEntry& GetEntry() const;
```
then access the per entry directly off of the struct?
The `OverscrollEntry` is currently a struct - it's possible it could re-route the `ScrollElasticityHelper` methods as well (nicer API) but that possibly would require duplication of the `ScrollElasticityHelper` pointer per entry, or an additional class that acts like a reference? (maybe there's another way to structure though that I'm not thinking of) 😊
// called and momentum_animation_start_time. Using this information, theFree Debreuilnit if you wrap the variable names in backticks (`) in comments, then they will get linkified in codesearch as well
updated :)
Related details
- Robert Flack
- 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. |
Free Debreuil (Gerrit)
Free Debreuil added 1 comment![]( "Open in Gerrit")
// called and momentum_animation_start_time. Using this information, theFree Debreuilnit if you wrap the variable names in backticks (`) in comments, then they will get linkified in codesearch as well
updated :)
Done