Schedule a CreatePermissionRequest after the success of a previous request

Schedule a CreatePermissionRequest after the success of a previous request
unless a channel binding request is already scheduled.

BUG=5178
R=pthatcher@webrtc.org

Committed: https://crrev.com/8597543ae845073ca32876bfaaff207017b6c0eb
Cr-Commit-Position: refs/heads/master@{#10625}

[honghaiz3, 2015-11-06 19:09:43]

Patchset #1 (id:1) has been deleted

[honghaiz3, 2015-11-06 19:10:03]

honghaiz@webrtc.org changed reviewers:
+ pthatcher@webrtc.org

[honghaiz3, 2015-11-06 19:11:18]

Description was changed from

==========
Schedule a CreatePermissionRequest after the success of a previous request
unless a channel binding request is already scheduled.

BUG=
==========

to

==========
Schedule a CreatePermissionRequest after the success of a previous request
unless a channel binding request is already scheduled.

BUG=5178
==========

[honghaiz3, 2015-11-09 18:15:35]

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

[commit-bot: I haz the power, 2015-11-09 18:15:44]

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

[commit-bot: I haz the power, 2015-11-09 20:15:57]

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

[commit-bot: I haz the power, 2015-11-09 20:15:57]

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

[pthatcher1, 2015-11-11 20:22:09]

https://codereview.webrtc.org/1434603002/diff/20001/webrtc/p2p/base/turnport.cc
File webrtc/p2p/base/turnport.cc (right):

https://codereview.webrtc.org/1434603002/diff/20001/webrtc/p2p/base/turnport....
webrtc/p2p/base/turnport.cc:1298: }
To make it a little more readable, can you make two methods?

SendCreatePermissionRequest()
SendCreatePermissionRequestAfter(int delay)

Obviously, the first can call the second.

https://codereview.webrtc.org/1434603002/diff/20001/webrtc/p2p/base/turnport....
webrtc/p2p/base/turnport.cc:1342: // by ChannelBindRequest.
If we are STATE_BINDING and the ChannelBindRequest fails, wouldn't we still need
the CreatePermissionRequest refresh?  Should we just check for != STATE_BOUND?

https://codereview.webrtc.org/1434603002/diff/20001/webrtc/p2p/base/turnport....
webrtc/p2p/base/turnport.cc:1344:
SendCreatePermissionRequest(TURN_PERMISSION_TIMEOUT - 60000);
Can you explain the 60000, or make it a named constant?

https://codereview.webrtc.org/1434603002/diff/20001/webrtc/p2p/base/turnport_...
File webrtc/p2p/base/turnport_unittest.cc (right):

https://codereview.webrtc.org/1434603002/diff/20001/webrtc/p2p/base/turnport_...
webrtc/p2p/base/turnport_unittest.cc:403:
ASSERT_TRUE_WAIT(turn_create_permission_success_, kTimeout);
Unfortunately, this doesn't test *when* the permission refresh will be sent.  Is
there some way we could test that?

https://codereview.webrtc.org/1434603002/diff/20001/webrtc/p2p/base/turnport_...
webrtc/p2p/base/turnport_unittest.cc:740: TestRefreshCreatePermissionRequest();
Why did you put this in a separate method?  Why not just put it here?

[honghaiz3, 2015-11-11 23:51:59]

PTAL.

https://codereview.webrtc.org/1434603002/diff/20001/webrtc/p2p/base/turnport.cc
File webrtc/p2p/base/turnport.cc (right):

https://codereview.webrtc.org/1434603002/diff/20001/webrtc/p2p/base/turnport....
webrtc/p2p/base/turnport.cc:1298: }
On 2015/11/11 20:22:09, pthatcher1 wrote:
> To make it a little more readable, can you make two methods?
> 
> SendCreatePermissionRequest()
> SendCreatePermissionRequestAfter(int delay)
> 
> Obviously, the first can call the second.
Of course I can, but using a delay parameter for both delayed and non-delayed
sending is used in multiple occasions in this file (as well as request manager).
For example, The next method SendChannelBindRequest, TurnPort::SendRequest. So
perhaps, using a single method with a delay parameter is more consistent with
the rest of the implementation.

