Remove raw base::DictionaryValue::SetWithoutPathExpansion in //chrome

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

[jdoerrie, 2017-04-27 13:55:01]

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

[commit-bot: I haz the power, 2017-04-27 13:55:21]

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-27 15:26:54]

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

[commit-bot: I haz the power, 2017-04-27 15:26:54]

Dry run: This issue passed the CQ dry run.

[jdoerrie, 2017-04-27 15:50:28]

jdoerrie@chromium.org changed reviewers:
+ thakis@chromium.org, vabr@chromium.org

[jdoerrie, 2017-04-27 15:50:29]

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_browsin...
File chrome/browser/safe_browsing/incident_reporting/platform_state_store.cc
(right):

https://codereview.chromium.org/2845113002/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/incident_reporting/platform_state_store.cc:121:
value_dict->GetDictionaryWithoutPathExpansion(type_string, &type_dict);
While the call to GetWithoutPathExpansion is not strictly necessary yet, it
prepares the code for the upcoming change to base::Value::DictStorage after
which calls to Set* will invalidate pointers to the passed in Value. Querying
the pointer immediately after Set will be necessary.

This also applies to similar changes below.

[Nico, 2017-04-27 15:58:25]

https://codereview.chromium.org/2845113002/diff/1/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/incident_reporting/platform_state_store.cc
(right):

https://codereview.chromium.org/2845113002/diff/1/chrome/browser/safe_browsin...
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 the call to GetWithoutPathExpansion is not strictly necessary yet, it
> prepares the code for the upcoming change to base::Value::DictStorage after
> which calls to Set* will invalidate pointers to the passed in Value. Querying
> the pointer immediately after Set will be necessary.
> 
> This also applies to similar changes below.

That seems kind of inefficient, maybe SetWithoutPathExpansion() should return
the inserted DictionaryValue() so one doesn't have to re-query?

[Nico, 2017-04-27 16:01:47]

I'd omit all the GetDictionaryWithoutPathExpansion() since CLs shouldn't do many
things, and I'm not sure I'm a fan of that design. The rest of this change looks
great.

[jdoerrie, 2017-04-27 16:09:31]

Adding brettw@ to the discussion.

https://codereview.chromium.org/2845113002/diff/1/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/incident_reporting/platform_state_store.cc
(right):

https://codereview.chromium.org/2845113002/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/incident_reporting/platform_state_store.cc:121:
value_dict->GetDictionaryWithoutPathExpansion(type_string, &type_dict);
On 2017/04/27 15:58:25, Nico wrote:
> On 2017/04/27 15:50:29, jdoerrie wrote:
> > While the call to GetWithoutPathExpansion is not strictly necessary yet, it
> > prepares the code for the upcoming change to base::Value::DictStorage after
> > which calls to Set* will invalidate pointers to the passed in Value.
Querying
> > the pointer immediately after Set will be necessary.
> > 
> > This also applies to similar changes below.
> 
> That seems kind of inefficient, maybe SetWithoutPathExpansion() should return
> the inserted DictionaryValue() so one doesn't have to re-query?

I made a similar suggestion when doing the ListValue change which involved
Append returning a pointer to the just added element. brettw@ argued against it,
basically saying we should try to mimic push_back as much as possible:
https://codereview.chromium.org/2740143002/diff/140001/base/values.h#newcode445

Here the situation is different though, since all of
std::map::{insert,emplace,operator[]} either return an iterator or a reference
to the inserted element.

What do you think, brettw@? Do you agree with DictionaryValue::Set* returning
pointers to the inserted Values?

[brettw, 2017-04-27 21:54:10]

brettw@chromium.org changed reviewers:
+ brettw@chromium.org

[brettw, 2017-04-27 21:54:11]

https://codereview.chromium.org/2845113002/diff/1/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/incident_reporting/platform_state_store.cc
(right):

https://codereview.chromium.org/2845113002/diff/1/chrome/browser/safe_browsin...
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 2017/04/27 15:58:25, Nico wrote:
> > On 2017/04/27 15:50:29, jdoerrie wrote:
> > > While the call to GetWithoutPathExpansion is not strictly necessary yet,
it
> > > prepares the code for the upcoming change to base::Value::DictStorage
after
> > > which calls to Set* will invalidate pointers to the passed in Value.
> Querying
> > > the pointer immediately after Set will be necessary.
> > > 
> > > This also applies to similar changes below.
> > 
> > That seems kind of inefficient, maybe SetWithoutPathExpansion() should
return
> > the inserted DictionaryValue() so one doesn't have to re-query?
> 
> I made a similar suggestion when doing the ListValue change which involved
> Append returning a pointer to the just added element. brettw@ argued against
it,
> basically saying we should try to mimic push_back as much as possible:
>
https://codereview.chromium.org/2740143002/diff/140001/base/values.h#newcode445
> 
> Here the situation is different though, since all of
> std::map::{insert,emplace,operator[]} either return an iterator or a reference
> to the inserted element.
> 
> What do you think, brettw@? Do you agree with DictionaryValue::Set* returning
> pointers to the inserted Values?

