remoting: Extract input filters into InputPipeline [chromium/src : main]
Yuwei Huang (Gerrit)
Yuwei Huang added 4 comments![]( "Open in Gerrit")
// Prevent dangling pointer in input_pipeline_ if input_injector_ is stillWrap these with ``` `` ```.
input_pipeline_.SetTarget(nullptr);This awkward line can be avoided if you move `input_pipeline_` after `input_injector_`.
// Avoid dangling raw_ptr in `input_tracker_` after deletingSince you are now calling `input_pipeline_.SetTarget(nullptr);` instead of `input_tracker_.set_input_stub(nullptr);`, consider updating this comment to refer to `input_pipeline_` for consistency (e.g., "Avoid dangling raw_ptr in `input_pipeline_`...").
protocol::FractionalInputFilter* fractional_input_filter() {Unlike the other accessors, `fractional_input_filter()` doesn't appear to be used anywhere in `ClientSession` (since it is configured internally using the `CoordinateConverter` passed to the constructor). Feel free to remove it for better encapsulation if it's not needed.
Related details
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 added 4 comments![]( "Open in Gerrit")
// Prevent dangling pointer in input_pipeline_ if input_injector_ is stillYuwei HuangWrap these with ``` `` ```.
Done
This awkward line can be avoided if you move `input_pipeline_` after `input_injector_`.
Done
// Avoid dangling raw_ptr in `input_tracker_` after deletingSince you are now calling `input_pipeline_.SetTarget(nullptr);` instead of `input_tracker_.set_input_stub(nullptr);`, consider updating this comment to refer to `input_pipeline_` for consistency (e.g., "Avoid dangling raw_ptr in `input_pipeline_`...").
Done
protocol::FractionalInputFilter* fractional_input_filter() {Unlike the other accessors, `fractional_input_filter()` doesn't appear to be used anywhere in `ClientSession` (since it is configured internally using the `CoordinateConverter` passed to the constructor). Feel free to remove it for better encapsulation if it's not needed.
Done
Related details
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
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 4 comments![]( "Open in Gerrit")
Votes added by Joe Downing
| Code-Review | +1 |
4 comments
lgtm with a couple of questions/suggestions.
// Declared after input_injector_ because it holds a reference to it (vianit: |input_injector_|
// Accessors for configuring or observing specific filters in the chain.I like the idea of encapsulating the input pipeline since it is so error-prone and complicated and it's nice to have something that is more testable but I think exposing the individual filters does not take advantage of this encapsulation.
Ideally the InputPipeline would provide methods directly, rather than accessors. Also it would be great to see some InputPipeline tests added.
I don't feel too strongly since this is a yak-shaving CL so feel free to ack but I think this would be a nice improvement in a follow-up.
void SetTarget(protocol::InputStub* target);Why change the name of the method and arg to 'target'? IMO using 'SetInputStub' would be clearer and is easier to search for. Target is generic and makes me think of DOM events.
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 4 comments![]( "Open in Gerrit")
Votes added by Yuwei Huang
| Commit-Queue | +2 |
4 comments
// Declared after input_injector_ because it holds a reference to it (viaYuwei Huangnit: |input_injector_|
Done, using ``` `` ```.
// Accessors for configuring or observing specific filters in the chain.I like the idea of encapsulating the input pipeline since it is so error-prone and complicated and it's nice to have something that is more testable but I think exposing the individual filters does not take advantage of this encapsulation.
Ideally the InputPipeline would provide methods directly, rather than accessors. Also it would be great to see some InputPipeline tests added.
I don't feel too strongly since this is a yak-shaving CL so feel free to ack but I think this would be a nice improvement in a follow-up.
Acknowledged. We will do it in a follow-up CL.
Why change the name of the method and arg to 'target'? IMO using 'SetInputStub' would be clearer and is easier to search for. Target is generic and makes me think of DOM events.
I didn't do it because `InputPipeline` itself is an `InputStub` which feels confusing, but given `InputFilter` already has `set_input_filter`, I think this is fine. Done.
Related details
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 with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
2 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: remoting/host/client_session.h
Insertions: 1, Deletions: 1.
The diff is too large to show. Please review the diff.
```
```
The name of the file: remoting/host/input_pipeline.cc
Insertions: 2, Deletions: 2.
The diff is too large to show. Please review the diff.
```
```
The name of the file: remoting/host/input_pipeline.h
Insertions: 3, Deletions: 2.
The diff is too large to show. Please review the diff.
```
```
The name of the file: remoting/host/client_session.cc
Insertions: 2, Deletions: 2.
The diff is too large to show. Please review the diff.
```
Change information
remoting: Extract input filters into InputPipeline
This CL refactors ClientSession to encapsulate its 7 individual input
filters into a new dedicated class, InputPipeline.
Why this is needed:
This is a preparatory refactoring for the dedicated Peer Connection
(PC) process architecture. To support running WebRTC in a sandboxed
process, we will move WebRTC connection and input injection logic
from ClientSession (Network process) to PeerSessionImpl (PC process).
Currently, ClientSession manages and chains these 7 filters manually.
Extracting them into a self-contained InputPipeline class simplifies
ClientSession and makes the input path modular. This allows us to
easily move the entire input pipeline to PeerSessionImpl in a
subsequent CL, keeping the multi-process integration CLs small and
reviewable.
This CL also improves code health by isolating the input path and
making the lifetime safety requirements (reverse-declaration order)
explicit.
TAG=agy
CONV=bf005916-ada6-4139-af52-be0884b5cbf0
- M remoting/host/BUILD.gn
- M remoting/host/client_session.cc
- M remoting/host/client_session.h
- A remoting/host/input_pipeline.cc
- A remoting/host/input_pipeline.h
Code-Review: +1 by Joe Downing
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |