Refactor blacklist client into own .h/.cc.

Refactor blacklist client into own .h/.cc.

The size of the Safe Browsing client class was getting to be too large
to be contained entirely within PermissionContextBase, so it has been
moved to its own header and implementation.

Test coverage is supplied in permission_context_base_unitttest.cc, but
there will be follow up unit tests for this class.

BUG=561867
Review-Url: https://codereview.chromium.org/2626853002
Cr-Commit-Position: refs/heads/master@{#442805}
Committed: https://chromium.googlesource.com/chromium/src/+/397bca8885b9bd50a808517fdb93b512189c114a

[meredithl, 2017-01-10 23:02:53]

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

[commit-bot: I haz the power, 2017-01-10 23:03:52]

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

[meredithl, 2017-01-10 23:31:30]

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

[meredithl, 2017-01-10 23:31:30]

Hey guys!
PTAL, thanks :)

[raymes, 2017-01-11 00:00:37]

Thanks!

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

https://codereview.chromium.org/2626853002/diff/1/chrome/browser/permissions/...
chrome/browser/permissions/permission_blacklist_client.cc:1: // Copyright 2014
The Chromium Authors. All rights reserved.
nit: 2017

https://codereview.chromium.org/2626853002/diff/1/chrome/browser/permissions/...
chrome/browser/permissions/permission_blacklist_client.cc:11: #include
"components/safe_browsing_db/database_manager.h"
this is in the header, so not needed here

https://codereview.chromium.org/2626853002/diff/1/chrome/browser/permissions/...
chrome/browser/permissions/permission_blacklist_client.cc:40: is_active_(true)
/* Web Contents is active upon construction */ {
nit: don't use an inline comment here. I think it's ok to omit the
comment in this case. 

This was fixed in the committed code - is the code in this file up to date with
the code in permission_context_base? Please make sure they are identical.

https://codereview.chromium.org/2626853002/diff/1/chrome/browser/permissions/...
chrome/browser/permissions/permission_blacklist_client.cc:51:
DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
#include base/logging.h

https://codereview.chromium.org/2626853002/diff/1/chrome/browser/permissions/...
chrome/browser/permissions/permission_blacklist_client.cc:56: timer_ =
base::MakeUnique<base::OneShotTimer>();
#include base/timer.h

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

https://codereview.chromium.org/2626853002/diff/1/chrome/browser/permissions/...
chrome/browser/permissions/permission_blacklist_client.h:1: // Copyright 2014
The Chromium Authors. All rights reserved.
2017

https://codereview.chromium.org/2626853002/diff/1/chrome/browser/permissions/...
chrome/browser/permissions/permission_blacklist_client.h:8: #include <stddef.h>
What's stddef needed for?

https://codereview.chromium.org/2626853002/diff/1/chrome/browser/permissions/...
chrome/browser/permissions/permission_blacklist_client.h:75:
std::unique_ptr<base::OneShotTimer> timer_;
nit: This should be forward declared

[meredithl, 2017-01-11 00:34:05]

Description was changed from

==========
Refactor blacklist client into own .h/.cc.

The size of the Safe Browsing client class was getting to be too large
to be contained entirely within PermissionContextBase, so it has been
moved to its own header and implementation.

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

to

==========
Refactor blacklist client into own .h/.cc.

The size of the Safe Browsing client class was getting to be too large
to be contained entirely within PermissionContextBase, so it has been
moved to its own header and implementation.

Test coverage is supplied in permission_context_base_unitttest.cc, but
there will be follow up unit tests for this class.

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

[commit-bot: I haz the power, 2017-01-11 00:45:54]

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

[commit-bot: I haz the power, 2017-01-11 00:45:54]

Dry run: This issue passed the CQ dry run.

[dominickn, 2017-01-11 01:47:39]

lgtm % most of raymes' comments

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

https://codereview.chromium.org/2626853002/diff/1/chrome/browser/permissions/...
chrome/browser/permissions/permission_blacklist_client.cc:51:
DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
On 2017/01/11 00:00:36, raymes (a bit slow) wrote:
> #include base/logging.h

Why do we need base/logging.h? DCHECK_CURRENTLY_ON is defined in
browser_thread.h

[raymes, 2017-01-11 01:56:30]

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

https://codereview.chromium.org/2626853002/diff/1/chrome/browser/permissions/...
chrome/browser/permissions/permission_blacklist_client.cc:51:
DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
On 2017/01/11 01:47:38, dominickn wrote:
> On 2017/01/11 00:00:36, raymes (a bit slow) wrote:
> > #include base/logging.h
> 
> Why do we need base/logging.h? DCHECK_CURRENTLY_ON is defined in
> browser_thread.h

You're right, ignore me :)

[meredithl, 2017-01-11 02:24:53]

Thanks!

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

https://codereview.chromium.org/2626853002/diff/1/chrome/browser/permissions/...
chrome/browser/permissions/permission_blacklist_client.cc:1: // Copyright 2014
The Chromium Authors. All rights reserved.
On 2017/01/11 00:00:36, raymes (a bit slow) wrote:
> nit: 2017

Done.

https://codereview.chromium.org/2626853002/diff/1/chrome/browser/permissions/...
chrome/browser/permissions/permission_blacklist_client.cc:11: #include
"components/safe_browsing_db/database_manager.h"
On 2017/01/11 00:00:36, raymes (a bit slow) wrote:
> this is in the header, so not needed here

Done.

https://codereview.chromium.org/2626853002/diff/1/chrome/browser/permissions/...
chrome/browser/permissions/permission_blacklist_client.cc:40: is_active_(true)
/* Web Contents is active upon construction */ {
On 2017/01/11 00:00:36, raymes (a bit slow) wrote:
> nit: don't use an inline comment here. I think it's ok to omit the
> comment in this case. 
> 
> This was fixed in the committed code - is the code in this file up to date
with
> the code in permission_context_base? Please make sure they are identical.

Sorry raymes - yes, I had already pulled most of the client out before landing
the previous CL, so didn't have the last few changes in there. Should be fixed
now.

https://codereview.chromium.org/2626853002/diff/1/chrome/browser/permissions/...
chrome/browser/permissions/permission_blacklist_client.cc:51:
DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
On 2017/01/11 01:56:30, raymes (a bit slow) wrote:
> On 2017/01/11 01:47:38, dominickn wrote:
> > On 2017/01/11 00:00:36, raymes (a bit slow) wrote:
> > > #include base/logging.h
> > 
> > Why do we need base/logging.h? DCHECK_CURRENTLY_ON is defined in
> > browser_thread.h
> 
> You're right, ignore me :)

Acknowledged.

https://codereview.chromium.org/2626853002/diff/1/chrome/browser/permissions/...
chrome/browser/permissions/permission_blacklist_client.cc:56: timer_ =
base::MakeUnique<base::OneShotTimer>();
On 2017/01/11 00:00:36, raymes (a bit slow) wrote:
> #include base/timer.h

Done.

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

https://codereview.chromium.org/2626853002/diff/1/chrome/browser/permissions/...
chrome/browser/permissions/permission_blacklist_client.h:1: // Copyright 2014
The Chromium Authors. All rights reserved.
On 2017/01/11 00:00:37, raymes (a bit slow) wrote:
> 2017

Done.

https://codereview.chromium.org/2626853002/diff/1/chrome/browser/permissions/...
chrome/browser/permissions/permission_blacklist_client.h:8: #include <stddef.h>
On 2017/01/11 00:00:36, raymes (a bit slow) wrote:
> What's stddef needed for?

Nothing, was meant to be removed. Done.

https://codereview.chromium.org/2626853002/diff/1/chrome/browser/permissions/...
chrome/browser/permissions/permission_blacklist_client.h:75:
std::unique_ptr<base::OneShotTimer> timer_;
On 2017/01/11 00:00:36, raymes (a bit slow) wrote:
> nit: This should be forward declared

Done.

[raymes, 2017-01-11 03:02:52]

lgtm!

[meredithl, 2017-01-11 03:40:34]

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

[commit-bot: I haz the power, 2017-01-11 03:41: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, 2017-01-11 04:58:19]

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

[commit-bot: I haz the power, 2017-01-11 04:58:20]

Dry run: This issue passed the CQ dry run.

[meredithl, 2017-01-11 04:59:24]

The CQ bit was checked by meredithl@google.com

[meredithl, 2017-01-11 04:59:24]

The patchset sent to the CQ was uploaded after l-g-t-m from
dominickn@chromium.org
Link to the patchset: https://codereview.chromium.org/2626853002/#ps20001
(title: "Nits")

[commit-bot: I haz the power, 2017-01-11 04:59:38]

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-01-11 05:03:52]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1484110764489540,
"parent_rev": "ae554451211cf78a57a6257abf4943f679fa5298", "commit_rev":
"397bca8885b9bd50a808517fdb93b512189c114a"}

[commit-bot: I haz the power, 2017-01-11 05:04:59]

Description was changed from

==========
Refactor blacklist client into own .h/.cc.

The size of the Safe Browsing client class was getting to be too large
to be contained entirely within PermissionContextBase, so it has been
moved to its own header and implementation.

Test coverage is supplied in permission_context_base_unitttest.cc, but
there will be follow up unit tests for this class.

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

to

==========
Refactor blacklist client into own .h/.cc.

The size of the Safe Browsing client class was getting to be too large
to be contained entirely within PermissionContextBase, so it has been
moved to its own header and implementation.

Test coverage is supplied in permission_context_base_unitttest.cc, but
there will be follow up unit tests for this class.

BUG=561867

Review-Url: https://codereview.chromium.org/2626853002
Cr-Commit-Position: refs/heads/master@{#442805}
Committed:
https://chromium.googlesource.com/chromium/src/+/397bca8885b9bd50a808517fdb93...
==========

[commit-bot: I haz the power, 2017-01-11 05:05:00]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/397bca8885b9bd50a808517fdb93...