Implement SequencedTaskChecker.

Implement SequencedTaskChecker.

SequencedTaskChecker is a helper class used to help verify that some methods
of a class are called on the same task queue or thread. A
SequencedTaskChecker is bound to a a task queue if the object is
created on a task queue, or a thread otherwise.

BUG=webrtc:5687
Committed: https://crrev.com/9c16fe84b99d5c2aca6109aab6bcb1d11b8f88b5
Cr-Commit-Position: refs/heads/master@{#13452}

[perkj_webrtc, 2016-07-07 12:13:10]

perkj@webrtc.org changed reviewers:
+ tommi@webrtc.org

[perkj_webrtc, 2016-07-07 12:13:10]

please?

[tommi, 2016-07-08 13:15:16]

https://codereview.webrtc.org/2125113003/diff/1/webrtc/base/sequenced_task_ch...
File webrtc/base/sequenced_task_checker.h (right):

https://codereview.webrtc.org/2125113003/diff/1/webrtc/base/sequenced_task_ch...
webrtc/base/sequenced_task_checker.h:41: bool IsCurrent() const { return true; }
CalledSequentially?  I'm thinking of something a little closer to the
ThreadChecker's CalledOnValidThread.

https://codereview.webrtc.org/2125113003/diff/1/webrtc/base/sequenced_task_ch...
File webrtc/base/sequenced_task_checker_impl.cc (right):

https://codereview.webrtc.org/2125113003/diff/1/webrtc/base/sequenced_task_ch...
webrtc/base/sequenced_task_checker_impl.cc:23: TaskQueue* current_queu =
TaskQueue::Current();
queue

https://codereview.webrtc.org/2125113003/diff/1/webrtc/base/sequenced_task_ch...
webrtc/base/sequenced_task_checker_impl.cc:26: valid_queu_ =
TaskQueue::Current();
valid_queue_ = current_queue;

(since you've already called TaskQueue::Current())

https://codereview.webrtc.org/2125113003/diff/1/webrtc/base/sequenced_task_ch...
File webrtc/base/sequenced_task_checker_impl.h (right):

https://codereview.webrtc.org/2125113003/diff/1/webrtc/base/sequenced_task_ch...
webrtc/base/sequenced_task_checker_impl.h:37: CriticalSection lock_;
would like to avoid a lock if we at all can.

https://codereview.webrtc.org/2125113003/diff/1/webrtc/base/sequenced_task_ch...
webrtc/base/sequenced_task_checker_impl.h:40: mutable TaskQueue* valid_queu_;
queue

https://codereview.webrtc.org/2125113003/diff/1/webrtc/base/sequenced_task_ch...
File webrtc/base/sequenced_task_checker_unittest.cc (right):

https://codereview.webrtc.org/2125113003/diff/1/webrtc/base/sequenced_task_ch...
webrtc/base/sequenced_task_checker_unittest.cc:25: #define
ENABLE_SEQUENCED_TASK_CHECKER 1
this doesn't look like the right place to change this.  Instead, we should be
able to test SequencedTaskCheckerImpl directly when it's available (and not rely
on the SequencedTaskChecker typedef).

https://codereview.webrtc.org/2125113003/diff/1/webrtc/base/sequenced_task_ch...
webrtc/base/sequenced_task_checker_unittest.cc:40: void DoStuff() {
RTC_DCHECK(IsCurrent()); }
since this is a test, it's probably better to do EXPECT_TRUE(IsCurrent());.
If the DCHECK fails, it could crash the process and prevent following tests from
executing.

https://codereview.webrtc.org/2125113003/diff/1/webrtc/base/sequenced_task_ch...
webrtc/base/sequenced_task_checker_unittest.cc:42: void DetachFromSequence() {
SequencedTaskChecker::Detach(); }
Is this method needed or can the caller just call Detach() instead?

https://codereview.webrtc.org/2125113003/diff/1/webrtc/base/sequenced_task_ch...
webrtc/base/sequenced_task_checker_unittest.cc:54: class CallDoStuffOnThread :
public Thread {
instead of using rtc::Thread (which we're using less and less), can you use
PlatformThread?

https://codereview.webrtc.org/2125113003/diff/1/webrtc/base/sequenced_task_ch...
webrtc/base/sequenced_task_checker_unittest.cc:65: void Join() { Thread::Join();
}
I think you can also do:

using Thread::Join;

https://codereview.webrtc.org/2125113003/diff/1/webrtc/base/sequenced_task_ch...
webrtc/base/sequenced_task_checker_unittest.cc:98: new SequencedCheckerClass);
new SequencedCheckerClass()

https://codereview.webrtc.org/2125113003/diff/1/webrtc/base/sequenced_task_ch...
webrtc/base/sequenced_task_checker_unittest.cc:170: #if GTEST_HAS_DEATH_TEST ||
!ENABLE_SEQUENCED_TASK_CHECKER
I'm having problems groking this check... can you add a comment that explains
the behavior in either case?

https://codereview.webrtc.org/2125113003/diff/1/webrtc/base/sequenced_task_ch...
webrtc/base/sequenced_task_checker_unittest.cc:267: void
access_var_no_annotate() {
AccessVarNoAnnotate (since this isn't a getter)

https://codereview.webrtc.org/2125113003/diff/1/webrtc/base/sequenced_task_ch...
webrtc/base/sequenced_task_checker_unittest.cc:272: void
access_fun_no_annotate() {
AccessFunctionNoAnnotate etc

https://codereview.webrtc.org/2125113003/diff/1/webrtc/base/sequenced_task_ch...
webrtc/base/sequenced_task_checker_unittest.cc:301: #undef
ENABLE_SEQUENCED_TASK_CHECKER
this could only happen if another file #includes
sequenced_task_checker_unittest.cc - and we should never do that, right?

[perkj_webrtc, 2016-07-11 08:38:01]

ptal

https://codereview.webrtc.org/2125113003/diff/1/webrtc/base/sequenced_task_ch...
File webrtc/base/sequenced_task_checker.h (right):

https://codereview.webrtc.org/2125113003/diff/1/webrtc/base/sequenced_task_ch...
webrtc/base/sequenced_task_checker.h:41: bool IsCurrent() const { return true; }
On 2016/07/08 13:15:15, tommi-webrtc wrote:
> CalledSequentially?  I'm thinking of something a little closer to the
> ThreadChecker's CalledOnValidThread.

Done.

https://codereview.webrtc.org/2125113003/diff/1/webrtc/base/sequenced_task_ch...
File webrtc/base/sequenced_task_checker_impl.cc (right):

https://codereview.webrtc.org/2125113003/diff/1/webrtc/base/sequenced_task_ch...
webrtc/base/sequenced_task_checker_impl.cc:23: TaskQueue* current_queu =
TaskQueue::Current();
On 2016/07/08 13:15:15, tommi-webrtc wrote:
> queue

Done.

https://codereview.webrtc.org/2125113003/diff/1/webrtc/base/sequenced_task_ch...
webrtc/base/sequenced_task_checker_impl.cc:26: valid_queu_ =
TaskQueue::Current();
On 2016/07/08 13:15:15, tommi-webrtc wrote:
> valid_queue_ = current_queue;
> 
> (since you've already called TaskQueue::Current())

Done.

https://codereview.webrtc.org/2125113003/diff/1/webrtc/base/sequenced_task_ch...
File webrtc/base/sequenced_task_checker_impl.h (right):

https://codereview.webrtc.org/2125113003/diff/1/webrtc/base/sequenced_task_ch...
webrtc/base/sequenced_task_checker_impl.h:37: CriticalSection lock_;
On 2016/07/08 13:15:15, tommi-webrtc wrote:
> would like to avoid a lock if we at all can.

Acknowledged.

https://codereview.webrtc.org/2125113003/diff/1/webrtc/base/sequenced_task_ch...
webrtc/base/sequenced_task_checker_impl.h:40: mutable TaskQueue* valid_queu_;
On 2016/07/08 13:15:15, tommi-webrtc wrote:
> queue

Done.

https://codereview.webrtc.org/2125113003/diff/1/webrtc/base/sequenced_task_ch...
File webrtc/base/sequenced_task_checker_unittest.cc (right):

https://codereview.webrtc.org/2125113003/diff/1/webrtc/base/sequenced_task_ch...
webrtc/base/sequenced_task_checker_unittest.cc:25: #define
ENABLE_SEQUENCED_TASK_CHECKER 1
On 2016/07/08 13:15:15, tommi-webrtc wrote:
> this doesn't look like the right place to change this.  Instead, we should be
> able to test SequencedTaskCheckerImpl directly when it's available (and not
rely
> on the SequencedTaskChecker typedef).

Ok- These tests were a copy of the threadcheker unittests. Now I have rewritten
them quite a lot. Ptal.

https://codereview.webrtc.org/2125113003/diff/1/webrtc/base/sequenced_task_ch...
webrtc/base/sequenced_task_checker_unittest.cc:40: void DoStuff() {
RTC_DCHECK(IsCurrent()); }
On 2016/07/08 13:15:15, tommi-webrtc wrote:
> since this is a test, it's probably better to do EXPECT_TRUE(IsCurrent());.
> If the DCHECK fails, it could crash the process and prevent following tests
from
> executing.

Done. - No longer using death tests.

https://codereview.webrtc.org/2125113003/diff/1/webrtc/base/sequenced_task_ch...
webrtc/base/sequenced_task_checker_unittest.cc:42: void DetachFromSequence() {
SequencedTaskChecker::Detach(); }
On 2016/07/08 13:15:16, tommi-webrtc wrote:
> Is this method needed or can the caller just call Detach() instead?

Done.

https://codereview.webrtc.org/2125113003/diff/1/webrtc/base/sequenced_task_ch...
webrtc/base/sequenced_task_checker_unittest.cc:54: class CallDoStuffOnThread :
public Thread {
On 2016/07/08 13:15:15, tommi-webrtc wrote:
> instead of using rtc::Thread (which we're using less and less), can you use
> PlatformThread?

Done.

https://codereview.webrtc.org/2125113003/diff/1/webrtc/base/sequenced_task_ch...
webrtc/base/sequenced_task_checker_unittest.cc:65: void Join() { Thread::Join();
}
On 2016/07/08 13:15:15, tommi-webrtc wrote:
> I think you can also do:
> 
> using Thread::Join;

Acknowledged.

https://codereview.webrtc.org/2125113003/diff/1/webrtc/base/sequenced_task_ch...
webrtc/base/sequenced_task_checker_unittest.cc:98: new SequencedCheckerClass);
On 2016/07/08 13:15:15, tommi-webrtc wrote:
> new SequencedCheckerClass()

Done.

https://codereview.webrtc.org/2125113003/diff/1/webrtc/base/sequenced_task_ch...
webrtc/base/sequenced_task_checker_unittest.cc:170: #if GTEST_HAS_DEATH_TEST ||
!ENABLE_SEQUENCED_TASK_CHECKER
On 2016/07/08 13:15:15, tommi-webrtc wrote:
> I'm having problems groking this check... can you add a comment that explains
> the behavior in either case?

Refactored to no longer use death tests.

https://codereview.webrtc.org/2125113003/diff/1/webrtc/base/sequenced_task_ch...
webrtc/base/sequenced_task_checker_unittest.cc:267: void
access_var_no_annotate() {
On 2016/07/08 13:15:15, tommi-webrtc wrote:
> AccessVarNoAnnotate (since this isn't a getter)

removed all for now.

https://codereview.webrtc.org/2125113003/diff/1/webrtc/base/sequenced_task_ch...
webrtc/base/sequenced_task_checker_unittest.cc:272: void
access_fun_no_annotate() {
On 2016/07/08 13:15:15, tommi-webrtc wrote:
> AccessFunctionNoAnnotate etc

dito

https://codereview.webrtc.org/2125113003/diff/1/webrtc/base/sequenced_task_ch...
webrtc/base/sequenced_task_checker_unittest.cc:301: #undef
ENABLE_SEQUENCED_TASK_CHECKER
On 2016/07/08 13:15:15, tommi-webrtc wrote:
> this could only happen if another file #includes
> sequenced_task_checker_unittest.cc - and we should never do that, right?

Acknowledged.

[tommi, 2016-07-12 13:08:57]

the below comments are for a slightly older patch set than the latest one, but I
think the code hasn't changed that much

https://codereview.webrtc.org/2125113003/diff/20001/webrtc/base/sequenced_tas...
File webrtc/base/sequenced_task_checker_unittest.cc (right):

https://codereview.webrtc.org/2125113003/diff/20001/webrtc/base/sequenced_tas...
webrtc/base/sequenced_task_checker_unittest.cc:26: class CallDoStuffOnThread :
public PlatformThread {
instead of inheriting from PlatformThread, add a member variable of type
PlatformThread.  As is, CallDoStuffOnThread has Start() and Stop() methods etc.

https://codereview.webrtc.org/2125113003/diff/20001/webrtc/base/sequenced_tas...
webrtc/base/sequenced_task_checker_unittest.cc:28: explicit
CallDoStuffOnThread(bool expect_true,
no need for explicit (2 args)

https://codereview.webrtc.org/2125113003/diff/20001/webrtc/base/sequenced_tas...
webrtc/base/sequenced_task_checker_unittest.cc:30: : PlatformThread(Run, this,
"call_do_stuff_on_thread"),
&Run

https://codereview.webrtc.org/2125113003/diff/20001/webrtc/base/sequenced_tas...
webrtc/base/sequenced_task_checker_unittest.cc:34: static bool Run(void* obj) {
make private

https://codereview.webrtc.org/2125113003/diff/20001/webrtc/base/sequenced_tas...
webrtc/base/sequenced_task_checker_unittest.cc:37: if
(call_stuff_on_thread->expect_true_)
{}

https://codereview.webrtc.org/2125113003/diff/20001/webrtc/base/sequenced_tas...
webrtc/base/sequenced_task_checker_unittest.cc:38: EXPECT_TRUE(
could also do:

EXPECT_EQ(call_stuff_on_thread->expect_true_,
    call_stuff_on_thread->sequenced_task_checker_->CalledSequentially());

https://codereview.webrtc.org/2125113003/diff/20001/webrtc/base/sequenced_tas...
webrtc/base/sequenced_task_checker_unittest.cc:54: class
DeleteThreadCheckerOnThread : public PlatformThread {
Favor object composition over class inheritance.
(https://en.wikipedia.org/wiki/Composition_over_inheritance)

https://codereview.webrtc.org/2125113003/diff/20001/webrtc/base/sequenced_tas...
webrtc/base/sequenced_task_checker_unittest.cc:58: : PlatformThread(Run, this,
"delete_sequenced_task_checker_on_thread"),
&Run

https://codereview.webrtc.org/2125113003/diff/20001/webrtc/base/sequenced_tas...
webrtc/base/sequenced_task_checker_unittest.cc:61: static bool Run(void* obj) {
private (etc)

https://codereview.webrtc.org/2125113003/diff/20001/webrtc/base/sequenced_tas...
webrtc/base/sequenced_task_checker_unittest.cc:79: call_on_thread.Start();
Start/Stop could occur implicitly in the ctor/dtor.

[pbos-webrtc, 2016-07-12 13:24:29]

On 2016/07/12 13:08:57, tommi-webrtc wrote:
> the below comments are for a slightly older patch set than the latest one, but
I
> think the code hasn't changed that much
> 
>
https://codereview.webrtc.org/2125113003/diff/20001/webrtc/base/sequenced_tas...
> File webrtc/base/sequenced_task_checker_unittest.cc (right):
> 
>
https://codereview.webrtc.org/2125113003/diff/20001/webrtc/base/sequenced_tas...
> webrtc/base/sequenced_task_checker_unittest.cc:26: class CallDoStuffOnThread :
> public PlatformThread {
> instead of inheriting from PlatformThread, add a member variable of type
> PlatformThread.  As is, CallDoStuffOnThread has Start() and Stop() methods
etc.
> 
>
https://codereview.webrtc.org/2125113003/diff/20001/webrtc/base/sequenced_tas...
> webrtc/base/sequenced_task_checker_unittest.cc:28: explicit
> CallDoStuffOnThread(bool expect_true,
> no need for explicit (2 args)
> 
>
https://codereview.webrtc.org/2125113003/diff/20001/webrtc/base/sequenced_tas...
> webrtc/base/sequenced_task_checker_unittest.cc:30: : PlatformThread(Run, this,
> "call_do_stuff_on_thread"),
> &Run
> 
>
https://codereview.webrtc.org/2125113003/diff/20001/webrtc/base/sequenced_tas...
> webrtc/base/sequenced_task_checker_unittest.cc:34: static bool Run(void* obj)
{
> make private
> 
>
https://codereview.webrtc.org/2125113003/diff/20001/webrtc/base/sequenced_tas...
> webrtc/base/sequenced_task_checker_unittest.cc:37: if
> (call_stuff_on_thread->expect_true_)
> {}
> 
>
https://codereview.webrtc.org/2125113003/diff/20001/webrtc/base/sequenced_tas...
> webrtc/base/sequenced_task_checker_unittest.cc:38: EXPECT_TRUE(
> could also do:
> 
> EXPECT_EQ(call_stuff_on_thread->expect_true_,
>     call_stuff_on_thread->sequenced_task_checker_->CalledSequentially());
> 
>
https://codereview.webrtc.org/2125113003/diff/20001/webrtc/base/sequenced_tas...
> webrtc/base/sequenced_task_checker_unittest.cc:54: class
> DeleteThreadCheckerOnThread : public PlatformThread {
> Favor object composition over class inheritance.
> (https://en.wikipedia.org/wiki/Composition_over_inheritance)
> 
>
https://codereview.webrtc.org/2125113003/diff/20001/webrtc/base/sequenced_tas...
> webrtc/base/sequenced_task_checker_unittest.cc:58: : PlatformThread(Run, this,
> "delete_sequenced_task_checker_on_thread"),
> &Run
> 
>
https://codereview.webrtc.org/2125113003/diff/20001/webrtc/base/sequenced_tas...
> webrtc/base/sequenced_task_checker_unittest.cc:61: static bool Run(void* obj)
{
> private (etc)
> 
>
https://codereview.webrtc.org/2125113003/diff/20001/webrtc/base/sequenced_tas...
> webrtc/base/sequenced_task_checker_unittest.cc:79: call_on_thread.Start();
> Start/Stop could occur implicitly in the ctor/dtor.

I think this should support GUARDED_BY/EXCLUSIVE_LOCKS_REQUIRED annotations to
compile-time enforce that variables protected by it are only accessed when
checking/dchecking the sequencedtaskchecker. I did something similar here:
https://chromium.googlesource.com/external/webrtc/+/master/webrtc/base/race_c...
and it enforces that either of the RTC_(D)CHECK macros in that file is used when
accessing something.

[perkj_webrtc, 2016-07-12 15:21:47]

Patchset #5 (id:80001) has been deleted

[perkj_webrtc, 2016-07-12 15:22:00]

ptal

https://codereview.webrtc.org/2125113003/diff/20001/webrtc/base/sequenced_tas...
File webrtc/base/sequenced_task_checker_unittest.cc (right):

https://codereview.webrtc.org/2125113003/diff/20001/webrtc/base/sequenced_tas...
webrtc/base/sequenced_task_checker_unittest.cc:26: class CallDoStuffOnThread :
public PlatformThread {
On 2016/07/12 13:08:56, tommi-webrtc wrote:
> instead of inheriting from PlatformThread, add a member variable of type
> PlatformThread.  As is, CallDoStuffOnThread has Start() and Stop() methods
etc.

Done.

https://codereview.webrtc.org/2125113003/diff/20001/webrtc/base/sequenced_tas...
webrtc/base/sequenced_task_checker_unittest.cc:28: explicit
CallDoStuffOnThread(bool expect_true,
On 2016/07/12 13:08:56, tommi-webrtc wrote:
> no need for explicit (2 args)

Done.

https://codereview.webrtc.org/2125113003/diff/20001/webrtc/base/sequenced_tas...
webrtc/base/sequenced_task_checker_unittest.cc:30: : PlatformThread(Run, this,
"call_do_stuff_on_thread"),
On 2016/07/12 13:08:56, tommi-webrtc wrote:
> &Run

Done.

https://codereview.webrtc.org/2125113003/diff/20001/webrtc/base/sequenced_tas...
webrtc/base/sequenced_task_checker_unittest.cc:34: static bool Run(void* obj) {
On 2016/07/12 13:08:56, tommi-webrtc wrote:
> make private

Done.

https://codereview.webrtc.org/2125113003/diff/20001/webrtc/base/sequenced_tas...
webrtc/base/sequenced_task_checker_unittest.cc:37: if
(call_stuff_on_thread->expect_true_)
On 2016/07/12 13:08:56, tommi-webrtc wrote:
> {}

Done.

https://codereview.webrtc.org/2125113003/diff/20001/webrtc/base/sequenced_tas...
webrtc/base/sequenced_task_checker_unittest.cc:38: EXPECT_TRUE(
On 2016/07/12 13:08:56, tommi-webrtc wrote:
> could also do:
> 
> EXPECT_EQ(call_stuff_on_thread->expect_true_,
>     call_stuff_on_thread->sequenced_task_checker_->CalledSequentially());

Done.

https://codereview.webrtc.org/2125113003/diff/20001/webrtc/base/sequenced_tas...
webrtc/base/sequenced_task_checker_unittest.cc:54: class
DeleteThreadCheckerOnThread : public PlatformThread {
On 2016/07/12 13:08:56, tommi-webrtc wrote:
> Favor object composition over class inheritance.
> (https://en.wikipedia.org/wiki/Composition_over_inheritance)

Done.

https://codereview.webrtc.org/2125113003/diff/20001/webrtc/base/sequenced_tas...
webrtc/base/sequenced_task_checker_unittest.cc:58: : PlatformThread(Run, this,
"delete_sequenced_task_checker_on_thread"),
On 2016/07/12 13:08:56, tommi-webrtc wrote:
> &Run

Done.

https://codereview.webrtc.org/2125113003/diff/20001/webrtc/base/sequenced_tas...
webrtc/base/sequenced_task_checker_unittest.cc:61: static bool Run(void* obj) {
On 2016/07/12 13:08:56, tommi-webrtc wrote:
> private (etc)

Done.

https://codereview.webrtc.org/2125113003/diff/20001/webrtc/base/sequenced_tas...
webrtc/base/sequenced_task_checker_unittest.cc:79: call_on_thread.Start();
On 2016/07/12 13:08:56, tommi-webrtc wrote:
> Start/Stop could occur implicitly in the ctor/dtor.

Done.

[tommi, 2016-07-12 17:19:23]

https://codereview.webrtc.org/2125113003/diff/100001/webrtc/base/sequenced_ta...
File webrtc/base/sequenced_task_checker_unittest.cc (right):

https://codereview.webrtc.org/2125113003/diff/100001/webrtc/base/sequenced_ta...
webrtc/base/sequenced_task_checker_unittest.cc:29: thread_.Stop();
call Stop() in the dtor? (same below)
How do you guarantee that the call is made?  It looks like if an implementation
detail of PlatformThread were to change, then this would break.  E.g. if you
call Stop() before the thread routine has been called, the thread could be
terminated.

https://codereview.webrtc.org/2125113003/diff/100001/webrtc/base/sequenced_ta...
webrtc/base/sequenced_task_checker_unittest.cc:38:
call_stuff_on_thread->expect_true_);
nit: expectation first

https://codereview.webrtc.org/2125113003/diff/100001/webrtc/base/sequenced_ta...
webrtc/base/sequenced_task_checker_unittest.cc:235: #if GTEST_HAS_DEATH_TEST
check the source code for where we use this. There are some configurations that
are troublesome.

[perkj_webrtc, 2016-07-12 20:50:57]

ptal

https://codereview.webrtc.org/2125113003/diff/100001/webrtc/base/sequenced_ta...
File webrtc/base/sequenced_task_checker_unittest.cc (right):

https://codereview.webrtc.org/2125113003/diff/100001/webrtc/base/sequenced_ta...
webrtc/base/sequenced_task_checker_unittest.cc:29: thread_.Stop();
On 2016/07/12 17:19:23, tommi-webrtc wrote:
> call Stop() in the dtor? (same below)
> How do you guarantee that the call is made?  It looks like if an
implementation
> detail of PlatformThread were to change, then this would break.  E.g. if you
> call Stop() before the thread routine has been called, the thread could be
> terminated.

Done but why would calling stop in the dtor be any better? 
I did read the comment in the link below  before I did the previouse patchet-
but I can create an event and wait for it as as well to be sure if it ever
changes.
https://cs.chromium.org/chromium/src/third_party/webrtc/base/platform_thread....

https://codereview.webrtc.org/2125113003/diff/100001/webrtc/base/sequenced_ta...
webrtc/base/sequenced_task_checker_unittest.cc:38:
call_stuff_on_thread->expect_true_);
On 2016/07/12 17:19:23, tommi-webrtc wrote:
> nit: expectation first

Done.

https://codereview.webrtc.org/2125113003/diff/100001/webrtc/base/sequenced_ta...
webrtc/base/sequenced_task_checker_unittest.cc:235: #if GTEST_HAS_DEATH_TEST
On 2016/07/12 17:19:22, tommi-webrtc wrote:
> check the source code for where we use this. There are some configurations
that
> are troublesome.

do not run on android it looks like.

[tommi, 2016-07-12 21:11:06]

lgtm.

See below for Start/Run/Stop.

https://codereview.webrtc.org/2125113003/diff/100001/webrtc/base/sequenced_ta...
File webrtc/base/sequenced_task_checker_unittest.cc (right):

https://codereview.webrtc.org/2125113003/diff/100001/webrtc/base/sequenced_ta...
webrtc/base/sequenced_task_checker_unittest.cc:29: thread_.Stop();
On 2016/07/12 20:50:57, perkj_webrtc wrote:
> On 2016/07/12 17:19:23, tommi-webrtc wrote:
> > call Stop() in the dtor? (same below)
> > How do you guarantee that the call is made?  It looks like if an
> implementation
> > detail of PlatformThread were to change, then this would break.  E.g. if you
> > call Stop() before the thread routine has been called, the thread could be
> > terminated.
> 
> Done but why would calling stop in the dtor be any better? 
> I did read the comment in the link below  before I did the previouse patchet-
> but I can create an event and wait for it as as well to be sure if it ever
> changes.
>
https://cs.chromium.org/chromium/src/third_party/webrtc/base/platform_thread....

Ah, that's great.  If that's the contract, then it sounds fair to rely on it.
I was actually thinking of the other approach to
ctor: Start()
Run: signal event
dtor: Wait for event to be signaled for some period of time, then Stop(), fail
if timeout.

[perkj_webrtc, 2016-07-12 21:20:07]

The CQ bit was checked by perkj@webrtc.org

[commit-bot: I haz the power, 2016-07-12 21:20:18]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-07-12 22:04:14]

Committed patchset #6 (id:120001)

[commit-bot: I haz the power, 2016-07-12 22:04:16]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-12 22:04:20]

Description was changed from

==========
Implement SequencedTaskChecker.

SequencedTaskChecker is a helper class used to help verify that some methods
of a class are called on the same task queue or thread. A
SequencedTaskChecker is bound to a a task queue if the object is
created on a task queue, or a thread otherwise.

BUG=webrtc:5687
==========

to

==========
Implement SequencedTaskChecker.

SequencedTaskChecker is a helper class used to help verify that some methods
of a class are called on the same task queue or thread. A
SequencedTaskChecker is bound to a a task queue if the object is
created on a task queue, or a thread otherwise.

BUG=webrtc:5687

Committed: https://crrev.com/9c16fe84b99d5c2aca6109aab6bcb1d11b8f88b5
Cr-Commit-Position: refs/heads/master@{#13452}
==========

[commit-bot: I haz the power, 2016-07-12 22:04:21]

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