Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2842613002:
Geolocation Win: remove Xp/Vista specific WindowsNdisApi class (Closed)
DescriptionGeolocation Win: remove Xp/Vista specific WindowsNdisApi class
This CL primarily removes the internal WindowsNdisApi class
which is only meant to be used for the unsupported Win XP and
Vista, and leaves only WindowWlanApi internal class in place.
Furthermore it simplifies the remaining WindosWlanApi code
by linking directly against wlanapi.dll, which makes unnecessary
a number of other file-static functions, the factory method
and the library functions defintiions and variables, yay!
It also removes a chunk of file-static functions that became
unused; only one GetNetworkData() remains.
PS4 UPDATE: this CL actually could not remove the dynamic loading
of wlan_api.dll because it's not available by default in
some/all Server 2008 R2 installations, including some of our
win_chromium_rel_ng slaves, which failed. See
https://www.bonusbits.com/wiki/KB:Wlanapi.dll_missing_on_Windows_Server_2008_R2
(However, we can remove the WindowsNdisApi class because those
Server2008R2 installations would explicitly enable WlanApi.dll
to access Wifi anyway).
BUG=714477
Review-Url: https://codereview.chromium.org/2842613002
Cr-Commit-Position: refs/heads/master@{#467807}
Committed: https://chromium.googlesource.com/chromium/src/+/fddf5df3d84f5c32e2dff8296b875b180afe9f02
Patch Set 1 : #
Total comments: 6
Patch Set 2 : robliao@s comments - static lib loading #
Total comments: 12
Patch Set 3 : dougt@ and robliao@ nits #Patch Set 4 : Restated dynamic loading of wlan_api.dll #Messages
Total messages: 43 (25 generated)
Description was changed from ========== Geolocation Win: remove Xp/Vsita specific WindowsNdisApi class style Nuked WindowsNdisApi class and accessory static functions BUG=714477 ========== to ========== Geolocation Win: remove Xp/Vsita specific WindowsNdisApi class This CL primarily removes the internal WindowsNdisApi class which is only meant to be used for the unsupported Win XP and Vista. It also removes a chunk of file-static functions that are not used anymore so that only one such function remains. Furthermore it simplifies the remaining WindosWlanApi code: - removes the file-static function GetSystemDirectory(); instead the code to open the library is moved into WindowsWlanApi::Create(), and a simpler version taken from the more modern WlanApi class [1] is used. - The library functions are now initialized in the ctor ISO having a specific separated GetWLANFunctions() method. [1] https://cs.chromium.org/chromium/src/net/base/network_interfaces_win.cc?type=... BUG=714477 ==========
Patchset #1 (id:1) has been deleted
Description was changed from ========== Geolocation Win: remove Xp/Vsita specific WindowsNdisApi class This CL primarily removes the internal WindowsNdisApi class which is only meant to be used for the unsupported Win XP and Vista. It also removes a chunk of file-static functions that are not used anymore so that only one such function remains. Furthermore it simplifies the remaining WindosWlanApi code: - removes the file-static function GetSystemDirectory(); instead the code to open the library is moved into WindowsWlanApi::Create(), and a simpler version taken from the more modern WlanApi class [1] is used. - The library functions are now initialized in the ctor ISO having a specific separated GetWLANFunctions() method. [1] https://cs.chromium.org/chromium/src/net/base/network_interfaces_win.cc?type=... BUG=714477 ========== to ========== Geolocation Win: remove Xp/Vsita specific WindowsNdisApi class This CL primarily removes the internal WindowsNdisApi class which is only meant to be used for the unsupported Win XP and Vista, and leaves only WindowWlanApi internal class in place. It also removes a chunk of file-static functions that are become unused; only one such file-static function remains. Furthermore it simplifies the remaining WindosWlanApi code: - removes the file-static function GetSystemDirectory(); instead the code to open the library is moved into WindowsWlanApi::Create(), and a simpler version taken from the more modern WlanApi class [1] is used. - The library functions are now initialized in the ctor ISO having a specific separated GetWLANFunctions() method. [1] https://cs.chromium.org/chromium/src/net/base/network_interfaces_win.cc?type=... BUG=714477 ==========
mcasas@chromium.org changed reviewers: + robliao@chromium.org
robliao@ PTAL
Description was changed from ========== Geolocation Win: remove Xp/Vsita specific WindowsNdisApi class This CL primarily removes the internal WindowsNdisApi class which is only meant to be used for the unsupported Win XP and Vista, and leaves only WindowWlanApi internal class in place. It also removes a chunk of file-static functions that are become unused; only one such file-static function remains. Furthermore it simplifies the remaining WindosWlanApi code: - removes the file-static function GetSystemDirectory(); instead the code to open the library is moved into WindowsWlanApi::Create(), and a simpler version taken from the more modern WlanApi class [1] is used. - The library functions are now initialized in the ctor ISO having a specific separated GetWLANFunctions() method. [1] https://cs.chromium.org/chromium/src/net/base/network_interfaces_win.cc?type=... BUG=714477 ========== to ========== Geolocation Win: remove Xp/Vista specific WindowsNdisApi class This CL primarily removes the internal WindowsNdisApi class which is only meant to be used for the unsupported Win XP and Vista, and leaves only WindowWlanApi internal class in place. It also removes a chunk of file-static functions that are become unused; only one such file-static function remains. Furthermore it simplifies the remaining WindosWlanApi code: - removes the file-static function GetSystemDirectory(); instead the code to open the library is moved into WindowsWlanApi::Create(), and a simpler version taken from the more modern WlanApi class [1] is used. - The library functions are now initialized in the ctor ISO having a specific separated GetWLANFunctions() method. [1] https://cs.chromium.org/chromium/src/net/base/network_interfaces_win.cc?type=... BUG=714477 ==========
https://codereview.chromium.org/2842613002/diff/20001/device/geolocation/wifi... File device/geolocation/wifi_data_provider_win.cc (right): https://codereview.chromium.org/2842613002/diff/20001/device/geolocation/wifi... device/geolocation/wifi_data_provider_win.cc:104: DCHECK(base::win::GetVersion() >= base::win::VERSION_WIN7); Is this DCHECK important? I would remove as if this fails, a lot of other things will likely be failing too. https://codereview.chromium.org/2842613002/diff/20001/device/geolocation/wifi... device/geolocation/wifi_data_provider_win.cc:110: HINSTANCE library = LoadLibraryEx(path, NULL, LOAD_WITH_ALTERED_SEARCH_PATH); Can we just directly make the Windows API calls now? https://codereview.chromium.org/2842613002/diff/20001/device/geolocation/wifi... device/geolocation/wifi_data_provider_win.cc:236: WifiDataProviderWin::WifiDataProviderWin() {} WifiDataProviderWin() = default here and below.
Description was changed from ========== Geolocation Win: remove Xp/Vista specific WindowsNdisApi class This CL primarily removes the internal WindowsNdisApi class which is only meant to be used for the unsupported Win XP and Vista, and leaves only WindowWlanApi internal class in place. It also removes a chunk of file-static functions that are become unused; only one such file-static function remains. Furthermore it simplifies the remaining WindosWlanApi code: - removes the file-static function GetSystemDirectory(); instead the code to open the library is moved into WindowsWlanApi::Create(), and a simpler version taken from the more modern WlanApi class [1] is used. - The library functions are now initialized in the ctor ISO having a specific separated GetWLANFunctions() method. [1] https://cs.chromium.org/chromium/src/net/base/network_interfaces_win.cc?type=... BUG=714477 ========== to ========== Geolocation Win: remove Xp/Vista specific WindowsNdisApi class This CL primarily removes the internal WindowsNdisApi class which is only meant to be used for the unsupported Win XP and Vista, and leaves only WindowWlanApi internal class in place. Furthermore it simplifies the remaining WindosWlanApi code by linking directly against wlanapi.dll, which makes unnecessary a number of other file-static functions, the factory method and the library functions defintiions and variables, yay! It also removes a chunk of file-static functions that became unused; only one GetNetworkData() remains. BUG=714477 ==========
ptal (assuming bots are happy) https://codereview.chromium.org/2842613002/diff/20001/device/geolocation/wifi... File device/geolocation/wifi_data_provider_win.cc (right): https://codereview.chromium.org/2842613002/diff/20001/device/geolocation/wifi... device/geolocation/wifi_data_provider_win.cc:104: DCHECK(base::win::GetVersion() >= base::win::VERSION_WIN7); On 2017/04/25 20:11:19, robliao wrote: > Is this DCHECK important? I would remove as if this fails, a lot of other things > will likely be failing too. Done. https://codereview.chromium.org/2842613002/diff/20001/device/geolocation/wifi... device/geolocation/wifi_data_provider_win.cc:110: HINSTANCE library = LoadLibraryEx(path, NULL, LOAD_WITH_ALTERED_SEARCH_PATH); On 2017/04/25 20:11:19, robliao wrote: > Can we just directly make the Windows API calls now? All gone! Another haircut for the file! https://codereview.chromium.org/2842613002/diff/20001/device/geolocation/wifi... device/geolocation/wifi_data_provider_win.cc:236: WifiDataProviderWin::WifiDataProviderWin() {} On 2017/04/25 20:11:19, robliao wrote: > WifiDataProviderWin() = default here and below. Done.
mcasas@chromium.org changed reviewers: + dougt@chromium.org
dougt@ PTAL (also, while waiting for robliao@ review)
Windows Usage lgtm. Thanks for updating the code! https://codereview.chromium.org/2842613002/diff/40001/device/geolocation/wifi... File device/geolocation/wifi_data_provider_win.cc (right): https://codereview.chromium.org/2842613002/diff/40001/device/geolocation/wifi... device/geolocation/wifi_data_provider_win.cc:28: static const int kNoWifiPollingIntervalMs = 20 * 1000; // 20s Accidental removal? https://codereview.chromium.org/2842613002/diff/40001/device/geolocation/wifi... device/geolocation/wifi_data_provider_win.cc:48: // WlanApiInterface imlpementation Nit: implementation
lgtm w/ nits https://codereview.chromium.org/2842613002/diff/40001/device/geolocation/wifi... File device/geolocation/wifi_data_provider_win.cc (left): https://codereview.chromium.org/2842613002/diff/40001/device/geolocation/wifi... device/geolocation/wifi_data_provider_win.cc:245: static const int kXpWlanClientVersion = 1; This is pretty interesting, I would have thought you could have passed two now as it's the client version for Windows Vista and Windows Server 2008 (and up?). https://codereview.chromium.org/2842613002/diff/40001/device/geolocation/wifi... device/geolocation/wifi_data_provider_win.cc:312: // According to http://www.attnetclient.com/kb/questions.php?questionid=75 Dead link. Nothing in web.archive.org. Just kill this line. https://codereview.chromium.org/2842613002/diff/40001/device/geolocation/wifi... File device/geolocation/wifi_data_provider_win.cc (right): https://codereview.chromium.org/2842613002/diff/40001/device/geolocation/wifi... device/geolocation/wifi_data_provider_win.cc:40: return access_point_data; I think you should leave steve's comment block. Someday we can figure out how to fill out the other attributes of the AccessPointData: TODO(steveblock): Is it possible to get the following? https://codereview.chromium.org/2842613002/diff/40001/device/geolocation/wifi... device/geolocation/wifi_data_provider_win.cc:126: NULL, // Use all SSIDs. do you want to convert these NULLs to nullptr's?
https://codereview.chromium.org/2842613002/diff/40001/device/geolocation/wifi... File device/geolocation/wifi_data_provider_win.cc (left): https://codereview.chromium.org/2842613002/diff/40001/device/geolocation/wifi... device/geolocation/wifi_data_provider_win.cc:245: static const int kXpWlanClientVersion = 1; On 2017/04/26 00:15:47, dougt wrote: > This is pretty interesting, I would have thought you could have passed two now > as it's the client version for Windows Vista and Windows Server 2008 (and up?). This could have subtle behavior changes in the API. This would probably be safer as a separate change.
Patchset #3 (id:60001) has been deleted
https://codereview.chromium.org/2842613002/diff/40001/device/geolocation/wifi... File device/geolocation/wifi_data_provider_win.cc (left): https://codereview.chromium.org/2842613002/diff/40001/device/geolocation/wifi... device/geolocation/wifi_data_provider_win.cc:312: // According to http://www.attnetclient.com/kb/questions.php?questionid=75 On 2017/04/26 00:15:47, dougt wrote: > Dead link. Nothing in http://web.archive.org. Just kill this line. Done. https://codereview.chromium.org/2842613002/diff/40001/device/geolocation/wifi... File device/geolocation/wifi_data_provider_win.cc (right): https://codereview.chromium.org/2842613002/diff/40001/device/geolocation/wifi... device/geolocation/wifi_data_provider_win.cc:28: static const int kNoWifiPollingIntervalMs = 20 * 1000; On 2017/04/26 00:02:40, robliao wrote: > // 20s > Accidental removal? Oh yes, well spotted. https://codereview.chromium.org/2842613002/diff/40001/device/geolocation/wifi... device/geolocation/wifi_data_provider_win.cc:40: return access_point_data; On 2017/04/26 00:15:46, dougt wrote: > I think you should leave steve's comment block. Someday we can figure out how to > fill out the other attributes of the AccessPointData: > > TODO(steveblock): Is it possible to get the following? Done. https://codereview.chromium.org/2842613002/diff/40001/device/geolocation/wifi... device/geolocation/wifi_data_provider_win.cc:48: // WlanApiInterface imlpementation On 2017/04/26 00:02:40, robliao wrote: > Nit: implementation Done. https://codereview.chromium.org/2842613002/diff/40001/device/geolocation/wifi... device/geolocation/wifi_data_provider_win.cc:126: NULL, // Use all SSIDs. On 2017/04/26 00:15:47, dougt wrote: > do you want to convert these NULLs to nullptr's? I left the NULLs for API calls, I think Win APIs are better left untouched.
The CQ bit was checked by mcasas@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: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by mcasas@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from robliao@chromium.org, dougt@chromium.org Link to the patchset: https://codereview.chromium.org/2842613002/#ps80001 (title: "dougt@ and robliao@ nits")
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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by mcasas@chromium.org
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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by mcasas@chromium.org
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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Description was changed from ========== Geolocation Win: remove Xp/Vista specific WindowsNdisApi class This CL primarily removes the internal WindowsNdisApi class which is only meant to be used for the unsupported Win XP and Vista, and leaves only WindowWlanApi internal class in place. Furthermore it simplifies the remaining WindosWlanApi code by linking directly against wlanapi.dll, which makes unnecessary a number of other file-static functions, the factory method and the library functions defintiions and variables, yay! It also removes a chunk of file-static functions that became unused; only one GetNetworkData() remains. BUG=714477 ========== to ========== Geolocation Win: remove Xp/Vista specific WindowsNdisApi class This CL primarily removes the internal WindowsNdisApi class which is only meant to be used for the unsupported Win XP and Vista, and leaves only WindowWlanApi internal class in place. Furthermore it simplifies the remaining WindosWlanApi code by linking directly against wlanapi.dll, which makes unnecessary a number of other file-static functions, the factory method and the library functions defintiions and variables, yay! It also removes a chunk of file-static functions that became unused; only one GetNetworkData() remains. PS4 UPDATE: this CL actually could not remove the dynamic loading of wlan_api.dll because it's not available by default in some/all Server 2008 R2 installations, including some of our win_chromium_rel_ng slaves, which failed. See https://www.bonusbits.com/wiki/KB:Wlanapi.dll_missing_on_Windows_Server_2008_R2 (However, we can remove the WindowsNdisApi class because those Server2008R2 installations would explicitly enable WlanApi.dll to access Wifi anyway). BUG=714477 ==========
Patchset #4 (id:100001) has been deleted
On 2017/04/26 16:26:57, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) robliao@, dougt@ PS4 restates the dynamic loading of wlan_api.dll after failures in win_chromium_rel_ng indicated that said library is not available in some cases, see CL description and/or WindowsWlanApi class comments. PTAL or RS or just let the CL roll :-) Thanks!
Still lgtm. Sorry that Server stopped the party!
The CQ bit was checked by mcasas@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dougt@chromium.org Link to the patchset: https://codereview.chromium.org/2842613002/#ps120001 (title: "Restated dynamic loading of wlan_api.dll")
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": 120001, "attempt_start_ts": 1493329418594600,
"parent_rev": "de456d8a5abd7f80414bec689f45e29dc9521f6a", "commit_rev":
"fddf5df3d84f5c32e2dff8296b875b180afe9f02"}
Message was sent while issue was closed.
Description was changed from ========== Geolocation Win: remove Xp/Vista specific WindowsNdisApi class This CL primarily removes the internal WindowsNdisApi class which is only meant to be used for the unsupported Win XP and Vista, and leaves only WindowWlanApi internal class in place. Furthermore it simplifies the remaining WindosWlanApi code by linking directly against wlanapi.dll, which makes unnecessary a number of other file-static functions, the factory method and the library functions defintiions and variables, yay! It also removes a chunk of file-static functions that became unused; only one GetNetworkData() remains. PS4 UPDATE: this CL actually could not remove the dynamic loading of wlan_api.dll because it's not available by default in some/all Server 2008 R2 installations, including some of our win_chromium_rel_ng slaves, which failed. See https://www.bonusbits.com/wiki/KB:Wlanapi.dll_missing_on_Windows_Server_2008_R2 (However, we can remove the WindowsNdisApi class because those Server2008R2 installations would explicitly enable WlanApi.dll to access Wifi anyway). BUG=714477 ========== to ========== Geolocation Win: remove Xp/Vista specific WindowsNdisApi class This CL primarily removes the internal WindowsNdisApi class which is only meant to be used for the unsupported Win XP and Vista, and leaves only WindowWlanApi internal class in place. Furthermore it simplifies the remaining WindosWlanApi code by linking directly against wlanapi.dll, which makes unnecessary a number of other file-static functions, the factory method and the library functions defintiions and variables, yay! It also removes a chunk of file-static functions that became unused; only one GetNetworkData() remains. PS4 UPDATE: this CL actually could not remove the dynamic loading of wlan_api.dll because it's not available by default in some/all Server 2008 R2 installations, including some of our win_chromium_rel_ng slaves, which failed. See https://www.bonusbits.com/wiki/KB:Wlanapi.dll_missing_on_Windows_Server_2008_R2 (However, we can remove the WindowsNdisApi class because those Server2008R2 installations would explicitly enable WlanApi.dll to access Wifi anyway). BUG=714477 Review-Url: https://codereview.chromium.org/2842613002 Cr-Commit-Position: refs/heads/master@{#467807} Committed: https://chromium.googlesource.com/chromium/src/+/fddf5df3d84f5c32e2dff8296b87... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:120001) as https://chromium.googlesource.com/chromium/src/+/fddf5df3d84f5c32e2dff8296b87... |
