Implement EmulatedSmartCardContextFactory interception logic. [chromium/src : main]
Paulina Gacek (Gerrit)
Paulina Gacek removed Alex Moshchuk from this change![]( "Open in Gerrit")
- Alex Moshchuk
Related details
- Zgroza (Luke) Klimek
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Patryk Chodur (Gerrit)
Patryk Chodur added 3 comments![]( "Open in Gerrit")
std::map<std::string, std::unique_ptr<PendingRequest>> pending_requests_;Could you please use `std::unordered_map`? It's usually way faster and should be the default unless you specifically need `std::map` (for example if data needs to be sorted).
auto* req = static_cast<PendingListReaders*>(base_req.get());Why do we need a `static_cast` here? It's UB if we get it wrong. Maybe there could be two containers with concrete types (`PendingCreateContext` and `PendingListReaders`), or just a single class instead of both `PendingCreateContext` and `PendingListReaders`?
auto* req = static_cast<PendingCreateContext*>(base_req.get());Same as above.
Related details
- Paulina Gacek
- Zgroza (Luke) Klimek
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. |
Paulina Gacek (Gerrit)
Paulina Gacek added 3 comments![]( "Open in Gerrit")
std::map<std::string, std::unique_ptr<PendingRequest>> pending_requests_;Could you please use `std::unordered_map`? It's usually way faster and should be the default unless you specifically need `std::map` (for example if data needs to be sorted).
Done
auto* req = static_cast<PendingListReaders*>(base_req.get());Why do we need a `static_cast` here? It's UB if we get it wrong. Maybe there could be two containers with concrete types (`PendingCreateContext` and `PendingListReaders`), or just a single class instead of both `PendingCreateContext` and `PendingListReaders`?
Good point, thanks. To avoid UB without introducing multiple maps (which would become messy as I add more request types later), I added a type tag to the base class. The code now verifies the request type explicitly before casting.
auto* req = static_cast<PendingCreateContext*>(base_req.get());Paulina GacekSame as above.
Done
Related details
- Patryk Chodur
- Zgroza (Luke) Klimek
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Patryk Chodur (Gerrit)
Patryk Chodur voted Code-Review+1![]( "Open in Gerrit")
- Paulina Gacek
- Zgroza (Luke) Klimek
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Zgroza (Luke) Klimek (Gerrit)
Zgroza (Luke) Klimek added 4 comments![]( "Open in Gerrit")
Implement EmulatedSmartCardContextFactory interception logic.Nit: I would prefer saying something like
```suggestion
Implement EmulatedSmartCardContextFactory CDP implementation.
```
since, unless I see something wrong, there's not yet any actual interception here yet, just the implementation of a thing that will be intercepting when the interception logic is here.
Implement EmulatedSmartCardContextFactory interception logic.Can You add `SmartCardEmulation: ...` to CL titles in this entire chain, as well as the corresponding hashtag (I think this will be added automatically but idk)? It helps with finding CLs relevant to a feature.
# TODO(crbug.com/470349523): Migrate more smart card files to use this flag.Actually, can this entire block be merged with [those](https://source.chromium.org/chromium/chromium/src/+/main:content/browser/BUILD.gn;l=2670;drc=e6b9ce5422805a5459232191b770d746a525e15e), either by moving it or moving those file inclusions here? It'd be nice to have all the smart card files included here in the same place.
void OnListReaders(I have a feeling that those methods belong in `EmulatedSmartCardContext` class, as those functionalities depend on the context. Why are they here?
Related details
- Paulina Gacek
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. |
Zgroza (Luke) Klimek (Gerrit)
Zgroza (Luke) Klimek added 4 comments![]( "Open in Gerrit")
raw_ptr<EmulatedSmartCardNotifier> notifier_;I... don't particularly like `raw_ptr`s. What is the lifetime of `EmulatedSmartCardNotifier`, what owns it? (In here just tests, but what will own it by design in the future?)
If this is trivially correct here, please at least add a comment on why. Otherwise, depending on the ownership, maybe `WeakPtr` or something?
void OnListReaders(I have a feeling that those methods belong in `EmulatedSmartCardContext` class, as those functionalities depend on the context. Why are they here?
To add to this: if the reason is that this class is currently the only one that has access to the `notifier_`, I have a feeling that this might not be a good choice from the single responsibility perspective (this class is responsible not only for creating `Context`s, but also for relaying all the responses from the other classes).
In essence, I think that `notifier_` probably should be directly accessible from all of the classes that have the functionality of notifying, having `EmulatedSmartCardContextFactory` as universal proxy for this is not in line with its purpose.
// The handler must outlive this factory.As below, what will ensure this?
if (enable_smart_card) {As above, can You move to this block other smart card files in this file? Examples: [here](https://source.chromium.org/chromium/chromium/src/+/main:content/test/BUILD.gn;drc=7a77fe52e7fbc676291a32292e11f9204086588b;l=887) and [here](https://source.chromium.org/chromium/chromium/src/+/main:content/test/BUILD.gn;drc=7a77fe52e7fbc676291a32292e11f9204086588b;l=2325).