[iOS][PageInfo] Add certificate detail model, mediator, utils, and fix OID crash [chromium/src : main]
Tian Ye (Gerrit)
Tian Ye added 3 comments![]( "Open in Gerrit")
NSString* FormatTime(const base::Time& time) {Tian Ye`base::Time` is a wrapper around an integral value. It is better to pass it by value than by reference. So
```
NSString* FormatTime(base::Time time) {
...
```
Done
- (PageInfoSiteCertificateDetailInfo*)certificateDetailInfoFromX509Model:Tian YeI looks like this method and all the helper methods don't use `self`. I think it would be better to change them to free functions.
Additionally, since they are complex, I would move them to a new file, make them public and test test individually.
So `page_info_site_certificate_chain_utils.h` file that contains functions like this:
```
// Creates a PageInfoSiteCertificateDetailInfo from an X509CertificateModel.
PageInfoSiteCertificateDetailInfo*
CertificateDetailInfoFromX509Model(const X509CertificateModel& model);// Creates a PageInfoCertificateSignatureInfo from an X509CertificateModel.
PageInfoCertificateSignatureInfo*
CertificateSignatureInfoFromX509Model(const X509CertificateModel& model);...
```
Done
// Signature info.Tian YeI don't think those comments bring any added value. This is self-explanatory code as the variable and methods names are explicitly. Please remove them.
Done
Related details
- Filipa Senra
- Sylvain Defresne
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Filipa Senra (Gerrit)
Filipa Senra voted and added 4 comments![]( "Open in Gerrit")
Votes added by Filipa Senra
| Code-Review | +1 |
4 comments
if (!_webState) {optional nit: you can probably combine the two ifs.
PageInfoCertificateSignatureInfo* SignatureInfoFromModel(nit: it's a bit unfortunate that all the functions bellow are on the header file just for testing purposes. would it be possible to still test this functionality, without putting them there? could you instead use CertificateDetailInfoFromX509Model and check for a specific field?
// then intermediates). If this method is not called, the certificate row shouldnit: or the array is empty?
Related details
- Sylvain Defresne
- Tian Ye
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. |
Tian Ye (Gerrit)
Tian Ye added 3 comments![]( "Open in Gerrit")
optional nit: you can probably combine the two ifs.
Done
PageInfoCertificateSignatureInfo* SignatureInfoFromModel(nit: it's a bit unfortunate that all the functions bellow are on the header file just for testing purposes. would it be possible to still test this functionality, without putting them there? could you instead use CertificateDetailInfoFromX509Model and check for a specific field?
Done
// then intermediates). If this method is not called, the certificate row shouldnit: or the array is empty?
- Filipa Senra
- Sylvain Defresne
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sylvain Defresne (Gerrit)
Sylvain Defresne voted and added 2 comments![]( "Open in Gerrit")
Votes added by Sylvain Defresne
| Code-Review | +1 |
2 comments
lgtm once the comment has been adressed
if (!self.consumer || !_webState) {I think you should also test whether `WebState` is realized or not. The reason is that unrealized `WebState` does not have a `NavigationManager`, but calling `GetNavigationManager()` on it will cause the object to be created, and we want to avoid forcing realization of `WebState`s.
So
```
if (!self.consumer || !_webState || !_webState->IsRealized()) {
return;
}
```
Related details
- Filipa Senra
- Tian Ye
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. |
Filipa Senra (Gerrit)
Filipa Senra voted Code-Review+1![]( "Open in Gerrit")
- Tian Ye
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. |
Tian Ye (Gerrit)
Tian Ye added 1 comment![]( "Open in Gerrit")
I think you should also test whether `WebState` is realized or not. The reason is that unrealized `WebState` does not have a `NavigationManager`, but calling `GetNavigationManager()` on it will cause the object to be created, and we want to avoid forcing realization of `WebState`s.
So
```
if (!self.consumer || !_webState || !_webState->IsRealized()) {
return;
}
```
- Filipa Senra
- Sylvain Defresne
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Matt Mueller (Gerrit)
Matt Mueller voted and added 1 comment![]( "Open in Gerrit")
Votes added by Matt Mueller
| Code-Review | -1 |
1 comment
please address my comment on earlier CL before landing
Related details
- Filipa Senra
- Sylvain Defresne
- Tian Ye
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. |
Tian Ye (Gerrit)
Tian Ye abandoned this change.
Tian Ye abandoned this change![]( "Open in Gerrit")
Related details
Code-Coverage
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |