DevTools: Add Smart Card Emulation Handler Skeleton. [chromium/src : main]
Paulina Gacek (Gerrit)
Paulina Gacek added 1 comment![]( "Open in Gerrit")
session->CreateAndAddHandler<protocol::SmartCardEmulationHandler>();Paulina GacekMy understanding was that the emulation is meant to be global and not related to a specific render frame? has that changed? if it is still global, I think this should be in content/browser/devtools/browser_devtools_agent_host.cc
Alex RudenkoThank you for the comment! While Smart Card devices are physically global, the API implementation (`navigator.smartCard`) and the Mojo interface are bound to the specific RenderFrame.
Paulina GacekIs the emulation also global or would it be per render frame?
Per frame.
Related details
- Alex Rudenko
- 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. |
Andrey Kosyakov (Gerrit)
Andrey Kosyakov added 6 comments![]( "Open in Gerrit")
base::WeakPtrFactory<SmartCardEmulationHandler> weak_factory_{this};unused?
base::WeakPtr<RenderFrameHostImpl> host_;raw_ptr<>, please! We're managing this explicitly in all handlers (as in, there would be SetRenderer calls before the host goes away).
.SwapImplForTesting(original_broker_impl_);No ForTesting calls in prod code, please!
if (!enabled_) {So... it's the same whether enabled_ or not?
auto& receiver = rfh_impl->browser_interface_broker_receiver_for_testing();ditto.
return DispatchResponse::ServerError(Just skip these in protocol_config.json instead, the backend will report an error just as well.
Related details
- Alex Rudenko
- Patryk Chodur
- 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. |
Andrey Kosyakov (Gerrit)
Andrey Kosyakov added 1 comment![]( "Open in Gerrit")
It's usually way more practical so write CDP tests in JavaScript. Also, all your implementation is in content/ layer, so I assume it may be possible to test it in the content layer as well? Please see //third_party/blink/web_tests/http/tests/inspector-protocol for examples of content-side CDP tests. It may be possible to run these in chrome as well, the JS-side framework would be the same, but there's a tiny bit of boilerplate required on the C++ side, see chrome/browser/headless/headless_mode_protocol_browsertest.cc.
Paulina Gacek (Gerrit)
Paulina Gacek added 3 comments![]( "Open in Gerrit")
base::WeakPtrFactory<SmartCardEmulationHandler> weak_factory_{this};Paulina Gacekunused?
Done
raw_ptr<>, please! We're managing this explicitly in all handlers (as in, there would be SetRenderer calls before the host goes away).
Done
So... it's the same whether enabled_ or not?
- Alex Rudenko
- Andrey Kosyakov
- Patryk Chodur
- 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 7 comments![]( "Open in Gerrit")
It's usually way more practical so write CDP tests in JavaScript. Also, all your implementation is in content/ layer, so I assume it may be possible to test it in the content layer as well? Please see //third_party/blink/web_tests/http/tests/inspector-protocol for examples of content-side CDP tests. It may be possible to run these in chrome as well, the JS-side framework would be the same, but there's a tiny bit of boilerplate required on the C++ side, see chrome/browser/headless/headless_mode_protocol_browsertest.cc.
Thanks for the suggestion! The Smart Card API must be run within an Isolated Web App (IWA) context. I used IsolatedWebAppBrowserTestHarness (from chrome/) because it handles the complex IWA setup and permissions automatically. Porting this to the content/ layer or web_tests would require significant mocking of the IWA infrastructure.
base::WeakPtrFactory<SmartCardEmulationHandler> weak_factory_{this};Paulina Gacekunused?
Done
I started being used in this CL: https://chromium-review.googlesource.com/c/chromium/src/+/7264773. I moved the declaration there, thanks!
No ForTesting calls in prod code, please!
Done
No ForTesting calls in prod code, please!
Done
auto& receiver = rfh_impl->browser_interface_broker_receiver_for_testing();Paulina Gacekditto.
Done
auto& receiver = rfh_impl->browser_interface_broker_receiver_for_testing();Paulina Gacekditto.
Done
Just skip these in protocol_config.json instead, the backend will report an error just as well.
Done
Related details
- Alex Rudenko
- Andrey Kosyakov
- Patryk Chodur
- 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")
Andrey Kosyakov (Gerrit)
Andrey Kosyakov added 1 comment![]( "Open in Gerrit")
Paulina GacekIt's usually way more practical so write CDP tests in JavaScript. Also, all your implementation is in content/ layer, so I assume it may be possible to test it in the content layer as well? Please see //third_party/blink/web_tests/http/tests/inspector-protocol for examples of content-side CDP tests. It may be possible to run these in chrome as well, the JS-side framework would be the same, but there's a tiny bit of boilerplate required on the C++ side, see chrome/browser/headless/headless_mode_protocol_browsertest.cc.
Thanks for the suggestion! The Smart Card API must be run within an Isolated Web App (IWA) context. I used IsolatedWebAppBrowserTestHarness (from chrome/) because it handles the complex IWA setup and permissions automatically. Porting this to the content/ layer or web_tests would require significant mocking of the IWA infrastructure.
Wouldn't this take care of it?
https://source.chromium.org/chromium/chromium/src/+/main:content/shell/common/shell_switches.h;l=45?
(note we've got a virtual test suite setup to use this https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/virtual/isolated-context/README.md, https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/VirtualTestSuites;l=1748?)
Related details
- Alex Rudenko
- Patryk Chodur
- 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. |
Alex Rudenko (Gerrit)
Alex Rudenko added 3 comments![]( "Open in Gerrit")
Paulina GacekIt's usually way more practical so write CDP tests in JavaScript. Also, all your implementation is in content/ layer, so I assume it may be possible to test it in the content layer as well? Please see //third_party/blink/web_tests/http/tests/inspector-protocol for examples of content-side CDP tests. It may be possible to run these in chrome as well, the JS-side framework would be the same, but there's a tiny bit of boilerplate required on the C++ side, see chrome/browser/headless/headless_mode_protocol_browsertest.cc.
Andrey KosyakovThanks for the suggestion! The Smart Card API must be run within an Isolated Web App (IWA) context. I used IsolatedWebAppBrowserTestHarness (from chrome/) because it handles the complex IWA setup and permissions automatically. Porting this to the content/ layer or web_tests would require significant mocking of the IWA infrastructure.
Wouldn't this take care of it?
https://source.chromium.org/chromium/chromium/src/+/main:content/shell/common/shell_switches.h;l=45?(note we've got a virtual test suite setup to use this https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/virtual/isolated-context/README.md, https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/VirtualTestSuites;l=1748?)
In addition to converting the tests I think the following scenarios would be useful to cover: 1) .enable() applies after a navigation 2) after a reload 3) applies to iframes (local and OOPIFS? unless the feature is not applicable to iframes but in that case we can verify that response is properly handling it).
ASSERT_THAT(SendCommand("SmartCardEmulation.disable"), IsSuccess());I think most of assertions here should be converted to exceptions (EXPECT_*)?
return "";nit: Should there be IIFTT rules to make sure that new mapping are reflected here if the mojo interface changes?
Related details
- Patryk Chodur
- 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. |
Alex Rudenko (Gerrit)
Alex Rudenko added 1 comment![]( "Open in Gerrit")
session->CreateAndAddHandler<protocol::SmartCardEmulationHandler>();Paulina GacekMy understanding was that the emulation is meant to be global and not related to a specific render frame? has that changed? if it is still global, I think this should be in content/browser/devtools/browser_devtools_agent_host.cc
Alex RudenkoThank you for the comment! While Smart Card devices are physically global, the API implementation (`navigator.smartCard`) and the Mojo interface are bound to the specific RenderFrame.
Paulina GacekIs the emulation also global or would it be per render frame?
Per frame.
Done
Paulina Gacek (Gerrit)
Paulina Gacek added 3 comments![]( "Open in Gerrit")
Paulina GacekIt's usually way more practical so write CDP tests in JavaScript. Also, all your implementation is in content/ layer, so I assume it may be possible to test it in the content layer as well? Please see //third_party/blink/web_tests/http/tests/inspector-protocol for examples of content-side CDP tests. It may be possible to run these in chrome as well, the JS-side framework would be the same, but there's a tiny bit of boilerplate required on the C++ side, see chrome/browser/headless/headless_mode_protocol_browsertest.cc.
Andrey KosyakovThanks for the suggestion! The Smart Card API must be run within an Isolated Web App (IWA) context. I used IsolatedWebAppBrowserTestHarness (from chrome/) because it handles the complex IWA setup and permissions automatically. Porting this to the content/ layer or web_tests would require significant mocking of the IWA infrastructure.
Wouldn't this take care of it?
https://source.chromium.org/chromium/chromium/src/+/main:content/shell/common/shell_switches.h;l=45?(note we've got a virtual test suite setup to use this https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/virtual/isolated-context/README.md, https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/VirtualTestSuites;l=1748?)
@alexr...@chromium.org thanks for these test scenarios!
However, since this CL only implements the base enable/disable commands, adding these tests now triggers Mojo crashes because we can't yet gracefully resolve the establishContext calls they rely on. I implemented test scenarios (1) and (2) in the subsequent CL: 7361937.
ASSERT_THAT(SendCommand("SmartCardEmulation.disable"), IsSuccess());I think most of assertions here should be converted to exceptions (EXPECT_*)?
Done
nit: Should there be IIFTT rules to make sure that new mapping are reflected here if the mojo interface changes?
Thank you for the suggestion. I generally prefer not to add LINT tags to shared Mojo definitions if possible to keep them clean. Instead, default case was omitted to ensure a compile-time error if a new value is added to the enum.
Related details
- Alex Rudenko
- Andrey Kosyakov
- 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. |
Alex Rudenko (Gerrit)
Alex Rudenko added 1 comment![]( "Open in Gerrit")
Paulina GacekIt's usually way more practical so write CDP tests in JavaScript. Also, all your implementation is in content/ layer, so I assume it may be possible to test it in the content layer as well? Please see //third_party/blink/web_tests/http/tests/inspector-protocol for examples of content-side CDP tests. It may be possible to run these in chrome as well, the JS-side framework would be the same, but there's a tiny bit of boilerplate required on the C++ side, see chrome/browser/headless/headless_mode_protocol_browsertest.cc.
Andrey KosyakovThanks for the suggestion! The Smart Card API must be run within an Isolated Web App (IWA) context. I used IsolatedWebAppBrowserTestHarness (from chrome/) because it handles the complex IWA setup and permissions automatically. Porting this to the content/ layer or web_tests would require significant mocking of the IWA infrastructure.
Paulina GacekWouldn't this take care of it?
https://source.chromium.org/chromium/chromium/src/+/main:content/shell/common/shell_switches.h;l=45?(note we've got a virtual test suite setup to use this https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/virtual/isolated-context/README.md, https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/VirtualTestSuites;l=1748?)
@alexr...@chromium.org thanks for these test scenarios!
However, since this CL only implements the base enable/disable commands, adding these tests now triggers Mojo crashes because we can't yet gracefully resolve the establishContext calls they rely on. I implemented test scenarios (1) and (2) in the subsequent CL: 7361937.
Could you please clarify what is the consensus in the discussion with @ca...@chromium.org about the type of tests best suitable for this?
Related details
- Andrey Kosyakov
- 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. |