Check result of ApiBlacklist query in client.

Check result of ApiBlacklist query in client.

If the API blacklist query can be responded to synchronously, the Safe
Browsing Database Manager will respond true to the calling client. This
was not being checked previously, and hence the callback method on the
client not being called.

BUG=683811
Review-Url: https://codereview.chromium.org/2684523002
Cr-Commit-Position: refs/heads/master@{#448888}
Committed: https://chromium.googlesource.com/chromium/src/+/48ad1681cf6efac487c260d75ac5d5f35ab493e3

[meredithl, 2017-02-07 07:18:54]

The CQ bit was checked by meredithl@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-02-07 07:19:06]

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

[meredithl, 2017-02-07 07:19:50]

meredithl@google.com changed reviewers:
+ dominickn@chromium.org, kcarattini@chromium.org, raymes@chromium.org

[meredithl, 2017-02-07 07:19:50]

Hey guys, PTAL, thanks!

[commit-bot: I haz the power, 2017-02-07 08:02:46]

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

[commit-bot: I haz the power, 2017-02-07 08:02:47]

Dry run: This issue passed the CQ dry run.

[dominickn, 2017-02-07 08:27:06]

lgtm % nit

https://codereview.chromium.org/2684523002/diff/1/chrome/browser/permissions/...
File chrome/browser/permissions/permission_blacklist_client.cc (right):

https://codereview.chromium.org/2684523002/diff/1/chrome/browser/permissions/...
chrome/browser/permissions/permission_blacklist_client.cc:68:
this->OnCheckApiBlacklistUrlResult(request_origin, empty_metadata);
Nit: no need for this->

[kcarattini, 2017-02-07 22:15:18]

Thanks for doing this.  Please add a test with a mock DB Manager, as we
discussed.

Kendra

[meredithl, 2017-02-08 00:56:52]

Description was changed from

==========
Check result of ApiBlacklist query in client.

If the API blacklist query can be responded to synchronously, the Safe
Browsing Database Manager will respond true to the calling client. This
was not being checked previously, and hence the callback method on the
client not being called.

There are currently no tests that cover this case, as it is difficult to mimic
this behaviour. One solution would be to use an enabled_ member variable in the
MockSafeBrowsingDatabaseManager class, but out of context this doesn't really
make much sense. The Safe Browsing Database Manager provides a test for
ensuring that the wrong scheme causes this method to return true.

BUG=683811
==========

to

==========
Check result of ApiBlacklist query in client.

If the API blacklist query can be responded to synchronously, the Safe
Browsing Database Manager will respond true to the calling client. This
was not being checked previously, and hence the callback method on the
client not being called.

BUG=683811
==========

[meredithl, 2017-02-08 00:58:03]

Okay, strange linking behaviour aside, testing a disabled database has been
added. Thanks!

https://codereview.chromium.org/2684523002/diff/1/chrome/browser/permissions/...
File chrome/browser/permissions/permission_blacklist_client.cc (right):

https://codereview.chromium.org/2684523002/diff/1/chrome/browser/permissions/...
chrome/browser/permissions/permission_blacklist_client.cc:68:
this->OnCheckApiBlacklistUrlResult(request_origin, empty_metadata);
On 2017/02/07 08:27:06, dominickn wrote:
> Nit: no need for this->

Done.

[raymes, 2017-02-08 01:42:07]

lgtm

https://codereview.chromium.org/2684523002/diff/20001/chrome/browser/permissi...
File chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc
(right):

https://codereview.chromium.org/2684523002/diff/20001/chrome/browser/permissi...
chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc:10:
#include "base/logging.h"
nit: is this needed?

[kcarattini, 2017-02-08 02:07:28]

https://codereview.chromium.org/2684523002/diff/20001/chrome/browser/permissi...
File chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc
(right):

https://codereview.chromium.org/2684523002/diff/20001/chrome/browser/permissi...
chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc:500:
EXPECT_FALSE(last_embargoed_status());
Can you also check that the callback was run. Otherwise this could pass by
accident since this is set to false SetUp.

[meredithl, 2017-02-08 02:18:23]

https://codereview.chromium.org/2684523002/diff/20001/chrome/browser/permissi...
File chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc
(right):

https://codereview.chromium.org/2684523002/diff/20001/chrome/browser/permissi...
chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc:10:
#include "base/logging.h"
On 2017/02/08 01:42:06, raymes wrote:
> nit: is this needed?

No! Forgot to remove it. Done.

https://codereview.chromium.org/2684523002/diff/20001/chrome/browser/permissi...
chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc:500:
EXPECT_FALSE(last_embargoed_status());
On 2017/02/08 02:07:28, kcarattini wrote:
> Can you also check that the callback was run. Otherwise this could pass by
> accident since this is set to false SetUp.

Done.

[meredithl, 2017-02-08 02:19:26]

The CQ bit was checked by meredithl@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-02-08 02:20:17]

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

[kcarattini, 2017-02-08 02:21:37]

lgtm

[commit-bot: I haz the power, 2017-02-08 03:09:41]

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

[commit-bot: I haz the power, 2017-02-08 03:09:42]

Dry run: This issue passed the CQ dry run.

[meredithl, 2017-02-08 03:10:32]

The CQ bit was checked by meredithl@google.com

[meredithl, 2017-02-08 03:10:32]

The patchset sent to the CQ was uploaded after l-g-t-m from
dominickn@chromium.org, raymes@chromium.org
Link to the patchset: https://codereview.chromium.org/2684523002/#ps40001
(title: "Check callback.")

[commit-bot: I haz the power, 2017-02-08 03:11:01]

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-02-08 03:15:42]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1486523432260580,
"parent_rev": "4d0a6c63e80cbe4806f4a9928a685a6cc99187b7", "commit_rev":
"48ad1681cf6efac487c260d75ac5d5f35ab493e3"}

[commit-bot: I haz the power, 2017-02-08 03:16:20]

Description was changed from

==========
Check result of ApiBlacklist query in client.

If the API blacklist query can be responded to synchronously, the Safe
Browsing Database Manager will respond true to the calling client. This
was not being checked previously, and hence the callback method on the
client not being called.

BUG=683811
==========

to

==========
Check result of ApiBlacklist query in client.

If the API blacklist query can be responded to synchronously, the Safe
Browsing Database Manager will respond true to the calling client. This
was not being checked previously, and hence the callback method on the
client not being called.

BUG=683811

Review-Url: https://codereview.chromium.org/2684523002
Cr-Commit-Position: refs/heads/master@{#448888}
Committed:
https://chromium.googlesource.com/chromium/src/+/48ad1681cf6efac487c260d75ac5...
==========

[commit-bot: I haz the power, 2017-02-08 03:16:21]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/48ad1681cf6efac487c260d75ac5...