Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2894243002:
Add quiet safe browsing interstitials to chrome://interstitials for in browser testing (Closed)
|
Created:
3 years, 7 months ago by edwardjung Modified:
3 years, 7 months ago CC:
chromium-reviews, grt+watch_chromium.org, vakh+watch_chromium.org, timvolodine Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd quiet safe browsing interstitials to chrome://interstitials for in browser testing
BUG=716097
Review-Url: https://codereview.chromium.org/2894243002
Cr-Commit-Position: refs/heads/master@{#474671}
Committed: https://chromium.googlesource.com/chromium/src/+/6483de8b9dab69e6b7f83d5239bb8ee009425bf1
Patch Set 1 #
Total comments: 6
Patch Set 2 : Address comments #Patch Set 3 : Merge and fix display_options #
Messages
Total messages: 39 (25 generated)
edwardjung@chromium.org changed reviewers: + felt@chromium.org, nparker@chromium.org
PTAL at my initial implementation for getting the quiet interstitial on the
chrome://interstitial page
@felt:
c/b/ui/webui/interstitials/interstitial_ui.cc
c/b/ui/webui/interstitials/interstitial_ui_browsertest.cc
components/security_interstitials/core/browser/resources/interstitial_ui.html
@nparker:
c/b/safe_browsing/test_safe_browsing_blocking_page_quiet.cc
c/b/safe_browsing/test_safe_browsing_blocking_page_quiet.h
I actually wasn't sure if this test class should sit in safe_browsing or
webui/interstitials
Thanks.
nathan, what would you think if I made myself an OWNER in the safe_browsing dir specifically for the blocking pages? i review the rest of the code but then can't review those, and it comes up often.
Definitely, that s-g-t-m Adrienne.
two small q's https://codereview.chromium.org/2894243002/diff/1/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/test_safe_browsing_blocking_page_quiet.cc (right): https://codereview.chromium.org/2894243002/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/test_safe_browsing_blocking_page_quiet.cc:61: // blocking_page->Show(); commented out? https://codereview.chromium.org/2894243002/diff/1/chrome/browser/ui/webui/int... File chrome/browser/ui/webui/interstitials/interstitial_ui.cc (right): https://codereview.chromium.org/2894243002/diff/1/chrome/browser/ui/webui/int... chrome/browser/ui/webui/interstitials/interstitial_ui.cc:290: threat_type = safe_browsing::SB_THREAT_TYPE_URL_MALWARE; Is the giant the same for malware & phishing? https://codereview.chromium.org/2894243002/diff/1/chrome/browser/ui/webui/int... chrome/browser/ui/webui/interstitials/interstitial_ui.cc:428: if (html.length() == 0 && !base::StartsWith(path, "quietsafebrowsing", why do you need to add that check?
https://codereview.chromium.org/2894243002/diff/1/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/test_safe_browsing_blocking_page_quiet.cc (right): https://codereview.chromium.org/2894243002/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/test_safe_browsing_blocking_page_quiet.cc:61: // blocking_page->Show(); On 2017/05/20 00:00:06, felt wrote: > commented out? Removed. https://codereview.chromium.org/2894243002/diff/1/chrome/browser/ui/webui/int... File chrome/browser/ui/webui/interstitials/interstitial_ui.cc (right): https://codereview.chromium.org/2894243002/diff/1/chrome/browser/ui/webui/int... chrome/browser/ui/webui/interstitials/interstitial_ui.cc:290: threat_type = safe_browsing::SB_THREAT_TYPE_URL_MALWARE; On 2017/05/20 00:00:06, felt wrote: > Is the giant the same for malware & phishing? Yes. Only the icon is shown. Did you want to specify separate types in case this state changes in the future? https://codereview.chromium.org/2894243002/diff/1/chrome/browser/ui/webui/int... chrome/browser/ui/webui/interstitials/interstitial_ui.cc:428: if (html.length() == 0 && !base::StartsWith(path, "quietsafebrowsing", On 2017/05/20 00:00:06, felt wrote: > why do you need to add that check? For quiet Safe browsing the HTML is assigned above, but actually I see how this can be simplified. I'll refactor.
lgtm
The CQ bit was checked by edwardjung@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by edwardjung@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by edwardjung@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from felt@chromium.org Link to the patchset: https://codereview.chromium.org/2894243002/#ps40001 (title: "Merge and fix display_options")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
@Nathan, I also need a lgtm from you. Unfortunately the per file line felt added to the OWNERS doesn't cover my awkwardly named test_safe_browsing_blocking_page_quiet.* files. Thanks.
On 2017/05/23 16:46:07, edwardjung wrote: > @Nathan, I also need a lgtm from you. Unfortunately the per file line felt added > to the OWNERS doesn't cover my awkwardly named > test_safe_browsing_blocking_page_quiet.* files. > > Thanks. Ahhh, good point. Nathan can approve this one and I'll also fix the OWNERS file.
The CQ bit was checked by edwardjung@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by edwardjung@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by edwardjung@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by edwardjung@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1495729132068830,
"parent_rev": "f6319b29cab58d45f95c0a8f2388cf5650732870", "commit_rev":
"6483de8b9dab69e6b7f83d5239bb8ee009425bf1"}
Message was sent while issue was closed.
Description was changed from ========== Add quiet safe browsing interstitials to chrome://interstitials for in browser testing BUG=716097 ========== to ========== Add quiet safe browsing interstitials to chrome://interstitials for in browser testing BUG=716097 Review-Url: https://codereview.chromium.org/2894243002 Cr-Commit-Position: refs/heads/master@{#474671} Committed: https://chromium.googlesource.com/chromium/src/+/6483de8b9dab69e6b7f83d5239bb... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/6483de8b9dab69e6b7f83d5239bb... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
