[MagiChrome] Integrate SigninQRCodeModel and QR drawing in InfoBar [chromium/src : main]
Monica Salama (Gerrit)
Monica Salama added 1 comment![]( "Open in Gerrit")
Hi Nina,
PTAL on the CL, let me know if something looks wrong to you or if I missed an imprtant flow/use case, thanks!
FYI: I am working on parallel on design document but it is not fully polished, there is pressure to get this done quickly, that's why I am sending CLs although the design document was not reviewed. The implementation is based on the 1-1 converstaion we had.
Thanks!
Related details
- Nina Satragno
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. |
Monica Salama (Gerrit)
Monica Salama added 1 comment![]( "Open in Gerrit")
Hi Nina,
PTAL on the CL, let me know if something looks wrong to you or if I missed an imprtant flow/use case, thanks!
FYI: I am working on parallel on design document but it is not fully polished, there is pressure to get this done quickly, that's why I am sending CLs although the design document was not reviewed. The implementation is based on the 1-1 converstaion we had.
Thanks!
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. |
Monica Salama (Gerrit)
Monica Salama added 6 comments![]( "Open in Gerrit")
// Retrieve and observe the SigninQRCodeModel.this comment is not needed
// If the QR code is already cached, render it immediately.this one as well
signin_metrics::AccessPoint::kWebSignin,that shouldn't be possible user ProfileMenu or something similar
TEST_F(SigninQRCodeInfoBarDelegateTest, DestructionObservationSafety) {double check this test passess if it doesn't:
Move changes in this file to another CL on top of the stack (or on top of this one if changes are required for the CL currently on top od the stack) and ensure all tests passes
: qr_string_(qr_string), qr_size_(qr_size), qr_margin_(qr_margin) {this is 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. |
Monica Salama (Gerrit)
Monica Salama added 1 comment![]( "Open in Gerrit")
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. |
Monica Salama (Gerrit)
Monica Salama added 5 comments![]( "Open in Gerrit")
this comment is not needed
Done
// If the QR code is already cached, render it immediately.Monica Salamathis one as well
Done
const std::string qr_string_;Monica Salamanot used?
Done
: qr_string_(qr_string), qr_size_(qr_size), qr_margin_(qr_margin) {Monica Salamathis is not needed
Done
Related details
- Nina Satragno
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. |
Nina Satragno (Gerrit)
Nina Satragno added 4 comments![]( "Open in Gerrit")
Screenshot: https://screenshot.googleplex.com/api/85usPBPkTFZQ4zh.pngNote that this looks different to the latest mocks. To my untrained eye, we're missing the colour gradient, bold title, and there's too much space between the title and subtitle. If this is intended, please disregard this comment.
auto* model = SigninQRCodeModel::GetOrCreateForWebContents(active_contents);Usually we want to inject models into views to make writing tests easier instead of having the view retrieve them. The approach on the CL forces you to expose the webcontents on the delegate, too.
CHECK(model);This CHECK is redundant, it'll cause a safe crash if `model` is `nullptr`. But any attempt to derefence `model` will do the same. Remove it?
Profile* profile() const;style: if this is no longer an inlined simple accessor, rename it to GetProfile.
https://chromium.googlesource.com/chromium/src/+/main/styleguide/c++/c++.md#inline-functions
Related details
- David Roger
- Monica Salama
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. |
Monica Salama (Gerrit)
Monica Salama added 6 comments![]( "Open in Gerrit")
Screenshot: https://screenshot.googleplex.com/api/85usPBPkTFZQ4zh.pngNote that this looks different to the latest mocks. To my untrained eye, we're missing the colour gradient, bold title, and there's too much space between the title and subtitle. If this is intended, please disregard this comment.
This is the initial version, we might add/skip the gradient. Make more changes later.
I made few changes and updated the screenshot.
auto* model = SigninQRCodeModel::GetOrCreateForWebContents(active_contents);Usually we want to inject models into views to make writing tests easier instead of having the view retrieve them. The approach on the CL forces you to expose the webcontents on the delegate, too.
Done
This CHECK is redundant, it'll cause a safe crash if `model` is `nullptr`. But any attempt to derefence `model` will do the same. Remove it?
With model passed to the instructor, the CHECK makes the expectation clear.
I am not a fan of derefrencing an null pointer.
style: if this is no longer an inlined simple accessor, rename it to GetProfile.
https://chromium.googlesource.com/chromium/src/+/main/styleguide/c++/c++.md#inline-functions
Done
that shouldn't be possible user ProfileMenu or something similar
Done
TEST_F(SigninQRCodeInfoBarDelegateTest, DestructionObservationSafety) {double check this test passess if it doesn't:
Move changes in this file to another CL on top of the stack (or on top of this one if changes are required for the CL currently on top od the stack) and ensure all tests passes
- Nina Satragno
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. |
Monica Salama (Gerrit)
Monica Salama added 1 comment![]( "Open in Gerrit")
Monica SalamaHi Nina,
PTAL on the CL, let me know if something looks wrong to you or if I missed an imprtant flow/use case, thanks!
FYI: I am working on parallel on design document but it is not fully polished, there is pressure to get this done quickly, that's why I am sending CLs although the design document was not reviewed. The implementation is based on the 1-1 converstaion we had.
Thanks!
go/magichrome-passkey-qr-banner
Done
Related details
- Nina Satragno
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nina Satragno (Gerrit)
Nina Satragno voted and added 2 comments![]( "Open in Gerrit")
Votes added by Nina Satragno
| Code-Review | +1 |
2 comments
#include "content/public/browser/web_contents.h"I think you don't need this anymore.
Related details
- Monica Salama
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. |
David Roger (Gerrit)
David Roger voted and added 1 comment![]( "Open in Gerrit")
Votes added by David Roger
| Code-Review | +1 |
1 comment
static constexpr int kDefaultQrCodeMargin = 40;optional: I wonder if we should remove these constants from this file, and also make the qr_size and qr_margin parameters non-optional.
It looks like only one caller would need to be updated.
Monica Salama (Gerrit)
Monica Salama voted and added 2 comments![]( "Open in Gerrit")
Votes added by Monica Salama
| Commit-Queue | +2 |
2 comments
Thanks for the review!
static constexpr int kDefaultQrCodeMargin = 40;optional: I wonder if we should remove these constants from this file, and also make the qr_size and qr_margin parameters non-optional.
It looks like only one caller would need to be updated.
I think it makes sense to have the default Webauthn values. I will re-consider in a follow-up CL. Thanks!
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. |
Monica Salama (Gerrit)
Monica Salama voted and added 1 comment![]( "Open in Gerrit")
Votes added by Monica Salama
| Commit-Queue | +2 |
1 comment
I think you don't need this anymore.
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. |