Extend TrackedElementRect to support sub-rectangle tracking [chromium/src : main]
Przemyslaw Szczepaniak (Gerrit)
Przemyslaw Szczepaniak has uploaded the change for review![]( "Open in Gerrit")
Commit message
Extend TrackedElementRect to support sub-rectangle tracking
This change allows tracking a specific sub-region of an element instead
of its entire bounds.
- Added SubRect struct to TrackedElementRect to hold a relative region
and an 'intersect_with_element_rect' flag.
- Introduced TrackedElementRect::GetEffectiveBounds() to handle
coordinate calculations and optional clipping.
- Updated BoxPainter, BoxFragmentPainter and InlineBoxFragmentPainter
to use the new sub-rectangle logic during paint recording.
- Added unit tests to verify sub-rectangle tracking with and without
intersection with the element bounds.
Change diff
Change information
- M third_party/blink/renderer/core/exported/web_plugin_container_impl.cc
- M third_party/blink/renderer/core/paint/box_fragment_painter.cc
- M third_party/blink/renderer/core/paint/box_fragment_painter_test.cc
- M third_party/blink/renderer/core/paint/box_painter.cc
- M third_party/blink/renderer/core/paint/inline_box_fragment_painter.cc
- M third_party/blink/renderer/platform/graphics/paint/tracked_element_data.cc
- M third_party/blink/renderer/platform/graphics/paint/tracked_element_data.h
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
AI Code Reviewer (Gerrit)
AI Code Reviewer added 2 comments![]( "Open in Gerrit")
bool intersect_with_element_rect) {Blink Style Guide: Prefer enums or StrongAliases to bare bools for function parameters. To improve readability at the call site, consider using a base::StrongAlias<class IntersectWithElementRectTag, bool> or an enum instead of a bare bool for 'intersect_with_element_rect'.
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)_
bool intersect_with_element_rect = true;nit: Blink Style Guide: Precede boolean values with words like “is” and “did”. Consider renaming 'intersect_with_element_rect' to 'should_intersect_with_element_rect' (or similar) to clearly indicate it is a boolean flag.
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. |
Przemyslaw Szczepaniak (Gerrit)
Przemyslaw Szczepaniak has uploaded the change for review![]( "Open in Gerrit")
Commit message
Extend TrackedElementRect to support sub-rectangle tracking
This change allows tracking a specific sub-region of an element instead
of its entire bounds.
- Added SubRect struct to TrackedElementRect to hold a relative region
and an 'intersect_with_element_rect' flag.
- Introduced TrackedElementRect::GetEffectiveBounds() to handle
coordinate calculations and optional clipping.
- Updated BoxPainter, BoxFragmentPainter and InlineBoxFragmentPainter
to use the new sub-rectangle logic during paint recording.
- Added unit tests to verify sub-rectangle tracking with and without
intersection with the element bounds.
Change diff
Change information
- M third_party/blink/renderer/core/exported/web_plugin_container_impl.cc
- M third_party/blink/renderer/core/paint/box_fragment_painter.cc
- M third_party/blink/renderer/core/paint/box_fragment_painter_test.cc
- M third_party/blink/renderer/core/paint/box_painter.cc
- M third_party/blink/renderer/core/paint/inline_box_fragment_painter.cc
- M third_party/blink/renderer/platform/graphics/paint/tracked_element_data.cc
- M third_party/blink/renderer/platform/graphics/paint/tracked_element_data.h
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Przemyslaw Szczepaniak (Gerrit)
Przemyslaw Szczepaniak abandoned this change![]( "Open in Gerrit")
Related details
Code-Coverage
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
AI Code Reviewer (Gerrit)
AI Code Reviewer added 1 comment![]( "Open in Gerrit")
static std::unique_ptr<TrackedElementRect> CreateFull(TrackedElementId id) {Blink Style Guide: Don't mix Create () factory methods and public constructors in one class. Since this struct has public constructors, please remove these `Create` methods and use `std::make_unique<TrackedElementRect>(...)` directly at the call sites.
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
- Khushal Sagar
- Philip Rogers
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. |
Philip Rogers (Gerrit)
Philip Rogers added 6 comments![]( "Open in Gerrit")
Pease add Bug: 459385624 to keep everything connected.
gfx::Rect rect = tracked_element_rect->GetEffectiveBounds(Should we move the call to GetEffectiveBounds down into
paint_info.context.GetPaintController().RecordTrackedElementData(Should we move the call to GetEffectiveBounds down into RecordTrackedElementData?
static std::unique_ptr<TrackedElementRect> CreateFull(TrackedElementId id) {Blink Style Guide: Don't mix Create () factory methods and public constructors in one class. Since this struct has public constructors, please remove these `Create` methods and use `std::make_unique<TrackedElementRect>(...)` directly at the call sites.
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)_
I agree with AI Code Reviewer, can we just have one create function?
gfx::Rect region;Region is a type which is a collection of rects, so this could be confusing. Can you rename this region -> rect?
// Represents a single tracked element, currently it tracks full elementPlease update this comment
Related details
- Khushal Sagar
- Przemyslaw Szczepaniak
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 added 2 comments![]( "Open in Gerrit")
gfx::Rect region;we should add a comment to clarify what this `region` is relative to. @p...@chromium.org any suggestions?
Since this is calculated using the geometry in APC, it will be based on the visible_bounding_box computed [here](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/modules/content_extraction/ai_page_content_agent.cc;l=116;drc=f3b299d131a2d94c2bc1cae88e703f62880cf60d). This patch is assuming that the visible_bounding_box is in the same coordinate space as visual_rect. Is that assumption correct? If so, what's the canonical way to refer to this coordinate space in the code..?
kIntersectWithElementRect,
kNoIntersection,Do we need both these cases right now? I'm assuming this sub-rect should always be a subset of the element's painted bounds. I'm not sure what we should be doing if the element is resized such that it's no longer a subset. But in that case the subset is probably incorrect anyway, it's relative to the element's previous painted size.
Related details
- Przemyslaw Szczepaniak
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. |
Przemyslaw Szczepaniak (Gerrit)
Przemyslaw Szczepaniak has uploaded the change for review![]( "Open in Gerrit")
Commit message
Extend TrackedElementRect to support sub-rectangle tracking
This change allows tracking a specific sub-region of an element instead
of its entire bounds.
- Added SubRect struct to TrackedElementRect to hold a relative region
and an 'intersect_with_element_rect' flag.
- Introduced TrackedElementRect::GetEffectiveBounds() to handle
coordinate calculations and optional clipping.
- Updated BoxPainter, BoxFragmentPainter and InlineBoxFragmentPainter
to use the new sub-rectangle logic during paint recording.
- Added unit tests to verify sub-rectangle tracking with and without
intersection with the element bounds.
Change diff
Change information
- M third_party/blink/renderer/core/exported/web_plugin_container_impl.cc
- M third_party/blink/renderer/core/page/scrolling/scrolling_test.cc
- M third_party/blink/renderer/core/paint/box_fragment_painter.cc
- M third_party/blink/renderer/core/paint/box_fragment_painter_test.cc
- M third_party/blink/renderer/core/paint/box_painter.cc
- M third_party/blink/renderer/core/paint/inline_box_fragment_painter.cc
- M third_party/blink/renderer/platform/graphics/paint/paint_controller.cc
- M third_party/blink/renderer/platform/graphics/paint/paint_controller.h
- M third_party/blink/renderer/platform/graphics/paint/paint_controller_test.cc
- M third_party/blink/renderer/platform/graphics/paint/tracked_element_data.cc
- M third_party/blink/renderer/platform/graphics/paint/tracked_element_data.h
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Przemyslaw Szczepaniak (Gerrit)
Przemyslaw Szczepaniak abandoned this change![]( "Open in Gerrit")
Related details
Code-Coverage
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Przemyslaw Szczepaniak (Gerrit)
Przemyslaw Szczepaniak added 9 comments![]( "Open in Gerrit")
Right, I would love to update revised version, but 'git cl upload --no-squish' for 2 dependant cls seems to hate the idea of adding Change-Id to the latter cl. It generates new cl for every invocation, will probably have to wait for the base cl to be submitted?
Pease add Bug: 459385624 to keep everything connected.
Done
gfx::Rect rect = tracked_element_rect->GetEffectiveBounds(Should we move the call to GetEffectiveBounds down into
Done
paint_info.context.GetPaintController().RecordTrackedElementData(Should we move the call to GetEffectiveBounds down into RecordTrackedElementData?
Done
static std::unique_ptr<TrackedElementRect> CreateFull(TrackedElementId id) {Philip RogersBlink Style Guide: Don't mix Create () factory methods and public constructors in one class. Since this struct has public constructors, please remove these `Create` methods and use `std::make_unique<TrackedElementRect>(...)` directly at the call sites.
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)_
I agree with AI Code Reviewer, can we just have one create function?
Done
Region is a type which is a collection of rects, so this could be confusing. Can you rename this region -> rect?
Done
gfx::Rect region;we should add a comment to clarify what this `region` is relative to. @p...@chromium.org any suggestions?
Since this is calculated using the geometry in APC, it will be based on the visible_bounding_box computed [here](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/modules/content_extraction/ai_page_content_agent.cc;l=116;drc=f3b299d131a2d94c2bc1cae88e703f62880cf60d). This patch is assuming that the visible_bounding_box is in the same coordinate space as visual_rect. Is that assumption correct? If so, what's the canonical way to refer to this coordinate space in the code..?
just to clarify the purpose of this field - we're going to have the rect in screen space coordinates, and then try to map it against elements, as seen in the APC.
Do we need both these cases right now? I'm assuming this sub-rect should always be a subset of the element's painted bounds. I'm not sure what we should be doing if the element is resized such that it's no longer a subset. But in that case the subset is probably incorrect anyway, it's relative to the element's previous painted size.
I would prefer to keep no-intersection it as an "escape hatch" option if we find that the intersection is not working correctly.
// Represents a single tracked element, currently it tracks full elementPrzemyslaw SzczepaniakPlease update this comment
Done
Related details
- Khushal Sagar
- Philip Rogers
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. |
Przemyslaw Szczepaniak (Gerrit)
Przemyslaw Szczepaniak added 1 comment![]( "Open in Gerrit")
Right, I would love to update revised version, but 'git cl upload --no-squish' for 2 dependant cls seems to hate the idea of adding Change-Id to the latter cl. It generates new cl for every invocation, will probably have to wait for the base cl to be submitted?
tried to generate a valid change-id for this one, but something is borked, best I could do is the new change with change-id: https://chromium-review.googlesource.com/c/chromium/src/+/7464882 , can we move the review there?
Przemyslaw Szczepaniak (Gerrit)
Przemyslaw Szczepaniak added 1 comment![]( "Open in Gerrit")
Przemyslaw SzczepaniakRight, I would love to update revised version, but 'git cl upload --no-squish' for 2 dependant cls seems to hate the idea of adding Change-Id to the latter cl. It generates new cl for every invocation, will probably have to wait for the base cl to be submitted?
tried to generate a valid change-id for this one, but something is borked, best I could do is the new change with change-id: https://chromium-review.googlesource.com/c/chromium/src/+/7464882 , can we move the review there?
Ignore previous messages, git cl patch into branch made it generated correct change Id, I pulled the other cl into this one.
Related details
- Khushal Sagar
- Philip Rogers
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. |
Przemyslaw Szczepaniak (Gerrit)
Przemyslaw Szczepaniak abandoned this change![]( "Open in Gerrit")
Related details
Code-Coverage
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Philip Rogers (Gerrit)
Philip Rogers voted and added 5 comments![]( "Open in Gerrit")
Votes added by Philip Rogers
| Code-Review | +1 |
5 comments
TrackedElementRect(TrackedElementId id,Please fix this WARNING reported by ClangTidy: check: google-explicit-constructor
constructors that are callable with a single...
check: google-explicit-constructor
constructors that are callable with a single argument must be marked explicit to avoid unintentional implicit conversions (https://clang.llvm.org/extra/clang-tidy/checks/google/explicit-constructor.html)
(Note: You can add `Skip-Clang-Tidy-Checks: google-explicit-constructor` footer to the CL description to skip the check)
(Lint observed on `linux-clang-tidy-rel`, but not on `android-clang-tidy-rel`)
// While tracking the rectangle, the output is intesection of specifiedPlease fix this WARNING reported by Spellchecker: "intesection" is a possible misspelling of "intersection".
To bypass Spellcheck...
"intesection" is a possible misspelling of "intersection".
To bypass Spellchecker, add a footer with DISABLE_SPELLCHECKER
Related details
- Khushal Sagar
- Przemyslaw Szczepaniak
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 1 comment![]( "Open in Gerrit")
Votes added by Khushal Sagar
| Code-Review | +1 |
1 comment
gfx::Rect region;Przemyslaw Szczepaniakwe should add a comment to clarify what this `region` is relative to. @p...@chromium.org any suggestions?
Since this is calculated using the geometry in APC, it will be based on the visible_bounding_box computed [here](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/modules/content_extraction/ai_page_content_agent.cc;l=116;drc=f3b299d131a2d94c2bc1cae88e703f62880cf60d). This patch is assuming that the visible_bounding_box is in the same coordinate space as visual_rect. Is that assumption correct? If so, what's the canonical way to refer to this coordinate space in the code..?
just to clarify the purpose of this field - we're going to have the rect in screen space coordinates, and then try to map it against elements, as seen in the APC.
Using APC to map from screen space coordinates to a subrect in the element makes sense. There's differences in coordinate spaces between APC and paint, APC works in layout/hit-testing space while paint will include visual effects (like shadow on a box). We'll have to figure out how to deal with this when writing the mapping.
The code in GetEffectiveBounds is making an assumption about the coordinate space for rect. That's why I was suggesting being explicit about it. Maybe `paint_subset` with a comment: "A subset relative to this element's visual/painted rect. The visual rect includes all effects from the element like shadow, filter etc." @p...@chromium.org can confirm that I got this right.
Related details
- Przemyslaw Szczepaniak
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. |