Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(1)

Issue 2647323009: Add extended reporting level in the update request (Closed)

Created:
10 months, 2 weeks ago by vakh (use Gerrit instead)
Modified:
10 months, 2 weeks ago
CC:
woz, chromium-reviews, grt+watch_chromium.org, lpz, vakh+watch_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Add extended reporting level in the update request Step 1: In SafeBrowsingService, determine which extended reporting preference is set. It could be: OFF, LEGACY, SCOUT, BOTH. Step 2: ServicesDelegateImpl, proxies that method. Step 3: Pass a callback to this method to V4LocalDBM, which passes it to V4UpdateManager. Step 4: V4UpdateManager calls this callback method when sending a new update request. Note that this detection of current extended reporting level is approximate at best because: 1. We don't have a user profile that the PVer4 updates are tied to. Whereas, the extneded reporting level is tied to a profile so we report it as enabled if it is enabled for any of the profiles. 2. The preference can only be checked on the UI thread, whereas we query it on the task runner so there's a race condition there. That said, since this is only for reporting, and not for actually sending reports from user's machine, this approximation is acceptable. BUG=655802 Review-Url: https://codereview.chromium.org/2647323009 Cr-Commit-Position: refs/heads/master@{#446411} Committed: https://chromium.googlesource.com/chromium/src/+/0e143114a2eff79cd99bb289ce8956c8c01d675f

Patch Set 1 #

Total comments: 6

Patch Set 2 : Add tests for ERL #

Total comments: 2

Patch Set 3 : Jialiu's feedback #

Patch Set 4 : rebase #

Total comments: 2

Patch Set 5 : lpz@ feedback #

Total comments: 2

Patch Set 6 : rebase #

Unified diffs Side-by-side diffs Delta from patch set Stats (+171 lines, -42 lines) Patch
M chrome/browser/safe_browsing/safe_browsing_service.h View 1 2 3 4 3 chunks +14 lines, -0 lines 0 comments Download
M chrome/browser/safe_browsing/safe_browsing_service.cc View 1 2 3 4 4 chunks +15 lines, -11 lines 0 comments Download
M chrome/browser/safe_browsing/services_delegate_impl.h View 1 2 1 chunk +8 lines, -0 lines 0 comments Download
M chrome/browser/safe_browsing/services_delegate_impl.cc View 1 2 3 4 2 chunks +9 lines, -2 lines 0 comments Download
M components/safe_browsing_db/BUILD.gn View 1 chunk +1 line, -0 lines 0 comments Download
M components/safe_browsing_db/v4_local_database_manager.h View 1 2 3 3 chunks +9 lines, -2 lines 0 comments Download
M components/safe_browsing_db/v4_local_database_manager.cc View 1 2 3 2 chunks +12 lines, -6 lines 0 comments Download
M components/safe_browsing_db/v4_local_database_manager_unittest.cc View 6 chunks +23 lines, -8 lines 0 comments Download
M components/safe_browsing_db/v4_update_protocol_manager.h View 1 7 chunks +13 lines, -3 lines 0 comments Download
M components/safe_browsing_db/v4_update_protocol_manager.cc View 1 6 chunks +34 lines, -7 lines 0 comments Download
M components/safe_browsing_db/v4_update_protocol_manager_unittest.cc View 1 3 chunks +33 lines, -3 lines 0 comments Download

Messages

