Member initialization for scoped_refptr<T>::ptr_.

Member initialization for scoped_refptr<T>::ptr_.

Also augment OpaqueRefCounted tests to cover move constructors
and RefCountedThreadSafe.

BUG=653916
Committed: https://crrev.com/37af9ea6080ce52e001189255512bd5bfa1c6c45
Cr-Commit-Position: refs/heads/master@{#428746}

[gab, 2016-10-27 15:29:03]

Description was changed from

==========
Member initialization for scoped_refptr<T>::ptr_.

Also add RefCountedThreadSafe to OpaqueRefCounted tests

BUG=653916
==========

to

==========
Member initialization for scoped_refptr<T>::ptr_.

Also augment OpaqueRefCounted tests to cover move constructors
and RefCountedThreadSafe.

BUG=653916
==========

[gab, 2016-10-27 15:29:07]

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

[commit-bot: I haz the power, 2016-10-27 15:29:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[gab, 2016-10-27 15:31:47]

The CQ bit was unchecked by gab@chromium.org

[gab, 2016-10-27 15:32:07]

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

[commit-bot: I haz the power, 2016-10-27 15:32: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, 2016-10-27 15:52:22]

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

[commit-bot: I haz the power, 2016-10-27 15:52:23]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[gab, 2016-10-27 18:03:34]

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

[commit-bot: I haz the power, 2016-10-27 18:04:13]

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, 2016-10-27 18:05:57]

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

[commit-bot: I haz the power, 2016-10-27 18:05:58]

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...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[gab, 2016-10-27 18:07:34]

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

[commit-bot: I haz the power, 2016-10-27 18:08:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[gab, 2016-10-27 18:34:33]

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

[gab, 2016-10-27 18:34:34]

Dana PTAL, for some reason which I don't understand: {} compiles but not =
default; ?! (see PS1)

Improved coverage of OpaqueRefCounted tests which initially led me to believe
full-type definition isn't required after all... but it appears to depend on the
declaration of

extern template class scoped_refptr<base::OpaqueRefCounted>;

which I haven't found a single matching instance of in the codebase:

$ git grep -n "template class scoped_refptr"
base/memory/ref_counted.h:391:  //     extern template class
scoped_refptr<Opaque>;
base/test/opaque_ref_counted.cc:58:template class
scoped_refptr<base::OpaqueRefCounted>;
base/test/opaque_ref_counted.cc:59:template class
scoped_refptr<base::OpaqueRefCountedThreadSafe>;
base/test/opaque_ref_counted.h:26:extern template class
scoped_refptr<base::OpaqueRefCounted>;
base/test/opaque_ref_counted.h:27:extern template class
scoped_refptr<base::OpaqueRefCountedThreadSafe>;

[gab, 2016-10-27 18:37:51]

On 2016/10/27 18:34:34, gab wrote:
> Dana PTAL, for some reason which I don't understand: {} compiles but not =
> default; ?! (see PS1)
> 
> Improved coverage of OpaqueRefCounted tests which initially led me to believe
> full-type definition isn't required after all... but it appears to depend on
the
> declaration of
> 
> extern template class scoped_refptr<base::OpaqueRefCounted>;
> 
> which I haven't found a single matching instance of in the codebase:
> 
> $ git grep -n "template class scoped_refptr"
> base/memory/ref_counted.h:391:  //     extern template class
> scoped_refptr<Opaque>;
> base/test/opaque_ref_counted.cc:58:template class
> scoped_refptr<base::OpaqueRefCounted>;
> base/test/opaque_ref_counted.cc:59:template class
> scoped_refptr<base::OpaqueRefCountedThreadSafe>;
> base/test/opaque_ref_counted.h:26:extern template class
> scoped_refptr<base::OpaqueRefCounted>;
> base/test/opaque_ref_counted.h:27:extern template class
> scoped_refptr<base::OpaqueRefCountedThreadSafe>;

FYI, I plan to figure exactly how to do the scoped_refptr fwd-declaration in
https://codereview.chromium.org/2443103003/ and then inform chromium-dev.

[commit-bot: I haz the power, 2016-10-27 20:06:38]

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

[commit-bot: I haz the power, 2016-10-27 20:06:39]

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

[danakj, 2016-10-27 22:10:35]

On 2016/10/27 18:37:51, gab wrote:
> On 2016/10/27 18:34:34, gab wrote:
> > Dana PTAL, for some reason which I don't understand: {} compiles but not =
> > default; ?! (see PS1)

= default isn't user-defined so it ends up tickling different parts of
compilers. Found a bug today where xcode's compiler can't handle a constexpr =
default constructor but can handle constexpr {} so.. yah.

> > Improved coverage of OpaqueRefCounted tests which initially led me to
believe
> > full-type definition isn't required after all... but it appears to depend on
> the
> > declaration of
> > 
> > extern template class scoped_refptr<base::OpaqueRefCounted>;
> > 
> > which I haven't found a single matching instance of in the codebase:
> > 
> > $ git grep -n "template class scoped_refptr"
> > base/memory/ref_counted.h:391:  //     extern template class
> > scoped_refptr<Opaque>;
> > base/test/opaque_ref_counted.cc:58:template class
> > scoped_refptr<base::OpaqueRefCounted>;
> > base/test/opaque_ref_counted.cc:59:template class
> > scoped_refptr<base::OpaqueRefCountedThreadSafe>;
> > base/test/opaque_ref_counted.h:26:extern template class
> > scoped_refptr<base::OpaqueRefCounted>;
> > base/test/opaque_ref_counted.h:27:extern template class
> > scoped_refptr<base::OpaqueRefCountedThreadSafe>;
> 
> FYI, I plan to figure exactly how to do the scoped_refptr fwd-declaration in
> https://codereview.chromium.org/2443103003/ and then inform chromium-dev.

[danakj, 2016-10-27 22:10:54]

https://codereview.chromium.org/2446403006/diff/40001/base/memory/ref_counted...
File base/memory/ref_counted_unittest.cc (right):

https://codereview.chromium.org/2446403006/diff/40001/base/memory/ref_counted...
base/memory/ref_counted_unittest.cc:173: scoped_refptr<base::OpaqueRefCounted>
moved(std::move(initial));
Isn't this just testing different ways to make a scoped_refptr? Once you have
one, all called to TestOpaqueRefCounted are the same at that point?

[gab, 2016-10-28 14:02:27]

https://codereview.chromium.org/2446403006/diff/40001/base/memory/ref_counted...
File base/memory/ref_counted_unittest.cc (right):

https://codereview.chromium.org/2446403006/diff/40001/base/memory/ref_counted...
base/memory/ref_counted_unittest.cc:173: scoped_refptr<base::OpaqueRefCounted>
moved(std::move(initial));
On 2016/10/27 22:10:54, danakj wrote:
> Isn't this just testing different ways to make a scoped_refptr? Once you have
> one, all called to TestOpaqueRefCounted are the same at that point?

Right I guess the multiple calls to TestOpaqueRefCounted() aren't verifying much
(i.e. the test is mostly about making sure it compiles not about logic testing
so the multiple calls aren't necessarily helpful -- other than to verify the
move happened correctly but other tests are already testing move so this is
perhaps overkill, WDYT?)

[gab, 2016-10-28 14:02:57]

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

[commit-bot: I haz the power, 2016-10-28 14:03:16]

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, 2016-10-28 17:37:19]

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

[commit-bot: I haz the power, 2016-10-28 17:37:20]

Dry run: This issue passed the CQ dry run.

[danakj, 2016-10-29 00:49:09]

https://codereview.chromium.org/2446403006/diff/40001/base/memory/ref_counted...
File base/memory/ref_counted_unittest.cc (right):

https://codereview.chromium.org/2446403006/diff/40001/base/memory/ref_counted...
base/memory/ref_counted_unittest.cc:173: scoped_refptr<base::OpaqueRefCounted>
moved(std::move(initial));
On 2016/10/28 14:02:26, gab wrote:
> On 2016/10/27 22:10:54, danakj wrote:
> > Isn't this just testing different ways to make a scoped_refptr? Once you
have
> > one, all called to TestOpaqueRefCounted are the same at that point?
> 
> Right I guess the multiple calls to TestOpaqueRefCounted() aren't verifying
much
> (i.e. the test is mostly about making sure it compiles not about logic testing
> so the multiple calls aren't necessarily helpful -- other than to verify the
> move happened correctly but other tests are already testing move so this is
> perhaps overkill, WDYT?)

Yes if u wanna test them do it as a separate test if its not redundant with
other tests, but no need to call TestOpaqueRefCounted after each one :)

[danakj, 2016-10-29 00:50:00]

LGTM otherwise

[gab, 2016-10-31 15:01:15]

Thanks, done

https://codereview.chromium.org/2446403006/diff/40001/base/memory/ref_counted...
File base/memory/ref_counted_unittest.cc (right):

https://codereview.chromium.org/2446403006/diff/40001/base/memory/ref_counted...
base/memory/ref_counted_unittest.cc:173: scoped_refptr<base::OpaqueRefCounted>
moved(std::move(initial));
On 2016/10/29 00:49:09, danakj wrote:
> On 2016/10/28 14:02:26, gab wrote:
> > On 2016/10/27 22:10:54, danakj wrote:
> > > Isn't this just testing different ways to make a scoped_refptr? Once you
> have
> > > one, all called to TestOpaqueRefCounted are the same at that point?
> > 
> > Right I guess the multiple calls to TestOpaqueRefCounted() aren't verifying
> much
> > (i.e. the test is mostly about making sure it compiles not about logic
testing
> > so the multiple calls aren't necessarily helpful -- other than to verify the
> > move happened correctly but other tests are already testing move so this is
> > perhaps overkill, WDYT?)
> 
> Yes if u wanna test them do it as a separate test if its not redundant with
> other tests, but no need to call TestOpaqueRefCounted after each one :)

