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
Xiaohan Wang (Gerrit)
Xiaohan Wang voted and added 2 comments![]( "Open in Gerrit")
Votes added by Xiaohan Wang
| Commit-Queue | +1 |
2 comments
Thanks for the great suggestion. PTLA again!
19..20: Rebase only
20..21: Changes addressing the comment
[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
Great suggestion. I worked withe Jetski and believe this is done. Please help check!
Related details
- 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. |
Tommy Steimel (Gerrit)
Tommy Steimel voted and added 3 comments![]( "Open in Gerrit")
Votes added by Tommy Steimel
| Code-Review | +1 |
3 comments
Looks like there's unnecessary newlines added here
[MinVersion=25] bool is_video_frame_available;Xiaohan WangInstead 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
Great suggestion. I worked withe Jetski and believe this is done. Please help check!
- 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. |
Xiaohan Wang (Gerrit)
Xiaohan Wang voted and added 1 comment![]( "Open in Gerrit")
Votes added by Xiaohan Wang
| Commit-Queue | +1 |
1 comment
Looks like there's unnecessary newlines added here
- Zijie He
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")
OWNERS Review Request:
dalecurtis: third_party/blink/renderer/core/html/media*
security reviewers: *.mojom
evliu: enums.xml
Related details
- Chrome Security IPC Reviews
- Dale Curtis
- Evan Liu
- Zijie He
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
gwsq (Gerrit)
Message from gwsq![]( "Open in Gerrit")
From googleclient/chrome/chromium_gwsq/ipc/config.gwsq:
IPC: w...@chromium.org
📎 It looks like you’re making a possibly security-sensitive change! 📎 IPC security review isn’t a rubberstamp, so your friendly security reviewer will need a fair amount of context to review your CL effectively. Please review your CL description and code comments to make sure they provide context for someone unfamiliar with your project/area. Pay special attention to where data comes from and which processes it flows between (and their privilege levels). Feel free to point your security reviewer at design docs, bugs, or other links if you can’t reasonably make a self-contained CL description. (Also see https://cbea.ms/git-commit/).
IPC reviewer(s): w...@chromium.org
Reviewer source(s):
w...@chromium.org is from context(googleclient/chrome/chromium_gwsq/ipc/config.gwsq)
Related details
- Dale Curtis
- Evan Liu
- Will Harris
- Zijie He
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Will Harris (Gerrit)
Will Harris voted and added 1 comment![]( "Open in Gerrit")
Votes added by Will Harris
| Code-Review | +1 |
1 comment
the mojom changes look fine from security perspective. lgtm
Related details
- Dale Curtis
- Evan Liu
- Xiaohan Wang
- Zijie He
Evan Liu (Gerrit)
Evan Liu voted Code-Review+1![]( "Open in Gerrit")
- Dale Curtis
- Xiaohan Wang
- Zijie He
Zijie He (Gerrit)
Zijie He voted and added 1 comment![]( "Open in Gerrit")
Votes added by Zijie He
| Code-Review | +1 |
1 comment
Sorry for the late response, fuchsia_web/ LGTM.
Related details
- Dale Curtis
- Xiaohan Wang
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)
Dale Curtis voted Code-Review+1![]( "Open in Gerrit")
- Xiaohan Wang
Xiaohan Wang (Gerrit)
Xiaohan Wang added 1 comment![]( "Open in Gerrit")
More OWNERS review request for some trivial changes:
ammedina: chrome/browser/readaloud/*
arthursonzogni: content/public/*
mjwilson: t/b/r/m/w/audio_context.h
Related details
- Andres Munoz Medina
- Arthur Sonzogni
- Michael Wilson
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Wilson (Gerrit)
Michael Wilson voted and added 1 comment![]( "Open in Gerrit")
Votes added by Michael Wilson
| Code-Review | +1 |
1 comment
- Andres Munoz Medina
- Arthur Sonzogni
- Xiaohan Wang
Arthur Sonzogni (Gerrit)
Arthur Sonzogni voted and added 4 comments![]( "Open in Gerrit")
Votes added by Arthur Sonzogni
| Code-Review | +1 |
4 comments
contnet/public/ lgtm;
- Fixed a latent bug in MediaWebContentsObserver where has_audio and
has_video parameters were inconsistent between the header and
implementation.Where is this in the patch?
// The given |player_id| has been requested to save the current video frame.
virtual void OnSaveVideoFrame(int player_id) = 0;**content/public/README.md**:
> - Interfaces that embedders implement, especially ones which are used in tests
or are observer-style and have many implementations, should have default
(empty) implementations.
Related details
- Andres Munoz Medina
- Xiaohan Wang
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement