Stop passing raw pointers to base::Value API in c/b/ui

Stop passing raw pointers to base::Value API in c/b/ui

Passing ownership of base::Value into API methods DictionaryValue::Set,
DictionaryValue::SetWithoutPathExpansion, and ListValue::Set through raw
pointers is deprecated, can hide bugs and should be done via unique_ptr
instead.

Therefore, this CL migrates c/b/ui to use the unique_ptr-based API.

BUG=581865
TBR=dbeam@chromium.org

Review-Url: https://codereview.chromium.org/2812953002
Cr-Commit-Position: refs/heads/master@{#464860}
Committed: https://chromium.googlesource.com/chromium/src/+/4aac471cc7394249b1a3d32a777d0539c2ae54d5

[vabr (Chromium), 2017-04-11 17:14:51]

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

[vabr (Chromium), 2017-04-11 17:15:31]

Description was changed from

==========
Stop passing raw pointers to base::Value API in c/b/ui

Passing ownership of base::Value into API methods DictionaryValue::Set,
DictionaryValue::SetWithoutPathExpansion, and ListValue::Set through raw
pointers is deprecated, can hide bugs and should be done via unique_ptr
instead.

Therefore, this CL migrates c/b/ui to use the unique_ptr-based API.

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

to

==========
Stop passing raw pointers to base::Value API in c/b/ui

Passing ownership of base::Value into API methods DictionaryValue::Set,
DictionaryValue::SetWithoutPathExpansion, and ListValue::Set through raw
pointers is deprecated, can hide bugs and should be done via unique_ptr
instead.

Therefore, this CL migrates c/b/ui to use the unique_ptr-based API.

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

[commit-bot: I haz the power, 2017-04-11 17:15:38]

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-04-11 18:32:39]

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

[commit-bot: I haz the power, 2017-04-11 18:32:40]

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-04-11 19:20:19]

Note to self:

https://codereview.chromium.org/2812953002/diff/1/chrome/browser/ui/webui/cer...
File chrome/browser/ui/webui/certificate_viewer_webui.cc (right):

https://codereview.chromium.org/2812953002/diff/1/chrome/browser/ui/webui/cer...
chrome/browser/ui/webui/certificate_viewer_webui.cc:444:
cert_field->Set("children", std::move(cert_sub_fields));
This line needs to be deleted.

[vabr (Chromium), 2017-04-12 11:36:12]

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

[commit-bot: I haz the power, 2017-04-12 11:36:27]

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-04-12 11:39:25]

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

[commit-bot: I haz the power, 2017-04-12 11:39:26]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[vabr (Chromium), 2017-04-12 12:29:10]

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

[commit-bot: I haz the power, 2017-04-12 12:29:20]

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-04-12 13:16:42]

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

[commit-bot: I haz the power, 2017-04-12 13:16:43]

Dry run: This issue passed the CQ dry run.

[vabr (Chromium), 2017-04-12 15:27:21]

vabr@chromium.org changed reviewers:
+ derat@chromium.org, dpapad@chromium.org, jdoerrie@chromium.org,
msw@chromium.org

[vabr (Chromium), 2017-04-12 15:27:22]

Hi all.
Could you please review as follows?

jdoerrie@ -- the whole CL

derat@ -- owners rubberstamp for c/b/chromeos
dpapad@ -- owners rubberstamp for c/b/ui/webui
msw@ -- owners rubberstamp for c/b/ui without the webui subdirectory

Thanks!
Vaclav

[msw, 2017-04-12 17:03:32]

c/b/ui without the webui subdirectory lgtm

[Daniel Erat, 2017-04-12 18:35:35]

c/b/chromeos lgtm

[dpapad, 2017-04-12 20:59:42]

dpapad@chromium.org changed reviewers:
+ estade@chromium.org

[dpapad, 2017-04-12 20:59:43]

Deferring this review to estade@, who is still listed as a WebUI OWNER and is
also mentioned at the related TODOs

https://cs.chromium.org/webrtc/src/base/values.h?l=242,257

[dpapad, 2017-04-12 20:59:53]

dpapad@chromium.org changed reviewers:
- dpapad@chromium.org

[Evan Stade, 2017-04-12 21:25:26]

On 2017/04/12 20:59:43, dpapad wrote:
> Deferring this review to estade@, who is still listed as a WebUI OWNER and is
> also mentioned at the related TODOs
> 
> https://cs.chromium.org/webrtc/src/base/values.h?l=242,257

I am no longer a webui owner.

[jdoerrie, 2017-04-13 09:08:53]

Thanks Vaclav! LGTM with nits.

https://codereview.chromium.org/2812953002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/chromeos/login/app_launch_splash_screen_handler.cc
(right):

https://codereview.chromium.org/2812953002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/app_launch_splash_screen_handler.cc:6: 
#include <utility>

#include "base/memory/ptr_util.h"
#include "base/values.h"

https://codereview.chromium.org/2812953002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/chromeos/power_ui.cc (right):

https://codereview.chromium.org/2812953002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/power_ui.cc:9: #include <utility>
#include <memory>

https://codereview.chromium.org/2812953002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/extensions/extension_loader_handler.cc (right):

https://codereview.chromium.org/2812953002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/extensions/extension_loader_handler.cc:7: #include
<utility>
#include <memory>

https://codereview.chromium.org/2812953002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/extensions/extension_loader_handler.cc:210:
failure->Set("path", base::MakeUnique<base::Value>(
SetString

https://codereview.chromium.org/2812953002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/extensions/extension_loader_handler.cc:212:
failure->Set("error",
SetString

https://codereview.chromium.org/2812953002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/flags_ui.cc (right):

https://codereview.chromium.org/2812953002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/flags_ui.cc:7: #include <string>
#include <memory>
#include <utility>

https://codereview.chromium.org/2812953002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/instant_ui.cc (right):

https://codereview.chromium.org/2812953002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/instant_ui.cc:16: #include "base/time/time.h"
#include "base/values.h"

https://codereview.chromium.org/2812953002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc
(right):

https://codereview.chromium.org/2812953002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:8:
#include <utility>
#include <memory>

https://codereview.chromium.org/2812953002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/ntp/app_launcher_handler.cc (right):

https://codereview.chromium.org/2812953002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/ntp/app_launcher_handler.cc:9: #include <vector>
#include <utility>

https://codereview.chromium.org/2812953002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/ntp/app_launcher_handler.cc:384:
l10n_util::GetStringUTF16(IDS_APP_DEFAULT_PAGE_NAME)));
SetString

https://codereview.chromium.org/2812953002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/ntp/app_launcher_handler.cc:695:
base::MakeUnique<base::Value>(name));
SetString

https://codereview.chromium.org/2812953002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc
(right):

https://codereview.chromium.org/2812953002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc:178:
std::unique_ptr<base::Value> value = nullptr;
= nullptr is not technically necessary, you should be consistent with line 416,
so either drop it here or add it there.

https://codereview.chromium.org/2812953002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.h
(right):

https://codereview.chromium.org/2812953002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.h:9:
#include <string>
#include <memory>

https://codereview.chromium.org/2812953002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc (right):

https://codereview.chromium.org/2812953002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc:148:
std::unique_ptr<base::DictionaryValue> response =
auto

https://codereview.chromium.org/2812953002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/signin/sync_confirmation_handler_unittest.cc
(right):

https://codereview.chromium.org/2812953002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/sync_confirmation_handler_unittest.cc:193:
args.Set(0, base::MakeUnique<base::Value>(kDefaultDialogHeight));
SetDouble

https://codereview.chromium.org/2812953002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/sync_confirmation_handler_unittest.cc:222:
args.Set(0, base::MakeUnique<base::Value>(kDefaultDialogHeight));
SetDouble

https://codereview.chromium.org/2812953002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/sync_confirmation_handler_unittest.cc:274:
args.Set(0, base::MakeUnique<base::Value>(kDefaultDialogHeight));
SetDouble

https://codereview.chromium.org/2812953002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/translate_internals/translate_internals_handler.cc
(right):

https://codereview.chromium.org/2812953002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/translate_internals/translate_internals_handler.cc:92:
dict.Set("time", base::MakeUnique<base::Value>(
SetDouble

https://codereview.chromium.org/2812953002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/translate_internals/translate_internals_handler.cc:94:
dict.Set("url", base::MakeUnique<base::Value>(
SetString

https://codereview.chromium.org/2812953002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/translate_internals/translate_internals_handler.cc:96:
dict.Set("content_language",
SetString

https://codereview.chromium.org/2812953002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/translate_internals/translate_internals_handler.cc:99:
dict.Set("cld_language", base::MakeUnique<base::Value>(
SetString

https://codereview.chromium.org/2812953002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/translate_internals/translate_internals_handler.cc:101:
dict.Set("is_cld_reliable", base::MakeUnique<base::Value>(
SetBoolean

https://codereview.chromium.org/2812953002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/translate_internals/translate_internals_handler.cc:103:
dict.Set("has_notranslate", base::MakeUnique<base::Value>(
SetBoolean

https://codereview.chromium.org/2812953002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/translate_internals/translate_internals_handler.cc:105:
dict.Set("html_root_language",
SetString

https://codereview.chromium.org/2812953002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/translate_internals/translate_internals_handler.cc:108:
dict.Set("adopted_language",
SetString

https://codereview.chromium.org/2812953002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/translate_internals/translate_internals_handler.cc:111:
dict.Set("content",
SetString

https://codereview.chromium.org/2812953002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/translate_internals/translate_internals_handler.cc:121:
dict.Set("error", base::MakeUnique<base::Value>(details.error));
SetDouble
SetString
SetInteger

https://codereview.chromium.org/2812953002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/translate_internals/translate_internals_handler.cc:131:
dict.Set("message", base::MakeUnique<base::Value>(details.message));
SetDouble
SetString
SetInteger
SetString

https://codereview.chromium.org/2812953002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/translate_internals/translate_internals_handler.cc:246:
dict.Set("last_updated",
SetDouble

https://codereview.chromium.org/2812953002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/translate_internals/translate_internals_handler.cc:261:
dict.Set("update", base::MakeUnique<base::Value>(was_updated));
SetString
SetBoolean

[vabr (Chromium), 2017-04-13 12:01:36]

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

[commit-bot: I haz the power, 2017-04-13 12:01:48]

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-04-13 12:03:15]

Thanks, Jan!

Apart from ListValue having no SetString helpers (unlike DirectoryValue, which
was most of the cases), I addressed your comments.

Cheers,
Vaclav

https://codereview.chromium.org/2812953002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/chromeos/login/app_launch_splash_screen_handler.cc
(right):

https://codereview.chromium.org/2812953002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/app_launch_splash_screen_handler.cc:6: 
On 2017/04/13 09:08:51, jdoerrie wrote:
> #include <utility>
> 
> #include "base/memory/ptr_util.h"
> #include "base/values.h"

Done.

https://codereview.chromium.org/2812953002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/chromeos/power_ui.cc (right):

https://codereview.chromium.org/2812953002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/power_ui.cc:9: #include <utility>
On 2017/04/13 09:08:51, jdoerrie wrote:
> #include <memory>

Done.

https://codereview.chromium.org/2812953002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/extensions/extension_loader_handler.cc (right):

https://codereview.chromium.org/2812953002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/extensions/extension_loader_handler.cc:7: #include
<utility>
On 2017/04/13 09:08:51, jdoerrie wrote:
> #include <memory>

Done.

https://codereview.chromium.org/2812953002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/extensions/extension_loader_handler.cc:210:
failure->Set("path", base::MakeUnique<base::Value>(
On 2017/04/13 09:08:51, jdoerrie wrote:
> SetString

Done.

https://codereview.chromium.org/2812953002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/extensions/extension_loader_handler.cc:212:
failure->Set("error",
On 2017/04/13 09:08:51, jdoerrie wrote:
> SetString

Done.

https://codereview.chromium.org/2812953002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/flags_ui.cc (right):

https://codereview.chromium.org/2812953002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/flags_ui.cc:7: #include <string>
On 2017/04/13 09:08:51, jdoerrie wrote:
> #include <memory>
> #include <utility>

Done.

https://codereview.chromium.org/2812953002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/instant_ui.cc (right):

https://codereview.chromium.org/2812953002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/instant_ui.cc:16: #include "base/time/time.h"
On 2017/04/13 09:08:51, jdoerrie wrote:
> #include "base/values.h"

Done.

https://codereview.chromium.org/2812953002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc
(right):

https://codereview.chromium.org/2812953002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:8:
#include <utility>
On 2017/04/13 09:08:52, jdoerrie wrote:
> #include <memory>

Done.

https://codereview.chromium.org/2812953002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/ntp/app_launcher_handler.cc (right):

https://codereview.chromium.org/2812953002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/ntp/app_launcher_handler.cc:9: #include <vector>
On 2017/04/13 09:08:52, jdoerrie wrote:
> #include <utility>

Done.

https://codereview.chromium.org/2812953002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/ntp/app_launcher_handler.cc:384:
l10n_util::GetStringUTF16(IDS_APP_DEFAULT_PAGE_NAME)));
On 2017/04/13 09:08:52, jdoerrie wrote:
> SetString

There is no SetString in ListValue.

https://codereview.chromium.org/2812953002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/ntp/app_launcher_handler.cc:695:
base::MakeUnique<base::Value>(name));
On 2017/04/13 09:08:52, jdoerrie wrote:
> SetString

There is no SetString in ListValue.
But I at least changed |name| to 8-bit string to avoid the unnecessary
conversions going on inside Value's implementation.

https://codereview.chromium.org/2812953002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc
(right):

https://codereview.chromium.org/2812953002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc:178:
std::unique_ptr<base::Value> value = nullptr;
On 2017/04/13 09:08:52, jdoerrie wrote:
> = nullptr is not technically necessary, you should be consistent with line
416,
> so either drop it here or add it there.

Done.

https://codereview.chromium.org/2812953002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.h
(right):

https://codereview.chromium.org/2812953002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.h:9:
#include <string>
On 2017/04/13 09:08:52, jdoerrie wrote:
> #include <memory>

Done.

https://codereview.chromium.org/2812953002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc (right):

https://codereview.chromium.org/2812953002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc:148:
std::unique_ptr<base::DictionaryValue> response =
On 2017/04/13 09:08:52, jdoerrie wrote:
> auto

Done.

https://codereview.chromium.org/2812953002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/signin/sync_confirmation_handler_unittest.cc
(right):

https://codereview.chromium.org/2812953002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/sync_confirmation_handler_unittest.cc:193:
args.Set(0, base::MakeUnique<base::Value>(kDefaultDialogHeight));
On 2017/04/13 09:08:52, jdoerrie wrote:
> SetDouble

Done.

https://codereview.chromium.org/2812953002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/sync_confirmation_handler_unittest.cc:222:
args.Set(0, base::MakeUnique<base::Value>(kDefaultDialogHeight));
On 2017/04/13 09:08:52, jdoerrie wrote:
> SetDouble

Done.

https://codereview.chromium.org/2812953002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/sync_confirmation_handler_unittest.cc:274:
args.Set(0, base::MakeUnique<base::Value>(kDefaultDialogHeight));
On 2017/04/13 09:08:52, jdoerrie wrote:
> SetDouble

Done.

https://codereview.chromium.org/2812953002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/translate_internals/translate_internals_handler.cc
(right):

https://codereview.chromium.org/2812953002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/translate_internals/translate_internals_handler.cc:92:
dict.Set("time", base::MakeUnique<base::Value>(
On 2017/04/13 09:08:52, jdoerrie wrote:
> SetDouble

Done.

https://codereview.chromium.org/2812953002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/translate_internals/translate_internals_handler.cc:94:
dict.Set("url", base::MakeUnique<base::Value>(
On 2017/04/13 09:08:52, jdoerrie wrote:
> SetString

Done.

https://codereview.chromium.org/2812953002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/translate_internals/translate_internals_handler.cc:96:
dict.Set("content_language",
On 2017/04/13 09:08:53, jdoerrie wrote:
> SetString

Done.

https://codereview.chromium.org/2812953002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/translate_internals/translate_internals_handler.cc:99:
dict.Set("cld_language", base::MakeUnique<base::Value>(
On 2017/04/13 09:08:52, jdoerrie wrote:
> SetString

Done.

https://codereview.chromium.org/2812953002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/translate_internals/translate_internals_handler.cc:101:
dict.Set("is_cld_reliable", base::MakeUnique<base::Value>(
On 2017/04/13 09:08:53, jdoerrie wrote:
> SetBoolean

Done.

https://codereview.chromium.org/2812953002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/translate_internals/translate_internals_handler.cc:103:
dict.Set("has_notranslate", base::MakeUnique<base::Value>(
On 2017/04/13 09:08:52, jdoerrie wrote:
> SetBoolean

Done.

https://codereview.chromium.org/2812953002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/translate_internals/translate_internals_handler.cc:105:
dict.Set("html_root_language",
On 2017/04/13 09:08:53, jdoerrie wrote:
> SetString

Done.

https://codereview.chromium.org/2812953002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/translate_internals/translate_internals_handler.cc:108:
dict.Set("adopted_language",
On 2017/04/13 09:08:52, jdoerrie wrote:
> SetString

Done.

https://codereview.chromium.org/2812953002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/translate_internals/translate_internals_handler.cc:111:
dict.Set("content",
On 2017/04/13 09:08:53, jdoerrie wrote:
> SetString

Done.

https://codereview.chromium.org/2812953002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/translate_internals/translate_internals_handler.cc:121:
dict.Set("error", base::MakeUnique<base::Value>(details.error));
On 2017/04/13 09:08:52, jdoerrie wrote:
> SetDouble
> SetString
> SetInteger

Done.

https://codereview.chromium.org/2812953002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/translate_internals/translate_internals_handler.cc:131:
dict.Set("message", base::MakeUnique<base::Value>(details.message));
On 2017/04/13 09:08:52, jdoerrie wrote:
> SetDouble
> SetString
> SetInteger
> SetString

Done.

https://codereview.chromium.org/2812953002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/translate_internals/translate_internals_handler.cc:246:
dict.Set("last_updated",
On 2017/04/13 09:08:52, jdoerrie wrote:
> SetDouble

Done.

https://codereview.chromium.org/2812953002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/translate_internals/translate_internals_handler.cc:261:
dict.Set("update", base::MakeUnique<base::Value>(was_updated));
On 2017/04/13 09:08:53, jdoerrie wrote:
> SetString
> SetBoolean

Done.

[vabr (Chromium), 2017-04-13 12:04:33]

vabr@chromium.org changed reviewers:
+ dpapad@chromium.org

[jdoerrie, 2017-04-13 12:09:41]

Thanks, Vaclav. Sorry about the ListValue::Set confusion.

https://codereview.chromium.org/2812953002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/ntp/app_launcher_handler.cc (right):

https://codereview.chromium.org/2812953002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/ntp/app_launcher_handler.cc:695:
base::MakeUnique<base::Value>(name));
On 2017/04/13 12:03:14, vabr (Chromium) wrote:
> On 2017/04/13 09:08:52, jdoerrie wrote:
> > SetString
> 
> There is no SetString in ListValue.
> But I at least changed |name| to 8-bit string to avoid the unnecessary
> conversions going on inside Value's implementation.

True, my bad. I saw 'Set' and immediately assumed a |dict|, sorry about that.
Making name a std::string sounds good to me :)

[vabr (Chromium), 2017-04-13 12:10:05]

dpapad@, estade@ -- I'm trying to improve code for which you are listed
as OWNERS. And jdoerrie@ even reviewed the whole patch thoroughly, so
that you really just need to rubberstamp. I hope to get a pinch of
support from either of you. :)

On 2017/04/12 21:25:26, Evan Stade wrote:
> On 2017/04/12 20:59:43, dpapad wrote:
> > Deferring this review to estade@, who is still listed as a WebUI OWNER and
is
> > also mentioned at the related TODOs
> > 
> > https://cs.chromium.org/webrtc/src/base/values.h?l=242,257
> 
> I am no longer a webui owner.

Evan, you are still listed in ui/webui/OWNERS (to which c/b/ui/webui delegates).
The "Emeritus" flags does not seem helpful to others, perhaps you want to
clarify
you expected involvement in that area of code?


Thanks to all reviewers here for your time spent on this!
Vaclav

[commit-bot: I haz the power, 2017-04-13 12:15:56]

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

[commit-bot: I haz the power, 2017-04-13 12:15:57]

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

[vabr (Chromium), 2017-04-13 12:30:25]

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

[commit-bot: I haz the power, 2017-04-13 12:30:35]

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-04-13 13:18:58]

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

[commit-bot: I haz the power, 2017-04-13 13:18:59]

Dry run: This issue passed the CQ dry run.

[dpapad, 2017-04-13 21:09:30]

On 2017/04/13 at 12:10:05, vabr wrote:
> dpapad@, estade@ -- I'm trying to improve code for which you are listed
> as OWNERS. And jdoerrie@ even reviewed the whole patch thoroughly, so
> that you really just need to rubberstamp. I hope to get a pinch of
> support from either of you. :)

@vabr: I appreciate the contribution. Wanted to defer to estade@ in case he had
deeper understanding of the changes made in this CL. I am mostly worried
introducing any last moment regression (M59 branch cut is today) in
chrome/browser/ui/webui/settings/* and MD Settings is targeting M59 for launch.

Rubberstamp LGTM. I leave it up to you on whether you want to land this today,
or just wait until after today's branch cut.

[vabr (Chromium), 2017-04-14 05:48:44]

On 2017/04/13 21:09:30, dpapad wrote:
> On 2017/04/13 at 12:10:05, vabr wrote:
> > dpapad@, estade@ -- I'm trying to improve code for which you are listed
> > as OWNERS. And jdoerrie@ even reviewed the whole patch thoroughly, so
> > that you really just need to rubberstamp. I hope to get a pinch of
> > support from either of you. :)
> 
> @vabr: I appreciate the contribution. Wanted to defer to estade@ in case he
had
> deeper understanding of the changes made in this CL. I am mostly worried
> introducing any last moment regression (M59 branch cut is today) in
> chrome/browser/ui/webui/settings/* and MD Settings is targeting M59 for
launch.
> 
> Rubberstamp LGTM. I leave it up to you on whether you want to land this today,
> or just wait until after today's branch cut.

Thanks, dpapad@, for the review, and also for raising your concerns with the
branch point.

This can certainly wait until after the branch point. Also it depends on another
CL where
I am still waiting for review. :) So I'm happy to postpone landing until then.

Finally -- woohoo to MD settings targeting M59! :)

Cheers,
Vaclav

[vabr (Chromium), 2017-04-14 22:47:27]

The CQ bit was checked by vabr@chromium.org

[vabr (Chromium), 2017-04-14 22:47:27]

The patchset sent to the CQ was uploaded after l-g-t-m from
jdoerrie@chromium.org, derat@chromium.org, msw@chromium.org
Link to the patchset: https://codereview.chromium.org/2812953002/#ps80001
(title: "No ListValue::SetDouble")

[commit-bot: I haz the power, 2017-04-14 22:47:43]

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-14 22:58:52]

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

[commit-bot: I haz the power, 2017-04-14 22:58:55]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[dpapad, 2017-04-15 01:49:23]

On 2017/04/14 at 22:58:55, commit-bot wrote:
> Try jobs failed on following builders:
>   chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/options/OWNERS?l=2

[vabr (Chromium), 2017-04-15 07:13:14]

On 2017/04/15 01:49:23, dpapad wrote:
> On 2017/04/14 at 22:58:55, commit-bot wrote:
> > Try jobs failed on following builders:
> >   chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
> 
>
https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/options/OWNERS?l=2

Thanks!
I missed that "set noparent". I will add a reviewer for that soon.

Cheers,
Vaclav

[vabr (Chromium), 2017-04-15 07:22:51]

Description was changed from

==========
Stop passing raw pointers to base::Value API in c/b/ui

Passing ownership of base::Value into API methods DictionaryValue::Set,
DictionaryValue::SetWithoutPathExpansion, and ListValue::Set through raw
pointers is deprecated, can hide bugs and should be done via unique_ptr
instead.

Therefore, this CL migrates c/b/ui to use the unique_ptr-based API.

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

to

==========
Stop passing raw pointers to base::Value API in c/b/ui

Passing ownership of base::Value into API methods DictionaryValue::Set,
DictionaryValue::SetWithoutPathExpansion, and ListValue::Set through raw
pointers is deprecated, can hide bugs and should be done via unique_ptr
instead.

Therefore, this CL migrates c/b/ui to use the unique_ptr-based API.

BUG=581865
TBR=dbeam@chromium.org
==========

[vabr (Chromium), 2017-04-15 07:22:51]

vabr@chromium.org changed reviewers:
+ dbeam@chromium.org

[vabr (Chromium), 2017-04-15 07:27:49]

Hi Dan,

Could you please rubberstamp c/b/ui/webui/options/* and
chrome/browser/ui/webui/browsing_history_handler.cc ?

Those files (and the rest of the CL) were thoroughly reviewed by jdoerrie@, and
the changes are mechanical: replacing raw pointers with unique_ptr when talking
to base::Value API. The general intention was also reported on chromium-dev@ [1]
and was not objected to. Based on all the above and on our TBR guidance [2], I'm
taking the liberty to TBR you for the above files, and landing the CL now.
Please let me know if you don't think this is appropriate.

Cheers,
Vaclav


[1]
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/zRGyMMIZbvU/z1oQR...
[2]
https://chromium.googlesource.com/chromium/src/+/master/docs/code_reviews.md#...

[vabr (Chromium), 2017-04-15 07:28:08]

The CQ bit was checked by vabr@chromium.org

[commit-bot: I haz the power, 2017-04-15 07:28:16]

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-15 08:54:22]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1492241288499220,
"parent_rev": "bf8ae17c1bc517749889c3cb5a8adaebf6d7fbee", "commit_rev":
"4aac471cc7394249b1a3d32a777d0539c2ae54d5"}

[commit-bot: I haz the power, 2017-04-15 08:56:18]

Description was changed from

==========
Stop passing raw pointers to base::Value API in c/b/ui

Passing ownership of base::Value into API methods DictionaryValue::Set,
DictionaryValue::SetWithoutPathExpansion, and ListValue::Set through raw
pointers is deprecated, can hide bugs and should be done via unique_ptr
instead.

Therefore, this CL migrates c/b/ui to use the unique_ptr-based API.

BUG=581865
TBR=dbeam@chromium.org
==========

to

==========
Stop passing raw pointers to base::Value API in c/b/ui

Passing ownership of base::Value into API methods DictionaryValue::Set,
DictionaryValue::SetWithoutPathExpansion, and ListValue::Set through raw
pointers is deprecated, can hide bugs and should be done via unique_ptr
instead.

Therefore, this CL migrates c/b/ui to use the unique_ptr-based API.

BUG=581865
TBR=dbeam@chromium.org

Review-Url: https://codereview.chromium.org/2812953002
Cr-Commit-Position: refs/heads/master@{#464860}
Committed:
https://chromium.googlesource.com/chromium/src/+/4aac471cc7394249b1a3d32a777d...
==========

[commit-bot: I haz the power, 2017-04-15 08:56:21]

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

[findit-for-me, 2017-04-15 10:07:45]

A revert of this CL (patchset #5 id:80001) has been created in
https://codereview.chromium.org/2820823005/ by
findit-for-me@appspot.gserviceaccount.com.

The reason for reverting is: 
Findit(https://goo.gl/kROfz5) identified CL at revision 464860 as the
culprit for failures in the build cycles as shown on:
https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb....