[subresource_filter] Move throttle insertion into the client

[subresource_filter] Move throttle insertion into the client

This patch also  makes the SB throttle pass-through if there isn't a valid
DB manager.

BUG=717590
Review-Url: https://codereview.chromium.org/2858483003
Cr-Commit-Position: refs/heads/master@{#469388}
Committed: https://chromium.googlesource.com/chromium/src/+/77bf83460738f6b1465d016ccfc26e930babfc00

[Charlie Harrison, 2017-05-02 06:08:06]

The CQ bit was checked by csharrison@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-02 06:08:29]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Charlie Harrison, 2017-05-02 06:10:19]

The CQ bit was checked by csharrison@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-02 06:10:31]

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-05-02 07:07:37]

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

[commit-bot: I haz the power, 2017-05-02 07:07:38]

Dry run: This issue passed the CQ dry run.

[Charlie Harrison, 2017-05-02 16:20:36]

The CQ bit was checked by csharrison@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-02 16:20:54]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Charlie Harrison, 2017-05-02 16:24:01]

csharrison@chromium.org changed reviewers:
+ bmcquade@chromium.org, engedy@chromium.org

[Charlie Harrison, 2017-05-02 16:24:02]

engedy, bmcquade: PTAL?

Here's the second step of the plan.

[Charlie Harrison, 2017-05-02 16:46:43]

Description was changed from

==========
[subresource_filter] Move throttle insertion into the client

This patch also  makes the SB throttle pass-through if there isn't a valid
DB manager.

BUG=todo
==========

to

==========
[subresource_filter] Move throttle insertion into the client

This patch also  makes the SB throttle pass-through if there isn't a valid
DB manager.

BUG=717590
==========

[commit-bot: I haz the power, 2017-05-02 17:32:30]

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

[commit-bot: I haz the power, 2017-05-02 17:32:31]

Dry run: This issue passed the CQ dry run.

[engedy, 2017-05-02 18:21:54]

LGTM % comments.

https://codereview.chromium.org/2858483003/diff/40001/chrome/browser/subresou...
File chrome/browser/subresource_filter/chrome_subresource_filter_client.cc
(right):

https://codereview.chromium.org/2858483003/diff/40001/chrome/browser/subresou...
chrome/browser/subresource_filter/chrome_subresource_filter_client.cc:11: 
nit: Accidental blank line?

https://codereview.chromium.org/2858483003/diff/40001/chrome/browser/subresou...
chrome/browser/subresource_filter/chrome_subresource_filter_client.cc:48: if
(navigation_handle->IsInMainFrame()) {
Given that we are delaying the navigation, do we want to put all this behind the
main feature toggle at least?

https://codereview.chromium.org/2858483003/diff/40001/chrome/browser/subresou...
chrome/browser/subresource_filter/chrome_subresource_filter_client.cc:60:
base::MakeUnique<subresource_filter::
nit: #include "base/memory/ptr_util.h"

https://codereview.chromium.org/2858483003/diff/40001/components/subresource_...
File
components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.cc
(right):

https://codereview.chromium.org/2858483003/diff/40001/components/subresource_...
components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.cc:115:
database_client_ = std::move(database_client);
nit: Does .reset() work for unique_ptr's with custom deleters?

[Charlie Harrison, 2017-05-02 20:07:06]

The CQ bit was checked by csharrison@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-02 20:07:40]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Charlie Harrison, 2017-05-02 20:08:49]

Thanks,

bmcquade PTAL?

https://codereview.chromium.org/2858483003/diff/40001/chrome/browser/subresou...
File chrome/browser/subresource_filter/chrome_subresource_filter_client.cc
(right):

https://codereview.chromium.org/2858483003/diff/40001/chrome/browser/subresou...
chrome/browser/subresource_filter/chrome_subresource_filter_client.cc:11: 
On 2017/05/02 18:21:54, engedy wrote:
> nit: Accidental blank line?

Yep, removed.

https://codereview.chromium.org/2858483003/diff/40001/chrome/browser/subresou...
chrome/browser/subresource_filter/chrome_subresource_filter_client.cc:48: if
(navigation_handle->IsInMainFrame()) {
On 2017/05/02 18:21:54, engedy wrote:
> Given that we are delaying the navigation, do we want to put all this behind
the
> main feature toggle at least?

Done, using the activation_scope of the current active configuration.

https://codereview.chromium.org/2858483003/diff/40001/chrome/browser/subresou...
chrome/browser/subresource_filter/chrome_subresource_filter_client.cc:60:
base::MakeUnique<subresource_filter::
On 2017/05/02 18:21:54, engedy wrote:
> nit: #include "base/memory/ptr_util.h"

Done.

https://codereview.chromium.org/2858483003/diff/40001/components/subresource_...
File
components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.cc
(right):

https://codereview.chromium.org/2858483003/diff/40001/components/subresource_...
components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.cc:115:
database_client_ = std::move(database_client);
On 2017/05/02 18:21:54, engedy wrote:
> nit: Does .reset() work for unique_ptr's with custom deleters?

Nope, but you can use the move equality operator. I simplified this to use a
ternary op in the initializer list though.

[Charlie Harrison, 2017-05-02 20:41:48]

FYI need to wait for https://codereview.chromium.org/2814733002/ to land before
the third part of this work can be finished.

[commit-bot: I haz the power, 2017-05-02 21:55:20]

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

[commit-bot: I haz the power, 2017-05-02 21:55:21]

Dry run: This issue passed the CQ dry run.

[Bryan McQuade, 2017-05-04 13:48:42]

lgtm

[Charlie Harrison, 2017-05-04 14:30:13]

The CQ bit was checked by csharrison@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-04 14:30:31]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Charlie Harrison, 2017-05-04 14:57:23]

The CQ bit was unchecked by csharrison@chromium.org

[Charlie Harrison, 2017-05-04 14:57:23]

The CQ bit was checked by csharrison@chromium.org

[Charlie Harrison, 2017-05-04 14:57:23]

The patchset sent to the CQ was uploaded after l-g-t-m from engedy@chromium.org,
bmcquade@chromium.org
Link to the patchset: https://codereview.chromium.org/2858483003/#ps80001
(title: "git cl try")

[commit-bot: I haz the power, 2017-05-04 14:57:31]

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-05-04 16:03:25]

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

[commit-bot: I haz the power, 2017-05-04 16:03:28]

Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Charlie Harrison, 2017-05-04 16:56:33]

The CQ bit was checked by csharrison@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-04 16:56:54]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Charlie Harrison, 2017-05-04 16:58:02]

The CQ bit was checked by csharrison@chromium.org

[Charlie Harrison, 2017-05-04 16:58:03]

The CQ bit was unchecked by csharrison@chromium.org

[Charlie Harrison, 2017-05-04 16:58:03]

The patchset sent to the CQ was uploaded after l-g-t-m from
bmcquade@chromium.org, engedy@chromium.org
Link to the patchset: https://codereview.chromium.org/2858483003/#ps100001
(title: "fix rebase")

[commit-bot: I haz the power, 2017-05-04 16:58:21]

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-05-04 18:23:15]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1493917082914990,
"parent_rev": "7093bce085613efd785b72f2a66ee8be27f7d6ed", "commit_rev":
"77bf83460738f6b1465d016ccfc26e930babfc00"}

[commit-bot: I haz the power, 2017-05-04 18:23:27]

Description was changed from

==========
[subresource_filter] Move throttle insertion into the client

This patch also  makes the SB throttle pass-through if there isn't a valid
DB manager.

BUG=717590
==========

to

==========
[subresource_filter] Move throttle insertion into the client

This patch also  makes the SB throttle pass-through if there isn't a valid
DB manager.

BUG=717590

Review-Url: https://codereview.chromium.org/2858483003
Cr-Commit-Position: refs/heads/master@{#469388}
Committed:
https://chromium.googlesource.com/chromium/src/+/77bf83460738f6b1465d016ccfc2...
==========

[commit-bot: I haz the power, 2017-05-04 18:23:30]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/77bf83460738f6b1465d016ccfc2...