[Indigo] Ensure elements with tracked subrects draw content and paint [chromium/src : main]
William Liu (Gerrit)
William Liu added 2 comments![]( "Open in Gerrit")
StopObservingState(kVisualStateUpdated),William Liumight be a silly question: Why does `WaitForShow(IndigoToolbar::kToolbarElementId)` on its own not work? It sounds like the toolbar should eventually show (and this is a WaitFor as opposed to expecting the toolbar to be immediately shown)
Khushal SagarOkay I actually can't quite pinpoint why the OOPIF does not submit a frame if we don't force one.
The sequence we have is:
```
---- time --->
Main renderer -------- frame0 embeds the OOPIF's surface layer with empty rect
OOPIF -------- surface layer emits frame1 with our rect
```
so technically we shouldn't need to force anything and this is the case in production (as in the recording).@khusha...@chromium.org do you know?
William LiuSorry, missed this earlier. I'm missing detail on how the whole thing works but from your comment it sounds like `kToolbarElementId` showing up depends on the renderer sending a tracked element rect. And `WaitForShow` blocks until a frame with that rect has reached the browser. I agree that we shouldn't need to force a frame here, that might mask a production bug if there's no frame trigger.
The tracking rect is associated with the main Document's compositor frame itself (even with the current PS moving that to the OOPIF's SurfaceLayer). So I'm not sure how the OOPIF not submitting a frame could impact this.
I suspect the fix in the other comment to ensure background painting phase for replaced elements with tracking rects will avoid the need for this.
Obsolete.
// For out-of-process iframes (RemoteFrame), the frame owner element (e.g.William LiuInstead of adding the rects to the layer directly, better to follow the pattern [here](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/exported/web_plugin_container_impl.cc;l=198;drc=cedb6902cdefa33e500ef38eca6227a8f50c9502) which creates a paint chunk before recording the display item for the foreign layer.
_________
Actually digging into this more, we're just missing a condition similar to region capture [here](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/paint/replaced_painter.cc;l=142;drc=cedb6902cdefa33e500ef38eca6227a8f50c9502) to force background painting if there's any element rects to track. Can you try that instead? I suspect that will also avoid the need to switch from tracking the image element to its OOPIF (which is also unnecessary complexity).
TYSM! background paint works with a little addition 😊
Related details
- Adithya Srinivasan
- Khushal Sagar
Code-Coverage
Code-Owners
Code-Review
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")
(!drawable_bounds.IsEmpty() || tracked_element_rects);Can you figure out why this wasn't necessary for the `GetRegionCaptureCropId` case?
Related details
- Adithya Srinivasan
- William Liu
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. |
William Liu (Gerrit)
William Liu voted and added 1 comment![]( "Open in Gerrit")
Votes added by William Liu
| Commit-Queue | +1 |
1 comment
(!drawable_bounds.IsEmpty() || tracked_element_rects);Can you figure out why this wasn't necessary for the `GetRegionCaptureCropId` case?
It's in the CL description but basically the region captures are set on CFMetadata. So even if we don't generate a RenderFrame, a CF is always generated.
Related details
- Adithya Srinivasan
- Khushal Sagar
Code-Coverage
Code-Owners
Code-Review
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")
(!drawable_bounds.IsEmpty() || tracked_element_rects);William LiuCan you figure out why this wasn't necessary for the `GetRegionCaptureCropId` case?
It's in the CL description but basically the region captures are set on CFMetadata. So even if we don't generate a RenderFrame, a CF is always generated.
This seems like a bug we should fix in CC then. CC should be sending rects for layers even if drawing is skipping them consistently to CF and RenderFrameMetadata.
Is this happening because the layer iteration we're doing using EffectTreeLayerListIterator skips layers with no drawable content?
Related details
- Adithya Srinivasan
- William Liu
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
William Liu (Gerrit)
William Liu added 1 comment![]( "Open in Gerrit")
(!drawable_bounds.IsEmpty() || tracked_element_rects);William LiuCan you figure out why this wasn't necessary for the `GetRegionCaptureCropId` case?
Khushal SagarIt's in the CL description but basically the region captures are set on CFMetadata. So even if we don't generate a RenderFrame, a CF is always generated.
This seems like a bug we should fix in CC then. CC should be sending rects for layers even if drawing is skipping them consistently to CF and RenderFrameMetadata.
Is this happening because the layer iteration we're doing using EffectTreeLayerListIterator skips layers with no drawable content?
Is this happening because the layer iteration we're doing using EffectTreeLayerListIterator skips layers with no drawable content?
Yup. So we shall also keep the layers w/o drawable content but with rects?
Related details
- Adithya Srinivasan
- 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. |
William Liu (Gerrit)
William Liu voted and added 1 comment![]( "Open in Gerrit")
Votes added by William Liu
| Commit-Queue | +1 |
1 comment
(!drawable_bounds.IsEmpty() || tracked_element_rects);William LiuCan you figure out why this wasn't necessary for the `GetRegionCaptureCropId` case?
Khushal SagarIt's in the CL description but basically the region captures are set on CFMetadata. So even if we don't generate a RenderFrame, a CF is always generated.
William LiuThis seems like a bug we should fix in CC then. CC should be sending rects for layers even if drawing is skipping them consistently to CF and RenderFrameMetadata.
Is this happening because the layer iteration we're doing using EffectTreeLayerListIterator skips layers with no drawable content?
Is this happening because the layer iteration we're doing using EffectTreeLayerListIterator skips layers with no drawable content?
Yup. So we shall also keep the layers w/o drawable content but with rects?
okay I uploaded another patchset... let's hope CQ is happy about the change..
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. |
Khushal Sagar (Gerrit)
Khushal Sagar added 1 comment![]( "Open in Gerrit")
(!drawable_bounds.IsEmpty() || tracked_element_rects);William LiuCan you figure out why this wasn't necessary for the `GetRegionCaptureCropId` case?
Khushal SagarIt's in the CL description but basically the region captures are set on CFMetadata. So even if we don't generate a RenderFrame, a CF is always generated.
William LiuThis seems like a bug we should fix in CC then. CC should be sending rects for layers even if drawing is skipping them consistently to CF and RenderFrameMetadata.
Is this happening because the layer iteration we're doing using EffectTreeLayerListIterator skips layers with no drawable content?
William LiuIs this happening because the layer iteration we're doing using EffectTreeLayerListIterator skips layers with no drawable content?
Yup. So we shall also keep the layers w/o drawable content but with rects?
okay I uploaded another patchset... let's hope CQ is happy about the change..
So we used the walk in PrepareToDraw as an optimization to avoid more layer walks per frame just to check if there's any tracked rects. Didn't realize that it skips layers which don't have drawable content but the fix shouldn't be to force these layers to draw. That'll turn off a bunch of other optimizations.
Maybe we can add a walk over all layers in that function to cache state that needs to visit layers which don'r draw. And use it both for tracked elements and CollectRegionCaptureBounds. Because that one also unconditionally walks over all layers every frame. So at least we'll have one spot going forward to go through all the layers and do stuff later on for features which are rarely used.
Related details
- William Liu
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. |