Use std::unique_ptr to express ownership of base::Value in PolicyMap::Entry

Use std::unique_ptr to express ownership of base::Value in PolicyMap::Entry

BUG=581865
Committed: https://crrev.com/3b344bc2793161243e2718cc02a311143440396f
Cr-Commit-Position: refs/heads/master@{#392521}

[dcheng, 2016-05-03 06:17:37]

Description was changed from

==========
Moo

BUG=581865
==========

to

==========
Use std::unique_ptr to express ownership of base::Value in PolicyMap::Entry

BUG=581865
==========

[dcheng, 2016-05-03 06:17:38]

dcheng@chromium.org changed reviewers:
+ atwilson@chromium.org, jochen@chromium.org, joedow@chromium.org

[dcheng, 2016-05-03 06:46:18]

+atwilson for //chrome/browser/chromeos/policy and //components/policy
+jochen for //chrome and //components files outside the previous two directories
+joedow for //remoting

This started as a simple cleanup CL to get rid of some calls to
base::Value::DeepCopy(). I didn't realize just how big this CL would be until I
was hundreds of lines in =(

https://codereview.chromium.org/1940153002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/device_policy_decoder_chromeos.cc (right):

https://codereview.chromium.org/1940153002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/device_policy_decoder_chromeos.cc:470: //
TODO(dcheng): Shouldn't this use DecodeIntegerValue?
This looks pretty sketchy. Can I assign this TODO to someone who works on
policy?

https://codereview.chromium.org/1940153002/diff/40001/components/policy/core/...
File components/policy/core/common/policy_map.cc (right):

https://codereview.chromium.org/1940153002/diff/40001/components/policy/core/...
components/policy/core/common/policy_map.cc:89: map_[policy] = std::move(entry);
Added this helper to avoid duplicating a logical copy of DeepCopy() in
CopyFrom() and MergeFrom(). It doesn't have much use outside PolicyMap though,
so if you like, I can make it private.

https://codereview.chromium.org/1940153002/diff/40001/components/policy/core/...
components/policy/core/common/policy_map.cc:107: std::unique_ptr<PolicyMap>
copy(new PolicyMap());
In theory, we could make this movable as well, but that is out of scope of the
base::Value work.

https://codereview.chromium.org/1940153002/diff/40001/components/policy/core/...
File components/policy/core/common/policy_map.h (right):

https://codereview.chromium.org/1940153002/diff/40001/components/policy/core/...
components/policy/core/common/policy_map.h:32:
std::unique_ptr<ExternalDataFetcher> external_data_fetcher;
These pointers logically own the values, so they ought to be std::unique_ptr.

https://codereview.chromium.org/1940153002/diff/40001/components/policy/core/...
components/policy/core/common/policy_map.h:44: Entry DeepCopy() const;
Since Entry is now movable, we may as well take advantage of it for DeepCopy(),
rather than indirecting through std::unique_ptr and then moving the result out.

This means we can write:
  map_entry = entry->DeepCopy();

Rather than:
  map_entry = std::move(*entry->DeepCopy());

[jochen (gone - plz use gerrit), 2016-05-03 10:59:44]

lgtm

[Andrew T Wilson (Slow), 2016-05-03 11:59:04]

*/policy/* LGTM with nits and want to make sure that TODO() is addressed to your
satisfaction.

https://codereview.chromium.org/1940153002/diff/80001/components/policy/core/...
File components/policy/core/browser/proxy_policy_handler_unittest.cc (right):

https://codereview.chromium.org/1940153002/diff/80001/components/policy/core/...
components/policy/core/browser/proxy_policy_handler_unittest.cc:287:
key::kProxyPacUrl, POLICY_LEVEL_MANDATORY, POLICY_SCOPE_USER,
I think if you are wrapping after the open paren, you are supposed to put
one-param-per-line.
https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#...

Here and below also.

https://codereview.chromium.org/1940153002/diff/80001/components/policy/core/...
File components/policy/core/common/policy_loader_win.cc (right):

https://codereview.chromium.org/1940153002/diff/80001/components/policy/core/...
components/policy/core/common/policy_loader_win.cc:173: // when I'm more awake.
What's the concern here?

[joedow, 2016-05-03 15:35:43]

lgtm for remoting

[dcheng, 2016-05-03 23:30:09]

The CQ bit was checked by dcheng@chromium.org

[dcheng, 2016-05-03 23:30:10]

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

[commit-bot: I haz the power, 2016-05-03 23:30:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1940153002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1940153002/140001

[dcheng, 2016-05-03 23:32:23]

The CQ bit was unchecked by dcheng@chromium.org

[dcheng, 2016-05-03 23:40:40]

atwilson: PTAL

https://codereview.chromium.org/1940153002/diff/80001/components/policy/core/...
File components/policy/core/browser/proxy_policy_handler_unittest.cc (right):

https://codereview.chromium.org/1940153002/diff/80001/components/policy/core/...
components/policy/core/browser/proxy_policy_handler_unittest.cc:287:
key::kProxyPacUrl, POLICY_LEVEL_MANDATORY, POLICY_SCOPE_USER,
On 2016/05/03 at 11:59:03, Andrew T Wilson (Slow) wrote:
> I think if you are wrapping after the open paren, you are supposed to put
one-param-per-line.
https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#...
> 
> Here and below also.

This is from clang-format. I believe that this is permissible according to the
style guide: there's an example where this is true, but the list of guidelines
doesn't seem to explicitly prohibit this either.

Note that there's one statement that would rule this out, but this particular
guideline is also at odds with one of the examples provided in the same section.
Specifically:
All parameters should be aligned if possible.

Doesn't seem compatible with the second example:
ReturnType ClassName::ReallyLongFunctionName(Type par_name1, Type par_name2,
                                             Type par_name3) {
  DoSomething();
  ...
}

In light of this, I'd prefer to keep whatever clang-format came up with.

https://codereview.chromium.org/1940153002/diff/80001/components/policy/core/...
File components/policy/core/common/policy_loader_win.cc (right):

https://codereview.chromium.org/1940153002/diff/80001/components/policy/core/...
components/policy/core/common/policy_loader_win.cc:173: // when I'm more awake.
On 2016/05/03 at 11:59:03, Andrew T Wilson (Slow) wrote:
> What's the concern here?

ExternalDataFetcher is now an owned pointer, so it can't just be passed around
like this. Ordinarily, I would just transfer ownership, but that's not possible
here: PolicyMap::Get() returns a const Entry*.

Instead, I opted to just use Entry::DeepCopy() to build a new entry with the
correct filtered values. From a pure efficiency standpoint, it's probably a bit
suboptimal (since it clones the original Value and then immediately replaces it
with the filtered one), but I figure this code shouldn't be on the hot path
anyway, so the cost shouldn't be too large.

https://codereview.chromium.org/1940153002/diff/140001/components/policy/core...
File components/policy/core/common/policy_service_impl.cc (right):

https://codereview.chromium.org/1940153002/diff/140001/components/policy/core...
components/policy/core/common/policy_service_impl.cc:57: current_priority =
entry->DeepCopy();
Like policy_loader_win.cc, this means we'll end up making a pointless copy of
the base::Value. There's not really anyway around this unless we're willing to
make PolicyMap::Get() return a non-const Entry*.

[dcheng, 2016-05-09 20:02:41]

atwilson: ping

[Andrew T Wilson (Slow), 2016-05-09 22:08:21]

On 2016/05/09 20:02:41, dcheng wrote:
> atwilson: ping

still lgtm :)

[dcheng, 2016-05-10 01:37:00]

The CQ bit was checked by dcheng@chromium.org

[dcheng, 2016-05-10 01:37:00]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
joedow@chromium.org
Link to the patchset: https://codereview.chromium.org/1940153002/#ps200001
(title: "another-fix")

[commit-bot: I haz the power, 2016-05-10 01:37:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1940153002/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1940153002/200001

[commit-bot: I haz the power, 2016-05-10 02:26:27]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2016-05-10 02:27:33]

Description was changed from

==========
Use std::unique_ptr to express ownership of base::Value in PolicyMap::Entry

BUG=581865
==========

to

==========
Use std::unique_ptr to express ownership of base::Value in PolicyMap::Entry

BUG=581865

Committed: https://crrev.com/3b344bc2793161243e2718cc02a311143440396f
Cr-Commit-Position: refs/heads/master@{#392521}
==========

[commit-bot: I haz the power, 2016-05-10 02:27:36]

Patchset 11 (id:??) landed as
https://crrev.com/3b344bc2793161243e2718cc02a311143440396f
Cr-Commit-Position: refs/heads/master@{#392521}