Add Web Smart Card Protocol configuration. [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 1 comment![]( "Open in Gerrit")
enable_smart_card_emulation = is_chromeosHm, now I'm thinking... there's quite a lot of places in which smart card stuff is excluded based on `is_chromeos`. Maybe it would be a good idea to create not `enable_smart_card_emulation` but rather `smart_card_api_supported` (+ the right macro) and file a bug in smart card component (public one) to migrate current ifs to this?
In the current state expanding the API to other platforms sounds like a giant mess of changing a lot of `if`s and `#if`s.
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 1 comment![]( "Open in Gerrit")
enable_smart_card_emulation = is_chromeosHm, now I'm thinking... there's quite a lot of places in which smart card stuff is excluded based on `is_chromeos`. Maybe it would be a good idea to create not `enable_smart_card_emulation` but rather `smart_card_api_supported` (+ the right macro) and file a bug in smart card component (public one) to migrate current ifs to this?
In the current state expanding the API to other platforms sounds like a giant mess of changing a lot of `if`s and `#if`s.
Do you want me to also change the flag `ENABLE_SMART_CARD_EMULATION` to `IS_SMART_CARD_API_SUPPORTED`?
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. |
Paulina Gacek (Gerrit)
Paulina Gacek added 1 comment![]( "Open in Gerrit")
Paulina GacekHm, now I'm thinking... there's quite a lot of places in which smart card stuff is excluded based on `is_chromeos`. Maybe it would be a good idea to create not `enable_smart_card_emulation` but rather `smart_card_api_supported` (+ the right macro) and file a bug in smart card component (public one) to migrate current ifs to this?
In the current state expanding the API to other platforms sounds like a giant mess of changing a lot of `if`s and `#if`s.
Do you want me to also change the flag `ENABLE_SMART_CARD_EMULATION` to `IS_SMART_CARD_API_SUPPORTED`?
Done
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 voted and added 2 comments![]( "Open in Gerrit")
Votes added by Zgroza (Luke) Klimek
| Code-Review | +1 |
2 comments
defines += [ "IS_SMART_CARD_API_SUPPORTED" ]Please put here something like this, if possible.
```suggestion
# TODO(crbug.com/470349523): migrate more smart card files to this
defines += [ "IS_SMART_CARD_API_SUPPORTED" ]
```
is_smart_card_api_supported = is_chromeosPlease put something like this here:
```suggestion
# TODO(crbug.com/470349523): migrate more smart card files to this
is_smart_card_api_supported = is_chromeos
```
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")
Please put here something like this, if possible.
```suggestion
# TODO(crbug.com/470349523): migrate more smart card files to this
defines += [ "IS_SMART_CARD_API_SUPPORTED" ]
```
Done
Please put something like this here:
```suggestion
# TODO(crbug.com/470349523): migrate more smart card files to this
is_smart_card_api_supported = is_chromeos
```
- 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. |
Rakina Zata Amni (Gerrit)
Rakina Zata Amni added 1 comment![]( "Open in Gerrit")
import("//content/public/common/features.gni")Other cases like `enable_bluetooth_emulation` seems to use `.gni` files from directories more specific to the feature instead of `//content/public/common/features.gni`. Is that possible to do for this as well? (Just not sure if the flag belongs in that file, but if there's no other better choice maybe it's fine)
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 1 comment![]( "Open in Gerrit")
import("//content/public/common/features.gni")Other cases like `enable_bluetooth_emulation` seems to use `.gni` files from directories more specific to the feature instead of `//content/public/common/features.gni`. Is that possible to do for this as well? (Just not sure if the flag belongs in that file, but if there's no other better choice maybe it's fine)
Hi Akita, thanks for the feedback! We've placed the `enable_smart_card` flag here because this feature will have code in both the `content/` layer and in `third_party/blink/`.
Related details
- Rakina Zata Amni
- 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 1 comment![]( "Open in Gerrit")
import("//content/public/common/features.gni")Paulina GacekOther cases like `enable_bluetooth_emulation` seems to use `.gni` files from directories more specific to the feature instead of `//content/public/common/features.gni`. Is that possible to do for this as well? (Just not sure if the flag belongs in that file, but if there's no other better choice maybe it's fine)
Hi Akita, thanks for the feedback! We've placed the `enable_smart_card` flag here because this feature will have code in both the `content/` layer and in `third_party/blink/`.
*Rakina, apologies 🙏
Related details
- Rakina Zata Amni
- 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 Code-Review+1![]( "Open in Gerrit")
- Paulina Gacek
- Rakina Zata Amni
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 1 comment![]( "Open in Gerrit")
import("//content/public/common/features.gni")Paulina GacekOther cases like `enable_bluetooth_emulation` seems to use `.gni` files from directories more specific to the feature instead of `//content/public/common/features.gni`. Is that possible to do for this as well? (Just not sure if the flag belongs in that file, but if there's no other better choice maybe it's fine)
Paulina GacekHi Akita, thanks for the feedback! We've placed the `enable_smart_card` flag here because this feature will have code in both the `content/` layer and in `third_party/blink/`.
*Rakina, apologies 🙏
- Alex Moshchuk
- Rakina Zata Amni
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Moshchuk (Gerrit)
Alex Moshchuk added 1 comment![]( "Open in Gerrit")
import("//content/public/common/features.gni")Paulina GacekOther cases like `enable_bluetooth_emulation` seems to use `.gni` files from directories more specific to the feature instead of `//content/public/common/features.gni`. Is that possible to do for this as well? (Just not sure if the flag belongs in that file, but if there's no other better choice maybe it's fine)
Paulina GacekHi Akita, thanks for the feedback! We've placed the `enable_smart_card` flag here because this feature will have code in both the `content/` layer and in `third_party/blink/`.
Paulina Gacek*Rakina, apologies 🙏
Acknowledged
Placing it in content/public would expose it more widely outside of content/, though (e.g., it would be accessible from //chrome). I'm not sure that's needed, though maybe as Rakina said, there's no better choice? Typically for cases where content and blink need to share some code, the feature definition would go into blink/public, and content would include that dependency (content can use stuff from blink/public). So just curious if it'd be better to define this in blink? (e.g., maybe [here](https://source.chromium.org/chromium/chromium/src/+/main:content/browser/BUILD.gn;l=26;drc=a35f3c68a82831260fbf2d9ce29c00c9007ad3ac)? - though I'd defer to Blink owners for guidance)
Related details
- Paulina Gacek
- Rakina Zata Amni
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 1 comment![]( "Open in Gerrit")
import("//content/public/common/features.gni")Paulina GacekOther cases like `enable_bluetooth_emulation` seems to use `.gni` files from directories more specific to the feature instead of `//content/public/common/features.gni`. Is that possible to do for this as well? (Just not sure if the flag belongs in that file, but if there's no other better choice maybe it's fine)
Paulina GacekHi Akita, thanks for the feedback! We've placed the `enable_smart_card` flag here because this feature will have code in both the `content/` layer and in `third_party/blink/`.
Paulina Gacek*Rakina, apologies 🙏
Alex MoshchukAcknowledged
Placing it in content/public would expose it more widely outside of content/, though (e.g., it would be accessible from //chrome). I'm not sure that's needed, though maybe as Rakina said, there's no better choice? Typically for cases where content and blink need to share some code, the feature definition would go into blink/public, and content would include that dependency (content can use stuff from blink/public). So just curious if it'd be better to define this in blink? (e.g., maybe [here](https://source.chromium.org/chromium/chromium/src/+/main:content/browser/BUILD.gn;l=26;drc=a35f3c68a82831260fbf2d9ce29c00c9007ad3ac)? - though I'd defer to Blink owners for guidance)
So, the plan is to use it from:
- `//third_party/blink`
- `//content`
- `//chrome`
so something as widely accessible as possible would be good indeed.