base: Support setting thread priorities generically.

Reland: base: Support setting thread priorities generically.

This patch supports setting priorities across platforms
at the PlatformThread level, by stashing thread id into the
thread handle on linux/android.

Since this adds more platform specific code, and #ifdefs
were starting to get unwieldy, all platform specific code
is moved into _platform.cc files, with the exception of the
'default' implementation, which stay in _posix.

BUG=170549
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=201202

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=201389

[epenner, 2013-03-26 03:36:12]

This is a continuation of this patch (I had to switch my dev branch):
https://codereview.chromium.org/11934002/

This makes it possible to set priorities generically on any platform.

Ptal.

[Wei James(wistoch), 2013-03-26 05:04:54]

The code for Android may not be removed. 

we need to use Java API to change the cgroups of the thread.

[epennerAtGoogle, 2013-03-26 05:15:17]

On 2013/03/26 05:04:54, Wei James(wistoch) wrote:
> The code for Android may not be removed. 
> 
> we need to use Java API to change the cgroups of the thread.

Thanks for the heads up! Can you describe cgroups in more detail? I didn't see
this in the code I removed. We have set thread priorities using native code on
Android (for other purposes) and it hasn't been a problem. If we need too, we
can route all priority setting through Java, but I need to understand why first.

[Wei James(wistoch), 2013-03-26 05:23:45]

On 2013/03/26 05:15:17, epennerAtGoogle wrote:
> On 2013/03/26 05:04:54, Wei James(wistoch) wrote:
> > The code for Android may not be removed. 
> > 
> > we need to use Java API to change the cgroups of the thread.
> 
> Thanks for the heads up! Can you describe cgroups in more detail? I didn't see
> this in the code I removed. We have set thread priorities using native code on
> Android (for other purposes) and it hasn't been a problem. If we need too, we
> can route all priority setting through Java, but I need to understand why
first.

Calling Android SDK API to set priority will setting pthread priority and also
setting cgroups(In the implementation of SDK, it calls setpriority and also
cgroups). 

If the thread is in background, it will use little CPU(<5%) even it has high
thread priority.

[Wei James(wistoch), 2013-03-26 05:34:36]

On 2013/03/26 05:23:45, Wei James(wistoch) wrote:
> On 2013/03/26 05:15:17, epennerAtGoogle wrote:
> > On 2013/03/26 05:04:54, Wei James(wistoch) wrote:
> > > The code for Android may not be removed. 
> > > 
> > > we need to use Java API to change the cgroups of the thread.
> > 
> > Thanks for the heads up! Can you describe cgroups in more detail? I didn't
see
> > this in the code I removed. We have set thread priorities using native code
on
> > Android (for other purposes) and it hasn't been a problem. If we need too,
we
> > can route all priority setting through Java, but I need to understand why
> first.
> 
> Calling Android SDK API to set priority will setting pthread priority and also
> setting cgroups(In the implementation of SDK, it calls setpriority and also
> cgroups). 
> 
> If the thread is in background, it will use little CPU(<5%) even it has high
> thread priority.

in the case of Audio thread, if using NDK API to set thread priority and then
put the audio in background, this thread will use very little CPU. 

if using SDK API to set thread priority, Android will set the thread to
corresponding audio cgroups and this thread will use more CPU even it is in
background.

[qinmin, 2013-03-26 05:41:00]

for cgroups, check set_sched_policy() in
android/system/core/libcutils/sched_policy.c 

On 2013/03/26 05:34:36, Wei James(wistoch) wrote:
> On 2013/03/26 05:23:45, Wei James(wistoch) wrote:
> > On 2013/03/26 05:15:17, epennerAtGoogle wrote:
> > > On 2013/03/26 05:04:54, Wei James(wistoch) wrote:
> > > > The code for Android may not be removed. 
> > > > 
> > > > we need to use Java API to change the cgroups of the thread.
> > > 
> > > Thanks for the heads up! Can you describe cgroups in more detail? I didn't
> see
> > > this in the code I removed. We have set thread priorities using native
code
> on
> > > Android (for other purposes) and it hasn't been a problem. If we need too,
> we
> > > can route all priority setting through Java, but I need to understand why
> > first.
> > 
> > Calling Android SDK API to set priority will setting pthread priority and
also
> > setting cgroups(In the implementation of SDK, it calls setpriority and also
> > cgroups). 
> > 
> > If the thread is in background, it will use little CPU(<5%) even it has high
> > thread priority.
> 
> in the case of Audio thread, if using NDK API to set thread priority and then
> put the audio in background, this thread will use very little CPU. 
> 
> if using SDK API to set thread priority, Android will set the thread to
> corresponding audio cgroups and this thread will use more CPU even it is in
> background.

