Take a Profile when adding an incident to the incident reporting service.

Take a Profile when adding an incident to the incident reporting service.

This will simplify use of the API for consumers who don't know a
Profile* at the time of getting the callback.

BUG=none
R=robertshield@chromium.org

Committed: https://crrev.com/36ef2a703d981ec5f8e277033f5a70cebd9e0a98
Cr-Commit-Position: refs/heads/master@{#314328}

[grt (UTC plus 2), 2015-01-30 20:25:35]

I think this is ready for review. Please take a look.

[robertshield, 2015-01-30 21:04:52]

https://codereview.chromium.org/891793002/diff/20001/chrome/browser/safe_brow...
File
chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer.cc
(right):

https://codereview.chromium.org/891793002/diff/20001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer.cc:82:
new BinaryIntegrityIncident(incident.Pass())));
What do you think about having two types of callbacks, one that takes a Profile
and one that doesn't?

[grt (UTC plus 2), 2015-01-30 21:16:29]

https://codereview.chromium.org/891793002/diff/20001/chrome/browser/safe_brow...
File
chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer.cc
(right):

https://codereview.chromium.org/891793002/diff/20001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/incident_reporting/binary_integrity_analyzer.cc:82:
new BinaryIntegrityIncident(incident.Pass())));
On 2015/01/30 21:04:52, robertshield wrote:
> What do you think about having two types of callbacks, one that takes a
Profile
> and one that doesn't?

Yeah, I thought about that. I wonder if an interface is better than two
callbacks... Will consider on Monday.

[grt (UTC plus 2), 2015-02-02 18:10:58]

PTAL

[robertshield, 2015-02-03 02:54:42]

lgtm

https://codereview.chromium.org/891793002/diff/80001/chrome/browser/safe_brow...
File
chrome/browser/safe_browsing/incident_reporting/incident_reporting_service.cc
(right):

https://codereview.chromium.org/891793002/diff/80001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/incident_reporting/incident_reporting_service.cc:244:
DCHECK(thread_runner_->BelongsToCurrentThread());
For my own education: this DCHECK in practice checks that this is called on the
UI thread, so is there a reason not to check for being on the UI thread
explicitly?  It seems like not doing so (as you've done here) is nicer from a
not-as-coupled-to-specific-threading-model point of view, and yet the static
method is called AddIncident*OnUIThread* which is kind of specific.

[grt (UTC plus 2), 2015-02-03 03:27:59]

Thanks.

https://codereview.chromium.org/891793002/diff/80001/chrome/browser/safe_brow...
File
chrome/browser/safe_browsing/incident_reporting/incident_reporting_service.cc
(right):

https://codereview.chromium.org/891793002/diff/80001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/incident_reporting/incident_reporting_service.cc:244:
DCHECK(thread_runner_->BelongsToCurrentThread());
On 2015/02/03 02:54:42, robertshield wrote:
> For my own education: this DCHECK in practice checks that this is called on
the
> UI thread, so is there a reason not to check for being on the UI thread
> explicitly?  It seems like not doing so (as you've done here) is nicer from a
> not-as-coupled-to-specific-threading-model point of view, and yet the static
> method is called AddIncident*OnUIThread* which is kind of specific. 

I wish I didn't have to use BrowserThread anywhere in the impl since it leads to
tighter coupling and more weight in unit tests. I just noticed that the comment
for the class (line 208) says "main thread" rather than "UI thread," so I guess
the answer is that sometimes I'm inconsistent. I'll rename the function on line
262 so that it matches the comment and is therefore less jarring.

[grt (UTC plus 2), 2015-02-03 03:31:56]

The CQ bit was checked by grt@chromium.org

[commit-bot: I haz the power, 2015-02-03 03:33:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/891793002/120001

[commit-bot: I haz the power, 2015-02-03 05:34:39]

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

[commit-bot: I haz the power, 2015-02-03 05:34:39]

Try jobs failed on following builders:
  win8_chromium_rel on tryserver.chromium.win (None)

[grt (UTC plus 2), 2015-02-03 13:53:39]

The CQ bit was checked by grt@chromium.org

[commit-bot: I haz the power, 2015-02-03 13:53:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/891793002/120001

[commit-bot: I haz the power, 2015-02-03 13:55:06]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-02-03 13:56:20]

Patchset 7 (id:??) landed as
https://crrev.com/36ef2a703d981ec5f8e277033f5a70cebd9e0a98
Cr-Commit-Position: refs/heads/master@{#314328}