Stop passing raw pointers to DictionaryValue::Set, part 2

Stop passing raw pointers to DictionaryValue::Set, part 2

base::Value API has alternatives with unique_ptr, and passing ownership with
raw pointers can hide bugs. Therefore, this CL migrates //c/b/bookmarks and
//c/b/chromeos to use the unique_ptr-based API.

BUG=697817
Review-Url: https://codereview.chromium.org/2765363004
Cr-Commit-Position: refs/heads/master@{#459277}
Committed: https://chromium.googlesource.com/chromium/src/+/6e421c60e822ffaba52747b847bd4a72df4f82d8

[vabr (Chromium), 2017-03-22 22:26:04]

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

[commit-bot: I haz the power, 2017-03-22 22:26:44]

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-22 22:48:21]

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

[commit-bot: I haz the power, 2017-03-22 22:48:22]

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_...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[vabr (Chromium), 2017-03-22 22:57:18]

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

[commit-bot: I haz the power, 2017-03-22 22:58:23]

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-22 23:15:21]

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

[commit-bot: I haz the power, 2017-03-22 23:15:22]

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_...)
  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-23 07:17:14]

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

[commit-bot: I haz the power, 2017-03-23 07:17:26]

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-23 07:28:39]

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

[commit-bot: I haz the power, 2017-03-23 07:28:41]

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-23 07:56:59]

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

[commit-bot: I haz the power, 2017-03-23 07:57:12]

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

[vabr (Chromium), 2017-03-23 08:47:24]

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

[commit-bot: I haz the power, 2017-03-23 08:47:39]

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

[vabr (Chromium), 2017-03-23 08:49:01]

vabr@chromium.org changed reviewers:
+ derat@chromium.org, sky@chromium.org

[vabr (Chromium), 2017-03-23 08:49:02]

Hello derat@ and sky@!

Please review the following

derat@ -- chrome/browser/chromeos/*

sky@ -- chrome/browser/bookmarks/managed_bookmark_service_unittest.cc

Thank you!
Vaclav

[commit-bot: I haz the power, 2017-03-23 10:34:41]

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

[commit-bot: I haz the power, 2017-03-23 10:34:42]

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

[vabr (Chromium), 2017-03-23 10:45:12]

The error in DriveFileSystemExtensionApiTest is real, and I'll investigate.

sky@ -- chrome/browser/bookmarks/managed_bookmark_service_unittest.cc is not
affected, you may continue reviewing it.

derat@ -- please hold on with reviewing until I fix the problem and notify you;
sorry for the noise!

Vaclav

[Daniel Erat, 2017-03-23 14:21:31]

lgtm

thanks!

https://codereview.chromium.org/2765363004/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/extensions/external_cache.cc (right):

https://codereview.chromium.org/2765363004/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/extensions/external_cache.cc:320: // Copy original_entry
to don't modify it inside extensions_.
nit: if you want to fix this comment, s/don't/not/

https://codereview.chromium.org/2765363004/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/proxy_cros_settings_parser.cc (right):

https://codereview.chromium.org/2765363004/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/proxy_cros_settings_parser.cc:288: base::Value**
out_value) {
nit: add a TODO to make this be a std::unique_ptr<base::Value>*?

[sky, 2017-03-23 16:35:19]

LGTM

[vabr (Chromium), 2017-03-23 22:43:50]

Thanks for the review!
I fixed the failure and addressed the comments, so sending to the CQ now.

Cheers,
Vaclav

https://codereview.chromium.org/2765363004/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/extensions/external_cache.cc (right):

https://codereview.chromium.org/2765363004/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/extensions/external_cache.cc:320: // Copy original_entry
to don't modify it inside extensions_.
On 2017/03/23 14:21:30, Daniel Erat wrote:
> nit: if you want to fix this comment, s/don't/not/

Good point!
It still sounded a bit clumsy to me, so I opted for "avoid modifying".

https://codereview.chromium.org/2765363004/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/proxy_cros_settings_parser.cc (right):

https://codereview.chromium.org/2765363004/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/proxy_cros_settings_parser.cc:288: base::Value**
out_value) {
On 2017/03/23 14:21:30, Daniel Erat wrote:
> nit: add a TODO to make this be a std::unique_ptr<base::Value>*?

Good point!
Done.

[vabr (Chromium), 2017-03-23 22:44:13]

The CQ bit was checked by vabr@chromium.org

[vabr (Chromium), 2017-03-23 22:44:13]

The patchset sent to the CQ was uploaded after l-g-t-m from derat@chromium.org,
sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2765363004/#ps120001
(title: "Fix comments")

[commit-bot: I haz the power, 2017-03-23 22:44:40]

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-23 23:28:00]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1490309053290430,
"parent_rev": "c6eefbe8f53fdcc35ec42a4af901eefcd612dcc9", "commit_rev":
"6e421c60e822ffaba52747b847bd4a72df4f82d8"}

[commit-bot: I haz the power, 2017-03-23 23:28:46]

Description was changed from

==========
Stop passing raw pointers to DictionaryValue::Set, part 2

base::Value API has alternatives with unique_ptr, and passing ownership with
raw pointers can hide bugs. Therefore, this CL migrates //c/b/bookmarks and
//c/b/chromeos to use the unique_ptr-based API.

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

to

==========
Stop passing raw pointers to DictionaryValue::Set, part 2

base::Value API has alternatives with unique_ptr, and passing ownership with
raw pointers can hide bugs. Therefore, this CL migrates //c/b/bookmarks and
//c/b/chromeos to use the unique_ptr-based API.

BUG=697817

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

[commit-bot: I haz the power, 2017-03-23 23:28:56]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/6e421c60e822ffaba52747b847bd...