Return a long timeout value from TimeUntilNextProcess when the PacedSender is paused

Return a long timeout value from TimeUntilNextProcess when the PacedSender is paused

BUG=webrtc:7335
Review-Url: https://codereview.webrtc.org/2746153002
Cr-Commit-Position: refs/heads/master@{#17252}
Committed: https://chromium.googlesource.com/external/webrtc/+/919dce22d5414761b1ff1a93de3e6ab6078ae3a5

[tommi, 2017-03-13 14:51:39]

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

[tommi, 2017-03-13 14:51:44]

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

[commit-bot: I haz the power, 2017-03-13 14:51:49]

Dry run: 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, 2017-03-13 14:56:37]

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

[commit-bot: I haz the power, 2017-03-13 14:56:38]

Dry run: Try jobs failed on following builders:
  mac_baremetal on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_baremetal/builds/19783)

[tommi, 2017-03-13 15:39:27]

Update BUILD.gn

[tommi, 2017-03-13 15:39:29]

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

[commit-bot: I haz the power, 2017-03-13 15:39:31]

Dry run: 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, 2017-03-13 16:02:41]

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

[commit-bot: I haz the power, 2017-03-13 16:02:41]

Dry run: This issue passed the CQ dry run.

[stefan-webrtc, 2017-03-15 12:43:18]

https://codereview.webrtc.org/2746153002/diff/20001/webrtc/modules/pacing/pac...
File webrtc/modules/pacing/paced_sender.cc (right):

https://codereview.webrtc.org/2746153002/diff/20001/webrtc/modules/pacing/pac...
webrtc/modules/pacing/paced_sender.cc:284: process_thread_->WakeUp(this);
Is there a point in waking up the thread if we're being paused? In that case I
assume there's nothing to send?

https://codereview.webrtc.org/2746153002/diff/20001/webrtc/modules/pacing/pac...
webrtc/modules/pacing/paced_sender.cc:472: void
PacedSender::ProcessThreadAttached(ProcessThread* process_thread) {
We're never detaching because the thread is assumed to outlive the object?

https://codereview.webrtc.org/2746153002/diff/20001/webrtc/modules/pacing/pac...
File webrtc/modules/pacing/paced_sender_unittest.cc (right):

https://codereview.webrtc.org/2746153002/diff/20001/webrtc/modules/pacing/pac...
webrtc/modules/pacing/paced_sender_unittest.cc:639: EXPECT_LT(1000,
send_bucket_->TimeUntilNextProcess());
EXPECT_GT(..., 1000), please... I can't read this. :)

[tommi, 2017-03-15 13:42:48]

Address comments

[tommi, 2017-03-15 13:42:56]

https://codereview.webrtc.org/2746153002/diff/20001/webrtc/modules/pacing/pac...
File webrtc/modules/pacing/paced_sender.cc (right):

https://codereview.webrtc.org/2746153002/diff/20001/webrtc/modules/pacing/pac...
webrtc/modules/pacing/paced_sender.cc:284: process_thread_->WakeUp(this);
On 2017/03/15 12:43:18, stefan-webrtc wrote:
> Is there a point in waking up the thread if we're being paused? In that case I
> assume there's nothing to send?

Yes, I added a comment that explains why:

  // Tell the process thread to call our TimeUntilNextProcess() method to get
  // a new (longer) estimate for when to call Process().

https://codereview.webrtc.org/2746153002/diff/20001/webrtc/modules/pacing/pac...
webrtc/modules/pacing/paced_sender.cc:472: void
PacedSender::ProcessThreadAttached(ProcessThread* process_thread) {
On 2017/03/15 12:43:18, stefan-webrtc wrote:
> We're never detaching because the thread is assumed to outlive the object?

This method is called when the module is registered with the thread,
unregistered (process_thread == nullptr) or when the process thread itself
stops.  Here's the documentation:

  // This method is called when the module is attached to a *running* process
  // thread or detached from one.  In the case of detaching, |process_thread|
  // will be nullptr.
  //
  // This method will be called in the following cases:
  //
  // * Non-null process_thread:
  //   * ProcessThread::RegisterModule() is called while the thread is running.
  //   * ProcessThread::Start() is called and RegisterModule has previously
  //     been called.  The thread will be started immediately after notifying
  //     all modules.
  //
  // * Null process_thread:
  //   * ProcessThread::DeRegisterModule() is called while the thread is
  //     running.
  //   * ProcessThread::Stop() was called and the thread has been stopped.
  //
  // NOTE: This method is not called from the worker thread itself, but from
  //       the thread that registers/deregisters the module or calls Start/Stop.

https://codereview.webrtc.org/2746153002/diff/20001/webrtc/modules/pacing/pac...
File webrtc/modules/pacing/paced_sender_unittest.cc (right):

https://codereview.webrtc.org/2746153002/diff/20001/webrtc/modules/pacing/pac...
webrtc/modules/pacing/paced_sender_unittest.cc:639: EXPECT_LT(1000,
send_bucket_->TimeUntilNextProcess());
On 2017/03/15 12:43:18, stefan-webrtc wrote:
> EXPECT_GT(..., 1000), please... I can't read this. :)

haha, yeah, it looks pretty terrible :)  In general I feel that the convention
of |expected, actual| in the gtest macros, to be backwards.  I changed this to
use EXPECT_GE now though :)

    EXPECT_GE(send_bucket_->TimeUntilNextProcess(), 1000);