[epennerAtGoogle, 2013-03-26 05:52:53]

On 2013/03/26 05:41:00, qinmin wrote:
> for cgroups, check set_sched_policy() in
> android/system/core/libcutils/sched_policy.c 
> 
> On 2013/03/26 05:34:36, Wei James(wistoch) wrote:
> > On 2013/03/26 05:23:45, Wei James(wistoch) wrote:
> > > On 2013/03/26 05:15:17, epennerAtGoogle wrote:
> > > > On 2013/03/26 05:04:54, Wei James(wistoch) wrote:
> > > > > The code for Android may not be removed. 
> > > > > 
> > > > > we need to use Java API to change the cgroups of the thread.
> > > > 
> > > > Thanks for the heads up! Can you describe cgroups in more detail? I
didn't
> > see
> > > > this in the code I removed. We have set thread priorities using native
> code
> > on
> > > > Android (for other purposes) and it hasn't been a problem. If we need
too,
> > we
> > > > can route all priority setting through Java, but I need to understand
why
> > > first.
> > > 
> > > Calling Android SDK API to set priority will setting pthread priority and
> also
> > > setting cgroups(In the implementation of SDK, it calls setpriority and
also
> > > cgroups). 
> > > 
> > > If the thread is in background, it will use little CPU(<5%) even it has
high
> > > thread priority.
> > 
> > in the case of Audio thread, if using NDK API to set thread priority and
then
> > put the audio in background, this thread will use very little CPU. 
> > 
> > if using SDK API to set thread priority, Android will set the thread to
> > corresponding audio cgroups and this thread will use more CPU even it is in
> > background.

I see. And there is no way to change the cgroup in native code? Since the
original calls are native it would be nice to stay native and replicate what the
SDK is doing.

If we can't do that and we need to keep the Android code, would you recommend
setting only the audio thread this way then? The other threads should continue
to have low CPU when the application is in the background.

[epenner, 2013-04-18 23:25:51]

Sorry for the long delay. I added back the JNI call for Audio.

Please take a look.

[Wei James(wistoch), 2013-04-19 01:14:19]

On 2013/04/18 23:25:51, epenner wrote:
> Sorry for the long delay. I added back the JNI call for Audio.
> 
> Please take a look.

LGTM for andorid part.

[jar (doing other things), 2013-04-24 01:04:57]

Rather than putting in a ton of ifdefs, can you consider landing in some
platform specific files to hold some of these changes? You are making it look
like posix is not a fine grained enough resolution.  I guess there may be some
repeated code, but it is getting very hard to read this ifdef jungle.

I'm also not clear about why we want to pass in useless parameters (two flavors
of spelling the thread name) on some platforms.  IMO, that makes the code rather
confusing, even if the compiler can perhaps "figure out" that some parms are
unused on some platforms.  When writing code, I'm sure such an API would force
me to look around a lot more than I'd like.

https://codereview.chromium.org/12741012/diff/14001/base/threading/platform_t...
File base/threading/platform_thread.h (left):

https://codereview.chromium.org/12741012/diff/14001/base/threading/platform_t...
base/threading/platform_thread.h:113: 
nit: preserve the white-space line before the private keyword.  I couldn't find
explicit support in the style guide, but *all* the examples they provide had a
blank line before it.... so you should probably at least keep it here.

https://codereview.chromium.org/12741012/diff/14001/base/threading/platform_t...
File base/threading/platform_thread.h (right):

https://codereview.chromium.org/12741012/diff/14001/base/threading/platform_t...
base/threading/platform_thread.h:47: kThreadPriority_Display,
Although I can guess what Normal means, vs Background, I don't know how to fit
in Display and RealtimeAudio.

Can you at a minimum add comments, and probably better would be to also lengthen
the name so that its significance would be clear at point of use (without
reading the comments).

https://codereview.chromium.org/12741012/diff/14001/base/threading/platform_t...
base/threading/platform_thread.h:114: PlatformThreadId id,
Rather than having both, can you unify this in a class some how?  What is the
significance of listing both in call??

https://codereview.chromium.org/12741012/diff/14001/base/threading/platform_t...
File base/threading/platform_thread_posix.cc (right):

https://codereview.chromium.org/12741012/diff/14001/base/threading/platform_t...
base/threading/platform_thread_posix.cc:71: // We use -6 for display, but we may
want to split this
This comment does not relate well to line 64 or 70.  You are not using -6 for
anything now.

