media: Selectively attach video visibility tracker based on feature flag [chromium/src : main]
0 views
Skip to first unread message
Vikram Pasupathy (Gerrit)
Apr 1, 2026, 3:11:09 PM (15 hours ago) Apr 1
to Benjamin Keen, Chromium LUCI CQ, srirama chandra sekhar, AyeAye, feature-me...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, eric.c...@apple.com
Attention needed from Benjamin Keen
Vikram Pasupathy added 1 comment![]( "Open in Gerrit")
Patchset-level comments
Related details
Attention is currently required from:
- Benjamin Keen
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Vikram Pasupathy (Gerrit)
Apr 1, 2026, 4:30:24 PM (14 hours ago) Apr 1
to Dale Curtis, Sangbaek Park, Benjamin Keen, Chromium LUCI CQ, srirama chandra sekhar, AyeAye, feature-me...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, eric.c...@apple.com
Attention needed from Benjamin Keen, Dale Curtis and Sangbaek Park
Vikram Pasupathy added 1 comment![]( "Open in Gerrit")
Patchset-level comments
File-level comment, Patchset 5 (Latest):
Vikram Pasupathy . resolved
Tested manually that this does not affect visibility ratio reporting logic.
I see Ben is OOO today, so sending to Dale and Sangbaek for review, TY!
Related details
Attention is currently required from:
- Benjamin Keen
- Dale Curtis
- Sangbaek Park
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dale Curtis (Gerrit)
Apr 1, 2026, 6:32:08 PM (12 hours ago) Apr 1
to Vikram Pasupathy, Sangbaek Park, Benjamin Keen, Chromium LUCI CQ, srirama chandra sekhar, AyeAye, feature-me...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, eric.c...@apple.com
Attention needed from Benjamin Keen, Sangbaek Park and Vikram Pasupathy
Dale Curtis added 1 comment![]( "Open in Gerrit")
File third_party/blink/renderer/core/html/media/html_video_element.cc
Line 465, Patchset 5 (Latest):
UpdateVideoVisibilityTracker();Dale Curtis . unresolved
Can we make this part of `CreateVisibilityTrackerIfNeeded` then? A bit weird to have to call both.
Related details
Attention is currently required from:
- Benjamin Keen
- Sangbaek Park
- Vikram Pasupathy
Submit Requirements:
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. |
Vikram Pasupathy (Gerrit)
Apr 1, 2026, 8:41:14 PM (10 hours ago) Apr 1
to Dale Curtis, Sangbaek Park, Benjamin Keen, Chromium LUCI CQ, srirama chandra sekhar, AyeAye, feature-me...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, eric.c...@apple.com
Attention needed from Benjamin Keen, Dale Curtis and Sangbaek Park
Vikram Pasupathy added 1 comment![]( "Open in Gerrit")
File third_party/blink/renderer/core/html/media/html_video_element.cc
Can we make this part of `CreateVisibilityTrackerIfNeeded` then? A bit weird to have to call both.
Attention is currently required from:
- Benjamin Keen
- Dale Curtis
- Sangbaek Park
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Benjamin Keen (Gerrit)
Apr 1, 2026, 10:06:19 PM (8 hours ago) Apr 1
to Vikram Pasupathy, Dale Curtis, Sangbaek Park, Chromium LUCI CQ, srirama chandra sekhar, AyeAye, feature-me...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, eric.c...@apple.com
Attention needed from Dale Curtis, Sangbaek Park and Vikram Pasupathy
Benjamin Keen added 1 comment![]( "Open in Gerrit")
File third_party/blink/renderer/core/html/media/html_video_element.cc
Line 503, Patchset 6 (Latest):
CreateVisibilityTrackerIfNeeded();Benjamin Keen . unresolved
You would still need to update the video visibility tracker outside of this if statement (line 505):
```
if (visibility_tracker_) {
UpdateVideoVisibilityTracker();
}
```
If we don't do this, and the tracker already exists, we would miss the state update, because of the early return in `CreateVisibilityTrackerIfNeeded`.
Related details
Attention is currently required from:
- Dale Curtis
- Sangbaek Park
- Vikram Pasupathy
Submit Requirements:
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. |
Vikram Pasupathy (Gerrit)
Apr 1, 2026, 11:28:25 PM (7 hours ago) Apr 1
to Dale Curtis, Sangbaek Park, Benjamin Keen, Chromium LUCI CQ, srirama chandra sekhar, AyeAye, feature-me...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, eric.c...@apple.com
Attention needed from Benjamin Keen, Dale Curtis and Sangbaek Park
Vikram Pasupathy added 1 comment![]( "Open in Gerrit")
File third_party/blink/renderer/core/html/media/html_video_element.cc
Line 503, Patchset 6:
CreateVisibilityTrackerIfNeeded();Benjamin Keen . unresolved
You would still need to update the video visibility tracker outside of this if statement (line 505):
```
if (visibility_tracker_) {
UpdateVideoVisibilityTracker();
}
```If we don't do this, and the tracker already exists, we would miss the state update, because of the early return in `CreateVisibilityTrackerIfNeeded`.
Vikram Pasupathy
Ah thanks, should have seen that.
Since we didn't want this:
```
CreateVisibilityTrackerIfNeeded();
UpdateVideoVisibilityTracker();
```
I just moved the `UpdateVideoVisibilityTracker` to the early return in `UpdateVideoVisibilityTracker`.
Related details
Attention is currently required from:
- Benjamin Keen
- Dale Curtis
- Sangbaek Park
Submit Requirements:
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. |
Reply all
Reply to author
Forward
0 new messages