Use std::unique_ptr for base::DictionaryValue and base::ListValue's internal store.

Use std::unique_ptr for base::DictionaryValue and base::ListValue's internal store.

BUG=581865
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
R=brettw@chromium.org,danakj@chromium.org
TBR=kenrb@chromium.org,stevenjb@chromium.org

Committed: https://crrev.com/cb60e7032d871c4f6a7d03de41c8a81b9c234089
Cr-Commit-Position: refs/heads/master@{#395938}

[dcheng, 2016-05-20 19:49:46]

dcheng@chromium.org changed reviewers:
+ danakj@chromium.org

[dcheng, 2016-05-20 19:49:48]

https://codereview.chromium.org/2000803003/diff/1/base/trace_event/trace_even...
File base/trace_event/trace_event_argument.cc (right):

https://codereview.chromium.org/2000803003/diff/1/base/trace_event/trace_even...
base/trace_event/trace_event_argument.cc:291: for (const base::Value* base_value
: *list_value)
Because I changed const_iterator to return a const Value* rather than a Value*.
Dunno if this is useful or if I should just have it match
std::vector<>::const_iterator more closely.

https://codereview.chromium.org/2000803003/diff/1/base/values.cc
File base/values.cc (right):

https://codereview.chromium.org/2000803003/diff/1/base/values.cc#newcode875
base/values.cc:875: return Set(index, base::WrapUnique(in_value));
I changed a few of the obvious things I touched so that the std::unique_ptr
version is now the canonical version.

https://codereview.chromium.org/2000803003/diff/1/base/values.h
File base/values.h (right):

https://codereview.chromium.org/2000803003/diff/1/base/values.h#newcode392
base/values.h:392: class iterator : public Storage::iterator {
There's ~160 dependencies on iterator wrapping a raw pointer right now, which
makes it a little annoying to change ListValue and DictionaryValue to hold
std::unique_ptr<Value>'s internally rather than raw Value pointers.

This is a shim so the external consumers still iterate over the raw pointer
version. The use of std::unique_ptr internally feels like an implementation
detail, so it might be nice to avoid exposing it.

But even if we do want to expose it, it's probably best to separate it into a
separate patch given the number of changes.

https://codereview.chromium.org/2000803003/diff/1/base/values.h#newcode401
base/values.h:401: const Value* operator*() {
I believe const_iterator just returns Value* here. I could make it match.

[danakj, 2016-05-20 20:51:38]

Looks goodish

https://codereview.chromium.org/2000803003/diff/1/base/values.cc
File base/values.cc (right):

https://codereview.chromium.org/2000803003/diff/1/base/values.cc#newcode875
base/values.cc:875: return Set(index, base::WrapUnique(in_value));
nit: no base:: in base

https://codereview.chromium.org/2000803003/diff/1/base/values.cc#newcode1101
base/values.cc:1101: list_.insert(list_.begin() + index,
base::WrapUnique(in_value));
nit -base::

https://codereview.chromium.org/2000803003/diff/1/base/values.cc#newcode1151
base/values.cc:1151: if (!(*lhs_it)->Equals(&**rhs_it))
lol.. how about using &rhs_it->get() idk if that's more obvious wdyt?

https://codereview.chromium.org/2000803003/diff/1/base/values.h
File base/values.h (right):

https://codereview.chromium.org/2000803003/diff/1/base/values.h#newcode392
base/values.h:392: class iterator : public Storage::iterator {
On 2016/05/20 19:49:47, dcheng wrote:
> There's ~160 dependencies on iterator wrapping a raw pointer right now, which
> makes it a little annoying to change ListValue and DictionaryValue to hold
> std::unique_ptr<Value>'s internally rather than raw Value pointers.
> 
> This is a shim so the external consumers still iterate over the raw pointer
> version. The use of std::unique_ptr internally feels like an implementation
> detail, so it might be nice to avoid exposing it.
> 
> But even if we do want to expose it, it's probably best to separate it into a
> separate patch given the number of changes.

The subclassing part is weird. It'd be better if you made it a member I think
and just expose the API you want to expose.

[dcheng, 2016-05-20 22:41:55]

https://codereview.chromium.org/2000803003/diff/1/base/values.cc
File base/values.cc (right):

https://codereview.chromium.org/2000803003/diff/1/base/values.cc#newcode875
base/values.cc:875: return Set(index, base::WrapUnique(in_value));
On 2016/05/20 at 20:51:38, danakj wrote:
> nit: no base:: in base

Done.

https://codereview.chromium.org/2000803003/diff/1/base/values.cc#newcode1101
base/values.cc:1101: list_.insert(list_.begin() + index,
base::WrapUnique(in_value));
On 2016/05/20 at 20:51:38, danakj wrote:
> nit -base::

Done.

https://codereview.chromium.org/2000803003/diff/1/base/values.cc#newcode1151
base/values.cc:1151: if (!(*lhs_it)->Equals(&**rhs_it))
On 2016/05/20 at 20:51:38, danakj wrote:
> lol.. how about using &rhs_it->get() idk if that's more obvious wdyt?

Hmm. I have no idea why I thought this was a useful thing to write. Simplified
this considerably.

https://codereview.chromium.org/2000803003/diff/1/base/values.h
File base/values.h (right):

https://codereview.chromium.org/2000803003/diff/1/base/values.h#newcode392
base/values.h:392: class iterator : public Storage::iterator {
On 2016/05/20 at 20:51:38, danakj wrote:
> On 2016/05/20 19:49:47, dcheng wrote:
> > There's ~160 dependencies on iterator wrapping a raw pointer right now,
which
> > makes it a little annoying to change ListValue and DictionaryValue to hold
> > std::unique_ptr<Value>'s internally rather than raw Value pointers.
> > 
> > This is a shim so the external consumers still iterate over the raw pointer
> > version. The use of std::unique_ptr internally feels like an implementation
> > detail, so it might be nice to avoid exposing it.
> > 
> > But even if we do want to expose it, it's probably best to separate it into
a
> > separate patch given the number of changes.
> 
> The subclassing part is weird. It'd be better if you made it a member I think
and just expose the API you want to expose.

I could do that, but it makes it a bit more painful. I'll have to double up the
implementations of:
- operator++
- operator*
- operator->
- operator==
- operator!=
- in addition, operator== and operator!= need to be explicitly made comparable
now.
- the iterator conversion constructor and the iterator copy constructor both
need to be out-of-lined since the clang plugin will now complain
- for STL friendliness, both should derive from
std::iterator<std::forward_iterator_tag, Value> as well.

Finally, I need to expose a way to poke at the internal iterator for code in
values.cc, but this is going to be publicly visible outside base::Value anyway.

In short, there's quite a bit of overhead from not subclassing and it actually
gets a bit harder to read.

With the current subclassing, it's pretty straightforward: we want the exact
same behavior as the wrapped iterator, except for unary operator*.

[dcheng, 2016-05-20 22:42:57]

On 2016/05/20 at 22:41:55, dcheng wrote:
> https://codereview.chromium.org/2000803003/diff/1/base/values.cc
> File base/values.cc (right):
> 
> https://codereview.chromium.org/2000803003/diff/1/base/values.cc#newcode875
> base/values.cc:875: return Set(index, base::WrapUnique(in_value));
> On 2016/05/20 at 20:51:38, danakj wrote:
> > nit: no base:: in base
> 
> Done.
> 
> https://codereview.chromium.org/2000803003/diff/1/base/values.cc#newcode1101
> base/values.cc:1101: list_.insert(list_.begin() + index,
base::WrapUnique(in_value));
> On 2016/05/20 at 20:51:38, danakj wrote:
> > nit -base::
> 
> Done.
> 
> https://codereview.chromium.org/2000803003/diff/1/base/values.cc#newcode1151
> base/values.cc:1151: if (!(*lhs_it)->Equals(&**rhs_it))
> On 2016/05/20 at 20:51:38, danakj wrote:
> > lol.. how about using &rhs_it->get() idk if that's more obvious wdyt?
> 
> Hmm. I have no idea why I thought this was a useful thing to write. Simplified
this considerably.
> 
> https://codereview.chromium.org/2000803003/diff/1/base/values.h
> File base/values.h (right):
> 
> https://codereview.chromium.org/2000803003/diff/1/base/values.h#newcode392
> base/values.h:392: class iterator : public Storage::iterator {
> On 2016/05/20 at 20:51:38, danakj wrote:
> > On 2016/05/20 19:49:47, dcheng wrote:
> > > There's ~160 dependencies on iterator wrapping a raw pointer right now,
which
> > > makes it a little annoying to change ListValue and DictionaryValue to hold
> > > std::unique_ptr<Value>'s internally rather than raw Value pointers.
> > > 
> > > This is a shim so the external consumers still iterate over the raw
pointer
> > > version. The use of std::unique_ptr internally feels like an
implementation
> > > detail, so it might be nice to avoid exposing it.
> > > 
> > > But even if we do want to expose it, it's probably best to separate it
into a
> > > separate patch given the number of changes.
> > 
> > The subclassing part is weird. It'd be better if you made it a member I
think and just expose the API you want to expose.
> 
> I could do that, but it makes it a bit more painful. I'll have to double up
the implementations of:
> - operator++
> - operator*
> - operator->
> - operator==
> - operator!=
> - in addition, operator== and operator!= need to be explicitly made comparable
now.

err, I meant operator== and operator!= need explicit overloads for iterator and
const_iterator to be comparable with each other.

> - the iterator conversion constructor and the iterator copy constructor both
need to be out-of-lined since the clang plugin will now complain
> - for STL friendliness, both should derive from
std::iterator<std::forward_iterator_tag, Value> as well.
> 
> Finally, I need to expose a way to poke at the internal iterator for code in
values.cc, but this is going to be publicly visible outside base::Value anyway.
> 
> In short, there's quite a bit of overhead from not subclassing and it actually
gets a bit harder to read.
> 
> With the current subclassing, it's pretty straightforward: we want the exact
same behavior as the wrapped iterator, except for unary operator*.

[danakj, 2016-05-23 19:50:56]

LGTM

[dcheng, 2016-05-24 07:20:57]

Description was changed from

==========
Use std::unique_ptr for base::DictionaryValue and base::ListValue's internal
store.

BUG=none
==========

to

==========
Use std::unique_ptr for base::DictionaryValue and base::ListValue's internal
store.

BUG=none
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[dcheng, 2016-05-24 17:09:26]

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

[dcheng, 2016-05-24 17:09:28]

danakj: I need to fix some builds, but do you mind doing the overall review? I'm
going to upload PS5 after this which will be a rebase, and then PS6 which will
fix remaining failures.

I'll get global approval from brettw@ after this.

https://codereview.chromium.org/2000803003/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/api/content_settings/content_settings_store.cc
(left):

https://codereview.chromium.org/2000803003/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/api/content_settings/content_settings_store.cc:297:
base::DictionaryValue* dict = static_cast<base::DictionaryValue*>(*it);
In general, I tried to replace these with the corresponding GetAsX() helpers.

https://codereview.chromium.org/2000803003/diff/60001/extensions/browser/api/...
File
extensions/browser/api/declarative_webrequest/webrequest_condition_attribute.cc
(right):

https://codereview.chromium.org/2000803003/diff/60001/extensions/browser/api/...
extensions/browser/api/declarative_webrequest/webrequest_condition_attribute.cc:425:
return base::WrapUnique(new StringMatchTest(str, type, case_sensitive));
I usually don't bundle unrelated cleanups, but this one was particularly
egregious, since the code below releases the unique_ptr and then rewraps it.

[danakj, 2016-05-24 20:08:35]

LGTM

No BUG btw? Might be nice for your tracking purposes.

https://codereview.chromium.org/2000803003/diff/80001/base/values.cc
File base/values.cc (right):

https://codereview.chromium.org/2000803003/diff/80001/base/values.cc#newcode1040
base/values.cc:1040: list_.emplace_back(std::move(in_value));
push_back?

https://codereview.chromium.org/2000803003/diff/80001/base/values.cc#newcode1045
base/values.cc:1045: Append(WrapUnique(in_value));
this one would be emplace if u wanted.

https://codereview.chromium.org/2000803003/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/api/content_settings/content_settings_store.cc
(right):

https://codereview.chromium.org/2000803003/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/api/content_settings/content_settings_store.cc:293: if
(value->GetAsDictionary(&dict)) {
do you mean if (!GetAsDictionary) ?

https://codereview.chromium.org/2000803003/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/api/content_settings/content_settings_store.cc:295:
continue;
ಠ_ಠ Should either DCHECK or continue not both. I see this is a popular meme with
value iteration.

https://codereview.chromium.org/2000803003/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/certificate_manager_handler.cc (right):

https://codereview.chromium.org/2000803003/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/certificate_manager_handler.cc:94: NOTREACHED();
nit: I a lil bit prefer

bool a_is_dictionary = a->GetAsDictionary(&a_dict);
DCHECK(a_is_dictionary);

cuz NOTREACHED has the most useless output of them all.

https://codereview.chromium.org/2000803003/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/certificates_handler.cc (right):

https://codereview.chromium.org/2000803003/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/certificates_handler.cc:97: NOTREACHED();
nitto

https://codereview.chromium.org/2000803003/diff/80001/chromeos/network/onc/on...
File chromeos/network/onc/onc_mapper.cc (right):

https://codereview.chromium.org/2000803003/diff/80001/chromeos/network/onc/on...
chromeos/network/onc/onc_mapper.cc:119: for (const auto& entry : onc_array) {
You need to ++original_index still. wtb unit tests D:

https://codereview.chromium.org/2000803003/diff/80001/chromeos/network/onc/on...
File chromeos/network/onc/onc_utils.cc (right):

https://codereview.chromium.org/2000803003/diff/80001/chromeos/network/onc/on...
chromeos/network/onc/onc_utils.cc:368: DCHECK(cert);
how about DCHECK the return of GetAsDictionary instead?

https://codereview.chromium.org/2000803003/diff/80001/chromeos/network/onc/on...
chromeos/network/onc/onc_utils.cc:563: if (!entry->GetAsString(&guid_ref))
nitto

https://codereview.chromium.org/2000803003/diff/80001/chromeos/network/prohib...
File chromeos/network/prohibited_technologies_handler.cc (right):

https://codereview.chromium.org/2000803003/diff/80001/chromeos/network/prohib...
chromeos/network/prohibited_technologies_handler.cc:68: if
(!item->GetAsString(&prohibited_technology))
nitto?

[dcheng, 2016-05-24 20:35:58]

Description was changed from

==========
Use std::unique_ptr for base::DictionaryValue and base::ListValue's internal
store.

BUG=none
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Use std::unique_ptr for base::DictionaryValue and base::ListValue's internal
store.

BUG=581865
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[dcheng, 2016-05-24 20:39:02]

https://codereview.chromium.org/2000803003/diff/80001/base/values.cc
File base/values.cc (right):

https://codereview.chromium.org/2000803003/diff/80001/base/values.cc#newcode1040
base/values.cc:1040: list_.emplace_back(std::move(in_value));
On 2016/05/24 at 20:08:34, danakj wrote:
> push_back?

Done.

https://codereview.chromium.org/2000803003/diff/80001/base/values.cc#newcode1045
base/values.cc:1045: Append(WrapUnique(in_value));
On 2016/05/24 at 20:08:34, danakj wrote:
> this one would be emplace if u wanted.

As discussed, will just delegate to the other Append().

https://codereview.chromium.org/2000803003/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/api/content_settings/content_settings_store.cc
(right):

https://codereview.chromium.org/2000803003/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/api/content_settings/content_settings_store.cc:293: if
(value->GetAsDictionary(&dict)) {
On 2016/05/24 at 20:08:34, danakj wrote:
> do you mean if (!GetAsDictionary) ?

Yes. =X

https://codereview.chromium.org/2000803003/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/api/content_settings/content_settings_store.cc:295:
continue;
On 2016/05/24 at 20:08:34, danakj wrote:
> ಠ_ಠ Should either DCHECK or continue not both. I see this is a popular meme
with value iteration.

Yeah, I think I'd rather not change the semantics of existing code if I can
avoid it though.

https://codereview.chromium.org/2000803003/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/certificate_manager_handler.cc (right):

https://codereview.chromium.org/2000803003/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/certificate_manager_handler.cc:94: NOTREACHED();
On 2016/05/24 at 20:08:34, danakj wrote:
> nit: I a lil bit prefer
> 
> bool a_is_dictionary = a->GetAsDictionary(&a_dict);
> DCHECK(a_is_dictionary);
> 
> cuz NOTREACHED has the most useless output of them all.

Done. I don't feel super strongly, but I actually prefer the existing way for
two reasons:
- Having to explicitly name the DCHECKed thing is kind of annoying (since it
duplicates the information about what operation is being done)
- Having no indent makes the code harder to read here (IMO). NOTREACHED() may
not have useful output, but since this should never "happen", I prefer to
optimize for the code reader to make that easier to see.

What I'd really like is a DCHECK variant that always executes the statement, but
that would be confusing.

https://codereview.chromium.org/2000803003/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/certificates_handler.cc (right):

https://codereview.chromium.org/2000803003/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/certificates_handler.cc:97: NOTREACHED();
On 2016/05/24 at 20:08:34, danakj wrote:
> nitto

Done.

https://codereview.chromium.org/2000803003/diff/80001/chromeos/network/onc/on...
File chromeos/network/onc/onc_mapper.cc (right):

https://codereview.chromium.org/2000803003/diff/80001/chromeos/network/onc/on...
chromeos/network/onc/onc_mapper.cc:119: for (const auto& entry : onc_array) {
On 2016/05/24 at 20:08:35, danakj wrote:
> You need to ++original_index still. wtb unit tests D:

Done. Clever code is clever.

https://codereview.chromium.org/2000803003/diff/80001/chromeos/network/onc/on...
File chromeos/network/onc/onc_utils.cc (right):

https://codereview.chromium.org/2000803003/diff/80001/chromeos/network/onc/on...
chromeos/network/onc/onc_utils.cc:368: DCHECK(cert);
On 2016/05/24 at 20:08:35, danakj wrote:
> how about DCHECK the return of GetAsDictionary instead?

Done.

https://codereview.chromium.org/2000803003/diff/80001/chromeos/network/onc/on...
chromeos/network/onc/onc_utils.cc:563: if (!entry->GetAsString(&guid_ref))
On 2016/05/24 at 20:08:35, danakj wrote:
> nitto

Done.

[brettw, 2016-05-25 17:41:54]

rs lgtm

[dcheng, 2016-05-25 17:42:20]

The CQ bit was checked by dcheng@chromium.org

[dcheng, 2016-05-25 17:42:21]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org
Link to the patchset: https://codereview.chromium.org/2000803003/#ps160001
(title: "More fixes")

[commit-bot: I haz the power, 2016-05-25 17:42:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2000803003/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2000803003/160001

[commit-bot: I haz the power, 2016-05-25 17:52:55]

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

[commit-bot: I haz the power, 2016-05-25 17:52:57]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[dcheng, 2016-05-25 17:56:38]

Description was changed from

==========
Use std::unique_ptr for base::DictionaryValue and base::ListValue's internal
store.

BUG=581865
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Use std::unique_ptr for base::DictionaryValue and base::ListValue's internal
store.

BUG=581865
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
R=brettw@chromium.org,danakj@chromium.org
TBR=kenrb@chromium.org,stevenjb@chromium.org
==========

[dcheng, 2016-05-25 17:56:38]

dcheng@chromium.org changed reviewers:
+ kenrb@chromium.org, stevenjb@chromium.org

[dcheng, 2016-05-25 17:57:33]

The CQ bit was checked by dcheng@chromium.org

[dcheng, 2016-05-25 17:57:34]

TBRing various noparent things:
- kenrb for //chrome/browser/devtools/devtools_embedder_message_dispatcher.cc
- stevenjb@ for //chromeos/network

[commit-bot: I haz the power, 2016-05-25 17:58:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2000803003/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2000803003/160001

[commit-bot: I haz the power, 2016-05-25 18:31:00]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-05-25 18:32:43]

Description was changed from

==========
Use std::unique_ptr for base::DictionaryValue and base::ListValue's internal
store.

BUG=581865
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
R=brettw@chromium.org,danakj@chromium.org
TBR=kenrb@chromium.org,stevenjb@chromium.org
==========

to

==========
Use std::unique_ptr for base::DictionaryValue and base::ListValue's internal
store.

BUG=581865
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
R=brettw@chromium.org,danakj@chromium.org
TBR=kenrb@chromium.org,stevenjb@chromium.org

Committed: https://crrev.com/cb60e7032d871c4f6a7d03de41c8a81b9c234089
Cr-Commit-Position: refs/heads/master@{#395938}
==========

[commit-bot: I haz the power, 2016-05-25 18:32:44]

Patchset 9 (id:??) landed as
https://crrev.com/cb60e7032d871c4f6a7d03de41c8a81b9c234089
Cr-Commit-Position: refs/heads/master@{#395938}