Make base::DictionaryValue::Set* return pointers

Make base::DictionaryValue::Set* return pointers

This change changes the various base::DictionaryValue::Set* methods to
return a pointer to the newly added Value. This is in response to the
discussion on https://codereview.chromium.org/2845113002/.

Furthermore, this is motivated by the fact that various insert methods
of std::map also return a handle to the just added element.

R=brettw@chromium.org
BUG=646113
Review-Url: https://codereview.chromium.org/2850773002
Cr-Commit-Position: refs/heads/master@{#468146}
Committed: https://chromium.googlesource.com/chromium/src/+/b94e542cdd7e52a69d1c439d38cec3631e133583

[jdoerrie, 2017-04-28 07:06:01]

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

[commit-bot: I haz the power, 2017-04-28 07:06: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-28 07:43:31]

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

[commit-bot: I haz the power, 2017-04-28 07:43:32]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[jdoerrie, 2017-04-28 08:56:37]

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

[commit-bot: I haz the power, 2017-04-28 08:56:54]

Dry run: CQ is trying da patch.

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

[jdoerrie, 2017-04-28 10:08:30]

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

[commit-bot: I haz the power, 2017-04-28 10:08:33]

Dry run: CQ is trying da patch.

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

[jdoerrie, 2017-04-28 11:05:18]

Hi Brett, please review.

The main point of this CL is to simplify removing the raw pointer based
DictionaryValue::Set* methods. A common pattern across the code base is thie
following (see e.g.
https://codesearch.chromium.org/chromium/src/chrome/browser/safe_browsing/inc...):

base::DictionaryValue* my_dict = nullptr;
if (!my_value->GetDictionaryWithoutPathExpansion(my_key,
                                                   &my_dict)) {
  my_dict = new base::DictionaryValue();
  my_value->SetWithoutPathExpansion(my_key, my_dict);
}

Further usage of my_dict at this point will result in use-after-move once we
change DictStorage::mapped_type to base::Value.

To guard against this, I proposed following up with a call to GetDict*:

base::DictionaryValue* my_dict = nullptr;
if (!my_value->GetDictionary(my_key, &my_dict)) {
  my_value->Set(my_key, base::MakeUnique<base::DictionaryValue>());
  my_value->GetDictionary(my_key, &my_dict);
}

Doing another map lookup is unneccessarily inefficient, which is why returning a
pointer from Set* makes sense:

base::DictionaryValue* my_dict = nullptr;
if (!my_value->GetDictionary(my_key, &my_dict)) {
  my_dict = static_cast<base::DictionaryValue>(
      my_value->Set(my_key, base::MakeUnique<base::DictionaryValue>()));
}

However, Set* can only safely return pointer to base::Value, that then often
would need to be cast to base::DictionaryValue or base::ListValue. Since this
pattern is common with these derived classes, a lot of static_casts would need
to be added. I thus further propose the introduction of SetDictionary* and
SetList*, returning pointers to the appropriate derived classes. This mirrors
the already existing GetDictionary* and GetList* and allows the code to be
written in the following way:

base::DictionaryValue* my_dict = nullptr;
if (!my_value->GetDictionary(my_key, &my_dict)) {
  my_dict = my_value->SetDictionary(my_key,
                                    base::MakeUnique<base::DictionaryValue>());
}

[commit-bot: I haz the power, 2017-04-28 11:55:34]

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

[commit-bot: I haz the power, 2017-04-28 11:55:35]

Dry run: This issue passed the CQ dry run.

[brettw, 2017-04-28 21:40:02]

lgtm

[jdoerrie, 2017-04-28 21:47:38]

The CQ bit was checked by jdoerrie@chromium.org

[commit-bot: I haz the power, 2017-04-28 21:48:15]

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-28 21:53:00]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1493416058974660,
"parent_rev": "bf219b5053732f0e750bd0d3a19e3c53106a0e35", "commit_rev":
"b94e542cdd7e52a69d1c439d38cec3631e133583"}

[commit-bot: I haz the power, 2017-04-28 21:53:10]

Description was changed from

==========
Make base::DictionaryValue::Set* return pointers

This change changes the various base::DictionaryValue::Set* methods to
return a pointer to the newly added Value. This is in response to the
discussion on https://codereview.chromium.org/2845113002/.

Furthermore, this is motivated by the fact that various insert methods
of std::map also return a handle to the just added element.

R=brettw@chromium.org
BUG=646113
==========

to

==========
Make base::DictionaryValue::Set* return pointers

This change changes the various base::DictionaryValue::Set* methods to
return a pointer to the newly added Value. This is in response to the
discussion on https://codereview.chromium.org/2845113002/.

Furthermore, this is motivated by the fact that various insert methods
of std::map also return a handle to the just added element.

R=brettw@chromium.org
BUG=646113

Review-Url: https://codereview.chromium.org/2850773002
Cr-Commit-Position: refs/heads/master@{#468146}
Committed:
https://chromium.googlesource.com/chromium/src/+/b94e542cdd7e52a69d1c439d38ce...
==========

[commit-bot: I haz the power, 2017-04-28 21:53:11]

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