I'm happy to treat this like std::map::insert and return some kind of pointer or
iterator thingy. For lists it's easy to get access to the inserted element if
you need it, but that's not the case here.

[vabr (Chromium), 2017-04-28 07:23:40]

It seems that there is agreement on changing SetWithoutPathExpansion to return a
pointer or something for the inserted value. We should also update
DictionaryValue::Set to do the same.

Other than that just a question about CreateDeepCopy vs. copy c-tor.

The rest LGTM, so happy to see this gone!

Cheers,
Vaclav

https://codereview.chromium.org/2845113002/diff/1/chrome/browser/signin/easy_...
File chrome/browser/signin/easy_unlock_service_regular.cc (right):

https://codereview.chromium.org/2845113002/diff/1/chrome/browser/signin/easy_...
chrome/browser/signin/easy_unlock_service_regular.cc:238:
permit.CreateDeepCopy());
I thought the best way to do a deep copy is
MakeUnique<Value>(permit)
?

(Also below.)

https://codereview.chromium.org/2845113002/diff/1/chrome/browser/supervised_u...
File
chrome/browser/supervised_user/legacy/supervised_user_shared_settings_service.cc
(right):

https://codereview.chromium.org/2845113002/diff/1/chrome/browser/supervised_u...
chrome/browser/supervised_user/legacy/supervised_user_shared_settings_service.cc:119:
dict->SetWithoutPathExpansion(kValue, value.CreateDeepCopy());
Ditto about using copy constructor instead.

https://codereview.chromium.org/2845113002/diff/1/chrome/browser/supervised_u...
File chrome/browser/supervised_user/supervised_user_settings_service.cc (right):

https://codereview.chromium.org/2845113002/diff/1/chrome/browser/supervised_u...
chrome/browser/supervised_user/supervised_user_settings_service.cc:285:
dict->SetWithoutPathExpansion(key_suffix, it.value().CreateDeepCopy());
and here a copy constructor instead as well?

https://codereview.chromium.org/2845113002/diff/1/chrome/browser/ui/webui/chr...
File chrome/browser/ui/webui/chromeos/login/base_webui_handler.cc (right):

https://codereview.chromium.org/2845113002/diff/1/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/login/base_webui_handler.cc:82:
screen_params.SetWithoutPathExpansion("data", data->CreateDeepCopy());
and here a copy constructor as well

[jdoerrie, 2017-04-28 08:40:12]

https://codereview.chromium.org/2845113002/diff/1/chrome/browser/signin/easy_...
File chrome/browser/signin/easy_unlock_service_regular.cc (right):

https://codereview.chromium.org/2845113002/diff/1/chrome/browser/signin/easy_...
chrome/browser/signin/easy_unlock_service_regular.cc:238:
permit.CreateDeepCopy());
On 2017/04/28 07:23:40, vabr (Chromium) wrote:
> I thought the best way to do a deep copy is
> MakeUnique<Value>(permit)
> ?
> 
> (Also below.)

Yes, you are right, thank you! I initially didn't do it, because |Set(key,
unique_ptr)| will also be deprecated at some point, which means this code will
need to touched again. I figured using |CreateDeepCopy| here will make it easier
to find and remove all these usages.

However, on second thought this is not a strong argument, and I should avoid
further usages of already deprecated methods. I will fix this in the next patch
set.

[jdoerrie, 2017-05-02 16:29:11]

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

[commit-bot: I haz the power, 2017-05-02 16:29:33]

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-05-02 17:15:10]

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

[commit-bot: I haz the power, 2017-05-02 17:15:11]

Dry run: This issue passed the CQ dry run.

[jdoerrie, 2017-05-02 18:08:06]

Dear both, please take another look.

https://codereview.chromium.org/2845113002/diff/1/chrome/browser/supervised_u...
File
chrome/browser/supervised_user/legacy/supervised_user_shared_settings_service.cc
(right):

https://codereview.chromium.org/2845113002/diff/1/chrome/browser/supervised_u...
chrome/browser/supervised_user/legacy/supervised_user_shared_settings_service.cc:119:
dict->SetWithoutPathExpansion(kValue, value.CreateDeepCopy());
On 2017/04/28 07:23:40, vabr (Chromium) wrote:
> Ditto about using copy constructor instead.

Done.

https://codereview.chromium.org/2845113002/diff/1/chrome/browser/supervised_u...
File chrome/browser/supervised_user/supervised_user_settings_service.cc (right):

