Plumb TrackedElement data from Blink to RenderFrameMetadata [chromium/src : main]
Przemyslaw Szczepaniak (Gerrit)
Przemyslaw Szczepaniak added 14 comments![]( "Open in Gerrit")
void SetTrackingHighlightBounds(TrackingHighlightBounds bounds);Przemyslaw SzczepaniakTrackedElements? Just to make it clear that this data structure tracks the position of multiple elements.
Done
// NOTE: we use a TrackingHighlightIdBoundsPair instead of a map below due to
// our need to serialize to blink objects, where blink::HashMap only allows a
// very limited set of objects to be used as a key. The pair includes a trackingPrzemyslaw SzczepaniakI'm not following this comment. Why can't we just have a map<mojo_base.mojom.Token, TrackingHighlightRectData>?
Will rephrase, it's mimicking existing traits in RegionCapture, which boils down to blink::HashTable not allowing base::Token as a key.
When I try, this is what I get:
```
In file included from gen/cc/mojom/tracking_highlight_bounds.mojom-blink.cc:12:
In file included from gen/cc/mojom/tracking_highlight_bounds.mojom-blink.h:28:
In file included from gen/ui/gfx/geometry/mojom/geometry.mojom-blink.h:29:
In file included from ../../mojo/public/cpp/bindings/lib/wtf_clone_equals_util.h:12:
In file included from ../../third_party/blink/renderer/platform/wtf/hash_map.h:33:
In file included from ../../third_party/blink/renderer/platform/wtf/hash_table.h:44:
../../third_party/blink/renderer/platform/wtf/hash_traits.h:444:29: error: attempt to use a deleted function
444 | return value == Traits::DeletedValue();
| ^
../../third_party/blink/renderer/platform/wtf/hash_traits.h:479:58: note: in instantiation of member function 'blink::internal::HashTraitsDeletedValueHelper<blink::HashTra
its<base::Token>>::IsDeletedValue' requested here
479 | return internal::HashTraitsDeletedValueHelper<Traits>::IsDeletedValue(value);
| ^
../../third_party/blink/renderer/platform/wtf/hash_traits.h:492:10: note: in instantiation of function template specialization 'blink::IsHashTraitsDeletedValue<blink::Hash
Traits<base::Token>, base::Token>' requested here
492 | IsHashTraitsDeletedValue<Traits, T>(value);
| ^
../../third_party/blink/renderer/platform/wtf/hash_table.h:756:12: note: in instantiation of function template specialization 'blink::IsHashTraitsEmptyOrDeletedValue<blink
::HashTraits<base::Token>, base::Token>' requested here
756 | return IsHashTraitsEmptyOrDeletedValue<KeyTraits>(
| ^
../../third_party/blink/renderer/platform/wtf/hash_table.h:1597:14: note: in instantiation of member function 'blink::HashTable<base::Token, blink::KeyValuePair<base::Toke
n, mojo::StructPtr<cc::mojom::blink::TrackingHighlightRectData>>, blink::KeyValuePairExtractor, blink::HashMapValueTraits<blink::HashTraits<base::Token>, blink::HashTraits
<mojo::StructPtr<cc::mojom::blink::TrackingHighlightRectData>>>, blink::HashTraits<base::Token>, blink::PartitionAllocator>::IsEmptyOrDeletedBucket' requested here
1597 | if (!IsEmptyOrDeletedBucket(table[i]))
| ^
../../third_party/blink/renderer/platform/wtf/hash_table.h:660:5: note: in instantiation of member function 'blink::HashTable<base::Token, blink::KeyValuePair<base::Token,
mojo::StructPtr<cc::mojom::blink::TrackingHighlightRectData>>, blink::KeyValuePairExtractor, blink::HashMapValueTraits<blink::HashTraits<base::Token>, blink::HashTraits<m
ojo::StructPtr<cc::mojom::blink::TrackingHighlightRectData>>>, blink::HashTraits<base::Token>, blink::PartitionAllocator>::DeleteAllBucketsAndDeallocate' requested here
660 | DeleteAllBucketsAndDeallocate(table_, table_size_);
| ^
../../third_party/blink/renderer/platform/wtf/hash_map.h:86:7: note: in instantiation of member function 'blink::HashTable<base::Token, blink::KeyValuePair<base::Token, mo
jo::StructPtr<cc::mojom::blink::TrackingHighlightRectData>>, blink::KeyValuePairExtractor, blink::HashMapValueTraits<blink::HashTraits<base::Token>, blink::HashTraits<mojo
::StructPtr<cc::mojom::blink::TrackingHighlightRectData>>>, blink::HashTraits<base::Token>, blink::PartitionAllocator>::~HashTable' requested here
86 | class HashMap {
| ^
../../third_party/blink/renderer/platform/wtf/hash_traits.h:151:12: note: 'DeletedValue' has been explicitly marked deleted here
151 | static T DeletedValue() = delete;
| ^
```
gfx.mojom.Rect bounds;
gfx.mojom.Rect? clip_rect;Przemyslaw SzczepaniakDo you need both these rects: the completely bounds of the element and the visible subset? I was expecting just the latter, visible area of the element, would be enough.
It might be useful to know if the tracker rectangle is obscured/clipped, we might provide different visual effect of it's partially and fully obscured, having ratio of clipped/clipped rectangles would be useful for that.
for (const auto* layer : base::Reversed(*active_tree())) {Przemyslaw Szczepaniaknit: The number of layers is generally small so it matters less but could we do this in the same walk for both region capture and the new tracking?
Sure, I will leave a to-do to merge them in follow-up cl.
// Manually walk the clip tree to calculate the combined clip in screenPrzemyslaw SzczepaniakWe compute each layer's visible rect (as a part of rendering) before sending a frame. So you should be able to use the cached value [here](https://source.chromium.org/chromium/chromium/src/+/main:cc/layers/draw_properties.h;l=52;drc=adf1c0b0a7acf756e039e4203f41018e4bae22d0).
thanks, that simply files a lot of things!
last_draw_render_frame_metadata_->tracking_highlight_bounds !=Przemyslaw Szczepaniakthis is enabling it for iOS. Is that intentional? I figured this is for desktop only.
it's removed in
std::optional<gfx::Rect> clip_rect;Przemyslaw SzczepaniakSame thing here, not following the use-case for both clip_rect and bounds instead of just the visible rect.
ack, provided rationale in previous comment. We need both for providing visuals where tracked element is occluded.
// This method may be called at most once. The ID must be non-null.Przemyslaw SzczepaniakHow is this reset when tracking is no longer needed?
ack, added a clear method for TrackingHighlightRect.
// The SubCaptureTarget ID needs to be propagated to the paint system.Przemyslaw Szczepaniaknit: Copied comments from region capture. :)
Done
// If SetRegionCaptureCropId(id) was previously called on `this`,
// returns the non-empty `id` which it previously provided.
// Otherwise, returns a nullptr.Przemyslaw Szczepaniaknit: remove.
Done
std::nullopt);Przemyslaw Szczepaniakwhen is this not null?
fixed, it's not optional in recent version
case TrackingHighlightRectType::kSubsection:
return *rect;Przemyslaw SzczepaniakThis doesn't seem right. The caller is passing the element's painted rect which is computed as a part of painting. But in the subsection case, we return `rect` which was set on the Element by the caller..?
IIUC we're using this when a subsection of the element needs to be highlighted. It's unclear what coordinate space the rect is in. We'll need the paint system to map it into the element's local coordinate space. Also not sure how we deal with element resizes or if it's clipped by an ancestor.
For this change, let's keep things simpler and track the full painted rect for the element. We can add the subset based tracking in a follow up CL.
To clarify, the use case for AI highlight will be primarily the subsection, we will receive a screen space rect, that we want to anchor to underlyng, salient Element. The use cases for subsection here is "arbitrary screen space rectangle that probably has non-zero union with element".
I'm ok to remove it from this cl, but will leave comments for follow-up work.
TrackingHighlightRectType type;Przemyslaw Szczepaniaknit: do you need this? it seems like `rect` being set implies we want to use that subset.
Done
// TrackingHighlightId is a strong alias for base::Token, used to uniquelyPrzemyslaw SzczepaniakHow about "TrackedElement" instead of "TrackingHighlight". From the renderer's perspective, it's tracking the position of DOM elements. The type of feature that needs this tracking can be in `TrackedElementFeature` but the rest of the code can stay agnostic to it?
See comment above about excluding `TrackingHighlightFeature` for now.
TrackedElement SGTM
Related details
- Khushal Sagar
- Philip Rogers
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Przemyslaw Szczepaniak (Gerrit)
Przemyslaw Szczepaniak added 3 comments![]( "Open in Gerrit")
last_draw_render_frame_metadata_->tracking_highlight_bounds !=Przemyslaw Szczepaniakthis is enabling it for iOS. Is that intentional? I figured this is for desktop only.
it's removed in
Done
std::nullopt);Przemyslaw Szczepaniakwhen is this not null?
fixed, it's not optional in recent version
Done
// TrackingHighlightId is a strong alias for base::Token, used to uniquelyPrzemyslaw SzczepaniakHow about "TrackedElement" instead of "TrackingHighlight". From the renderer's perspective, it's tracking the position of DOM elements. The type of feature that needs this tracking can be in `TrackedElementFeature` but the rest of the code can stay agnostic to it?
See comment above about excluding `TrackingHighlightFeature` for now.
TrackedElement SGTM
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. |
Khushal Sagar (Gerrit)
Khushal Sagar added 16 comments![]( "Open in Gerrit")
// Set or get the tracking highlight regionnit: Set or get the tracking region for an element.
cc.mojom.TrackedElementBounds tracked_element_bounds;nit: comment please.
// NOTE: we use a TrackedElementIdBoundsPair instead of a map below due towe use a TrackedElementRectData ...
gfx.mojom.Rect bounds;
gfx.mojom.Rect visible_bounds;Left a suggestion below for only having `visible_bounds` for now with a comment clarifying what it is.
// to a gfx::Rect representing the region of the viewport that should be cropped
// to for tracked elements.viewport where the element is rendered in a frame.
// The tracked_element ID and bounds are stored internally in Blink and are
// passed to and tracked by the compositor/viz. If a previously-generated
// tracked_element ID is selected via a client-side API, the compositor uses
// the tracked_element IDs associated with the compositor render passes to
// determine what section of the frame should be used.This comment seems to much implementation detail, can remove it. I also didn't follow what "determine what section of the frame should be used" means.
gfx.mojom.Rect bounds;
gfx.mojom.Rect? clip_rect;Przemyslaw SzczepaniakDo you need both these rects: the completely bounds of the element and the visible subset? I was expecting just the latter, visible area of the element, would be enough.
It might be useful to know if the tracker rectangle is obscured/clipped, we might provide different visual effect of it's partially and fully obscured, having ratio of clipped/clipped rectangles would be useful for that.
It might be useful to know if the tracker rectangle is obscured/clipped
I want to minimize complexity until we have the details of how it'll be used. Clipping in general can be hard to reason about:
1. It's possible an element's content is clipped by a wrapper element to achieve a visual effect. That clip would be by-design and from the user's perspective they are seeing the full content.
2. The element is clipped by an ancestor scroller. Showing a UI which indicates its clipped could be useful to tell the user there's more content to get to.
You likely want the clip from #2 but not #1. Since this change is already big, let's keep it focused on the geometry which is trivially available in the rendering stack which is `visible_rect` with this comment: "Provides the bounds of the element visible in the frame submitted by the compositor. The bounds are relative to the compositor's viewport. This means for an iframe rendered by a child compositor, the coordinates are relative to this iframe."
________
I like that you mentioned "obscured". :) I don't think we handle occlusion correctly in this patch. Definitely not for the CC side, unsure about the paint side. Occlusion is also tricky so let's defer it
for (const auto* layer : base::Reversed(*active_tree())) {Przemyslaw Szczepaniaknit: The number of layers is generally small so it matters less but could we do this in the same walk for both region capture and the new tracking?
Sure, I will leave a to-do to merge them in follow-up cl.
Can we flag guard this until the follow up? I want to avoid a redundant layer walk every frame.
gfx::RectF total_clip_in_screen_space(root_drawable_content_rect);
bool first_clip = true;
for (const auto* current_clip_node =
clip_tree.Node(layer->clip_tree_index());
current_clip_node && current_clip_node->id != kRootPropertyNodeId;
current_clip_node = clip_tree.parent(current_clip_node)) {
gfx::RectF clip_rect_in_target_space = current_clip_node->clip;
const auto* current_transform_node =
transform_tree.Node(current_clip_node->transform_id);
const gfx::Transform& transform_to_screen =
transform_tree.ToScreen(current_transform_node->id);
gfx::RectF clip_in_screen_space = MathUtil::MapClippedRect(
transform_to_screen, clip_rect_in_target_space);
if (first_clip) {
total_clip_in_screen_space = clip_in_screen_space;
first_clip = false;
} else {
total_clip_in_screen_space.Intersect(clip_in_screen_space);
}
}
gfx::Rect clip_in_screen_space_rect =
gfx::ToEnclosingRect(total_clip_in_screen_space);
std::optional<gfx::Rect> optional_clip_rect;
// Only set the optional_clip_rect if it\'s not effectively the full
// viewport.
if (clip_in_screen_space_rect != root_drawable_content_rect) {
optional_clip_rect = clip_in_screen_space_rect;
}why do we still need this clip tree walk?
visible_element_bounds_in_screen_space.Intersect(
root_drawable_content_rect);I'm not sure if this is necessary, `visible_layer_rect` should already consider what part of it is visible in the root surface. So you can use `visible_element_bounds_in_screen_space` directly.
// This method may be called at most once. The ID must be non-null.Przemyslaw SzczepaniakHow is this reset when tracking is no longer needed?
ack, added a clear method for TrackingHighlightRect.
We'll need that method in the Element API too.
}Clear the element tracking and confirm we're no longer adding it to layers after that.
if (element && element->GetRegionCaptureCropId()) {
paint_info.context.GetPaintController().RecordRegionCaptureData(
*background_client, *(element->GetRegionCaptureCropId()),
ToPixelSnappedRect(paint_rect));
}
if (element && element->GetTrackingHighlightRect()) {
paint_info.context.GetPaintController().RecordTrackingHighlightData(
*background_client, element->GetTrackingHighlightRect()->id,
element->GetTrackingHighlightRect()->GetTrackedRect(
ToPixelSnappedRect(paint_rect)));
}Khushal Sagarnit: Move this to a helper that we can use below instead of duplicating? Also easier to make sure that every call-site which is plumbing region capture also plumbs this.
ping on this
LOG(ERROR) << "PZS RecordTrackedElementData"
<< highlight_id.value().ToString();nit: Remember to remove before landing this. :)
gfx::Rect GetTrackedRect(const gfx::Rect full_rect) const {
// TODO: Implement subsection tracking
return full_rect;
}nit: Let's remove this API for now and make it explicit at the call-site that the element's visual rect is being used. We can figure out the exact API as a part of subsection support.
// Defines the type of element that is tracked.
enum class TrackedElementFeature {
kAIHighlight,
};Let's remove this for now. We can add this as a part of the next feature that uses this infra.
Related details
- Philip Rogers
- Przemyslaw Szczepaniak
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Przemyslaw Szczepaniak (Gerrit)
Przemyslaw Szczepaniak added 15 comments![]( "Open in Gerrit")
// Set or get the tracking highlight regionnit: Set or get the tracking region for an element.
Done
cc.mojom.TrackedElementBounds tracked_element_bounds;Przemyslaw Szczepaniaknit: comment please.
Done
// NOTE: we use a TrackedElementIdBoundsPair instead of a map below due towe use a TrackedElementRectData ...
Done
gfx.mojom.Rect bounds;
gfx.mojom.Rect visible_bounds;Left a suggestion below for only having `visible_bounds` for now with a comment clarifying what it is.
Acknowledged
// to a gfx::Rect representing the region of the viewport that should be cropped
// to for tracked elements.viewport where the element is rendered in a frame.
Done
// The tracked_element ID and bounds are stored internally in Blink and are
// passed to and tracked by the compositor/viz. If a previously-generated
// tracked_element ID is selected via a client-side API, the compositor uses
// the tracked_element IDs associated with the compositor render passes to
// determine what section of the frame should be used.This comment seems to much implementation detail, can remove it. I also didn't follow what "determine what section of the frame should be used" means.
Acknowledged
gfx.mojom.Rect bounds;
gfx.mojom.Rect? clip_rect;Przemyslaw SzczepaniakDo you need both these rects: the completely bounds of the element and the visible subset? I was expecting just the latter, visible area of the element, would be enough.
Khushal SagarIt might be useful to know if the tracker rectangle is obscured/clipped, we might provide different visual effect of it's partially and fully obscured, having ratio of clipped/clipped rectangles would be useful for that.
It might be useful to know if the tracker rectangle is obscured/clipped
I want to minimize complexity until we have the details of how it'll be used. Clipping in general can be hard to reason about:
1. It's possible an element's content is clipped by a wrapper element to achieve a visual effect. That clip would be by-design and from the user's perspective they are seeing the full content.
2. The element is clipped by an ancestor scroller. Showing a UI which indicates its clipped could be useful to tell the user there's more content to get to.
You likely want the clip from #2 but not #1. Since this change is already big, let's keep it focused on the geometry which is trivially available in the rendering stack which is `visible_rect` with this comment: "Provides the bounds of the element visible in the frame submitted by the compositor. The bounds are relative to the compositor's viewport. This means for an iframe rendered by a child compositor, the coordinates are relative to this iframe."
________
I like that you mentioned "obscured". :) I don't think we handle occlusion correctly in this patch. Definitely not for the CC side, unsure about the paint side. Occlusion is also tricky so let's defer it
Acknowledged
for (const auto* layer : base::Reversed(*active_tree())) {Przemyslaw Szczepaniaknit: The number of layers is generally small so it matters less but could we do this in the same walk for both region capture and the new tracking?
Khushal SagarSure, I will leave a to-do to merge them in follow-up cl.
Can we flag guard this until the follow up? I want to avoid a redundant layer walk every frame.
I just realized that capture bounds are captured for CompositorFrameMetadata, while TrackedRegionBounds go for RenderFrameMetadata, combining them in one layer walk would require pulling them out... to ::GenerateCompositorFrame? and then passing as argument to MakeRenderFrameMetadata and MakeCompositorFrameMetadata. Would that make sense?
why do we still need this clip tree walk?
my mistake, it should be removed.
visible_element_bounds_in_screen_space.Intersect(
root_drawable_content_rect);I'm not sure if this is necessary, `visible_layer_rect` should already consider what part of it is visible in the root surface. So you can use `visible_element_bounds_in_screen_space` directly.
Acknowledged
// This method may be called at most once. The ID must be non-null.Przemyslaw SzczepaniakHow is this reset when tracking is no longer needed?
Khushal Sagarack, added a clear method for TrackingHighlightRect.
We'll need that method in the Element API too.
Done
Clear the element tracking and confirm we're no longer adding it to layers after that.
Done
LOG(ERROR) << "PZS RecordTrackedElementData"
<< highlight_id.value().ToString();nit: Remember to remove before landing this. :)
Done
gfx::Rect GetTrackedRect(const gfx::Rect full_rect) const {
// TODO: Implement subsection tracking
return full_rect;
}nit: Let's remove this API for now and make it explicit at the call-site that the element's visual rect is being used. We can figure out the exact API as a part of subsection support.
Done
// Defines the type of element that is tracked.
enum class TrackedElementFeature {
kAIHighlight,
};Let's remove this for now. We can add this as a part of the next feature that uses this infra.
- Khushal Sagar
- Philip Rogers
Przemyslaw Szczepaniak (Gerrit)
Przemyslaw Szczepaniak added 4 comments![]( "Open in Gerrit")
// Manually walk the clip tree to calculate the combined clip in screenPrzemyslaw SzczepaniakWe compute each layer's visible rect (as a part of rendering) before sending a frame. So you should be able to use the cached value [here](https://source.chromium.org/chromium/chromium/src/+/main:cc/layers/draw_properties.h;l=52;drc=adf1c0b0a7acf756e039e4203f41018e4bae22d0).
thanks, that simply files a lot of things!
Done
if (element && element->GetRegionCaptureCropId()) {
paint_info.context.GetPaintController().RecordRegionCaptureData(
*background_client, *(element->GetRegionCaptureCropId()),
ToPixelSnappedRect(paint_rect));
}
if (element && element->GetTrackingHighlightRect()) {
paint_info.context.GetPaintController().RecordTrackingHighlightData(
*background_client, element->GetTrackingHighlightRect()->id,
element->GetTrackingHighlightRect()->GetTrackedRect(
ToPixelSnappedRect(paint_rect)));
}nit: Move this to a helper that we can use below instead of duplicating? Also easier to make sure that every call-site which is plumbing region capture also plumbs this.
ping on this
Done
base::flat_map<TrackingHighlightId, gfx::Rect> map;Przemyslaw SzczepaniakBlink Style Guide: Prefer blink:: types over STL and base types. Please use WTF::HashMap instead of base::flat_map.
To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **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)_
Przemyslaw SzczepaniakOK But Won't Fix: HashMap is not happy with strong alias type TrackingHighlightId, attempts to use a deleted function.
Done
using TrackingHighlightId =Przemyslaw SzczepaniakI don't think you need anything below in this file for this change, just the TrackingHighlightId is enough. It can probably go into tracking_highlight_data.h itself for now. We'll see what needs to be in blink/public when adding the hook for the browser to add/remove this tracking.
Done
Khushal Sagar (Gerrit)
Khushal Sagar added 8 comments![]( "Open in Gerrit")
Can you add a [LayerTreeHostTest](https://source.chromium.org/chromium/chromium/src/+/main:cc/trees/layer_tree_host_unittest.cc;l=164;drc=7666bc1983c2a5b98e5dc6fa6c28f8f53c07d06f). It'll test the e2e flow through CC, setting the tracked rect on a layer and making sure it's on the RenderFrameData.
A few comments below otherwise LG.
// Set or get the tracking highlight regionnit: Set or get data for tracked elements on this layer. The geometry provided is in layer space.
for (const auto* layer : base::Reversed(*active_tree())) {Przemyslaw Szczepaniaknit: The number of layers is generally small so it matters less but could we do this in the same walk for both region capture and the new tracking?
Khushal SagarSure, I will leave a to-do to merge them in follow-up cl.
Przemyslaw SzczepaniakCan we flag guard this until the follow up? I want to avoid a redundant layer walk every frame.
I just realized that capture bounds are captured for CompositorFrameMetadata, while TrackedRegionBounds go for RenderFrameMetadata, combining them in one layer walk would require pulling them out... to ::GenerateCompositorFrame? and then passing as argument to MakeRenderFrameMetadata and MakeCompositorFrameMetadata. Would that make sense?
Hmmm, another alternative can be to cache a bit indicating if there are any layers with `tracked_element_bounds` on FrameData during the walk [here](https://source.chromium.org/chromium/chromium/src/+/main:cc/trees/layer_tree_host_impl.cc;l=1794;drc=7666bc1983c2a5b98e5dc6fa6c28f8f53c07d06f) and call `CollectTrackedElementBounds` based on that. Seems simpler?
I don't mind the extra walks if there are elements to track, since that will be pretty rare. Just want to avoid adding it when the feature is not being used.
// Set the element data with both the bounds and the optional clip.nit: stale comment.
TrackedElementBounds tracked_element_bounds;nit: comment please.
if (element && element->GetTrackedElementRect()) {nit: I was actually suggesting using the helper for both region capture and tracked element since the common pattern is to update both at the call-sites.
const TrackedElementId& highlight_id,nit: tracked_element_id?
LOG(ERROR) << "PZS RecordTrackedElementData"
<< highlight_id.value().ToString();debug logs
Related details
- Philip Rogers
- Przemyslaw Szczepaniak
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Przemyslaw Szczepaniak (Gerrit)
Przemyslaw Szczepaniak added 7 comments![]( "Open in Gerrit")
nit: Set or get data for tracked elements on this layer. The geometry provided is in layer space.
Done
for (const auto* layer : base::Reversed(*active_tree())) {Przemyslaw Szczepaniaknit: The number of layers is generally small so it matters less but could we do this in the same walk for both region capture and the new tracking?
Khushal SagarSure, I will leave a to-do to merge them in follow-up cl.
Przemyslaw SzczepaniakCan we flag guard this until the follow up? I want to avoid a redundant layer walk every frame.
Khushal SagarI just realized that capture bounds are captured for CompositorFrameMetadata, while TrackedRegionBounds go for RenderFrameMetadata, combining them in one layer walk would require pulling them out... to ::GenerateCompositorFrame? and then passing as argument to MakeRenderFrameMetadata and MakeCompositorFrameMetadata. Would that make sense?
Hmmm, another alternative can be to cache a bit indicating if there are any layers with `tracked_element_bounds` on FrameData during the walk [here](https://source.chromium.org/chromium/chromium/src/+/main:cc/trees/layer_tree_host_impl.cc;l=1794;drc=7666bc1983c2a5b98e5dc6fa6c28f8f53c07d06f) and call `CollectTrackedElementBounds` based on that. Seems simpler?
I don't mind the extra walks if there are elements to track, since that will be pretty rare. Just want to avoid adding it when the feature is not being used.
Done
// Set the element data with both the bounds and the optional clip.Przemyslaw Szczepaniaknit: stale comment.
Done
TrackedElementBounds tracked_element_bounds;Przemyslaw Szczepaniaknit: comment please.
Done
nit: I was actually suggesting using the helper for both region capture and tracked element since the common pattern is to update both at the call-sites.
Done
const TrackedElementId& highlight_id,Przemyslaw Szczepaniaknit: tracked_element_id?
Done
LOG(ERROR) << "PZS RecordTrackedElementData"
<< highlight_id.value().ToString();Przemyslaw Szczepaniakdebug logs
Done
Related details
- Khushal Sagar
- Philip Rogers
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Przemyslaw Szczepaniak (Gerrit)
Przemyslaw Szczepaniak added 3 comments![]( "Open in Gerrit")
Can you add a [LayerTreeHostTest](https://source.chromium.org/chromium/chromium/src/+/main:cc/trees/layer_tree_host_unittest.cc;l=164;drc=7666bc1983c2a5b98e5dc6fa6c28f8f53c07d06f). It'll test the e2e flow through CC, setting the tracked rect on a layer and making sure it's on the RenderFrameData.
A few comments below otherwise LG.
Done
std::optional<gfx::Rect> clip_rect;Same thing here, not following the use-case for both clip_rect and bounds instead of just the visible rect.
ack, provided rationale in previous comment. We need both for providing visuals where tracked element is occluded.
Done
case TrackingHighlightRectType::kSubsection:
return *rect;This doesn't seem right. The caller is passing the element's painted rect which is computed as a part of painting. But in the subsection case, we return `rect` which was set on the Element by the caller..?
IIUC we're using this when a subsection of the element needs to be highlighted. It's unclear what coordinate space the rect is in. We'll need the paint system to map it into the element's local coordinate space. Also not sure how we deal with element resizes or if it's clipped by an ancestor.
For this change, let's keep things simpler and track the full painted rect for the element. We can add the subset based tracking in a follow up CL.
To clarify, the use case for AI highlight will be primarily the subsection, we will receive a screen space rect, that we want to anchor to underlyng, salient Element. The use cases for subsection here is "arbitrary screen space rectangle that probably has non-zero union with element".
I'm ok to remove it from this cl, but will leave comments for follow-up work.
Done
Philip Rogers (Gerrit)
Philip Rogers added 2 comments![]( "Open in Gerrit")
TEST_P(ScrollingTest, NonCompositedMainThreadRepaintWithTrackedElement) {I think we need some tests in third_party/blink/renderer/core/paint/box_painter_test.cc. I think you don't really care about scrolling (is that true?) and were just copying existing tests, so you can just move these tests to box_painter_test.cc, or add new ones. Alternatively, compositing_test.cc may be another option for end-to-end style tests.
Can you add a test that <span id="highlight">aaaa</span> can be tracked?
: public GarbageCollected<TrackedElementData> {Does this need to be garbage collected? It doesn't have any GC types.
Related details
- Khushal Sagar
- Przemyslaw Szczepaniak
Khushal Sagar (Gerrit)
Khushal Sagar voted and added 7 comments![]( "Open in Gerrit")
Votes added by Khushal Sagar
| Code-Review | +1 |
7 comments
One comment about a behaviour change which I'm assuming is unintentional. LGTM % pdr otherwise.
// TODO: Merge this with CollectRegionCaptureBoundsnot necessary anymore.
// Check DelegatedInkMetadata on the CompositorFrameMetadata when the
// compositor frame is submitted.unrelated comment?
void ExpectBounds(const TrackedElementBounds& actual_bounds) {nit: ExpectBoundsOnThread.
These tests get callbacks on multiple threads so just to make it explicit that this one runs on the compositor thread, and shouldn't access state on the main thread.
scoped_refptr<Layer> child_a_;
scoped_refptr<Layer> child_b_;nit: we don't need to cache these as member variables.
std::optional<TrackedElementBounds> expected_bounds_;this is unused.
bool paints_tracking_highlight_and_region_capture_data =
element &&This is changing behaviour. It should be this instead:
```
bool paints_tracking_or_region_capture_data =
element && (element->GetRegionCaptureCropId() || element->GetTrackedElementRect()) &&
```
Related details
- Przemyslaw Szczepaniak
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |