Refactor TrackedElementData to support multiple tracking features [chromium/src : main]
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.
Nan LinWe 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.
Nan LinYeah, that makes sense. I think this also allows for more flexibility on the identifiers, e.g. if we ever need more than 128 bits.
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. |
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?
Ryan KallaA 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?
Philip RogersI 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.
Ryan KallaI 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?
Ryan KallaThanks 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.
@p...@chromium.org I've updated this CL to remove the origin information from the tracked element structs. Whether we add it back here, or track the origin mappings out of band is tbd based on a more fleshed out design which takes into account the concerns above.
Philip Rogers (Gerrit)
Philip Rogers added 1 comment![]( "Open in Gerrit")
Sorry, I ran out of time before getting to this review today. I will review this tomorrow. I don't have high-level concerns, just want to do a line-by-line review.
Related details
- Khushal Sagar
- Ryan Kalla
Nan Lin (Gerrit)
Nan Lin added 1 comment![]( "Open in Gerrit")
settings().tracked_element_features_for_render_frame_metadata);I'm not familiar with this code, but just curious, does this have to be set in `LayerTreeSettings` instead of being a static set in this file? I see there are various code paths that create the setting, do we need to initialize all of them?
Philip Rogers (Gerrit)
Philip Rogers added 4 comments![]( "Open in Gerrit")
settings().tracked_element_features_for_render_frame_metadata);I'm not familiar with this code, but just curious, does this have to be set in `LayerTreeSettings` instead of being a static set in this file? I see there are various code paths that create the setting, do we need to initialize all of them?
+1, this doesn't seem right. We shouldn't use settings for this at all?
base::flat_map<TrackedElementFeature, std::vector<TrackedElementRectData>>;We used to have a map from ElementId -> Rect, which only allows one rect per element id. We now have a map from Feature -> vector of ElementId+rect. Using a vector means that we now allow for there to be multiple rects for the same ElementId and feature. I think you don't need this? WDYT of switching to:
```
using TrackedElementBounds =
base::flat_map<TrackedElementId, TrackedElementRectData>;
using TrackedFeatures =
base::flat_map< TrackedElementFeature, TrackedElementBounds >;
```
?
// This method may be called at most once per feature. The ID must beThe "id" concept is now gone, so please remove those references in the comments.
const cc::Layer* cc_layer =The new tests in blink are updated to pass a feature, which is good, but can you also add at least one test of tracking multiple features simultaneously?
Nan Lin (Gerrit)
Nan Lin added 1 comment![]( "Open in Gerrit")
base::flat_map<TrackedElementFeature, std::vector<TrackedElementRectData>>;We used to have a map from ElementId -> Rect, which only allows one rect per element id. We now have a map from Feature -> vector of ElementId+rect. Using a vector means that we now allow for there to be multiple rects for the same ElementId and feature. I think you don't need this? WDYT of switching to:
```
using TrackedElementBounds =
base::flat_map<TrackedElementId, TrackedElementRectData>;
using TrackedFeatures =
base::flat_map< TrackedElementFeature, TrackedElementBounds >;
```
?
Currently `TrackedElementId` is just a `base::Token`. There may be use cases to track the element uniquely by both [`LocalFrameToken`](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/public/common/tokens/tokens.h;drc=976a71ef2a509d4391d814b52b9f4700fdf35789;l=34) and [`DOMNodeId`](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/graphics/dom_node_id.h;l=13;drc=976a71ef2a509d4391d814b52b9f4700fdf35789;bpv=1;bpt=1). These two would not fit into a single `base::Token`. So I feel like keeping the element ID in the structure instead of a key of the map may make it easier to extend? Or are we fine with e.g. using a list of `base::Token` as the map key?
Philip Rogers (Gerrit)
Philip Rogers added 1 comment![]( "Open in Gerrit")
base::flat_map<TrackedElementFeature, std::vector<TrackedElementRectData>>;Nan LinWe used to have a map from ElementId -> Rect, which only allows one rect per element id. We now have a map from Feature -> vector of ElementId+rect. Using a vector means that we now allow for there to be multiple rects for the same ElementId and feature. I think you don't need this? WDYT of switching to:
```
using TrackedElementBounds =
base::flat_map<TrackedElementId, TrackedElementRectData>;
using TrackedFeatures =
base::flat_map< TrackedElementFeature, TrackedElementBounds >;
```
?
Currently `TrackedElementId` is just a `base::Token`. There may be use cases to track the element uniquely by both [`LocalFrameToken`](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/public/common/tokens/tokens.h;drc=976a71ef2a509d4391d814b52b9f4700fdf35789;l=34) and [`DOMNodeId`](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/graphics/dom_node_id.h;l=13;drc=976a71ef2a509d4391d814b52b9f4700fdf35789;bpv=1;bpt=1). These two would not fit into a single `base::Token`. So I feel like keeping the element ID in the structure instead of a key of the map may make it easier to extend? Or are we fine with e.g. using a list of `base::Token` as the map key?
That's a fair point, but can we keep this change scoped down to just adding the feature enum, and address the token key question in a followup? Or, is it important that we address this here?
Nan Lin (Gerrit)
Nan Lin added 1 comment![]( "Open in Gerrit")
base::flat_map<TrackedElementFeature, std::vector<TrackedElementRectData>>;Nan LinWe used to have a map from ElementId -> Rect, which only allows one rect per element id. We now have a map from Feature -> vector of ElementId+rect. Using a vector means that we now allow for there to be multiple rects for the same ElementId and feature. I think you don't need this? WDYT of switching to:
```
using TrackedElementBounds =
base::flat_map<TrackedElementId, TrackedElementRectData>;
using TrackedFeatures =
base::flat_map< TrackedElementFeature, TrackedElementBounds >;
```
?
Philip RogersCurrently `TrackedElementId` is just a `base::Token`. There may be use cases to track the element uniquely by both [`LocalFrameToken`](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/public/common/tokens/tokens.h;drc=976a71ef2a509d4391d814b52b9f4700fdf35789;l=34) and [`DOMNodeId`](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/graphics/dom_node_id.h;l=13;drc=976a71ef2a509d4391d814b52b9f4700fdf35789;bpv=1;bpt=1). These two would not fit into a single `base::Token`. So I feel like keeping the element ID in the structure instead of a key of the map may make it easier to extend? Or are we fine with e.g. using a list of `base::Token` as the map key?
That's a fair point, but can we keep this change scoped down to just adding the feature enum, and address the token key question in a followup? Or, is it important that we address this here?
Yeah we can definitely address this in a follow up. But the other question is do we really need the nested map.
@ryan...@google.com compared these two options in this comment thread: https://chromium-review.googlesource.com/c/chromium/src/+/7506970/comment/7b33dee8_c18047db/
Ryan Kalla (Gerrit)
Ryan Kalla added 1 comment![]( "Open in Gerrit")
base::flat_map<TrackedElementFeature, std::vector<TrackedElementRectData>>;Nan LinWe used to have a map from ElementId -> Rect, which only allows one rect per element id. We now have a map from Feature -> vector of ElementId+rect. Using a vector means that we now allow for there to be multiple rects for the same ElementId and feature. I think you don't need this? WDYT of switching to:
```
using TrackedElementBounds =
base::flat_map<TrackedElementId, TrackedElementRectData>;
using TrackedFeatures =
base::flat_map< TrackedElementFeature, TrackedElementBounds >;
```
?
Philip RogersCurrently `TrackedElementId` is just a `base::Token`. There may be use cases to track the element uniquely by both [`LocalFrameToken`](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/public/common/tokens/tokens.h;drc=976a71ef2a509d4391d814b52b9f4700fdf35789;l=34) and [`DOMNodeId`](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/graphics/dom_node_id.h;l=13;drc=976a71ef2a509d4391d814b52b9f4700fdf35789;bpv=1;bpt=1). These two would not fit into a single `base::Token`. So I feel like keeping the element ID in the structure instead of a key of the map may make it easier to extend? Or are we fine with e.g. using a list of `base::Token` as the map key?
Nan LinThat's a fair point, but can we keep this change scoped down to just adding the feature enum, and address the token key question in a followup? Or, is it important that we address this here?
Yeah we can definitely address this in a follow up. But the other question is do we really need the nested map.
@ryan...@google.com compared these two options in this comment thread: https://chromium-review.googlesource.com/c/chromium/src/+/7506970/comment/7b33dee8_c18047db/
I'd prefer not to have the nested map if possible, to keep the data structures a bit simpler. Given that `Element::SetTrackedElementRect` can only be called once per feature I don't think we will end up with the same element being duplicated in any of the `TrackedElementRectData` lists.
But it's true that it's not enforced by the `TrackedElementBounds` struct anymore, and I'm not super familiar with how the BoxPainter, BoxFragmentPainter, etc. are invoked. For example if multiple of those painter classes are being triggered for the same element, then that element could get added to the list multiple times (but I'm not sure if that ever happens).
Related details
- Khushal Sagar
- Nan Lin
- Philip Rogers
Ryan Kalla (Gerrit)
Ryan Kalla added 3 comments![]( "Open in Gerrit")
settings().tracked_element_features_for_render_frame_metadata);Philip RogersI'm not familiar with this code, but just curious, does this have to be set in `LayerTreeSettings` instead of being a static set in this file? I see there are various code paths that create the setting, do we need to initialize all of them?
+1, this doesn't seem right. We shouldn't use settings for this at all?
Done
// This method may be called at most once per feature. The ID must beThe "id" concept is now gone, so please remove those references in the comments.
Done
The new tests in blink are updated to pass a feature, which is good, but can you also add at least one test of tracking multiple features simultaneously?
Done
Related details
- Khushal Sagar
- Nan Lin
- 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 2 comments![]( "Open in Gerrit")
base::flat_map<TrackedElementFeature, std::vector<TrackedElementRectData>>;Nan LinWe used to have a map from ElementId -> Rect, which only allows one rect per element id. We now have a map from Feature -> vector of ElementId+rect. Using a vector means that we now allow for there to be multiple rects for the same ElementId and feature. I think you don't need this? WDYT of switching to:
```
using TrackedElementBounds =
base::flat_map<TrackedElementId, TrackedElementRectData>;
using TrackedFeatures =
base::flat_map< TrackedElementFeature, TrackedElementBounds >;
```
?
Philip RogersCurrently `TrackedElementId` is just a `base::Token`. There may be use cases to track the element uniquely by both [`LocalFrameToken`](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/public/common/tokens/tokens.h;drc=976a71ef2a509d4391d814b52b9f4700fdf35789;l=34) and [`DOMNodeId`](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/graphics/dom_node_id.h;l=13;drc=976a71ef2a509d4391d814b52b9f4700fdf35789;bpv=1;bpt=1). These two would not fit into a single `base::Token`. So I feel like keeping the element ID in the structure instead of a key of the map may make it easier to extend? Or are we fine with e.g. using a list of `base::Token` as the map key?
Nan LinThat's a fair point, but can we keep this change scoped down to just adding the feature enum, and address the token key question in a followup? Or, is it important that we address this here?
Ryan KallaYeah we can definitely address this in a follow up. But the other question is do we really need the nested map.
@ryan...@google.com compared these two options in this comment thread: https://chromium-review.googlesource.com/c/chromium/src/+/7506970/comment/7b33dee8_c18047db/
I'd prefer not to have the nested map if possible, to keep the data structures a bit simpler. Given that `Element::SetTrackedElementRect` can only be called once per feature I don't think we will end up with the same element being duplicated in any of the `TrackedElementRectData` lists.
But it's true that it's not enforced by the `TrackedElementBounds` struct anymore, and I'm not super familiar with how the BoxPainter, BoxFragmentPainter, etc. are invoked. For example if multiple of those painter classes are being triggered for the same element, then that element could get added to the list multiple times (but I'm not sure if that ever happens).
This may be a problem, for example:
```
TEST_P(BoxFragmentPainterTest, TrackElementDivFragmented) {
SetBodyInnerHTML(R"HTML(
<div style="columns:3; column-fill:auto; height:50px; position: relative; overflow: hidden;">
<div id="target" style="width: 100px; height: 60px; background: linear-gradient(red, blue);">
</div>
</div>
)HTML");
// Initially all the texts are painted.
auto* target = GetDocument().getElementById(AtomicString("target"));
auto highlight_id = base::Token(1, 2);
auto highlight =
std::make_unique<TrackedElementRect>(TrackedElementId(highlight_id));
target->SetTrackedElementRect(std::move(highlight));
UpdateAllLifecyclePhasesForTest();
}
```
This will paint #target twice, one for each column. Will this be an issue for this approach? Note that this is probably a problem before this patch too, so I am okay with not fixing this. If this is an issue, can you file a bug for fixing it in a followup?
third_party/blink/renderer/platform/graphics/paint/tracked_element_data.h has:
1. TrackedElementRect: an optional SubRect and an id.
2. TrackedElementMap: maps a feature enum to a TrackedElementRect.
3. TrackedElementMetadata: a rect and an id.
4. TrackedElementData: maps a feature enum to a vector of TrackedElementMetadata.
The "input" blink API is on Element:
SetTrackedElementRect(feature enum, TrackedElementRect)
The "output" blink API is on Layer:
SetTrackedElementBounds(TrackedElementBounds)
cc/trees/tracked_element_bounds.h has:
1. TrackedElementRectData: an id and a rect.
2. TrackedElementBounds: maps a feature enum to a vector of TrackedElementRectData.
These names are too confusing, mixing Rect/Bounds/Data/Metadata/etc.
What do you think of the following ideas:
- Only use a single TrackedElementFeature enum, defined in cc. cc/paint/element_id.h has an example of a cc datatype used in both cc and blink, and I think you can use the same pattern.
- Blink needs to track both subrect data and rect data. What if the data types were TrackedElementSubRect for the id+SubRect and TrackedElementRect for the id+rect?
- What if we use the pattern of TrackedElementRect being the id+rect data, and TrackedElementRects being the map<feature, TrackedElementRect>? This would mean renaming TrackedElementMap -> TrackedElementSubRects, etc.
- Remove all complex logic from the painter classes, and have them simply pass the data from the element down to paint controller.
Related details
- Khushal Sagar
- Nan Lin
- Ryan Kalla
Ryan Kalla (Gerrit)
Ryan Kalla added 2 comments![]( "Open in Gerrit")
Thanks for the example! I created a bug to track this: http://crbug.com/483096628.
third_party/blink/renderer/platform/graphics/paint/tracked_element_data.h has:
1. TrackedElementRect: an optional SubRect and an id.
2. TrackedElementMap: maps a feature enum to a TrackedElementRect.
3. TrackedElementMetadata: a rect and an id.
4. TrackedElementData: maps a feature enum to a vector of TrackedElementMetadata.The "input" blink API is on Element:
SetTrackedElementRect(feature enum, TrackedElementRect)The "output" blink API is on Layer:
SetTrackedElementBounds(TrackedElementBounds)cc/trees/tracked_element_bounds.h has:
1. TrackedElementRectData: an id and a rect.
2. TrackedElementBounds: maps a feature enum to a vector of TrackedElementRectData.These names are too confusing, mixing Rect/Bounds/Data/Metadata/etc.
What do you think of the following ideas:
- Only use a single TrackedElementFeature enum, defined in cc. cc/paint/element_id.h has an example of a cc datatype used in both cc and blink, and I think you can use the same pattern.
- Blink needs to track both subrect data and rect data. What if the data types were TrackedElementSubRect for the id+SubRect and TrackedElementRect for the id+rect?
- What if we use the pattern of TrackedElementRect being the id+rect data, and TrackedElementRects being the map<feature, TrackedElementRect>? This would mean renaming TrackedElementMap -> TrackedElementSubRects, etc.
- Remove all complex logic from the painter classes, and have them simply pass the data from the element down to paint controller.
Yeah that makes sense. I made updates to use the `TrackedElementRect` and `TrackedElementRects` naming conventions.
Related details
- Khushal Sagar
- Nan Lin
- 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 voted and added 9 comments![]( "Open in Gerrit")
Votes added by Philip Rogers
| Code-Review | +1 |
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.
Philip Rogers@p...@chromium.org I've updated this CL to remove the origin information from the tracked element structs. Whether we add it back here, or track the origin mappings out of band is tbd based on a more fleshed out design which takes into account the concerns above.
Sorry, I ran out of time before getting to this review today. I will review this tomorrow. I don't have high-level concerns, just want to do a line-by-line review.
Done
if (!element_map.contains(item.feature)) {Can this be:
```
for (const auto& item : element_rect_data_list) {
element_map[item.feature].emplace_back(item.tracked_element_id, item.visible_bounds);
}
```
if (!rects.contains(feature)) {similar question as in third_party/blink/renderer/platform/graphics/paint/tracked_element_data.h
base::flat_map<TrackedElementFeature, std::vector<TrackedElementRectData>>;Done
if (!tracked_element_rects_.contains(feature)) {Can this be:
```
tracked_element_rects_[feature].push_back(std::move(rect_data));
```
?
if (!chunk.tracked_element_rects->map.contains(feature)) {Can this be:
```
chunk.tracked_element_rects->map[feature].push_back(tracked_element_rect);
```
?
struct PLATFORM_EXPORT TrackedElementRectsCan this just be:
```
using TrackedElementRects =
base::flat_map<cc::TrackedElementFeature, std::vector<TrackedElementRect>>;
```
?
// being tracked by that feature. The element identifier is a randomlynit: there's no element identifier on this
Related details
- Khushal Sagar
- Nan Lin
- 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. |
Ryan Kalla (Gerrit)
Ryan Kalla added 6 comments![]( "Open in Gerrit")
Can this be:
```
for (const auto& item : element_rect_data_list) {
element_map[item.feature].emplace_back(item.tracked_element_id, item.visible_bounds);
}
```
Done
similar question as in third_party/blink/renderer/platform/graphics/paint/tracked_element_data.h
Done
Can this be:
```
tracked_element_rects_[feature].push_back(std::move(rect_data));
```
?
Done
if (!chunk.tracked_element_rects->map.contains(feature)) {Can this be:
```
chunk.tracked_element_rects->map[feature].push_back(tracked_element_rect);
```
?
Done
Can this just be:
```
using TrackedElementRects =
base::flat_map<cc::TrackedElementFeature, std::vector<TrackedElementRect>>;
```
?
It seems like `TrackedElementRects` needs to extend GarbageCollected in order to be a member var of PaintChunk.
// being tracked by that feature. The element identifier is a randomlynit: there's no element identifier on this
Done
Related details
- Khushal Sagar
- Nan Lin
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ryan Kalla (Gerrit)
Ryan Kalla added 1 comment![]( "Open in Gerrit")
Hey @pszcze...@google.com
Can you take a look at `chrome/browser/ui/context_highlight/`?
This CL extends the TrackedElement structs to support multiple tracking features. There's been some refactoring of the data structures to include the new enum, and some renaming of the classes for more consistency between the Blink & CC layers.
Related details
- Khushal Sagar
- Nan Lin
- Przemyslaw Szczepaniak
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ryan Kalla (Gerrit)
Ryan Kalla voted and added 1 comment![]( "Open in Gerrit")
Votes added by Ryan Kalla
| Commit-Queue | +1 |
1 comment
Hi dcheng, can you take a look at:
third_party/blink/tools/blinkpy/presubmit/audit_non_blink_usage.py
Related details
- Daniel Cheng
- Khushal Sagar
- Nan Lin
- Przemyslaw Szczepaniak
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
gwsq (Gerrit)
Message from gwsq![]( "Open in Gerrit")
From googleclient/chrome/chromium_gwsq/ipc/config.gwsq:
IPC: dch...@chromium.org, d...@chromium.org
📎 It looks like you’re making a possibly security-sensitive change! 📎 IPC security review isn’t a rubberstamp, so your friendly security reviewer will need a fair amount of context to review your CL effectively. Please review your CL description and code comments to make sure they provide context for someone unfamiliar with your project/area. Pay special attention to where data comes from and which processes it flows between (and their privilege levels). Feel free to point your security reviewer at design docs, bugs, or other links if you can’t reasonably make a self-contained CL description. (Also see https://cbea.ms/git-commit/).
IPC reviewer(s): dch...@chromium.org, d...@chromium.org
Reviewer source(s):
d...@chromium.org is from context(googleclient/chrome/chromium_gwsq/ipc/config.gwsq)
Related details
- Daniel Cheng
- Dominic Farolino
- Khushal Sagar
- Nan Lin
- Przemyslaw Szczepaniak
Code-Coverage
Code-Owners
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 3 comments![]( "Open in Gerrit")
Votes added by Philip Rogers
| Code-Review | +1 |
LGTM
void SetTrackedElementRects(TrackedElementRects bounds);Optional nit: rename bounds -> rects (to match .cc file)
struct PLATFORM_EXPORT TrackedElementRectsRyan KallaCan this just be:
```
using TrackedElementRects =
base::flat_map<cc::TrackedElementFeature, std::vector<TrackedElementRect>>;
```
?
It seems like `TrackedElementRects` needs to extend GarbageCollected in order to be a member var of PaintChunk.
Ack. We talked about this before, and I forgot it was because of GC.
Related details
- Daniel Cheng
- Dominic Farolino
- Khushal Sagar
- Nan Lin
- Przemyslaw Szczepaniak
- Ryan Kalla
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 8 comments![]( "Open in Gerrit")
array<TrackedElementRect> element_data;This feels like it should be a map of features to TrackedElementRect.
static std::vector<TrackedElementRect> element_data(Then we wouldn't have to build a temporary vector just to serialize it.
const base::flat_set<TrackedElementFeature>&
GetTrackedElementFeaturesForRenderFrameMetadata() {
static const base::NoDestructor<base::flat_set<TrackedElementFeature>> kSet(
{TrackedElementFeature::kAIHighlight});
return *kSet;
}```
constexpr auto kTrackedElementFeaturesForRenderFrameMetadata =
base::MakeFixedFlatSet<TrackedElementFeature>({
TrackedElementFeature::kAIHighlight});
```
const base::flat_set<TrackedElementFeature>& included_features) {(Do we really need to take an argument here instead of just using the set directly?)
using TrackedElementId = base::Token;This is pre-existing, but is there is a reason this is specifically a base::Token and not a base::UnguessableToken?
bool operator==(const TrackedElementRect& other) const {
return id == other.id && bounds == other.bounds;
}
bool operator!=(const TrackedElementRect& other) const {
return !(*this == other);
}```
bool operator==(const TrackedElementRect&) = default;
```
Should work.
std::unique_ptr<TrackedElementSubRect>>;Is there a specific reason we need unique_ptr here? Do we need address stability for the sub rect? Otherwise, just make TrackedElementSubRect movable/copyable and store it by value without the indirection?
for (auto it2 = vec.begin(); it2 != vec.end(); ++it2) {
if (it2 != vec.begin()) {
sb.Append(", ");
}
sb.Append(it2->ToString());
}```suggestion
for (const auto& rect : vec) {
if (&rect != &*vec.begin()) {
sb.Append(", ");
}
sb.Append(rect.ToString());
}
```
Related details
- Dominic Farolino
- Khushal Sagar
- Nan Lin
- Przemyslaw Szczepaniak
- Ryan Kalla
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
Dominic Farolino (Gerrit)
Dominic Farolino added 1 comment![]( "Open in Gerrit")
Moving myself to CC since dcheng@ has already taken a look at the traits changes.
Related details
- Khushal Sagar
- Nan Lin
- Przemyslaw Szczepaniak
- 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. |
Ryan Kalla (Gerrit)
Ryan Kalla added 8 comments![]( "Open in Gerrit")
This feels like it should be a map of features to TrackedElementRect.
Done
Then we wouldn't have to build a temporary vector just to serialize it.
Done
const base::flat_set<TrackedElementFeature>&
GetTrackedElementFeaturesForRenderFrameMetadata() {
static const base::NoDestructor<base::flat_set<TrackedElementFeature>> kSet(
{TrackedElementFeature::kAIHighlight});
return *kSet;
}```
constexpr auto kTrackedElementFeaturesForRenderFrameMetadata =
base::MakeFixedFlatSet<TrackedElementFeature>({
TrackedElementFeature::kAIHighlight});
```
This set is passed in as an argument to `CollectTrackedElementRects`. My understanding is that a FixedFlatSet type def needs a size as well, i.e. `FixedFlatSet<TrackedElementFeature, size>`. In the next CL we will have a second set of features for the compositor frame metadata, and these two sets may have different sizes, making it difficult to use as an arg to that function. I'd prefer to keep the `base::flat_set` for that flexibility.
const base::flat_set<TrackedElementFeature>& included_features) {(Do we really need to take an argument here instead of just using the set directly?)
In the next CL we will also use this function to get Tracked Elements for the compositor frame metadata, which will have a different set of features.
using TrackedElementId = base::Token;This is pre-existing, but is there is a reason this is specifically a base::Token and not a base::UnguessableToken?
I'm not too sure why it was added as a `base:Token` specifically. But I know this feature has been following the example of RegionCapture, which also uses a `base::Token`, https://source.chromium.org/chromium/chromium/src/+/main:components/viz/common/surfaces/region_capture_bounds.h;l=18?q=region_capture_bounds&ss=chromium
bool operator==(const TrackedElementRect& other) const {
return id == other.id && bounds == other.bounds;
}
bool operator!=(const TrackedElementRect& other) const {
return !(*this == other);
}```
bool operator==(const TrackedElementRect&) = default;
```Should work.
Done
Is there a specific reason we need unique_ptr here? Do we need address stability for the sub rect? Otherwise, just make TrackedElementSubRect movable/copyable and store it by value without the indirection?
Done
for (auto it2 = vec.begin(); it2 != vec.end(); ++it2) {
if (it2 != vec.begin()) {
sb.Append(", ");
}
sb.Append(it2->ToString());
}```suggestion
for (const auto& rect : vec) {
if (&rect != &*vec.begin()) {
sb.Append(", ");
}
sb.Append(rect.ToString());
}
```
- Daniel Cheng
- Khushal Sagar
- Nan Lin
- 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. |
Ryan Kalla (Gerrit)
Ryan Kalla voted and added 1 comment![]( "Open in Gerrit")
Votes added by Ryan Kalla
| Commit-Queue | +1 |
1 comment
Hi avi! Can you take a look at:
//chrome/browser/*
//content/*
Related details
- Avi Drissman
- Daniel Cheng
- Khushal Sagar
- Nan Lin
- 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. |
Avi Drissman (Gerrit)
Avi Drissman added 2 comments![]( "Open in Gerrit")
The chrome/ code seems like a straightforward expansion into multiple rects, but the special-casing for the ai features (both in chrome/ and cc/) feels odd.
// The feature that is tracking the element.
enum TrackedElementFeature {
kAIHighlight,
kCrossOriginFrameTracking,
};It’s odd that cc, which is a low-level compositor component, would have to know about every high-level feature that uses tracked elements. Can we do it in a more layered fashion?
Related details
- Daniel Cheng
- Khushal Sagar
- Nan Lin
- Przemyslaw Szczepaniak
- Ryan Kalla
Ryan Kalla (Gerrit)
Ryan Kalla added 1 comment![]( "Open in Gerrit")
// The feature that is tracking the element.
enum TrackedElementFeature {
kAIHighlight,
kCrossOriginFrameTracking,
};It’s odd that cc, which is a low-level compositor component, would have to know about every high-level feature that uses tracked elements. Can we do it in a more layered fashion?
In my next CL I'll need to refactor this a bit more and move these TrackedElement defs to //components/viz (in order to add this data to the Viz CompositorFrameMetadata). This CL was already getting really large so I've opted to first make the changes here in cc, then I'll do a simpler move to viz in a follow up. Would that address your concern?
Related details
- Avi Drissman
- Daniel Cheng
- Khushal Sagar
- Nan Lin
- Przemyslaw Szczepaniak
Avi Drissman (Gerrit)
Avi Drissman added 1 comment![]( "Open in Gerrit")
// The feature that is tracking the element.
enum TrackedElementFeature {
kAIHighlight,
kCrossOriginFrameTracking,
};Ryan KallaIt’s odd that cc, which is a low-level compositor component, would have to know about every high-level feature that uses tracked elements. Can we do it in a more layered fashion?
In my next CL I'll need to refactor this a bit more and move these TrackedElement defs to //components/viz (in order to add this data to the Viz CompositorFrameMetadata). This CL was already getting really large so I've opted to first make the changes here in cc, then I'll do a simpler move to viz in a follow up. Would that address your concern?
It’s not clear that Viz is any higher-level than cc with respect to AI.
@dch...@chromium.org, this is in Mojo anyway, so this is also relevant to you.
Related details
- Daniel Cheng
- Khushal Sagar
- Nan Lin
- Przemyslaw Szczepaniak
- Ryan Kalla
Ryan Kalla (Gerrit)
Ryan Kalla added 1 comment![]( "Open in Gerrit")
// The feature that is tracking the element.
enum TrackedElementFeature {
kAIHighlight,
kCrossOriginFrameTracking,
};Ryan KallaIt’s odd that cc, which is a low-level compositor component, would have to know about every high-level feature that uses tracked elements. Can we do it in a more layered fashion?
Avi DrissmanIn my next CL I'll need to refactor this a bit more and move these TrackedElement defs to //components/viz (in order to add this data to the Viz CompositorFrameMetadata). This CL was already getting really large so I've opted to first make the changes here in cc, then I'll do a simpler move to viz in a follow up. Would that address your concern?
It’s not clear that Viz is any higher-level than cc with respect to AI.
@dch...@chromium.org, this is in Mojo anyway, so this is also relevant to you.
Can you explain what you mean by doing this in a more layered fashion? This is just being used as an enum so the consumers of this data have an easier time differentiating the tracked elements (and so that we can reuse the structs across use cases instead of needing to duplicate code each time we have a new type of element to track). The code in these lower level components is not doing anything special for any of the features, the enums are just keys to a map which are set by the blink code.
Related details
- Avi Drissman
- Daniel Cheng
- Khushal Sagar
- Nan Lin
- Przemyslaw Szczepaniak
Avi Drissman (Gerrit)
Avi Drissman added 1 comment![]( "Open in Gerrit")
// The feature that is tracking the element.
enum TrackedElementFeature {
kAIHighlight,
kCrossOriginFrameTracking,
};Ryan KallaIt’s odd that cc, which is a low-level compositor component, would have to know about every high-level feature that uses tracked elements. Can we do it in a more layered fashion?
Avi DrissmanIn my next CL I'll need to refactor this a bit more and move these TrackedElement defs to //components/viz (in order to add this data to the Viz CompositorFrameMetadata). This CL was already getting really large so I've opted to first make the changes here in cc, then I'll do a simpler move to viz in a follow up. Would that address your concern?
Ryan KallaIt’s not clear that Viz is any higher-level than cc with respect to AI.
@dch...@chromium.org, this is in Mojo anyway, so this is also relevant to you.
Can you explain what you mean by doing this in a more layered fashion? This is just being used as an enum so the consumers of this data have an easier time differentiating the tracked elements (and so that we can reuse the structs across use cases instead of needing to duplicate code each time we have a new type of element to track). The code in these lower level components is not doing anything special for any of the features, the enums are just keys to a map which are set by the blink code.
We have named enum values for concepts that are at a higher level. If we want to define these as some opaque token value that’s kept and transmitted without semantic meaning, and the higher layers have their own reference list of what they mean, something like that?
Related details
- Daniel Cheng
- Khushal Sagar
- Nan Lin
- Przemyslaw Szczepaniak
- Ryan Kalla
Ryan Kalla (Gerrit)
Ryan Kalla added 1 comment![]( "Open in Gerrit")
// The feature that is tracking the element.
enum TrackedElementFeature {
kAIHighlight,
kCrossOriginFrameTracking,
};Ryan KallaIt’s odd that cc, which is a low-level compositor component, would have to know about every high-level feature that uses tracked elements. Can we do it in a more layered fashion?
Avi DrissmanIn my next CL I'll need to refactor this a bit more and move these TrackedElement defs to //components/viz (in order to add this data to the Viz CompositorFrameMetadata). This CL was already getting really large so I've opted to first make the changes here in cc, then I'll do a simpler move to viz in a follow up. Would that address your concern?
Ryan KallaIt’s not clear that Viz is any higher-level than cc with respect to AI.
@dch...@chromium.org, this is in Mojo anyway, so this is also relevant to you.
Avi DrissmanCan you explain what you mean by doing this in a more layered fashion? This is just being used as an enum so the consumers of this data have an easier time differentiating the tracked elements (and so that we can reuse the structs across use cases instead of needing to duplicate code each time we have a new type of element to track). The code in these lower level components is not doing anything special for any of the features, the enums are just keys to a map which are set by the blink code.
We have named enum values for concepts that are at a higher level. If we want to define these as some opaque token value that’s kept and transmitted without semantic meaning, and the higher layers have their own reference list of what they mean, something like that?
I would prefer not to have an additional layer of complexity here if it's not needed. I don't see how this is very different from other human-readable enum values and class names in the cc code that are referencing concepts that exist at a higher level (for example, RegionCapture which the tracked element code is based on).
But maybe it would be preferable to have the enum define a "type" rather than a "feature" (i.e. "kHighlightRect", "kCrossOriginFrame") which doesn't say as much about what the higher levels are using the data for?
I'll also tag @p...@chromium.org for his thoughts on the cc layering.
Related details
- Avi Drissman
- Daniel Cheng
- Khushal Sagar
- Nan Lin
- Przemyslaw Szczepaniak
Philip Rogers (Gerrit)
Philip Rogers voted and added 1 comment![]( "Open in Gerrit")
Votes added by Philip Rogers
| Code-Review | +1 |
1 comment
// The feature that is tracking the element.
enum TrackedElementFeature {
kAIHighlight,
kCrossOriginFrameTracking,
};Ryan KallaIt’s odd that cc, which is a low-level compositor component, would have to know about every high-level feature that uses tracked elements. Can we do it in a more layered fashion?
Avi DrissmanIn my next CL I'll need to refactor this a bit more and move these TrackedElement defs to //components/viz (in order to add this data to the Viz CompositorFrameMetadata). This CL was already getting really large so I've opted to first make the changes here in cc, then I'll do a simpler move to viz in a follow up. Would that address your concern?
Ryan KallaIt’s not clear that Viz is any higher-level than cc with respect to AI.
@dch...@chromium.org, this is in Mojo anyway, so this is also relevant to you.
Avi DrissmanCan you explain what you mean by doing this in a more layered fashion? This is just being used as an enum so the consumers of this data have an easier time differentiating the tracked elements (and so that we can reuse the structs across use cases instead of needing to duplicate code each time we have a new type of element to track). The code in these lower level components is not doing anything special for any of the features, the enums are just keys to a map which are set by the blink code.
Ryan KallaWe have named enum values for concepts that are at a higher level. If we want to define these as some opaque token value that’s kept and transmitted without semantic meaning, and the higher layers have their own reference list of what they mean, something like that?
I would prefer not to have an additional layer of complexity here if it's not needed. I don't see how this is very different from other human-readable enum values and class names in the cc code that are referencing concepts that exist at a higher level (for example, RegionCapture which the tracked element code is based on).
But maybe it would be preferable to have the enum define a "type" rather than a "feature" (i.e. "kHighlightRect", "kCrossOriginFrame") which doesn't say as much about what the higher levels are using the data for?
I'll also tag @p...@chromium.org for his thoughts on the cc layering.
I agree with Avi's point about layering: blink/cc/viz should know as little as possible about these high-level feature details. This patch started off with much deeper integration, and we got it to just an enum. At a high level, the "input" blink API is `Element ::SetTrackedElementSubRect(cc::TrackedElementFeature, rect)`, and the "output" cc API is a map of cc::TrackedElementFeature -> rects on RenderFrameMetadata.
@a...@chromium.org or @dch...@chromium.org, is there precedent for a more opaque identifier?
Related details
- Avi Drissman
- Daniel Cheng
- Khushal Sagar
- Nan Lin
- Przemyslaw Szczepaniak
- Ryan Kalla