https://codereview.webrtc.org/1434603002/diff/20001/webrtc/p2p/base/turnport....
webrtc/p2p/base/turnport.cc:1342: // by ChannelBindRequest.
On 2015/11/11 20:22:09, pthatcher1 wrote:
> If we are STATE_BINDING and the ChannelBindRequest fails, wouldn't we still
need
> the CreatePermissionRequest refresh?  Should we just check for != STATE_BOUND?

Done

https://codereview.webrtc.org/1434603002/diff/20001/webrtc/p2p/base/turnport....
webrtc/p2p/base/turnport.cc:1344:
SendCreatePermissionRequest(TURN_PERMISSION_TIMEOUT - 60000);
On 2015/11/11 20:22:09, pthatcher1 wrote:
> Can you explain the 60000, or make it a named constant?
Added comments.

https://codereview.webrtc.org/1434603002/diff/20001/webrtc/p2p/base/turnport_...
File webrtc/p2p/base/turnport_unittest.cc (right):

https://codereview.webrtc.org/1434603002/diff/20001/webrtc/p2p/base/turnport_...
webrtc/p2p/base/turnport_unittest.cc:403:
ASSERT_TRUE_WAIT(turn_create_permission_success_, kTimeout);
On 2015/11/11 20:22:09, pthatcher1 wrote:
> Unfortunately, this doesn't test *when* the permission refresh will be sent. 
Is
> there some way we could test that?
It is pretty hard to test that. We don't want to wait for 4 minutes in the
tests.
In order to test that, we will need to 
1. create a member variable turn_permission_timeout_ and change it to a smaller
value in the tests and wait until it fires. 
But that's not too much different from FlushRequests here because we are not
testing the actually scheduled time in the implementation.

https://codereview.webrtc.org/1434603002/diff/20001/webrtc/p2p/base/turnport_...
webrtc/p2p/base/turnport_unittest.cc:740: TestRefreshCreatePermissionRequest();
On 2015/11/11 20:22:09, pthatcher1 wrote:
> Why did you put this in a separate method?  Why not just put it here?

Done.

[pthatcher1, 2015-11-12 00:21:08]

lgtm

https://codereview.webrtc.org/1434603002/diff/20001/webrtc/p2p/base/turnport.cc
File webrtc/p2p/base/turnport.cc (right):

https://codereview.webrtc.org/1434603002/diff/20001/webrtc/p2p/base/turnport....
webrtc/p2p/base/turnport.cc:1298: }
On 2015/11/11 23:51:58, honghaiz3 wrote:
> On 2015/11/11 20:22:09, pthatcher1 wrote:
> > To make it a little more readable, can you make two methods?
> > 
> > SendCreatePermissionRequest()
> > SendCreatePermissionRequestAfter(int delay)
> > 
> > Obviously, the first can call the second.
> Of course I can, but using a delay parameter for both delayed and non-delayed
> sending is used in multiple occasions in this file (as well as request
manager).
> For example, The next method SendChannelBindRequest, TurnPort::SendRequest. So
> perhaps, using a single method with a delay parameter is more consistent with
> the rest of the implementation. 

Good point.  Feel free to leave it.

[honghaiz3, 2015-11-12 17:17:28]

The CQ bit was checked by honghaiz@webrtc.org

[commit-bot: I haz the power, 2015-11-12 17:17:34]

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

[honghaiz3, 2015-11-12 19:07:31]

Committed patchset #2 (id:40001) manually as
8597543ae845073ca32876bfaaff207017b6c0eb (presubmit successful).

[commit-bot: I haz the power, 2015-11-12 19:07:32]

Patchset 2 (id:??) landed as
https://crrev.com/8597543ae845073ca32876bfaaff207017b6c0eb
Cr-Commit-Position: refs/heads/master@{#10625}