[tommi, 2017-03-15 13:43:19]

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

[commit-bot: I haz the power, 2017-03-15 13:43:25]

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

[stefan-webrtc, 2017-03-15 13:51:17]

lgtm

https://codereview.webrtc.org/2746153002/diff/20001/webrtc/modules/pacing/pac...
File webrtc/modules/pacing/paced_sender.cc (right):

https://codereview.webrtc.org/2746153002/diff/20001/webrtc/modules/pacing/pac...
webrtc/modules/pacing/paced_sender.cc:472: void
PacedSender::ProcessThreadAttached(ProcessThread* process_thread) {
On 2017/03/15 13:42:56, tommi (webrtc) wrote:
> On 2017/03/15 12:43:18, stefan-webrtc wrote:
> > We're never detaching because the thread is assumed to outlive the object?
> 
> This method is called when the module is registered with the thread,
> unregistered (process_thread == nullptr) or when the process thread itself
> stops.  Here's the documentation:
> 
>   // This method is called when the module is attached to a *running* process
>   // thread or detached from one.  In the case of detaching, |process_thread|
>   // will be nullptr.
>   //
>   // This method will be called in the following cases:
>   //
>   // * Non-null process_thread:
>   //   * ProcessThread::RegisterModule() is called while the thread is
running.
>   //   * ProcessThread::Start() is called and RegisterModule has previously
>   //     been called.  The thread will be started immediately after notifying
>   //     all modules.
>   //
>   // * Null process_thread:
>   //   * ProcessThread::DeRegisterModule() is called while the thread is
>   //     running.
>   //   * ProcessThread::Stop() was called and the thread has been stopped.
>   //
>   // NOTE: This method is not called from the worker thread itself, but from
>   //       the thread that registers/deregisters the module or calls
Start/Stop.

I see, thanks

[commit-bot: I haz the power, 2017-03-15 14:03:12]

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

[commit-bot: I haz the power, 2017-03-15 14:03:13]

Dry run: This issue passed the CQ dry run.

[tommi, 2017-03-15 14:43:08]

The CQ bit was checked by tommi@webrtc.org

[commit-bot: I haz the power, 2017-03-15 14:43:20]

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, 2017-03-15 14:45:36]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1489588988775270,
"parent_rev": "3f9b12d0564fb8e5c640b1cf114a8b763b1ca49d", "commit_rev":
"919dce22d5414761b1ff1a93de3e6ab6078ae3a5"}

[commit-bot: I haz the power, 2017-03-15 14:45:39]

Description was changed from

==========
Return a long timeout value from TimeUntilNextProcess when the PacedSender is
paused

BUG=webrtc:7335
==========

to

==========
Return a long timeout value from TimeUntilNextProcess when the PacedSender is
paused

BUG=webrtc:7335

Review-Url: https://codereview.webrtc.org/2746153002
Cr-Commit-Position: refs/heads/master@{#17252}
Committed:
https://chromium.googlesource.com/external/webrtc/+/919dce22d5414761b1ff1a93d...
==========

[commit-bot: I haz the power, 2017-03-15 14:45:40]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/external/webrtc/+/919dce22d5414761b1ff1a93d...