https://codereview.chromium.org/12741012/diff/14001/base/threading/platform_t...
base/threading/platform_thread_posix.cc:114: if (!error && prio !=
NiceValue(thread_params->priority)) {
nit: Given the up-front-comment, I'd rather see an "else if" than see a repeat
test for "if (!error && ...".

https://codereview.chromium.org/12741012/diff/14001/base/threading/platform_t...
base/threading/platform_thread_posix.cc:343: LOG(ERROR) << "Failed to set nice
value of thread to " << kNiceSetting;
DLOG is generally preferred.  No one will read this in production, so we'd
rather have this compiled out.

[epennerAtGoogle, 2013-04-24 04:57:14]

Thanks for the feedback!  Most comments seems straight forward to address, but
let me address the "redundant parameter" issue:

The problem is that PlatformThread is very low level. That is good in that you
can use it to operate on 'raw' threads that were not even created using Chrome's
thread abstraction, but it's bad in that it is difficult to fully abstract away
platform differences. For the former, ThreadID and ThreadHandle need to map
directly to native types on each platform. But then the problem is that some
platforms require the ThreadID to set the priority, while some require the
ThreadHandle.

My thinking was that you can just pass both when writing cross platform code,
and the Thread class can abstract this further to hide this since it has both
anyway. So if you want cleaner code but slightly reduced functionality,
thread->SetPriority() is the way to go, whereas PlatformThread still lets you do
anything supported on any platform.

Another option mentioned making ThreadHandle into a struct on some platforms and
hiding the ID in there. But IMO that reduces the flexibility of the API in some
cases.

[epenner, 2013-04-30 04:37:08]

Ptal. I've addressed the feedback and broken it up into platforms.

https://codereview.chromium.org/12741012/diff/14001/base/threading/platform_t...
File base/threading/platform_thread.h (left):

https://codereview.chromium.org/12741012/diff/14001/base/threading/platform_t...
base/threading/platform_thread.h:113: 
On 2013/04/24 01:04:57, jar wrote:
> nit: preserve the white-space line before the private keyword.  I couldn't
find
> explicit support in the style guide, but *all* the examples they provide had a
> blank line before it.... so you should probably at least keep it here.

Done.

https://codereview.chromium.org/12741012/diff/14001/base/threading/platform_t...
File base/threading/platform_thread.h (right):

https://codereview.chromium.org/12741012/diff/14001/base/threading/platform_t...
base/threading/platform_thread.h:47: kThreadPriority_Display,
On 2013/04/24 01:04:57, jar wrote:
> Although I can guess what Normal means, vs Background, I don't know how to fit
> in Display and RealtimeAudio.
> 
> Can you at a minimum add comments, and probably better would be to also
lengthen
> the name so that its significance would be clear at point of use (without
> reading the comments).

Done.

https://codereview.chromium.org/12741012/diff/14001/base/threading/platform_t...
base/threading/platform_thread.h:114: PlatformThreadId id,
On 2013/04/24 01:04:57, jar wrote:
> Rather than having both, can you unify this in a class some how?  What is the
> significance of listing both in call??

See also my previous general comment on the patch.

The closest I think would be trying to store the ID inside the Handle itself, at
thread creation time. Perhaps the ID could then be removed entirely from the
PlatformThread abstraction. The trouble is that this imposes limits on when
priorities can be set on certain platforms.

Specifically, I think you would need to have created the thread yourself in
order to set it's priority (on all platforms), even though this is only a real
limitation on windows (IIUC).

https://codereview.chromium.org/12741012/diff/14001/base/threading/platform_t...
File base/threading/platform_thread_posix.cc (right):

https://codereview.chromium.org/12741012/diff/14001/base/threading/platform_t...
base/threading/platform_thread_posix.cc:71: // We use -6 for display, but we may
want to split this
On 2013/04/24 01:04:57, jar wrote:
> This comment does not relate well to line 64 or 70.  You are not using -6 for
> anything now.

Thanks! This was a typo. It should be -6 below.

Done.

https://codereview.chromium.org/12741012/diff/14001/base/threading/platform_t...
base/threading/platform_thread_posix.cc:114: if (!error && prio !=
NiceValue(thread_params->priority)) {
On 2013/04/24 01:04:57, jar wrote:
> nit: Given the up-front-comment, I'd rather see an "else if" than see a repeat
> test for "if (!error && ...". 

This has moved into SetThreadPriority now, I think I address ed this there (in
the _platform.cc files).

https://codereview.chromium.org/12741012/diff/14001/base/threading/platform_t...
base/threading/platform_thread_posix.cc:343: LOG(ERROR) << "Failed to set nice
value of thread to " << kNiceSetting;
On 2013/04/24 01:04:57, jar wrote:
> DLOG is generally preferred.  No one will read this in production, so we'd
> rather have this compiled out.

Done.

[epenner, 2013-05-02 21:29:04]

Rebase. Ptal.

[jar (doing other things), 2013-05-03 23:54:07]

https://codereview.chromium.org/12741012/diff/55001/base/threading/platform_t...
File base/threading/platform_thread_posix.cc (right):

https://codereview.chromium.org/12741012/diff/55001/base/threading/platform_t...
base/threading/platform_thread_posix.cc:211: #if !defined(OS_MACOSX)
This section is still messy (lines 193-230) with all these ifdefs on platform
(even though you now have several platform specific files).

Effectively, you're defining some no-op implementations for some platforms. 
This smells a lot like the situation where a base class is defined, and derived
class (for each platform) optionally provide virtual replacements for some
methods.

Certainly performance in the calls to these thread initialization and
termination methods is not critical.

WDYT?

[epennerAtGoogle, 2013-05-04 01:09:45]

https://codereview.chromium.org/12741012/diff/55001/base/threading/platform_t...
File base/threading/platform_thread_posix.cc (right):

https://codereview.chromium.org/12741012/diff/55001/base/threading/platform_t...
base/threading/platform_thread_posix.cc:211: #if !defined(OS_MACOSX)
On 2013/05/03 23:54:07, jar wrote:
> This section is still messy (lines 193-230) with all these ifdefs on platform
> (even though you now have several platform specific files).
> 
> Effectively, you're defining some no-op implementations for some platforms. 
> This smells a lot like the situation where a base class is defined, and
derived
> class (for each platform) optionally provide virtual replacements for some
> methods.
> 
> Certainly performance in the calls to these thread initialization and
> termination methods is not critical.
> 
> WDYT?

Yes, I really did this for code clarity as opposed to complete removal of
#ifdefs. I do think it's a lot more readable this way.

As a first option, these defaults wouldn't need to be here if I knew all the
platforms we need to support. But in PlatformThread::CurrentId() there are
several platforms (Solaris?!) that I didn't add custom files for that I don't
want to break.

As a second option, the using virtual methods would also solve the #ifdefs, but
that's a pretty big change? I don't think it's worth doing that just for the
customized bits of the Posix implementation, but if the entire PlatformThread
interface was "thunked" into a virtual class that would be more clean probably.
In that case the new methods I have added would become new virtuals in the
default Posix implementation.

Would you like me to proceed with "thunking" the entire PlatformThread
interface? That will be a pure refactor so I'd want to do it in a different CL.
I can do it before or after this change.

[epennerAtGoogle, 2013-05-04 01:16:51]

If we do thunk the interface, I don't think there is a clean place to set the
singleton pointer at startup, so my plan would be to route all the static
methods to a singleton lazy-instance object.

[reveman, 2013-05-07 18:17:38]

On 2013/04/24 04:57:14, epennerAtGoogle wrote:
> Thanks for the feedback!  Most comments seems straight forward to address, but
> let me address the "redundant parameter" issue:
> 
> The problem is that PlatformThread is very low level. That is good in that you
> can use it to operate on 'raw' threads that were not even created using
Chrome's
> thread abstraction, but it's bad in that it is difficult to fully abstract
away
> platform differences. For the former, ThreadID and ThreadHandle need to map
> directly to native types on each platform. But then the problem is that some
> platforms require the ThreadID to set the priority, while some require the
> ThreadHandle.
> 
> My thinking was that you can just pass both when writing cross platform code,
> and the Thread class can abstract this further to hide this since it has both
> anyway. So if you want cleaner code but slightly reduced functionality,
> thread->SetPriority() is the way to go, whereas PlatformThread still lets you
do
> anything supported on any platform.
> 
> Another option mentioned making ThreadHandle into a struct on some platforms
and
> hiding the ID in there. But IMO that reduces the flexibility of the API in
some
> cases.

You could also keep a <pthread, tid_t> map in platform_thread_posix.cc. Maybe
not the cleanest solution but it at least makes it an implementation detail
rather than a public API problem.

[reveman, 2013-05-07 18:58:56]

Not clear to me why virtual methods would be needed here. All PlatformThread
functions are static.

Looks to me like the problem is that we're trying to have
platform_thread_posix.cc be both a full implementation of PlatformThread on some
platforms (i.e. solaris) and a subset on some (linux, android).

I would vote for always having it be a subset and move all functions in
platform_thread_posix.cc that depend on ifdefs out into platform specific files.

[epennerAtGoogle, 2013-05-07 20:34:06]

Yeah this is along the lines of my first option. I really prefer that as well.
I'll remove the #ifdefs and add the implementations to linux/mac. If some
obscure build breaks they can add the _linux.cc or create a new one.

[epennerAtGoogle, 2013-05-07 20:37:01]

On 2013/05/07 18:17:38, David Reveman wrote:
> On 2013/04/24 04:57:14, epennerAtGoogle wrote:
> > Thanks for the feedback!  Most comments seems straight forward to address,
but
> > let me address the "redundant parameter" issue:
> > 
> > The problem is that PlatformThread is very low level. That is good in that
you
> > can use it to operate on 'raw' threads that were not even created using
> Chrome's
> > thread abstraction, but it's bad in that it is difficult to fully abstract
> away
> > platform differences. For the former, ThreadID and ThreadHandle need to map
> > directly to native types on each platform. But then the problem is that some
> > platforms require the ThreadID to set the priority, while some require the
> > ThreadHandle.
> > 
> > My thinking was that you can just pass both when writing cross platform
code,
> > and the Thread class can abstract this further to hide this since it has
both
> > anyway. So if you want cleaner code but slightly reduced functionality,
> > thread->SetPriority() is the way to go, whereas PlatformThread still lets
you
> do
> > anything supported on any platform.
> > 
> > Another option mentioned making ThreadHandle into a struct on some platforms
> and
> > hiding the ID in there. But IMO that reduces the flexibility of the API in
> some
> > cases.
> 
> You could also keep a <pthread, tid_t> map in platform_thread_posix.cc. Maybe
> not the cleanest solution but it at least makes it an implementation detail
> rather than a public API problem.

The problem here is, what if you are changing a thread that wasn't created using
Chrome's APIs? You might not have the tid_t in your map.

[reveman, 2013-05-08 02:32:15]

On 2013/05/07 20:37:01, epennerAtGoogle wrote:
> On 2013/05/07 18:17:38, David Reveman wrote:
> > On 2013/04/24 04:57:14, epennerAtGoogle wrote:
> > > Thanks for the feedback!  Most comments seems straight forward to address,
> but
> > > let me address the "redundant parameter" issue:
> > > 
> > > The problem is that PlatformThread is very low level. That is good in that
> you
> > > can use it to operate on 'raw' threads that were not even created using
> > Chrome's
> > > thread abstraction, but it's bad in that it is difficult to fully abstract
> > away
> > > platform differences. For the former, ThreadID and ThreadHandle need to
map
> > > directly to native types on each platform. But then the problem is that
some
> > > platforms require the ThreadID to set the priority, while some require the
> > > ThreadHandle.
> > > 
> > > My thinking was that you can just pass both when writing cross platform
> code,
> > > and the Thread class can abstract this further to hide this since it has
> both
> > > anyway. So if you want cleaner code but slightly reduced functionality,
> > > thread->SetPriority() is the way to go, whereas PlatformThread still lets
> you
> > do
> > > anything supported on any platform.
> > > 
> > > Another option mentioned making ThreadHandle into a struct on some
platforms
> > and
> > > hiding the ID in there. But IMO that reduces the flexibility of the API in
> > some
> > > cases.
> > 
> > You could also keep a <pthread, tid_t> map in platform_thread_posix.cc.
Maybe
> > not the cleanest solution but it at least makes it an implementation detail
> > rather than a public API problem.
> 
> The problem here is, what if you are changing a thread that wasn't created
using
> Chrome's APIs? You might not have the tid_t in your map.

Do we need to support that? If someone creates a platform specific thread handle
on their own, they can probably also set the thread priority on their own.

[epenner, 2013-05-09 19:44:16]

> Do we need to support that? If someone creates a platform specific thread
handle on their own, they can probably also set the thread priority on their
own.

We use a lot of third party libs but maybe it doesn't matter. However, the
addition of the id to the map would also need to be synchronized at thread start
time (we can only query the id on the new thread on linux/android), so this
comes back to just stashing the id in the handle at that time.

I went ahead and made SetThreadPriority only take the handle. I did some more
research and it looks like this won't reduce flexibility, since we can still
implement 'CurrentHandle()' on all platforms.

PTAL

[jar (doing other things), 2013-05-15 02:18:36]

https://codereview.chromium.org/12741012/diff/78016/base/threading/platform_t...
File base/threading/platform_thread.h (right):

https://codereview.chromium.org/12741012/diff/78016/base/threading/platform_t...
base/threading/platform_thread.h:22: #include <ostream>
nit: probably alphabetize by placing before line 20.

https://codereview.chromium.org/12741012/diff/78016/base/threading/platform_t...
base/threading/platform_thread.h:37: struct PlatformThreadHandle {
nit: this should probably be a class.

You'll probably want a copy constructors and such.

https://codereview.chromium.org/12741012/diff/78016/base/threading/platform_t...
base/threading/platform_thread.h:43: PlatformThreadHandle(pthread_t handle)
nit: explicit.

https://codereview.chromium.org/12741012/diff/78016/base/threading/platform_t...
base/threading/platform_thread.h:58: bool operator!() {
Instead of operator overload, use method names, like IsNull()

https://codereview.chromium.org/12741012/diff/78016/base/threading/platform_t...
base/threading/platform_thread.h:62: bool operator==(const PlatformThreadHandle&
other) {
nit: again, just define a method name.

https://codereview.chromium.org/12741012/diff/78016/base/threading/platform_t...
base/threading/platform_thread.h:75: std::ostream& operator<< (std::ostream
&out,
This is pretty uncommon, as use of streams is not generally endorsed.  Why did
you add this? To help with CHECK_*() calls?

https://codereview.chromium.org/12741012/diff/78016/base/threading/platform_t...
File base/threading/platform_thread_posix.cc (right):

https://codereview.chromium.org/12741012/diff/78016/base/threading/platform_t...
base/threading/platform_thread_posix.cc:39: std::ostream& operator<<
(std::ostream &out,
Why do we need this?

https://codereview.chromium.org/12741012/diff/78016/base/threading/platform_t...
base/threading/platform_thread_posix.cc:53: ThreadParams()
nit: Constructors come before members.

https://codereview.chromium.org/12741012/diff/78016/base/threading/platform_t...
base/threading/platform_thread_posix.cc:54: : thread_id_set(false, false) {
nit: indent colon by 4.

Where are initializers for the other POD members?

[epennerAtGoogle, 2013-05-15 05:09:08]

I was trying to mimic the integer handle type before. But I went ahead and
wrapped the windows handle as well and removed all operator overloads.

Let me know if there is anything else. There may be some straggling call sites
that rely on the operators (I'm letting the bots find those).

https://codereview.chromium.org/12741012/diff/78016/base/threading/platform_t...
File base/threading/platform_thread.h (right):

https://codereview.chromium.org/12741012/diff/78016/base/threading/platform_t...
base/threading/platform_thread.h:22: #include <ostream>
On 2013/05/15 02:18:36, jar wrote:
> nit: probably alphabetize by placing before line 20.

Done. I actually removed this.

https://codereview.chromium.org/12741012/diff/78016/base/threading/platform_t...
base/threading/platform_thread.h:37: struct PlatformThreadHandle {
On 2013/05/15 02:18:36, jar wrote:
> nit: this should probably be a class.
> 
> You'll probably want a copy constructors and such.

I made it a class to protect members, but default copy seems okay?

https://codereview.chromium.org/12741012/diff/78016/base/threading/platform_t...
base/threading/platform_thread.h:43: PlatformThreadHandle(pthread_t handle)
On 2013/05/15 02:18:36, jar wrote:
> nit: explicit.

Done.

https://codereview.chromium.org/12741012/diff/78016/base/threading/platform_t...
base/threading/platform_thread.h:58: bool operator!() {
On 2013/05/15 02:18:36, jar wrote:
> Instead of operator overload, use method names, like IsNull()

I couldn't do this because the handle on windows was an integer type. So I have
now refactored windows also uses this container (and changed all call sites).

Done.

https://codereview.chromium.org/12741012/diff/78016/base/threading/platform_t...
base/threading/platform_thread.h:62: bool operator==(const PlatformThreadHandle&
other) {
On 2013/05/15 02:18:36, jar wrote:
> nit: again, just define a method name.

Done.

https://codereview.chromium.org/12741012/diff/78016/base/threading/platform_t...
base/threading/platform_thread.h:75: std::ostream& operator<< (std::ostream
&out,
On 2013/05/15 02:18:36, jar wrote:
> This is pretty uncommon, as use of streams is not generally endorsed.  Why did
> you add this? To help with CHECK_*() calls?

Yes, I'm trying to remove it by changing a few call-sites.

https://codereview.chromium.org/12741012/diff/78016/base/threading/platform_t...
File base/threading/platform_thread_posix.cc (right):

https://codereview.chromium.org/12741012/diff/78016/base/threading/platform_t...
base/threading/platform_thread_posix.cc:39: std::ostream& operator<<
(std::ostream &out,
On 2013/05/15 02:18:36, jar wrote:
> Why do we need this?

Looks like I can get rid of it completely.

https://codereview.chromium.org/12741012/diff/78016/base/threading/platform_t...
base/threading/platform_thread_posix.cc:53: ThreadParams()
On 2013/05/15 02:18:36, jar wrote:
> nit: Constructors come before members.

Done.

https://codereview.chromium.org/12741012/diff/78016/base/threading/platform_t...
base/threading/platform_thread_posix.cc:54: : thread_id_set(false, false) {
On 2013/05/15 02:18:36, jar wrote:
> nit: indent colon by 4.
> 
> Where are initializers for the other POD members?

Done.

[epenner, 2013-05-17 19:18:13]

Rebase. Ptal.

https://codereview.chromium.org/12741012/diff/78016/base/threading/platform_t...
File base/threading/platform_thread.h (right):

https://codereview.chromium.org/12741012/diff/78016/base/threading/platform_t...
base/threading/platform_thread.h:22: #include <ostream>
On 2013/05/15 02:18:36, jar wrote:
> nit: probably alphabetize by placing before line 20.

I went ahead and removed this.

https://codereview.chromium.org/12741012/diff/78016/base/threading/platform_t...
base/threading/platform_thread.h:37: struct PlatformThreadHandle {
On 2013/05/15 02:18:36, jar wrote:
> nit: this should probably be a class.
> 
> You'll probably want a copy constructors and such.

I made it into a class to protect private members, but default copy constructor
etc. is fine?

https://codereview.chromium.org/12741012/diff/78016/base/threading/platform_t...
base/threading/platform_thread.h:43: PlatformThreadHandle(pthread_t handle)
On 2013/05/15 02:18:36, jar wrote:
> nit: explicit.

Done.

[jar (doing other things), 2013-05-17 22:51:47]

Mostly style nits, but one request below to get brett or jam to approve (see the
last comment below).

https://codereview.chromium.org/12741012/diff/156001/base/threading/platform_...
File base/threading/platform_thread.h (right):

https://codereview.chromium.org/12741012/diff/156001/base/threading/platform_...
base/threading/platform_thread.h:41: : handle_(0),
nit: colon should be indented by 4 (same on lines 47 and 52

https://codereview.chromium.org/12741012/diff/156001/base/threading/platform_...
base/threading/platform_thread.h:56: bool equals(const PlatformThreadHandle&
other) {
nit: probably better is 
is_equal
or
IsEqual

https://codereview.chromium.org/12741012/diff/156001/base/threading/platform_...
base/threading/platform_thread.h:60: bool isNull() {
nit: bouncy caps, or hacker style... but this was mixed.  Use either

is_null
or
IsNull

https://codereview.chromium.org/12741012/diff/156001/base/threading/platform_...
base/threading/platform_thread.h:68: public:
typo: private

https://codereview.chromium.org/12741012/diff/156001/base/threading/platform_...
base/threading/platform_thread.h:77: const PlatformThreadId kInvalidThreadId =
0;
nit: probably should use the same initializer style in both of these lines.

https://codereview.chromium.org/12741012/diff/156001/base/threading/platform_...
File base/threading/platform_thread_posix.cc (right):

https://codereview.chromium.org/12741012/diff/156001/base/threading/platform_...
base/threading/platform_thread_posix.cc:43: : delegate(NULL),
nit: indent colon by 4 (2 more than currently), and then continue to align slots
as you did.

https://codereview.chromium.org/12741012/diff/156001/base/threading/thread_re...
File base/threading/thread_restrictions.h (right):

https://codereview.chromium.org/12741012/diff/156001/base/threading/thread_re...
base/threading/thread_restrictions.h:171: // DO NOT ADD ANY OTHER FRIEND
STATEMENTS, talk to jam or brettw first.
I think this is fine.... but talk to jam or brettw as per request.  

This additional on line 185 is really just allowing platform specific threads to
be friends, just as Thread currently is on line 183.

[epenner, 2013-05-17 23:46:35]

Fixed Nits. I'll add other owners and Brett/Jam for the restrictions issue.

https://codereview.chromium.org/12741012/diff/156001/base/threading/platform_...
File base/threading/platform_thread.h (right):

https://codereview.chromium.org/12741012/diff/156001/base/threading/platform_...
base/threading/platform_thread.h:41: : handle_(0),
On 2013/05/17 22:51:48, jar wrote:
> nit: colon should be indented by 4 (same on lines 47 and 52

Done.

https://codereview.chromium.org/12741012/diff/156001/base/threading/platform_...
base/threading/platform_thread.h:56: bool equals(const PlatformThreadHandle&
other) {
On 2013/05/17 22:51:48, jar wrote:
> nit: probably better is 
> is_equal
> or
> IsEqual

Done.

https://codereview.chromium.org/12741012/diff/156001/base/threading/platform_...
base/threading/platform_thread.h:60: bool isNull() {
On 2013/05/17 22:51:48, jar wrote:
> nit: bouncy caps, or hacker style... but this was mixed.  Use either
> 
> is_null
> or
> IsNull

Done.

https://codereview.chromium.org/12741012/diff/156001/base/threading/platform_...
base/threading/platform_thread.h:68: public:
On 2013/05/17 22:51:48, jar wrote:
> typo: private

Thanks! I must have switched it back temporarily and forgot.

Done.

https://codereview.chromium.org/12741012/diff/156001/base/threading/platform_...
base/threading/platform_thread.h:77: const PlatformThreadId kInvalidThreadId =
0;
On 2013/05/17 22:51:48, jar wrote:
> nit: probably should use the same initializer style in both of these lines.

Done.

https://codereview.chromium.org/12741012/diff/156001/base/threading/platform_...
File base/threading/platform_thread_posix.cc (right):

https://codereview.chromium.org/12741012/diff/156001/base/threading/platform_...
base/threading/platform_thread_posix.cc:43: : delegate(NULL),
On 2013/05/17 22:51:48, jar wrote:
> nit: indent colon by 4 (2 more than currently), and then continue to align
slots
> as you did.

Done.

https://codereview.chromium.org/12741012/diff/156001/base/threading/thread_re...
File base/threading/thread_restrictions.h (right):

https://codereview.chromium.org/12741012/diff/156001/base/threading/thread_re...
base/threading/thread_restrictions.h:171: // DO NOT ADD ANY OTHER FRIEND
STATEMENTS, talk to jam or brettw first.
On 2013/05/17 22:51:48, jar wrote:
> I think this is fine.... but talk to jam or brettw as per request.  
> 
> This additional on line 185 is really just allowing platform specific threads
to
> be friends, just as Thread currently is on line 183.

Okay I'll add them together with OWNERS.

[epenner, 2013-05-18 00:04:48]

+brettw / jar

Can you have a look at thread_restrictions and let me know if there is any
issues? It's exactly the same as what Thread does, so it shouldn't be an issue.

I also need an OWNERS lgtm for some of the minor changes to chrome/content.

[Sergey Ulanov, 2013-05-18 00:05:56]

remoting - lgtm

https://codereview.chromium.org/12741012/diff/181001/remoting/base/auto_threa...
File remoting/base/auto_thread.cc (right):

https://codereview.chromium.org/12741012/diff/181001/remoting/base/auto_threa...
remoting/base/auto_thread.cc:95: thread_(0),
nit: I think this can be removed now.

https://codereview.chromium.org/12741012/diff/181001/remoting/base/auto_threa...
remoting/base/auto_thread.cc:105: thread_(0),
and this.

[Ami GONE FROM CHROMIUM, 2013-05-18 00:14:37]

LGTM for media/

[jar (doing other things), 2013-05-18 00:18:53]

lgtm

[ananta, 2013-05-18 01:25:07]

lgtm

ChromeFrame LGTM

[tommi (sloooow) - chröme, 2013-05-18 04:35:37]

lgtm for chrome_frame.

[commit-bot: I haz the power, 2013-05-20 21:42:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/epenner@chromium.org/12741012/199001

[commit-bot: I haz the power, 2013-05-20 21:52:50]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[epenner, 2013-05-20 22:18:05]

+abodenha for printing OWNERS lgtm.

[Albert Bodenhamer, 2013-05-20 22:36:57]

LGTM for printing.

[commit-bot: I haz the power, 2013-05-20 22:38:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/epenner@chromium.org/12741012/199001

[commit-bot: I haz the power, 2013-05-21 03:15:51]

Change committed as 201202

[epenner, 2013-05-21 07:24:02]

On 2013/05/21 03:15:51, I haz the power (commit-bot) wrote:
> Change committed as 201202

Second time lucky. I removed kNullThreadHandle, as it's static initializer as
causing trouble. No need for it now anyway since the constructor inits to a null
handle.

[commit-bot: I haz the power, 2013-05-21 07:25:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/epenner@chromium.org/12741012/220001

[commit-bot: I haz the power, 2013-05-21 07:33:17]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[epenner, 2013-05-21 19:05:14]

On 2013/05/21 07:33:17, I haz the power (commit-bot) wrote:
> Retried try job too often on chromium_presubmit for step(s) presubmit
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

+bauerb@chromium.org for OWNERS review of content settings.

[Bernhard Bauer, 2013-05-21 19:45:16]

content_settings LGTM.

In general, feel free to TBR OWNERS for just updating method signatures when the
actual signature change has been reviewed.

[commit-bot: I haz the power, 2013-05-21 19:47:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/epenner@chromium.org/12741012/220001

[commit-bot: I haz the power, 2013-05-22 00:01:46]

Change committed as 201389