Fix: Avoid redundant draggable region updates. [chromium/src : main]
AI Code Reviewer (Gerrit)
AI Code Reviewer added 1 comment![]( "Open in Gerrit")
std::vector<WebDraggableRegion> web_regions =Blink Style Guide: Prefer blink:: types over STL and base types. Please use WTF::Vector (or WebVector if matching the return type) instead of std::vector.
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. |
Zoraiz Naeem (Gerrit)
Zoraiz Naeem voted and added 1 comment![]( "Open in Gerrit")
Votes added by Zoraiz Naeem
| Commit-Queue | +1 |
1 comment
Related details
- Daniel Cheng
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. |
Zoraiz Naeem (Gerrit)
Zoraiz Naeem voted and added 1 comment![]( "Open in Gerrit")
Votes added by Zoraiz Naeem
| Commit-Queue | +1 |
1 comment
std::vector<WebDraggableRegion> web_regions =Blink Style Guide: Prefer blink:: types over STL and base types. Please use WTF::Vector (or WebVector if matching the return type) instead of std::vector.
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)_
Done
Related details
- Daniel Cheng
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Cheng (Gerrit)
Daniel Cheng added 1 comment![]( "Open in Gerrit")
It's not clear what problem this fixes or why the update was unexpected/redundant.
Isn't it possible that the caller isn't supposed to be calling this function multiple times in a row if the state hasn't changed?
Also, note that this area is not owned by anyone at Google anymoreโand furthermore, changes have a high risk of randomly breaking things since a lot of the drag-and-drop paths are poorly understood. So at minimum, the CL description needs to be a lot clearer about what exactly is broken and what the fix is (so we can have some confidence that this isn't going to break something else that works fine today), and the CL itself needs to include some tests.
Related details
- Khushal Sagar
- Zoraiz Naeem
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. |
Khushal Sagar (Gerrit)
Khushal Sagar voted and added 2 comments![]( "Open in Gerrit")
Votes added by Khushal Sagar
| Code-Review | +1 |
2 comments
MainFrameImpl()->GetDocument().DraggableRegions();nitty nit: `local_frame->GetDocument()`
Took me a min to realize `local_frame` is the same as `MainFrameImpl()`. It reads odd because we update using `local_frame` below.
if (web_regions.empty()) {Not for this patch since this pattern is with existing code, how does `HasDraggableRegions()` relate to this vector being empty? Is it possible for the bool to be true while the vector is empty? Seems like duplicate state.
Related details
- Zoraiz Naeem
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. |
Zoraiz Naeem (Gerrit)
Zoraiz Naeem added 1 comment![]( "Open in Gerrit")
nitty nit: `local_frame->GetDocument()`
Took me a min to realize `local_frame` is the same as `MainFrameImpl()`. It reads odd because we update using `local_frame` below.
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. |