Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2081493002:
Obtain WiFi SSID on Android using Android APIs (Closed)
DescriptionObtain WiFi SSID on Android using Android APIs
BUG=620529
Committed: https://crrev.com/c5a1c91d91800672965832bbd62838cd16a022fe
Cr-Commit-Position: refs/heads/master@{#403549}
Patch Set 1 : Fix #
Total comments: 6
Patch Set 2 : Rebased #Patch Set 3 : Addressed kundaji comments #Patch Set 4 : Moved common code to AndroidNetworkLibrary.java #Patch Set 5 : Rebased #Patch Set 6 : Remove NQE changes from this CL #Patch Set 7 : Fix tests #
Messages
Total messages: 34 (18 generated)
Description was changed from ========== Plumb WiFi SSID from NCN to native BUG= ========== to ========== Plumb WiFi SSID from NCN to native Also, record how frequently the network ID (WiFi SSID or MCC/MNC information was available on Android). BUG= ==========
Patchset #1 (id:1) has been deleted
Description was changed from ========== Plumb WiFi SSID from NCN to native Also, record how frequently the network ID (WiFi SSID or MCC/MNC information was available on Android). BUG= ========== to ========== Plumb WiFi SSID from NCN to native Also, record how frequently the network ID (WiFi SSID or MCC/MNC information was available on Android). BUG=620529 ==========
Description was changed from ========== Plumb WiFi SSID from NCN to native Also, record how frequently the network ID (WiFi SSID or MCC/MNC information was available on Android). BUG=620529 ========== to ========== Plumb WiFi SSID from NCN to native Also, record in NQE how frequently the network ID (WiFi SSID or MCC/MNC information) was available on Android. BUG=620529 ==========
Description was changed from ========== Plumb WiFi SSID from NCN to native Also, record in NQE how frequently the network ID (WiFi SSID or MCC/MNC information) was available on Android. BUG=620529 ========== to ========== Plumb WiFi SSID from NCN to native Also, record in NQE how frequently the network ID (WiFi SSID or MCC/MNC information) was available on Android. BUG=620529 ==========
tbansal@chromium.org changed reviewers: + bengr@chromium.org
bengr: ptal. Thanks.
tbansal@chromium.org changed reviewers: + kundaji@chromium.org - bengr@chromium.org
Switching to kundaji@ for first round. PTAL. Thanks!!
lgtm Mostly looks good. Some minor comments. https://codereview.chromium.org/2081493002/diff/20001/net/android/network_cha... File net/android/network_change_notifier_android.cc (right): https://codereview.chromium.org/2081493002/diff/20001/net/android/network_cha... net/android/network_change_notifier_android.cc:11: // - GetCurrentConnectionType() can be called on any thread. Should GetCurrentWiFiSSID be added here as well? Although the .h file is typically where this information is documented. https://codereview.chromium.org/2081493002/diff/20001/net/base/network_change... File net/base/network_change_notifier.h (right): https://codereview.chromium.org/2081493002/diff/20001/net/base/network_change... net/base/network_change_notifier.h:280: // is not CONNECTION_WIFI, or if the SSID is unavailable, it may return an "may" -> "will"? https://codereview.chromium.org/2081493002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2081493002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:34858: + Recorded only on Wi-Fi and cellular connections. "Recorded only on Wi-Fi and cellular" is repeated. Also, would a rename to IsNetworkIdAvailable be more appropriate to reflect that this is a boolean, as opposed to actually containing the available network id?
tbansal@chromium.org changed reviewers: + pauljensen@chromium.org
pauljensen: ptal. Thanks. https://codereview.chromium.org/2081493002/diff/20001/net/android/network_cha... File net/android/network_change_notifier_android.cc (right): https://codereview.chromium.org/2081493002/diff/20001/net/android/network_cha... net/android/network_change_notifier_android.cc:11: // - GetCurrentConnectionType() can be called on any thread. On 2016/06/23 20:08:56, kundaji wrote: > Should GetCurrentWiFiSSID be added here as well? Although the .h file is > typically where this information is documented. Not sure if it is necessary since this is sort of an internal class. Consumers are expected to only use NetworkChangeNotifier which describes the function GetCurrentWiFiSSID. https://codereview.chromium.org/2081493002/diff/20001/net/base/network_change... File net/base/network_change_notifier.h (right): https://codereview.chromium.org/2081493002/diff/20001/net/base/network_change... net/base/network_change_notifier.h:280: // is not CONNECTION_WIFI, or if the SSID is unavailable, it may return an On 2016/06/23 20:08:56, kundaji wrote: > "may" -> "will"? Done. https://codereview.chromium.org/2081493002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2081493002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:34858: + Recorded only on Wi-Fi and cellular connections. On 2016/06/23 20:08:56, kundaji wrote: > "Recorded only on Wi-Fi and cellular" is repeated. > > Also, would a rename to IsNetworkIdAvailable be more appropriate to reflect that > this is a boolean, as opposed to actually containing the available network id? Done.
Please expose through net::GetWifiSSID(), not by adding a new API.
On 2016/06/28 11:11:31, pauljensen wrote: > Please expose through net::GetWifiSSID(), not by adding a new API. Can you suggest a good way to reuse the WiFiDelegate code in NCNAutoDetect. One way is to move it to a separate class, convert it into a static class, and then let net::GetWiFiSSID() use it too through JNI bindings. Other way is to keep it as non-static class, but then I am not sure who should own it (may be IOThread). Are there any better ways? Thanks.
On 2016/06/28 22:34:07, tbansal1 wrote: > On 2016/06/28 11:11:31, pauljensen wrote: > > Please expose through net::GetWifiSSID(), not by adding a new API. > > Can you suggest a good way to reuse the WiFiDelegate code in NCNAutoDetect. > > One way is to move it to a separate class, convert it into a static class, and > then > let net::GetWiFiSSID() use it too through JNI bindings. > Other way is to keep it as non-static class, but then I am not sure who should > own it (may be IOThread). > > Are there any better ways? Thanks. Move the code to AndroidNetworkLibrary.java
Description was changed from ========== Plumb WiFi SSID from NCN to native Also, record in NQE how frequently the network ID (WiFi SSID or MCC/MNC information) was available on Android. BUG=620529 ========== to ========== Obtain WiFi SSID on Android using Android APIs Also, record in NQE how frequently the network ID (WiFi SSID or MCC/MNC information) was available on Android. BUG=620529 ==========
pauljensen: ptal. thanks.
Please move the NQE changes into another CL, then it lgtm.
Removed NQE specific changes.
The CQ bit was checked by tbansal@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kundaji@chromium.org, pauljensen@chromium.org Link to the patchset: https://codereview.chromium.org/2081493002/#ps120001 (title: "Remove NQE changes from this CL")
Description was changed from ========== Obtain WiFi SSID on Android using Android APIs Also, record in NQE how frequently the network ID (WiFi SSID or MCC/MNC information) was available on Android. BUG=620529 ========== to ========== Obtain WiFi SSID on Android using Android APIs BUG=620529 ==========
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
Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
Patchset #7 (id:140001) has been deleted
The CQ bit was checked by tbansal@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pauljensen@chromium.org, kundaji@chromium.org Link to the patchset: https://codereview.chromium.org/2081493002/#ps160001 (title: "Fix tests")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Obtain WiFi SSID on Android using Android APIs BUG=620529 ========== to ========== Obtain WiFi SSID on Android using Android APIs BUG=620529 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:160001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Obtain WiFi SSID on Android using Android APIs BUG=620529 ========== to ========== Obtain WiFi SSID on Android using Android APIs BUG=620529 Committed: https://crrev.com/c5a1c91d91800672965832bbd62838cd16a022fe Cr-Commit-Position: refs/heads/master@{#403549} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/c5a1c91d91800672965832bbd62838cd16a022fe Cr-Commit-Position: refs/heads/master@{#403549} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
