Move ThreadWrapper to ProcessThread in base.

Move ThreadWrapper to ProcessThread in base.

Also removes all virtual methods. Permits using a thread from
rtc_base_approved (namely event tracing).

BUG=webrtc:5158
R=tommi@webrtc.org

Committed: https://crrev.com/12411ef40e08c5e28ccde54ab3418c96676ffcbc
Cr-Commit-Position: refs/heads/master@{#10760}

[pbos-webrtc, 2015-11-23 12:33:55]

make constructor public

[pbos-webrtc, 2015-11-23 12:35:02]

PTAL, lemme know if I can bribe you somehow. :)

[pbos-webrtc, 2015-11-23 12:35:09]

The CQ bit was checked by pbos@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2015-11-23 12:35:28]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1469013002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1469013002/20001

[commit-bot: I haz the power, 2015-11-23 12:36:32]

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

[commit-bot: I haz the power, 2015-11-23 12:36:32]

Dry run: Try jobs failed on following builders:
  android_compile_x64_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_compile_x64_dbg...)
  android_compile_x86_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_compile_x86_dbg...)
  android_gn_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_gn_rel/builds/7161)
  linux_asan on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_asan/builds/10790)
  linux_compile_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_compile_dbg/build...)
  linux_gn_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_gn_dbg/builds/6957)
  linux_msan on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_msan/builds/6126)
  linux_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_rel/builds/10964)
  linux_tsan2 on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_tsan2/builds/8179)
  presubmit on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/1900)

[pbos-webrtc, 2015-11-23 12:40:25]

rebase

[pbos-webrtc, 2015-11-23 12:40:52]

The CQ bit was checked by pbos@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2015-11-23 12:41:16]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1469013002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1469013002/40001

[commit-bot: I haz the power, 2015-11-23 12:43:44]

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

[commit-bot: I haz the power, 2015-11-23 12:43:45]

Dry run: Try jobs failed on following builders:
  android_gn_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_gn_rel/builds/7162)
  mac_compile_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_dbg/builds/...)
  mac_compile_x64_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_x64_dbg/bui...)
  mac_x64_gn_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_x64_gn_dbg/builds/5437)
  mac_x64_gn_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_x64_gn_rel/builds/5385)
  win_x64_gn_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_gn_dbg/builds/5400)

[pbos-webrtc, 2015-11-23 13:22:52]

include string for name_

[pbos-webrtc, 2015-11-23 13:23:06]

The CQ bit was checked by pbos@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2015-11-23 13:23:22]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1469013002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1469013002/60001

[pbos-webrtc, 2015-11-23 13:27:28]

remove duplicate win ThreadChecker

[pbos-webrtc, 2015-11-23 14:45:04]

rebase

[tommi, 2015-11-23 14:49:12]

first set of comments. Didn't look through every file but assume there's a lot
of boilerplate updates.

https://codereview.webrtc.org/1469013002/diff/80001/webrtc/base/platform_thre...
File webrtc/base/platform_thread.cc (right):

https://codereview.webrtc.org/1469013002/diff/80001/webrtc/base/platform_thre...
webrtc/base/platform_thread.cc:93: .Pass();
std::move?

