Make CloudPolicyValidator memory management clearer

Make CloudPolicyValidator memory management clearer

1. Create() becomes returning a unique_ptr instead of a raw
pointer.

2. StartValidation() becomes accepting the validator as a
unique_ptr argument instead of taking ownership over "this".
This should prevent from making bugs leading to memory
leaks or double freeing, which were hard to notice with the
old interface.

3. Minor cleanup related to C++11 features.

No change of behavior is expected to be introduced.

BUG=710679
TEST=existing tests

Review-Url: https://codereview.chromium.org/2817643002
Cr-Commit-Position: refs/heads/master@{#465579}
Committed: https://chromium.googlesource.com/chromium/src/+/18f3a62dabe898a03ad69569feb0ce37c53fc001

[emaxx, 2017-04-12 01:15:31]

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

[commit-bot: I haz the power, 2017-04-12 01:16:02]

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

[emaxx, 2017-04-12 01:32:08]

Description was changed from

==========
Cleaner memory management in CloudPolicyValidator

BUG=710679
==========

to

==========
Make CloudPolicyValidator memory management clearer

1. Create() becomes returning a unique_ptr instead of raw pointer.

2. StartValidation() becomes accepting the validator as a
unique_ptr argument instead of taking ownership over "this".

3. Minor cleanup related to C++11 features.

BUG=710679
==========

[emaxx, 2017-04-12 01:33:21]

Description was changed from

==========
Make CloudPolicyValidator memory management clearer

1. Create() becomes returning a unique_ptr instead of raw pointer.

2. StartValidation() becomes accepting the validator as a
unique_ptr argument instead of taking ownership over "this".

3. Minor cleanup related to C++11 features.

BUG=710679
==========

to

==========
Make CloudPolicyValidator memory management clearer

1. Create() becomes returning a unique_ptr instead of raw pointer.

2. StartValidation() becomes accepting the validator as a
unique_ptr argument instead of taking ownership over "this".

3. Minor cleanup related to C++11 features.

No change of behavior is expected to be introduced.

BUG=710679
TEST=existing tests
==========

[emaxx, 2017-04-12 01:34:17]

Description was changed from

==========
Make CloudPolicyValidator memory management clearer

1. Create() becomes returning a unique_ptr instead of raw pointer.

2. StartValidation() becomes accepting the validator as a
unique_ptr argument instead of taking ownership over "this".

3. Minor cleanup related to C++11 features.

No change of behavior is expected to be introduced.

BUG=710679
TEST=existing tests
==========

to

==========
Make CloudPolicyValidator memory management clearer

1. Create() becomes returning a unique_ptr instead of a raw
pointer.

2. StartValidation() becomes accepting the validator as a
unique_ptr argument instead of taking ownership over "this".

3. Minor cleanup related to C++11 features.

No change of behavior is expected to be introduced.

BUG=710679
TEST=existing tests
==========

[emaxx, 2017-04-12 01:36:06]

Description was changed from

==========
Make CloudPolicyValidator memory management clearer

1. Create() becomes returning a unique_ptr instead of a raw
pointer.

2. StartValidation() becomes accepting the validator as a
unique_ptr argument instead of taking ownership over "this".

3. Minor cleanup related to C++11 features.

No change of behavior is expected to be introduced.

BUG=710679
TEST=existing tests
==========

to

==========
Make CloudPolicyValidator memory management clearer

1. Create() becomes returning a unique_ptr instead of a raw
pointer.

2. StartValidation() becomes accepting the validator as a
unique_ptr argument instead of taking ownership over "this".
This should prevent from making bugs leading to memory
leaks or double freeing, which were hard to notice with the
old interface.

3. Minor cleanup related to C++11 features.

No change of behavior is expected to be introduced.

BUG=710679
TEST=existing tests
==========

[emaxx, 2017-04-12 01:37:15]

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

[commit-bot: I haz the power, 2017-04-12 01:37:25]

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

[emaxx, 2017-04-12 02:26:26]

emaxx@chromium.org changed reviewers:
+ tnagel@chromium.org

[emaxx, 2017-04-12 02:26:27]

Thiemo, PTAL.

[commit-bot: I haz the power, 2017-04-12 02:55:53]

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

[commit-bot: I haz the power, 2017-04-12 02:55:54]

Dry run: This issue passed the CQ dry run.

[Thiemo Nagel, 2017-04-19 12:21:44]

Thank you for this CL!  Definitely helps the readability of the code.

LGTM % nits.

https://codereview.chromium.org/2817643002/diff/20001/components/policy/core/...
File components/policy/core/common/cloud/cloud_policy_validator.h (right):

https://codereview.chromium.org/2817643002/diff/20001/components/policy/core/...
components/policy/core/common/cloud/cloud_policy_validator.h:350: using
CompletionCallback = base::Callback<void(CloudPolicyValidator*)>;
Nit: Could this be moved to the base class because it's not templatized?

https://codereview.chromium.org/2817643002/diff/20001/components/policy/core/...
components/policy/core/common/cloud/cloud_policy_validator.h:370: static void
StartValidation(std::unique_ptr<CloudPolicyValidator> validator,
Nit: Could this be moved to the base class because it's not templatized?

[emaxx, 2017-04-19 12:42:49]

On 2017/04/19 12:21:44, Thiemo Nagel wrote:
> Thank you for this CL!  Definitely helps the readability of the code.

Thanks! Yes, the old interface was tricky, and a memory leak was almost
introduced in one of the recent CLs - that's why I decided to improve this
sooner rather than later.

https://codereview.chromium.org/2817643002/diff/20001/components/policy/core/...
File components/policy/core/common/cloud/cloud_policy_validator.h (right):

https://codereview.chromium.org/2817643002/diff/20001/components/policy/core/...
components/policy/core/common/cloud/cloud_policy_validator.h:350: using
CompletionCallback = base::Callback<void(CloudPolicyValidator*)>;
On 2017/04/19 12:21:43, Thiemo Nagel wrote:
> Nit: Could this be moved to the base class because it's not templatized?

This definition is actually depending on the template parameter: the function
parameter is CloudPolicyValidator<PayloadProto> but not
CloudPolicyValidatorBase.
This is used for the callbacks that want to access the payload using the
payload() member.

https://codereview.chromium.org/2817643002/diff/20001/components/policy/core/...
components/policy/core/common/cloud/cloud_policy_validator.h:370: static void
StartValidation(std::unique_ptr<CloudPolicyValidator> validator,
On 2017/04/19 12:21:43, Thiemo Nagel wrote:
> Nit: Could this be moved to the base class because it's not templatized?

Same as above: it's actually templatized due to CompletionCallback and
std::unique_ptr<CloudPolicyValidator> (I think unique_ptr is not capable of
child-to-parent cast).

[Thiemo Nagel, 2017-04-19 12:45:04]

Thanks for the explanation!  Lgtm.

[emaxx, 2017-04-19 12:45:44]

The CQ bit was checked by emaxx@chromium.org

[commit-bot: I haz the power, 2017-04-19 12:46:05]

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-04-19 13:37:12]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1492605944428840,
"parent_rev": "f38129c81c161fdd52874bf178b9110427a8c2ed", "commit_rev":
"18f3a62dabe898a03ad69569feb0ce37c53fc001"}

[commit-bot: I haz the power, 2017-04-19 13:37:20]

Description was changed from

==========
Make CloudPolicyValidator memory management clearer

1. Create() becomes returning a unique_ptr instead of a raw
pointer.

2. StartValidation() becomes accepting the validator as a
unique_ptr argument instead of taking ownership over "this".
This should prevent from making bugs leading to memory
leaks or double freeing, which were hard to notice with the
old interface.

3. Minor cleanup related to C++11 features.

No change of behavior is expected to be introduced.

BUG=710679
TEST=existing tests
==========

to

==========
Make CloudPolicyValidator memory management clearer

1. Create() becomes returning a unique_ptr instead of a raw
pointer.

2. StartValidation() becomes accepting the validator as a
unique_ptr argument instead of taking ownership over "this".
This should prevent from making bugs leading to memory
leaks or double freeing, which were hard to notice with the
old interface.

3. Minor cleanup related to C++11 features.

No change of behavior is expected to be introduced.

BUG=710679
TEST=existing tests

Review-Url: https://codereview.chromium.org/2817643002
Cr-Commit-Position: refs/heads/master@{#465579}
Committed:
https://chromium.googlesource.com/chromium/src/+/18f3a62dabe898a03ad69569feb0...
==========

[commit-bot: I haz the power, 2017-04-19 13:37:21]

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