Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(66)

Issue 2812953002: Stop passing raw pointers to base::Value API in c/b/ui (Closed)

Created:
3 years, 8 months ago by vabr (Chromium)
Modified:
3 years, 8 months ago
CC:
achuith+watch_chromium.org, alemate+watch_chromium.org, chromium-apps-reviews_chromium.org, chromium-reviews, davemoore+watch_chromium.org, dbeam+watch-history_chromium.org, dbeam+watch-options_chromium.org, dbeam+watch-ntp_chromium.org, dbeam+watch-settings_chromium.org, Dan Beam, David Black, dcheng, donnd+watch_chromium.org, Patrick Dubroy, estade+watch_chromium.org, extensions-reviews_chromium.org, Jered, jfweitz+watch_chromium.org, kalyank, kinuko+fileapi, kmadhusu+watch_chromium.org, mathp+autofillwatch_chromium.org, media-router+watch_chromium.org, melevin+watch_chromium.org, Matt Giuca, michaelpg+watch-options_chromium.org, michaelpg+watch-md-settings_chromium.org, nhiroki, nona+watch_chromium.org, noyau+watch_chromium.org, ntp-dev+reviews_chromium.org, oshima+watch_chromium.org, pam+watch_chromium.org, pedrosimonetti+watch_chromium.org, rogerm+autofillwatch_chromium.org, rouslan+autofill_chromium.org, sadrul, samarth+watch_chromium.org, sebsg+autofillwatch_chromium.org, shuchen+watch_chromium.org, skanuj+watch_chromium.org, stevenjb+watch-md-settings_chromium.org, tfarina, tzik, vabr+watchlistautofill_chromium.org, wjmaclean, yusukes+watch_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

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

Patch Set 1 #

Total comments: 1

Patch Set 2 : CertNodeBuilder #

Patch Set 3 : Rebased #

Total comments: 61

Patch Set 4 : Comments #

Patch Set 5 : No ListValue::SetDouble #

Unified diffs Side-by-side diffs Delta from patch set Stats (+350 lines, -290 lines) Patch
M chrome/browser/chromeos/proxy_cros_settings_parser.h View 2 chunks +2 lines, -1 line 0 comments Download
M chrome/browser/chromeos/proxy_cros_settings_parser.cc View 3 chunks +4 lines, -6 lines 0 comments Download
M chrome/browser/ui/app_list/search/common/webservice_cache.h View 1 chunk +2 lines, -1 line 0 comments Download
M chrome/browser/ui/app_list/search/common/webservice_cache.cc View 1 chunk +3 lines, -3 lines 0 comments Download
M chrome/browser/ui/ash/chrome_launcher_prefs.cc View 2 chunks +8 lines, -5 lines 0 comments Download
M chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc View 2 chunks +6 lines, -5 lines 0 comments Download
M chrome/browser/ui/browser_window_state.cc View 2 chunks +8 lines, -5 lines 0 comments Download
M chrome/browser/ui/webui/browsing_history_handler.cc View 1 2 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/ui/webui/chromeos/drive_internals_ui.cc View 2 chunks +2 lines, -2 lines 0 comments Download
M chrome/browser/ui/webui/chromeos/login/app_launch_splash_screen_handler.cc View 1 2 3 2 chunks +7 lines, -4 lines 0 comments Download
M chrome/browser/ui/webui/chromeos/login/network_dropdown.cc View 2 chunks +4 lines, -3 lines 0 comments Download
M chrome/browser/ui/webui/chromeos/login/network_screen_handler.h View 2 chunks +5 lines, -1 line 0 comments Download
M chrome/browser/ui/webui/chromeos/login/network_screen_handler.cc View 3 chunks +6 lines, -7 lines 0 comments Download
M chrome/browser/ui/webui/chromeos/login/supervised_user_creation_screen_handler.cc View 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/ui/webui/chromeos/network_ui.cc View 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/ui/webui/chromeos/power_ui.cc View 1 2 3 2 chunks +2 lines, -1 line 0 comments Download
M chrome/browser/ui/webui/chromeos/set_time_ui.cc View 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/ui/webui/components_ui.h View 3 chunks +3 lines, -1 line 0 comments Download
M chrome/browser/ui/webui/components_ui.cc View 2 chunks +3 lines, -4 lines 0 comments Download
M chrome/browser/ui/webui/cookies_tree_model_util.cc View 4 chunks +5 lines, -4 lines 0 comments Download
M chrome/browser/ui/webui/extensions/extension_loader_handler.cc View 1 2 3 2 chunks +6 lines, -3 lines 0 comments Download
M chrome/browser/ui/webui/flags_ui.cc View 1 2 3 2 chunks +4 lines, -2 lines 0 comments Download
M chrome/browser/ui/webui/flash_ui.cc View 5 chunks +32 lines, -36 lines 0 comments Download
M chrome/browser/ui/webui/foreign_session_handler.cc View 2 chunks +3 lines, -3 lines 0 comments Download
M chrome/browser/ui/webui/identity_internals_ui.cc View 2 chunks +3 lines, -3 lines 0 comments Download
M chrome/browser/ui/webui/instant_ui.cc View 1 2 3 3 chunks +3 lines, -2 lines 0 comments Download
M chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc View 1 2 3 3 chunks +5 lines, -4 lines 0 comments Download
M chrome/browser/ui/webui/nacl_ui.cc View 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/ui/webui/ntp/app_launcher_handler.cc View 1 2 3 6 chunks +12 lines, -9 lines 0 comments Download
M chrome/browser/ui/webui/options/autofill_options_handler.cc View 2 chunks +2 lines, -2 lines 0 comments Download
M chrome/browser/ui/webui/options/browser_options_handler.cc View 1 2 5 chunks +5 lines, -5 lines 0 comments Download
M chrome/browser/ui/webui/options/certificate_manager_handler.cc View 1 2 2 chunks +2 lines, -2 lines 0 comments Download
M chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.h View 1 2 3 2 chunks +3 lines, -2 lines 0 comments Download
M chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc View 1 2 3 7 chunks +17 lines, -16 lines 0 comments Download
M chrome/browser/ui/webui/options/chromeos/cros_language_options_handler.h View 3 chunks +8 lines, -2 lines 0 comments Download
M chrome/browser/ui/webui/options/chromeos/cros_language_options_handler.cc View 6 chunks +15 lines, -14 lines 0 comments Download
M chrome/browser/ui/webui/options/chromeos/display_options_handler.cc View 1 2 3 chunks +9 lines, -9 lines 0 comments Download
M chrome/browser/ui/webui/options/chromeos/keyboard_handler.cc View 2 chunks +2 lines, -2 lines 0 comments Download
M chrome/browser/ui/webui/options/clear_browser_data_handler.cc View 2 chunks +2 lines, -2 lines 0 comments Download
M chrome/browser/ui/webui/options/core_options_handler.h View 3 chunks +3 lines, -2 lines 0 comments Download
M chrome/browser/ui/webui/options/core_options_handler.cc View 6 chunks +11 lines, -9 lines 0 comments Download
M chrome/browser/ui/webui/options/handler_options_handler.cc View 2 chunks +5 lines, -3 lines 0 comments Download
M chrome/browser/ui/webui/options/language_options_handler.h View 2 chunks +7 lines, -1 line 0 comments Download
M chrome/browser/ui/webui/options/language_options_handler.cc View 3 chunks +3 lines, -2 lines 0 comments Download
M chrome/browser/ui/webui/options/language_options_handler_common.h View 3 chunks +4 lines, -2 lines 0 comments Download
M chrome/browser/ui/webui/options/language_options_handler_common.cc View 1 2 4 chunks +9 lines, -6 lines 0 comments Download
M chrome/browser/ui/webui/options/reset_profile_settings_handler.cc View 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/ui/webui/options/search_engine_manager_handler.cc View 1 chunk +1 line, -2 lines 0 comments Download
M chrome/browser/ui/webui/options/supervised_user_import_handler.cc View 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/ui/webui/policy_ui_handler.cc View 6 chunks +19 lines, -19 lines 0 comments Download
M chrome/browser/ui/webui/predictors/predictors_handler.cc View 6 chunks +11 lines, -10 lines 0 comments Download
M chrome/browser/ui/webui/settings/certificates_handler.cc View 1 2 2 chunks +2 lines, -2 lines 0 comments Download
M chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc View 1 2 3 1 chunk +3 lines, -4 lines 0 comments Download
M chrome/browser/ui/webui/settings/protocol_handlers_handler.cc View 2 chunks +5 lines, -3 lines 0 comments Download
M chrome/browser/ui/webui/settings/search_engines_handler.cc View 2 chunks +3 lines, -1 line 0 comments Download
M chrome/browser/ui/webui/signin/sync_confirmation_handler_unittest.cc View 4 4 chunks +4 lines, -3 lines 0 comments Download
M chrome/browser/ui/webui/sync_file_system_internals/sync_file_system_internals_handler.cc View 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/ui/webui/system_info_ui.cc View 2 chunks +2 lines, -2 lines 0 comments Download
M chrome/browser/ui/webui/translate_internals/translate_internals_handler.cc View 1 2 3 6 chunks +31 lines, -31 lines 0 comments Download
M chrome/browser/ui/webui/voice_search_ui.cc View 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/ui/zoom/chrome_zoom_level_prefs.cc View 2 chunks +15 lines, -8 lines 0 comments Download

Depends on Patchset:

Messages

Total messages: 53 (33 generated)
vabr (Chromium)
Note to self: https://codereview.chromium.org/2812953002/diff/1/chrome/browser/ui/webui/certificate_viewer_webui.cc File chrome/browser/ui/webui/certificate_viewer_webui.cc (right): https://codereview.chromium.org/2812953002/diff/1/chrome/browser/ui/webui/certificate_viewer_webui.cc#newcode444 chrome/browser/ui/webui/certificate_viewer_webui.cc:444: cert_field->Set("children", std::move(cert_sub_fields)); This line needs to ...
3 years, 8 months ago (2017-04-11 19:20:19 UTC) #6
vabr (Chromium)
Hi all. Could you please review as follows? jdoerrie@ -- the whole CL derat@ -- ...
3 years, 8 months ago (2017-04-12 15:27:22 UTC) #16
msw
c/b/ui without the webui subdirectory lgtm
3 years, 8 months ago (2017-04-12 17:03:32 UTC) #17
Daniel Erat
c/b/chromeos lgtm
3 years, 8 months ago (2017-04-12 18:35:35 UTC) #18
dpapad
Deferring this review to estade@, who is still listed as a WebUI OWNER and is ...
3 years, 8 months ago (2017-04-12 20:59:43 UTC) #20
Evan Stade
On 2017/04/12 20:59:43, dpapad wrote: > Deferring this review to estade@, who is still listed ...
3 years, 8 months ago (2017-04-12 21:25:26 UTC) #22
jdoerrie
Thanks Vaclav! LGTM with nits. https://codereview.chromium.org/2812953002/diff/40001/chrome/browser/ui/webui/chromeos/login/app_launch_splash_screen_handler.cc 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/chromeos/login/app_launch_splash_screen_handler.cc#newcode6 chrome/browser/ui/webui/chromeos/login/app_launch_splash_screen_handler.cc:6: #include <utility> #include "base/memory/ptr_util.h" ...
3 years, 8 months ago (2017-04-13 09:08:53 UTC) #23
vabr (Chromium)
Thanks, Jan! Apart from ListValue having no SetString helpers (unlike DirectoryValue, which was most of ...
3 years, 8 months ago (2017-04-13 12:03:15 UTC) #26
jdoerrie
Thanks, Vaclav. Sorry about the ListValue::Set confusion. https://codereview.chromium.org/2812953002/diff/40001/chrome/browser/ui/webui/ntp/app_launcher_handler.cc File chrome/browser/ui/webui/ntp/app_launcher_handler.cc (right): https://codereview.chromium.org/2812953002/diff/40001/chrome/browser/ui/webui/ntp/app_launcher_handler.cc#newcode695 chrome/browser/ui/webui/ntp/app_launcher_handler.cc:695: base::MakeUnique<base::Value>(name)); On ...
3 years, 8 months ago (2017-04-13 12:09:41 UTC) #28
vabr (Chromium)
dpapad@, estade@ -- I'm trying to improve code for which you are listed as OWNERS. ...
3 years, 8 months ago (2017-04-13 12:10:05 UTC) #29
dpapad
On 2017/04/13 at 12:10:05, vabr wrote: > dpapad@, estade@ -- I'm trying to improve code ...
3 years, 8 months ago (2017-04-13 21:09:30 UTC) #36
vabr (Chromium)
On 2017/04/13 21:09:30, dpapad wrote: > On 2017/04/13 at 12:10:05, vabr wrote: > > dpapad@, ...
3 years, 8 months ago (2017-04-14 05:48:44 UTC) #37
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2812953002/80001
3 years, 8 months ago (2017-04-14 22:47:43 UTC) #40
commit-bot: I haz the power
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_presubmit/builds/411929)
3 years, 8 months ago (2017-04-14 22:58:55 UTC) #42
dpapad
On 2017/04/14 at 22:58:55, commit-bot wrote: > Try jobs failed on following builders: > chromium_presubmit ...
3 years, 8 months ago (2017-04-15 01:49:23 UTC) #43
vabr (Chromium)
On 2017/04/15 01:49:23, dpapad wrote: > On 2017/04/14 at 22:58:55, commit-bot wrote: > > Try ...
3 years, 8 months ago (2017-04-15 07:13:14 UTC) #44
vabr (Chromium)
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 ...
3 years, 8 months ago (2017-04-15 07:27:49 UTC) #47
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2812953002/80001
3 years, 8 months ago (2017-04-15 07:28:16 UTC) #49
commit-bot: I haz the power
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/4aac471cc7394249b1a3d32a777d0539c2ae54d5
3 years, 8 months ago (2017-04-15 08:56:21 UTC) #52
findit-for-me
3 years, 8 months ago (2017-04-15 10:07:45 UTC) #53
Message was sent while issue was closed.
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....

Powered by Google App Engine
This is Rietveld 408576698