Issue 27722003: Windows-specific implementation of Networking Private API.

mef

Hi guys, I've split off Windows-specific implementation of WiFi Service (using WLANAPI.LIB) from the basic ...

7 years, 2 months ago (2013-10-17 19:27:50 UTC) #1

afontan

On 2013/10/17 19:27:50, mef wrote: > Hi guys, > > I've split off Windows-specific implementation ...

7 years, 2 months ago (2013-10-19 15:20:26 UTC) #2

afontan

On 2013/10/19 15:20:26, afontan wrote: > On 2013/10/17 19:27:50, mef wrote: > > Hi guys, ...

7 years, 2 months ago (2013-10-19 15:38:32 UTC) #3

Jói

Cool stuff. Lots of comments but mostly minor nits and questions. I didn't really try ...

7 years, 2 months ago (2013-10-19 21:14:44 UTC) #4

mef

Hi Jói, thanks for your comments, PTAL! https://codereview.chromium.org/27722003/diff/5001/chrome/chrome_exe.gypi File chrome/chrome_exe.gypi (right): https://codereview.chromium.org/27722003/diff/5001/chrome/chrome_exe.gypi#newcode498 chrome/chrome_exe.gypi:498: 'wlanapi.dll', On ...

7 years, 2 months ago (2013-10-21 15:37:15 UTC) #5

Jói

https://codereview.chromium.org/27722003/diff/5001/chrome/chrome_exe.gypi File chrome/chrome_exe.gypi (right): https://codereview.chromium.org/27722003/diff/5001/chrome/chrome_exe.gypi#newcode498 chrome/chrome_exe.gypi:498: 'wlanapi.dll', On 2013/10/21 15:37:16, mef wrote: > On 2013/10/19 ...

7 years, 2 months ago (2013-10-21 16:44:02 UTC) #6

https://codereview.chromium.org/27722003/diff/5001/chrome/chrome_exe.gypi
File chrome/chrome_exe.gypi (right):

https://codereview.chromium.org/27722003/diff/5001/chrome/chrome_exe.gypi#new...
chrome/chrome_exe.gypi:498: 'wlanapi.dll',
On 2013/10/21 15:37:16, mef wrote:
> On 2013/10/19 21:14:45, Jói wrote:
> > Are you sure you need this? Is your code linked into chrome.exe?
> Yes, the code is linked into chrome.exe, but used only of extension api. 
> So, instead of explicit LoadLibrary I've added it to Delay Load list.

Most code is only linked into the chrome_dll target. Are you sure this gets
linked into the EXE? If you are, then this is fine.

https://codereview.chromium.org/27722003/diff/5001/chrome/utility/wifi/wifi_s...
File chrome/utility/wifi/wifi_service_win.cc (right):

https://codereview.chromium.org/27722003/diff/5001/chrome/utility/wifi/wifi_s...
chrome/utility/wifi/wifi_service_win.cc:19: namespace wifi {
On 2013/10/21 15:37:16, mef wrote:
> On 2013/10/19 21:14:45, Jói wrote:
> > I believe in the src/chrome folder we're mostly getting rid of namespaces.
> That
> > and the class names are already prefixed WiFi.
> > 
> > OTOH, I wonder if this code (I mean all of the chrome/utility/wifi code)
> should
> > live in src/chrome or whether it is separate enough from src/chrome (and
> > potentially reusable by other top-level targets such as Android WebView) to
> live
> > in src/components. See the following:
> > 
> > http://www.chromium.org/developers/design-documents/browser-components
> >
>
http://www.chromium.org/developers/design-documents/browser-components/cookbook
> Interesting. I'll check the component idea. Per discussion with cbentzel@
there
> is no immediate plans to make this code reusable anywhere but in
> networkingPrivate API. I've modeled utility/wifi/ after
> utility/media_galleries/, which use namespaces.

If there are no plans to use it other than for extension APIs, then there is no
need to move it to src/components. I would however recommend adding a
restrictive DEPS file to help ensure that your code doesn't start depending on
"everything" in src/chrome (i.e. it looks like it can depend only on its own
directlry under src/chrome and nothing else except lower layers).

I think you probably don't want a namespace if it's staying under src/chrome.

https://codereview.chromium.org/27722003/diff/5001/chrome/utility/wifi/wifi_s...
chrome/utility/wifi/wifi_service_win.cc:217: const NetworkPropertiesCallback&
callback,
On 2013/10/21 15:37:16, mef wrote:
> On 2013/10/19 21:14:45, Jói wrote:
> > I'm assuming the interface here and other places is designed using callbacks
> > instead of just returning values because on some other platforms you can't
> > return synchronously? If this is not the case we should discuss.
> Yes, AFAIR Mac OS X WiFiFoundation uses callbacks interface, but I need to
> double check.

OK, fine by me, just checking.

https://codereview.chromium.org/27722003/diff/5001/chrome/utility/wifi/wifi_s...
chrome/utility/wifi/wifi_service_win.cc:288: callback.Run(network_guid);
On 2013/10/21 15:37:16, mef wrote:
> On 2013/10/19 21:14:45, Jói wrote:
> > Just checking: Are you sure you want to run this before NotifyNetworkChanged
> and
> > before you WaitForNetworkConnect?
> I think so. Logically caller is start the Connect, and callback tells that
that
> start is successful, and afterwards all network notifications are fired.

OK.

https://codereview.chromium.org/27722003/diff/5001/chrome/utility/wifi/wifi_s...
chrome/utility/wifi/wifi_service_win.cc:361: }
On 2013/10/21 15:37:16, mef wrote:
> On 2013/10/19 21:14:45, Jói wrote:
> > I would add a default: case that DCHECKs.
> I can't, there are more notification types that we don't handle.

If you get one of the other notifications, then your cast to
PWLAN_CONNECTION_NOTIFICATION_DATA is invalid. I guess it's "safe" in that you
won't access it if it isn't one of these types, but I think it'd be better to
return early (before casting) if it isn't one of the types you handle. That
would imply another switch statement before the cast, I guess.

https://codereview.chromium.org/27722003/diff/5001/chrome/utility/wifi/wifi_s...
chrome/utility/wifi/wifi_service_win.cc:366: DWORD error =
GetVisibleNetworkList(&networks);
On 2013/10/21 15:37:16, mef wrote:
> On 2013/10/19 21:14:45, Jói wrote:
> > Why do you need to do this before notifying that the list has changed, and
why
> > do you notify only on success? A comment might be helpful.
> 
> Done. Added a comment. Not sure what would a good action be if
> GetVisibleNetworkList returns an error. DLOG? DCHECK?

DCHECK seems right if you expect this to always succeed.

