Add extended reporting level in the update request

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

[vakh (use Gerrit instead), 2017-01-25 00:14:29]

The CQ bit was checked by vakh@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-25 00:15:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vakh (use Gerrit instead), 2017-01-25 00:15:19]

vakh@chromium.org changed reviewers:
+ jialiul@chromium.org, nparker@chromium.org

[commit-bot: I haz the power, 2017-01-25 00:28:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-25 00:28:39]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[vakh (use Gerrit instead), 2017-01-25 01:18:02]

Add tests for ERL

[vakh (use Gerrit instead), 2017-01-25 01:18:04]

The CQ bit was checked by vakh@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-25 01:19:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Jialiu Lin, 2017-01-25 02:03:29]

lgtm with some nits.

https://codereview.chromium.org/2647323009/diff/1/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/safe_browsing_service.cc (right):

https://codereview.chromium.org/2647323009/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/safe_browsing_service.cc:725: 
nit: extra empty line.

https://codereview.chromium.org/2647323009/diff/1/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/services_delegate_impl.h (right):

https://codereview.chromium.org/2647323009/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/services_delegate_impl.h:36: 
nit: Is this extra empty line intentional?

https://codereview.chromium.org/2647323009/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/services_delegate_impl.h:55: ExtendedReportingLevel
GetMayBeExtendedReportingLevel() const;
nit: function name sounds a little bit funny. maybe
"GetEstimatedExtendedReportingLevel()"?

https://codereview.chromium.org/2647323009/diff/20001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/services_delegate_impl.cc (right):

https://codereview.chromium.org/2647323009/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/services_delegate_impl.cc:59: if
(safe_browsing_service_->maybe_enabled_extended_reporting_by_prefs()) {
nit: It is possible that both maybe_enabled_extended_reporting_by_prefs() and
maybe_enabled_scout_reporting_by_prefs() are true (a.k.a some profiles are in
scout, some profiles are in legacy SBER). Maybe we should document either here
or in the .h file that in case of a tie, we fall back to SBER.

[vakh (use Gerrit instead), 2017-01-25 02:13:35]

Jialiu's feedback

[vakh (use Gerrit instead), 2017-01-25 02:13:36]

The CQ bit was checked by vakh@chromium.org to run a CQ dry run

[vakh (use Gerrit instead), 2017-01-25 02:13:41]

Thanks for the quick review. All done.

https://codereview.chromium.org/2647323009/diff/1/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/safe_browsing_service.cc (right):

https://codereview.chromium.org/2647323009/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/safe_browsing_service.cc:725: 
On 2017/01/25 02:03:29, Jialiu Lin wrote:
> nit: extra empty line.

Was intentional but removed now.

https://codereview.chromium.org/2647323009/diff/1/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/services_delegate_impl.h (right):

https://codereview.chromium.org/2647323009/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/services_delegate_impl.h:36: 
On 2017/01/25 02:03:29, Jialiu Lin wrote:
> nit: Is this extra empty line intentional?

Nope. Removed.

https://codereview.chromium.org/2647323009/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/services_delegate_impl.h:55: ExtendedReportingLevel
GetMayBeExtendedReportingLevel() const;
On 2017/01/25 02:03:29, Jialiu Lin wrote:
> nit: function name sounds a little bit funny. maybe
> "GetEstimatedExtendedReportingLevel()"?

Done.

https://codereview.chromium.org/2647323009/diff/20001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/services_delegate_impl.cc (right):

https://codereview.chromium.org/2647323009/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/services_delegate_impl.cc:59: if
(safe_browsing_service_->maybe_enabled_extended_reporting_by_prefs()) {
On 2017/01/25 02:03:29, Jialiu Lin wrote:
> nit: It is possible that both maybe_enabled_extended_reporting_by_prefs() and
> maybe_enabled_scout_reporting_by_prefs() are true (a.k.a some profiles are in
> scout, some profiles are in legacy SBER). Maybe we should document either here
> or in the .h file that in case of a tie, we fall back to SBER. 

Done.

[commit-bot: I haz the power, 2017-01-25 02:14:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-25 02:29:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-25 02:29:20]

Dry run: 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_...)

[vakh (use Gerrit instead), 2017-01-25 02:38:28]

The CQ bit was checked by vakh@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-25 02:39:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vakh (use Gerrit instead), 2017-01-25 02:45:15]

rebase

[vakh (use Gerrit instead), 2017-01-25 02:45:18]

The CQ bit was checked by vakh@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-25 02:45:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-25 03:47:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-25 03:47:35]

Dry run: This issue passed the CQ dry run.

[lpz, 2017-01-25 16:24:46]

