Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
86123003:
Use WiFi.Frequency property to set desired band for networkingPrivateApi.StartConnect() (Closed)
|
Created:
7 years ago by mef Modified:
7 years ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionUse WiFi.Frequency property to set desired band for networkingPrivateApi.StartConnect().
BUG=267667
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=238540
Patch Set 1 #Patch Set 2 : Separated move of GetVisibleNetworks filtering into https://codereview.chromium.org/88653002 #
Total comments: 12
Patch Set 3 : Address Steven's comment. #Patch Set 4 : Address Toni's comments. #
Total comments: 12
Patch Set 5 : Address Toni's comments. #Patch Set 6 : Use VLOG instead of cout. #
Total comments: 3
Patch Set 7 : Sync up to r238411 #Patch Set 8 : Address Antonio's comment. #Messages
Total messages: 23 (0 generated)
Hi guys, please take a look: Toni - overall. Steven - chrome/browser/extensions/api/networking_private/*, use of SetProperties to express desired band. Antonio - wifi_service_win (adopted from https://codereview.chromium.org/32193015). Chris - FYI, move GetVisibleNetworks network type filtering to WiFiService interface (adopted from https://codereview.chromium.org/30753002/). thanks, -m
Sorry to ask this, but could you separate out the GetVisibleNetworks() changes from the WiFi.Frequency property change? That will make the review easier and also help down the road (bugs, blame, etc). Thanks!
On 2013/11/25 23:43:23, stevenjb wrote: > Sorry to ask this, but could you separate out the GetVisibleNetworks() changes > from the WiFi.Frequency property change? That will make the review easier and > also help down the road (bugs, blame, etc). Thanks! Sure thing. I've separated GetVisibleNetworks() changes into https://codereview.chromium.org/88653002. Remains here: Toni - overall. Steven - Use of SetProperties to express desired band. Antonio - Selection of BSS based on desired band (adopted from https://codereview.chromium.org/32193015). thanks, -m
https://codereview.chromium.org/86123003/diff/10001/components/wifi/wifi_serv... File components/wifi/wifi_service_win.cc (right): https://codereview.chromium.org/86123003/diff/10001/components/wifi/wifi_serv... components/wifi/wifi_service_win.cc:347: DictionaryValue temporary_network_properties_; How about 'connect_properties_' to better describe what these properties are used for. Also, document that these properties persist unless changed. https://codereview.chromium.org/86123003/diff/10001/components/wifi/wifi_serv... components/wifi/wifi_service_win.cc:430: DCHECK(properties->HasKey(onc::network_type::kWiFi)); This should set |error| instead of CHECK https://codereview.chromium.org/86123003/diff/10001/components/wifi/wifi_serv... components/wifi/wifi_service_win.cc:1056: WiFiService::Frequency WiFiServiceImpl::GetDesiredFrequency( GetConnectFrequency or GetFrequencyForConnect? https://codereview.chromium.org/86123003/diff/10001/components/wifi/wifi_serv... components/wifi/wifi_service_win.cc:1064: &properties) && nit: OK (and a little clearer imho) to combine these two lines.
Steven, thanks! PTAL. https://codereview.chromium.org/86123003/diff/10001/components/wifi/wifi_serv... File components/wifi/wifi_service_win.cc (right): https://codereview.chromium.org/86123003/diff/10001/components/wifi/wifi_serv... components/wifi/wifi_service_win.cc:347: DictionaryValue temporary_network_properties_; On 2013/11/26 18:00:34, stevenjb wrote: > How about 'connect_properties_' to better describe what these properties are > used for. Also, document that these properties persist unless changed. Done. https://codereview.chromium.org/86123003/diff/10001/components/wifi/wifi_serv... components/wifi/wifi_service_win.cc:430: DCHECK(properties->HasKey(onc::network_type::kWiFi)); On 2013/11/26 18:00:34, stevenjb wrote: > This should set |error| instead of CHECK Done. https://codereview.chromium.org/86123003/diff/10001/components/wifi/wifi_serv... components/wifi/wifi_service_win.cc:1056: WiFiService::Frequency WiFiServiceImpl::GetDesiredFrequency( On 2013/11/26 18:00:34, stevenjb wrote: > GetConnectFrequency or GetFrequencyForConnect? > Done. https://codereview.chromium.org/86123003/diff/10001/components/wifi/wifi_serv... components/wifi/wifi_service_win.cc:1064: &properties) && On 2013/11/26 18:00:34, stevenjb wrote: > nit: OK (and a little clearer imho) to combine these two lines. Done.
https://codereview.chromium.org/86123003/diff/10001/components/wifi/wifi_serv... File components/wifi/wifi_service_win.cc (right): https://codereview.chromium.org/86123003/diff/10001/components/wifi/wifi_serv... components/wifi/wifi_service_win.cc:469: // Connect only if network |network_guid| is not connected already. is |network_guid| unique for (ssid, frequency) pair? if not, this will not handle the case where we are connected to 2.4G band and want to connect to the 5G band (for the same ssid). https://codereview.chromium.org/86123003/diff/10001/components/wifi/wifi_serv... components/wifi/wifi_service_win.cc:1066: wifi->GetInteger(onc::wifi::kFrequency, &frequency)) { should frequency be normalized (here or in SetProperties)?
lgtm
lgtm LGTM
Thanks guys, PTAL. https://codereview.chromium.org/86123003/diff/10001/components/wifi/wifi_serv... File components/wifi/wifi_service_win.cc (right): https://codereview.chromium.org/86123003/diff/10001/components/wifi/wifi_serv... components/wifi/wifi_service_win.cc:469: // Connect only if network |network_guid| is not connected already. On 2013/11/26 19:02:21, tbarzic wrote: > is |network_guid| unique for (ssid, frequency) pair? > if not, this will not handle the case where we are connected to 2.4G band and > want to connect to the 5G band (for the same ssid). Good point, done. Currently connected band is not readily available, but I think I've figured it out. https://codereview.chromium.org/86123003/diff/10001/components/wifi/wifi_serv... components/wifi/wifi_service_win.cc:1066: wifi->GetInteger(onc::wifi::kFrequency, &frequency)) { On 2013/11/26 19:02:21, tbarzic wrote: > should frequency be normalized (here or in SetProperties)? Done.
lgtm, but the new patchset should probably be re-reviewed by afontan https://codereview.chromium.org/86123003/diff/50001/components/wifi/wifi_serv... File components/wifi/wifi_service_win.cc (right): https://codereview.chromium.org/86123003/diff/50001/components/wifi/wifi_serv... components/wifi/wifi_service_win.cc:311: Frequency GetFrequencyForConnect(const std::string& network_guid) const; nit: how about GetFrequencyToConnect https://codereview.chromium.org/86123003/diff/50001/components/wifi/wifi_serv... components/wifi/wifi_service_win.cc:1119: wlan_connection_attributes->wlanAssociationAttributes; indent += 2 https://codereview.chromium.org/86123003/diff/50001/components/wifi/wifi_serv... components/wifi/wifi_service_win.cc:1120: // Try to fined connected frequency based on bss. s/fined/find/ https://codereview.chromium.org/86123003/diff/50001/components/wifi/wifi_serv... components/wifi/wifi_service_win.cc:1146: // clean up nit: should be "Clean up." https://codereview.chromium.org/86123003/diff/50001/components/wifi/wifi_test.cc File components/wifi/wifi_test.cc (right): https://codereview.chromium.org/86123003/diff/50001/components/wifi/wifi_test... components/wifi/wifi_test.cc:6: #include <iostream> you should not have to use stdio/iostream. Isn't LOG macro from base/logging sufficient for usage here?
Thanks, Toni! Antonio, could you give a once-over to WiFiServiceImpl::GetConnectedFrequency? thanks and happy Thanksgiving! -m https://codereview.chromium.org/86123003/diff/50001/components/wifi/wifi_serv... File components/wifi/wifi_service_win.cc (right): https://codereview.chromium.org/86123003/diff/50001/components/wifi/wifi_serv... components/wifi/wifi_service_win.cc:311: Frequency GetFrequencyForConnect(const std::string& network_guid) const; On 2013/11/27 19:59:57, tbarzic wrote: > nit: how about GetFrequencyToConnect Done. https://codereview.chromium.org/86123003/diff/50001/components/wifi/wifi_serv... components/wifi/wifi_service_win.cc:1119: wlan_connection_attributes->wlanAssociationAttributes; On 2013/11/27 19:59:57, tbarzic wrote: > indent += 2 Done. https://codereview.chromium.org/86123003/diff/50001/components/wifi/wifi_serv... components/wifi/wifi_service_win.cc:1120: // Try to fined connected frequency based on bss. On 2013/11/27 19:59:57, tbarzic wrote: > s/fined/find/ Done. https://codereview.chromium.org/86123003/diff/50001/components/wifi/wifi_serv... components/wifi/wifi_service_win.cc:1146: // clean up On 2013/11/27 19:59:57, tbarzic wrote: > nit: should be "Clean up." Done. https://codereview.chromium.org/86123003/diff/50001/components/wifi/wifi_test.cc File components/wifi/wifi_test.cc (right): https://codereview.chromium.org/86123003/diff/50001/components/wifi/wifi_test... components/wifi/wifi_test.cc:6: #include <iostream> On 2013/11/27 19:59:57, tbarzic wrote: > you should not have to use stdio/iostream. Isn't LOG macro from base/logging > sufficient for usage here? Is there strong preference / suggestion of one vs another? It seems that presubmit checks now discourage LOG in favor of VLOG, but that is probably requiring special command line parameter...
https://codereview.chromium.org/86123003/diff/50001/components/wifi/wifi_test.cc File components/wifi/wifi_test.cc (right): https://codereview.chromium.org/86123003/diff/50001/components/wifi/wifi_test... components/wifi/wifi_test.cc:6: #include <iostream> On 2013/11/27 21:23:57, mef wrote: > On 2013/11/27 19:59:57, tbarzic wrote: > > you should not have to use stdio/iostream. Isn't LOG macro from base/logging > > sufficient for usage here? > Is there strong preference / suggestion of one vs another? It seems that > presubmit checks now discourage LOG in favor of VLOG, but that is probably > requiring special command line parameter... Yes, there is a strong preference for using VLOG/LOG over std::cout.
Sounds good. https://codereview.chromium.org/86123003/diff/50001/components/wifi/wifi_test.cc File components/wifi/wifi_test.cc (right): https://codereview.chromium.org/86123003/diff/50001/components/wifi/wifi_test... components/wifi/wifi_test.cc:6: #include <iostream> On 2013/12/02 19:05:56, tbarzic wrote: > On 2013/11/27 21:23:57, mef wrote: > > On 2013/11/27 19:59:57, tbarzic wrote: > > > you should not have to use stdio/iostream. Isn't LOG macro from base/logging > > > sufficient for usage here? > > Is there strong preference / suggestion of one vs another? It seems that > > presubmit checks now discourage LOG in favor of VLOG, but that is probably > > requiring special command line parameter... > > Yes, there is a strong preference for using VLOG/LOG over std::cout. Done.
Looks good, just a comment on a possible optimization. https://codereview.chromium.org/86123003/diff/90001/components/wifi/wifi_serv... File components/wifi/wifi_service_win.cc (right): https://codereview.chromium.org/86123003/diff/90001/components/wifi/wifi_serv... components/wifi/wifi_service_win.cc:1125: NULL, NULL means no filtering but if you pass the DOT11_SSID of the connected_wlan I think the list should be filtered.
Antonio, thanks! https://codereview.chromium.org/86123003/diff/90001/components/wifi/wifi_serv... File components/wifi/wifi_service_win.cc (right): https://codereview.chromium.org/86123003/diff/90001/components/wifi/wifi_serv... components/wifi/wifi_service_win.cc:1125: NULL, On 2013/12/03 01:32:34, afontan wrote: > NULL means no filtering but if you pass the DOT11_SSID of the connected_wlan I > think the list should be filtered. Theoretically yes, but practically on my machine it returns empty bss list.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mef@chromium.org/86123003/90001
Failed to apply patch for components/wifi/wifi_test.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file components/wifi/wifi_test.cc
Hunk #4 FAILED at 113.
1 out of 7 hunks FAILED -- saving rejects to file
components/wifi/wifi_test.cc.rej
Patch: components/wifi/wifi_test.cc
Index: components/wifi/wifi_test.cc
diff --git a/components/wifi/wifi_test.cc b/components/wifi/wifi_test.cc
index
8fd0aaf9deaee427573166f251d95e046770b9f7..5d675a4f352cf97e79203720b993032d968a7518
100644
--- a/components/wifi/wifi_test.cc
+++ b/components/wifi/wifi_test.cc
@@ -3,7 +3,6 @@
// found in the LICENSE file.
#include <stdio.h>
-#include <iostream>
#include <string>
#include "base/at_exit.h"
@@ -65,15 +64,14 @@ class WiFiTest {
WiFiTest::Result WiFiTest::Main(int argc, const char* argv[]) {
if (!ParseCommandLine(argc, argv)) {
- fprintf(stderr,
- "usage: %s [--list]"
- " [--get_properties]"
- " [--connect]"
- " [--disconnect]"
- " [--network_guid=<network_guid>]"
- " [--frequency=0|2400|5000]"
- " [<network_guid>]\n",
- argv[0]);
+ VLOG(0) << "Usage: " << argv[0] <<
+ " [--list]"
+ " [--get_properties]"
+ " [--connect]"
+ " [--disconnect]"
+ " [--network_guid=<network_guid>]"
+ " [--frequency=0|2400|5000]"
+ " [<network_guid>]\n";
return RESULT_WRONG_USAGE;
}
@@ -88,6 +86,8 @@ bool WiFiTest::ParseCommandLine(int argc, const char* argv[])
{
const CommandLine& parsed_command_line = *CommandLine::ForCurrentProcess();
std::string network_guid =
parsed_command_line.GetSwitchValueASCII("network_guid");
+ std::string frequency =
+ parsed_command_line.GetSwitchValueASCII("frequency");
if (parsed_command_line.GetArgs().size() == 1) {
#if defined(OS_WIN)
@@ -113,7 +113,7 @@ bool WiFiTest::ParseCommandLine(int argc, const char*
argv[]) {
if (parsed_command_line.HasSwitch("list")) {
ListValue network_list;
wifi_service->GetVisibleNetworks(&network_list);
- std::cout << network_list;
+ VLOG(0) << network_list;
return true;
}
@@ -122,16 +122,35 @@ bool WiFiTest::ParseCommandLine(int argc, const char*
argv[]) {
DictionaryValue properties;
std::string error;
wifi_service->GetProperties(network_guid, &properties, &error);
- std::cout << error << ":\n" << properties;
+ VLOG(0) << error << ":\n" << properties;
return true;
}
}
+ // Optional properties (frequency, password) to use for connect.
+ scoped_ptr<DictionaryValue> connect_properties(new DictionaryValue());
+
+ if (parsed_command_line.HasSwitch("frequency")) {
+ int value = 0;
+ if (!network_guid.empty() &&
+ !frequency.empty() &&
+ base::StringToInt(frequency, &value)) {
+ connect_properties->SetInteger("WiFi.Frequency", value);
+ // fall through to connect.
+ }
+ }
+
if (parsed_command_line.HasSwitch("connect")) {
if (network_guid.length() > 0) {
std::string error;
+ if (!connect_properties->empty()) {
+ VLOG(0) << "Using connect properties: " << *connect_properties;
+ wifi_service->SetProperties(network_guid,
+ connect_properties.Pass(),
+ &error);
+ }
wifi_service->StartConnect(network_guid, &error);
- std::cout << error;
+ VLOG(0) << error;
return true;
}
}
@@ -140,7 +159,7 @@ bool WiFiTest::ParseCommandLine(int argc, const char*
argv[]) {
if (network_guid.length() > 0) {
std::string error;
wifi_service->StartDisconnect(network_guid, &error);
- std::cout << error;
+ VLOG(0) << error;
return true;
}
}
@@ -151,6 +170,13 @@ bool WiFiTest::ParseCommandLine(int argc, const char*
argv[]) {
} // namespace wifi
int main(int argc, const char* argv[]) {
+ base::AtExitManager at_exit_manager;
+ CommandLine::Init(argc, argv);
+ const CommandLine& command_line = *CommandLine::ForCurrentProcess();
+ logging::LoggingSettings settings;
+ settings.logging_dest = logging::LOG_TO_SYSTEM_DEBUG_LOG;
+ logging::InitLogging(settings);
+
wifi::WiFiTest wifi_test;
return wifi_test.Main(argc, argv);
}
Antonio, thanks for suggestion! https://codereview.chromium.org/86123003/diff/90001/components/wifi/wifi_serv... File components/wifi/wifi_service_win.cc (right): https://codereview.chromium.org/86123003/diff/90001/components/wifi/wifi_serv... components/wifi/wifi_service_win.cc:1125: NULL, On 2013/12/03 15:00:25, mef wrote: > On 2013/12/03 01:32:34, afontan wrote: > > NULL means no filtering but if you pass the DOT11_SSID of the connected_wlan I > > think the list should be filtered. > > Theoretically yes, but practically on my machine it returns empty bss list. Done. Specifying correct dot11BssType has fixed the empty bss list problem.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mef@chromium.org/86123003/130001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mef@chromium.org/86123003/130001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mef@chromium.org/86123003/130001
Message was sent while issue was closed.
Change committed as 238540 |
