[crd host][linux] Client-controlled layout for multi-process host [chromium/src : main]
Yuwei Huang (Gerrit)
Yuwei Huang voted and added 1 comment![]( "Open in Gerrit")
Votes added by Yuwei Huang
| Commit-Queue | +1 |
1 comment
Related details
- Joe Downing
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joe Downing (Gerrit)
Joe Downing added 3 comments![]( "Open in Gerrit")
std::optional<std::int32_t> screen_id);Screen IDs are int64 in mojom but int32 in the network process. Just wanted to confirm that this is OK, ISTR we have a check for overflow for the conversion but I haven't looked at that in a few years : )
std::string expected_capabilities = "rateLimitResizeRequests multiStream";Do we need to add the check for these capabilities back?
// An unset screen_id indicates that the client is requesting a new display to
// be added to the display layout.
int64? screen_id;Would it be possible to have an explicit 'AddDisplay' method rather than rely on the existence of a struct member to add a new display? I think that would be less error-prone and more obvious for stack traces and debugging if an error occurs.
Related details
- Yuwei Huang
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. |
Yuwei Huang (Gerrit)
Yuwei Huang voted and added 3 comments![]( "Open in Gerrit")
Votes added by Yuwei Huang
| Commit-Queue | +1 |
3 comments
Screen IDs are int64 in mojom but int32 in the network process. Just wanted to confirm that this is OK, ISTR we have a check for overflow for the conversion but I haven't looked at that in a few years : )
Ah, good catch. I think it should be `webrtc::ScreenId`. Not sure how I got this wrong :)
std::string expected_capabilities = "rateLimitResizeRequests multiStream";Do we need to add the check for these capabilities back?
I intentionally removed them, since the test only cares about `touchEvents`. I could put the full list of capabilities here, but then I'd need an #if-else block for `clientControlledLayout`, which doesn't seem to make sense for `TouchEventsCapabilities`.
// An unset screen_id indicates that the client is requesting a new display to
// be added to the display layout.
int64? screen_id;Would it be possible to have an explicit 'AddDisplay' method rather than rely on the existence of a struct member to add a new display? I think that would be less error-prone and more obvious for stack traces and debugging if an error occurs.
The client is sending the protobuf message in this format, so I think makes more sense to just pass it down to the `GnomeDesktopResizer` as-is. I can imagine that the client side editor is just manipulating the array of display layouts so it may actually prefer this type of representation.
Related details
- Joe Downing
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joe Downing (Gerrit)
Joe Downing voted and added 2 comments![]( "Open in Gerrit")
Votes added by Joe Downing
| Code-Review | +1 |
2 comments
std::string expected_capabilities = "rateLimitResizeRequests multiStream";Yuwei HuangDo we need to add the check for these capabilities back?
I intentionally removed them, since the test only cares about `touchEvents`. I could put the full list of capabilities here, but then I'd need an #if-else block for `clientControlledLayout`, which doesn't seem to make sense for `TouchEventsCapabilities`.
That's reasonable, adding a test for it would just be a change detector which wouldn't be very useful by itself.
// An unset screen_id indicates that the client is requesting a new display to
// be added to the display layout.
int64? screen_id;Yuwei HuangWould it be possible to have an explicit 'AddDisplay' method rather than rely on the existence of a struct member to add a new display? I think that would be less error-prone and more obvious for stack traces and debugging if an error occurs.
The client is sending the protobuf message in this format, so I think makes more sense to just pass it down to the `GnomeDesktopResizer` as-is. I can imagine that the client side editor is just manipulating the array of display layouts so it may actually prefer this type of representation.
I'm thinking of the case we had recently where an 'empty' resolution meant restore the previous resolution which always seemed kinda weird and ended up being something we had to address. I would prefer explicit control messages but if this mirrors the client messages then I guess that's fine, espe.cially since in this case, using the absence of a field to mean 'allocate a display' is an add/update operation rather than a revert.
Related details
- Yuwei Huang
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yuwei Huang (Gerrit)
Yuwei Huang voted and added 1 comment![]( "Open in Gerrit")
Votes added by Yuwei Huang
- Chromium IPC Reviews
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: na...@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): na...@chromium.org
Reviewer source(s):
na...@chromium.org is from context(googleclient/chrome/chromium_gwsq/ipc/config.gwsq)
Related details
- Nasko Oskov
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yuwei Huang (Gerrit)
Yuwei Huang voted and added 1 comment![]( "Open in Gerrit")
Votes added by Yuwei Huang
| Commit-Queue | +1 |
1 comment
- Chromium IPC Reviews
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: mpde...@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): mpde...@chromium.org
Reviewer source(s):
mpde...@chromium.org is from context(googleclient/chrome/chromium_gwsq/ipc/config.gwsq)
Related details
- Matthew Denton
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yuwei Huang (Gerrit)
Yuwei Huang added 1 comment![]( "Open in Gerrit")
- Chromium IPC Reviews
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: na...@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): na...@chromium.org
Reviewer source(s):
na...@chromium.org is from context(googleclient/chrome/chromium_gwsq/ipc/config.gwsq)
- Nasko Oskov
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: mea...@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): mea...@chromium.org
Reviewer source(s):
mea...@chromium.org is from context(googleclient/chrome/chromium_gwsq/ipc/config.gwsq)
Related details
- Mustafa Emre Acer
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mustafa Emre Acer (Gerrit)
Mustafa Emre Acer voted and added 1 comment![]( "Open in Gerrit")
Votes added by Mustafa Emre Acer
| Code-Review | +1 |
| Commit-Queue | +2 |
1 comment
- Yuwei Huang
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yuwei Huang (Gerrit)
Yuwei Huang voted Commit-Queue+2![]( "Open in Gerrit")
- Mustafa Emre Acer
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| 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
[crd host][linux] Client-controlled layout for multi-process host
The single-process host already supports this, so this is pretty much
just fixing the piping that plumbs the VideoLayout from the network
process to the desktop process.
- M remoting/host/desktop_session_agent.cc
- M remoting/host/desktop_session_agent.h
- M remoting/host/desktop_session_proxy.cc
- M remoting/host/desktop_session_proxy.h
- M remoting/host/file_transfer/ipc_file_operations_unittest.cc
- M remoting/host/ipc_desktop_environment_unittest.cc
- M remoting/host/ipc_screen_controls.cc
- M remoting/host/mojom/BUILD.gn
- M remoting/host/mojom/desktop_session.mojom
- M remoting/host/mojom/remoting_mojom_traits.cc
- M remoting/host/mojom/remoting_mojom_traits.h
Code-Review: +1 by Mustafa Emre Acer, +1 by Joe Downing
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |