Swap ownership of RulesetService and the content delegate

Swap ownership of RulesetService and the content delegate

This patch also adds a getter to the content delegate on the browser
process. This is in preparation for a chrome client to pull the
verified ruleset dealer handle off the delegate.

BUG=637415
Review-Url: https://codereview.chromium.org/2731283009
Cr-Commit-Position: refs/heads/master@{#456065}
Committed: https://chromium.googlesource.com/chromium/src/+/8a923d69f115eb6c5e48f548feea0afa30d8f4b9

[Charlie Harrison, 2017-03-08 18:56:55]

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

[commit-bot: I haz the power, 2017-03-08 18:57:55]

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-03-08 18:59:44]

Description was changed from

==========
Swap ownership of RulesetService and the content delegate

BUG=637415
==========

to

==========
Swap ownership of RulesetService and the content delegate

This patch also adds a getter to the content delegate on the browser
process. This is in preparation for a chrome client to pull the
verified ruleset dealer handle off the delegate.

BUG=637415
==========

[commit-bot: I haz the power, 2017-03-08 19:15:37]

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

[commit-bot: I haz the power, 2017-03-08 19:15:38]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Charlie Harrison, 2017-03-08 19:30:44]

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

[commit-bot: I haz the power, 2017-03-08 19:31:49]

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-03-08 20:03:51]

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

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

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Charlie Harrison, 2017-03-08 20:33:35]

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

[commit-bot: I haz the power, 2017-03-08 20:34:04]

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-03-08 21:18:17]

csharrison@chromium.org changed reviewers:
+ pkalinnikov@chromium.org

[Charlie Harrison, 2017-03-08 21:18:18]

Pavel, this is the idea for swapping the ownership model of the content delegate
and the rule service.

I used a set_ruleset_service() method on the delegate to allow inserting
rulesets in test code.

[commit-bot: I haz the power, 2017-03-08 21:31:06]

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

[commit-bot: I haz the power, 2017-03-08 21:31:07]

Dry run: This issue passed the CQ dry run.

[Charlie Harrison, 2017-03-09 16:00:46]

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

[commit-bot: I haz the power, 2017-03-09 16:01:11]

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-03-09 16:06:40]

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

[commit-bot: I haz the power, 2017-03-09 16:07:04]

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-03-09 16:07:58]

+engedy cc (FYI)

The patch now includes the rename
s/ContentRulesetServiceDelegate/ContentRulesetService as discussed offline.

[pkalinnikov, 2017-03-09 16:09:01]

Flushing comments to patch#3.

https://codereview.chromium.org/2731283009/diff/40001/components/subresource_...
File components/subresource_filter/core/browser/ruleset_service.h (right):

https://codereview.chromium.org/2731283009/diff/40001/components/subresource_...
components/subresource_filter/core/browser/ruleset_service.h:253: // Must
outlive this class.
nit: Must outlive |this| object.

https://codereview.chromium.org/2731283009/diff/40001/components/subresource_...
File components/subresource_filter/core/browser/ruleset_service_unittest.cc
(right):

https://codereview.chromium.org/2731283009/diff/40001/components/subresource_...
components/subresource_filter/core/browser/ruleset_service_unittest.cc:297:
std::unique_ptr<MockRulesetServiceDelegate> mock_delegate_;
Please swap the lines to ensure that |mock_delegate_| outlives |service_|.

[Charlie Harrison, 2017-03-09 16:10:42]

Thanks, let me quickly throw up a new PS responding to those comments.

[Charlie Harrison, 2017-03-09 16:12:44]

https://codereview.chromium.org/2731283009/diff/40001/components/subresource_...
File components/subresource_filter/core/browser/ruleset_service.h (right):

https://codereview.chromium.org/2731283009/diff/40001/components/subresource_...
components/subresource_filter/core/browser/ruleset_service.h:253: // Must
outlive this class.
On 2017/03/09 16:09:01, pkalinnikov wrote:
> nit: Must outlive |this| object.

Done.

https://codereview.chromium.org/2731283009/diff/40001/components/subresource_...
File components/subresource_filter/core/browser/ruleset_service_unittest.cc
(right):

https://codereview.chromium.org/2731283009/diff/40001/components/subresource_...
components/subresource_filter/core/browser/ruleset_service_unittest.cc:297:
std::unique_ptr<MockRulesetServiceDelegate> mock_delegate_;
On 2017/03/09 16:09:01, pkalinnikov wrote:
> Please swap the lines to ensure that |mock_delegate_| outlives |service_|.

Done.

[Charlie Harrison, 2017-03-09 16:12:47]

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

[commit-bot: I haz the power, 2017-03-09 16:13:29]

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-03-09 16:19:25]

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

[commit-bot: I haz the power, 2017-03-09 16:19:26]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[pkalinnikov, 2017-03-09 16:26:00]

LGTM % nit and forward declaration of RulesetService to make the bots happy.

https://codereview.chromium.org/2731283009/diff/80001/components/subresource_...
File components/subresource_filter/content/browser/content_ruleset_service.h
(right):

https://codereview.chromium.org/2731283009/diff/80001/components/subresource_...
components/subresource_filter/content/browser/content_ruleset_service.h:60: //
Forward calls to the underlying ruleset_service_.
nit: We prefer starting method comments with infinitives.
e.g.: Forwards the call to the underlying |ruleset_service_|.

[Charlie Harrison, 2017-03-09 16:33:49]

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

[commit-bot: I haz the power, 2017-03-09 16:34:38]

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-03-09 16:35:24]

csharrison@chromium.org changed reviewers:
+ jochen@chromium.org

[Charlie Harrison, 2017-03-09 16:35:25]

jochen, would you please look at //chrome? This is mostly a mechanical change.

https://codereview.chromium.org/2731283009/diff/80001/components/subresource_...
File components/subresource_filter/content/browser/content_ruleset_service.h
(right):

https://codereview.chromium.org/2731283009/diff/80001/components/subresource_...
components/subresource_filter/content/browser/content_ruleset_service.h:60: //
Forward calls to the underlying ruleset_service_.
On 2017/03/09 16:26:00, pkalinnikov wrote:
> nit: We prefer starting method comments with infinitives.
> e.g.: Forwards the call to the underlying |ruleset_service_|.

Done.

[commit-bot: I haz the power, 2017-03-09 17:30:46]

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

[commit-bot: I haz the power, 2017-03-09 17:30:46]

Dry run: This issue passed the CQ dry run.

[jochen (gone - plz use gerrit), 2017-03-10 10:44:07]

lgtm

[engedy, 2017-03-10 11:57:13]

engedy@chromium.org changed reviewers:
+ engedy@chromium.org

[engedy, 2017-03-10 11:57:14]

Thanks a lot for this refactoring, quick drive-by comment.

https://codereview.chromium.org/2731283009/diff/120001/chrome/browser/subreso...
File chrome/browser/subresource_filter/test_ruleset_publisher.cc (right):

https://codereview.chromium.org/2731283009/diff/120001/chrome/browser/subreso...
chrome/browser/subresource_filter/test_ruleset_publisher.cc:23: :
content_service_(static_cast<ContentRulesetService*>(
nit: This static_cast should no longer be needed.

[engedy, 2017-03-10 12:08:30]

engedy@chromium.org changed reviewers:
- pkalinnikov@chromium.org

[engedy, 2017-03-10 12:09:27]

engedy@chromium.org changed reviewers:
+ pkalinnikov@chromium.org
- engedy@chromium.org

[Charlie Harrison, 2017-03-10 14:25:36]

Thanks all

https://codereview.chromium.org/2731283009/diff/120001/chrome/browser/subreso...
File chrome/browser/subresource_filter/test_ruleset_publisher.cc (right):

https://codereview.chromium.org/2731283009/diff/120001/chrome/browser/subreso...
chrome/browser/subresource_filter/test_ruleset_publisher.cc:23: :
content_service_(static_cast<ContentRulesetService*>(
On 2017/03/10 11:57:14, engedy (slow) wrote:
> nit: This static_cast should no longer be needed.

Done. Good catch

[Charlie Harrison, 2017-03-10 14:26:01]

The CQ bit was checked by csharrison@chromium.org

[Charlie Harrison, 2017-03-10 14:26:01]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkalinnikov@chromium.org, jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/2731283009/#ps140001
(title: "engedy review")

[commit-bot: I haz the power, 2017-03-10 14:26:13]

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-03-10 15:42:18]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1489155961194680,
"parent_rev": "dfc151cf408ffc92bfdc1d40f06487f8a256bc0a", "commit_rev":
"8a923d69f115eb6c5e48f548feea0afa30d8f4b9"}

[commit-bot: I haz the power, 2017-03-10 15:42:56]

Description was changed from

==========
Swap ownership of RulesetService and the content delegate

This patch also adds a getter to the content delegate on the browser
process. This is in preparation for a chrome client to pull the
verified ruleset dealer handle off the delegate.

BUG=637415
==========

to

==========
Swap ownership of RulesetService and the content delegate

This patch also adds a getter to the content delegate on the browser
process. This is in preparation for a chrome client to pull the
verified ruleset dealer handle off the delegate.

BUG=637415

Review-Url: https://codereview.chromium.org/2731283009
Cr-Commit-Position: refs/heads/master@{#456065}
Committed:
https://chromium.googlesource.com/chromium/src/+/8a923d69f115eb6c5e48f548feea...
==========

[commit-bot: I haz the power, 2017-03-10 15:42:58]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/8a923d69f115eb6c5e48f548feea...