More base::Values-related bare pointer -> scoped_ptr conversions

More base::Values-related bare pointer -> scoped_ptr conversions

main change: Value::CreateNullValue returns a scoped_ptr

BUG=none

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

[Evan Stade, 2015-05-08 17:21:51]

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

[danakj, 2015-05-08 18:07:25]

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

[danakj, 2015-05-08 18:07:26]

https://codereview.chromium.org/1129083003/diff/20001/base/json/json_parser.cc
File base/json/json_parser.cc (right):

https://codereview.chromium.org/1129083003/diff/20001/base/json/json_parser.c...
base/json/json_parser.cc:902: Value* JSONParser::ConsumeLiteral() {
TODO to make this return a scoped_ptr?

https://codereview.chromium.org/1129083003/diff/20001/base/json/json_writer_u...
File base/json/json_writer_unittest.cc (right):

https://codereview.chromium.org/1129083003/diff/20001/base/json/json_writer_u...
base/json/json_writer_unittest.cc:15: Value* root =
Value::CreateNullValue().release();
can you change |root| to a scoped_ptr?

https://codereview.chromium.org/1129083003/diff/20001/base/values.cc
File base/values.cc (right):

https://codereview.chromium.org/1129083003/diff/20001/base/values.cc#newcode136
base/values.cc:136: Value* Value::DeepCopy() const {
TODO to return scoped_ptr?

https://codereview.chromium.org/1129083003/diff/20001/base/values_unittest.cc
File base/values_unittest.cc (right):

https://codereview.chromium.org/1129083003/diff/20001/base/values_unittest.cc...
base/values_unittest.cc:392: Value* original_null =
Value::CreateNullValue().release();
can you make original_null a scoped_ptr?

https://codereview.chromium.org/1129083003/diff/20001/base/values_unittest.cc...
base/values_unittest.cc:595: Value* original_null =
Value::CreateNullValue().release();
can you make original_null a scoped_ptr too?

[Evan Stade, 2015-05-08 18:32:10]

https://codereview.chromium.org/1129083003/diff/20001/base/json/json_parser.cc
File base/json/json_parser.cc (right):

https://codereview.chromium.org/1129083003/diff/20001/base/json/json_parser.c...
base/json/json_parser.cc:902: Value* JSONParser::ConsumeLiteral() {
On 2015/05/08 18:07:25, danakj wrote:
> TODO to make this return a scoped_ptr?

I don't think that's all that helpful. If there were a TODO in every place that
ought to be using a scoped_ptr, it would probably add 10,000 lines to Chrome. In
general, it's not that difficult to find which places should have scoped_ptrs,
so I don't think a comment adds much anyway.

https://codereview.chromium.org/1129083003/diff/20001/base/json/json_writer_u...
File base/json/json_writer_unittest.cc (right):

https://codereview.chromium.org/1129083003/diff/20001/base/json/json_writer_u...
base/json/json_writer_unittest.cc:15: Value* root =
Value::CreateNullValue().release();
On 2015/05/08 18:07:25, danakj wrote:
> can you change |root| to a scoped_ptr?

I could, but then I have to add like 12 .get()s and rewrite a bunch of operator=
lines. JSONWriter::Write should probably take a const ref, and we can stack
allocate all these values. So I don't think there's much benefit in updating
this code right now.

https://codereview.chromium.org/1129083003/diff/20001/base/values.cc
File base/values.cc (right):

https://codereview.chromium.org/1129083003/diff/20001/base/values.cc#newcode136
base/values.cc:136: Value* Value::DeepCopy() const {
On 2015/05/08 18:07:26, danakj wrote:
> TODO to return scoped_ptr?

See CreateDeepCopy below. There's a TODO in the header to get rid of this
function altogether.

https://codereview.chromium.org/1129083003/diff/20001/base/values_unittest.cc
File base/values_unittest.cc (right):

https://codereview.chromium.org/1129083003/diff/20001/base/values_unittest.cc...
base/values_unittest.cc:392: Value* original_null =
Value::CreateNullValue().release();
On 2015/05/08 18:07:26, danakj wrote:
> can you make original_null a scoped_ptr?

not really --- it's referenced below; this test needs to maintain a weak ref.

https://codereview.chromium.org/1129083003/diff/20001/base/values_unittest.cc...
base/values_unittest.cc:595: Value* original_null =
Value::CreateNullValue().release();
On 2015/05/08 18:07:26, danakj wrote:
> can you make original_null a scoped_ptr too?

Done.

[danakj, 2015-05-08 18:33:42]

https://codereview.chromium.org/1129083003/diff/20001/base/json/json_writer_u...
File base/json/json_writer_unittest.cc (right):

https://codereview.chromium.org/1129083003/diff/20001/base/json/json_writer_u...
base/json/json_writer_unittest.cc:15: Value* root =
Value::CreateNullValue().release();
On 2015/05/08 18:32:09, Evan Stade wrote:
> On 2015/05/08 18:07:25, danakj wrote:
> > can you change |root| to a scoped_ptr?
> 
> I could, but then I have to add like 12 .get()s and rewrite a bunch of
operator=
> lines. JSONWriter::Write should probably take a const ref, and we can stack
> allocate all these values. So I don't think there's much benefit in updating
> this code right now.

I think storing owned pointers in raw pointers is undesirable and we should use
a scoped_ptr for it to express ownership.

https://codereview.chromium.org/1129083003/diff/20001/base/values_unittest.cc
File base/values_unittest.cc (right):

https://codereview.chromium.org/1129083003/diff/20001/base/values_unittest.cc...
base/values_unittest.cc:392: Value* original_null =
Value::CreateNullValue().release();
On 2015/05/08 18:32:09, Evan Stade wrote:
> On 2015/05/08 18:07:26, danakj wrote:
> > can you make original_null a scoped_ptr?
> 
> not really --- it's referenced below; this test needs to maintain a weak ref.

Can you put the weak ref in a separate variable so we don't express ownership
with a raw pointer?

[Evan Stade, 2015-05-08 18:38:27]

https://codereview.chromium.org/1129083003/diff/20001/base/json/json_writer_u...
File base/json/json_writer_unittest.cc (right):

https://codereview.chromium.org/1129083003/diff/20001/base/json/json_writer_u...
base/json/json_writer_unittest.cc:15: Value* root =
Value::CreateNullValue().release();
On 2015/05/08 18:33:41, danakj wrote:
> On 2015/05/08 18:32:09, Evan Stade wrote:
> > On 2015/05/08 18:07:25, danakj wrote:
> > > can you change |root| to a scoped_ptr?
> > 
> > I could, but then I have to add like 12 .get()s and rewrite a bunch of
> operator=
> > lines. JSONWriter::Write should probably take a const ref, and we can stack
> > allocate all these values. So I don't think there's much benefit in updating
> > this code right now.
> 
> I think storing owned pointers in raw pointers is undesirable and we should
use
> a scoped_ptr for it to express ownership.

We should not be using heap allocation in the first place. Updating this code
from one form of bad to another is not a good use of time when it should ideally
be changing to "good" soon.

https://codereview.chromium.org/1129083003/diff/20001/base/values_unittest.cc
File base/values_unittest.cc (right):

https://codereview.chromium.org/1129083003/diff/20001/base/values_unittest.cc...
base/values_unittest.cc:392: Value* original_null =
Value::CreateNullValue().release();
On 2015/05/08 18:33:42, danakj wrote:
> On 2015/05/08 18:32:09, Evan Stade wrote:
> > On 2015/05/08 18:07:26, danakj wrote:
> > > can you make original_null a scoped_ptr?
> > 
> > not really --- it's referenced below; this test needs to maintain a weak
ref.
> 
> Can you put the weak ref in a separate variable so we don't express ownership
> with a raw pointer?

I don't think expressing ownership with a raw pointer is that bad if

a) it's a test file
b) ownership is handed off on literally the next line of code

Notice how every other function in this test uses make_scoped_ptr for the same
reason.

[danakj, 2015-05-08 18:48:28]

https://codereview.chromium.org/1129083003/diff/20001/base/values_unittest.cc
File base/values_unittest.cc (right):

https://codereview.chromium.org/1129083003/diff/20001/base/values_unittest.cc...
base/values_unittest.cc:392: Value* original_null =
Value::CreateNullValue().release();
On 2015/05/08 18:38:27, Evan Stade wrote:
> On 2015/05/08 18:33:42, danakj wrote:
> > On 2015/05/08 18:32:09, Evan Stade wrote:
> > > On 2015/05/08 18:07:26, danakj wrote:
> > > > can you make original_null a scoped_ptr?
> > > 
> > > not really --- it's referenced below; this test needs to maintain a weak
> ref.
> > 
> > Can you put the weak ref in a separate variable so we don't express
ownership
> > with a raw pointer?
> 
> I don't think expressing ownership with a raw pointer is that bad if
> 
> a) it's a test file
> b) ownership is handed off on literally the next line of code
> 
> Notice how every other function in this test uses make_scoped_ptr for the same
> reason.

