Add new TrackedElement type for cross-origin elements [chromium/src : main]
Ryan Kalla (Gerrit)
Ryan Kalla voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Khushal Sagar
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 3 comments![]( "Open in Gerrit")
frame->GetSecurityContext()->GetSecurityOrigin()->ToUrlOrigin()));Blink Style Guide: Prefer blink:: types over STL and base types. Avoid converting 'SecurityOrigin' to 'url::Origin' within Blink core; store 'SecurityOrigin' directly in 'TrackedElementRect'.
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)_
gfx::Rect GetEffectiveBounds(const gfx::Rect& element_paint_rect) const;Blink Style Guide: Prefer blink:: types over STL and base types. 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** | **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)_
std::optional<url::Origin> origin;Blink Style Guide: Prefer blink:: types over STL and base types. Use 'scoped_refptr<const SecurityOrigin>' instead of 'std::optional<url::Origin>'.
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
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 1 comment![]( "Open in Gerrit")
Sorry been meaning to get to this, will definitely take a look before monday. +p...@chromium.org for a pass as well.
Related details
- Philip Rogers
- Ryan Kalla
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 6 comments![]( "Open in Gerrit")
Left some comments. I'll be OOO starting tomorrow, deferring to @p...@chromium.org.
TrackedElementBounds LayerTreeHostImpl::CollectTrackedElementBounds() {There will be 2 consumers of this metadata:
1. Currently it's RenderFrameMetadata which sends the information to the browser. That's what AIHighlight needs.
2. With your change, we'll have it on CompositorFrameMetadata which sends the information to the GPU. That's what we need for the cross-origin case.
Add 2 new sets to LayerTreeSettings to configure which features go to each. Something like tracked_element_features_for_render_frame_metadata and tracked_element_features_for_compositor_frame_metadata. These can be initialized [here](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/widget/compositing/layer_tree_settings.cc;l=219;drc=bf712ec1a13783224debb691ba88ad5c15b93194).
Limiting what goes into RenderFrameMetadata is important to avoid redundant IPCs to the browser. And limiting it in CompositorFrameMetadata is probably good to avoid redundant geometry calculations later in the stack.
base::flat_map<TrackedElementFeature,Instead of switching to a map of feature -> rect, seems simpler to add a flat_set<TrackedElementFeature> to TrackedElementRectData for the set of features tracking an element. The per-feature optional fields on TrackedElementRectData is fine.
The current approach is complicating the code and also could have redundant geometry computation for the same node tracked by multiple features.
kCrossOriginFrameRedaction,nit: kCrossOriginFrameTracking
void HTMLElement::UpdateTrackedElementRect() {This won't be enough. Move the code to use the new tracking feature to a separate CL. We'll need to carefully figure out a good spot to ensure all cross-origin frames are covered.
explicit TrackedElementRect(TrackedElementId id,Same here, extend this to have a set of tracking features.
Philip Rogers (Gerrit)
Philip Rogers added 1 comment![]( "Open in Gerrit")
Meta question: It would be ideal if we could avoid massive changes for each feature using this codepath. You've added a new type for that which helps, but I wonder if we could go further and track this cross-origin information out-of-band? For example, could you use the existing element rect tracking api with no changes, and simply loop over all all cross-origin frames and track those rects, then add your own logic (in cc or later) to subtract out cross-origin tracked rects from the rest of the rects?
High-level question: can you help me understand your plan here? Can you give me the really simple example of what you're doing? For example, let's say we have the following page:
```
<!doctype html>
<div id=foo style="width: 200px; height: 200px; background: lightblue;">
<div id=bar style="width: 100px; height: 100px; background: lightgreen;"></div>
<iframe id=baz style="width: 100px; height: 100px; margin-top: -50px;" src="cross origin"></iframe>
</div>
```
What is your end-goal when tracking foo? What about when tracking bar?
Related details
- Ryan Kalla
Ryan Kalla (Gerrit)
Ryan Kalla added 1 comment![]( "Open in Gerrit")
Meta question: It would be ideal if we could avoid massive changes for each feature using this codepath. You've added a new type for that which helps, but I wonder if we could go further and track this cross-origin information out-of-band? For example, could you use the existing element rect tracking api with no changes, and simply loop over all all cross-origin frames and track those rects, then add your own logic (in cc or later) to subtract out cross-origin tracked rects from the rest of the rects?
High-level question: can you help me understand your plan here? Can you give me the really simple example of what you're doing? For example, let's say we have the following page:
```
<!doctype html>
<div id=foo style="width: 200px; height: 200px; background: lightblue;">
<div id=bar style="width: 100px; height: 100px; background: lightgreen;"></div>
<iframe id=baz style="width: 100px; height: 100px; margin-top: -50px;" src="cross origin"></iframe>
</div>
```
What is your end-goal when tracking foo? What about when tracking bar?
A large part of this CL deals with the restructuring of the TrackedElementBounds data (adding the feature enum and the TrackedElementMetadata struct). I think subsequent tracking features would be simpler to implement. At that point it should just be a matter of adding a new enum value and any extra optional metadata. I don't think we're able to use the existing tracking api as is, since we need to track the element origin as well (it's not enough to just know the frame is cross origin, we need to know which origin it comes from).
The end goal is to have a list of elements (specifically the element rect and the element origin) which represents all of the cross-origin iframes on the page. This element metadata would be returned along with the screen bitmap when CopyFromSurface is called in the browser process. We would use that element metadata to draw black boxes over certain iframes in the bitmap. The specific iframes which are drawn over are determined by the frame origin (matching that origin against an allowlist).
So we need to track the element rects (which is covered by the current tracking api), as well as the element origins (not currently supported).
In your example, we would just need to track "baz" element.
Related details
- Philip Rogers
Philip Rogers (Gerrit)
Philip Rogers added 1 comment![]( "Open in Gerrit")
Ryan KallaMeta question: It would be ideal if we could avoid massive changes for each feature using this codepath. You've added a new type for that which helps, but I wonder if we could go further and track this cross-origin information out-of-band? For example, could you use the existing element rect tracking api with no changes, and simply loop over all all cross-origin frames and track those rects, then add your own logic (in cc or later) to subtract out cross-origin tracked rects from the rest of the rects?
High-level question: can you help me understand your plan here? Can you give me the really simple example of what you're doing? For example, let's say we have the following page:
```
<!doctype html>
<div id=foo style="width: 200px; height: 200px; background: lightblue;">
<div id=bar style="width: 100px; height: 100px; background: lightgreen;"></div>
<iframe id=baz style="width: 100px; height: 100px; margin-top: -50px;" src="cross origin"></iframe>
</div>
```
What is your end-goal when tracking foo? What about when tracking bar?
A large part of this CL deals with the restructuring of the TrackedElementBounds data (adding the feature enum and the TrackedElementMetadata struct). I think subsequent tracking features would be simpler to implement. At that point it should just be a matter of adding a new enum value and any extra optional metadata. I don't think we're able to use the existing tracking api as is, since we need to track the element origin as well (it's not enough to just know the frame is cross origin, we need to know which origin it comes from).
The end goal is to have a list of elements (specifically the element rect and the element origin) which represents all of the cross-origin iframes on the page. This element metadata would be returned along with the screen bitmap when CopyFromSurface is called in the browser process. We would use that element metadata to draw black boxes over certain iframes in the bitmap. The specific iframes which are drawn over are determined by the frame origin (matching that origin against an allowlist).
So we need to track the element rects (which is covered by the current tracking api), as well as the element origins (not currently supported).
In your example, we would just need to track "baz" element.
Can the origin information be tracked out of band, rather than putting it in the tracked element bounds? For example, can you just maintain a list {frame element id, frame origin} pairs? This will be more efficient than dealing with origin information in the paint system. For example, I think you need to catch changes to origins and cause additional invalidations for those, and this would probably be best easier to solve out of the paint system.
Related details
- Ryan Kalla
Ryan Kalla (Gerrit)
Ryan Kalla added 1 comment![]( "Open in Gerrit")
Ryan KallaMeta question: It would be ideal if we could avoid massive changes for each feature using this codepath. You've added a new type for that which helps, but I wonder if we could go further and track this cross-origin information out-of-band? For example, could you use the existing element rect tracking api with no changes, and simply loop over all all cross-origin frames and track those rects, then add your own logic (in cc or later) to subtract out cross-origin tracked rects from the rest of the rects?
High-level question: can you help me understand your plan here? Can you give me the really simple example of what you're doing? For example, let's say we have the following page:
```
<!doctype html>
<div id=foo style="width: 200px; height: 200px; background: lightblue;">
<div id=bar style="width: 100px; height: 100px; background: lightgreen;"></div>
<iframe id=baz style="width: 100px; height: 100px; margin-top: -50px;" src="cross origin"></iframe>
</div>
```
What is your end-goal when tracking foo? What about when tracking bar?
Philip RogersA large part of this CL deals with the restructuring of the TrackedElementBounds data (adding the feature enum and the TrackedElementMetadata struct). I think subsequent tracking features would be simpler to implement. At that point it should just be a matter of adding a new enum value and any extra optional metadata. I don't think we're able to use the existing tracking api as is, since we need to track the element origin as well (it's not enough to just know the frame is cross origin, we need to know which origin it comes from).
The end goal is to have a list of elements (specifically the element rect and the element origin) which represents all of the cross-origin iframes on the page. This element metadata would be returned along with the screen bitmap when CopyFromSurface is called in the browser process. We would use that element metadata to draw black boxes over certain iframes in the bitmap. The specific iframes which are drawn over are determined by the frame origin (matching that origin against an allowlist).
So we need to track the element rects (which is covered by the current tracking api), as well as the element origins (not currently supported).
In your example, we would just need to track "baz" element.
Can the origin information be tracked out of band, rather than putting it in the tracked element bounds? For example, can you just maintain a list {frame element id, frame origin} pairs? This will be more efficient than dealing with origin information in the paint system. For example, I think you need to catch changes to origins and cause additional invalidations for those, and this would probably be best easier to solve out of the paint system.
I'm not super familiar with this part of the code, could you explain a bit more about where I could store the {element id, origin} mappings outside of the paint system? I think I would need to set them at the point I'm deciding to track an element (so somewhere at the Blink Element level where I have the ID and origin) and then I'll need to access them by the time I reach `cc/trees/layer_tree_host_impl.cc` in order to determine which elements need to get added to the CompositorFrameMetadata. Maybe adding them to the cc Layer in some way?
Related details
- Philip Rogers
Philip Rogers (Gerrit)
Philip Rogers added 1 comment![]( "Open in Gerrit")
Ryan KallaMeta question: It would be ideal if we could avoid massive changes for each feature using this codepath. You've added a new type for that which helps, but I wonder if we could go further and track this cross-origin information out-of-band? For example, could you use the existing element rect tracking api with no changes, and simply loop over all all cross-origin frames and track those rects, then add your own logic (in cc or later) to subtract out cross-origin tracked rects from the rest of the rects?
High-level question: can you help me understand your plan here? Can you give me the really simple example of what you're doing? For example, let's say we have the following page:
```
<!doctype html>
<div id=foo style="width: 200px; height: 200px; background: lightblue;">
<div id=bar style="width: 100px; height: 100px; background: lightgreen;"></div>
<iframe id=baz style="width: 100px; height: 100px; margin-top: -50px;" src="cross origin"></iframe>
</div>
```
What is your end-goal when tracking foo? What about when tracking bar?
Philip RogersA large part of this CL deals with the restructuring of the TrackedElementBounds data (adding the feature enum and the TrackedElementMetadata struct). I think subsequent tracking features would be simpler to implement. At that point it should just be a matter of adding a new enum value and any extra optional metadata. I don't think we're able to use the existing tracking api as is, since we need to track the element origin as well (it's not enough to just know the frame is cross origin, we need to know which origin it comes from).
The end goal is to have a list of elements (specifically the element rect and the element origin) which represents all of the cross-origin iframes on the page. This element metadata would be returned along with the screen bitmap when CopyFromSurface is called in the browser process. We would use that element metadata to draw black boxes over certain iframes in the bitmap. The specific iframes which are drawn over are determined by the frame origin (matching that origin against an allowlist).
So we need to track the element rects (which is covered by the current tracking api), as well as the element origins (not currently supported).
In your example, we would just need to track "baz" element.
Ryan KallaCan the origin information be tracked out of band, rather than putting it in the tracked element bounds? For example, can you just maintain a list {frame element id, frame origin} pairs? This will be more efficient than dealing with origin information in the paint system. For example, I think you need to catch changes to origins and cause additional invalidations for those, and this would probably be best easier to solve out of the paint system.
I'm not super familiar with this part of the code, could you explain a bit more about where I could store the {element id, origin} mappings outside of the paint system? I think I would need to set them at the point I'm deciding to track an element (so somewhere at the Blink Element level where I have the ID and origin) and then I'll need to access them by the time I reach `cc/trees/layer_tree_host_impl.cc` in order to determine which elements need to get added to the CompositorFrameMetadata. Maybe adding them to the cc Layer in some way?
A possible design is: you could add an api like... Page::GetAllIframeElementOriginPairs() which returns all of the iframe elements and their origins. You could call that and then turn around and call SetTrackedElementRect for all of these elements. On the receiving side, you could join your list of iframe elements and origins with the rects they produced, and then you'll have the same data as this patch.
You definitely don't have to do the design I'm describing, but I think the high-level options and tradeoffs need to be thought about. For example, what is the mechanism for notifying your code when an element's origin changes, iframes are added, etc?
Related details
- Ryan Kalla
Ryan Kalla (Gerrit)
Ryan Kalla added 1 comment![]( "Open in Gerrit")
Ryan KallaMeta question: It would be ideal if we could avoid massive changes for each feature using this codepath. You've added a new type for that which helps, but I wonder if we could go further and track this cross-origin information out-of-band? For example, could you use the existing element rect tracking api with no changes, and simply loop over all all cross-origin frames and track those rects, then add your own logic (in cc or later) to subtract out cross-origin tracked rects from the rest of the rects?
High-level question: can you help me understand your plan here? Can you give me the really simple example of what you're doing? For example, let's say we have the following page:
```
<!doctype html>
<div id=foo style="width: 200px; height: 200px; background: lightblue;">
<div id=bar style="width: 100px; height: 100px; background: lightgreen;"></div>
<iframe id=baz style="width: 100px; height: 100px; margin-top: -50px;" src="cross origin"></iframe>
</div>
```
What is your end-goal when tracking foo? What about when tracking bar?
Philip RogersA large part of this CL deals with the restructuring of the TrackedElementBounds data (adding the feature enum and the TrackedElementMetadata struct). I think subsequent tracking features would be simpler to implement. At that point it should just be a matter of adding a new enum value and any extra optional metadata. I don't think we're able to use the existing tracking api as is, since we need to track the element origin as well (it's not enough to just know the frame is cross origin, we need to know which origin it comes from).
The end goal is to have a list of elements (specifically the element rect and the element origin) which represents all of the cross-origin iframes on the page. This element metadata would be returned along with the screen bitmap when CopyFromSurface is called in the browser process. We would use that element metadata to draw black boxes over certain iframes in the bitmap. The specific iframes which are drawn over are determined by the frame origin (matching that origin against an allowlist).
So we need to track the element rects (which is covered by the current tracking api), as well as the element origins (not currently supported).
In your example, we would just need to track "baz" element.
Ryan KallaCan the origin information be tracked out of band, rather than putting it in the tracked element bounds? For example, can you just maintain a list {frame element id, frame origin} pairs? This will be more efficient than dealing with origin information in the paint system. For example, I think you need to catch changes to origins and cause additional invalidations for those, and this would probably be best easier to solve out of the paint system.
Philip RogersI'm not super familiar with this part of the code, could you explain a bit more about where I could store the {element id, origin} mappings outside of the paint system? I think I would need to set them at the point I'm deciding to track an element (so somewhere at the Blink Element level where I have the ID and origin) and then I'll need to access them by the time I reach `cc/trees/layer_tree_host_impl.cc` in order to determine which elements need to get added to the CompositorFrameMetadata. Maybe adding them to the cc Layer in some way?
A possible design is: you could add an api like... Page::GetAllIframeElementOriginPairs() which returns all of the iframe elements and their origins. You could call that and then turn around and call SetTrackedElementRect for all of these elements. On the receiving side, you could join your list of iframe elements and origins with the rects they produced, and then you'll have the same data as this patch.
You definitely don't have to do the design I'm describing, but I think the high-level options and tradeoffs need to be thought about. For example, what is the mechanism for notifying your code when an element's origin changes, iframes are added, etc?
I assume elements would be repainted if the content changes (e.g. from adding an iframe or changing the iframe source), so it seems like tying into the painting process would be a good way to handle that. From my manual testing this seemed to be the case - elements were redrawn and the tracking data was updated accordingly.
I'm hesitant to split this tracking feature into separate parts if we don't need to. There are more tracking features coming that might need their own metadata (password inputs and credit card forms for example). I was under the impression that TrackedElementData was made generic with the intent to be extended for other features with added metadata. It seems like it would be simpler to keep the tracked data in a single pipeline, rather than having separate paths tracking the different pieces of data that need to be joined later.
Btw, I'm in the process of making Khushal's suggested changes (removing the outer feature map from TrackedElementData and going back to a single map of {id, element_data} where element_data includes a set of features. I think this will cut down the diff size of this CL and simplify things more.
Related details
- Philip Rogers
Philip Rogers (Gerrit)
Philip Rogers added 1 comment![]( "Open in Gerrit")
I don't think we paint when origins change, for example, see the test in https://chromium-review.googlesource.com/c/chromium/src/+/2036269. Similarly, do we paint anything for a cross-origin iframe that is out-of-process?
Related details
- Ryan Kalla
Ryan Kalla (Gerrit)
Ryan Kalla added 8 comments![]( "Open in Gerrit")
TrackedElementBounds LayerTreeHostImpl::CollectTrackedElementBounds() {There will be 2 consumers of this metadata:
1. Currently it's RenderFrameMetadata which sends the information to the browser. That's what AIHighlight needs.
2. With your change, we'll have it on CompositorFrameMetadata which sends the information to the GPU. That's what we need for the cross-origin case.Add 2 new sets to LayerTreeSettings to configure which features go to each. Something like tracked_element_features_for_render_frame_metadata and tracked_element_features_for_compositor_frame_metadata. These can be initialized [here](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/widget/compositing/layer_tree_settings.cc;l=219;drc=bf712ec1a13783224debb691ba88ad5c15b93194).
Limiting what goes into RenderFrameMetadata is important to avoid redundant IPCs to the browser. And limiting it in CompositorFrameMetadata is probably good to avoid redundant geometry calculations later in the stack.
Done
Instead of switching to a map of feature -> rect, seems simpler to add a flat_set<TrackedElementFeature> to TrackedElementRectData for the set of features tracking an element. The per-feature optional fields on TrackedElementRectData is fine.
The current approach is complicating the code and also could have redundant geometry computation for the same node tracked by multiple features.
Done
kCrossOriginFrameRedaction,Ryan Kallanit: kCrossOriginFrameTracking
Done
This won't be enough. Move the code to use the new tracking feature to a separate CL. We'll need to carefully figure out a good spot to ensure all cross-origin frames are covered.
Done
frame->GetSecurityContext()->GetSecurityOrigin()->ToUrlOrigin()));Blink Style Guide: Prefer blink:: types over STL and base types. Avoid converting 'SecurityOrigin' to 'url::Origin' within Blink core; store 'SecurityOrigin' directly in 'TrackedElementRect'.
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)_
OK But Won't Fix: This data will be piped into the browser where we will want to use url::Origin.
gfx::Rect GetEffectiveBounds(const gfx::Rect& element_paint_rect) const;Blink Style Guide: Prefer blink:: types over STL and base types. 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** | **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)_
Invalid: Code removed.
Blink Style Guide: Prefer blink:: types over STL and base types. Use 'scoped_refptr<const SecurityOrigin>' instead of 'std::optional<url::Origin>'.
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)_
OK But Won't Fix: This data will be piped into the browser where we will want to use url::Origin.
Same here, extend this to have a set of tracking features.
- Khushal Sagar
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. |
Nan Lin (Gerrit)
Nan Lin added 1 comment![]( "Open in Gerrit")
std::optional<SubRect> sub_rect = std::nullopt,`Element::SetTrackedElementRect()` is only allowed to call once on an element. In the case that different features want to track different `SubRects`, it looks like this would not work as is? Also, the triggering of tracking for different features may be at different times.
Similarly, I think `Element::ClearTrackedElementRect()` should support un-tracking the element for a feature, not for all features.
Alternatively, a per-feature tracking may be cleaner, although it may need duplicated memory when an element is tracked by multiple features.
Related details
- Khushal Sagar
- Ryan Kalla
Ryan Kalla (Gerrit)
Ryan Kalla added 1 comment![]( "Open in Gerrit")
Thanks for the context! I'm trying to think through the scenarios here. If the iframe's source origin changes, but nothing is repainted, I think we would want to still be tracking the previous origin (associated with the content that was painted). Ultimately we want this metadata (rect + origin) to match what is shown on screen and what will make up the screenshot bitmap. If it's the main frame whose origin is changing, causing previously same-origin iframes to become cross-origin, I think we can handle that by tracking all iframes and making the cross-origin determination later on in the browser process if needed.
Either way though, I think extending TrackedElementData to support different tracking features with added metadata is still needed for upcoming work. If it turns out that we need to pull out the origin information and track it separately then we can do that without changing too much of this code.
Related details
- Khushal Sagar
- Philip Rogers
- Ryan Kalla
Ryan Kalla (Gerrit)
Ryan Kalla added 1 comment![]( "Open in Gerrit")
std::optional<SubRect> sub_rect = std::nullopt,`Element::SetTrackedElementRect()` is only allowed to call once on an element. In the case that different features want to track different `SubRects`, it looks like this would not work as is? Also, the triggering of tracking for different features may be at different times.
Similarly, I think `Element::ClearTrackedElementRect()` should support un-tracking the element for a feature, not for all features.
Alternatively, a per-feature tracking may be cleaner, although it may need duplicated memory when an element is tracked by multiple features.
Thanks for bringing this up! Yeah I think you're right, this breaks when multiple features want to track different `SubRects`. I think we could deal with this a few ways:
For Element:
1. We could go back to having Element store a map of `{TrackedElementFeature, TrackedElementRect}` (like in the previous patchset).
2. We could have Element define separate `SetTrackedElementRectForFeature` methods for each feature.
For TrackedElementData
1. We could have TrackedElementData be a 2d map of `{TrackedElementFeature, {TrackedElementId, TrackedElementMetadata}}` (previous patchset). The downside of that is dealing with the added code complexity of the extra maps.
2. We could have TrackedElementData be a map of `{TrackedElementFeature, [TrackedElementMetadata]}`. Basically swapping the inner element ID map with a vector of element metadata objects (each of which would include the element ID). That simplifies things a bit, and I don't think we ever need to lookup by element ID anyway.
3. We could have TrackedElementData define separate `{TrackedElementId, TrackedElementMetadata}` maps for each feature (e.g. `base::flat_map<TrackedElementId, TrackedElementMetadata> highlight_map;`).
There could be some duplicate geometry when multiple features are tracking the same element. But I think that will be pretty rare in practice.
Related details
- Khushal Sagar
- Nan Lin
- Philip Rogers
Nan Lin (Gerrit)
Nan Lin added 1 comment![]( "Open in Gerrit")
std::optional<SubRect> sub_rect = std::nullopt,Ryan Kalla`Element::SetTrackedElementRect()` is only allowed to call once on an element. In the case that different features want to track different `SubRects`, it looks like this would not work as is? Also, the triggering of tracking for different features may be at different times.
Similarly, I think `Element::ClearTrackedElementRect()` should support un-tracking the element for a feature, not for all features.
Alternatively, a per-feature tracking may be cleaner, although it may need duplicated memory when an element is tracked by multiple features.
Thanks for bringing this up! Yeah I think you're right, this breaks when multiple features want to track different `SubRects`. I think we could deal with this a few ways:
For Element:
1. We could go back to having Element store a map of `{TrackedElementFeature, TrackedElementRect}` (like in the previous patchset).
2. We could have Element define separate `SetTrackedElementRectForFeature` methods for each feature.
For TrackedElementData
1. We could have TrackedElementData be a 2d map of `{TrackedElementFeature, {TrackedElementId, TrackedElementMetadata}}` (previous patchset). The downside of that is dealing with the added code complexity of the extra maps.
2. We could have TrackedElementData be a map of `{TrackedElementFeature, [TrackedElementMetadata]}`. Basically swapping the inner element ID map with a vector of element metadata objects (each of which would include the element ID). That simplifies things a bit, and I don't think we ever need to lookup by element ID anyway.
3. We could have TrackedElementData define separate `{TrackedElementId, TrackedElementMetadata}` maps for each feature (e.g. `base::flat_map<TrackedElementId, TrackedElementMetadata> highlight_map;`).
There could be some duplicate geometry when multiple features are tracking the same element. But I think that will be pretty rare in practice.
That simplifies things a bit, and I don't think we ever need to lookup by element ID anyway.
If we never need to look up by element ID, do we actually need it? If not, option 2 seems to make sense, and we just need to track a list of `TrackedElementMetadata` by feature.
Related details
- Khushal Sagar
- Philip Rogers
- Ryan Kalla
Nan Lin (Gerrit)
Nan Lin added 1 comment![]( "Open in Gerrit")
std::optional<SubRect> sub_rect = std::nullopt,Ryan Kalla`Element::SetTrackedElementRect()` is only allowed to call once on an element. In the case that different features want to track different `SubRects`, it looks like this would not work as is? Also, the triggering of tracking for different features may be at different times.
Similarly, I think `Element::ClearTrackedElementRect()` should support un-tracking the element for a feature, not for all features.
Alternatively, a per-feature tracking may be cleaner, although it may need duplicated memory when an element is tracked by multiple features.
Nan LinThanks for bringing this up! Yeah I think you're right, this breaks when multiple features want to track different `SubRects`. I think we could deal with this a few ways:
For Element:
1. We could go back to having Element store a map of `{TrackedElementFeature, TrackedElementRect}` (like in the previous patchset).
2. We could have Element define separate `SetTrackedElementRectForFeature` methods for each feature.
For TrackedElementData
1. We could have TrackedElementData be a 2d map of `{TrackedElementFeature, {TrackedElementId, TrackedElementMetadata}}` (previous patchset). The downside of that is dealing with the added code complexity of the extra maps.
2. We could have TrackedElementData be a map of `{TrackedElementFeature, [TrackedElementMetadata]}`. Basically swapping the inner element ID map with a vector of element metadata objects (each of which would include the element ID). That simplifies things a bit, and I don't think we ever need to lookup by element ID anyway.
3. We could have TrackedElementData define separate `{TrackedElementId, TrackedElementMetadata}` maps for each feature (e.g. `base::flat_map<TrackedElementId, TrackedElementMetadata> highlight_map;`).
There could be some duplicate geometry when multiple features are tracking the same element. But I think that will be pretty rare in practice.
That simplifies things a bit, and I don't think we ever need to lookup by element ID anyway.
If we never need to look up by element ID, do we actually need it? If not, option 2 seems to make sense, and we just need to track a list of `TrackedElementMetadata` by feature.
Actually taking this back, there may be use cases that use the element ID for mapping, e.g. by DOM node ID, so we should keep the ID.
Ryan Kalla (Gerrit)
Ryan Kalla added 1 comment![]( "Open in Gerrit")
std::optional<SubRect> sub_rect = std::nullopt,Ryan Kalla`Element::SetTrackedElementRect()` is only allowed to call once on an element. In the case that different features want to track different `SubRects`, it looks like this would not work as is? Also, the triggering of tracking for different features may be at different times.
Similarly, I think `Element::ClearTrackedElementRect()` should support un-tracking the element for a feature, not for all features.
Alternatively, a per-feature tracking may be cleaner, although it may need duplicated memory when an element is tracked by multiple features.
Nan LinThanks for bringing this up! Yeah I think you're right, this breaks when multiple features want to track different `SubRects`. I think we could deal with this a few ways:
For Element:
1. We could go back to having Element store a map of `{TrackedElementFeature, TrackedElementRect}` (like in the previous patchset).
2. We could have Element define separate `SetTrackedElementRectForFeature` methods for each feature.
For TrackedElementData
1. We could have TrackedElementData be a 2d map of `{TrackedElementFeature, {TrackedElementId, TrackedElementMetadata}}` (previous patchset). The downside of that is dealing with the added code complexity of the extra maps.
2. We could have TrackedElementData be a map of `{TrackedElementFeature, [TrackedElementMetadata]}`. Basically swapping the inner element ID map with a vector of element metadata objects (each of which would include the element ID). That simplifies things a bit, and I don't think we ever need to lookup by element ID anyway.
3. We could have TrackedElementData define separate `{TrackedElementId, TrackedElementMetadata}` maps for each feature (e.g. `base::flat_map<TrackedElementId, TrackedElementMetadata> highlight_map;`).
There could be some duplicate geometry when multiple features are tracking the same element. But I think that will be pretty rare in practice.
Nan LinThat simplifies things a bit, and I don't think we ever need to lookup by element ID anyway.
If we never need to look up by element ID, do we actually need it? If not, option 2 seems to make sense, and we just need to track a list of `TrackedElementMetadata` by feature.
Actually taking this back, there may be use cases that use the element ID for mapping, e.g. by DOM node ID, so we should keep the ID.
We could still stick the ID in the `TrackedElementMetadata` struct. The consumer of the `TrackedElementData` could search through the list of metadata objects to find an element by ID, or even build out a `{TrackedElementId, TrackedElementMetadata}` map from the list if needed. I was more thinking that this base part of the code, which is building up and passing around the `TrackedElementData`, doesn't necessarily need to lookup the elements by ID.
Just trading off some of the code complexity a bit. Having a simpler struct here, but the consumer may need to do a bit more work.
Related details
- Khushal Sagar
- Nan Lin
- Philip Rogers
Nan Lin (Gerrit)
Nan Lin added 1 comment![]( "Open in Gerrit")
std::optional<SubRect> sub_rect = std::nullopt,Ryan Kalla`Element::SetTrackedElementRect()` is only allowed to call once on an element. In the case that different features want to track different `SubRects`, it looks like this would not work as is? Also, the triggering of tracking for different features may be at different times.
Similarly, I think `Element::ClearTrackedElementRect()` should support un-tracking the element for a feature, not for all features.
Alternatively, a per-feature tracking may be cleaner, although it may need duplicated memory when an element is tracked by multiple features.
Nan LinThanks for bringing this up! Yeah I think you're right, this breaks when multiple features want to track different `SubRects`. I think we could deal with this a few ways:
For Element:
1. We could go back to having Element store a map of `{TrackedElementFeature, TrackedElementRect}` (like in the previous patchset).
2. We could have Element define separate `SetTrackedElementRectForFeature` methods for each feature.
For TrackedElementData
1. We could have TrackedElementData be a 2d map of `{TrackedElementFeature, {TrackedElementId, TrackedElementMetadata}}` (previous patchset). The downside of that is dealing with the added code complexity of the extra maps.
2. We could have TrackedElementData be a map of `{TrackedElementFeature, [TrackedElementMetadata]}`. Basically swapping the inner element ID map with a vector of element metadata objects (each of which would include the element ID). That simplifies things a bit, and I don't think we ever need to lookup by element ID anyway.
3. We could have TrackedElementData define separate `{TrackedElementId, TrackedElementMetadata}` maps for each feature (e.g. `base::flat_map<TrackedElementId, TrackedElementMetadata> highlight_map;`).
There could be some duplicate geometry when multiple features are tracking the same element. But I think that will be pretty rare in practice.
Nan LinThat simplifies things a bit, and I don't think we ever need to lookup by element ID anyway.
If we never need to look up by element ID, do we actually need it? If not, option 2 seems to make sense, and we just need to track a list of `TrackedElementMetadata` by feature.
Ryan KallaActually taking this back, there may be use cases that use the element ID for mapping, e.g. by DOM node ID, so we should keep the ID.
We could still stick the ID in the `TrackedElementMetadata` struct. The consumer of the `TrackedElementData` could search through the list of metadata objects to find an element by ID, or even build out a `{TrackedElementId, TrackedElementMetadata}` map from the list if needed. I was more thinking that this base part of the code, which is building up and passing around the `TrackedElementData`, doesn't necessarily need to lookup the elements by ID.
Just trading off some of the code complexity a bit. Having a simpler struct here, but the consumer may need to do a bit more work.
Yeah, that makes sense. I think this also allows for more flexibility on the identifiers, e.g. if we ever need more than 128 bits.
Related details
- Khushal Sagar
- Philip Rogers
- Ryan Kalla