Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2896203003:
[NetworkChangeNotifier] Return more specific network connection types (ethernet/wifi) on OSX (Closed)
|
Created:
3 years, 7 months ago by jkarlin Modified:
3 years, 6 months ago Reviewers:
pauljensen CC:
chromium-reviews, cbentzel+watch_chromium.org, bnc+watch_chromium.org, mac-reviews_chromium.org, net-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[NetworkChangeNotifier] Return more specific network connection types (ethernet/wifi) on OSX
BUG=725485, 112937
Review-Url: https://codereview.chromium.org/2896203003
Cr-Commit-Position: refs/heads/master@{#475940}
Committed: https://chromium.googlesource.com/chromium/src/+/5780bb0e949db479fd386c5ad175074e23c12356
Patch Set 1 #Patch Set 2 : Fixes #
Total comments: 10
Patch Set 3 : Use notifier thread #Patch Set 4 : Use notifier thread #Patch Set 5 : Fix join issue #
Total comments: 2
Patch Set 6 : Address issues from PS5 #
Messages
Total messages: 34 (23 generated)
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
Patchset #2 (id:60001) has been deleted
The CQ bit was checked by jkarlin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
jkarlin@chromium.org changed reviewers: + pauljensen@chromium.org
Paul, PTAL, thanks!
https://codereview.chromium.org/2896203003/diff/80001/net/base/network_change... File net/base/network_change_notifier_mac.cc (left): https://codereview.chromium.org/2896203003/diff/80001/net/base/network_change... net/base/network_change_notifier_mac.cc:251: whitespace change? https://codereview.chromium.org/2896203003/diff/80001/net/base/network_change... File net/base/network_change_notifier_mac.cc (right): https://codereview.chromium.org/2896203003/diff/80001/net/base/network_change... net/base/network_change_notifier_mac.cc:103: // http://crbug.com/112937 I think we can remove this comment now. Can you also add 112937 to the "BUG=" line in the commit description. https://codereview.chromium.org/2896203003/diff/80001/net/base/network_change... File net/base/network_change_notifier_mac.h (right): https://codereview.chromium.org/2896203003/diff/80001/net/base/network_change... net/base/network_change_notifier_mac.h:62: void ReachabilityCallbackImpl(ConnectionType new_type); can you add a comment for this function? https://codereview.chromium.org/2896203003/diff/80001/net/base/network_change... net/base/network_change_notifier_mac.h:65: static ConnectionType CalculateConnectionType(SCNetworkConnectionFlags flags); ditto
https://codereview.chromium.org/2896203003/diff/80001/net/base/network_change... File net/base/network_change_notifier_mac.cc (right): https://codereview.chromium.org/2896203003/diff/80001/net/base/network_change... net/base/network_change_notifier_mac.cc:261: ->task_runner() hmm so I think this may not be the thread we want to use. NCNMac connection types are calculated on the NetworkConfigWatcherMac::notifier_thread_ thread, which I think might race the task posted here which could be bad.
Description was changed from ========== [NetworkChangeNotifier] Return more specific network connection types (ethernet/wifi) on OSX BUG=725485 ========== to ========== [NetworkChangeNotifier] Return more specific network connection types (ethernet/wifi) on OSX BUG=725485,112937 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by jkarlin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Changed it as discussed, PTAL, thanks! https://codereview.chromium.org/2896203003/diff/80001/net/base/network_change... File net/base/network_change_notifier_mac.cc (left): https://codereview.chromium.org/2896203003/diff/80001/net/base/network_change... net/base/network_change_notifier_mac.cc:251: On 2017/05/23 15:10:30, pauljensen wrote: > whitespace change? Done. https://codereview.chromium.org/2896203003/diff/80001/net/base/network_change... File net/base/network_change_notifier_mac.cc (right): https://codereview.chromium.org/2896203003/diff/80001/net/base/network_change... net/base/network_change_notifier_mac.cc:103: // http://crbug.com/112937 On 2017/05/23 15:10:30, pauljensen wrote: > I think we can remove this comment now. Can you also add 112937 to the "BUG=" > line in the commit description. Done. https://codereview.chromium.org/2896203003/diff/80001/net/base/network_change... net/base/network_change_notifier_mac.cc:261: ->task_runner() On 2017/05/23 15:18:15, pauljensen wrote: > hmm so I think this may not be the thread we want to use. NCNMac connection > types are calculated on the NetworkConfigWatcherMac::notifier_thread_ thread, > which I think might race the task posted here which could be bad. After F2F, moved to notifier thread and enabled blocking operations on the notifier thread. In terms of concerns of this patch on startup cost, I ran it on my 3 year old macbook and it ran ConnectionTypeFromInterfaces() on startup in < 1ms in each of three runs. run 1: 980 microseconds run 2: 446 microseconds run 3: 497 microseconds https://codereview.chromium.org/2896203003/diff/80001/net/base/network_change... File net/base/network_change_notifier_mac.h (right): https://codereview.chromium.org/2896203003/diff/80001/net/base/network_change... net/base/network_change_notifier_mac.h:62: void ReachabilityCallbackImpl(ConnectionType new_type); On 2017/05/23 15:10:30, pauljensen wrote: > can you add a comment for this function? No longer needed. https://codereview.chromium.org/2896203003/diff/80001/net/base/network_change... net/base/network_change_notifier_mac.h:65: static ConnectionType CalculateConnectionType(SCNetworkConnectionFlags flags); On 2017/05/23 15:10:30, pauljensen wrote: > ditto No longer needed.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by jkarlin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
PTAL, thanks!
Could you add a test for the ignored interfaces to the end of network_change_notifier_unittest.cc? There are already several similar tests towards the end of the file. https://codereview.chromium.org/2896203003/diff/140001/net/base/network_confi... File net/base/network_config_watcher_mac.cc (left): https://codereview.chromium.org/2896203003/diff/140001/net/base/network_confi... net/base/network_config_watcher_mac.cc:65: needless whitespace change
The CQ bit was checked by jkarlin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Done, PTAL! https://codereview.chromium.org/2896203003/diff/140001/net/base/network_confi... File net/base/network_config_watcher_mac.cc (left): https://codereview.chromium.org/2896203003/diff/140001/net/base/network_confi... net/base/network_config_watcher_mac.cc:65: On 2017/05/31 14:52:38, pauljensen wrote: > needless whitespace change Done.
lgtm
Thanks!
The CQ bit was unchecked by jkarlin@chromium.org
The CQ bit was checked by jkarlin@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 160001, "attempt_start_ts": 1496244656031490,
"parent_rev": "c70ce762d963655ec12866e88f3f1107dcaea560", "commit_rev":
"5780bb0e949db479fd386c5ad175074e23c12356"}
Message was sent while issue was closed.
Description was changed from ========== [NetworkChangeNotifier] Return more specific network connection types (ethernet/wifi) on OSX BUG=725485,112937 ========== to ========== [NetworkChangeNotifier] Return more specific network connection types (ethernet/wifi) on OSX BUG=725485,112937 Review-Url: https://codereview.chromium.org/2896203003 Cr-Commit-Position: refs/heads/master@{#475940} Committed: https://chromium.googlesource.com/chromium/src/+/5780bb0e949db479fd386c5ad175... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:160001) as https://chromium.googlesource.com/chromium/src/+/5780bb0e949db479fd386c5ad175... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
