Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1984803002:
Separate phishing/SE ads/ SE landing interstitial UMA stats (Closed)
|
Created:
4 years, 7 months ago by Jialiu Lin Modified:
4 years, 7 months ago CC:
chromium-reviews, grt+watch_chromium.org, asvitkine+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUpdate UMA stats to help calculate CTRs of phishing, SE ads and SE landing interstitials separately
BUG=603174
Committed: https://crrev.com/e7f203226b19ae0be9823387124fbf317d7645c8
Cr-Commit-Position: refs/heads/master@{#394487}
Patch Set 1 #Patch Set 2 : compatible with default cases #
Total comments: 4
Patch Set 3 : nit #
Messages
Total messages: 21 (12 generated)
Description was changed from ========== Separate phishing/social engineering ads/ social engineering landing interstitial UMA stats BUG=603174 ========== to ========== Separate phishing/SE ads/ SE landing interstitial UMA stats BUG=603174 ==========
Description was changed from ========== Separate phishing/SE ads/ SE landing interstitial UMA stats BUG=603174 ========== to ========== Update UMA stats to separate CTRs of phishing, SE ads and SE landing interstitial BUG=603174 ==========
Description was changed from ========== Update UMA stats to separate CTRs of phishing, SE ads and SE landing interstitial BUG=603174 ========== to ========== Update UMA stats to help calculate CTRs of phishing, SE ads and SE landing interstitials separately BUG=603174 ==========
jialiul@chromium.org changed reviewers: + nparker@chromium.org
Hi nparker@, This CL use ThreatMetadata to separate interstitial related UMA stats of phishing, SE ads and SE landing. PTAL. Thanks!
lgtm https://codereview.chromium.org/1984803002/diff/20001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/safe_browsing_blocking_page.cc (right): https://codereview.chromium.org/1984803002/diff/20001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:559: return std::string(); Maybe this should return "unknown_interstitial_type" or some readable string. That way if SB starts sending weird enums we don't (yet) recognize, we'll at least see the issue in UMA. https://codereview.chromium.org/1984803002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/1984803002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:94777: <suffix name="phishing_subresource"/> This is splitting phishing* into three (six, really) subtypes. Add that to the description so people will understand why the value dropped in M52.
jialiul@chromium.org changed reviewers: + holte@google.com
Thanks, nparker@! your comments are all addressed. +holte@ to review updates in histograms.xml Thanks! https://codereview.chromium.org/1984803002/diff/20001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/safe_browsing_blocking_page.cc (right): https://codereview.chromium.org/1984803002/diff/20001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:559: return std::string(); On 2016/05/17 19:23:58, Nathan Parker wrote: > Maybe this should return "unknown_interstitial_type" or some readable string. > That way if SB starts sending weird enums we don't (yet) recognize, we'll at > least see the issue in UMA. Changed from std::string() to "unkown_metric_prefix". Though.. this function returns part of the UMA name, not a enum. So unless we declare a group of interstitial.unkown_metric_prefix.* uma stats in histograms.xml, it will not help in identifying issues. And this return is guarded by the NOTREACHED() statement before it, should not get here anyway. https://codereview.chromium.org/1984803002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/1984803002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:94777: <suffix name="phishing_subresource"/> On 2016/05/17 19:23:58, Nathan Parker wrote: > This is splitting phishing* into three (six, really) subtypes. Add that to the > description so people will understand why the value dropped in M52. Done.
holte@chromium.org changed reviewers: + holte@chromium.org
lgtm
The CQ bit was checked by jialiul@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nparker@chromium.org Link to the patchset: https://codereview.chromium.org/1984803002/#ps40001 (title: "nit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1984803002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1984803002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by jialiul@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1984803002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1984803002/40001
Message was sent while issue was closed.
Description was changed from ========== Update UMA stats to help calculate CTRs of phishing, SE ads and SE landing interstitials separately BUG=603174 ========== to ========== Update UMA stats to help calculate CTRs of phishing, SE ads and SE landing interstitials separately BUG=603174 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Update UMA stats to help calculate CTRs of phishing, SE ads and SE landing interstitials separately BUG=603174 ========== to ========== Update UMA stats to help calculate CTRs of phishing, SE ads and SE landing interstitials separately BUG=603174 Committed: https://crrev.com/e7f203226b19ae0be9823387124fbf317d7645c8 Cr-Commit-Position: refs/heads/master@{#394487} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/e7f203226b19ae0be9823387124fbf317d7645c8 Cr-Commit-Position: refs/heads/master@{#394487} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