Removed extraneous calls.

[gab, 2016-10-31 15:02:20]

The CQ bit was checked by gab@chromium.org

[gab, 2016-10-31 15:02: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/2446403006/#ps60001
(title: "remove extraneous TestOpaqueRefCounted calls")

[commit-bot: I haz the power, 2016-10-31 15:02:33]

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, 2016-10-31 17:03:51]

Description was changed from

==========
Member initialization for scoped_refptr<T>::ptr_.

Also augment OpaqueRefCounted tests to cover move constructors
and RefCountedThreadSafe.

BUG=653916
==========

to

==========
Member initialization for scoped_refptr<T>::ptr_.

Also augment OpaqueRefCounted tests to cover move constructors
and RefCountedThreadSafe.

BUG=653916
==========

[commit-bot: I haz the power, 2016-10-31 17:03:52]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-10-31 17:16:06]

Description was changed from

==========
Member initialization for scoped_refptr<T>::ptr_.

Also augment OpaqueRefCounted tests to cover move constructors
and RefCountedThreadSafe.

BUG=653916
==========

to

==========
Member initialization for scoped_refptr<T>::ptr_.

Also augment OpaqueRefCounted tests to cover move constructors
and RefCountedThreadSafe.

BUG=653916

Committed: https://crrev.com/37af9ea6080ce52e001189255512bd5bfa1c6c45
Cr-Commit-Position: refs/heads/master@{#428746}
==========

[commit-bot: I haz the power, 2016-10-31 17:16:07]

Patchset 4 (id:??) landed as
https://crrev.com/37af9ea6080ce52e001189255512bd5bfa1c6c45
Cr-Commit-Position: refs/heads/master@{#428746}