SafeBrowsing: Adds response handler for API blacklist check in Database Manager.

SafeBrowsing: Adds response handler for API blacklist check in Database Manager.

BUG=561867
Committed: https://crrev.com/4d1048bb163a405c0ab53bff755fc0552966718a
Cr-Commit-Position: refs/heads/master@{#388149}

[kcarattini, 2016-04-13 04:15:34]

kcarattini@chromium.org changed reviewers:
+ nparker@chromium.org

[Nathan Parker, 2016-04-15 23:22:00]

lgtm

https://codereview.chromium.org/1881353002/diff/40001/components/safe_browsin...
File components/safe_browsing_db/database_manager.cc (right):

https://codereview.chromium.org/1881353002/diff/40001/components/safe_browsin...
components/safe_browsing_db/database_manager.cc:89: DCHECK(check);
Sounds like a microphone check.  "Check. Check. Check one two."

https://codereview.chromium.org/1881353002/diff/40001/components/safe_browsin...
components/safe_browsing_db/database_manager.cc:93: for (const SBFullHashResult&
result : full_hash_results) {
This is sort of O(N^2), where N is small but could easily be 10.  Maybe add a
todo to look at performance? This could be optimized by making one of them a
map, or sorting them and doing a binary search, etc.

https://codereview.chromium.org/1881353002/diff/40001/components/safe_browsin...
components/safe_browsing_db/database_manager.cc:104:
check->client()->OnCheckApiBlacklistUrlResult(check->url(), md);
Consider if we need the URL to be passed here, and remove it if not.
Some of the other client->methods have some fields they don't need.

[kcarattini, 2016-04-18 07:12:02]

https://codereview.chromium.org/1881353002/diff/40001/components/safe_browsin...
File components/safe_browsing_db/database_manager.cc (right):

https://codereview.chromium.org/1881353002/diff/40001/components/safe_browsin...
components/safe_browsing_db/database_manager.cc:89: DCHECK(check);
On 2016/04/15 23:22:00, Nathan Parker wrote:
> Sounds like a microphone check.  "Check. Check. Check one two."

Acknowledged.

https://codereview.chromium.org/1881353002/diff/40001/components/safe_browsin...
components/safe_browsing_db/database_manager.cc:93: for (const SBFullHashResult&
result : full_hash_results) {
On 2016/04/15 23:22:00, Nathan Parker wrote:
> This is sort of O(N^2), where N is small but could easily be 10.  Maybe add a
> todo to look at performance? This could be optimized by making one of them a
> map, or sorting them and doing a binary search, etc.

Done.

https://codereview.chromium.org/1881353002/diff/40001/components/safe_browsin...
components/safe_browsing_db/database_manager.cc:104:
check->client()->OnCheckApiBlacklistUrlResult(check->url(), md);
On 2016/04/15 23:22:00, Nathan Parker wrote:
> Consider if we need the URL to be passed here, and remove it if not.
> Some of the other client->methods have some fields they don't need.

I can imagine a client making calls for multiple urls and needing to know which
one the reply is for. I think it's useful.

[Nathan Parker, 2016-04-18 17:34:09]

lgtm

https://codereview.chromium.org/1881353002/diff/40001/components/safe_browsin...
File components/safe_browsing_db/database_manager.cc (right):

https://codereview.chromium.org/1881353002/diff/40001/components/safe_browsin...
components/safe_browsing_db/database_manager.cc:104:
check->client()->OnCheckApiBlacklistUrlResult(check->url(), md);
On 2016/04/18 07:12:02, kcarattini wrote:
> On 2016/04/15 23:22:00, Nathan Parker wrote:
> > Consider if we need the URL to be passed here, and remove it if not.
> > Some of the other client->methods have some fields they don't need.
> 
> I can imagine a client making calls for multiple urls and needing to know
which
> one the reply is for. I think it's useful.

k.  Another approach is that the caller could use different client objects, so
they can keep track of whatever state they want in them.

I think the localdbmanager has the requirement that any single client ptr can
have at most one outstanding callback.  Otherwise, when you cancel(client), it
wouldn't know which one to cancel.

[kcarattini, 2016-04-19 04:14:21]

On 2016/04/18 17:34:09, Nathan Parker wrote:
> lgtm
> 
>
https://codereview.chromium.org/1881353002/diff/40001/components/safe_browsin...
> File components/safe_browsing_db/database_manager.cc (right):
> 
>
https://codereview.chromium.org/1881353002/diff/40001/components/safe_browsin...
> components/safe_browsing_db/database_manager.cc:104:
> check->client()->OnCheckApiBlacklistUrlResult(check->url(), md);
> On 2016/04/18 07:12:02, kcarattini wrote:
> > On 2016/04/15 23:22:00, Nathan Parker wrote:
> > > Consider if we need the URL to be passed here, and remove it if not.
> > > Some of the other client->methods have some fields they don't need.
> > 
> > I can imagine a client making calls for multiple urls and needing to know
> which
> > one the reply is for. I think it's useful.
> 
> k.  Another approach is that the caller could use different client objects, so
> they can keep track of whatever state they want in them.
> 
> I think the localdbmanager has the requirement that any single client ptr can
> have at most one outstanding callback.  Otherwise, when you cancel(client), it
> wouldn't know which one to cancel.

Hmm, well if that's the case, then this class should behave consistently. I've
added  a TODO to consider removing the url param. I'd like to think on it a bit
more.

[kcarattini, 2016-04-19 06:02:58]

The CQ bit was checked by kcarattini@chromium.org

[kcarattini, 2016-04-19 06:02:58]

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

[commit-bot: I haz the power, 2016-04-19 06:03:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1881353002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1881353002/100001

[commit-bot: I haz the power, 2016-04-19 06:52:42]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-04-19 06:53:59]

Description was changed from

==========
SafeBrowsing: Adds response handler for API blacklist check in Database Manager.

BUG=561867
==========

to

==========
SafeBrowsing: Adds response handler for API blacklist check in Database Manager.

BUG=561867

Committed: https://crrev.com/4d1048bb163a405c0ab53bff755fc0552966718a
Cr-Commit-Position: refs/heads/master@{#388149}
==========

[commit-bot: I haz the power, 2016-04-19 06:54:00]

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