Update Page Info Chip for Suspicious Sites on Android [chromium/src : main]
Richard Chen (Gerrit)
Richard Chen added 4 comments![]( "Open in Gerrit")
Richard ChenPlease fix this WARNING reported by Trailing Whitespace: Please remove the trailing whitespace.
Done
Richard ChenPlease fix this WARNING reported by Trailing Whitespace: Please remove the trailing whitespace.
Done
if (details.contains("<link>")) {Richard Chennit: does this make the link clickable. Do we need a `ClickableSpan` here?
Done. When details contains `<link>`, `SpanApplier.applySpans` wraps the link text in a `ChromeClickableSpan` (which extends `ClickableSpan`) and attaches the `openSuspiciousSiteHelpCenter()` click listener:
Related details
- Awad Osman
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Awad Osman (Gerrit)
Awad Osman voted Code-Review+1![]( "Open in Gerrit")
- Richard Chen
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sinan Sahin (Gerrit)
Sinan Sahin added 1 comment![]( "Open in Gerrit")
Is this provided by a UX designer? [This icon](https://fonts.corp.google.com/icons?selected=Google+Symbols:public_off:FILL@0;ROND@50;wght@400;GRAD@0;opsz@24&icon.query=earth&icon.size=24&icon.color=%231f1f1f&icon.platform=web) is also available if we want to go with a standard material icon.
Related details
- Awad Osman
- Emily Stark
- Richard Chen
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. |
Richard Chen (Gerrit)
Richard Chen added 1 comment![]( "Open in Gerrit")
Is this provided by a UX designer? [This icon](https://fonts.corp.google.com/icons?selected=Google+Symbols:public_off:FILL@0;ROND@50;wght@400;GRAD@0;opsz@24&icon.query=earth&icon.size=24&icon.color=%231f1f1f&icon.platform=web) is also available if we want to go with a standard material icon.
- Emily Stark
- Sinan Sahin
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Emily Stark (Gerrit)
Emily Stark added 8 comments![]( "Open in Gerrit")
* Adds corresponding unit tests in `page_info_unittest.cc`.This file isn't touched in the CL; is this perhaps a typo for page_info_ui_unittest.cc, or did you forget to add unit tests in page_info_unittest.cc?
if (details.contains("<link>")) {
messageBuilder.append(
SpanApplier.applySpans(
details,
new SpanInfo(
"<link>",
"</link>",
new ChromeClickableSpan(
mRowView.getContext(),
(view) ->
mMainController
.openSafeBrowsingHelpCenter()))));
} else {
messageBuilder.append(details);
}This seems a bit brittle in that it's assuming any <link> in `details` should go to the Safe Browsing help center. Would it be possible to provide a link destination as a function argument or otherwise make sure that we're replacing <link>s with the correct destination?
private boolean mIsSuspiciousSite;Please add a comment explaining what this field is. Also might be helpful to explain why PageInfoController needs to know about this state specifically (i.e. why do we need `mIsSuspiciousSite` but not `mIsPhishingSite` or `mIsDangerousSite` etc.).
...
after reading further, maybe it would make more sense to save this as something about the favicon (`mUseSiteFavicon`) or something since it's specific to that use case?
if (mDialog == null || mIsSuspiciousSite) return;It'd be good to have a test for this favicon overriding behavior -- but I imagine it might be tricky to test if you need to control when the callback fires in the test. It might be okay to not have test coverage for this if it's really tricky, but maybe you could look into whether it's possible or not?
public void setSecurityDescription(String summary, String details, boolean isSuspiciousSite) {optional: I find it a bit weird that this method sets opaque strings and then also does this very suspicious-site-specific stuff. Maybe the suspicious site UI should be configured by a separate method that gets called separately? Ok to do in a follow-up CL though.
View wrapper = findViewById(R.id.page_info_connection_wrapper);nit: This is amplifying a pre-existing problem that the "connection" wrapper is not just used for connection information, but for Safe Browsing status as well. Would you mind filing a bug to clean this up?
Can you add tests to verify that clicking on the buttons and link work as expected?
It could be good to also test that the UMA metrics are recorded as expected though it'd be okay to do that in a follow-up CL IMO.
if (web_contents_->GetController().CanGoBack()) {
web_contents_->GetController().GoBack();What will happen if the user visits multiple pages on the same suspicious site? (For example, if a user visits https://suspicious[.]com and then clicks a link to another website on the same domain.) Will they see the warning on each page load? Will they see this UI in Page Info on the second page load and if so will "Back to Safety" take them back to suspicious[.]com?
Related details
- Richard Chen
- Sinan Sahin
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. |
Sinan Sahin (Gerrit)
Sinan Sahin voted and added 1 comment![]( "Open in Gerrit")
Related details
- Richard Chen
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. |