https://codereview.chromium.org/27722003/diff/5001/chrome/utility/wifi/wifi_s...
chrome/utility/wifi/wifi_service_win.cc:385:
base::MessageLoop::current()->PostDelayedTask(
On 2013/10/21 15:37:16, mef wrote:
> On 2013/10/19 21:14:45, Jói wrote:
> > Why is this delayed if resetting DHCP was successful?
> I've added comment, but would love to change it to something more
deterministic
> if possible (is there some notification that I can subscribe to?).

I'm not aware of a notification you could subscribe to. I guess you could check
the adapter in the delayed task to see if it has an IP address, if not, delay
more, up to some maximum.

https://codereview.chromium.org/27722003/diff/5001/chrome/utility/wifi/wifi_s...
chrome/utility/wifi/wifi_service_win.cc:625: if (client_ == NULL) {
On 2013/10/21 15:37:16, mef wrote:
> On 2013/10/19 21:14:45, Jói wrote:
> > Should there be a NOTREACHED() here to help you catch incorrect usage in
debug
> > builds?
> 
> Done.

You still need a NOTREACHED() in EnsureInitialized when client_ is NULL.

https://codereview.chromium.org/27722003/diff/5001/chrome/utility/wifi/wifi_s...
chrome/utility/wifi/wifi_service_win.cc:713: error = ::WlanConnect(client_,
&interface_guid_, &wlan_params, NULL);
On 2013/10/21 15:37:16, mef wrote:
> On 2013/10/19 21:14:45, Jói wrote:
> > Note that per
> >
>
http://msdn.microsoft.com/en-us/library/windows/desktop/ms706844%2528v=vs.85%...,
> > only the wlan_connection_mode_profile is supported on XP SP3 and older. Not
> 100%
> > clear whether we already dropped support for XP so thought I'd mention it.
> Yes. This and unsupported WlanGetNetworkBssList function break XP at this
point.

This needs to be fixed before check-in. According to
https://support.google.com/chrome/answer/95411?hl=en we support XP SP2 and
newer.

For the dot11_BSS_type_infrastructure constant, it is probably just a matter of
not calling the function with that constant.

For the WlanGetNetworkBssList function though, since you are delay-loading a DLL
with the assumption that the function is there, this will break on old enough
versions of XP. The way to fix it is to use explicit LoadLibrary and do your own
conditional function binding. There are existing examples of this in the code I
think; search for GetProcAddress and you should find them.

https://codereview.chromium.org/27722003/diff/5001/chrome/utility/wifi/wifi_t...
File chrome/utility/wifi/wifi_test.cc (right):

https://codereview.chromium.org/27722003/diff/5001/chrome/utility/wifi/wifi_t...
chrome/utility/wifi/wifi_test.cc:15: #if defined(OS_MACOSX)
On 2013/10/21 15:37:16, mef wrote:
> On 2013/10/19 21:14:45, Jói wrote:
> > It seems more typical to move platform-specific includes to start one blank
> line
> > after the main set of includes.
> I started with that, but presubmit checks were complaining about wrong include
> order.

They won't complain if you leave a blank line before the #if.

https://codereview.chromium.org/27722003/diff/72001/chrome/utility/wifi/wifi_...
File chrome/utility/wifi/wifi_service_win.cc (right):

https://codereview.chromium.org/27722003/diff/72001/chrome/utility/wifi/wifi_...
chrome/utility/wifi/wifi_service_win.cc:500: // Remember first interface just in
case if none are connected.
nit: in case if none -> in case none

afontan

Hi Misha, please see my comments. I will be happy to discuss about the multiple ...

7 years, 2 months ago (2013-10-21 16:55:22 UTC) #7

Hi Misha,

please see my comments. I will be happy to discuss about the multiple profiles
for the same SSID if the scenario is not clear.

Thanks,

Antonio

https://codereview.chromium.org/27722003/diff/5001/chrome/utility/wifi/wifi_s...
File chrome/utility/wifi/wifi_service_win.cc (right):

https://codereview.chromium.org/27722003/diff/5001/chrome/utility/wifi/wifi_s...
chrome/utility/wifi/wifi_service_win.cc:133: std::string GuidFromSsid(const
DOT11_SSID& dot11Ssid) const {
dot11Ssid is not a GUID and it is not unique. You can have multiple profiles for
the same SSID (a typical scenario is for example 2.4 and 5 GHz).

https://codereview.chromium.org/27722003/diff/5001/chrome/utility/wifi/wifi_s...
chrome/utility/wifi/wifi_service_win.cc:144: std::string GuidFromWlan(const
WLAN_AVAILABLE_NETWORK& wlan) const {
ditto.

https://codereview.chromium.org/27722003/diff/5001/chrome/utility/wifi/wifi_s...
chrome/utility/wifi/wifi_service_win.cc:260: SortNetworks(&network_list);
I would have expected the API to retrieve the networks asynchronously and then
invoke the callback.

https://codereview.chromium.org/27722003/diff/5001/chrome/utility/wifi/wifi_s...
chrome/utility/wifi/wifi_service_win.cc:286: error = Connect(network_guid);
One thing we were asked is to avoid the network location wizard to pop up when
we change networks. There is a registry key that controls that
(Software\Microsoft\Windows NT\CurrentVersion\Network\NwCategoryWizard -> Show).
This is the same as if the user had clicked do not remind me.

Ideally you would disable at the beginning and restore it when you are done. In
this way the pop up does not show while you are automatically changing networks.
Unfortunately I do not know of an API that can disable the pop up temporarily.

https://codereview.chromium.org/27722003/diff/5001/chrome/utility/wifi/wifi_s...
chrome/utility/wifi/wifi_service_win.cc:459: 
I assume using WLAN_API_VERSION is fine (given that we only use 1.0 methods) but
in the Chromecast setup we use the same version 1.0 for all windows OS (although
we only support win 7 and 8). This code uses 2.0 for win 7/8 but you then you do
not use the negotiated version to know what API is being used.

https://codereview.chromium.org/27722003/diff/5001/chrome/utility/wifi/wifi_s...
chrome/utility/wifi/wifi_service_win.cc:473: interface_guid_ =
pIntfList->InterfaceInfo[itf].InterfaceGuid;
What if we want to setup with other wifi interfaces because they already have
the profile for the network requested (or if the first failed and you want to
try to setup with the second)? It is not common having more than one wifi but
could happen. Using only the first interface or the one connected looks more
like a decision that could be taken at application level. In the current
chromecast setup I think we try to find the interface that has the specific
profile we look for.

https://codereview.chromium.org/27722003/diff/5001/chrome/utility/wifi/wifi_s...
chrome/utility/wifi/wifi_service_win.cc:674:
WlanGetAvailableNetworkList(client_, &interface_guid_, 0, NULL, &pVList);
Flags is currently 0 but you should include hidden profiles.

https://codereview.chromium.org/27722003/diff/5001/chrome/utility/wifi/wifi_s...
chrome/utility/wifi/wifi_service_win.cc:694: DWORD
WiFiServiceImpl::Connect(const std::string& network_guid) {
One scenario we had to cover based on dogfooding was to connect to a network
with a SSID using the 5GHz profile (assuming the same SSID supports both 2.4 and
5 GHz), then try to setup Chromecast.

If we are unable, it means that the user has isolation between 2.4 and 5GHz. At
that point we need to connect to 2.4GHz and if we are successful, still provide
a warning to the user that he may not be able to use Chromecast properly (for
example if the laptop decides to use 5GHz).

For this scenario we need to be able to specify the specific WLAN_BSS_ENTRY.

https://codereview.chromium.org/27722003/diff/5001/chrome/utility/wifi/wifi_s...
chrome/utility/wifi/wifi_service_win.cc:740: client_, &interface_guid_,
profile_name.c_str(), NULL, 0, true, NULL);
On 2013/10/19 21:14:45, Jói wrote:
> Should the profile perhaps be saved per-user? See WLAN_PROFILE_USER for the
> dwFlags parameter in
>
http://msdn.microsoft.com/en-us/library/windows/desktop/ms706778%28v=vs.85%29...

In the current Chromecast implementation we save it for all users but I agree
that seems more reasonable to only save it for the current user.

mef

+tbarzic for extension insights. Hi guys, thanks for your comments! I've addressed some of them ...

7 years, 2 months ago (2013-10-21 19:41:59 UTC) #8

+tbarzic for extension insights.

Hi guys, thanks for your comments! I've addressed some of them and have
follow-up questions about others.

thanks,
-m

https://codereview.chromium.org/27722003/diff/5001/chrome/chrome_exe.gypi
File chrome/chrome_exe.gypi (right):

https://codereview.chromium.org/27722003/diff/5001/chrome/chrome_exe.gypi#new...
chrome/chrome_exe.gypi:498: 'wlanapi.dll',
On 2013/10/21 16:44:03, Jói wrote:
> On 2013/10/21 15:37:16, mef wrote:
> > On 2013/10/19 21:14:45, Jói wrote:
> > > Are you sure you need this? Is your code linked into chrome.exe?
> > Yes, the code is linked into chrome.exe, but used only of extension api. 
> > So, instead of explicit LoadLibrary I've added it to Delay Load list.
> 
> Most code is only linked into the chrome_dll target. Are you sure this gets
> linked into the EXE? If you are, then this is fine.

Done. I've missed the point that even in release builds everything goes into
chrome.dll, not chrome.exe.

https://codereview.chromium.org/27722003/diff/5001/chrome/utility/wifi/wifi_s...
File chrome/utility/wifi/wifi_service_win.cc (right):

https://codereview.chromium.org/27722003/diff/5001/chrome/utility/wifi/wifi_s...
chrome/utility/wifi/wifi_service_win.cc:19: namespace wifi {
On 2013/10/21 16:44:03, Jói wrote:
> On 2013/10/21 15:37:16, mef wrote:
> > On 2013/10/19 21:14:45, Jói wrote:
> > > I believe in the src/chrome folder we're mostly getting rid of namespaces.
> > That
> > > and the class names are already prefixed WiFi.
> > > 
> > > OTOH, I wonder if this code (I mean all of the chrome/utility/wifi code)
> > should
> > > live in src/chrome or whether it is separate enough from src/chrome (and
> > > potentially reusable by other top-level targets such as Android WebView)
to
> > live
> > > in src/components. See the following:
> > > 
> > > http://www.chromium.org/developers/design-documents/browser-components
> > >
> >
>
http://www.chromium.org/developers/design-documents/browser-components/cookbook
> > Interesting. I'll check the component idea. Per discussion with cbentzel@
> there
> > is no immediate plans to make this code reusable anywhere but in
> > networkingPrivate API. I've modeled utility/wifi/ after
> > utility/media_galleries/, which use namespaces.
> 
> If there are no plans to use it other than for extension APIs, then there is
no
> need to move it to src/components. I would however recommend adding a
> restrictive DEPS file to help ensure that your code doesn't start depending on
> "everything" in src/chrome (i.e. it looks like it can depend only on its own
> directlry under src/chrome and nothing else except lower layers).
> 
> I think you probably don't want a namespace if it's staying under src/chrome.

Done. If you don't have strong objections I'd keep the namespace - it was
suggested by stevenjb@ and seems consistent with utility/local_discovery,
utility/media_galleries, etc. 
OTO the chrome/networking_private_handler.* is using namespace chrome.

Please let me know.

https://codereview.chromium.org/27722003/diff/5001/chrome/utility/wifi/wifi_s...
chrome/utility/wifi/wifi_service_win.cc:133: std::string GuidFromSsid(const
DOT11_SSID& dot11Ssid) const {
On 2013/10/21 16:55:23, afontan wrote:
> dot11Ssid is not a GUID and it is not unique. You can have multiple profiles
for
> the same SSID (a typical scenario is for example 2.4 and 5 GHz).
I could make it an interface-ssid-bssid combination. How unique do we want them
to be? Right now this api is able to connect to arbitrary unsecured network
(e.g. Chromekey1501, GoogleGuest) or previously connected secured network if
there is a saved profile. What would be a scenario to connect to particular BSS?

https://codereview.chromium.org/27722003/diff/5001/chrome/utility/wifi/wifi_s...
chrome/utility/wifi/wifi_service_win.cc:260: SortNetworks(&network_list);
On 2013/10/21 16:55:23, afontan wrote:
> I would have expected the API to retrieve the networks asynchronously and then
> invoke the callback.
Is it something that could block for considerable time?

https://codereview.chromium.org/27722003/diff/5001/chrome/utility/wifi/wifi_s...
chrome/utility/wifi/wifi_service_win.cc:286: error = Connect(network_guid);
On 2013/10/21 16:55:23, afontan wrote:
> One thing we were asked is to avoid the network location wizard to pop up when
> we change networks. There is a registry key that controls that
> (Software\Microsoft\Windows NT\CurrentVersion\Network\NwCategoryWizard ->
Show).
> This is the same as if the user had clicked do not remind me.
> 
> Ideally you would disable at the beginning and restore it when you are done.
In
> this way the pop up does not show while you are automatically changing
networks.
> Unfortunately I do not know of an API that can disable the pop up temporarily.
Cool, Will do.

https://codereview.chromium.org/27722003/diff/5001/chrome/utility/wifi/wifi_s...
chrome/utility/wifi/wifi_service_win.cc:361: }
On 2013/10/21 16:44:03, Jói wrote:
> On 2013/10/21 15:37:16, mef wrote:
> > On 2013/10/19 21:14:45, Jói wrote:
> > > I would add a default: case that DCHECKs.
> > I can't, there are more notification types that we don't handle.
> 
> If you get one of the other notifications, then your cast to
> PWLAN_CONNECTION_NOTIFICATION_DATA is invalid. I guess it's "safe" in that you
> won't access it if it isn't one of these types, but I think it'd be better to
> return early (before casting) if it isn't one of the types you handle. That
> would imply another switch statement before the cast, I guess.

Done.

https://codereview.chromium.org/27722003/diff/5001/chrome/utility/wifi/wifi_s...
chrome/utility/wifi/wifi_service_win.cc:366: DWORD error =
GetVisibleNetworkList(&networks);
On 2013/10/21 16:44:03, Jói wrote:
> On 2013/10/21 15:37:16, mef wrote:
> > On 2013/10/19 21:14:45, Jói wrote:
> > > Why do you need to do this before notifying that the list has changed, and
> why
> > > do you notify only on success? A comment might be helpful.
> > 
> > Done. Added a comment. Not sure what would a good action be if
> > GetVisibleNetworkList returns an error. DLOG? DCHECK?
> 
> DCHECK seems right if you expect this to always succeed.

Done.

https://codereview.chromium.org/27722003/diff/5001/chrome/utility/wifi/wifi_s...
chrome/utility/wifi/wifi_service_win.cc:385:
base::MessageLoop::current()->PostDelayedTask(
On 2013/10/21 16:44:03, Jói wrote:
> On 2013/10/21 15:37:16, mef wrote:
> > On 2013/10/19 21:14:45, Jói wrote:
> > > Why is this delayed if resetting DHCP was successful?
> > I've added comment, but would love to change it to something more
> deterministic
> > if possible (is there some notification that I can subscribe to?).
> 
> I'm not aware of a notification you could subscribe to. I guess you could
check
> the adapter in the delayed task to see if it has an IP address, if not, delay
> more, up to some maximum.
Sounds good, I'll try to find appropriate API.

https://codereview.chromium.org/27722003/diff/5001/chrome/utility/wifi/wifi_s...
chrome/utility/wifi/wifi_service_win.cc:459: 
On 2013/10/21 16:55:23, afontan wrote:
> I assume using WLAN_API_VERSION is fine (given that we only use 1.0 methods)
but
> in the Chromecast setup we use the same version 1.0 for all windows OS
(although
> we only support win 7 and 8). This code uses 2.0 for win 7/8 but you then you
do
> not use the negotiated version to know what API is being used.
Could you elaborate on suggested change? Also, I've tried Chromecast APP on
Windows XP and it didn't seem to work (at least on my VM). Is this expected?

https://codereview.chromium.org/27722003/diff/5001/chrome/utility/wifi/wifi_s...
chrome/utility/wifi/wifi_service_win.cc:473: interface_guid_ =
pIntfList->InterfaceInfo[itf].InterfaceGuid;
On 2013/10/21 16:55:23, afontan wrote:
> What if we want to setup with other wifi interfaces because they already have
> the profile for the network requested (or if the first failed and you want to
> try to setup with the second)? It is not common having more than one wifi but
> could happen. Using only the first interface or the one connected looks more
> like a decision that could be taken at application level. In the current
> chromecast setup I think we try to find the interface that has the specific
> profile we look for.
Interesting. I suppose this loop could potentially select 5GHz-only adapter and
will be unable to connect to 2.4GHz-only ChromeCast device. Should I use
WlanQueryInterface with 
wlan_intf_opcode_channel_number opcode to select only 2.4 GHz interface
regardless of its connection state?

https://codereview.chromium.org/27722003/diff/5001/chrome/utility/wifi/wifi_s...
chrome/utility/wifi/wifi_service_win.cc:625: if (client_ == NULL) {
On 2013/10/21 16:44:03, Jói wrote:
> On 2013/10/21 15:37:16, mef wrote:
> > On 2013/10/19 21:14:45, Jói wrote:
> > > Should there be a NOTREACHED() here to help you catch incorrect usage in
> debug
> > > builds?
> > 
> > Done.
> 
> You still need a NOTREACHED() in EnsureInitialized when client_ is NULL.
Should I add explicit 'Initialize()' method as currently there isn't one?

https://codereview.chromium.org/27722003/diff/5001/chrome/utility/wifi/wifi_s...
chrome/utility/wifi/wifi_service_win.cc:674:
WlanGetAvailableNetworkList(client_, &interface_guid_, 0, NULL, &pVList);
On 2013/10/21 16:55:23, afontan wrote:
> Flags is currently 0 but you should include hidden profiles.

Done. But why?

https://codereview.chromium.org/27722003/diff/5001/chrome/utility/wifi/wifi_s...
chrome/utility/wifi/wifi_service_win.cc:694: DWORD
WiFiServiceImpl::Connect(const std::string& network_guid) {
On 2013/10/21 16:55:23, afontan wrote:
> One scenario we had to cover based on dogfooding was to connect to a network
> with a SSID using the 5GHz profile (assuming the same SSID supports both 2.4
and
> 5 GHz), then try to setup Chromecast.
> 
> If we are unable, it means that the user has isolation between 2.4 and 5GHz.
At
> that point we need to connect to 2.4GHz and if we are successful, still
provide
> a warning to the user that he may not be able to use Chromecast properly (for
> example if the laptop decides to use 5GHz).
> 
> For this scenario we need to be able to specify the specific WLAN_BSS_ENTRY. 

Interesting. Could you elaborate on this? Currently networkingPrivate API
doesn't 
provide parameters to specify BSSID or encryption key/password. 
Adding tbarzic@ for extension knowledge.

I could make network_guid to correspond to every BSSID, but that would mean that
in the office I get 12 GoogleGuests to choose from.

https://codereview.chromium.org/27722003/diff/5001/chrome/utility/wifi/wifi_s...
chrome/utility/wifi/wifi_service_win.cc:713: error = ::WlanConnect(client_,
&interface_guid_, &wlan_params, NULL);
On 2013/10/21 16:44:03, Jói wrote:
> On 2013/10/21 15:37:16, mef wrote:
> > On 2013/10/19 21:14:45, Jói wrote:
> > > Note that per
> > >
> >
>
http://msdn.microsoft.com/en-us/library/windows/desktop/ms706844%252528v=vs.8...,
> > > only the wlan_connection_mode_profile is supported on XP SP3 and older.
Not
> > 100%
> > > clear whether we already dropped support for XP so thought I'd mention it.
> > Yes. This and unsupported WlanGetNetworkBssList function break XP at this
> point.
> 
> This needs to be fixed before check-in. According to
> https://support.google.com/chrome/answer/95411?hl=en we support XP SP2 and
> newer.
> 
> For the dot11_BSS_type_infrastructure constant, it is probably just a matter
of
> not calling the function with that constant.
> 
> For the WlanGetNetworkBssList function though, since you are delay-loading a
DLL
> with the assumption that the function is there, this will break on old enough
> versions of XP. The way to fix it is to use explicit LoadLibrary and do your
own
> conditional function binding. There are existing examples of this in the code
I
> think; search for GetProcAddress and you should find them.

Sounds good to be on the safe side, although from
http://msdn.microsoft.com/en-us/library/windows/desktop/ms706735(v=vs.85).aspx
and my experiment on XP SP3 the function is there, it just returns
'ERROR_NOT_SUPPORTED'.

https://codereview.chromium.org/27722003/diff/5001/chrome/utility/wifi/wifi_s...
chrome/utility/wifi/wifi_service_win.cc:740: client_, &interface_guid_,
profile_name.c_str(), NULL, 0, true, NULL);
On 2013/10/21 16:55:23, afontan wrote:
> On 2013/10/19 21:14:45, Jói wrote:
> > Should the profile perhaps be saved per-user? See WLAN_PROFILE_USER for the
> > dwFlags parameter in
> >
>
http://msdn.microsoft.com/en-us/library/windows/desktop/ms706778%2528v=vs.85%...
> 
> In the current Chromecast implementation we save it for all users but I agree
> that seems more reasonable to only save it for the current user.

Done.

tbarzic

https://codereview.chromium.org/27722003/diff/5001/chrome/utility/wifi/wifi_service_win.cc File chrome/utility/wifi/wifi_service_win.cc (right): https://codereview.chromium.org/27722003/diff/5001/chrome/utility/wifi/wifi_service_win.cc#newcode694 chrome/utility/wifi/wifi_service_win.cc:694: DWORD WiFiServiceImpl::Connect(const std::string& network_guid) { On 2013/10/21 19:42:00, mef ...

7 years, 2 months ago (2013-10-21 20:13:29 UTC) #9

afontan

Let me know if the scenarios are still not clear. Today we do support: 1) ...

7 years, 2 months ago (2013-10-21 20:51:01 UTC) #10

Let me know if the scenarios are still not clear.

Today we do support:

1) Hidden networks (that do not have a public SSID)
2) Smart reporting of isolated 5GHz/2.4GHz networks that have been configured
with the same SSID.

https://codereview.chromium.org/27722003/diff/5001/chrome/utility/wifi/wifi_s...
File chrome/utility/wifi/wifi_service_win.cc (right):

https://codereview.chromium.org/27722003/diff/5001/chrome/utility/wifi/wifi_s...
chrome/utility/wifi/wifi_service_win.cc:133: std::string GuidFromSsid(const
DOT11_SSID& dot11Ssid) const {
On 2013/10/21 19:42:00, mef wrote:
> On 2013/10/21 16:55:23, afontan wrote:
> > dot11Ssid is not a GUID and it is not unique. You can have multiple profiles
> for
> > the same SSID (a typical scenario is for example 2.4 and 5 GHz).
> I could make it an interface-ssid-bssid combination. How unique do we want
them
> to be? Right now this api is able to connect to arbitrary unsecured network
> (e.g. Chromekey1501, GoogleGuest) or previously connected secured network if
> there is a saved profile. What would be a scenario to connect to particular
BSS?

Correct. The 5GHz/2.4GHz with same SSID is the scenario that comes to mind.

https://codereview.chromium.org/27722003/diff/5001/chrome/utility/wifi/wifi_s...
chrome/utility/wifi/wifi_service_win.cc:459: 
On 2013/10/21 19:42:00, mef wrote:
> On 2013/10/21 16:55:23, afontan wrote:
> > I assume using WLAN_API_VERSION is fine (given that we only use 1.0 methods)
> but
> > in the Chromecast setup we use the same version 1.0 for all windows OS
> (although
> > we only support win 7 and 8). This code uses 2.0 for win 7/8 but you then
you
> do
> > not use the negotiated version to know what API is being used.
> Could you elaborate on suggested change? Also, I've tried Chromecast APP on
> Windows XP and it didn't seem to work (at least on my VM). Is this expected?

Correct. Today we only support win 7/8. For vista or XP today we send the users
to manual web setup from the install site (where they manually connect to
Chromecast). It would be great if you could do better as Chrome supports
XP/Vista so it would be more consistent but the main issue with supporting XP is
that the wireless drivers out there often do things very inconsistently.

What I meant is that I believe that the WLAN_API_VERSION macro will
automatically expand to 2 in > Vista. The service_version will tell you exactly
the API version that was negotiated. You are not using the API negotiated value
to take decisions, as implicitly you are only using APIs supported in both 1 and
2.

In that case why not just ask for API 1? otherwise if you ever use an API that
does not exist in version 1, you will break XP (unless you consider the value
returned in service_version). So the question is, if you only use the version 1
APIs anyway, should you still request version 2 for win7/8? In the chromecast
setup we ask for API 1 but not sure that is the best option as I am not sure if
there are some behavior changes between 1/2 even for the same set of APIs, if
there is no behavior change it would make sense to request version 1.

http://msdn.microsoft.com/en-us/library/windows/desktop/ms706759(v=vs.85).aspx

Note, I just wanted to point it out so you take the decision with all the
information or can dig deeper to understand what is best in your scenario, it is
a difference with our current implementation and makes sense to understand the
trade-offs.

https://codereview.chromium.org/27722003/diff/5001/chrome/utility/wifi/wifi_s...
chrome/utility/wifi/wifi_service_win.cc:473: interface_guid_ =
pIntfList->InterfaceInfo[itf].InterfaceGuid;
On 2013/10/21 19:42:00, mef wrote:
> On 2013/10/21 16:55:23, afontan wrote:
> > What if we want to setup with other wifi interfaces because they already
have
> > the profile for the network requested (or if the first failed and you want
to
> > try to setup with the second)? It is not common having more than one wifi
but
> > could happen. Using only the first interface or the one connected looks more
> > like a decision that could be taken at application level. In the current
> > chromecast setup I think we try to find the interface that has the specific
> > profile we look for.
> Interesting. I suppose this loop could potentially select 5GHz-only adapter
and
> will be unable to connect to 2.4GHz-only ChromeCast device. Should I use
> WlanQueryInterface with 
> wlan_intf_opcode_channel_number opcode to select only 2.4 GHz interface
> regardless of its connection state?

The issue is that we decided to try to setup first from the 5GHz network as if
it is successful we know the household is good to go. If it fails, then we
connect to the 2.4GHz. If that fails then it is a total failure but if it
succeeds it is just a partial success as the user may experience issues with
devices that connect to 5GHz (macbooks prefer it, Android prefers it too) so we
notify them and send them to a page that explains the isolation configuration in
detail.

We did this because we found that some home routers had this issue where they
named the 5GHz and 2.4GHz with the same SSID but the networks were isolated.
Note that this type of setup procedure is only possible in Windows, MAC and
Android can't control the band they want to connect to.

https://codereview.chromium.org/27722003/diff/5001/chrome/utility/wifi/wifi_s...
chrome/utility/wifi/wifi_service_win.cc:674:
WlanGetAvailableNetworkList(client_, &interface_guid_, 0, NULL, &pVList);
On 2013/10/21 19:42:00, mef wrote:
> On 2013/10/21 16:55:23, afontan wrote:
> > Flags is currently 0 but you should include hidden profiles.
> 
> Done. But why?

Because the user may have configured a hidden nerwork that has no public SSID,
we do support this scenario where they need to type the SSID manually.

https://codereview.chromium.org/27722003/diff/5001/chrome/utility/wifi/wifi_s...
chrome/utility/wifi/wifi_service_win.cc:694: DWORD
WiFiServiceImpl::Connect(const std::string& network_guid) {
On 2013/10/21 19:42:00, mef wrote:
> On 2013/10/21 16:55:23, afontan wrote:
> > One scenario we had to cover based on dogfooding was to connect to a network
> > with a SSID using the 5GHz profile (assuming the same SSID supports both 2.4
> and
> > 5 GHz), then try to setup Chromecast.
> > 
> > If we are unable, it means that the user has isolation between 2.4 and 5GHz.
> At
> > that point we need to connect to 2.4GHz and if we are successful, still
> provide
> > a warning to the user that he may not be able to use Chromecast properly
(for
> > example if the laptop decides to use 5GHz).
> > 
> > For this scenario we need to be able to specify the specific WLAN_BSS_ENTRY.

> 
> Interesting. Could you elaborate on this? Currently networkingPrivate API
> doesn't 
> provide parameters to specify BSSID or encryption key/password. 
> Adding tbarzic@ for extension knowledge.
> 
> I could make network_guid to correspond to every BSSID, but that would mean
that
> in the office I get 12 GoogleGuests to choose from.

We get a list of band filtered Bss entries that contains the 5GHz entries that
have the same SSID or are hidden networks and we use that list of Bss entries to
connect. In this way we will not connect to the 2.4GHz network even if the SSID
matches.

WlanConnect takes a PWLAN_CONNECTION_PARAMETERS that takes a desired bssid list:

http://msdn.microsoft.com/en-us/library/windows/desktop/ms706613(v=vs.85).aspx
http://msdn.microsoft.com/en-us/library/windows/desktop/ms706851(v=vs.85).aspx

I assume you have access to the setup code as I see you have the DHCP
workaround, look for ConnectWithXML in wlanManager.

mef

Hi guys, PTAL, I've addressed all remaining comments except for multi-adapter/multi-band scenario. I will create ...

7 years, 2 months ago (2013-10-22 20:06:01 UTC) #11

Hi guys, PTAL, I've addressed all remaining comments except for
multi-adapter/multi-band scenario. 

I will create another CL with changes to add 'frequency' parameter to
startConnect function.

Antonio, could you also elaborate a little about reasons that prevent ChromeCast
App to work on Vista?

thanks,
-m

https://codereview.chromium.org/27722003/diff/5001/chrome/utility/wifi/wifi_s...
File chrome/utility/wifi/wifi_service_win.cc (right):

https://codereview.chromium.org/27722003/diff/5001/chrome/utility/wifi/wifi_s...
chrome/utility/wifi/wifi_service_win.cc:286: error = Connect(network_guid);
On 2013/10/21 19:42:00, mef wrote:
> On 2013/10/21 16:55:23, afontan wrote:
> > One thing we were asked is to avoid the network location wizard to pop up
when
> > we change networks. There is a registry key that controls that
> > (Software\Microsoft\Windows NT\CurrentVersion\Network\NwCategoryWizard ->
> Show).
> > This is the same as if the user had clicked do not remind me.
> > 
> > Ideally you would disable at the beginning and restore it when you are done.
> In
> > this way the pop up does not show while you are automatically changing
> networks.
> > Unfortunately I do not know of an API that can disable the pop up
temporarily.
> Cool, Will do.

Done.

https://codereview.chromium.org/27722003/diff/5001/chrome/utility/wifi/wifi_s...
chrome/utility/wifi/wifi_service_win.cc:385:
base::MessageLoop::current()->PostDelayedTask(
On 2013/10/21 19:42:00, mef wrote:
> On 2013/10/21 16:44:03, Jói wrote:
> > On 2013/10/21 15:37:16, mef wrote:
> > > On 2013/10/19 21:14:45, Jói wrote:
> > > > Why is this delayed if resetting DHCP was successful?
> > > I've added comment, but would love to change it to something more
> > deterministic
> > > if possible (is there some notification that I can subscribe to?).
> > 
> > I'm not aware of a notification you could subscribe to. I guess you could
> check
> > the adapter in the delayed task to see if it has an IP address, if not,
delay
> > more, up to some maximum.
> Sounds good, I'll try to find appropriate API.

Done. Upon further testing it turned that IP Address is renewed upon return from
IpRenewAddress, and previously observed flakiness which I've attributed to
address renewal delay was actually caused by out-of-band network change
notifications.

https://codereview.chromium.org/27722003/diff/5001/chrome/utility/wifi/wifi_s...
chrome/utility/wifi/wifi_service_win.cc:459: 
On 2013/10/21 20:51:02, afontan wrote:
> On 2013/10/21 19:42:00, mef wrote:
> > On 2013/10/21 16:55:23, afontan wrote:
> > > I assume using WLAN_API_VERSION is fine (given that we only use 1.0
methods)
> > but
> > > in the Chromecast setup we use the same version 1.0 for all windows OS
> > (although
> > > we only support win 7 and 8). This code uses 2.0 for win 7/8 but you then
> you
> > do
> > > not use the negotiated version to know what API is being used.
> > Could you elaborate on suggested change? Also, I've tried Chromecast APP on
> > Windows XP and it didn't seem to work (at least on my VM). Is this expected?
> 
> Correct. Today we only support win 7/8. For vista or XP today we send the
users
> to manual web setup from the install site (where they manually connect to
> Chromecast). It would be great if you could do better as Chrome supports
> XP/Vista so it would be more consistent but the main issue with supporting XP
is
> that the wireless drivers out there often do things very inconsistently.
> 
> What I meant is that I believe that the WLAN_API_VERSION macro will
> automatically expand to 2 in > Vista. The service_version will tell you
exactly
> the API version that was negotiated. You are not using the API negotiated
value
> to take decisions, as implicitly you are only using APIs supported in both 1
and
> 2.
> 
> In that case why not just ask for API 1? otherwise if you ever use an API that
> does not exist in version 1, you will break XP (unless you consider the value
> returned in service_version). So the question is, if you only use the version
1
> APIs anyway, should you still request version 2 for win7/8? In the chromecast
> setup we ask for API 1 but not sure that is the best option as I am not sure
if
> there are some behavior changes between 1/2 even for the same set of APIs, if
> there is no behavior change it would make sense to request version 1.
> 
>
http://msdn.microsoft.com/en-us/library/windows/desktop/ms706759%28v=vs.85%29...
> 
> Note, I just wanted to point it out so you take the decision with all the
> information or can dig deeper to understand what is best in your scenario, it
is
> a difference with our current implementation and makes sense to understand the
> trade-offs.
> 
> 
> 
> 

Done.

https://codereview.chromium.org/27722003/diff/5001/chrome/utility/wifi/wifi_t...
File chrome/utility/wifi/wifi_test.cc (right):

https://codereview.chromium.org/27722003/diff/5001/chrome/utility/wifi/wifi_t...
chrome/utility/wifi/wifi_test.cc:15: #if defined(OS_MACOSX)
On 2013/10/21 16:44:03, Jói wrote:
> On 2013/10/21 15:37:16, mef wrote:
> > On 2013/10/19 21:14:45, Jói wrote:
> > > It seems more typical to move platform-specific includes to start one
blank
> > line
> > > after the main set of includes.
> > I started with that, but presubmit checks were complaining about wrong
include
> > order.
> 
> They won't complain if you leave a blank line before the #if.

Done.

afontan

I do not think there are any reasons other than it was not tested. Antonio ...

7 years, 2 months ago (2013-10-23 05:55:14 UTC) #12

I do not think there are any reasons other than it was not tested.

Antonio
On Oct 22, 2013 1:06 PM, <mef@chromium.org> wrote:

> Hi guys, PTAL, I've addressed all remaining comments except for
> multi-adapter/multi-band scenario.
>
> I will create another CL with changes to add 'frequency' parameter to
> startConnect function.
>
> Antonio, could you also elaborate a little about reasons that prevent
> ChromeCast
> App to work on Vista?
>
> thanks,
> -m
>
>
> https://codereview.chromium.**org/27722003/diff/5001/chrome/**
>
utility/wifi/wifi_service_win.**cc<https://codereview.chromium.org/27722003/diff/5001/chrome/utility/wifi/wifi_service_win.cc>
> File chrome/utility/wifi/wifi_**service_win.cc (right):
>
> https://codereview.chromium.**org/27722003/diff/5001/chrome/**
>
utility/wifi/wifi_service_win.**cc#newcode286<https://codereview.chromium.org/27722003/diff/5001/chrome/utility/wifi/wifi_service_win.cc#newcode286>
> chrome/utility/wifi/wifi_**service_win.cc:286: error =
> Connect(network_guid);
> On 2013/10/21 19:42:00, mef wrote:
>
>> On 2013/10/21 16:55:23, afontan wrote:
>> > One thing we were asked is to avoid the network location wizard to
>>
> pop up when
>
>> > we change networks. There is a registry key that controls that
>> > (Software\Microsoft\Windows
>>
> NT\CurrentVersion\Network\**NwCategoryWizard ->
>
>> Show).
>> > This is the same as if the user had clicked do not remind me.
>> >
>> > Ideally you would disable at the beginning and restore it when you
>>
> are done.
>
>> In
>> > this way the pop up does not show while you are automatically
>>
> changing
>
>> networks.
>> > Unfortunately I do not know of an API that can disable the pop up
>>
> temporarily.
>
>> Cool, Will do.
>>
>
> Done.
>
> https://codereview.chromium.**org/27722003/diff/5001/chrome/**
>
utility/wifi/wifi_service_win.**cc#newcode385<https://codereview.chromium.org/27722003/diff/5001/chrome/utility/wifi/wifi_service_win.cc#newcode385>
> chrome/utility/wifi/wifi_**service_win.cc:385:
> base::MessageLoop::current()->**PostDelayedTask(
> On 2013/10/21 19:42:00, mef wrote:
>
>> On 2013/10/21 16:44:03, Jói wrote:
>> > On 2013/10/21 15:37:16, mef wrote:
>> > > On 2013/10/19 21:14:45, Jói wrote:
>> > > > Why is this delayed if resetting DHCP was successful?
>> > > I've added comment, but would love to change it to something more
>> > deterministic
>> > > if possible (is there some notification that I can subscribe to?).
>> >
>> > I'm not aware of a notification you could subscribe to. I guess you
>>
> could
>
>> check
>> > the adapter in the delayed task to see if it has an IP address, if
>>
> not, delay
>
>> > more, up to some maximum.
>> Sounds good, I'll try to find appropriate API.
>>
>
> Done. Upon further testing it turned that IP Address is renewed upon
> return from IpRenewAddress, and previously observed flakiness which I've
> attributed to address renewal delay was actually caused by out-of-band
> network change notifications.
>
> https://codereview.chromium.**org/27722003/diff/5001/chrome/**
>
utility/wifi/wifi_service_win.**cc#newcode459<https://codereview.chromium.org/27722003/diff/5001/chrome/utility/wifi/wifi_service_win.cc#newcode459>
> chrome/utility/wifi/wifi_**service_win.cc:459:
> On 2013/10/21 20:51:02, afontan wrote:
>
>> On 2013/10/21 19:42:00, mef wrote:
>> > On 2013/10/21 16:55:23, afontan wrote:
>> > > I assume using WLAN_API_VERSION is fine (given that we only use
>>
> 1.0 methods)
>
>> > but
>> > > in the Chromecast setup we use the same version 1.0 for all
>>
> windows OS
>
>> > (although
>> > > we only support win 7 and 8). This code uses 2.0 for win 7/8 but
>>
> you then
>
>> you
>> > do
>> > > not use the negotiated version to know what API is being used.
>> > Could you elaborate on suggested change? Also, I've tried Chromecast
>>
> APP on
>
>> > Windows XP and it didn't seem to work (at least on my VM). Is this
>>
> expected?
>
>  Correct. Today we only support win 7/8. For vista or XP today we send
>>
> the users
>
>> to manual web setup from the install site (where they manually connect
>>
> to
>
>> Chromecast). It would be great if you could do better as Chrome
>>
> supports
>
>> XP/Vista so it would be more consistent but the main issue with
>>
> supporting XP is
>
>> that the wireless drivers out there often do things very
>>
> inconsistently.
>
>  What I meant is that I believe that the WLAN_API_VERSION macro will
>> automatically expand to 2 in > Vista. The service_version will tell
>>
> you exactly
>
>> the API version that was negotiated. You are not using the API
>>
> negotiated value
>
>> to take decisions, as implicitly you are only using APIs supported in
>>
> both 1 and
>
>> 2.
>>
>
>  In that case why not just ask for API 1? otherwise if you ever use an
>>
> API that
>
>> does not exist in version 1, you will break XP (unless you consider
>>
> the value
>
>> returned in service_version). So the question is, if you only use the
>>
> version 1
>
>> APIs anyway, should you still request version 2 for win7/8? In the
>>
> chromecast
>
>> setup we ask for API 1 but not sure that is the best option as I am
>>
> not sure if
>
>> there are some behavior changes between 1/2 even for the same set of
>>
> APIs, if
>
>> there is no behavior change it would make sense to request version 1.
>>
>
>
> http://msdn.microsoft.com/en-**us/library/windows/desktop/**
>
ms706759%28v=vs.85%29.aspx<http://msdn.microsoft.com/en-us/library/windows/desktop/ms706759%28v=vs.85%29.aspx>
>
>  Note, I just wanted to point it out so you take the decision with all
>>
> the
>
>> information or can dig deeper to understand what is best in your
>>
> scenario, it is
>
>> a difference with our current implementation and makes sense to
>>
> understand the
>
>> trade-offs.
>>
>
>
>
>
>
> Done.
>
> https://codereview.chromium.**org/27722003/diff/5001/chrome/**
>
utility/wifi/wifi_test.cc<https://codereview.chromium.org/27722003/diff/5001/chrome/utility/wifi/wifi_test.cc>
> File chrome/utility/wifi/wifi_test.**cc (right):
>
> https://codereview.chromium.**org/27722003/diff/5001/chrome/**
>
utility/wifi/wifi_test.cc#**newcode15<https://codereview.chromium.org/27722003/diff/5001/chrome/utility/wifi/wifi_test.cc#newcode15>
> chrome/utility/wifi/wifi_test.**cc:15: #if defined(OS_MACOSX)
> On 2013/10/21 16:44:03, Jói wrote:
>
>> On 2013/10/21 15:37:16, mef wrote:
>> > On 2013/10/19 21:14:45, Jói wrote:
>> > > It seems more typical to move platform-specific includes to start
>>
> one blank
>
>> > line
>> > > after the main set of includes.
>> > I started with that, but presubmit checks were complaining about
>>
> wrong include
>
>> > order.
>>
>
>  They won't complain if you leave a blank line before the #if.
>>
>
> Done.
>
>
https://codereview.chromium.**org/27722003/<https://codereview.chromium.org/2...
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

mef

Hi Jói, PTAL. I've changed WiFiServiceImpl to use GetProcAddress to get WlanGetNetworkBssList function to avoid ...

7 years, 2 months ago (2013-10-24 23:44:46 UTC) #13

Hi Jói, PTAL. 

I've changed WiFiServiceImpl to use GetProcAddress to get WlanGetNetworkBssList
function to avoid potential XP loading issues. I believe that's the last one
that you pointed out.

thanks,
-m

https://codereview.chromium.org/27722003/diff/5001/chrome/utility/wifi/wifi_s...
File chrome/utility/wifi/wifi_service_win.cc (right):

https://codereview.chromium.org/27722003/diff/5001/chrome/utility/wifi/wifi_s...
chrome/utility/wifi/wifi_service_win.cc:133: std::string GuidFromSsid(const
DOT11_SSID& dot11Ssid) const {
On 2013/10/21 20:51:02, afontan wrote:
> On 2013/10/21 19:42:00, mef wrote:
> > On 2013/10/21 16:55:23, afontan wrote:
> > > dot11Ssid is not a GUID and it is not unique. You can have multiple
profiles
> > for
> > > the same SSID (a typical scenario is for example 2.4 and 5 GHz).
> > I could make it an interface-ssid-bssid combination. How unique do we want
> them
> > to be? Right now this api is able to connect to arbitrary unsecured network
> > (e.g. Chromekey1501, GoogleGuest) or previously connected secured network if
> > there is a saved profile. What would be a scenario to connect to particular
> BSS?
> 
> Correct. The 5GHz/2.4GHz with same SSID is the scenario that comes to mind.

Done. Per discussion with tbarzic@ we'll keep the same guid for both networks,
but will allow explicit parameter to connect on specific frequency.

https://codereview.chromium.org/27722003/diff/5001/chrome/utility/wifi/wifi_s...
chrome/utility/wifi/wifi_service_win.cc:144: std::string GuidFromWlan(const
WLAN_AVAILABLE_NETWORK& wlan) const {
On 2013/10/21 16:55:23, afontan wrote:
> ditto.

Done. Same as GUIDFromSSID().

https://codereview.chromium.org/27722003/diff/5001/chrome/utility/wifi/wifi_s...
chrome/utility/wifi/wifi_service_win.cc:694: DWORD
WiFiServiceImpl::Connect(const std::string& network_guid) {
On 2013/10/21 20:51:02, afontan wrote:
> On 2013/10/21 19:42:00, mef wrote:
> > On 2013/10/21 16:55:23, afontan wrote:
> > > One scenario we had to cover based on dogfooding was to connect to a
network
> > > with a SSID using the 5GHz profile (assuming the same SSID supports both
2.4
> > and
> > > 5 GHz), then try to setup Chromecast.
> > > 
> > > If we are unable, it means that the user has isolation between 2.4 and
5GHz.
> > At
> > > that point we need to connect to 2.4GHz and if we are successful, still
> > provide
> > > a warning to the user that he may not be able to use Chromecast properly
> (for
> > > example if the laptop decides to use 5GHz).
> > > 
> > > For this scenario we need to be able to specify the specific
WLAN_BSS_ENTRY.
> 
> > 
> > Interesting. Could you elaborate on this? Currently networkingPrivate API
> > doesn't 
> > provide parameters to specify BSSID or encryption key/password. 
> > Adding tbarzic@ for extension knowledge.
> > 
> > I could make network_guid to correspond to every BSSID, but that would mean
> that
> > in the office I get 12 GoogleGuests to choose from.
> 
> We get a list of band filtered Bss entries that contains the 5GHz entries that
> have the same SSID or are hidden networks and we use that list of Bss entries
to
> connect. In this way we will not connect to the 2.4GHz network even if the
SSID
> matches.
> 
> WlanConnect takes a PWLAN_CONNECTION_PARAMETERS that takes a desired bssid
list:
> 
>
http://msdn.microsoft.com/en-us/library/windows/desktop/ms706613%28v=vs.85%29...
>
http://msdn.microsoft.com/en-us/library/windows/desktop/ms706851%28v=vs.85%29...
> 
> I assume you have access to the setup code as I see you have the DHCP
> workaround, look for ConnectWithXML in wlanManager.
> 
> 
> 

Done. On separate CL.

Jói

Hi Misha, See my comments below, but let me summarize your options: a) If you ...

7 years, 1 month ago (2013-10-25 11:09:42 UTC) #14

Hi Misha,

See my comments below, but let me summarize your options:

a) If you intend to use some of the Wlan functions on XP (and conditionally use
the BSS one depending on whether it is available) then you need to do what I'm
suggesting below, i.e. bind all the functions from the Wlan library dynamically
and not link to it using an import library at all.

b) If you simply intend for your feature not to work on XP at all, then it is
probably possible to delay load the library, and do an OS version check before
the delay load gets triggered by any code, to ensure that the delay load is
never attempted on XP. That way you don't have to do any dynamic binding of
functions, but you need to be very careful that the delay load does not get
triggered on XP.

Cheers,
Jói

https://codereview.chromium.org/27722003/diff/412001/chrome/chrome_dll.gypi
File chrome/chrome_dll.gypi (right):

https://codereview.chromium.org/27722003/diff/412001/chrome/chrome_dll.gypi#n...
chrome/chrome_dll.gypi:201: 'wlanapi.dll',
I'm pretty sure your delay load will still fail on XP since the import library
you are using is going to be the one that includes the function not supported on
XP. So the implicit LoadLibrary that does function binding via the import
library will fail and the whole delayload will fail.

Normally when you have a DLL with a different set of functions between versions,
you need to not link to it using an import library, and instead only LoadLibrary
explicitly and bind all the functions at runtime as you are currently doing with
the single function that is not always available.

https://codereview.chromium.org/27722003/diff/412001/chrome/utility/wifi/wifi...
File chrome/utility/wifi/wifi_service_win.cc (right):

https://codereview.chromium.org/27722003/diff/412001/chrome/utility/wifi/wifi...
chrome/utility/wifi/wifi_service_win.cc:226: WlanGetNetworkBssListFunction
WlanGetNetworkBssList_function_;
You need to manually bind all of the functions that you're going to use from
WlanApi.dll and not link to it in any way, only load it explicitly via
LoadLibrary(Ex).

See e.g. content/browser/geolocation/wifi_data_provider_win.cc which does this
for the same library.

You may want to make the function pointers global (or static) rather than
instance variables, in case you expect there to be more than one instance of
WifiService during the lifetime of the program, to avoid unnecessary multiple
initialization.

https://codereview.chromium.org/27722003/diff/412001/chrome/utility/wifi/wifi...
chrome/utility/wifi/wifi_service_win.cc:536: DLOG(ERROR) << "Unable to find
WlanGetNetworkBssList function.";
This is an expected case on XP and as such shouldn't be an ERROR but perhaps a
WARNING or INFO.

https://codereview.chromium.org/27722003/diff/412001/chrome/utility/wifi/wifi...
chrome/utility/wifi/wifi_service_win.cc:786: error =
WlanGetNetworkBssList_function_(client_,
You shouldn't call this function unless it is available, i.e. NULL-check it
before calling it.

mef

On 2013/10/25 11:09:42, Jói wrote: > Hi Misha, > > See my comments below, but ...

7 years, 1 month ago (2013-10-25 12:03:00 UTC) #15

On 2013/10/25 11:09:42, Jói wrote:
> Hi Misha,
> 
> See my comments below, but let me summarize your options:
> 
> a) If you intend to use some of the Wlan functions on XP (and conditionally
use
> the BSS one depending on whether it is available) then you need to do what I'm
> suggesting below, i.e. bind all the functions from the Wlan library
dynamically
> and not link to it using an import library at all.
> 
> b) If you simply intend for your feature not to work on XP at all, then it is
> probably possible to delay load the library, and do an OS version check before
> the delay load gets triggered by any code, to ensure that the delay load is
> never attempted on XP. That way you don't have to do any dynamic binding of
> functions, but you need to be very careful that the delay load does not get
> triggered on XP.
> 
> Cheers,
> Jói
> 
> https://codereview.chromium.org/27722003/diff/412001/chrome/chrome_dll.gypi
> File chrome/chrome_dll.gypi (right):
> 
>
https://codereview.chromium.org/27722003/diff/412001/chrome/chrome_dll.gypi#n...
> chrome/chrome_dll.gypi:201: 'wlanapi.dll',
> I'm pretty sure your delay load will still fail on XP since the import library
> you are using is going to be the one that includes the function not supported
on
> XP. So the implicit LoadLibrary that does function binding via the import
> library will fail and the whole delayload will fail.
> 
> Normally when you have a DLL with a different set of functions between
versions,
> you need to not link to it using an import library, and instead only
LoadLibrary
> explicitly and bind all the functions at runtime as you are currently doing
with
> the single function that is not always available.
> 
>
https://codereview.chromium.org/27722003/diff/412001/chrome/utility/wifi/wifi...
> File chrome/utility/wifi/wifi_service_win.cc (right):
> 
>
https://codereview.chromium.org/27722003/diff/412001/chrome/utility/wifi/wifi...
> chrome/utility/wifi/wifi_service_win.cc:226: WlanGetNetworkBssListFunction
> WlanGetNetworkBssList_function_;
> You need to manually bind all of the functions that you're going to use from
> WlanApi.dll and not link to it in any way, only load it explicitly via
> LoadLibrary(Ex).
> 
> See e.g. content/browser/geolocation/wifi_data_provider_win.cc which does this
> for the same library.
> 
> You may want to make the function pointers global (or static) rather than
> instance variables, in case you expect there to be more than one instance of
> WifiService during the lifetime of the program, to avoid unnecessary multiple
> initialization.
> 
>
https://codereview.chromium.org/27722003/diff/412001/chrome/utility/wifi/wifi...
> chrome/utility/wifi/wifi_service_win.cc:536: DLOG(ERROR) << "Unable to find
> WlanGetNetworkBssList function.";
> This is an expected case on XP and as such shouldn't be an ERROR but perhaps a
> WARNING or INFO.
> 
>
https://codereview.chromium.org/27722003/diff/412001/chrome/utility/wifi/wifi...
> chrome/utility/wifi/wifi_service_win.cc:786: error =
> WlanGetNetworkBssList_function_(client_,
> You shouldn't call this function unless it is available, i.e. NULL-check it
> before calling it.

Thanks joi, given the limited number of WLAN API functions that I need, it is
better to be on the safe side and use dynamic binding for everything, and drop
delay loading completely.
I'll craft the change and send for review.

mef

Hi Jói, PTAL. I've changed WiFiService to dynamically load WlanApi.dll, so chrome.dll no longer links ...

7 years, 1 month ago (2013-10-25 15:09:04 UTC) #16

Hi Jói, PTAL. 

I've changed WiFiService to dynamically load WlanApi.dll, so chrome.dll no
longer links with it.

https://codereview.chromium.org/27722003/diff/412001/chrome/chrome_dll.gypi
File chrome/chrome_dll.gypi (right):

https://codereview.chromium.org/27722003/diff/412001/chrome/chrome_dll.gypi#n...
chrome/chrome_dll.gypi:201: 'wlanapi.dll',
On 2013/10/25 11:09:42, Jói wrote:
> I'm pretty sure your delay load will still fail on XP since the import library
> you are using is going to be the one that includes the function not supported
on
> XP. So the implicit LoadLibrary that does function binding via the import
> library will fail and the whole delayload will fail.
> 
> Normally when you have a DLL with a different set of functions between
versions,
> you need to not link to it using an import library, and instead only
LoadLibrary
> explicitly and bind all the functions at runtime as you are currently doing
with
> the single function that is not always available.

Done.

https://codereview.chromium.org/27722003/diff/412001/chrome/utility/wifi/wifi...
File chrome/utility/wifi/wifi_service_win.cc (right):

https://codereview.chromium.org/27722003/diff/412001/chrome/utility/wifi/wifi...
chrome/utility/wifi/wifi_service_win.cc:226: WlanGetNetworkBssListFunction
WlanGetNetworkBssList_function_;
On 2013/10/25 11:09:42, Jói wrote:
> You need to manually bind all of the functions that you're going to use from
> WlanApi.dll and not link to it in any way, only load it explicitly via
> LoadLibrary(Ex).
> 
> See e.g. content/browser/geolocation/wifi_data_provider_win.cc which does this
> for the same library.
> 
> You may want to make the function pointers global (or static) rather than
> instance variables, in case you expect there to be more than one instance of
> WifiService during the lifetime of the program, to avoid unnecessary multiple
> initialization.

Done.

https://codereview.chromium.org/27722003/diff/412001/chrome/utility/wifi/wifi...
chrome/utility/wifi/wifi_service_win.cc:536: DLOG(ERROR) << "Unable to find
WlanGetNetworkBssList function.";
On 2013/10/25 11:09:42, Jói wrote:
> This is an expected case on XP and as such shouldn't be an ERROR but perhaps a
> WARNING or INFO.

Done.

https://codereview.chromium.org/27722003/diff/412001/chrome/utility/wifi/wifi...
chrome/utility/wifi/wifi_service_win.cc:786: error =
WlanGetNetworkBssList_function_(client_,
On 2013/10/25 11:09:42, Jói wrote:
> You shouldn't call this function unless it is available, i.e. NULL-check it
> before calling it.

Done.

cpu_(ooo_6.6-7.5)

https://codereview.chromium.org/27722003/diff/412001/chrome/utility/wifi/wifi_service_win.cc File chrome/utility/wifi/wifi_service_win.cc (right): https://codereview.chromium.org/27722003/diff/412001/chrome/utility/wifi/wifi_service_win.cc#newcode263 chrome/utility/wifi/wifi_service_win.cc:263: DWORD error = EnsureInitialized(); shouldn't be an Initialize method ...

7 years, 1 month ago (2013-10-25 16:51:08 UTC) #17

mef

Hi Carlos, thanks for your comments! https://codereview.chromium.org/27722003/diff/412001/chrome/utility/wifi/wifi_service_win.cc File chrome/utility/wifi/wifi_service_win.cc (right): https://codereview.chromium.org/27722003/diff/412001/chrome/utility/wifi/wifi_service_win.cc#newcode263 chrome/utility/wifi/wifi_service_win.cc:263: DWORD error = ...

7 years, 1 month ago (2013-10-25 18:00:37 UTC) #18

mef

Hi Carlos, PTAL. https://codereview.chromium.org/27722003/diff/412001/chrome/utility/wifi/wifi_service_win.cc File chrome/utility/wifi/wifi_service_win.cc (right): https://codereview.chromium.org/27722003/diff/412001/chrome/utility/wifi/wifi_service_win.cc#newcode263 chrome/utility/wifi/wifi_service_win.cc:263: DWORD error = EnsureInitialized(); On 2013/10/25 ...

7 years, 1 month ago (2013-10-25 19:43:16 UTC) #19

mef

On 2013/10/28 13:09:18, Jói wrote: > LGTM Thanks, Jói! Antonio, do you have any additional ...

7 years, 1 month ago (2013-10-28 14:11:19 UTC) #21

afontan

lgtm I think the behavior is acceptable. We should probably investigate the possibility of creating ...

7 years, 1 month ago (2013-10-28 14:50:49 UTC) #22

mef

Thanks, Antonio! Now I would like to ask for OWNERS approval: finnur@ - chrome/common/extensions/api/_api_features.json (please ...

7 years, 1 month ago (2013-10-28 15:02:36 UTC) #23

Finnur

OWNERS LGTM. > please let me know if I should rename "windows" into "win" to ...

7 years, 1 month ago (2013-10-28 15:13:34 UTC) #24

mef

Thanks, Finnur! Per our conversation I've renamed feature platforms to 'win' and 'mac' to match ...

7 years, 1 month ago (2013-10-28 16:41:13 UTC) #25

Finnur

On 2013/10/28 16:41:13, mef wrote: > Thanks, Finnur! Per our conversation I've renamed feature platforms ...

7 years, 1 month ago (2013-10-28 16:44:10 UTC) #26

jam

we still have a conversation over at https://codereview.chromium.org/22295002/ about whether this should be in the ...

7 years, 1 month ago (2013-10-28 17:00:18 UTC) #27

mef

7 years, 1 month ago (2013-10-28 17:43:25 UTC) #28

On 2013/10/28 17:00:18, jam wrote:
> we still have a conversation over at https://codereview.chromium.org/22295002/
> about whether this should be in the utility process or not. let's finish that
> conversation there first.

Definitely. I just wanted to bring this one to your attention.

Issue 27722003: Windows-specific implementation of Networking Private API. (Closed)

Description

Patch Set 1 #

Patch Set 2 : Specify wlanapi.lib in chrome.gyp instead of #pragma comment. #

Patch Set 3 : Address code review comments. #

Patch Set 4 : Address codereview comments. #

Patch Set 5 : Temporarily disable network location wizard when WiFi network is connected. #

Patch Set 6 : Sync up to r230127 #

Patch Set 7 : Address codereview comments. #

Patch Set 8 : Filter out duplicate networks in GetVisibleNetworkList. #

Patch Set 9 : Use GetProcAddress to get WlanGetNetworkBssList function to avoid XP issues. #

Patch Set 10 : Dynamically LoadLibrary(WlanApi) to avoid missing functions on XP. #

Patch Set 11 : Address cpu's comments. #

Patch Set 12 : Sync to r231308 #

Patch Set 13 : Renamed feature platforms to 'win' and 'mac' to match Platforms from model.py #

Messages