This test was written before scoped_ptr existed from what I can see in blame.

[Evan Stade, 2015-05-08 18:54:13]

https://codereview.chromium.org/1129083003/diff/20001/base/values_unittest.cc
File base/values_unittest.cc (right):

https://codereview.chromium.org/1129083003/diff/20001/base/values_unittest.cc...
base/values_unittest.cc:392: Value* original_null =
Value::CreateNullValue().release();
On 2015/05/08 18:48:28, danakj wrote:
> On 2015/05/08 18:38:27, Evan Stade wrote:
> > On 2015/05/08 18:33:42, danakj wrote:
> > > On 2015/05/08 18:32:09, Evan Stade wrote:
> > > > On 2015/05/08 18:07:26, danakj wrote:
> > > > > can you make original_null a scoped_ptr?
> > > > 
> > > > not really --- it's referenced below; this test needs to maintain a weak
> > ref.
> > > 
> > > Can you put the weak ref in a separate variable so we don't express
> ownership
> > > with a raw pointer?
> > 
> > I don't think expressing ownership with a raw pointer is that bad if
> > 
> > a) it's a test file
> > b) ownership is handed off on literally the next line of code
> > 
> > Notice how every other function in this test uses make_scoped_ptr for the
same
> > reason.
> 
> This test was written before scoped_ptr existed from what I can see in blame.

All the lines with "make_scoped_ptr" were not written prior to the introduction
of scoped_ptr.

[danakj, 2015-05-08 19:09:37]

danakj@chromium.org changed reviewers:
- danakj@chromium.org

[danakj, 2015-05-09 00:12:52]

Fine. I was excited about seeing more scoped_ptr usage instead of raw ptr
ownership, so I wanted to help this land. Changing a couple tests to be
more robust in their ownership by using scoped_ptr correctly would have
taken far less time than this conversation. in the interest of not getting
in the way then I removed myself from reviewer again and you can grab
another base reviewer (brettw is not anymore which is why I jumped in).
On May 8, 2015 11:54 AM, <estade@chromium.org> wrote:

>
>
> https://codereview.chromium.org/1129083003/diff/20001/base/values_unittest.cc
> File base/values_unittest.cc (right):
>
>
>
https://codereview.chromium.org/1129083003/diff/20001/base/values_unittest.cc...
> base/values_unittest.cc:392: Value* original_null =
> Value::CreateNullValue().release();
> On 2015/05/08 18:48:28, danakj wrote:
>
>> On 2015/05/08 18:38:27, Evan Stade wrote:
>> > On 2015/05/08 18:33:42, danakj wrote:
>> > > On 2015/05/08 18:32:09, Evan Stade wrote:
>> > > > On 2015/05/08 18:07:26, danakj wrote:
>> > > > > can you make original_null a scoped_ptr?
>> > > >
>> > > > not really --- it's referenced below; this test needs to
>>
> maintain a weak
>
>> > ref.
>> > >
>> > > Can you put the weak ref in a separate variable so we don't
>>
> express
>
>> ownership
>> > > with a raw pointer?
>> >
>> > I don't think expressing ownership with a raw pointer is that bad if
>> >
>> > a) it's a test file
>> > b) ownership is handed off on literally the next line of code
>> >
>> > Notice how every other function in this test uses make_scoped_ptr
>>
> for the same
>
>> > reason.
>>
>
>  This test was written before scoped_ptr existed from what I can see in
>>
> blame.
>
> All the lines with "make_scoped_ptr" were not written prior to the
> introduction of scoped_ptr.
>
> https://codereview.chromium.org/1129083003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Evan Stade, 2015-05-09 00:58:14]

