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

Issue 2785883003: Use unique_ptr<DictionaryValue> in ProxyConfigDictionary (Closed)

Created:
3 years, 8 months ago by vabr (Chromium)
Modified:
3 years, 8 months ago
CC:
chromium-apps-reviews_chromium.org, chromium-reviews, davemoore+watch_chromium.org, extensions-reviews_chromium.org, oshima+watch_chromium.org, stevenjb+watch_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

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

Patch Set 1 #

Total comments: 6

Patch Set 2 : comments #

Total comments: 3

Patch Set 3 : one constructor #

Total comments: 2

Patch Set 4 : ConvertProxyConfigToOncProxySettings takes unique_ptr #

Patch Set 5 : Fix compilation #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+158 lines, -147 lines) Patch
M chrome/browser/chromeos/arc/intent_helper/arc_settings_service_browsertest.cc View 1 2 3 chunks +3 lines, -2 lines 0 comments Download
M chrome/browser/chromeos/proxy_config_service_impl_unittest.cc View 1 chunk +6 lines, -6 lines 0 comments Download
M chrome/browser/extensions/api/proxy/proxy_api.cc View 1 2 2 chunks +6 lines, -4 lines 0 comments Download
M chrome/browser/extensions/api/proxy/proxy_api_helpers.h View 1 2 chunks +2 lines, -1 line 0 comments Download
M chrome/browser/extensions/api/proxy/proxy_api_helpers.cc View 2 chunks +2 lines, -2 lines 0 comments Download
M chrome/browser/extensions/api/proxy/proxy_api_helpers_unittest.cc View 1 2 6 chunks +17 lines, -16 lines 0 comments Download
M chrome/browser/extensions/api/proxy/proxy_apitest.cc View 1 2 1 chunk +2 lines, -1 line 0 comments Download
M chrome/browser/prefs/chrome_command_line_pref_store.cc View 1 chunk +7 lines, -9 lines 0 comments Download
M chrome/browser/prefs/chrome_command_line_pref_store_unittest.cc View 1 2 2 chunks +3 lines, -2 lines 0 comments Download
M chrome/browser/prefs/proxy_policy_unittest.cc View 1 2 8 chunks +12 lines, -8 lines 0 comments Download
M chrome/browser/ui/webui/options/preferences_browsertest.cc View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download
M chromeos/network/onc/onc_translator_shill_to_onc.cc View 1 2 3 1 chunk +1 line, -1 line 0 comments Download
M chromeos/network/onc/onc_utils.h View 1 2 3 1 chunk +1 line, -1 line 0 comments Download
M chromeos/network/onc/onc_utils.cc View 1 2 3 3 chunks +9 lines, -9 lines 0 comments Download
M chromeos/network/onc/onc_utils_unittest.cc View 1 2 3 1 chunk +2 lines, -1 line 0 comments Download
M chromeos/network/proxy/proxy_config_handler.cc View 1 2 3 chunks +4 lines, -2 lines 0 comments Download
M chromeos/network/proxy/proxy_config_service_impl.cc View 1 2 1 chunk +2 lines, -1 line 0 comments Download
M chromeos/network/proxy/ui_proxy_config.h View 1 2 chunks +2 lines, -1 line 0 comments Download
M chromeos/network/proxy/ui_proxy_config.cc View 2 chunks +3 lines, -2 lines 0 comments Download
M chromeos/network/proxy/ui_proxy_config_service.cc View 1 2 2 chunks +2 lines, -1 line 0 comments Download
M components/policy/core/browser/proxy_policy_handler.cc View 2 chunks +10 lines, -12 lines 2 comments Download
M components/policy/core/browser/proxy_policy_handler_unittest.cc View 1 2 1 chunk +1 line, -1 line 0 comments Download
M components/proxy_config/pref_proxy_config_tracker_impl.cc View 1 2 4 chunks +6 lines, -5 lines 0 comments Download
M components/proxy_config/pref_proxy_config_tracker_impl_unittest.cc View 7 chunks +13 lines, -21 lines 0 comments Download
M components/proxy_config/proxy_config_dictionary.h View 1 2 3 chunks +9 lines, -9 lines 0 comments Download
M components/proxy_config/proxy_config_dictionary.cc View 1 2 8 chunks +16 lines, -12 lines 0 comments Download
M components/proxy_config/proxy_config_dictionary_unittest.cc View 1 2 6 chunks +16 lines, -16 lines 0 comments Download

