Add support for scout to 'ext' param when creating SafeBrowsing ping URLs.

Add support for scout to 'ext' param when creating SafeBrowsing ping URLs.

This is the quick and dirty version that updates all call sites to support the
"reporting level" enum instead of a boolean. There seems to be a lot of duplication
in this code so probably a candidate for refactoring as a follow-up. But this
will at least give us Scout insight in a shorter term.

BUG=665922
Committed: https://crrev.com/ed21b96db47a2ad59e4c9cf6b9e3c05dd4d85190
Cr-Commit-Position: refs/heads/master@{#433012}

[lpz, 2016-11-16 21:04:07]

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

[commit-bot: I haz the power, 2016-11-16 21:05:13]

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, 2016-11-16 21:11:19]

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

[commit-bot: I haz the power, 2016-11-16 21:11:20]

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

[lpz, 2016-11-17 16:24:18]

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

[commit-bot: I haz the power, 2016-11-17 16:24:51]

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

[lpz, 2016-11-17 16:36:09]

lpz@chromium.org changed reviewers:
+ awoz@chromium.org, jialiul@chromium.org, nparker@chromium.org

[lpz, 2016-11-17 16:36:09]

Here's a quick-and-dirty stab at this to try to get something into m56. I'd do a
follow-up to try to refactor some of this next week.

[commit-bot: I haz the power, 2016-11-17 18:28:42]

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

[commit-bot: I haz the power, 2016-11-17 18:28:43]

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

[lpz, 2016-11-17 18:32:15]

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

[commit-bot: I haz the power, 2016-11-17 18:32:58]

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

[Jialiu Lin, 2016-11-17 18:47:23]

I feel for some cases (e.g. whether sample unsupported files or sample
whitelisted download), a bool is_extended_reporting (or
IsExtendedReportingEnabled()) seems sufficient, or maybe you have some future
use cases in mind that needs distinguishing SBER and SCOUT for these things?

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

https://codereview.chromium.org/2505153002/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/download_protection_service.cc:951: auto population
= extended_reporting_level_ != SBER_LEVEL_OFF
You might want to add ChromeUserPopulation::SCOUT_REPORTING to 
c/c/safe_browsing/csd.proto. And ping awoz@ to change its corresponding proto in
google3. 
otherwise, it seems unnecessary to change this bool to ExtendedReportingLevel
enum.

https://codereview.chromium.org/2505153002/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/download_protection_service.cc:1332: auto
population = extended_reporting_level_ != SBER_LEVEL_OFF
Similar to above.

https://codereview.chromium.org/2505153002/diff/20001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/protocol_manager.h (right):

https://codereview.chromium.org/2505153002/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/protocol_manager.h:413: ExtendedReportingLevel /*
reporting_level */)>;
nit: align with the above two comments?

[vakh (use Gerrit instead), 2016-11-17 18:53:44]

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

[vakh (use Gerrit instead), 2016-11-17 18:53:44]

lgtm modulo Jialiu's comments.

Optional: You could also consider writing a helper method that takes a const
Profile* as an argument and returns ExtendedReportingLevel. That would simplify
the code in a number of places in this CL.

[lpz, 2016-11-17 19:46:05]

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

https://codereview.chromium.org/2505153002/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/download_protection_service.cc:951: auto population
= extended_reporting_level_ != SBER_LEVEL_OFF
On 2016/11/17 18:47:23, Jialiu Lin wrote:
> You might want to add ChromeUserPopulation::SCOUT_REPORTING to 
> c/c/safe_browsing/csd.proto. And ping awoz@ to change its corresponding proto
in
> google3. 
> otherwise, it seems unnecessary to change this bool to ExtendedReportingLevel
> enum. 

Agreed, back to a bool. I think adding another population can happen as a
followup.

https://codereview.chromium.org/2505153002/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/download_protection_service.cc:1332: auto
population = extended_reporting_level_ != SBER_LEVEL_OFF
On 2016/11/17 18:47:23, Jialiu Lin wrote:
> Similar to above.

Done.

https://codereview.chromium.org/2505153002/diff/20001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/protocol_manager.h (right):

https://codereview.chromium.org/2505153002/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/protocol_manager.h:413: ExtendedReportingLevel /*
reporting_level */)>;
On 2016/11/17 18:47:23, Jialiu Lin wrote:
> nit: align with the above two comments?

Done, although git cl format seems to put this back, so not sure I'll be able to
submit it that way.

[lpz, 2016-11-17 19:46:13]

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

[commit-bot: I haz the power, 2016-11-17 19:47:47]

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

[Jialiu Lin, 2016-11-17 19:50:30]

lgtm

https://codereview.chromium.org/2505153002/diff/40001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/download_protection_service.cc (right):

https://codereview.chromium.org/2505153002/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/download_protection_service.cc:1243:
GetExtendedReportingLevel(*profile->GetPrefs()) != SBER_LEVEL_OFF;
nit, feel free to ignore.: why not IsExtendedReportingEnabled()?

[lpz, 2016-11-17 19:57:08]

https://codereview.chromium.org/2505153002/diff/40001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/download_protection_service.cc (right):

https://codereview.chromium.org/2505153002/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/download_protection_service.cc:1243:
GetExtendedReportingLevel(*profile->GetPrefs()) != SBER_LEVEL_OFF;
On 2016/11/17 19:50:30, Jialiu Lin wrote:
> nit, feel free to ignore.: why not IsExtendedReportingEnabled()?  

Doh, I went too fast. Of course, it should be as you suggest. Thanks :)

[lpz, 2016-11-17 19:57:16]

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

[commit-bot: I haz the power, 2016-11-17 19:58:28]

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, 2016-11-17 21:58:49]

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

[commit-bot: I haz the power, 2016-11-17 21:58:51]

Dry run: This issue passed the CQ dry run.

[lpz, 2016-11-17 22:02:44]

The CQ bit was checked by lpz@chromium.org

[lpz, 2016-11-17 22:02:45]

The patchset sent to the CQ was uploaded after l-g-t-m from vakh@chromium.org,
jialiul@chromium.org
Link to the patchset: https://codereview.chromium.org/2505153002/#ps60001
(title: "Go back to IsExtendedReportingEnabled where level doesn't matter")

[commit-bot: I haz the power, 2016-11-17 22:04:52]

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, 2016-11-17 23:17:26]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-11-17 23:21:45]

Description was changed from

==========
Add support for scout to 'ext' param when creating SafeBrowsing ping URLs.

This is the quick and dirty version that updates all call sites to support the
"reporting level" enum instead of a boolean. There seems to be a lot of
duplication
in this code so probably a candidate for refactoring as a follow-up. But this
will at least give us Scout insight in a shorter term.

BUG=665922
==========

to

==========
Add support for scout to 'ext' param when creating SafeBrowsing ping URLs.

This is the quick and dirty version that updates all call sites to support the
"reporting level" enum instead of a boolean. There seems to be a lot of
duplication
in this code so probably a candidate for refactoring as a follow-up. But this
will at least give us Scout insight in a shorter term.

BUG=665922

Committed: https://crrev.com/ed21b96db47a2ad59e4c9cf6b9e3c05dd4d85190
Cr-Commit-Position: refs/heads/master@{#433012}
==========

[commit-bot: I haz the power, 2016-11-17 23:21:46]

Patchset 4 (id:??) landed as
https://crrev.com/ed21b96db47a2ad59e4c9cf6b9e3c05dd4d85190
Cr-Commit-Position: refs/heads/master@{#433012}