Prune all safe browsing incidents when already reported.

Prune all safe browsing incidents when already reported.

This is an overly-big hammer that will be refined later.

BUG=383043
R=asvitkine@chromium.org, gab@chromium.org, mattm@chromium.org, noms@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=278777

[grt (UTC plus 2), 2014-06-19 02:41:53]

Hi Matt,

Please review the delta between patch set 1 and patch set 2 of this change,
which is based on issue 341563003. Patch set 1 is a pure copy of patch set 8 of
issue 341563003. Patch set 2 contains the additions for this particular change.
Patch set 3 is 1+2 for the sake of some try runs. I hope that makes sense.

Cheers.

[grt (UTC plus 2), 2014-06-19 14:13:46]

On 2014/06/19 02:41:53, grt wrote:
> Hi Matt,
> 
> Please review the delta between patch set 1 and patch set 2 of this change,
> which is based on issue 341563003. Patch set 1 is a pure copy of patch set 8
of
> issue 341563003. Patch set 2 contains the additions for this particular
change.
> Patch set 3 is 1+2 for the sake of some try runs. I hope that makes sense.
> 
> Cheers.

Scratch that. Patch set 1 is now the real deal and ready for review. Thanks.

[grt (UTC plus 2), 2014-06-19 14:15:12]

+gab for prefs
+noms for profiles

thanks!

[noms (inactive), 2014-06-19 17:37:33]

profiles lgtm

These are the best profiles reviews ever :)

[grt (UTC plus 2), 2014-06-19 17:42:17]

On 2014/06/19 17:37:33, Monica Dinculescu wrote:
> profiles lgtm
> 
> These are the best profiles reviews ever :)

thanks. trying to keep it interesting for you...

[Alexei Svitkine (slow), 2014-06-19 18:26:21]

histograms lgtm

[gab, 2014-06-19 20:38:16]

prefs lgtm

https://codereview.chromium.org/348433004/diff/110001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/incident_reporting_service.cc (right):

https://codereview.chromium.org/348433004/diff/110001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting_service.cc:91: // The uploader
in use. This is NULL until the CSD killswitch is checked.
Will readers of this file know what "CSD" is? I certainly don't...

[grt (UTC plus 2), 2014-06-19 20:44:28]

Thanks!

https://codereview.chromium.org/348433004/diff/110001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/incident_reporting_service.cc (right):

https://codereview.chromium.org/348433004/diff/110001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting_service.cc:91: // The uploader
in use. This is NULL until the CSD killswitch is checked.
On 2014/06/19 20:38:16, gab wrote:
> Will readers of this file know what "CSD" is? I certainly don't...

Yeah. It's the client-side detection service that this and other safe browsing
client services talk to. If you didn't know, you could search for either "csd"
or "killswitch" in this file and pretty quickly get an idea of what "...until
the CSD killswitch is checked" means (hint: base::Bind, Csd, and KillSwitch all
appear on the same line. :-) ).

[mattm, 2014-06-19 21:43:11]

https://codereview.chromium.org/348433004/diff/110001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/incident_reporting_service_unittest.cc
(right):

https://codereview.chromium.org/348433004/diff/110001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting_service_unittest.cc:280: //
Tests that an incident added after upload is not uploaded again.
Add a test of profile gets destroyed while upload is pending.

https://codereview.chromium.org/348433004/diff/110001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting_service_unittest.cc:280: //
Tests that an incident added after upload is not uploaded again.
Add a test of incident reported and uploaded for one profile, then an incident
on another profile still gets uploaded.

[grt (UTC plus 2), 2014-06-20 03:24:10]

Thanks for the suggestions. PTAL.

https://codereview.chromium.org/348433004/diff/110001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/incident_reporting_service_unittest.cc
(right):

https://codereview.chromium.org/348433004/diff/110001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting_service_unittest.cc:280: //
Tests that an incident added after upload is not uploaded again.
On 2014/06/19 21:43:11, mattm wrote:
> Add a test of profile gets destroyed while upload is pending.

