Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(494)

Issue 2538483002: Add management related code to SafeBrowsingNavigationObserverManager (Closed)

Created:
4 years ago by Jialiu Lin
Modified:
4 years ago
CC:
asvitkine+watch_chromium.org, chromium-reviews, grt+watch_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Add management related code to SafeBrowsingNavigationObserverManager class: (1) Schedule clean up stale navigation footprint (2) Backtrace navigation events up to 2 user gesture for a given download, and add this info into ClientDownloadRequest proto. (3) Add UMA metrices to track attribution results BUG=639467 Committed: https://crrev.com/4e2ebcc9087c02598a638aa86cba21c043069b76 Cr-Commit-Position: refs/heads/master@{#438745}

Patch Set 1 #

Patch Set 2 : rebase update #

Patch Set 3 : rebase-update #

Patch Set 4 : filter out browser side navigation browser tests #

Total comments: 51

Patch Set 5 : address nparker's comments #

Patch Set 6 : rebase-update #

Total comments: 82

Patch Set 7 : address comments from creis@ and holte@ + rebase #

Total comments: 12

Patch Set 8 : address nits #

Unified diffs Side-by-side diffs Delta from patch set Stats (+1445 lines, -157 lines) Patch
M chrome/browser/safe_browsing/safe_browsing_navigation_observer.cc View 1 2 3 4 5 6 2 chunks +8 lines, -6 lines 0 comments Download
M chrome/browser/safe_browsing/safe_browsing_navigation_observer_browsertest.cc View 1 2 3 4 5 6 27 chunks +827 lines, -104 lines 0 comments Download
M chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.h View 1 2 3 4 5 6 7 6 chunks +80 lines, -2 lines 0 comments Download
M chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.cc View 1 2 3 4 5 6 7 5 chunks +244 lines, -9 lines 0 comments Download
M chrome/browser/safe_browsing/safe_browsing_navigation_observer_unittest.cc View 1 2 3 4 5 6 3 chunks +131 lines, -2 lines 0 comments Download
M chrome/common/safe_browsing/csd.proto View 1 2 3 4 5 6 1 chunk +27 lines, -31 lines 0 comments Download
A chrome/test/data/safe_browsing/download_protection/navigation_observer/landing.html View 1 2 3 4 5 6 7 1 chunk +19 lines, -0 lines 0 comments Download
A chrome/test/data/safe_browsing/download_protection/navigation_observer/landing_referrer.html View 1 2 3 4 5 6 7 1 chunk +17 lines, -0 lines 0 comments Download
M chrome/test/data/safe_browsing/download_protection/navigation_observer/navigation_observer_tests.html View 1 2 3 4 3 chunks +11 lines, -3 lines 0 comments Download
A chrome/test/data/safe_browsing/download_protection/navigation_observer/page_before_landing_referrer.html View 1 2 3 4 5 6 1 chunk +19 lines, -0 lines 0 comments Download
A chrome/test/data/safe_browsing/download_protection/navigation_observer/redirect_to_landing.html View 1 2 3 4 5 6 7 1 chunk +8 lines, -0 lines 0 comments Download
M testing/buildbot/filters/browser-side-navigation.linux.browser_tests.filter View 1 2 3 4 5 6 7 1 chunk +6 lines, -0 lines 0 comments Download
M tools/metrics/histograms/histograms.xml View 1 2 3 4 5 6 7 3 chunks +48 lines, -0 lines 0 comments Download

Dependent Patchsets:

Messages

Total messages: 70 (46 generated)
Jialiu Lin
Hi nparker@, Here is the second part of SafeBrowsingNavigationObserverManager code. PTAL. Thanks!
4 years ago (2016-11-28 23:21:32 UTC) #8
Nathan Parker
Sorry for the delay -- I shoulda been on this earlier. https://codereview.chromium.org/2538483002/diff/60001/chrome/browser/safe_browsing/safe_browsing_navigation_observer_browsertest.cc File chrome/browser/safe_browsing/safe_browsing_navigation_observer_browsertest.cc (right): ...
4 years ago (2016-12-05 22:21:24 UTC) #19
Jialiu Lin
Thanks, nparker@! Let me know if you have further comments. https://codereview.chromium.org/2538483002/diff/60001/chrome/browser/safe_browsing/safe_browsing_navigation_observer_browsertest.cc File chrome/browser/safe_browsing/safe_browsing_navigation_observer_browsertest.cc (right): https://codereview.chromium.org/2538483002/diff/60001/chrome/browser/safe_browsing/safe_browsing_navigation_observer_browsertest.cc#newcode198 ...
4 years ago (2016-12-06 23:10:07 UTC) #21
Jialiu Lin
Hi creis@, Here is the follow-up CL of SafeBrowsingNavigationObserverManager. Let me know if you have ...
4 years ago (2016-12-06 23:12:09 UTC) #24
Nathan Parker
LGTM from my point of view. Handing off to you, Charlie. https://codereview.chromium.org/2538483002/diff/60001/chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.h File chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.h (right): ...
4 years ago (2016-12-06 23:31:32 UTC) #27
Jialiu Lin
+holte@chromium.org for histograms.xml
4 years ago (2016-12-08 19:01:30 UTC) #33
Charlie Reis
Sure, I'll look soon-- I was OOO for a team offsite, but catching up now.
4 years ago (2016-12-08 22:18:22 UTC) #36
Charlie Reis
Thanks--generally looks in line with what we discussed. I continue to be a bit concerned ...
4 years ago (2016-12-09 22:00:24 UTC) #37
Steven Holte
https://codereview.chromium.org/2538483002/diff/100001/tools/metrics/histograms/histograms.xml File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2538483002/diff/100001/tools/metrics/histograms/histograms.xml#newcode53356 tools/metrics/histograms/histograms.xml:53356: + units="SafeBrowsingAttributionResultTypes"> units -> enum https://codereview.chromium.org/2538483002/diff/100001/tools/metrics/histograms/histograms.xml#newcode53377 tools/metrics/histograms/histograms.xml:53377: + The ...
4 years ago (2016-12-10 00:33:30 UTC) #38
Jialiu Lin
Thank you, holte@ and creis@. I've addressed all your comments. creis@, yes, I'll do a ...
4 years ago (2016-12-12 23:43:39 UTC) #46
Steven Holte
histograms lgtm
4 years ago (2016-12-13 01:41:29 UTC) #47
Jialiu Lin
creis@, any further comments?
4 years ago (2016-12-14 02:27:40 UTC) #50
Charlie Reis
[+clamy@] Thanks! Basically looks good now, but I'd like to check with clamy@ about whether ...
4 years ago (2016-12-14 07:43:58 UTC) #52
clamy
On 2016/12/14 07:43:58, Charlie Reis wrote: > [+clamy@] > > Thanks! Basically looks good now, ...
4 years ago (2016-12-14 15:53:31 UTC) #53
clamy
On 2016/12/14 07:43:58, Charlie Reis wrote: > [+clamy@] > > Thanks! Basically looks good now, ...
4 years ago (2016-12-14 15:53:32 UTC) #54
Jialiu Lin
On 2016/12/14 15:53:32, clamy wrote: > On 2016/12/14 07:43:58, Charlie Reis wrote: > > [+clamy@] ...
4 years ago (2016-12-14 19:01:13 UTC) #55
Jialiu Lin
Thanks creis@! https://codereview.chromium.org/2538483002/diff/100001/chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.cc File chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.cc (right): https://codereview.chromium.org/2538483002/diff/100001/chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.cc#newcode138 chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.cc:138: void SafeBrowsingNavigationObserverManager::CleanUpStaleNavigationFootprints() { On 2016/12/14 07:43:57, Charlie ...
4 years ago (2016-12-14 19:06:29 UTC) #56
Charlie Reis
Thanks! LGTM with a few thoughts below. https://codereview.chromium.org/2538483002/diff/100001/chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.cc File chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.cc (right): https://codereview.chromium.org/2538483002/diff/100001/chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.cc#newcode138 chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.cc:138: void SafeBrowsingNavigationObserverManager::CleanUpStaleNavigationFootprints() ...
4 years ago (2016-12-15 00:38:33 UTC) #57
Jialiu Lin
On 2016/12/15 00:38:33, Charlie Reis (OOO soon) wrote: > Thanks! LGTM with a few thoughts ...
4 years ago (2016-12-15 00:47:32 UTC) #58
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2538483002/160001
4 years ago (2016-12-15 01:05:58 UTC) #61
commit-bot: I haz the power
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_ng/builds/350364) win_clang on master.tryserver.chromium.win (JOB_FAILED, ...
4 years ago (2016-12-15 01:12:10 UTC) #63
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2538483002/160001
4 years ago (2016-12-15 03:48:11 UTC) #65
commit-bot: I haz the power
Committed patchset #8 (id:160001)
4 years ago (2016-12-15 04:42:18 UTC) #68
commit-bot: I haz the power
4 years ago (2016-12-15 04:44:40 UTC) #70
Message was sent while issue was closed.
Patchset 8 (id:??) landed as
https://crrev.com/4e2ebcc9087c02598a638aa86cba21c043069b76
Cr-Commit-Position: refs/heads/master@{#438745}

Powered by Google App Engine
This is Rietveld 408576698