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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
5 months ago by vakh (use Gerrit instead)
Modified:
5 months 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
Commit queue not available (can’t edit this change).

Messages

Total messages: 46 (31 generated)
vakh (use Gerrit instead)
5 months ago (2017-01-25 00:15:20 UTC) #4
vakh (use Gerrit instead)
5 months ago (2017-01-25 00:15:34 UTC) #5
vakh (use Gerrit instead)
Add tests for ERL
5 months 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 ...
5 months ago (2017-01-25 02:03:29 UTC) #11
vakh (use Gerrit instead)
Jialiu's feedback
5 months 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 ...
5 months ago (2017-01-25 02:13:41 UTC) #14
vakh (use Gerrit instead)
rebase
5 months 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 ...
5 months ago (2017-01-25 16:24:47 UTC) #26
vakh (use Gerrit instead)
lpz@ feedback
5 months 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 ...
5 months ago (2017-01-25 22:35:30 UTC) #30
vakh (use Gerrit instead)
rebase
5 months 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 ...
5 months 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: ...
5 months 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
5 months ago (2017-01-26 18:15:25 UTC) #43
commit-bot: I haz the power
5 months 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...
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld cb946e318