Implement MediaVideoVisibilityTracker RecordVideoOcclusionState method [chromium/src : main]
Dale Curtis (Gerrit)
Dale Curtis added 8 comments![]( "Open in Gerrit")
DCHECK(video_->visibility_tracker_for_tests());Huh, TIL I didn't think it would catch for_tests, but it does:
https://source.chromium.org/chromium/chromium/src/+/main:PRESUBMIT.py;l=2383;drc=8ef56c705652f376913f0b051d1769b4ed876c2e
EXPECT_CALL((*MockMediaPlayer()), RecordVideoOcclusionState(_));Probably you want to verify the right occlusion state is sent?
EXPECT_CALL((*MockMediaPlayer()), RecordVideoOcclusionState(_));Ditto.
occluding_rects_stream << "[" << "x: " << rect.x() << ", "base::StringPrintf?
if (occluding_rects_stream.str().empty()) {.str() emits the string, so I think you want `.tellp() == 0`?
std::ostringstream has_sufficiently_visible_video_stream;Just std::string?
Related details
- Benjamin Keen
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Benjamin Keen (Gerrit)
Benjamin Keen added 7 comments![]( "Open in Gerrit")
EXPECT_CALL((*MockMediaPlayer()), RecordVideoOcclusionState(_));Probably you want to verify the right occlusion state is sent?
Hmmm, I was mainly interested in the function call happening at the right time. Since its a string it may be annoying to keep the test up to date anytime the string changes.
That said, I've added the verification, there is value in making sure the string matches what's expected.
EXPECT_CALL((*MockMediaPlayer()), RecordVideoOcclusionState(_));Benjamin KeenDitto.
Done
occluding_rects_stream << "[" << "x: " << rect.x() << ", "Benjamin Keenbase::StringPrintf?
Regretfully usage of `base::StringPrintf()` is banned, and `absl::StrFormat` is not yet allowed [1].
.str() emits the string, so I think you want `.tellp() == 0`?
I used `str().empty()` just to make it more readable. Updated to your suggestion, to avoid emitting the string.
std::ostringstream has_sufficiently_visible_video_stream;Benjamin KeenJust std::string?
Done
: base::StrCat(Benjamin Keenbase::StringPrintf?
See first comment on this file.
: base::StrCat(Benjamin KeenStringPrintf?
See first comment on this file.
Related details
- Dale Curtis
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dale Curtis (Gerrit)
Dale Curtis added 1 comment![]( "Open in Gerrit")
occluding_rects_stream << "[" << "x: " << rect.x() << ", "Benjamin Keenbase::StringPrintf?
Regretfully usage of `base::StringPrintf()` is banned, and `absl::StrFormat` is not yet allowed [1].
Where do you see that StringPrintf is banned? It may need a non-blink function exception:
https://source.chromium.org/search?q=base::StringPrintf%20file:third_party%2Fblink%20-f:debug&sq=&ss=chromium
Related details
- Benjamin Keen
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Benjamin Keen (Gerrit)
Benjamin Keen added 1 comment![]( "Open in Gerrit")
occluding_rects_stream << "[" << "x: " << rect.x() << ", "Benjamin Keenbase::StringPrintf?
Dale CurtisRegretfully usage of `base::StringPrintf()` is banned, and `absl::StrFormat` is not yet allowed [1].
Where do you see that StringPrintf is banned? It may need a non-blink function exception:
https://source.chromium.org/search?q=base::StringPrintf%20file:third_party%2Fblink%20-f:debug&sq=&ss=chromium
- Dale Curtis
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dale Curtis (Gerrit)
Dale Curtis voted and added 1 comment![]( "Open in Gerrit")
Votes added by Dale Curtis
| Code-Review | +1 |
1 comment
const auto& rect = gfx::SkIRectToRect(occluding_rects[i]);FWIW, Rect.ToString() exists. Not sure if you intentionally chose this format though.
Related details
- Benjamin Keen
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Benjamin Keen (Gerrit)
Benjamin Keen added 1 comment![]( "Open in Gerrit")
const auto& rect = gfx::SkIRectToRect(occluding_rects[i]);FWIW, Rect.ToString() exists. Not sure if you intentionally chose this format though.
Yes! However it wraps the string in quotes, so I opted for formatting it here instead.
Related details
- Jeremy Roman
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jeremy Roman (Gerrit)
Jeremy Roman added 1 comment![]( "Open in Gerrit")
'third_party/blink/renderer/core/html/media/media_video_visibility_tracker.cc',Is this just for base::StringPrintf? Why not use the WTF formatting utilities (e.g., String::Format, the + operator on WTF::String, String::Number), like is usual in Blink?
Even if we did need to pass this as a UTF-8 string view rather than using WebString across the Blink API, StringUTF8Adaptor should work for that.
Related details
- Benjamin Keen
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dale Curtis (Gerrit)
Dale Curtis added 1 comment![]( "Open in Gerrit")
'third_party/blink/renderer/core/html/media/media_video_visibility_tracker.cc',Is this just for base::StringPrintf? Why not use the WTF formatting utilities (e.g., String::Format, the + operator on WTF::String, String::Number), like is usual in Blink?
Even if we did need to pass this as a UTF-8 string view rather than using WebString across the Blink API, StringUTF8Adaptor should work for that.
Ah right, sorry for leading you astray Ben, I forgot about String::Format.
Related details
- Benjamin Keen
- Jeremy Roman
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Benjamin Keen (Gerrit)
Benjamin Keen added 1 comment![]( "Open in Gerrit")
'third_party/blink/renderer/core/html/media/media_video_visibility_tracker.cc',Dale CurtisIs this just for base::StringPrintf? Why not use the WTF formatting utilities (e.g., String::Format, the + operator on WTF::String, String::Number), like is usual in Blink?
Even if we did need to pass this as a UTF-8 string view rather than using WebString across the Blink API, StringUTF8Adaptor should work for that.
Ah right, sorry for leading you astray Ben, I forgot about String::Format.
Yes, this is just for `base::StringPrintf`. No worries Dale, and thanks both for the review, now I'll know for next time that `String::Format` is the way to go in Blink :)
Related details
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dale Curtis (Gerrit)
Dale Curtis voted Code-Review+1![]( "Open in Gerrit")
- Benjamin Keen
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Benjamin Keen (Gerrit)
Benjamin Keen voted Commit-Queue+2![]( "Open in Gerrit")
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Implement MediaVideoVisibilityTracker RecordVideoOcclusionState method
This CL implements the tracker RecordVideoOcclusionState method, which
constructs the debug string and adds the log entry usning the MediaLog.
Sample screenshots, when video is:
* Sufficiently visible: http://screen/4Rzv4WqfEAiStNz
* Not sufficiently visible: http://screen/B9AB3EXasjirv4N
- M third_party/blink/renderer/core/html/media/html_video_element.h
- M third_party/blink/renderer/core/html/media/html_video_element_test.cc
- M third_party/blink/renderer/core/html/media/media_video_visibility_tracker.cc
- M third_party/blink/renderer/core/html/media/media_video_visibility_tracker.h
Code-Review: +1 by Dale Curtis
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |