Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2678253002:
Add metrics for Safe Browsing blacklist response. (Closed)
|
Created:
3 years, 10 months ago by meredithl Modified:
3 years, 10 months ago CC:
chromium-reviews, mlamouri+watch-permissions_chromium.org, asvitkine+watch_chromium.org, raymes+watch_chromium.org, chrome-apps-syd-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd metrics for Safe Browsing blacklist response.
Implements tracking of the response received from Safe Browsing when
querying the API blacklist, and the response time. Both metrics are
recorded in the PermissionBlacklistClient and implemented in
PermissionUmaUtil.
BUG=679877
Review-Url: https://codereview.chromium.org/2678253002
Cr-Commit-Position: refs/heads/master@{#449489}
Committed: https://chromium.googlesource.com/chromium/src/+/0cbf28a038b56406d8a8e2b4d6d70368b094e515
Patch Set 1 #Patch Set 2 : Rebase #Patch Set 3 : Fixing diff. #
Total comments: 12
Patch Set 4 : Header cleanup, review. #
Total comments: 10
Patch Set 5 : Histogram sanity check. #
Total comments: 4
Patch Set 6 : Nits and review #Patch Set 7 : Rebase #
Messages
Total messages: 37 (22 generated)
meredithl@google.com changed reviewers: + dominickn@chromium.org, raymes@chromium.org
Hey guys, PTAL. Thanks!
https://codereview.chromium.org/2678253002/diff/40001/chrome/browser/permissi... File chrome/browser/permissions/permission_blacklist_client.cc (right): https://codereview.chromium.org/2678253002/diff/40001/chrome/browser/permissi... chrome/browser/permissions/permission_blacklist_client.cc:14: #include "chrome/browser/permissions/permission_util.h" Not sure this is needed? https://codereview.chromium.org/2678253002/diff/40001/chrome/browser/permissi... File chrome/browser/permissions/permission_blacklist_client.h (right): https://codereview.chromium.org/2678253002/diff/40001/chrome/browser/permissi... chrome/browser/permissions/permission_blacklist_client.h:12: #include "chrome/browser/permissions/permission_uma_util.h" Move this include to the cc file? https://codereview.chromium.org/2678253002/diff/40001/chrome/browser/permissi... File chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc (right): https://codereview.chromium.org/2678253002/diff/40001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc:15: #include "chrome/browser/permissions/permission_uma_util.h" Not sure if this is used? https://codereview.chromium.org/2678253002/diff/40001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc:320: base::HistogramTester histograms; This is currently unused? https://codereview.chromium.org/2678253002/diff/40001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc:414: histograms.ExpectTotalCount("Permissions.AutoBlocker.SafeBrowsingResponse", Can you ExpectBucketCount the specific bucket that you expect to be added here?
Thanks! https://codereview.chromium.org/2678253002/diff/40001/chrome/browser/permissi... File chrome/browser/permissions/permission_blacklist_client.cc (right): https://codereview.chromium.org/2678253002/diff/40001/chrome/browser/permissi... chrome/browser/permissions/permission_blacklist_client.cc:14: #include "chrome/browser/permissions/permission_util.h" On 2017/02/07 05:36:54, dominickn wrote: > Not sure this is needed? It is used to convert between the PermissionType and the Safe Browsing name. https://codereview.chromium.org/2678253002/diff/40001/chrome/browser/permissi... File chrome/browser/permissions/permission_blacklist_client.h (right): https://codereview.chromium.org/2678253002/diff/40001/chrome/browser/permissi... chrome/browser/permissions/permission_blacklist_client.h:12: #include "chrome/browser/permissions/permission_uma_util.h" On 2017/02/07 05:36:54, dominickn wrote: > Move this include to the cc file? Done. https://codereview.chromium.org/2678253002/diff/40001/chrome/browser/permissi... File chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc (right): https://codereview.chromium.org/2678253002/diff/40001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc:15: #include "chrome/browser/permissions/permission_uma_util.h" On 2017/02/07 05:36:54, dominickn wrote: > Not sure if this is used? The enums for SafeBrowsingResponse are defined in permission_uma_util.h. https://codereview.chromium.org/2678253002/diff/40001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc:320: base::HistogramTester histograms; On 2017/02/07 05:36:54, dominickn wrote: > This is currently unused? Done. https://codereview.chromium.org/2678253002/diff/40001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc:414: histograms.ExpectTotalCount("Permissions.AutoBlocker.SafeBrowsingResponse", On 2017/02/07 05:36:54, dominickn wrote: > Can you ExpectBucketCount the specific bucket that you expect to be added here? Done.
Just a couple more histogram tests for sanity. https://codereview.chromium.org/2678253002/diff/40001/chrome/browser/permissi... File chrome/browser/permissions/permission_blacklist_client.cc (right): https://codereview.chromium.org/2678253002/diff/40001/chrome/browser/permissi... chrome/browser/permissions/permission_blacklist_client.cc:14: #include "chrome/browser/permissions/permission_util.h" On 2017/02/07 06:27:29, meredithl wrote: > On 2017/02/07 05:36:54, dominickn wrote: > > Not sure this is needed? > > It is used to convert between the PermissionType and the Safe Browsing name. Acknowledged. https://codereview.chromium.org/2678253002/diff/40001/chrome/browser/permissi... File chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc (right): https://codereview.chromium.org/2678253002/diff/40001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc:15: #include "chrome/browser/permissions/permission_uma_util.h" On 2017/02/07 06:27:29, meredithl wrote: > On 2017/02/07 05:36:54, dominickn wrote: > > Not sure if this is used? > > The enums for SafeBrowsingResponse are defined in permission_uma_util.h. Acknowledged. https://codereview.chromium.org/2678253002/diff/60001/chrome/browser/permissi... File chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc (right): https://codereview.chromium.org/2678253002/diff/60001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc:335: autoblocker()->IsUnderEmbargo(content::PermissionType::GEOLOCATION, url)); Just as a sanity check, can you add: histograms.ExpectTotalCount("Permissions.AutoBlocker.SafeBrowsingResponse", 0); histograms.ExpectTotalCount("Permissions.AutoBlocker.SafeBrowsingResponseTime", 0); https://codereview.chromium.org/2678253002/diff/60001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc:359: autoblocker()->IsUnderEmbargo(content::PermissionType::GEOLOCATION, url)); And here too. https://codereview.chromium.org/2678253002/diff/60001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc:406: db_manager->SetPerformCallback(true); Add: histograms.ExpectTotalCount("Permissions.AutoBlocker.SafeBrowsingResponseTime", 1); https://codereview.chromium.org/2678253002/diff/60001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc:413: histograms.ExpectTotalCount("Permissions.AutoBlocker.SafeBrowsingResponse", Add: histograms.ExpectTotalCount("Permissions.AutoBlocker.SafeBrowsingResponseTime", 2); https://codereview.chromium.org/2678253002/diff/60001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc:509: SafeBrowsingResponse::NOT_BLACKLISTED, 1); Add: histograms.ExpectTotalCount("Permissions.AutoBlocker.SafeBrowsingResponseTime", 1);
The CQ bit was checked by meredithl@google.com to run a CQ dry run
https://codereview.chromium.org/2678253002/diff/60001/chrome/browser/permissi... File chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc (right): https://codereview.chromium.org/2678253002/diff/60001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc:335: autoblocker()->IsUnderEmbargo(content::PermissionType::GEOLOCATION, url)); On 2017/02/07 22:28:25, dominickn wrote: > Just as a sanity check, can you add: > > histograms.ExpectTotalCount("Permissions.AutoBlocker.SafeBrowsingResponse", 0); > histograms.ExpectTotalCount("Permissions.AutoBlocker.SafeBrowsingResponseTime", > 0); Done. https://codereview.chromium.org/2678253002/diff/60001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc:359: autoblocker()->IsUnderEmbargo(content::PermissionType::GEOLOCATION, url)); On 2017/02/07 22:28:25, dominickn wrote: > And here too. Done. https://codereview.chromium.org/2678253002/diff/60001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc:406: db_manager->SetPerformCallback(true); On 2017/02/07 22:28:25, dominickn wrote: > Add: > > histograms.ExpectTotalCount("Permissions.AutoBlocker.SafeBrowsingResponseTime", > 1); Done. https://codereview.chromium.org/2678253002/diff/60001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc:413: histograms.ExpectTotalCount("Permissions.AutoBlocker.SafeBrowsingResponse", On 2017/02/07 22:28:25, dominickn wrote: > Add: > > histograms.ExpectTotalCount("Permissions.AutoBlocker.SafeBrowsingResponseTime", > 2); Done. https://codereview.chromium.org/2678253002/diff/60001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc:509: SafeBrowsingResponse::NOT_BLACKLISTED, 1); On 2017/02/07 22:28:25, dominickn wrote: > Add: > > histograms.ExpectTotalCount("Permissions.AutoBlocker.SafeBrowsingResponseTime", > 1); Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm, thanks!
lgtm https://codereview.chromium.org/2678253002/diff/80001/chrome/browser/permissi... File chrome/browser/permissions/permission_blacklist_client.cc (right): https://codereview.chromium.org/2678253002/diff/80001/chrome/browser/permissi... chrome/browser/permissions/permission_blacklist_client.cc:15: #include "chrome/browser/permissions/permission_util.h" nit: is this needed? https://codereview.chromium.org/2678253002/diff/80001/chrome/browser/permissi... File chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc (right): https://codereview.chromium.org/2678253002/diff/80001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc:507: TEST_F(PermissionDecisionAutoBlockerUnitTest, TestSafeBrowsingResponse) { nit: maybe this should just be called TestRequestNotEmbargoed (or something similar)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by meredithl@google.com to run a CQ dry run
https://codereview.chromium.org/2678253002/diff/80001/chrome/browser/permissi... File chrome/browser/permissions/permission_blacklist_client.cc (right): https://codereview.chromium.org/2678253002/diff/80001/chrome/browser/permissi... chrome/browser/permissions/permission_blacklist_client.cc:15: #include "chrome/browser/permissions/permission_util.h" On 2017/02/08 02:08:00, raymes wrote: > nit: is this needed? It is used to convert between the PermissionType to the Safe Browsing string. https://codereview.chromium.org/2678253002/diff/80001/chrome/browser/permissi... File chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc (right): https://codereview.chromium.org/2678253002/diff/80001/chrome/browser/permissi... chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc:507: TEST_F(PermissionDecisionAutoBlockerUnitTest, TestSafeBrowsingResponse) { On 2017/02/08 02:08:00, raymes wrote: > nit: maybe this should just be called TestRequestNotEmbargoed (or something > similar) Done. I've also moved it upwards to underneath TestUpdateEmbargoBlacklisted, which is basically the reverse case of this so logically the tests make a bit more sense.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
meredithl@google.com changed reviewers: + isherman@chromium.org
Hi, isherman. PTAL, thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
meredithl@google.com changed reviewers: + rkaplow@chromium.org - isherman@chromium.org
Removing Ilya as a reviewer, who is away. Robert, if you could PTAL that would be great, thanks!
lgtm
The CQ bit was checked by meredithl@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from dominickn@chromium.org, raymes@chromium.org Link to the patchset: https://codereview.chromium.org/2678253002/#ps100001 (title: "Nits and review")
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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by meredithl@google.com 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 meredithl@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from dominickn@chromium.org, raymes@chromium.org, rkaplow@chromium.org Link to the patchset: https://codereview.chromium.org/2678253002/#ps120001 (title: "Rebase")
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": 120001, "attempt_start_ts": 1486686459125360,
"parent_rev": "2914d0c127500fda119c8f3476334537b5f5b4fd", "commit_rev":
"0cbf28a038b56406d8a8e2b4d6d70368b094e515"}
Message was sent while issue was closed.
Description was changed from ========== Add metrics for Safe Browsing blacklist response. Implements tracking of the response received from Safe Browsing when querying the API blacklist, and the response time. Both metrics are recorded in the PermissionBlacklistClient and implemented in PermissionUmaUtil. BUG=679877 ========== to ========== Add metrics for Safe Browsing blacklist response. Implements tracking of the response received from Safe Browsing when querying the API blacklist, and the response time. Both metrics are recorded in the PermissionBlacklistClient and implemented in PermissionUmaUtil. BUG=679877 Review-Url: https://codereview.chromium.org/2678253002 Cr-Commit-Position: refs/heads/master@{#449489} Committed: https://chromium.googlesource.com/chromium/src/+/0cbf28a038b56406d8a8e2b4d6d7... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/0cbf28a038b56406d8a8e2b4d6d7... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