lpz@chromium.org changed reviewers:
+ lpz@chromium.org

[lpz, 2017-01-25 16:24:47]

https://codereview.chromium.org/2647323009/diff/60001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/safe_browsing_service.cc (right):

https://codereview.chromium.org/2647323009/diff/60001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_service.cc:727:
maybe_enabled_extended_reporting_by_prefs_ = false;
have you considered storing one field with the ExtendedReportingLevel enum here
rather than two bools? and just call
safe_browsing_prefs::GetExtendedReportingLevel below?

You would lose the ability to distinguish Scout from (scout with ER) vs (scout
without ER), but not sure that distinction matters? If it does matter,
GetExtendedReportingLevel could be extended do make that distinction.

One nice-to-have would be to avoid referencing
kSafeBrowsingExtendedReportingEnabled or kSafeBrowsingScoutReportingEnabled
outside of safe_browsing_prefs (if possible).

[vakh (use Gerrit instead), 2017-01-25 22:33:23]

lpz@ feedback

[vakh (use Gerrit instead), 2017-01-25 22:33:25]

The CQ bit was checked by vakh@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-25 22:34:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vakh (use Gerrit instead), 2017-01-25 22:35:30]

Thanks for the review. PTAL.

https://codereview.chromium.org/2647323009/diff/60001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/safe_browsing_service.cc (right):

https://codereview.chromium.org/2647323009/diff/60001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_service.cc:727:
maybe_enabled_extended_reporting_by_prefs_ = false;
On 2017/01/25 16:24:47, lpz wrote:
> have you considered storing one field with the ExtendedReportingLevel enum
here
> rather than two bools? and just call
> safe_browsing_prefs::GetExtendedReportingLevel below?
> 

Done.

> You would lose the ability to distinguish Scout from (scout with ER) vs (scout
> without ER), but not sure that distinction matters? If it does matter,
> GetExtendedReportingLevel could be extended do make that distinction.
> 

Not that important.

> One nice-to-have would be to avoid referencing
> kSafeBrowsingExtendedReportingEnabled or kSafeBrowsingScoutReportingEnabled
> outside of safe_browsing_prefs (if possible).

Unfortunately it is already being used in this file but I am not adding another
use of it now.

[commit-bot: I haz the power, 2017-01-25 22:48:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-25 22:48:16]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[vakh (use Gerrit instead), 2017-01-25 22:49:45]

rebase

[vakh (use Gerrit instead), 2017-01-25 22:49:46]

The CQ bit was checked by vakh@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-25 22:50:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vakh (use Gerrit instead), 2017-01-25 22:59:20]

Description was changed from

==========
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: In ServicesDelegateImpl, provide a method that choses one of those and
  reports it.
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
==========

to

==========
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
==========

[commit-bot: I haz the power, 2017-01-26 02:30:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-26 02:30:37]

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...)

[lpz, 2017-01-26 16:36:46]

lgtm

https://codereview.chromium.org/2647323009/diff/80001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/safe_browsing_service.cc (right):

https://codereview.chromium.org/2647323009/diff/80001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_service.cc:732:
estimated_extended_reporting_by_prefs_ = erl;
nit: would you perhaps care about finding the *max* reporting level?

[vakh (use Gerrit instead), 2017-01-26 18:14:30]

https://codereview.chromium.org/2647323009/diff/80001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/safe_browsing_service.cc (right):

https://codereview.chromium.org/2647323009/diff/80001/chrome/browser/safe_bro...
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:
> nit: would you perhaps care about finding the *max* reporting level?

That would bias us towards Scout and I am not sure if we want that.
Picking the first one gives us an equal chance of selecting legacy extended
reporting.

[vakh (use Gerrit instead), 2017-01-26 18:14:57]

The CQ bit was checked by vakh@chromium.org

[vakh (use Gerrit instead), 2017-01-26 18:14:58]

The patchset sent to the CQ was uploaded after l-g-t-m from jialiul@chromium.org
Link to the patchset: https://codereview.chromium.org/2647323009/#ps100001
(title: "rebase")

[commit-bot: I haz the power, 2017-01-26 18:15:25]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-26 19:57:59]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1485454497987780,
"parent_rev": "fa2a16567af6fc774c0a9fe44fb53eb5135056b0", "commit_rev":
"0e143114a2eff79cd99bb289ce8956c8c01d675f"}

[commit-bot: I haz the power, 2017-01-26 19:58:29]

Description was changed from

==========
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
==========

to

==========
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/+/0e143114a2eff79cd99bb289ce89...
==========

[commit-bot: I haz the power, 2017-01-26 19:58:30]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/0e143114a2eff79cd99bb289ce89...