Use unique_ptr<DictionaryValue> in ProxyConfigDictionary

Use unique_ptr<DictionaryValue> in ProxyConfigDictionary

ProxyConfigDictionary uses raw pointers to DictionaryValue to pass
ownership. This CL changes that to unique_ptr:
 * to improve code clarity
 * to go one step towards removing the obsolete raw-pointer-based
   API of base::Value, and
 * to fix a leak in ProxyConfigServiceImpl::GetActiveProxyConfigDictionary.

The problem with ProxyConfigServiceImpl::GetActiveProxyConfigDictionary was
the line:
  return base::MakeUnique<ProxyConfigDictionary>(
        ProxyConfigDictionary::CreateDirect());

In the old code, CreateDirect() would return an owning raw pointer, but
the ProxyConfigDictionary constructor would only make a Value::DeepCopy of
it and let it be, never freeing that memory.

This CL fixes the issue by switching the result of CreateDirect to be a
unique_ptr. One way to do that would to do 

  return base::MakeUnique<ProxyConfigDictionary>(
        ProxyConfigDictionary::CreateDirect().get());

but that would be an unnecessary copy. Instead, this CL added a version
of the ProxyConfigDictionary constructor which accepts a unique_ptr
and avoids the copy. As a side effect, that particular line in
GetActiveProxyConfigDictionary did not have to be changed at all.

BUG=697817
TBR=bartfab@chromium.org

Review-Url: https://codereview.chromium.org/2785883003
Cr-Commit-Position: refs/heads/master@{#461076}
Committed: https://chromium.googlesource.com/chromium/src/+/6f6e2669be3bd68c883ebe591099a18666a36ba0

[vabr (Chromium), 2017-03-30 08:22:32]

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

[commit-bot: I haz the power, 2017-03-30 08:22:47]

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-30 09:14:31]

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

[commit-bot: I haz the power, 2017-03-30 09:14:32]

Dry run: This issue passed the CQ dry run.

[vabr (Chromium), 2017-03-30 09:48:00]

vabr@chromium.org changed reviewers:
+ bartfab@chromium.org, jdoerrie@chromium.org, jochen@chromium.org,
stevenjb@chromium.org

[vabr (Chromium), 2017-03-30 09:48:00]

Hi all,

stevenjb@ -- please review:
  * components/proxy_config/proxy_config_dictionary.*
  * the mechanical changes in the rest of components/proxy_config/*
  * the mechanical changes in chromeos/*

jdoerrie@ -- please review the whole patch.

jochen@ -- please rubberstamp chrome/browser/*

bartfab@ -- please rubberstamp
components/policy/core/browser/proxy_policy_handler.cc

Thanks!
Vaclav

[jdoerrie, 2017-03-30 11:00:39]

LGTM % nits, thanks!

Description Nit:
...
This needs to be changed to unique_ptr to make leaks like that in
ProxyConfigServiceImpl::GetActiveProxyConfigDictionary (last line)
[MISSING_WORD] and to make
...

https://codereview.chromium.org/2785883003/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/proxy/proxy_api_helpers.h (right):

https://codereview.chromium.org/2785883003/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/proxy/proxy_api_helpers.h:10: #include <string>
nit: add #include <memory>

https://codereview.chromium.org/2785883003/diff/1/chromeos/network/proxy/ui_p...
File chromeos/network/proxy/ui_proxy_config.h (right):

https://codereview.chromium.org/2785883003/diff/1/chromeos/network/proxy/ui_p...
chromeos/network/proxy/ui_proxy_config.h:8: #include <string>
Nit: add #include <memory>

https://codereview.chromium.org/2785883003/diff/1/components/proxy_config/pro...
File components/proxy_config/proxy_config_dictionary.cc (right):

https://codereview.chromium.org/2785883003/diff/1/components/proxy_config/pro...
components/proxy_config/proxy_config_dictionary.cc:139:
std::unique_ptr<base::DictionaryValue> dict =
nit: auto

[jochen (gone - plz use gerrit), 2017-03-30 11:50:26]

lgtm

[vabr (Chromium), 2017-03-30 12:02:08]

Description was changed from

==========
Use unique_ptr<DictionaryValue> in ProxyConfigDictionary

ProxyConfigDictionary uses raw pointers to DictionaryValue to pass ownership.
This needs to be changed to unique_ptr to make leaks like that in
ProxyConfigServiceImpl::GetActiveProxyConfigDictionary (last line) and to make
the code clear. It is also a blocker for removing the obsolete
raw-pointer-based API of base::Value.

This CL implements the change. It also fixes the leak in
ProxyConfigServiceImpl::GetActiveProxyConfigDictionary by adding a new
constructor for ProxyConfigDictionary which accepts unique_ptr. (The
alternative would be to add ".get()" at the callsite, but that would be less
efficient, resulting in a deep copy of the dictionary.)

BUG=697817
==========

to

==========
Use unique_ptr<DictionaryValue> in ProxyConfigDictionary

ProxyConfigDictionary uses raw pointers to DictionaryValue to pass
ownership. This CL changes that to unique_ptr:
 * to improve code clarity
 * to go one step towards removing the obsolete raw-pointer-based
   API of base::Value, and
 * to fix a leak in ProxyConfigServiceImpl::GetActiveProxyConfigDictionary.

The problem with ProxyConfigServiceImpl::GetActiveProxyConfigDictionary was
the line:
  return base::MakeUnique<ProxyConfigDictionary>(
        ProxyConfigDictionary::CreateDirect());

In the old code, CreateDicrect() would return an owning raw pointer, but
the ProxyConfigDictionary constructor would only make a Value::DeepCopy of
it and let it be, never freeing that memory.

This CL fixes the issue by switching the result of CreateDiret to be a
unique_ptr. One way to do that would to do 

  return base::MakeUnique<ProxyConfigDictionary>(
        ProxyConfigDictionary::CreateDirect().get());

but that would be an unnecessary copy. Instead, this CL added a version
of the ProxyConfigDictionary constructor which accepts a unique_ptr
and avoids the copy. As a side effect, GetActiveProxyConfigDictionary
did not have to be changed at all.

BUG=697817
==========

[vabr (Chromium), 2017-03-30 12:07:42]

Thanks for the review!

Remaining requests:

stevenjb@ -- please review:
  * components/proxy_config/proxy_config_dictionary.*
  * the mechanical changes in the rest of components/proxy_config/*
  * the mechanical changes in chromeos/*

bartfab@ -- please rubberstamp
components/policy/core/browser/proxy_policy_handler.cc

Cheers,
Vaclav

https://codereview.chromium.org/2785883003/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/proxy/proxy_api_helpers.h (right):

https://codereview.chromium.org/2785883003/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/proxy/proxy_api_helpers.h:10: #include <string>
On 2017/03/30 11:00:39, jdoerrie wrote:
> nit: add #include <memory>

Done.

https://codereview.chromium.org/2785883003/diff/1/chromeos/network/proxy/ui_p...
File chromeos/network/proxy/ui_proxy_config.h (right):

https://codereview.chromium.org/2785883003/diff/1/chromeos/network/proxy/ui_p...
chromeos/network/proxy/ui_proxy_config.h:8: #include <string>
On 2017/03/30 11:00:39, jdoerrie wrote:
> Nit: add #include <memory>

Done.

https://codereview.chromium.org/2785883003/diff/1/components/proxy_config/pro...
File components/proxy_config/proxy_config_dictionary.cc (right):

https://codereview.chromium.org/2785883003/diff/1/components/proxy_config/pro...
components/proxy_config/proxy_config_dictionary.cc:139:
std::unique_ptr<base::DictionaryValue> dict =
On 2017/03/30 11:00:39, jdoerrie wrote:
> nit: auto

Done.

https://codereview.chromium.org/2785883003/diff/20001/components/proxy_config...
File components/proxy_config/proxy_config_dictionary.h (right):

https://codereview.chromium.org/2785883003/diff/20001/components/proxy_config...
components/proxy_config/proxy_config_dictionary.h:34: explicit
ProxyConfigDictionary(std::unique_ptr<base::DictionaryValue> dict);
stevenjb@ -- if you are wondering why I added this constructor, please see the
discussion in the CL; I'm happy to implement alternatives if you want me to.

[vabr (Chromium), 2017-03-30 12:07:56]

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

[commit-bot: I haz the power, 2017-03-30 12:08:11]

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-30 13:00:37]

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

[commit-bot: I haz the power, 2017-03-30 13:00:38]

Dry run: This issue passed the CQ dry run.

[stevenjb, 2017-03-30 17:00:58]

https://codereview.chromium.org/2785883003/diff/20001/components/proxy_config...
File components/proxy_config/proxy_config_dictionary.h (right):

https://codereview.chromium.org/2785883003/diff/20001/components/proxy_config...
components/proxy_config/proxy_config_dictionary.h:34: explicit
ProxyConfigDictionary(std::unique_ptr<base::DictionaryValue> dict);
On 2017/03/30 12:07:41, vabr (Chromium) wrote:
> stevenjb@ -- if you are wondering why I added this constructor, please see the
> discussion in the CL; I'm happy to implement alternatives if you want me to.

I'm a little uncomfortable with having two such similar constructors.

How bad would it be to remove the current constructor and use CreateDeepCopy()
to do any copies at the call sites?

[vabr (Chromium), 2017-03-30 17:04:21]

https://codereview.chromium.org/2785883003/diff/20001/components/proxy_config...
File components/proxy_config/proxy_config_dictionary.h (right):

https://codereview.chromium.org/2785883003/diff/20001/components/proxy_config...
components/proxy_config/proxy_config_dictionary.h:34: explicit
ProxyConfigDictionary(std::unique_ptr<base::DictionaryValue> dict);
On 2017/03/30 17:00:58, stevenjb wrote:
> On 2017/03/30 12:07:41, vabr (Chromium) wrote:
> > stevenjb@ -- if you are wondering why I added this constructor, please see
the
> > discussion in the CL; I'm happy to implement alternatives if you want me to.
> 
> I'm a little uncomfortable with having two such similar constructors.
> 
> How bad would it be to remove the current constructor and use CreateDeepCopy()
> to do any copies at the call sites?

That's no problem. I like that idea better than the 2 constructors. Not at my
developers machine at the moment, but as soon as I am, I will do that and ping
this CL.

[vabr (Chromium), 2017-03-30 19:09:33]

Description was changed from

==========
Use unique_ptr<DictionaryValue> in ProxyConfigDictionary

ProxyConfigDictionary uses raw pointers to DictionaryValue to pass
ownership. This CL changes that to unique_ptr:
 * to improve code clarity
 * to go one step towards removing the obsolete raw-pointer-based
   API of base::Value, and
 * to fix a leak in ProxyConfigServiceImpl::GetActiveProxyConfigDictionary.

The problem with ProxyConfigServiceImpl::GetActiveProxyConfigDictionary was
the line:
  return base::MakeUnique<ProxyConfigDictionary>(
        ProxyConfigDictionary::CreateDirect());

In the old code, CreateDicrect() would return an owning raw pointer, but
the ProxyConfigDictionary constructor would only make a Value::DeepCopy of
it and let it be, never freeing that memory.

This CL fixes the issue by switching the result of CreateDiret to be a
unique_ptr. One way to do that would to do 

  return base::MakeUnique<ProxyConfigDictionary>(
        ProxyConfigDictionary::CreateDirect().get());

but that would be an unnecessary copy. Instead, this CL added a version
of the ProxyConfigDictionary constructor which accepts a unique_ptr
and avoids the copy. As a side effect, GetActiveProxyConfigDictionary
did not have to be changed at all.

BUG=697817
==========

to

==========
Use unique_ptr<DictionaryValue> in ProxyConfigDictionary

ProxyConfigDictionary uses raw pointers to DictionaryValue to pass
ownership. This CL changes that to unique_ptr:
 * to improve code clarity
 * to go one step towards removing the obsolete raw-pointer-based
   API of base::Value, and
 * to fix a leak in ProxyConfigServiceImpl::GetActiveProxyConfigDictionary.

The problem with ProxyConfigServiceImpl::GetActiveProxyConfigDictionary was
the line:
  return base::MakeUnique<ProxyConfigDictionary>(
        ProxyConfigDictionary::CreateDirect());

In the old code, CreateDirect() would return an owning raw pointer, but
the ProxyConfigDictionary constructor would only make a Value::DeepCopy of
it and let it be, never freeing that memory.

This CL fixes the issue by switching the result of CreateDirect to be a
unique_ptr. One way to do that would to do 

  return base::MakeUnique<ProxyConfigDictionary>(
        ProxyConfigDictionary::CreateDirect().get());

but that would be an unnecessary copy. Instead, this CL added a version
of the ProxyConfigDictionary constructor which accepts a unique_ptr
and avoids the copy. As a side effect, GetActiveProxyConfigDictionary
did not have to be changed at all.

BUG=697817
==========

[vabr (Chromium), 2017-03-30 19:12:34]

Description was changed from

==========
Use unique_ptr<DictionaryValue> in ProxyConfigDictionary

ProxyConfigDictionary uses raw pointers to DictionaryValue to pass
ownership. This CL changes that to unique_ptr:
 * to improve code clarity
 * to go one step towards removing the obsolete raw-pointer-based
   API of base::Value, and
 * to fix a leak in ProxyConfigServiceImpl::GetActiveProxyConfigDictionary.

The problem with ProxyConfigServiceImpl::GetActiveProxyConfigDictionary was
the line:
  return base::MakeUnique<ProxyConfigDictionary>(
        ProxyConfigDictionary::CreateDirect());

In the old code, CreateDirect() would return an owning raw pointer, but
the ProxyConfigDictionary constructor would only make a Value::DeepCopy of
it and let it be, never freeing that memory.

This CL fixes the issue by switching the result of CreateDirect to be a
unique_ptr. One way to do that would to do 

  return base::MakeUnique<ProxyConfigDictionary>(
        ProxyConfigDictionary::CreateDirect().get());

but that would be an unnecessary copy. Instead, this CL added a version
of the ProxyConfigDictionary constructor which accepts a unique_ptr
and avoids the copy. As a side effect, GetActiveProxyConfigDictionary
did not have to be changed at all.

BUG=697817
==========

to

==========
Use unique_ptr<DictionaryValue> in ProxyConfigDictionary

ProxyConfigDictionary uses raw pointers to DictionaryValue to pass
ownership. This CL changes that to unique_ptr:
 * to improve code clarity
 * to go one step towards removing the obsolete raw-pointer-based
   API of base::Value, and
 * to fix a leak in ProxyConfigServiceImpl::GetActiveProxyConfigDictionary.

The problem with ProxyConfigServiceImpl::GetActiveProxyConfigDictionary was
the line:
  return base::MakeUnique<ProxyConfigDictionary>(
        ProxyConfigDictionary::CreateDirect());

In the old code, CreateDirect() would return an owning raw pointer, but
the ProxyConfigDictionary constructor would only make a Value::DeepCopy of
it and let it be, never freeing that memory.

This CL fixes the issue by switching the result of CreateDirect to be a
unique_ptr. One way to do that would to do 

  return base::MakeUnique<ProxyConfigDictionary>(
        ProxyConfigDictionary::CreateDirect().get());

but that would be an unnecessary copy. Instead, this CL added a version
of the ProxyConfigDictionary constructor which accepts a unique_ptr
and avoids the copy. As a side effect, that particular line in
GetActiveProxyConfigDictionary did not have to be changed at all.

BUG=697817
==========

[vabr (Chromium), 2017-03-30 19:23:49]

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

[vabr (Chromium), 2017-03-30 19:24:33]

Hi Steven,

I removed the old constructor as you suggested.

Please have a look.

Cheers,
Vaclav

[commit-bot: I haz the power, 2017-03-30 19:24:41]

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-30 19:49:24]

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

[commit-bot: I haz the power, 2017-03-30 19:49:25]

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

[stevenjb, 2017-03-30 20:39:55]

Thanks, i think this is more clear (and fewer copies in tests :))

https://codereview.chromium.org/2785883003/diff/40001/chromeos/network/onc/on...
File chromeos/network/onc/onc_utils.cc (right):

https://codereview.chromium.org/2785883003/diff/40001/chromeos/network/onc/on...
chromeos/network/onc/onc_utils.cc:968:
base::MakeUnique<ProxyConfigDictionary>(&proxy_config_value);
We should convert this to take a unique_ptr; taking the address of a const& is a
little wonky. We only call it twice and once we are dereferencing a unique_ptr
that is only used for calling this. The other time is in a test.

[vabr (Chromium), 2017-03-30 20:54:38]

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

[vabr (Chromium), 2017-03-30 20:55:06]

Thanks, Steven.

Please have another look.

Cheers,
Vaclav

https://codereview.chromium.org/2785883003/diff/40001/chromeos/network/onc/on...
File chromeos/network/onc/onc_utils.cc (right):

https://codereview.chromium.org/2785883003/diff/40001/chromeos/network/onc/on...
chromeos/network/onc/onc_utils.cc:968:
base::MakeUnique<ProxyConfigDictionary>(&proxy_config_value);
On 2017/03/30 20:39:54, stevenjb wrote:
> We should convert this to take a unique_ptr; taking the address of a const& is
a
> little wonky. We only call it twice and once we are dereferencing a unique_ptr
> that is only used for calling this. The other time is in a test.

Done.

[commit-bot: I haz the power, 2017-03-30 20:55:29]

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

[stevenjb, 2017-03-30 21:20:36]

Thanks! LGTM

[commit-bot: I haz the power, 2017-03-30 22:00:35]

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

[commit-bot: I haz the power, 2017-03-30 22:00:36]

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

[vabr (Chromium), 2017-03-31 07:19:44]

Description was changed from

==========
Use unique_ptr<DictionaryValue> in ProxyConfigDictionary

ProxyConfigDictionary uses raw pointers to DictionaryValue to pass
ownership. This CL changes that to unique_ptr:
 * to improve code clarity
 * to go one step towards removing the obsolete raw-pointer-based
   API of base::Value, and
 * to fix a leak in ProxyConfigServiceImpl::GetActiveProxyConfigDictionary.

The problem with ProxyConfigServiceImpl::GetActiveProxyConfigDictionary was
the line:
  return base::MakeUnique<ProxyConfigDictionary>(
        ProxyConfigDictionary::CreateDirect());

In the old code, CreateDirect() would return an owning raw pointer, but
the ProxyConfigDictionary constructor would only make a Value::DeepCopy of
it and let it be, never freeing that memory.

This CL fixes the issue by switching the result of CreateDirect to be a
unique_ptr. One way to do that would to do 

  return base::MakeUnique<ProxyConfigDictionary>(
        ProxyConfigDictionary::CreateDirect().get());

but that would be an unnecessary copy. Instead, this CL added a version
of the ProxyConfigDictionary constructor which accepts a unique_ptr
and avoids the copy. As a side effect, that particular line in
GetActiveProxyConfigDictionary did not have to be changed at all.

BUG=697817
==========

to

==========
Use unique_ptr<DictionaryValue> in ProxyConfigDictionary

ProxyConfigDictionary uses raw pointers to DictionaryValue to pass
ownership. This CL changes that to unique_ptr:
 * to improve code clarity
 * to go one step towards removing the obsolete raw-pointer-based
   API of base::Value, and
 * to fix a leak in ProxyConfigServiceImpl::GetActiveProxyConfigDictionary.

The problem with ProxyConfigServiceImpl::GetActiveProxyConfigDictionary was
the line:
  return base::MakeUnique<ProxyConfigDictionary>(
        ProxyConfigDictionary::CreateDirect());

In the old code, CreateDirect() would return an owning raw pointer, but
the ProxyConfigDictionary constructor would only make a Value::DeepCopy of
it and let it be, never freeing that memory.

This CL fixes the issue by switching the result of CreateDirect to be a
unique_ptr. One way to do that would to do 

  return base::MakeUnique<ProxyConfigDictionary>(
        ProxyConfigDictionary::CreateDirect().get());

but that would be an unnecessary copy. Instead, this CL added a version
of the ProxyConfigDictionary constructor which accepts a unique_ptr
and avoids the copy. As a side effect, that particular line in
GetActiveProxyConfigDictionary did not have to be changed at all.

BUG=697817
TBR=bartfab@chromium.org
==========

[vabr (Chromium), 2017-03-31 07:20:57]

Thanks all.
Putting bartfab@ in TBR for components/policy/core/browser/*.
Cheers,
Vaclav

[vabr (Chromium), 2017-03-31 07:21:03]

The CQ bit was checked by vabr@chromium.org

[vabr (Chromium), 2017-03-31 07:21:03]

The patchset sent to the CQ was uploaded after l-g-t-m from
jdoerrie@chromium.org, jochen@chromium.org, stevenjb@chromium.org
Link to the patchset: https://codereview.chromium.org/2785883003/#ps80001
(title: "Fix compilation")

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

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-31 08:08:37]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1490944863229870,
"parent_rev": "757a311011d6d1393ecde5e586bcc99ed58481c8", "commit_rev":
"6f6e2669be3bd68c883ebe591099a18666a36ba0"}

[commit-bot: I haz the power, 2017-03-31 08:09:27]

Description was changed from

==========
Use unique_ptr<DictionaryValue> in ProxyConfigDictionary

ProxyConfigDictionary uses raw pointers to DictionaryValue to pass
ownership. This CL changes that to unique_ptr:
 * to improve code clarity
 * to go one step towards removing the obsolete raw-pointer-based
   API of base::Value, and
 * to fix a leak in ProxyConfigServiceImpl::GetActiveProxyConfigDictionary.

The problem with ProxyConfigServiceImpl::GetActiveProxyConfigDictionary was
the line:
  return base::MakeUnique<ProxyConfigDictionary>(
        ProxyConfigDictionary::CreateDirect());

In the old code, CreateDirect() would return an owning raw pointer, but
the ProxyConfigDictionary constructor would only make a Value::DeepCopy of
it and let it be, never freeing that memory.

This CL fixes the issue by switching the result of CreateDirect to be a
unique_ptr. One way to do that would to do 

  return base::MakeUnique<ProxyConfigDictionary>(
        ProxyConfigDictionary::CreateDirect().get());

but that would be an unnecessary copy. Instead, this CL added a version
of the ProxyConfigDictionary constructor which accepts a unique_ptr
and avoids the copy. As a side effect, that particular line in
GetActiveProxyConfigDictionary did not have to be changed at all.

BUG=697817
TBR=bartfab@chromium.org
==========

to

==========
Use unique_ptr<DictionaryValue> in ProxyConfigDictionary

ProxyConfigDictionary uses raw pointers to DictionaryValue to pass
ownership. This CL changes that to unique_ptr:
 * to improve code clarity
 * to go one step towards removing the obsolete raw-pointer-based
   API of base::Value, and
 * to fix a leak in ProxyConfigServiceImpl::GetActiveProxyConfigDictionary.

The problem with ProxyConfigServiceImpl::GetActiveProxyConfigDictionary was
the line:
  return base::MakeUnique<ProxyConfigDictionary>(
        ProxyConfigDictionary::CreateDirect());

In the old code, CreateDirect() would return an owning raw pointer, but
the ProxyConfigDictionary constructor would only make a Value::DeepCopy of
it and let it be, never freeing that memory.

This CL fixes the issue by switching the result of CreateDirect to be a
unique_ptr. One way to do that would to do 

  return base::MakeUnique<ProxyConfigDictionary>(
        ProxyConfigDictionary::CreateDirect().get());

but that would be an unnecessary copy. Instead, this CL added a version
of the ProxyConfigDictionary constructor which accepts a unique_ptr
and avoids the copy. As a side effect, that particular line in
GetActiveProxyConfigDictionary did not have to be changed at all.

BUG=697817
TBR=bartfab@chromium.org

Review-Url: https://codereview.chromium.org/2785883003
Cr-Commit-Position: refs/heads/master@{#461076}
Committed:
https://chromium.googlesource.com/chromium/src/+/6f6e2669be3bd68c883ebe591099...
==========

[commit-bot: I haz the power, 2017-03-31 08:09:29]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/6f6e2669be3bd68c883ebe591099...

[bartfab (slow), 2017-03-31 11:34:14]

policy LGTM with nit

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

https://codereview.chromium.org/2785883003/diff/80001/components/policy/core/...
components/policy/core/browser/proxy_policy_handler.cc:11: #include
"base/memory/ptr_util.h"
Nit: No longer used.

[vabr (Chromium), 2017-04-04 15:32:15]

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

https://codereview.chromium.org/2785883003/diff/80001/components/policy/core/...
components/policy/core/browser/proxy_policy_handler.cc:11: #include
"base/memory/ptr_util.h"
On 2017/03/31 11:34:14, bartfab (slow) wrote:
> Nit: No longer used.

Good point, thanks!
I did it as a follow-up in https://codereview.chromium.org/2793093003. Sorry for
the delay, I was sick recently.