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

Issue 2845113002: Remove raw base::DictionaryValue::SetWithoutPathExpansion in //chrome (Closed)

Created:
3 years, 7 months ago by jdoerrie
Modified:
3 years, 7 months ago
CC:
chromium-reviews, vakh+watch_chromium.org, michaelpg+watch-md-settings_chromium.org, hidehiko+watch_chromium.org, lhchavez+watch_chromium.org, yusukes+watch_chromium.org, extensions-reviews_chromium.org, ios-reviews_chromium.org, mlamouri+watch-geolocation_chromium.org, grt+watch_chromium.org, achuith+watch_chromium.org, chromium-apps-reviews_chromium.org, alemate+watch_chromium.org, timvolodine, pam+watch_chromium.org, oshima+watch_chromium.org, johnchen+watch_chromium.org, Matt Giuca, elijahtaylor+arcwatch_chromium.org, tfarina, mcasas+geolocation_chromium.org, victorhsieh+watch_chromium.org, dbeam+watch-settings_chromium.org, stevenjb+watch-md-settings_chromium.org, brettw
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Remove raw base::DictionaryValue::SetWithoutPathExpansion in //chrome This change removes the deprecated raw pointer version of base::DictionaryValue::SetWithoutPathExpansion in //chrome and replaces it with the unique pointer version or other convenience functions where appropriate. BUG=646113 Review-Url: https://codereview.chromium.org/2845113002 Cr-Commit-Position: refs/heads/master@{#468902} Committed: https://chromium.googlesource.com/chromium/src/+/615a9a260066f2d84646ca9ecd1b285cfc3eb0b7

Patch Set 1 #

Total comments: 12

Patch Set 2 : Replace Set/Get pairs by new Set #

Total comments: 8

Patch Set 3 : Address comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+142 lines, -129 lines) Patch
M chrome/browser/apps/drive/drive_app_mapping.cc View 2 chunks +4 lines, -2 lines 0 comments Download
M chrome/browser/geolocation/chrome_access_token_store.cc View 1 chunk +2 lines, -2 lines 0 comments Download
M chrome/browser/profiles/profile_info_cache.h View 1 3 chunks +5 lines, -2 lines 0 comments Download
M chrome/browser/profiles/profile_info_cache.cc View 25 chunks +28 lines, -50 lines 0 comments Download
M chrome/browser/safe_browsing/incident_reporting/incident_reporting_service_unittest.cc View 1 2 chunks +6 lines, -4 lines 0 comments Download
M chrome/browser/safe_browsing/incident_reporting/platform_state_store.cc View 1 2 chunks +3 lines, -2 lines 0 comments Download
M chrome/browser/safe_browsing/incident_reporting/state_store.cc View 1 2 chunks +3 lines, -2 lines 0 comments Download
M chrome/browser/safe_search_api/safe_search_url_checker_unittest.cc View 2 chunks +4 lines, -2 lines 0 comments Download
M chrome/browser/signin/easy_unlock_service_regular.cc View 1 2 chunks +4 lines, -2 lines 0 comments Download
M chrome/browser/supervised_user/child_accounts/family_info_fetcher_unittest.cc View 5 chunks +8 lines, -8 lines 0 comments Download
M chrome/browser/supervised_user/child_accounts/permission_request_creator_apiary_unittest.cc View 1 2 chunks +4 lines, -2 lines 0 comments Download
M chrome/browser/supervised_user/legacy/supervised_user_shared_settings_service.cc View 1 2 5 chunks +10 lines, -9 lines 0 comments Download
M chrome/browser/supervised_user/legacy/supervised_user_sync_service.cc View 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/supervised_user/supervised_user_bookmarks_handler.cc View 2 chunks +3 lines, -1 line 0 comments Download
M chrome/browser/supervised_user/supervised_user_bookmarks_handler_unittest.cc View 2 chunks +24 lines, -17 lines 0 comments Download
M chrome/browser/supervised_user/supervised_user_settings_service.cc View 1 5 chunks +6 lines, -4 lines 0 comments Download
M chrome/browser/supervised_user/supervised_user_whitelist_service.cc View 2 chunks +3 lines, -1 line 0 comments Download
M chrome/browser/ui/app_list/app_list_syncable_service.cc View 1 3 chunks +9 lines, -8 lines 0 comments Download
M chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc View 1 2 chunks +3 lines, -2 lines 0 comments Download
M chrome/browser/ui/webui/chromeos/login/base_webui_handler.cc View 1 2 1 chunk +4 lines, -2 lines 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/extensions/chromeos/kiosk_apps_handler.cc View 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/ui/webui/settings/profile_info_handler_unittest.cc View 1 2 chunks +3 lines, -1 line 0 comments Download
M chrome/renderer/extensions/platform_keys_natives.cc View 2 chunks +2 lines, -1 line 0 comments Download
M chrome/test/chromedriver/chrome/log.cc View 1 chunk +1 line, -2 lines 0 comments Download

Messages

Total messages: 31 (17 generated)
jdoerrie
vabr@chromium.org: Please review the whole CL in detail. thakis@chromium.org: Please spot-check and rubber-stamp. https://codereview.chromium.org/2845113002/diff/1/chrome/browser/safe_browsing/incident_reporting/platform_state_store.cc File ...
3 years, 7 months ago (2017-04-27 15:50:29 UTC) #6
Nico
https://codereview.chromium.org/2845113002/diff/1/chrome/browser/safe_browsing/incident_reporting/platform_state_store.cc File chrome/browser/safe_browsing/incident_reporting/platform_state_store.cc (right): https://codereview.chromium.org/2845113002/diff/1/chrome/browser/safe_browsing/incident_reporting/platform_state_store.cc#newcode121 chrome/browser/safe_browsing/incident_reporting/platform_state_store.cc:121: value_dict->GetDictionaryWithoutPathExpansion(type_string, &type_dict); On 2017/04/27 15:50:29, jdoerrie wrote: > While ...
3 years, 7 months ago (2017-04-27 15:58:25 UTC) #7
Nico
I'd omit all the GetDictionaryWithoutPathExpansion() since CLs shouldn't do many things, and I'm not sure ...
3 years, 7 months ago (2017-04-27 16:01:47 UTC) #8
jdoerrie
Adding brettw@ to the discussion. https://codereview.chromium.org/2845113002/diff/1/chrome/browser/safe_browsing/incident_reporting/platform_state_store.cc File chrome/browser/safe_browsing/incident_reporting/platform_state_store.cc (right): https://codereview.chromium.org/2845113002/diff/1/chrome/browser/safe_browsing/incident_reporting/platform_state_store.cc#newcode121 chrome/browser/safe_browsing/incident_reporting/platform_state_store.cc:121: value_dict->GetDictionaryWithoutPathExpansion(type_string, &type_dict); On 2017/04/27 ...
3 years, 7 months ago (2017-04-27 16:09:31 UTC) #9
brettw
https://codereview.chromium.org/2845113002/diff/1/chrome/browser/safe_browsing/incident_reporting/platform_state_store.cc File chrome/browser/safe_browsing/incident_reporting/platform_state_store.cc (right): https://codereview.chromium.org/2845113002/diff/1/chrome/browser/safe_browsing/incident_reporting/platform_state_store.cc#newcode121 chrome/browser/safe_browsing/incident_reporting/platform_state_store.cc:121: value_dict->GetDictionaryWithoutPathExpansion(type_string, &type_dict); On 2017/04/27 16:09:31, jdoerrie wrote: > On ...
3 years, 7 months ago (2017-04-27 21:54:11 UTC) #11
vabr (Chromium)
It seems that there is agreement on changing SetWithoutPathExpansion to return a pointer or something ...
3 years, 7 months ago (2017-04-28 07:23:40 UTC) #12
jdoerrie
https://codereview.chromium.org/2845113002/diff/1/chrome/browser/signin/easy_unlock_service_regular.cc File chrome/browser/signin/easy_unlock_service_regular.cc (right): https://codereview.chromium.org/2845113002/diff/1/chrome/browser/signin/easy_unlock_service_regular.cc#newcode238 chrome/browser/signin/easy_unlock_service_regular.cc:238: permit.CreateDeepCopy()); On 2017/04/28 07:23:40, vabr (Chromium) wrote: > I ...
3 years, 7 months ago (2017-04-28 08:40:12 UTC) #13
jdoerrie
Dear both, please take another look. https://codereview.chromium.org/2845113002/diff/1/chrome/browser/supervised_user/legacy/supervised_user_shared_settings_service.cc File chrome/browser/supervised_user/legacy/supervised_user_shared_settings_service.cc (right): https://codereview.chromium.org/2845113002/diff/1/chrome/browser/supervised_user/legacy/supervised_user_shared_settings_service.cc#newcode119 chrome/browser/supervised_user/legacy/supervised_user_shared_settings_service.cc:119: dict->SetWithoutPathExpansion(kValue, value.CreateDeepCopy()); On ...
3 years, 7 months ago (2017-05-02 18:08:06 UTC) #18
Nico
looks great, but maybe your forgot one file: https://codereview.chromium.org/2845113002/diff/20001/chrome/browser/supervised_user/legacy/supervised_user_shared_settings_service.cc File chrome/browser/supervised_user/legacy/supervised_user_shared_settings_service.cc (right): https://codereview.chromium.org/2845113002/diff/20001/chrome/browser/supervised_user/legacy/supervised_user_shared_settings_service.cc#newcode50 chrome/browser/supervised_user/legacy/supervised_user_shared_settings_service.cc:50: parent->GetDictionaryWithoutPathExpansion(key, ...
3 years, 7 months ago (2017-05-02 19:00:34 UTC) #19
jdoerrie
https://codereview.chromium.org/2845113002/diff/20001/chrome/browser/supervised_user/legacy/supervised_user_shared_settings_service.cc File chrome/browser/supervised_user/legacy/supervised_user_shared_settings_service.cc (right): https://codereview.chromium.org/2845113002/diff/20001/chrome/browser/supervised_user/legacy/supervised_user_shared_settings_service.cc#newcode50 chrome/browser/supervised_user/legacy/supervised_user_shared_settings_service.cc:50: parent->GetDictionaryWithoutPathExpansion(key, &dict); On 2017/05/02 19:00:34, Nico wrote: > Use ...
3 years, 7 months ago (2017-05-02 19:39:32 UTC) #22
Nico
lgtm!
3 years, 7 months ago (2017-05-02 20:10:27 UTC) #23
vabr (Chromium)
LGTM, I like it very much. Vaclav
3 years, 7 months ago (2017-05-03 05:51:24 UTC) #26
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/2845113002/40001
3 years, 7 months ago (2017-05-03 06:02:28 UTC) #28
commit-bot: I haz the power
3 years, 7 months ago (2017-05-03 06:07:27 UTC) #31
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/615a9a260066f2d84646ca9ecd1b...

Powered by Google App Engine
This is Rietveld 408576698