[ImmersivePlayback] Simplify immersive Picture-in-Picture confirmation flow [chromium/src : main]
Oleh Desiatyrikov (xWF) (Gerrit)
Oleh Desiatyrikov (xWF) added 2 comments![]( "Open in Gerrit")
blink::mojom::ImmersiveOptionsPtr confirmed_immersive_options_;Oleh Desiatyrikov (xWF)this gets back to some earlier design decisions -- if i remember, `Request..Confirmation()` was here out of convenience, but this now ties it to the session. in my mind, the confirmation request is still supposed to be meaningfully separate from the rest of the pip service.
could the confirmation be moved into StartSession, with the pip request failing if the user doesn't confirm it? this might simplify everything -- if immersive is enabled and the renderer thinks it's worth trying => start pip with a request for immersive and, if it fails, do nothing. there is some additional complication on the browser side since it's a big async call in the middle of StartSession, but i can't picture if it'd end up being particularly hard to deal with.
not sure if it'd be simpler, but it'd be a single request from the renderer side and the the whole 'validate options' thing would stay on the browser side. the current path of (renderer -is it okay?-> browser -sure!-> renderer -here the thing you just told me was okay-> browser) feels a little off somehow.
WDYT?
Oleh Desiatyrikov (xWF)could the confirmation be moved into StartSession
It looks doable. The `StartSession` will only have one extra parameter, something like `request_immersive`.
```
[ Renderer Process ] [ Browser Process ]
HTMLVideoElement PictureInPictureServiceImpl
│ │
│ 1. Video becomes "effectively fullscreen" │
│ ────────────────────────────────────────> │
│ │
│ 2. StartSession(request_immersive=true) via Mojo IPC │
│ ───────────────────────────────────────────────────────────────> │
│ │
│ [ Check fullscreen status ]
│ [ Allocate PendingSession ]
│ │
│ 3. Request confirmation │
│ (Triggers Java UI) │
│ ─────────────────────> │
│ │
│ [ User selects options in ]
│ [ modal dialog (spinners) ]
│ │
│ 4. Dialog callback returns │
│ user-selected options │
│ <───────────────────── │
│ │
│ [ Create OverlayWindow & ]
│ [ Start immersive session ]
│ │
│ 5. Run held StartSessionCallback with session remote │
│ <─────────────────────────────────────────────────────────────── │
│ │
```
Does it match with what you were thinking of?Otherwise, we could turn the flow inside-out and try something similar to what Gurmeet suggested previously:
- track effectively fullscreen state on the java side and initiate the confirmation directly from Java;
- then send a request back to renderer to start immersive on that element;
- before creating an immersive activity, compare options to match what user has agreed to.
This will be more refactor though.
Done
confirmed_immersive_options_ = result->options.Clone();Oleh Desiatyrikov (xWF)is there any risk that one renderer could request immersive and another renderer could walk in and steal it?
i guess not, since the service is scoped to the render frame. however, for top-level frames especially, i don't remember if we recreate the service on top-level navigation -- you might want to verify that behavior. otherwise, `confirmed_immersive_options_` might be stale if the top-level frame is re-used.
Acknowledged
Related details
- Frank Liberato
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Oleh Desiatyrikov (xWF) (Gerrit)
Oleh Desiatyrikov (xWF) added 1 comment![]( "Open in Gerrit")
The `picture_in_picture.mojom` can be cleaned up further as we don't need to pass those data structures over mojom anymore. I'll address it separately in crbug.com/500441418
Related details
- Frank Liberato
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. |
Gurmeet Kalra (Gerrit)
Gurmeet Kalra added 4 comments![]( "Open in Gerrit")
#include <optional>It looks like `<optional>` is not used in this header and can be removed.
if (pending_session_) {Please fix this WARNING reported by autoreview issue finding: To avoid potential use-after-free or re-entrancy issues where `pending_session_` is accessed during the execution of the callback, it is standard practice to move it to a local variable first:
```cpp
if (auto pending = std::move(pending_session_)) {
std::move(pending->callback).Run(mojo::NullRemote(), gfx::Size());
}
```
StartSession(Please fix this WARNING reported by chrome-ipc-security: Per the Chrome IPC Engineering Guide (**API-T14-01**: Documenting Cumulative State in IPC Filtering), IPC APIs must explicitly define state transition semantics for consecutive method invocations that modify state.\n\nThe CL description notes that "subsequent sessions immediately preempt and cancel any pending confirmation," but this behavioral semantic is not documented in the interface definition. Please add a comment to `StartSession` explicitly clarifying that consecutive invocations will preempt and overwrite any existing pending confirmation state.
=> (pending_remote<PictureInPictureSession>? session,Please fix this WARNING reported by chrome-ipc-security: Per the Chrome IPC Engineering Guide (**API-T1-02**: Null State Semantics in Mojo Callbacks), nullable parameter signals in state-tracking IPC callbacks must explicitly document what the `null` state represents.\n\nWith the immersive confirmation flow now merged into `StartSession`, if the user closes or denies the confirmation prompt, this will presumably result in a `null` session being returned. Please explicitly document this semantic meaning of the `null` `session` state to prevent state machine desynchronization on the consumer side.
Related details
- Frank Liberato
- Oleh Desiatyrikov (xWF)
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. |
Oleh Desiatyrikov (xWF) (Gerrit)
Oleh Desiatyrikov (xWF) added 4 comments![]( "Open in Gerrit")
It looks like `<optional>` is not used in this header and can be removed.
Done
Please fix this WARNING reported by autoreview issue finding: To avoid potential use-after-free or re-entrancy issues where `pending_session_` is accessed during the execution of the callback, it is standard practice to move it to a local variable first:
```cpp
if (auto pending = std::move(pending_session_)) {
std::move(pending->callback).Run(mojo::NullRemote(), gfx::Size());
}
```
Done
Please fix this WARNING reported by chrome-ipc-security: Per the Chrome IPC Engineering Guide (**API-T14-01**: Documenting Cumulative State in IPC Filtering), IPC APIs must explicitly define state transition semantics for consecutive method invocations that modify state.\n\nThe CL description notes that "subsequent sessions immediately preempt and cancel any pending confirmation," but this behavioral semantic is not documented in the interface definition. Please add a comment to `StartSession` explicitly clarifying that consecutive invocations will preempt and overwrite any existing pending confirmation state.
Done
Please fix this WARNING reported by chrome-ipc-security: Per the Chrome IPC Engineering Guide (**API-T1-02**: Null State Semantics in Mojo Callbacks), nullable parameter signals in state-tracking IPC callbacks must explicitly document what the `null` state represents.\n\nWith the immersive confirmation flow now merged into `StartSession`, if the user closes or denies the confirmation prompt, this will presumably result in a `null` session being returned. Please explicitly document this semantic meaning of the `null` `session` state to prevent state machine desynchronization on the consumer side.
- Frank Liberato
- Gurmeet Kalra
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. |
Gurmeet Kalra (Gerrit)
Gurmeet Kalra added 1 comment![]( "Open in Gerrit")
if (!WebContents::FromRenderFrameHost(&render_frame_host())->IsFullscreen()) {`WebContents::FromRenderFrameHost(&render_frame_host())` possibly return a nullptr during abrupt teardown? It would be better to check for null before dereferencing it to call IsFullscreen().
Related details
- Frank Liberato
- Oleh Desiatyrikov (xWF)
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. |
Oleh Desiatyrikov (xWF) (Gerrit)
Oleh Desiatyrikov (xWF) added 1 comment![]( "Open in Gerrit")
if (!WebContents::FromRenderFrameHost(&render_frame_host())->IsFullscreen()) {`WebContents::FromRenderFrameHost(&render_frame_host())` possibly return a nullptr during abrupt teardown? It would be better to check for null before dereferencing it to call IsFullscreen().
`GetController()` implementation does not have the null check for the same call.
Related details
- Frank Liberato
- Gurmeet Kalra
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. |
Gurmeet Kalra (Gerrit)
Gurmeet Kalra added 1 comment![]( "Open in Gerrit")
if (!WebContents::FromRenderFrameHost(&render_frame_host())->IsFullscreen()) {Oleh Desiatyrikov (xWF)`WebContents::FromRenderFrameHost(&render_frame_host())` possibly return a nullptr during abrupt teardown? It would be better to check for null before dereferencing it to call IsFullscreen().
`GetController()` implementation does not have the null check for the same call.
Good point, Oleh. If it's missing in GetController() too, maybe there's an invariant I'm not aware of that guarantees WebContents is live here. But to be safe, adding the check seems prudent. We should probably investigate the safety of the call in GetController() as well in a follow-up in AXR case.
Related details
- Frank Liberato
- Oleh Desiatyrikov (xWF)
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 2 comments![]( "Open in Gerrit")
if (auto pending = std::move(pending_session_)) {this pattern is fragile -- for example, the confirmation dialog might still complete from the previous request, and `pending_session_` might be the new request by the time the previous `OnImmersivePlaybackConfirmation` happens. maybe there's some detail i missed that prevents this, but it's subtle at best. see below for ideas.
GetController().RequestImmersivePlaybackConfirmation(base::BindOnce(this should bind `pending_session` as the first argument, rather than storing it in `pending_session_`. that way, there's no question about what request is being referred to by the eventual callback. there are some things to take care of, though.
first, we need to be able to indicate that the request has been canceled. the easiest way i know if is to `weak_factory_.InvalidateWeakPtrs()` and magically any weak ptr vended earlier than this will return null => callback will not run. of course, you want to be sure this is the only use of `weak_factory_` 'cause that would surprise other users. `immersive_confirmation_weak_factory_` or similar would make it quite clear. note that `InvalidateWeakPtrs()` doesn't break pointers vended after it's called; you can still call `GetWeakPtr()` later and it'll be a normal, non-null weak pointer.
then you can just unconditionally reset the factory in both `StartSession()` and `StartSessionImmersive()` and any previous callback will simply no-op if they're run.
the other potential issue is the callback that the pending request holds. somebody needs to call it, eventually, in the failure case that you currently handle at line 99. one option is to add code in `~PendingSession()` that checks if it still has a valid callback and calls it at that point.
there's actually a mojo utility wrapper that does exactly this, but i don't remember the name. in this case, i think it's clearer to have `PendingSession` handle it, since it knows exactly what's going on.
Related details
- Oleh Desiatyrikov (xWF)
Oleh Desiatyrikov (xWF) (Gerrit)
Oleh Desiatyrikov (xWF) added 1 comment![]( "Open in Gerrit")
GetController().RequestImmersivePlaybackConfirmation(base::BindOnce(this should bind `pending_session` as the first argument, rather than storing it in `pending_session_`. that way, there's no question about what request is being referred to by the eventual callback. there are some things to take care of, though.
first, we need to be able to indicate that the request has been canceled. the easiest way i know if is to `weak_factory_.InvalidateWeakPtrs()` and magically any weak ptr vended earlier than this will return null => callback will not run. of course, you want to be sure this is the only use of `weak_factory_` 'cause that would surprise other users. `immersive_confirmation_weak_factory_` or similar would make it quite clear. note that `InvalidateWeakPtrs()` doesn't break pointers vended after it's called; you can still call `GetWeakPtr()` later and it'll be a normal, non-null weak pointer.
then you can just unconditionally reset the factory in both `StartSession()` and `StartSessionImmersive()` and any previous callback will simply no-op if they're run.
the other potential issue is the callback that the pending request holds. somebody needs to call it, eventually, in the failure case that you currently handle at line 99. one option is to add code in `~PendingSession()` that checks if it still has a valid callback and calls it at that point.
there's actually a mojo utility wrapper that does exactly this, but i don't remember the name. in this case, i think it's clearer to have `PendingSession` handle it, since it knows exactly what's going on.
- Frank Liberato
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. |
Oleh Desiatyrikov (xWF) (Gerrit)
Oleh Desiatyrikov (xWF) added 1 comment![]( "Open in Gerrit")
GetController().RequestImmersivePlaybackConfirmation(base::BindOnce(Oleh Desiatyrikov (xWF)this should bind `pending_session` as the first argument, rather than storing it in `pending_session_`. that way, there's no question about what request is being referred to by the eventual callback. there are some things to take care of, though.
first, we need to be able to indicate that the request has been canceled. the easiest way i know if is to `weak_factory_.InvalidateWeakPtrs()` and magically any weak ptr vended earlier than this will return null => callback will not run. of course, you want to be sure this is the only use of `weak_factory_` 'cause that would surprise other users. `immersive_confirmation_weak_factory_` or similar would make it quite clear. note that `InvalidateWeakPtrs()` doesn't break pointers vended after it's called; you can still call `GetWeakPtr()` later and it'll be a normal, non-null weak pointer.
then you can just unconditionally reset the factory in both `StartSession()` and `StartSessionImmersive()` and any previous callback will simply no-op if they're run.
the other potential issue is the callback that the pending request holds. somebody needs to call it, eventually, in the failure case that you currently handle at line 99. one option is to add code in `~PendingSession()` that checks if it still has a valid callback and calls it at that point.
there's actually a mojo utility wrapper that does exactly this, but i don't remember the name. in this case, i think it's clearer to have `PendingSession` handle it, since it knows exactly what's going on.
Sounds neat! Thank you for the suggestions!
@libe...@chromium.org should I maybe create a separate `immersive_picture_in_picture_service_impl` and extend it from the base `picture_in_picture_service_impl`? Then I could move the factory method to a new file and let it decide which one to instantiate based on the feature availability, similarly to the `chrome/browser/ui/android/overlay/overlay_window_android_factory.cc`. WDYT?
Frank Liberato (Gerrit)
Frank Liberato added 1 comment![]( "Open in Gerrit")
GetController().RequestImmersivePlaybackConfirmation(base::BindOnce(Oleh Desiatyrikov (xWF)this should bind `pending_session` as the first argument, rather than storing it in `pending_session_`. that way, there's no question about what request is being referred to by the eventual callback. there are some things to take care of, though.
first, we need to be able to indicate that the request has been canceled. the easiest way i know if is to `weak_factory_.InvalidateWeakPtrs()` and magically any weak ptr vended earlier than this will return null => callback will not run. of course, you want to be sure this is the only use of `weak_factory_` 'cause that would surprise other users. `immersive_confirmation_weak_factory_` or similar would make it quite clear. note that `InvalidateWeakPtrs()` doesn't break pointers vended after it's called; you can still call `GetWeakPtr()` later and it'll be a normal, non-null weak pointer.
then you can just unconditionally reset the factory in both `StartSession()` and `StartSessionImmersive()` and any previous callback will simply no-op if they're run.
the other potential issue is the callback that the pending request holds. somebody needs to call it, eventually, in the failure case that you currently handle at line 99. one option is to add code in `~PendingSession()` that checks if it still has a valid callback and calls it at that point.
there's actually a mojo utility wrapper that does exactly this, but i don't remember the name. in this case, i think it's clearer to have `PendingSession` handle it, since it knows exactly what's going on.
Oleh Desiatyrikov (xWF)Sounds neat! Thank you for the suggestions!
@libe...@chromium.org should I maybe create a separate `immersive_picture_in_picture_service_impl` and extend it from the base `picture_in_picture_service_impl`? Then I could move the factory method to a new file and let it decide which one to instantiate based on the feature availability, similarly to the `chrome/browser/ui/android/overlay/overlay_window_android_factory.cc`. WDYT?
not sure if it's worthwhile or not, but i'd do that as a follow-up CL if so. i think probably not, since i'm not sure how it would work if there were interleaved pip and immersive requests from the renderer.
Related details
- Frank Liberato
- Oleh Desiatyrikov (xWF)
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. |
Oleh Desiatyrikov (xWF) (Gerrit)
Oleh Desiatyrikov (xWF) added 3 comments![]( "Open in Gerrit")
this pattern is fragile -- for example, the confirmation dialog might still complete from the previous request, and `pending_session_` might be the new request by the time the previous `OnImmersivePlaybackConfirmation` happens. maybe there's some detail i missed that prevents this, but it's subtle at best. see below for ideas.
Done
if (!WebContents::FromRenderFrameHost(&render_frame_host())->IsFullscreen()) {Oleh Desiatyrikov (xWF)`WebContents::FromRenderFrameHost(&render_frame_host())` possibly return a nullptr during abrupt teardown? It would be better to check for null before dereferencing it to call IsFullscreen().
Gurmeet Kalra`GetController()` implementation does not have the null check for the same call.
Good point, Oleh. If it's missing in GetController() too, maybe there's an invariant I'm not aware of that guarantees WebContents is live here. But to be safe, adding the check seems prudent. We should probably investigate the safety of the call in GetController() as well in a follow-up in AXR case.
Done
GetController().RequestImmersivePlaybackConfirmation(base::BindOnce(Oleh Desiatyrikov (xWF)this should bind `pending_session` as the first argument, rather than storing it in `pending_session_`. that way, there's no question about what request is being referred to by the eventual callback. there are some things to take care of, though.
first, we need to be able to indicate that the request has been canceled. the easiest way i know if is to `weak_factory_.InvalidateWeakPtrs()` and magically any weak ptr vended earlier than this will return null => callback will not run. of course, you want to be sure this is the only use of `weak_factory_` 'cause that would surprise other users. `immersive_confirmation_weak_factory_` or similar would make it quite clear. note that `InvalidateWeakPtrs()` doesn't break pointers vended after it's called; you can still call `GetWeakPtr()` later and it'll be a normal, non-null weak pointer.
then you can just unconditionally reset the factory in both `StartSession()` and `StartSessionImmersive()` and any previous callback will simply no-op if they're run.
the other potential issue is the callback that the pending request holds. somebody needs to call it, eventually, in the failure case that you currently handle at line 99. one option is to add code in `~PendingSession()` that checks if it still has a valid callback and calls it at that point.
there's actually a mojo utility wrapper that does exactly this, but i don't remember the name. in this case, i think it's clearer to have `PendingSession` handle it, since it knows exactly what's going on.
Oleh Desiatyrikov (xWF)Sounds neat! Thank you for the suggestions!
Frank Liberato@libe...@chromium.org should I maybe create a separate `immersive_picture_in_picture_service_impl` and extend it from the base `picture_in_picture_service_impl`? Then I could move the factory method to a new file and let it decide which one to instantiate based on the feature availability, similarly to the `chrome/browser/ui/android/overlay/overlay_window_android_factory.cc`. WDYT?
not sure if it's worthwhile or not, but i'd do that as a follow-up CL if so. i think probably not, since i'm not sure how it would work if there were interleaved pip and immersive requests from the renderer.
- Frank Liberato
- Frank Liberato
- Gurmeet Kalra
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. |
Gurmeet Kalra (Gerrit)
Gurmeet Kalra voted and added 2 comments![]( "Open in Gerrit")
Votes added by Gurmeet Kalra
| Code-Review | +1 |
2 comments
LGTM.
Please add a recording of the simplified confirmation flow in the description, thanks!
return;Please fix this WARNING reported by autoreview issue finding: If the function returns here, the `pending_session->callback` is not invoked until `pending_session` is destroyed. It might be better to explicitly run the callback with (mojo::NullRemote(), gfx::Size()) before returning, to immediately signal failure to the caller, similar to what the destructor does.
Related details
- Frank Liberato
- Frank Liberato
- Oleh Desiatyrikov (xWF)
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. |
Oleh Desiatyrikov (xWF) (Gerrit)
Oleh Desiatyrikov (xWF) added 3 comments![]( "Open in Gerrit")
The `picture_in_picture.mojom` can be cleaned up further as we don't need to pass those data structures over mojom anymore. I'll address it separately in crbug.com/500441418
Done: crrev.com/c/7876303
LGTM.
Please add a recording of the simplified confirmation flow in the description, thanks!
There was no UI/UX changes only the backend API updates, so there is nothing to record.
return;Please fix this WARNING reported by autoreview issue finding: If the function returns here, the `pending_session->callback` is not invoked until `pending_session` is destroyed. It might be better to explicitly run the callback with (mojo::NullRemote(), gfx::Size()) before returning, to immediately signal failure to the caller, similar to what the destructor does.
I believe this is a false positive, the `pending_session` will go out of scope and be destroyed synchronously upon return because the `StartSessionImmersive` is an owner of the object.
Related details
- Frank Liberato
- Frank Liberato
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gurmeet Kalra (Gerrit)
Gurmeet Kalra voted Code-Review+1![]( "Open in Gerrit")
- Oleh Desiatyrikov (xWF)
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gurmeet Kalra (Gerrit)
Gurmeet Kalra voted Code-Review+0![]( "Open in Gerrit")
| Code-Review | +0 |
Related details
- Oleh Desiatyrikov (xWF)
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Oleh Desiatyrikov (xWF) (Gerrit)
Message from Oleh Desiatyrikov (xWF)![]( "Open in Gerrit")
- Frank Liberato
- Gurmeet Kalra
- Oleh Desiatyrikov (xWF)
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Frank Liberato (Gerrit)
Frank Liberato added 2 comments![]( "Open in Gerrit")
auto callback = std::move(pending_session->callback);nit: combine these two lines: `std::move(pending_session->callback).Run(...)`.
Related details
- Frank Liberato
- Gurmeet Kalra
- Oleh Desiatyrikov (xWF)
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. |
Oleh Desiatyrikov (xWF) (Gerrit)
Oleh Desiatyrikov (xWF) added 2 comments![]( "Open in Gerrit")
auto callback = std::move(pending_session->callback);nit: combine these two lines: `std::move(pending_session->callback).Run(...)`.
Done
if possible, please add some tests.
- Chromium IPC Reviews
- Frank Liberato
- Frank Liberato
- Gurmeet Kalra
- Kentaro Hara
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Frank Liberato (Gerrit)
Frank Liberato voted Code-Review+1![]( "Open in Gerrit")
- Chromium IPC Reviews
- Frank Liberato
- Kentaro Hara
- Oleh Desiatyrikov (xWF)
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: ort...@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): ort...@chromium.org
Reviewer source(s):
ort...@chromium.org is from context(googleclient/chrome/chromium_gwsq/ipc/config.gwsq)
Related details
- Frank Liberato
- Giovanni Ortuno Urquidi
- Kentaro Hara
- Oleh Desiatyrikov (xWF)
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gurmeet Kalra (Gerrit)
Gurmeet Kalra voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Frank Liberato
- Giovanni Ortuno Urquidi
- Kentaro Hara
- Oleh Desiatyrikov (xWF)
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |