Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
4 years, 7 months ago by honghaiz3 Modified:
4 years, 7 months ago Reviewers:
pthatcher2, Taylor Brandstetter, pthatcher1 CC:
juberti1 Base URL:
https://chromium.googlesource.com/external/webrtc@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionUpdate the type and cost of existing networks
if the network monitor detects it after the native code does.
Also set the network cost for ethernet, wifi, unknown, cellular network type to be 0, 10, 50, 900,
so that unknown networks will have lower precedence than known networks with low cost (like Wifi) but higher precedence than known networks with high cost.
And third, infer network type based on limited name matching in Android if there is no network monitor or network monitor did not find the type.
BUG=webrtc:5890
R=pthatcher@chromium.org, pthatcher@webrtc.org
Committed: https://crrev.com/351d77b702d61eef28c4b6273e0c091ce177d1a0
Cr-Commit-Position: refs/heads/master@{#12833}
Patch Set 1 : #Patch Set 2 : Update stun lifetime when network cost changes and add network type for "v4-rmnet" #Patch Set 3 : Fix a test failure #Patch Set 4 : Get network type from name matching only on IOS or Android. #
Total comments: 24
Patch Set 5 : Address comments #Patch Set 6 : rebase and merge to head #Patch Set 7 : Minor changes #
Created: 4 years, 7 months ago
Messages
Total messages: 50 (34 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Description was changed from ========== Update the network type and cost if it is collected before NetowkrMonitor is started. BUG= ========== to ========== Update the network type and cost if it is collected before NetowkrMonitor is started. BUG=webrtc:5890 ==========
honghaiz@webrtc.org changed reviewers: + pthatcher@webrtc.org
honghaiz@webrtc.org changed reviewers: + deadbeef@webrtc.org
Description was changed from ========== Update the network type and cost if it is collected before NetowkrMonitor is started. BUG=webrtc:5890 ========== to ========== Update the type and cost of existing networks after network monitor starts. Also set the network cost for unknown network type to be 10, so that it will be less preferred than known networks with zero-cost (like Wifi). BUG=webrtc:5890 ==========
Description was changed from ========== Update the type and cost of existing networks after network monitor starts. Also set the network cost for unknown network type to be 10, so that it will be less preferred than known networks with zero-cost (like Wifi). BUG=webrtc:5890 ========== to ========== Update the type and cost of existing networks after network monitor starts. Also set the network cost for unknown network type to be 10, so that it will be less preferred than known networks with zero-cost (like Wifi). And third, infer network type based on limited name matching if there is no network monitor or network monitor did not find the type. BUG=webrtc:5890 ==========
On 2016/05/13 01:13:33, honghaiz3 wrote: Please hold on this CL. I think there is a better way to solve this. Will try it tomorrow.
honghaiz@webrtc.org changed reviewers: - deadbeef@webrtc.org, pthatcher@webrtc.org
Description was changed from ========== Update the type and cost of existing networks after network monitor starts. Also set the network cost for unknown network type to be 10, so that it will be less preferred than known networks with zero-cost (like Wifi). And third, infer network type based on limited name matching if there is no network monitor or network monitor did not find the type. BUG=webrtc:5890 ========== to ========== Update the type and cost of existing networks if the network monitor detected it later than the native code. Also set the network cost for unknown network type to be 10, so that it will be less preferred than known networks with zero-cost (like Wifi). And third, infer network type based on limited name matching if there is no network monitor or network monitor did not find the type. BUG=webrtc:5890 ==========
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
Patchset #1 (id:100001) has been deleted
Patchset #1 (id:120001) has been deleted
Patchset #1 (id:140001) has been deleted
Patchset #1 (id:160001) has been deleted
Description was changed from ========== Update the type and cost of existing networks if the network monitor detected it later than the native code. Also set the network cost for unknown network type to be 10, so that it will be less preferred than known networks with zero-cost (like Wifi). And third, infer network type based on limited name matching if there is no network monitor or network monitor did not find the type. BUG=webrtc:5890 ========== to ========== Update the type and cost of existing networks if the network monitor detects it after the native code does. Also set the network cost for unknown network type to be 10, so that it will be less preferred than known networks with zero-cost (like Wifi). And third, infer network type based on limited name matching if there is no network monitor or network monitor did not find the type. BUG=webrtc:5890 ==========
honghaiz@webrtc.org changed reviewers: + deadbeef@webrtc.org, pthatcher@webrtc.org
Patchset #1 (id:180001) has been deleted
Patchset #2 (id:220001) has been deleted
Patchset #2 (id:240001) has been deleted
On 2016/05/14 01:55:01, honghaiz3 wrote: Made a few fixes and it is all done. Please take a look at this.
Good catch changing the ping keep alive lifetime. https://codereview.webrtc.org/1976683003/diff/300001/webrtc/api/java/jni/andr... File webrtc/api/java/jni/androidnetworkmonitor_jni.cc (right): https://codereview.webrtc.org/1976683003/diff/300001/webrtc/api/java/jni/andr... webrtc/api/java/jni/androidnetworkmonitor_jni.cc:273: // Fire the NetworksChangedSignal to update the list of networks. Do you mean SignalNetworksChanged? https://codereview.webrtc.org/1976683003/diff/300001/webrtc/api/java/jni/andr... webrtc/api/java/jni/androidnetworkmonitor_jni.cc:309: LOG(LS_INFO) << "Android network monitor finds " << network_infos.size() finds => found https://codereview.webrtc.org/1976683003/diff/300001/webrtc/base/network.h File webrtc/base/network.h (right): https://codereview.webrtc.org/1976683003/diff/300001/webrtc/base/network.h#ne... webrtc/base/network.h:41: static const uint16_t kNetworkCostForUnknownType = 10; See my comments about these names in another CL (I'd prefer something like kNetworkCostMin = 0, kNetworkCostLow = 10, kNetworkCostUnknown = 50, kNetworkCostHigh = 900, kNetworkCostMax = 999). https://codereview.webrtc.org/1976683003/diff/300001/webrtc/base/network.h#ne... webrtc/base/network.h:280: sigslot::signal1<const Network*> SignalNetworkTypeChanged; SignalTypeChanged would be enough, since this is a Network. https://codereview.webrtc.org/1976683003/diff/300001/webrtc/base/network.h#ne... webrtc/base/network.h:353: if (type_ != type) { I'd slightly prefer this with an early return: if (type_ == type) { return; } type_ = type; SignalTypeChanged(this); https://codereview.webrtc.org/1976683003/diff/300001/webrtc/base/network.h#ne... webrtc/base/network.h:365: default: Again, see my comments in the other CL where I think the default should be "UNKNOWN" and we should enumerate wifi/ethernet, and should prefer ethernet slightly over wifi. https://codereview.webrtc.org/1976683003/diff/300001/webrtc/base/network_unit... File webrtc/base/network_unittest.cc (right): https://codereview.webrtc.org/1976683003/diff/300001/webrtc/base/network_unit... webrtc/base/network_unittest.cc:828: // Note: do not call ClearNetworks here to check that an existing network do => Do https://codereview.webrtc.org/1976683003/diff/300001/webrtc/base/network_unit... webrtc/base/network_unittest.cc:829: // will be updated. Why not? It could use some explanation. https://codereview.webrtc.org/1976683003/diff/300001/webrtc/base/network_unit... webrtc/base/network_unittest.cc:853: // a few cases. Note that UNKNOW type for non-matching strings has been tested UNKNOW => UNKNOWN https://codereview.webrtc.org/1976683003/diff/300001/webrtc/p2p/base/port.cc File webrtc/p2p/base/port.cc (right): https://codereview.webrtc.org/1976683003/diff/300001/webrtc/p2p/base/port.cc#... webrtc/p2p/base/port.cc:1034: SignalStateChange(this); If we always send the cost, then this could cause issues when packets get reordered. It's not a very big deal, but we should note it. https://codereview.webrtc.org/1976683003/diff/300001/webrtc/p2p/base/stunport.h File webrtc/p2p/base/stunport.h (right): https://codereview.webrtc.org/1976683003/diff/300001/webrtc/p2p/base/stunport... webrtc/p2p/base/stunport.h:232: int GetStunKeepaliveLifetimeFromNetworkCost() { GetStunKeepaliveLifetime() is probably sufficient. https://codereview.webrtc.org/1976683003/diff/300001/webrtc/p2p/base/stunport... webrtc/p2p/base/stunport.h:233: return (network_cost() == rtc::kMaxNetworkCost) We should probably make this network_cost() > rtc::kNetworkCostHigh
Patchset #5 (id:320001) has been deleted
Description was changed from ========== Update the type and cost of existing networks if the network monitor detects it after the native code does. Also set the network cost for unknown network type to be 10, so that it will be less preferred than known networks with zero-cost (like Wifi). And third, infer network type based on limited name matching if there is no network monitor or network monitor did not find the type. BUG=webrtc:5890 ========== to ========== Update the type and cost of existing networks if the network monitor detects it after the native code does. Also set the network cost for ethernet, wifi, unknown, cellular network type to be 0, 10, 50, 900, so that it will be less preferred than known networks with low-cost (like Wifi). And third, infer network type based on limited name matching if there is no network monitor or network monitor did not find the type. BUG=webrtc:5890 ==========
PTAL. https://codereview.webrtc.org/1976683003/diff/300001/webrtc/api/java/jni/andr... File webrtc/api/java/jni/androidnetworkmonitor_jni.cc (right): https://codereview.webrtc.org/1976683003/diff/300001/webrtc/api/java/jni/andr... webrtc/api/java/jni/androidnetworkmonitor_jni.cc:273: // Fire the NetworksChangedSignal to update the list of networks. On 2016/05/17 21:23:11, pthatcher1 wrote: > Do you mean SignalNetworksChanged? > Ah Yes. https://codereview.webrtc.org/1976683003/diff/300001/webrtc/api/java/jni/andr... webrtc/api/java/jni/androidnetworkmonitor_jni.cc:309: LOG(LS_INFO) << "Android network monitor finds " << network_infos.size() On 2016/05/17 21:23:11, pthatcher1 wrote: > finds => found Done. https://codereview.webrtc.org/1976683003/diff/300001/webrtc/base/network.h File webrtc/base/network.h (right): https://codereview.webrtc.org/1976683003/diff/300001/webrtc/base/network.h#ne... webrtc/base/network.h:41: static const uint16_t kNetworkCostForUnknownType = 10; On 2016/05/17 21:23:11, pthatcher1 wrote: > See my comments about these names in another CL (I'd prefer something like > kNetworkCostMin = 0, kNetworkCostLow = 10, kNetworkCostUnknown = 50, > kNetworkCostHigh = 900, kNetworkCostMax = 999). Done. I once worried that one client may set its cellular cost to be 901 so that his cellular will be less used than the other side. So I set the cellular cost to the maximum vale 999. But since webrtc is controlling the cost assignment on both sides, it is perhaps OK to assign it to 900. Let me know if you think this may be an issue. https://codereview.webrtc.org/1976683003/diff/300001/webrtc/base/network.h#ne... webrtc/base/network.h:280: sigslot::signal1<const Network*> SignalNetworkTypeChanged; On 2016/05/17 21:23:11, pthatcher1 wrote: > SignalTypeChanged would be enough, since this is a Network. Done. https://codereview.webrtc.org/1976683003/diff/300001/webrtc/base/network.h#ne... webrtc/base/network.h:353: if (type_ != type) { On 2016/05/17 21:23:11, pthatcher1 wrote: > I'd slightly prefer this with an early return: > > if (type_ == type) { > return; > } > > type_ = type; > SignalTypeChanged(this); Done. https://codereview.webrtc.org/1976683003/diff/300001/webrtc/base/network.h#ne... webrtc/base/network.h:365: default: On 2016/05/17 21:23:11, pthatcher1 wrote: > Again, see my comments in the other CL where I think the default should be > "UNKNOWN" and we should enumerate wifi/ethernet, and should prefer ethernet > slightly over wifi. Done. https://codereview.webrtc.org/1976683003/diff/300001/webrtc/base/network_unit... File webrtc/base/network_unittest.cc (right): https://codereview.webrtc.org/1976683003/diff/300001/webrtc/base/network_unit... webrtc/base/network_unittest.cc:828: // Note: do not call ClearNetworks here to check that an existing network On 2016/05/17 21:23:11, pthatcher1 wrote: > do => Do Done. https://codereview.webrtc.org/1976683003/diff/300001/webrtc/base/network_unit... webrtc/base/network_unittest.cc:829: // will be updated. On 2016/05/17 21:23:11, pthatcher1 wrote: > Why not? It could use some explanation. Added explanations. https://codereview.webrtc.org/1976683003/diff/300001/webrtc/base/network_unit... webrtc/base/network_unittest.cc:853: // a few cases. Note that UNKNOW type for non-matching strings has been tested On 2016/05/17 21:23:11, pthatcher1 wrote: > UNKNOW => UNKNOWN Done. https://codereview.webrtc.org/1976683003/diff/300001/webrtc/p2p/base/port.cc File webrtc/p2p/base/port.cc (right): https://codereview.webrtc.org/1976683003/diff/300001/webrtc/p2p/base/port.cc#... webrtc/p2p/base/port.cc:1034: SignalStateChange(this); On 2016/05/17 21:23:11, pthatcher1 wrote: > If we always send the cost, then this could cause issues when packets get > reordered. It's not a very big deal, but we should note it. Added note at the beginning of the added code. https://codereview.webrtc.org/1976683003/diff/300001/webrtc/p2p/base/stunport.h File webrtc/p2p/base/stunport.h (right): https://codereview.webrtc.org/1976683003/diff/300001/webrtc/p2p/base/stunport... webrtc/p2p/base/stunport.h:232: int GetStunKeepaliveLifetimeFromNetworkCost() { On 2016/05/17 21:23:11, pthatcher1 wrote: > GetStunKeepaliveLifetime() is probably sufficient. Done. https://codereview.webrtc.org/1976683003/diff/300001/webrtc/p2p/base/stunport... webrtc/p2p/base/stunport.h:233: return (network_cost() == rtc::kMaxNetworkCost) On 2016/05/17 21:23:11, pthatcher1 wrote: > We should probably make this network_cost() > rtc::kNetworkCostHigh Done >=
Ping. PTAL when you get time.
pthatcher@chromium.org changed reviewers: + pthatcher@chromium.org
lgtm
Description was changed from ========== Update the type and cost of existing networks if the network monitor detects it after the native code does. Also set the network cost for ethernet, wifi, unknown, cellular network type to be 0, 10, 50, 900, so that it will be less preferred than known networks with low-cost (like Wifi). And third, infer network type based on limited name matching if there is no network monitor or network monitor did not find the type. BUG=webrtc:5890 ========== to ========== Update the type and cost of existing networks if the network monitor detects it after the native code does. Also set the network cost for ethernet, wifi, unknown, cellular network type to be 0, 10, 50, 900, so that unknown networks will have lower precedence than known networks with low cost (like Wifi) but higher precedence than known networks with high cost. And third, infer network type based on limited name matching in Android if there is no network monitor or network monitor did not find the type. BUG=webrtc:5890 ==========
The CQ bit was checked by honghaiz@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1976683003/360001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1976683003/360001
Patchset #7 (id:380001) has been deleted
Patchset #7 (id:390001) has been deleted
The CQ bit was checked by honghaiz@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from pthatcher@chromium.org Link to the patchset: https://codereview.webrtc.org/1976683003/#ps410001 (title: "Minor changes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1976683003/410001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1976683003/410001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/5717)
On 2016/05/20 21:05:52, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > presubmit on tryserver.webrtc (JOB_FAILED, > http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/5717) Peter, can you lgtm using your webrtc account?
lgtm Sorry about that.
The CQ bit was checked by honghaiz@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1976683003/410001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1976683003/410001
Description was changed from ========== Update the type and cost of existing networks if the network monitor detects it after the native code does. Also set the network cost for ethernet, wifi, unknown, cellular network type to be 0, 10, 50, 900, so that unknown networks will have lower precedence than known networks with low cost (like Wifi) but higher precedence than known networks with high cost. And third, infer network type based on limited name matching in Android if there is no network monitor or network monitor did not find the type. BUG=webrtc:5890 ========== to ========== Update the type and cost of existing networks if the network monitor detects it after the native code does. Also set the network cost for ethernet, wifi, unknown, cellular network type to be 0, 10, 50, 900, so that unknown networks will have lower precedence than known networks with low cost (like Wifi) but higher precedence than known networks with high cost. And third, infer network type based on limited name matching in Android if there is no network monitor or network monitor did not find the type. BUG=webrtc:5890 R=pthatcher@chromium.org, pthatcher@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/351d77b702d61eef28c4b6273... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:410001) manually as 351d77b702d61eef28c4b6273e0c091ce177d1a0 (presubmit successful).
Message was sent while issue was closed.
Description was changed from ========== Update the type and cost of existing networks if the network monitor detects it after the native code does. Also set the network cost for ethernet, wifi, unknown, cellular network type to be 0, 10, 50, 900, so that unknown networks will have lower precedence than known networks with low cost (like Wifi) but higher precedence than known networks with high cost. And third, infer network type based on limited name matching in Android if there is no network monitor or network monitor did not find the type. BUG=webrtc:5890 R=pthatcher@chromium.org, pthatcher@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/351d77b702d61eef28c4b6273... ========== to ========== Update the type and cost of existing networks if the network monitor detects it after the native code does. Also set the network cost for ethernet, wifi, unknown, cellular network type to be 0, 10, 50, 900, so that unknown networks will have lower precedence than known networks with low cost (like Wifi) but higher precedence than known networks with high cost. And third, infer network type based on limited name matching in Android if there is no network monitor or network monitor did not find the type. BUG=webrtc:5890 R=pthatcher@chromium.org, pthatcher@webrtc.org Committed: https://crrev.com/351d77b702d61eef28c4b6273e0c091ce177d1a0 Cr-Commit-Position: refs/heads/master@{#12833} ========== |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