https://codereview.chromium.org/2845113002/diff/1/chrome/browser/supervised_u...
chrome/browser/supervised_user/supervised_user_settings_service.cc:285:
dict->SetWithoutPathExpansion(key_suffix, it.value().CreateDeepCopy());
On 2017/04/28 07:23:40, vabr (Chromium) wrote:
> and here a copy constructor instead as well?

Done.

https://codereview.chromium.org/2845113002/diff/1/chrome/browser/ui/webui/chr...
File chrome/browser/ui/webui/chromeos/login/base_webui_handler.cc (right):

https://codereview.chromium.org/2845113002/diff/1/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/login/base_webui_handler.cc:82:
screen_params.SetWithoutPathExpansion("data", data->CreateDeepCopy());
On 2017/04/28 07:23:40, vabr (Chromium) wrote:
> and here a copy constructor as well

Done.

[Nico, 2017-05-02 19:00:34]

looks great, but maybe your forgot one file:

https://codereview.chromium.org/2845113002/diff/20001/chrome/browser/supervis...
File
chrome/browser/supervised_user/legacy/supervised_user_shared_settings_service.cc
(right):

https://codereview.chromium.org/2845113002/diff/20001/chrome/browser/supervis...
chrome/browser/supervised_user/legacy/supervised_user_shared_settings_service.cc:50:
parent->GetDictionaryWithoutPathExpansion(key, &dict);
Use new form here too.

https://codereview.chromium.org/2845113002/diff/20001/chrome/browser/supervis...
chrome/browser/supervised_user/legacy/supervised_user_shared_settings_service.cc:117:
update_dict->GetDictionaryWithoutPathExpansion(key, &dict);
Here too.

https://codereview.chromium.org/2845113002/diff/20001/chrome/browser/supervis...
chrome/browser/supervised_user/legacy/supervised_user_shared_settings_service.cc:349:
update_dict->GetDictionaryWithoutPathExpansion(key, &dict);
Here too

https://codereview.chromium.org/2845113002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/chromeos/login/base_webui_handler.cc (right):

https://codereview.chromium.org/2845113002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/base_webui_handler.cc:81: if (data)
nit: add {}

[jdoerrie, 2017-05-02 19:38:07]

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

[commit-bot: I haz the power, 2017-05-02 19:38:39]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jdoerrie, 2017-05-02 19:39:32]

https://codereview.chromium.org/2845113002/diff/20001/chrome/browser/supervis...
File
chrome/browser/supervised_user/legacy/supervised_user_shared_settings_service.cc
(right):

https://codereview.chromium.org/2845113002/diff/20001/chrome/browser/supervis...
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 new form here too.

Done.

https://codereview.chromium.org/2845113002/diff/20001/chrome/browser/supervis...
chrome/browser/supervised_user/legacy/supervised_user_shared_settings_service.cc:117:
update_dict->GetDictionaryWithoutPathExpansion(key, &dict);
On 2017/05/02 19:00:34, Nico wrote:
> Here too.

Done.

https://codereview.chromium.org/2845113002/diff/20001/chrome/browser/supervis...
chrome/browser/supervised_user/legacy/supervised_user_shared_settings_service.cc:349:
update_dict->GetDictionaryWithoutPathExpansion(key, &dict);
On 2017/05/02 19:00:34, Nico wrote:
> Here too

Done.

https://codereview.chromium.org/2845113002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/chromeos/login/base_webui_handler.cc (right):

https://codereview.chromium.org/2845113002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/base_webui_handler.cc:81: if (data)
On 2017/05/02 19:00:34, Nico wrote:
> nit: add {}

Done.

[Nico, 2017-05-02 20:10:27]

lgtm!

[commit-bot: I haz the power, 2017-05-02 20:27:31]

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

[commit-bot: I haz the power, 2017-05-02 20:27:32]

Dry run: This issue passed the CQ dry run.

[vabr (Chromium), 2017-05-03 05:51:24]

LGTM, I like it very much.
Vaclav

[jdoerrie, 2017-05-03 06:02:08]

The CQ bit was checked by jdoerrie@chromium.org

[commit-bot: I haz the power, 2017-05-03 06:02:28]

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-05-03 06:07:14]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1493791328036400,
"parent_rev": "15c2e6536d2afc4cbc47204f13fd61c884cd57aa", "commit_rev":
"615a9a260066f2d84646ca9ecd1b285cfc3eb0b7"}

[commit-bot: I haz the power, 2017-05-03 06:07:24]

Description was changed from

==========
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
==========

to

==========
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/+/615a9a260066f2d84646ca9ecd1b...
==========

[commit-bot: I haz the power, 2017-05-03 06:07:27]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/615a9a260066f2d84646ca9ecd1b...