thanks for the assistance.

git cl owners seems to approve brettw as a reviewer for this whole change (he's
in src/OWNERS).

I have a WIP here which addresses your concerns wrt one of the tests in this CL:
https://codereview.chromium.org/1131113004/

On 2015/05/09 00:12:52, danakj wrote:
> Fine. I was excited about seeing more scoped_ptr usage instead of raw ptr
> ownership, so I wanted to help this land. Changing a couple tests to be
> more robust in their ownership by using scoped_ptr correctly would have
> taken far less time than this conversation. in the interest of not getting
> in the way then I removed myself from reviewer again and you can grab
> another base reviewer (brettw is not anymore which is why I jumped in).
> On May 8, 2015 11:54 AM, <mailto:estade@chromium.org> wrote:
> 
> >
> >
> >
https://codereview.chromium.org/1129083003/diff/20001/base/values_unittest.cc
> > File base/values_unittest.cc (right):
> >
> >
> >
>
https://codereview.chromium.org/1129083003/diff/20001/base/values_unittest.cc...
> > base/values_unittest.cc:392: Value* original_null =
> > Value::CreateNullValue().release();
> > On 2015/05/08 18:48:28, danakj wrote:
> >
> >> On 2015/05/08 18:38:27, Evan Stade wrote:
> >> > On 2015/05/08 18:33:42, danakj wrote:
> >> > > On 2015/05/08 18:32:09, Evan Stade wrote:
> >> > > > On 2015/05/08 18:07:26, danakj wrote:
> >> > > > > can you make original_null a scoped_ptr?
> >> > > >
> >> > > > not really --- it's referenced below; this test needs to
> >>
> > maintain a weak
> >
> >> > ref.
> >> > >
> >> > > Can you put the weak ref in a separate variable so we don't
> >>
> > express
> >
> >> ownership
> >> > > with a raw pointer?
> >> >
> >> > I don't think expressing ownership with a raw pointer is that bad if
> >> >
> >> > a) it's a test file
> >> > b) ownership is handed off on literally the next line of code
> >> >
> >> > Notice how every other function in this test uses make_scoped_ptr
> >>
> > for the same
> >
> >> > reason.
> >>
> >
> >  This test was written before scoped_ptr existed from what I can see in
> >>
> > blame.
> >
> > All the lines with "make_scoped_ptr" were not written prior to the
> > introduction of scoped_ptr.
> >
> > https://codereview.chromium.org/1129083003/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[brettw, 2015-05-12 00:22:30]

LGTM if Dana is happy.

[Evan Stade, 2015-05-12 16:14:37]

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

[Evan Stade, 2015-05-12 16:14:37]

The patchset sent to the CQ was uploaded after l-g-t-m from brettw@chromium.org
Link to the patchset: https://codereview.chromium.org/1129083003/#ps80001
(title: "fix gpu internals, resolve merge conflicts")

[commit-bot: I haz the power, 2015-05-12 16:14:48]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1129083003/80001

[commit-bot: I haz the power, 2015-05-12 16:34:17]

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

[commit-bot: I haz the power, 2015-05-12 16:34:18]

Dry run: Try jobs failed on following builders:
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Evan Stade, 2015-05-12 16:44:28]

On 2015/05/12 00:22:30, brettw wrote:
> LGTM if Dana is happy.

Dana^

[danakj, 2015-05-12 16:49:48]

LGTM

[Evan Stade, 2015-05-12 17:17:57]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2015-05-12 17:17:57]

The patchset sent to the CQ was uploaded after l-g-t-m from brettw@chromium.org
Link to the patchset: https://codereview.chromium.org/1129083003/#ps100001
(title: "fix bad merge")

[commit-bot: I haz the power, 2015-05-12 17:18:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1129083003/100001

[commit-bot: I haz the power, 2015-05-12 18:12:03]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-05-12 18:12:41]

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