Revert "Revert of Update the BWE when the network route changes. (patchset #5 id:180001 of https://…

Revert "Revert of Update the BWE when the network route changes. (patchset #5 id:180001 of https://codereview.webrtc.org/2000063003/ )"

This reverts commit 72d41aa6da94dacb8a8464d1abd4ca7d1afffc65.

New change made:
Do not reset the BWE when the new network route is not ready to send media.

BUG=
R=pthatcher@webrtc.org, stefan@webrtc.org

Committed: https://crrev.com/5b5d2cdad7018993272525a723ef34f7da5c45f2
Cr-Commit-Position: refs/heads/master@{#13280}

[honghaiz3, 2016-06-06 17:23:11]

Patchset #1 (id:1) has been deleted

[honghaiz3, 2016-06-08 18:05:40]

Description was changed from

==========
Revert "Revert of Update the BWE when the network route changes. (patchset #5
id:180001 of https://codereview.webrtc.org/2000063003/ )"

This reverts commit 72d41aa6da94dacb8a8464d1abd4ca7d1afffc65.

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

to

==========
Revert "Revert of Update the BWE when the network route changes. (patchset #5
id:180001 of https://codereview.webrtc.org/2000063003/ )"

This reverts commit 72d41aa6da94dacb8a8464d1abd4ca7d1afffc65.

New change made:
Do not reset the BWE when the new network route is not ready to send media. 

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

[honghaiz3, 2016-06-08 18:06:01]

Patchset #2 (id:40001) has been deleted

[honghaiz3, 2016-06-08 18:06:57]

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

[honghaiz3, 2016-06-08 18:06:58]

PTAL.

[pthatcher1, 2016-06-08 18:26:40]

LGTM.  Mostly naming things I'd like to see different.

https://codereview.webrtc.org/2041593002/diff/60001/webrtc/base/networkroute.h
File webrtc/base/networkroute.h (right):

https://codereview.webrtc.org/2041593002/diff/60001/webrtc/base/networkroute....
webrtc/base/networkroute.h:33: NetworkRoute(bool is_connected,
Why not "connected"?

https://codereview.webrtc.org/2041593002/diff/60001/webrtc/p2p/base/port.h
File webrtc/p2p/base/port.h (right):

https://codereview.webrtc.org/2041593002/diff/60001/webrtc/p2p/base/port.h#ne...
webrtc/p2p/base/port.h:487: virtual bool ReadyToSendMedia() const {
Why not just connected()?

[honghaiz3, 2016-06-08 18:36:42]

https://codereview.webrtc.org/2041593002/diff/60001/webrtc/base/networkroute.h
File webrtc/base/networkroute.h (right):

https://codereview.webrtc.org/2041593002/diff/60001/webrtc/base/networkroute....
webrtc/base/networkroute.h:33: NetworkRoute(bool is_connected,
On 2016/06/08 18:26:39, pthatcher1 wrote:
> Why not "connected"?
Done

https://codereview.webrtc.org/2041593002/diff/60001/webrtc/p2p/base/port.h
File webrtc/p2p/base/port.h (right):

https://codereview.webrtc.org/2041593002/diff/60001/webrtc/p2p/base/port.h#ne...
webrtc/p2p/base/port.h:487: virtual bool ReadyToSendMedia() const {
On 2016/06/08 18:26:39, pthatcher1 wrote:
> Why not just connected()?

connected() has already been used in the class.

[pthatcher1, 2016-06-08 18:46:58]

https://codereview.webrtc.org/2041593002/diff/60001/webrtc/p2p/base/port.h
File webrtc/p2p/base/port.h (right):

https://codereview.webrtc.org/2041593002/diff/60001/webrtc/p2p/base/port.h#ne...
webrtc/p2p/base/port.h:487: virtual bool ReadyToSendMedia() const {
On 2016/06/08 18:36:42, honghaiz3 wrote:
> On 2016/06/08 18:26:39, pthatcher1 wrote:
> > Why not just connected()?
> 
> connected() has already been used in the class.

Ah, now you make me realize that this is almost the same as writable().   We're
just distinguishing between weakly and strongly writable.  Does it really matter
for the BWE?  Could we just use writable() instead of adding a new one?

Having connected, writable, and ReadyToSendMedia is getting to be a bit too many
things.

[honghaiz3, 2016-06-08 19:57:37]

On 2016/06/08 18:46:58, pthatcher1 wrote:
> https://codereview.webrtc.org/2041593002/diff/60001/webrtc/p2p/base/port.h
> File webrtc/p2p/base/port.h (right):
> 
>
https://codereview.webrtc.org/2041593002/diff/60001/webrtc/p2p/base/port.h#ne...
> webrtc/p2p/base/port.h:487: virtual bool ReadyToSendMedia() const {
> On 2016/06/08 18:36:42, honghaiz3 wrote:
> > On 2016/06/08 18:26:39, pthatcher1 wrote:
> > > Why not just connected()?
> > 
> > connected() has already been used in the class.
> 
> Ah, now you make me realize that this is almost the same as writable().  
We're
> just distinguishing between weakly and strongly writable.  Does it really
matter
> for the BWE?  Could we just use writable() instead of adding a new one?
> 
> Having connected, writable, and ReadyToSendMedia is getting to be a bit too
many
> things.

The reason I choose ReadyToSendMedia as either writable or write_unreliable is 
those are the two states where the connection will send media packets down (see
the changes in port.cc). 
It tells the app that the connection is ready to send things down. 
That might be a useful and more correct information to the app although we are
not using it now.

[honghaiz3, 2016-06-09 16:37:34]

Stefan, 

can you take another look at the CL?

https://codereview.webrtc.org/2041593002/diff/60001/webrtc/p2p/base/port.h
File webrtc/p2p/base/port.h (right):

https://codereview.webrtc.org/2041593002/diff/60001/webrtc/p2p/base/port.h#ne...
webrtc/p2p/base/port.h:487: virtual bool ReadyToSendMedia() const {
On 2016/06/08 18:46:58, pthatcher1 wrote:
> On 2016/06/08 18:36:42, honghaiz3 wrote:
> > On 2016/06/08 18:26:39, pthatcher1 wrote:
> > > Why not just connected()?
> > 
> > connected() has already been used in the class.
> 
> Ah, now you make me realize that this is almost the same as writable().  
We're
> just distinguishing between weakly and strongly writable.  Does it really
matter
> for the BWE?  Could we just use writable() instead of adding a new one?
> 
> Having connected, writable, and ReadyToSendMedia is getting to be a bit too
many
> things.
See my reply in the email (and the post of this change). 
Plus, this does not add new state, just a method derived from the write_state_.
I put it along with other convenience methods relying on write_state_.

[stefan-webrtc, 2016-06-09 17:08:02]

lgtm for call/ and modules/ which hasn't changed since my previous review.

[honghaiz3, 2016-06-09 17:20:05]

The CQ bit was checked by honghaiz@webrtc.org

[honghaiz3, 2016-06-09 17:20:06]

The patchset sent to the CQ was uploaded after l-g-t-m from pthatcher@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2041593002/#ps100001 (title:
"Re-arranged the method location.")

[commit-bot: I haz the power, 2016-06-09 17:20:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2041593002/100001

[honghaiz3, 2016-06-09 17:20:13]

The CQ bit was unchecked by honghaiz@webrtc.org

[honghaiz3, 2016-06-09 17:20:54]

The CQ bit was checked by honghaiz@webrtc.org

[commit-bot: I haz the power, 2016-06-09 17:20:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2041593002/100001

[commit-bot: I haz the power, 2016-06-09 17:27:02]

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

[commit-bot: I haz the power, 2016-06-09 17:27:03]

Try jobs failed on following builders:
  android_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_rel/builds/14059)

[honghaiz3, 2016-06-10 00:09:22]

The CQ bit was checked by honghaiz@webrtc.org

[commit-bot: I haz the power, 2016-06-10 00:09:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2041593002/100001

[commit-bot: I haz the power, 2016-06-10 00:19:58]

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

[commit-bot: I haz the power, 2016-06-10 00:19:59]

Try jobs failed on following builders:
  android_arm64_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_arm64_rel/build...)

[honghaiz3, 2016-06-24 17:01:05]

Description was changed from

==========
Revert "Revert of Update the BWE when the network route changes. (patchset #5
id:180001 of https://codereview.webrtc.org/2000063003/ )"

This reverts commit 72d41aa6da94dacb8a8464d1abd4ca7d1afffc65.

New change made:
Do not reset the BWE when the new network route is not ready to send media. 

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

to

==========
Revert "Revert of Update the BWE when the network route changes. (patchset #5
id:180001 of https://codereview.webrtc.org/2000063003/ )"

This reverts commit 72d41aa6da94dacb8a8464d1abd4ca7d1afffc65.

New change made:
Do not reset the BWE when the new network route is not ready to send media.

BUG=
R=pthatcher@webrtc.org, stefan@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/5b5d2cdad7018993272525a72...
==========

[honghaiz3, 2016-06-24 17:01:07]

Committed patchset #5 (id:120001) manually as
5b5d2cdad7018993272525a723ef34f7da5c45f2 (presubmit successful).

[commit-bot: I haz the power, 2016-06-24 17:01:09]

Description was changed from

==========
Revert "Revert of Update the BWE when the network route changes. (patchset #5
id:180001 of https://codereview.webrtc.org/2000063003/ )"

This reverts commit 72d41aa6da94dacb8a8464d1abd4ca7d1afffc65.

New change made:
Do not reset the BWE when the new network route is not ready to send media.

BUG=
R=pthatcher@webrtc.org, stefan@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/5b5d2cdad7018993272525a72...
==========

to

==========
Revert "Revert of Update the BWE when the network route changes. (patchset #5
id:180001 of https://codereview.webrtc.org/2000063003/ )"

This reverts commit 72d41aa6da94dacb8a8464d1abd4ca7d1afffc65.

New change made:
Do not reset the BWE when the new network route is not ready to send media.

BUG=
R=pthatcher@webrtc.org, stefan@webrtc.org

Committed: https://crrev.com/5b5d2cdad7018993272525a723ef34f7da5c45f2
Cr-Commit-Position: refs/heads/master@{#13280}
==========

[honghaiz3, 2016-06-24 17:05:57]

A revert of this CL (patchset #5 id:120001) has been created in
https://codereview.webrtc.org/2098703004/ by honghaiz@webrtc.org.

The reason for reverting is: ReadyToSendMedia did not consider the new
presumed_writable state. .