https://codereview.webrtc.org/1469013002/diff/80001/webrtc/base/platform_thre...
webrtc/base/platform_thread.cc:112: int
PlatformThread::ConvertToSystemPriority(ThreadPriority priority,
actually, I took a look at the test that calls this method and I'm not sure it's
that useful and it only exists for posix.  We could just delete it.

https://codereview.webrtc.org/1469013002/diff/80001/webrtc/base/platform_thre...
File webrtc/base/platform_thread.h (right):

https://codereview.webrtc.org/1469013002/diff/80001/webrtc/base/platform_thre...
webrtc/base/platform_thread.h:104: static uint32_t GetThreadId();
this should be gone now (cl in cq)

https://codereview.webrtc.org/1469013002/diff/80001/webrtc/base/platform_thre...
webrtc/base/platform_thread.h:126: static int
ConvertToSystemPriority(ThreadPriority priority,
this should be in the posix implementation only and it doesn't need to be a part
of PlatformThread.
If the test needs to fwd declare it specially, I'd rather do that than pollute
the interface of this class.

https://codereview.webrtc.org/1469013002/diff/80001/webrtc/base/platform_thre...
webrtc/base/platform_thread.h:136: // TODO(pbos): Make sure call sites use
string constants and update to a const
nit: s/string constants/literals

https://codereview.webrtc.org/1469013002/diff/80001/webrtc/base/platform_thre...
webrtc/base/platform_thread.h:139: rtc::scoped_ptr<rtc::ThreadChecker>
thread_checker_;
as per offline discussion, split PlatformThreadId and Ref out to
platform_thread_types.h so that we don't need to do this.

https://codereview.webrtc.org/1469013002/diff/80001/webrtc/base/platform_thre...
File webrtc/base/platform_thread_unittest.cc (right):

https://codereview.webrtc.org/1469013002/diff/80001/webrtc/base/platform_thre...
webrtc/base/platform_thread_unittest.cc:42: rtc::scoped_ptr<PlatformThread>
thread = PlatformThread::CreateThread(
do we need CreateThread() now that the implementation doesn't need to be
virtual?

https://codereview.webrtc.org/1469013002/diff/80001/webrtc/base/platform_thre...
webrtc/base/platform_thread_unittest.cc:54: TEST(PlatformThreadTestPosix,
PrioritySettings) {
delete this test?

[tommi, 2015-11-23 14:49:43]

I guess most importantly - do you intend to do the 'no vtable' change now or
later?

[pbos-webrtc, 2015-11-23 14:55:10]

threadchecker + split out platform_thread_tyeps.h

[pbos-webrtc, 2015-11-23 15:05:10]

feedback from the tomminator

[pbos-webrtc, 2015-11-23 15:05:40]

PTAL, the vtables are gone now but I'd like to follow up with CreateThread
removal separately.

https://codereview.webrtc.org/1469013002/diff/80001/webrtc/base/platform_thre...
File webrtc/base/platform_thread.cc (right):

https://codereview.webrtc.org/1469013002/diff/80001/webrtc/base/platform_thre...
webrtc/base/platform_thread.cc:93: .Pass();
On 2015/11/23 14:49:11, tommi (webrtc) wrote:
> std::move?

Done.

https://codereview.webrtc.org/1469013002/diff/80001/webrtc/base/platform_thre...
webrtc/base/platform_thread.cc:112: int
PlatformThread::ConvertToSystemPriority(ThreadPriority priority,
On 2015/11/23 14:49:11, tommi (webrtc) wrote:
> actually, I took a look at the test that calls this method and I'm not sure
it's
> that useful and it only exists for posix.  We could just delete it.

Deleted, nuked, gone, done (test is that is).

https://codereview.webrtc.org/1469013002/diff/80001/webrtc/base/platform_thre...
File webrtc/base/platform_thread.h (right):

https://codereview.webrtc.org/1469013002/diff/80001/webrtc/base/platform_thre...
webrtc/base/platform_thread.h:104: static uint32_t GetThreadId();
On 2015/11/23 14:49:11, tommi (webrtc) wrote:
> this should be gone now (cl in cq)

Done.

https://codereview.webrtc.org/1469013002/diff/80001/webrtc/base/platform_thre...
webrtc/base/platform_thread.h:126: static int
ConvertToSystemPriority(ThreadPriority priority,
On 2015/11/23 14:49:12, tommi (webrtc) wrote:
> this should be in the posix implementation only and it doesn't need to be a
part
> of PlatformThread.
> If the test needs to fwd declare it specially, I'd rather do that than pollute
> the interface of this class.

Done.

https://codereview.webrtc.org/1469013002/diff/80001/webrtc/base/platform_thre...
webrtc/base/platform_thread.h:136: // TODO(pbos): Make sure call sites use
string constants and update to a const
On 2015/11/23 14:49:11, tommi (webrtc) wrote:
> nit: s/string constants/literals

Done.

https://codereview.webrtc.org/1469013002/diff/80001/webrtc/base/platform_thre...
webrtc/base/platform_thread.h:139: rtc::scoped_ptr<rtc::ThreadChecker>
thread_checker_;
On 2015/11/23 14:49:11, tommi (webrtc) wrote:
> as per offline discussion, split PlatformThreadId and Ref out to
> platform_thread_types.h so that we don't need to do this.

Done.

https://codereview.webrtc.org/1469013002/diff/80001/webrtc/base/platform_thre...
File webrtc/base/platform_thread_unittest.cc (right):

https://codereview.webrtc.org/1469013002/diff/80001/webrtc/base/platform_thre...
webrtc/base/platform_thread_unittest.cc:42: rtc::scoped_ptr<PlatformThread>
thread = PlatformThread::CreateThread(
On 2015/11/23 14:49:12, tommi (webrtc) wrote:
> do we need CreateThread() now that the implementation doesn't need to be
> virtual?

Nope, hence the TODO in the header. Preferring to do it separately.

https://codereview.webrtc.org/1469013002/diff/80001/webrtc/base/platform_thre...
webrtc/base/platform_thread_unittest.cc:54: TEST(PlatformThreadTestPosix,
PrioritySettings) {
On 2015/11/23 14:49:12, tommi (webrtc) wrote:
> delete this test?

Done.

[pbos-webrtc, 2015-11-23 15:20:40]

we don't need no stinkin std::move

[tommi, 2015-11-23 15:54:29]

lgtm % nits.

I didn't review some of the files since I'm assuming this is a search/replace
kinda thing and you ran git cl format (right?)

https://codereview.webrtc.org/1469013002/diff/150001/webrtc/base/platform_thr...
File webrtc/base/platform_thread.cc (left):

https://codereview.webrtc.org/1469013002/diff/150001/webrtc/base/platform_thr...
webrtc/base/platform_thread.cc:61: RTC_DCHECK(strlen(name) < 64);
why remove?

https://codereview.webrtc.org/1469013002/diff/150001/webrtc/base/platform_thr...
File webrtc/base/platform_thread.h (right):

https://codereview.webrtc.org/1469013002/diff/150001/webrtc/base/platform_thr...
webrtc/base/platform_thread.h:36: namespace webrtc {
nit: threading?
(or is there a reason for having rtc::webrtc?)

https://codereview.webrtc.org/1469013002/diff/150001/webrtc/base/platform_thr...
File webrtc/base/platform_thread_types.h (right):

https://codereview.webrtc.org/1469013002/diff/150001/webrtc/base/platform_thr...
webrtc/base/platform_thread_types.h:13: // This file exists so that
PlatformThread can make use of ThreadChecker which
nit: I think you can skip this comment.  It just raises more questions :)

[pbos-webrtc, 2015-11-23 16:18:04]

removed comment

[pbos-webrtc, 2015-11-23 16:18:23]

https://codereview.webrtc.org/1469013002/diff/150001/webrtc/base/platform_thr...
File webrtc/base/platform_thread.cc (left):

https://codereview.webrtc.org/1469013002/diff/150001/webrtc/base/platform_thr...
webrtc/base/platform_thread.cc:61: RTC_DCHECK(strlen(name) < 64);
On 2015/11/23 15:54:28, tommi (webrtc) wrote:
> why remove?

Checked in ctor for all platforms now.

https://codereview.webrtc.org/1469013002/diff/150001/webrtc/base/platform_thr...
File webrtc/base/platform_thread.h (right):

https://codereview.webrtc.org/1469013002/diff/150001/webrtc/base/platform_thr...
webrtc/base/platform_thread.h:36: namespace webrtc {
On 2015/11/23 15:54:28, tommi (webrtc) wrote:
> nit: threading?
> (or is there a reason for having rtc::webrtc?)

Not under rtc, this is just webrtc::. Not moved yet because I'll have to update
a bunch of files. Will probably do this with CreateThread.

https://codereview.webrtc.org/1469013002/diff/150001/webrtc/base/platform_thr...
File webrtc/base/platform_thread_types.h (right):

https://codereview.webrtc.org/1469013002/diff/150001/webrtc/base/platform_thr...
webrtc/base/platform_thread_types.h:13: // This file exists so that
PlatformThread can make use of ThreadChecker which
On 2015/11/23 15:54:28, tommi (webrtc) wrote:
> nit: I think you can skip this comment.  It just raises more questions :)

Done.

[pbos-webrtc, 2015-11-23 16:19:08]

The CQ bit was checked by pbos@webrtc.org

[pbos-webrtc, 2015-11-23 16:19:09]

The patchset sent to the CQ was uploaded after l-g-t-m from tommi@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1469013002/#ps170001 (title:
"removed comment")

[commit-bot: I haz the power, 2015-11-23 16:19:32]

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

[tommi, 2015-11-23 16:51:50]

lgtm

https://codereview.webrtc.org/1469013002/diff/150001/webrtc/base/platform_thr...
File webrtc/base/platform_thread.h (right):

https://codereview.webrtc.org/1469013002/diff/150001/webrtc/base/platform_thr...
webrtc/base/platform_thread.h:36: namespace webrtc {
On 2015/11/23 16:18:22, pbos-webrtc wrote:
> On 2015/11/23 15:54:28, tommi (webrtc) wrote:
> > nit: threading?
> > (or is there a reason for having rtc::webrtc?)
> 
> Not under rtc, this is just webrtc::. Not moved yet because I'll have to
update
> a bunch of files. Will probably do this with CreateThread.

Acknowledged.

[commit-bot: I haz the power, 2015-11-23 18:19:49]

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

[commit-bot: I haz the power, 2015-11-23 18:19:50]

Try jobs failed on following builders:
  android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[pbos-webrtc, 2015-11-23 22:45:38]

The CQ bit was checked by pbos@webrtc.org

[commit-bot: I haz the power, 2015-11-23 22:45:59]

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

[commit-bot: I haz the power, 2015-11-23 22:48:00]

Committed patchset #10 (id:170001)

[commit-bot: I haz the power, 2015-11-23 22:48:05]

Patchset 10 (id:??) landed as
https://crrev.com/12411ef40e08c5e28ccde54ab3418c96676ffcbc
Cr-Commit-Position: refs/heads/master@{#10760}