Messages

Total messages: 41 (25 generated)
vabr (Chromium)
Hi all, stevenjb@ -- please review: * components/proxy_config/proxy_config_dictionary.* * the mechanical changes in the rest ...
3 years, 8 months ago (2017-03-30 09:48:00 UTC) #6
jdoerrie
LGTM % nits, thanks! Description Nit: ... This needs to be changed to unique_ptr to ...
3 years, 8 months ago (2017-03-30 11:00:39 UTC) #7
jochen (gone - plz use gerrit)
lgtm
3 years, 8 months ago (2017-03-30 11:50:26 UTC) #8
vabr (Chromium)
Thanks for the review! Remaining requests: stevenjb@ -- please review: * components/proxy_config/proxy_config_dictionary.* * the mechanical ...
3 years, 8 months ago (2017-03-30 12:07:42 UTC) #10
stevenjb
https://codereview.chromium.org/2785883003/diff/20001/components/proxy_config/proxy_config_dictionary.h File components/proxy_config/proxy_config_dictionary.h (right): https://codereview.chromium.org/2785883003/diff/20001/components/proxy_config/proxy_config_dictionary.h#newcode34 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: ...
3 years, 8 months ago (2017-03-30 17:00:58 UTC) #15
vabr (Chromium)
https://codereview.chromium.org/2785883003/diff/20001/components/proxy_config/proxy_config_dictionary.h File components/proxy_config/proxy_config_dictionary.h (right): https://codereview.chromium.org/2785883003/diff/20001/components/proxy_config/proxy_config_dictionary.h#newcode34 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: > ...
3 years, 8 months ago (2017-03-30 17:04:21 UTC) #16
vabr (Chromium)
Hi Steven, I removed the old constructor as you suggested. Please have a look. Cheers, ...
3 years, 8 months ago (2017-03-30 19:24:33 UTC) #20
stevenjb
Thanks, i think this is more clear (and fewer copies in tests :)) https://codereview.chromium.org/2785883003/diff/40001/chromeos/network/onc/onc_utils.cc File ...
3 years, 8 months ago (2017-03-30 20:39:55 UTC) #24
stevenjb
3 years, 8 months ago (2017-03-30 20:40:00 UTC) #25
vabr (Chromium)
Thanks, Steven. Please have another look. Cheers, Vaclav https://codereview.chromium.org/2785883003/diff/40001/chromeos/network/onc/onc_utils.cc File chromeos/network/onc/onc_utils.cc (right): https://codereview.chromium.org/2785883003/diff/40001/chromeos/network/onc/onc_utils.cc#newcode968 chromeos/network/onc/onc_utils.cc:968: base::MakeUnique<ProxyConfigDictionary>(&proxy_config_value); ...
3 years, 8 months ago (2017-03-30 20:55:06 UTC) #27
stevenjb
Thanks! LGTM
3 years, 8 months ago (2017-03-30 21:20:36 UTC) #29
vabr (Chromium)
Thanks all. Putting bartfab@ in TBR for components/policy/core/browser/*. Cheers, Vaclav
3 years, 8 months ago (2017-03-31 07:20:57 UTC) #33
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/2785883003/80001
3 years, 8 months ago (2017-03-31 07:21:11 UTC) #36
commit-bot: I haz the power
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/6f6e2669be3bd68c883ebe591099a18666a36ba0
3 years, 8 months ago (2017-03-31 08:09:29 UTC) #39
bartfab (slow)
policy LGTM with nit https://codereview.chromium.org/2785883003/diff/80001/components/policy/core/browser/proxy_policy_handler.cc File components/policy/core/browser/proxy_policy_handler.cc (right): https://codereview.chromium.org/2785883003/diff/80001/components/policy/core/browser/proxy_policy_handler.cc#newcode11 components/policy/core/browser/proxy_policy_handler.cc:11: #include "base/memory/ptr_util.h" Nit: No longer ...
3 years, 8 months ago (2017-03-31 11:34:14 UTC) #40
vabr (Chromium)
3 years, 8 months ago (2017-04-04 15:32:15 UTC) #41
Message was sent while issue was closed.
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.

Powered by Google App Engine
This is Rietveld 408576698