Total messages: 46 (31 generated)
vakh (use Gerrit instead)
10 months, 2 weeks ago (2017-01-25 00:15:20 UTC) #4
vakh (use Gerrit instead)
10 months, 2 weeks ago (2017-01-25 00:15:34 UTC) #5
vakh (use Gerrit instead)
Add tests for ERL
10 months, 2 weeks ago (2017-01-25 01:18:02 UTC) #8
Jialiu Lin
lgtm with some nits. https://codereview.chromium.org/2647323009/diff/1/chrome/browser/safe_browsing/safe_browsing_service.cc File chrome/browser/safe_browsing/safe_browsing_service.cc (right): https://codereview.chromium.org/2647323009/diff/1/chrome/browser/safe_browsing/safe_browsing_service.cc#newcode725 chrome/browser/safe_browsing/safe_browsing_service.cc:725: nit: extra empty line. https://codereview.chromium.org/2647323009/diff/1/chrome/browser/safe_browsing/services_delegate_impl.h ...
10 months, 2 weeks ago (2017-01-25 02:03:29 UTC) #11
vakh (use Gerrit instead)
Jialiu's feedback
10 months, 2 weeks ago (2017-01-25 02:13:35 UTC) #12
vakh (use Gerrit instead)
Thanks for the quick review. All done. https://codereview.chromium.org/2647323009/diff/1/chrome/browser/safe_browsing/safe_browsing_service.cc File chrome/browser/safe_browsing/safe_browsing_service.cc (right): https://codereview.chromium.org/2647323009/diff/1/chrome/browser/safe_browsing/safe_browsing_service.cc#newcode725 chrome/browser/safe_browsing/safe_browsing_service.cc:725: On 2017/01/25 ...
10 months, 2 weeks ago (2017-01-25 02:13:41 UTC) #14
vakh (use Gerrit instead)
rebase
10 months, 2 weeks ago (2017-01-25 02:45:15 UTC) #20
lpz
https://codereview.chromium.org/2647323009/diff/60001/chrome/browser/safe_browsing/safe_browsing_service.cc File chrome/browser/safe_browsing/safe_browsing_service.cc (right): https://codereview.chromium.org/2647323009/diff/60001/chrome/browser/safe_browsing/safe_browsing_service.cc#newcode727 chrome/browser/safe_browsing/safe_browsing_service.cc:727: maybe_enabled_extended_reporting_by_prefs_ = false; have you considered storing one field ...
10 months, 2 weeks ago (2017-01-25 16:24:47 UTC) #26
vakh (use Gerrit instead)
lpz@ feedback
10 months, 2 weeks ago (2017-01-25 22:33:23 UTC) #27
vakh (use Gerrit instead)
Thanks for the review. PTAL. https://codereview.chromium.org/2647323009/diff/60001/chrome/browser/safe_browsing/safe_browsing_service.cc File chrome/browser/safe_browsing/safe_browsing_service.cc (right): https://codereview.chromium.org/2647323009/diff/60001/chrome/browser/safe_browsing/safe_browsing_service.cc#newcode727 chrome/browser/safe_browsing/safe_browsing_service.cc:727: maybe_enabled_extended_reporting_by_prefs_ = false; On ...
10 months, 2 weeks ago (2017-01-25 22:35:30 UTC) #30
vakh (use Gerrit instead)
rebase
10 months, 2 weeks ago (2017-01-25 22:49:45 UTC) #33
lpz
lgtm https://codereview.chromium.org/2647323009/diff/80001/chrome/browser/safe_browsing/safe_browsing_service.cc File chrome/browser/safe_browsing/safe_browsing_service.cc (right): https://codereview.chromium.org/2647323009/diff/80001/chrome/browser/safe_browsing/safe_browsing_service.cc#newcode732 chrome/browser/safe_browsing/safe_browsing_service.cc:732: estimated_extended_reporting_by_prefs_ = erl; nit: would you perhaps care ...
10 months, 2 weeks ago (2017-01-26 16:36:46 UTC) #39
vakh (use Gerrit instead)
https://codereview.chromium.org/2647323009/diff/80001/chrome/browser/safe_browsing/safe_browsing_service.cc File chrome/browser/safe_browsing/safe_browsing_service.cc (right): https://codereview.chromium.org/2647323009/diff/80001/chrome/browser/safe_browsing/safe_browsing_service.cc#newcode732 chrome/browser/safe_browsing/safe_browsing_service.cc:732: estimated_extended_reporting_by_prefs_ = erl; On 2017/01/26 at 16:36:46, lpz wrote: ...
10 months, 2 weeks ago (2017-01-26 18:14:30 UTC) #40
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/2647323009/100001
10 months, 2 weeks ago (2017-01-26 18:15:25 UTC) #43
commit-bot: I haz the power
10 months, 2 weeks ago (2017-01-26 19:58:30 UTC) #46
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/0e143114a2eff79cd99bb289ce89...

Powered by Google App Engine
This is Rietveld 0eb02b776