media: Add Save Video Frame feature in Global Media Controls [chromium/src : main]
Xiaohan Wang (Gerrit)
Xiaohan Wang added 1 comment![]( "Open in Gerrit")
bool last_reported_video_frame_availability_ : 1 = false;Xiaohan Wangnit: Blink Style Guide: Naming - Precede boolean values with words like “is” and “did”. Consider renaming `last_reported_video_frame_availability_` to `is_last_reported_video_frame_available_` or `has_reported_video_frame_availability_`.
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)_
Invalid: "video_frame_availability" is a boolean, and this is just to record the last state.
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Xiaohan Wang (Gerrit)
Xiaohan Wang added 2 comments![]( "Open in Gerrit")
Sorry for the large CL size. I already create a prep refactor CL trying to reduce the diff, but I don't think I can make it smaller:
- Tests should be associated with the CL
- Icon is strongly recommended to be checked in in the same CL
- Plumbing code touches a lot of files.
steimel: PTAL at everything, especially the GMC stack
liberato: General media/ OWNER
Related details
- Frank Liberato
- Tommy Steimel
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Xiaohan Wang (Gerrit)
Xiaohan Wang added 1 comment![]( "Open in Gerrit")
base::FEATURE_DISABLED_BY_DEFAULT);Will enable this after the IPH change (separate CL) and more testing.
Tommy Steimel (Gerrit)
Tommy Steimel added 7 comments![]( "Open in Gerrit")
if (ShouldRouteAction(
media_session::mojom::MediaSessionAction::kSaveVideoFrame)) {
DidReceiveAction(media_session::mojom::MediaSessionAction::kSaveVideoFrame,
nullptr);
return;
}Since this action isn't exposed in the JS, this will never be true, so you can delete this
return fuchsia_media_sessions2::PlayerState::kIdle;Why are you adding this?
[MinVersion=26] bool is_video_frame_available;This should be MinVersion=25 and the next MinVersion should be 26
bool received_video_frame_availability_ = false;Instead of using this, you could make `last_video_frame_availability_` a std::optional<bool>, and then you could tell when you have a value. Though if you want to match the rest of these, then you'd name it `received_video_frame_availability_`
if (!video_has_played_) {Why are you changing this?
case MediaSessionAction::kSaveVideoFrame:Is there a reason you're reordering these? It doesn't really matter I guess but it would make the blame harder to interpret
Related details
- Frank Liberato
- Xiaohan Wang
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. |
Zijie He (Gerrit)
Zijie He added 1 comment![]( "Open in Gerrit")
return fuchsia_media_sessions2::PlayerState::kIdle;Why are you adding this?
🙏
Xiaohan Wang (Gerrit)
Xiaohan Wang added 7 comments![]( "Open in Gerrit")
Thanks for catching these issues, and I apologize that I didn't check the change thoroughly to catch them. PTAL again!
if (ShouldRouteAction(
media_session::mojom::MediaSessionAction::kSaveVideoFrame)) {
DidReceiveAction(media_session::mojom::MediaSessionAction::kSaveVideoFrame,
nullptr);
return;
}Since this action isn't exposed in the JS, this will never be true, so you can delete this
Done
Zijie HeWhy are you adding this?
🙏
Done
This should be MinVersion=25 and the next MinVersion should be 26
Done
Instead of using this, you could make `last_video_frame_availability_` a std::optional<bool>, and then you could tell when you have a value. Though if you want to match the rest of these, then you'd name it `received_video_frame_availability_`
Done
Why are you changing this?
Done
Is there a reason you're reordering these? It doesn't really matter I guess but it would make the blame harder to interpret
Was trying to match the MediaSessionAction declaration order, but you have a good point. Done.
Related details
- Frank Liberato
- Tommy Steimel
- Zijie He
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. |
Frank Liberato (Gerrit)
Frank Liberato added 1 comment![]( "Open in Gerrit")
moving myself to cc since i think Tommy is reviewing ~everything i own except media_switches (which lgtm).
-fl
Related details
- Tommy Steimel
- Xiaohan Wang
- Zijie He
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. |
Tommy Steimel (Gerrit)
Tommy Steimel added 2 comments![]( "Open in Gerrit")
[MinVersion=25] bool is_video_frame_available;Instead of adding this bool to the session info, should we instead just add the action to the list of actions? So the in `MediaSessionImpl::OnVideoFrameAvailabilityChanged()` we'd call `MediaSessionImpl::RebuildAndNotifyActionsChanged()` and in there we'd decide whether or not to add the action. Then in `MediaItemUIUpdatedView::UpdateMediaActionButtonsVisibility()` we can depend on the usual logic (though with a temporary check for the feature flag). As it stands we've added this new action but it acts differently
Tommy SteimelWhy are you removing this line?
Marked as resolved.
Related details
- Xiaohan Wang
- Zijie He
Xiaohan Wang (Gerrit)
Xiaohan Wang added 1 comment![]( "Open in Gerrit")
Sorry for being silent. I did plan to work on this but got distracted. I'll resume working on this soon.
Related details
- Zijie He