Done (ProfileDestroyedDuringUpload).

https://codereview.chromium.org/348433004/diff/110001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting_service_unittest.cc:280: //
Tests that an incident added after upload is not uploaded again.
On 2014/06/19 21:43:11, mattm wrote:
> Add a test of incident reported and uploaded for one profile, then an incident
> on another profile still gets uploaded.

Done (TwoProfilesTwoUploads).

[grt (UTC plus 2), 2014-06-20 05:06:13]

On 2014/06/20 03:24:10, grt wrote:
> Thanks for the suggestions. PTAL.
> 
>
https://codereview.chromium.org/348433004/diff/110001/chrome/browser/safe_bro...
> File chrome/browser/safe_browsing/incident_reporting_service_unittest.cc
> (right):
> 
>
https://codereview.chromium.org/348433004/diff/110001/chrome/browser/safe_bro...
> chrome/browser/safe_browsing/incident_reporting_service_unittest.cc:280: //
> Tests that an incident added after upload is not uploaded again.
> On 2014/06/19 21:43:11, mattm wrote:
> > Add a test of profile gets destroyed while upload is pending.
> 
> Done (ProfileDestroyedDuringUpload).
> 
>
https://codereview.chromium.org/348433004/diff/110001/chrome/browser/safe_bro...
> chrome/browser/safe_browsing/incident_reporting_service_unittest.cc:280: //
> Tests that an incident added after upload is not uploaded again.
> On 2014/06/19 21:43:11, mattm wrote:
> > Add a test of incident reported and uploaded for one profile, then an
incident
> > on another profile still gets uploaded.
> 
> Done (TwoProfilesTwoUploads).

I forgot to add: feel free to flip the CQ bit if it lgty (and the try runs are
largely green). Thanks.

[mattm, 2014-06-20 05:44:34]

https://codereview.chromium.org/348433004/diff/170001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/incident_reporting_service_unittest.cc
(right):

https://codereview.chromium.org/348433004/diff/170001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting_service_unittest.cc:95: // Handy
constants to make calls to CreateProfile easy to understand.
If you use enums it'll also be type-safe

https://codereview.chromium.org/348433004/diff/170001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting_service_unittest.cc:150:
std::string(),
Add comments with the parameter names.

https://codereview.chromium.org/348433004/diff/170001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting_service_unittest.cc:157: void
DeleteProfileOnUpload(Profile* profile) {
add an EXPECT/ASSERT_TRUE(on_start_upload_callback_.is_null())

[grt (UTC plus 2), 2014-06-20 14:52:54]

PTAL. I'm happy to address other test improvements in follow-on changes. Thanks.

https://codereview.chromium.org/348433004/diff/170001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/incident_reporting_service_unittest.cc
(right):

https://codereview.chromium.org/348433004/diff/170001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting_service_unittest.cc:95: // Handy
constants to make calls to CreateProfile easy to understand.
On 2014/06/20 05:44:34, mattm wrote:
> If you use enums it'll also be type-safe

Indeed. I chose not to since it would be more text for not much more gain. That
said, consider it done.

https://codereview.chromium.org/348433004/diff/170001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting_service_unittest.cc:150:
std::string(),
On 2014/06/20 05:44:34, mattm wrote:
> Add comments with the parameter names.

Done.

https://codereview.chromium.org/348433004/diff/170001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting_service_unittest.cc:157: void
DeleteProfileOnUpload(Profile* profile) {
On 2014/06/20 05:44:34, mattm wrote:
> add an EXPECT/ASSERT_TRUE(on_start_upload_callback_.is_null())

Done.

[mattm, 2014-06-20 15:55:13]

lgtm

[commit-bot: I haz the power, 2014-06-20 15:56:38]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/grt@chromium.org/348433004/190001

[grt (UTC plus 2), 2014-06-20 18:30:13]

Committed patchset #5 manually as r278777 (presubmit successful).