Delayed event support in JingleThread.

Delayed event support in JingleThread.

BUG=54347
TEST=unittests

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=58773

[awong, 2010-09-03 18:20:41]

http://codereview.chromium.org/3333014/diff/5001/6001
File remoting/jingle_glue/jingle_thread.cc (right):

http://codereview.chromium.org/3333014/diff/5001/6001#newcode29
remoting/jingle_glue/jingle_thread.cc:29: delayed_work_time_ = time;
What's the threading assumptions on who calls this function?

Should post ScheduleNextDelayedTask onto the thread_'s loop and pass the time
argument as a closure with the task?

http://codereview.chromium.org/3333014/diff/5001/6002
File remoting/jingle_glue/jingle_thread_unittest.cc (right):

http://codereview.chromium.org/3333014/diff/5001/6002#newcode20
remoting/jingle_glue/jingle_thread_unittest.cc:20: int kDelayMs = 100;
Add comment explaining what this is used for any it's a sane value.

http://codereview.chromium.org/3333014/diff/5001/6002#newcode41
remoting/jingle_glue/jingle_thread_unittest.cc:41: EXPECT_CALL(*task, Run())
can this fit on one line?

http://codereview.chromium.org/3333014/diff/5001/6002#newcode51
remoting/jingle_glue/jingle_thread_unittest.cc:51: EXPECT_TRUE((end -
start).InMillisecondsRoundedUp() > kDelayMs);
EXPECT_GT?

[Sergey Ulanov, 2010-09-03 18:42:01]

http://codereview.chromium.org/3333014/diff/5001/6001
File remoting/jingle_glue/jingle_thread.cc (right):

http://codereview.chromium.org/3333014/diff/5001/6001#newcode29
remoting/jingle_glue/jingle_thread.cc:29: delayed_work_time_ = time;
On 2010/09/03 18:20:42, awong wrote:
> What's the threading assumptions on who calls this function?
>
> Should post ScheduleNextDelayedTask onto the thread_'s loop and pass the time
> argument as a closure with the task?
It is called by the MessageLoop only on the thread this message loop belongs to.
Added DCHECK for this in ScheduleNextDelayedTask().

http://codereview.chromium.org/3333014/diff/5001/6002
File remoting/jingle_glue/jingle_thread_unittest.cc (right):

http://codereview.chromium.org/3333014/diff/5001/6002#newcode20
remoting/jingle_glue/jingle_thread_unittest.cc:20: int kDelayMs = 100;
On 2010/09/03 18:20:42, awong wrote:
> Add comment explaining what this is used for any it's a sane value.

Done.

http://codereview.chromium.org/3333014/diff/5001/6002#newcode41
remoting/jingle_glue/jingle_thread_unittest.cc:41: EXPECT_CALL(*task, Run())
On 2010/09/03 18:20:42, awong wrote:
> can this fit on one line?

Done.

http://codereview.chromium.org/3333014/diff/5001/6002#newcode51
remoting/jingle_glue/jingle_thread_unittest.cc:51: EXPECT_TRUE((end -
start).InMillisecondsRoundedUp() > kDelayMs);
On 2010/09/03 18:20:42, awong wrote:
> EXPECT_GT?

Done.

[Sergey Ulanov, 2010-09-07 21:33:13]

+hclam
ping

[Alpha Left Google, 2010-09-07 22:02:12]

http://codereview.chromium.org/3333014/diff/9001/10002
File remoting/jingle_glue/jingle_thread_unittest.cc (right):

http://codereview.chromium.org/3333014/diff/9001/10002#newcode39
remoting/jingle_glue/jingle_thread_unittest.cc:39: TEST(JingleThreadTest,
PostDelayedTask) {
In generally we shouldn't have time-dependant tests. The following code may fail
on windows and become flaky because the task can be executed earlier than the
desired delay which I have experienced before. Instead of checking the delay I
think we should rely on that jingle is tested and we just make sure the task is
executed.

[Sergey Ulanov, 2010-09-07 22:18:42]

http://codereview.chromium.org/3333014/diff/9001/10002
File remoting/jingle_glue/jingle_thread_unittest.cc (right):

http://codereview.chromium.org/3333014/diff/9001/10002#newcode39
remoting/jingle_glue/jingle_thread_unittest.cc:39: TEST(JingleThreadTest,
PostDelayedTask) {
On 2010/09/07 22:02:13, Alpha wrote:
> In generally we shouldn't have time-dependant tests. The following code may
fail
> on windows and become flaky because the task can be executed earlier than the
> desired delay which I have experienced before. Instead of checking the delay I
> think we should rely on that jingle is tested and we just make sure the task
is
> executed.
Manual-reset semaphore is used here, so the test will not fail if the task is
executed before we call TimedWait(). The only potential issue I see here, is
that we may not receive signal within kDelayMs*10, just because the machine is
slow. This should not be a problem because kDelayMs is rather big. I tried this
code several times on try bots, including valgrind, and there were no failures.
Also, notice that there is no check for upper bound of the delay.
MessageQueue has similar tests and it works fine.

[Alpha Left Google, 2010-09-07 22:23:22]

On 2010/09/07 22:18:42, sergeyu wrote:
> http://codereview.chromium.org/3333014/diff/9001/10002
> File remoting/jingle_glue/jingle_thread_unittest.cc (right):
> 
> http://codereview.chromium.org/3333014/diff/9001/10002#newcode39
> remoting/jingle_glue/jingle_thread_unittest.cc:39: TEST(JingleThreadTest,
> PostDelayedTask) {
> On 2010/09/07 22:02:13, Alpha wrote:
> > In generally we shouldn't have time-dependant tests. The following code may
> fail
> > on windows and become flaky because the task can be executed earlier than
the
> > desired delay which I have experienced before. Instead of checking the delay
I
> > think we should rely on that jingle is tested and we just make sure the task
> is
> > executed.
> Manual-reset semaphore is used here, so the test will not fail if the task is
> executed before we call TimedWait(). The only potential issue I see here, is
> that we may not receive signal within kDelayMs*10, just because the machine is
> slow. This should not be a problem because kDelayMs is rather big. I tried
this
> code several times on try bots, including valgrind, and there were no
failures.
> Also, notice that there is no check for upper bound of the delay.
> MessageQueue has similar tests and it works fine.

Then why don't we wait indefinitely for the task to be executed? This is make
the test less time dependant. In that cast the test harness will timeout but
will be easy to catch, rather than potentially being flaky.

[Sergey Ulanov, 2010-09-07 22:37:19]

On 2010/09/07 22:23:22, Alpha wrote:
> Then why don't we wait indefinitely for the task to be executed? This is make
> the test less time dependant. In that cast the test harness will timeout but
> will be easy to catch, rather than potentially being flaky.

It is just so that the tests take less time to execute if, for whatever reason,
this test gets broken. I've increased timeout, and added new constant for it.

[Alpha Left Google, 2010-09-07 22:38:37]

On 2010/09/07 22:37:19, sergeyu wrote:
> On 2010/09/07 22:23:22, Alpha wrote:
> > Then why don't we wait indefinitely for the task to be executed? This is
make
> > the test less time dependant. In that cast the test harness will timeout but
> > will be easy to catch, rather than potentially being flaky.
> 
> It is just so that the tests take less time to execute if, for whatever
reason,
> this test gets broken. I've increased timeout, and added new constant for it.

ok, lgtm.