Create SmartCardEmulation domain definition. [chromium/src : main]
Paulina Gacek (Gerrit)
Paulina Gacek voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
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. |
Zgroza (Luke) Klimek (Gerrit)
Zgroza (Luke) Klimek added 3 comments![]( "Open in Gerrit")
General comment: can You expand comments here with names and links to the particular native functions implemented by this those? Ideally every call that maps to PC/SC call should have adequate links to its equivalent in those 2 documentation:
- https://pcsclite.apdu.fr/api/group__API.html (PC/SC Lite)
- https://learn.microsoft.com/en-us/windows/win32/api/winscard/ (PC/SC by Microsoft)
# Reports the result of a call that returns only a result codeCan You expand this comment with a list of calls that can result with this?
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. |
Paulina Gacek (Gerrit)
Paulina Gacek added 2 comments![]( "Open in Gerrit")
General comment: can You expand comments here with names and links to the particular native functions implemented by this those? Ideally every call that maps to PC/SC call should have adequate links to its equivalent in those 2 documentation:
- https://pcsclite.apdu.fr/api/group__API.html (PC/SC Lite)
- https://learn.microsoft.com/en-us/windows/win32/api/winscard/ (PC/SC by Microsoft)
Done
# Reports the result of a call that returns only a result codeCan You expand this comment with a list of calls that can result with this?
- 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 2 comments![]( "Open in Gerrit")
Nit: maybe a wording like this one, here and elsewhere?
```suggestion
#
# This maps to:
# PC/SC Lite: https://pcsclite.apdu.fr/api/group__ErrorCodes.html
# Win32 API: https://learn.microsoft.com/en-us/windows/win32/secauthn/authentication-return-values
```
type Disposition extends stringThose also map to the values here: https://learn.microsoft.com/en-us/windows/win32/api/winscard/nf-winscard-scarddisconnect#parameters
(applies also to similar enums below)
Can You add links for those as well?
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. |
Paulina Gacek (Gerrit)
Paulina Gacek added 2 comments![]( "Open in Gerrit")
Nit: maybe a wording like this one, here and elsewhere?
```suggestion
#
# This maps to:
# PC/SC Lite: https://pcsclite.apdu.fr/api/group__ErrorCodes.html
# Win32 API: https://learn.microsoft.com/en-us/windows/win32/secauthn/authentication-return-values
```
Done
type Disposition extends stringThose also map to the values here: https://learn.microsoft.com/en-us/windows/win32/api/winscard/nf-winscard-scarddisconnect#parameters
(applies also to similar enums below)
Can You add links for those as well?
I also saw this, but these definitions don't have their own dedicated sections - the link you provided links to parameters of disconnect.
Related details
- 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. |
Zgroza (Luke) Klimek (Gerrit)
Zgroza (Luke) Klimek voted and added 1 comment![]( "Open in Gerrit")
Votes added by Zgroza (Luke) Klimek
| Code-Review | +1 |
1 comment
type Disposition extends stringPaulina GacekThose also map to the values here: https://learn.microsoft.com/en-us/windows/win32/api/winscard/nf-winscard-scarddisconnect#parameters
(applies also to similar enums below)
Can You add links for those as well?
I also saw this, but these definitions don't have their own dedicated sections - the link you provided links to parameters of disconnect.
Hm, maybe it's alright anyway, there are links to functions that use those.
Related details
- Paulina Gacek
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nico Weber (Gerrit)
Nico Weber added 1 comment![]( "Open in Gerrit")
replacing myself with a third_party/blink/public/devtools_protocol/OWNER
Related details
- Alex Rudenko
- Paulina Gacek
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jeremy Roman (Gerrit)
Jeremy Roman added 1 comment![]( "Open in Gerrit")
Is anyone in devtools_protocol/OWNERS available? I don't know this super well, so while it seems reasonable to me, I wouldn't necessarily know what to look out for here.
Related details
- Paulina Gacek
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Paulina Gacek (Gerrit)
Paulina Gacek added 1 comment![]( "Open in Gerrit")
Is anyone in devtools_protocol/OWNERS available? I don't know this super well, so while it seems reasonable to me, I wouldn't necessarily know what to look out for here.
- Andrey Kosyakov
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Paulina Gacek (Gerrit)
Paulina Gacek added 1 comment![]( "Open in Gerrit")
Paulina GacekIs anyone in devtools_protocol/OWNERS available? I don't know this super well, so while it seems reasonable to me, I wouldn't necessarily know what to look out for here.
I can try
I added the owner, thanks!
Andrey Kosyakov (Gerrit)
Andrey Kosyakov added 11 comments![]( "Open in Gerrit")
Mostly looks good, but here are some API suggestions.
unknownHow is this different from unknown-error?
type Protocols extends objectnit: ProtocolSet to further disambiguate from Protocol below? Or, perhaps, just pass `array of Protocol` instead?
undefinedCan we ditch this and make the argument `optional` instead? This would also make it easier to reuse this enum as suggested above.
integer currentCountcurrent count of what?
string contextIdI assume this should be `optional` to accomodate cases when resultCode is an error?
string handlemake optional in case of error? Alternatively, since this is a recurring pattern, we can let clients use `reportPlainResult` (or even a dedicated command `reportOperationFailed`) to report errors and expect `reportFoo` methods carrying specific arguments to only be invoked on success, thus ditching `resultCode` in calls like this one.
array of ReaderStateIn readerStatesnot sure I understand what's happening here. For the underlying API, this appears to be a in/out param, with only the names mattering for input. Why do we pass other fields in ReaderStateIn?
optional integer timeout`number` and document the units, e.g. seconds?
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 5 comments![]( "Open in Gerrit")
unknownHow is this different from unknown-error?
So,
- `unknown-error` means that `SCARD_E_UNKNOWN_CARD` was returned from PC/SC.
- `unknown` means that something outside of this enum (and thus, values supported by the Web Smart Card API) was returned from the native PC/SC. It maps to [this](https://source.chromium.org/chromium/chromium/src/+/main:services/device/public/mojom/smart_card.mojom;drc=288960ba66da3d15f1bfd392b783743538406990;l=66) in Mojo.
See the difference in handling them in Blink [here](https://crsrc.org/c/third_party/blink/renderer/modules/smart_card/smart_card_error.cc;l=190-202).
Might be wise to specify this behavior here, in hindsight.
type Protocols extends objectnit: ProtocolSet to further disambiguate from Protocol below? Or, perhaps, just pass `array of Protocol` instead?
`array of Protocol` theoretically is less constrained, could allow multiple entries of the same which is invalid input that could never be passed through Mojo. But this is passed from the browser which will ensure it, so maybe that's alright?
+1 to `ProtocolSet` though. Or maybe `PreferredProtocols`?
integer currentCountcurrent count of what?
Of insertions, this maps to the API field defined [here](https://wicg.github.io/web-smart-card/#dom-smartcardreaderstatein-currentcount) and is standardized in PC/SC [here](https://pcsclite.apdu.fr/api/group__API.html#:~:text=To%20detect%20a%20reader%20event%20betweeen%202%20calls%20to%20SCardGetStatusChange()%20you%20can%20use%20the%20upper%2016%20bits%20of%20dwCurrentState.).
To the user of this API, who anyway will have to be somehow proficient in PC/SC, this should be readable enough. But `currentInsertionCount` or something similar theoretically wouldn't hurt.
array of ReaderStateIn readerStatesnot sure I understand what's happening here. For the underlying API, this appears to be a in/out param, with only the names mattering for input. Why do we pass other fields in ReaderStateIn?
Native `GetStatusChange` also takes those other arguments and can change behavior based on them. An example would be detecting also past events via setting `currentCount` ([reference](https://pcsclite.apdu.fr/api/group__API.html#:~:text=dwEventState%20also%20contains%20a%20number%20of%20events%20in%20the%20upper%2016%20bits%20(dwEventState%20%26%200xFFFF0000).%20This%20number%20of%20events%20is%20incremented%20for%20each%20card%20insertion%20or%20removal%20in%20the%20specified%20reader.%20This%20can%20be%20used%20to%20detect%20a%20card%20removal/insertion%20between%20two%20calls%20to%20SCardGetStatusChange())).
optional integer timeout`number` and document the units, e.g. seconds?
This is already documented in the linked PC/SC doc [here](https://pcsclite.apdu.fr/api/group__API.html#:~:text=Maximum%20waiting%20time%20(-,in%20milliseconds,-)%20for%20status%20change). Since both Lite and Microsoft's use the same thing (milliseconds) specifying this here wouldn't hurt, but in general I would just send the user to external docs as much as possible.
Paulina Gacek (Gerrit)
Paulina Gacek added 5 comments![]( "Open in Gerrit")
Zgroza (Luke) Klimeknit: ProtocolSet to further disambiguate from Protocol below? Or, perhaps, just pass `array of Protocol` instead?
`array of Protocol` theoretically is less constrained, could allow multiple entries of the same which is invalid input that could never be passed through Mojo. But this is passed from the browser which will ensure it, so maybe that's alright?
+1 to `ProtocolSet` though. Or maybe `PreferredProtocols`?
I think that `ProtocolSet` is more appropriate here as it refers to the type, not specific usage. Done.
Can we ditch this and make the argument `optional` instead? This would also make it easier to reuse this enum as suggested above.
Right, thank you! Done 😊
I assume this should be `optional` to accomodate cases when resultCode is an error?
Done
Zgroza (Luke) Klimek`number` and document the units, e.g. seconds?
This is already documented in the linked PC/SC doc [here](https://pcsclite.apdu.fr/api/group__API.html#:~:text=Maximum%20waiting%20time%20(-,in%20milliseconds,-)%20for%20status%20change). Since both Lite and Microsoft's use the same thing (milliseconds) specifying this here wouldn't hurt, but in general I would just send the user to external docs as much as possible.
- Andrey Kosyakov
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 4 comments![]( "Open in Gerrit")
Zgroza (Luke) KlimekHow is this different from unknown-error?
So,
- `unknown-error` means that `SCARD_E_UNKNOWN_CARD` was returned from PC/SC.
- `unknown` means that something outside of this enum (and thus, values supported by the Web Smart Card API) was returned from the native PC/SC. It maps to [this](https://source.chromium.org/chromium/chromium/src/+/main:services/device/public/mojom/smart_card.mojom;drc=288960ba66da3d15f1bfd392b783743538406990;l=66) in Mojo.
See the difference in handling them in Blink [here](https://crsrc.org/c/third_party/blink/renderer/modules/smart_card/smart_card_error.cc;l=190-202).
Might be wise to specify this behavior here, in hindsight.
I added explanatory comments.
Zgroza (Luke) Klimekcurrent count of what?
Of insertions, this maps to the API field defined [here](https://wicg.github.io/web-smart-card/#dom-smartcardreaderstatein-currentcount) and is standardized in PC/SC [here](https://pcsclite.apdu.fr/api/group__API.html#:~:text=To%20detect%20a%20reader%20event%20betweeen%202%20calls%20to%20SCardGetStatusChange()%20you%20can%20use%20the%20upper%2016%20bits%20of%20dwCurrentState.).
To the user of this API, who anyway will have to be somehow proficient in PC/SC, this should be readable enough. But `currentInsertionCount` or something similar theoretically wouldn't hurt.
I changed the name to `currentInsertionCount`.
make optional in case of error? Alternatively, since this is a recurring pattern, we can let clients use `reportPlainResult` (or even a dedicated command `reportOperationFailed`) to report errors and expect `reportFoo` methods carrying specific arguments to only be invoked on success, thus ditching `resultCode` in calls like this one.
Done
Related details
- Andrey Kosyakov
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. |
Andrey Kosyakov (Gerrit)
Andrey Kosyakov added 3 comments![]( "Open in Gerrit")
unknownZgroza (Luke) KlimekHow is this different from unknown-error?
Paulina GacekSo,
- `unknown-error` means that `SCARD_E_UNKNOWN_CARD` was returned from PC/SC.
- `unknown` means that something outside of this enum (and thus, values supported by the Web Smart Card API) was returned from the native PC/SC. It maps to [this](https://source.chromium.org/chromium/chromium/src/+/main:services/device/public/mojom/smart_card.mojom;drc=288960ba66da3d15f1bfd392b783743538406990;l=66) in Mojo.
See the difference in handling them in Blink [here](https://crsrc.org/c/third_party/blink/renderer/modules/smart_card/smart_card_error.cc;l=190-202).
Might be wise to specify this behavior here, in hindsight.
I added explanatory comments.
Can we rather rename this to unknown-card-error then and, perhaps, follow up with a rename it in mojo too? I think unknown-error generally implies a very different thing from unkown card and mapping it like this can be confusing. I also did a search for SCARD_E_UNKNOWN_CARD and couldn't find us ever handling it -- I would expect I would see some code that converts from underlying API errors to mojom, but I didn't see it.
Please fix this WARNING reported by Trailing Whitespace: Please remove the trailing whitespace.
Please remove the trailing whitespace.
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. |
Andrey Kosyakov (Gerrit)
Andrey Kosyakov voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
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. |