Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1265453004:
Get WiFi SSID information for Windows (Closed)
|
Created:
5 years, 4 months ago by tbansal1 Modified:
5 years, 4 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, Randy Smith (Not in Mondays), mef, bengr Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionGet WiFi SSID information for Windows.
Factors out GetWifiPHYLayerProtocol() func. Also, slight
modification to NQE to use the SSID information. Tested
to be working on Windows 7 -- gives the correct SSID name.
BUG=503235
Committed: https://crrev.com/85da67aea5f72baf4cec5fd87d09c7b038465e7f
Cr-Commit-Position: refs/heads/master@{#341405}
Patch Set 1 #
Total comments: 5
Patch Set 2 : Addressed mef's comments #
Total comments: 14
Patch Set 3 : Addressed comments #
Total comments: 4
Patch Set 4 : Removed bool #
Total comments: 8
Patch Set 5 : Addressed comment #
Messages
Total messages: 18 (5 generated)
tbansal@chromium.org changed reviewers: + pauljensen@chromium.org
pauljensen: PTAL.
mef@chromium.org changed reviewers: + mef@chromium.org
https://codereview.chromium.org/1265453004/diff/1/net/base/network_interfaces... File net/base/network_interfaces_win.cc (right): https://codereview.chromium.org/1265453004/diff/1/net/base/network_interfaces... net/base/network_interfaces_win.cc:53: std::string GUIDFromSSID(const DOT11_SSID& dot11_ssid) { The GUID in this name is very WiFiService / networkingPrivate API specific. I'd call it 'StringFromDot11SSID' or just inline where it is called. https://codereview.chromium.org/1265453004/diff/1/net/base/network_interfaces... net/base/network_interfaces_win.cc:359: return GUIDFromSSID(conn_info->wlanAssociationAttributes.dot11Ssid); I wouldn't be surprised that wlanAssociationAttributes is NULL if adapter is disabled or disconnected. Also, inline GUIDFromSSID here.
PTAL. https://codereview.chromium.org/1265453004/diff/1/net/base/network_interfaces... File net/base/network_interfaces_win.cc (right): https://codereview.chromium.org/1265453004/diff/1/net/base/network_interfaces... net/base/network_interfaces_win.cc:53: std::string GUIDFromSSID(const DOT11_SSID& dot11_ssid) { On 2015/07/28 16:26:30, mef wrote: > The GUID in this name is very WiFiService / networkingPrivate API specific. I'd > call it 'StringFromDot11SSID' or just inline where it is called. Inlined. https://codereview.chromium.org/1265453004/diff/1/net/base/network_interfaces... net/base/network_interfaces_win.cc:359: return GUIDFromSSID(conn_info->wlanAssociationAttributes.dot11Ssid); On 2015/07/28 16:26:30, mef wrote: > I wouldn't be surprised that wlanAssociationAttributes is NULL if adapter is > disabled or disconnected. I am confused. How can the |wlanAssociationAttributes| struct be NULL since it is not a pointer. https://msdn.microsoft.com/en-us/library/windows/desktop/ms706842(v=vs.85).aspx > > Also, inline GUIDFromSSID here. Done.
looks good to me, up to Paul now. https://codereview.chromium.org/1265453004/diff/1/net/base/network_interfaces... File net/base/network_interfaces_win.cc (right): https://codereview.chromium.org/1265453004/diff/1/net/base/network_interfaces... net/base/network_interfaces_win.cc:359: return GUIDFromSSID(conn_info->wlanAssociationAttributes.dot11Ssid); On 2015/07/28 17:34:28, tbansal1 wrote: > On 2015/07/28 16:26:30, mef wrote: > > I wouldn't be surprised that wlanAssociationAttributes is NULL if adapter is > > disabled or disconnected. > I am confused. How can the |wlanAssociationAttributes| struct be > NULL since it is not a pointer. > https://msdn.microsoft.com/en-us/library/windows/desktop/ms706842(v=vs.85).aspx Oops, never mind. > > > > Also, inline GUIDFromSSID here. > Done.
https://codereview.chromium.org/1265453004/diff/20001/net/base/network_interf... File net/base/network_interfaces.h (right): https://codereview.chromium.org/1265453004/diff/20001/net/base/network_interf... net/base/network_interfaces.h:82: // Currently only implemented on Linux, ChromeOS, Android and OS_WIN. OS_WIN->Windows https://codereview.chromium.org/1265453004/diff/20001/net/base/network_interf... File net/base/network_interfaces_win.cc (right): https://codereview.chromium.org/1265453004/diff/20001/net/base/network_interf... net/base/network_interfaces_win.cc:205: bool GetConnectionAttributesPtr(WLAN_CONNECTION_ATTRIBUTES** conn_info_ptr) { Let's not give back a raw pointer that has explicit deletion semantics/requirements. How about either taking a scoped_ptr* argument or preferably just returning a scoped_ptr? See http://crbug.com/479898 https://codereview.chromium.org/1265453004/diff/20001/net/base/network_interf... net/base/network_interfaces_win.cc:249: if (result != ERROR_SUCCESS) how about combining these lines into "return result == ERROR_SUCCESS" https://codereview.chromium.org/1265453004/diff/20001/net/base/network_interf... net/base/network_interfaces_win.cc:256: WLAN_CONNECTION_ATTRIBUTES* conn_info_ptr = NULL; nullptr? https://codereview.chromium.org/1265453004/diff/20001/net/base/network_interf... net/base/network_interfaces_win.cc:258: DCHECK(!conn_info_ptr); I don't think WlanQueryInterface is guaranteed to not set the ppData argument when it returns something other than ERROR_SUCCESS. Can we remove this DCHECK and the one on line 345? https://codereview.chromium.org/1265453004/diff/20001/net/base/network_interf... net/base/network_interfaces_win.cc:261: DCHECK(conn_info_ptr); Can we move this into GetConnectionAttributesPtr? And the DCHECK on line 348? https://codereview.chromium.org/1265453004/diff/20001/net/base/network_interf... net/base/network_interfaces_win.cc:343: WLAN_CONNECTION_ATTRIBUTES* conn_info_ptr = NULL; nullptr?
PTAL. https://codereview.chromium.org/1265453004/diff/20001/net/base/network_interf... File net/base/network_interfaces.h (right): https://codereview.chromium.org/1265453004/diff/20001/net/base/network_interf... net/base/network_interfaces.h:82: // Currently only implemented on Linux, ChromeOS, Android and OS_WIN. On 2015/07/29 12:09:20, pauljensen wrote: > OS_WIN->Windows Done. Also, changed at couple of other places. https://codereview.chromium.org/1265453004/diff/20001/net/base/network_interf... File net/base/network_interfaces_win.cc (right): https://codereview.chromium.org/1265453004/diff/20001/net/base/network_interf... net/base/network_interfaces_win.cc:205: bool GetConnectionAttributesPtr(WLAN_CONNECTION_ATTRIBUTES** conn_info_ptr) { On 2015/07/29 12:09:20, pauljensen wrote: > Let's not give back a raw pointer that has explicit deletion > semantics/requirements. How about either taking a scoped_ptr* argument or > preferably just returning a scoped_ptr? See http://crbug.com/479898 IIUC, pointer to scoped_ptr is strongly discouraged. Quoting https://www.chromium.org/developers/smart-pointer-guidelines: "You should especially never see scoped_ptr<>*" Returning scoped_ptr now. https://codereview.chromium.org/1265453004/diff/20001/net/base/network_interf... net/base/network_interfaces_win.cc:249: if (result != ERROR_SUCCESS) On 2015/07/29 12:09:20, pauljensen wrote: > how about combining these lines into "return result == ERROR_SUCCESS" After moving DCHECK up, this becomes obsolete. https://codereview.chromium.org/1265453004/diff/20001/net/base/network_interf... net/base/network_interfaces_win.cc:256: WLAN_CONNECTION_ATTRIBUTES* conn_info_ptr = NULL; On 2015/07/29 12:09:20, pauljensen wrote: > nullptr? Done. https://codereview.chromium.org/1265453004/diff/20001/net/base/network_interf... net/base/network_interfaces_win.cc:258: DCHECK(!conn_info_ptr); On 2015/07/29 12:09:20, pauljensen wrote: > I don't think WlanQueryInterface is guaranteed to not set the ppData argument > when it returns something other than ERROR_SUCCESS. Can we remove this DCHECK > and the one on line 345? Done. https://codereview.chromium.org/1265453004/diff/20001/net/base/network_interf... net/base/network_interfaces_win.cc:261: DCHECK(conn_info_ptr); On 2015/07/29 12:09:20, pauljensen wrote: > Can we move this into GetConnectionAttributesPtr? And the DCHECK on line 348? Done. https://codereview.chromium.org/1265453004/diff/20001/net/base/network_interf... net/base/network_interfaces_win.cc:343: WLAN_CONNECTION_ATTRIBUTES* conn_info_ptr = NULL; On 2015/07/29 12:09:20, pauljensen wrote: > nullptr? Done.
https://codereview.chromium.org/1265453004/diff/40001/net/base/network_interf... File net/base/network_interfaces_win.cc (right): https://codereview.chromium.org/1265453004/diff/40001/net/base/network_interf... net/base/network_interfaces_win.cc:204: // returns them. How about removing |success| and just returning an empty scoped_ptr? https://codereview.chromium.org/1265453004/diff/40001/net/base/network_interf... net/base/network_interfaces_win.cc:213: new WLAN_CONNECTION_ATTRIBUTES()); eh why not just "scoped_ptr<blah,blah>()"? ditto for the other spots.
Patchset #4 (id:60001) has been deleted
PTAL. thanks. https://codereview.chromium.org/1265453004/diff/40001/net/base/network_interf... File net/base/network_interfaces_win.cc (right): https://codereview.chromium.org/1265453004/diff/40001/net/base/network_interf... net/base/network_interfaces_win.cc:204: // returns them. On 2015/07/30 13:47:40, pauljensen wrote: > How about removing |success| and just returning an empty scoped_ptr? Done. https://codereview.chromium.org/1265453004/diff/40001/net/base/network_interf... net/base/network_interfaces_win.cc:213: new WLAN_CONNECTION_ATTRIBUTES()); On 2015/07/30 13:47:40, pauljensen wrote: > eh why not just "scoped_ptr<blah,blah>()"? ditto for the other spots. Done.
lgtm with the three comments addressed. https://codereview.chromium.org/1265453004/diff/80001/net/base/network_interf... File net/base/network_interfaces_win.cc (right): https://codereview.chromium.org/1265453004/diff/80001/net/base/network_interf... net/base/network_interfaces_win.cc:206: GetConnectionAttributes() { Move this function up into the anonymous namespace. https://codereview.chromium.org/1265453004/diff/80001/net/base/network_interf... net/base/network_interfaces_win.cc:258: return conn_info.Pass(); Lets change these five lines to: DCHECK(conn_info_ptr); return scoped_ptr<WLAN_CONNECTION_ATTRIBUTES, internal::WlanApiDeleter>( conn_info_ptr); https://codereview.chromium.org/1265453004/diff/80001/net/base/network_interf... net/base/network_interfaces_win.cc:262: scoped_ptr<WLAN_CONNECTION_ATTRIBUTES, internal::WlanApiDeleter> conn_info = I think using auto here might be nice as the type is rather unwieldy. https://codereview.chromium.org/1265453004/diff/80001/net/base/network_interf... net/base/network_interfaces_win.cc:346: GetConnectionAttributes(); ditto
Addressed comments. Tested on Win 7. Sending to CQ now. Thanks for reviewing this. https://codereview.chromium.org/1265453004/diff/80001/net/base/network_interf... File net/base/network_interfaces_win.cc (right): https://codereview.chromium.org/1265453004/diff/80001/net/base/network_interf... net/base/network_interfaces_win.cc:206: GetConnectionAttributes() { On 2015/07/31 12:59:42, pauljensen wrote: > Move this function up into the anonymous namespace. Done. https://codereview.chromium.org/1265453004/diff/80001/net/base/network_interf... net/base/network_interfaces_win.cc:258: return conn_info.Pass(); On 2015/07/31 12:59:42, pauljensen wrote: > Lets change these five lines to: > DCHECK(conn_info_ptr); > return scoped_ptr<WLAN_CONNECTION_ATTRIBUTES, internal::WlanApiDeleter>( > conn_info_ptr); Done. https://codereview.chromium.org/1265453004/diff/80001/net/base/network_interf... net/base/network_interfaces_win.cc:262: scoped_ptr<WLAN_CONNECTION_ATTRIBUTES, internal::WlanApiDeleter> conn_info = On 2015/07/31 12:59:42, pauljensen wrote: > I think using auto here might be nice as the type is rather unwieldy. Done. https://codereview.chromium.org/1265453004/diff/80001/net/base/network_interf... net/base/network_interfaces_win.cc:346: GetConnectionAttributes(); On 2015/07/31 12:59:42, pauljensen wrote: > ditto Done.
The CQ bit was checked by tbansal@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pauljensen@chromium.org Link to the patchset: https://codereview.chromium.org/1265453004/#ps100001 (title: "Addressed comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1265453004/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1265453004/100001
Message was sent while issue was closed.
Committed patchset #5 (id:100001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/85da67aea5f72baf4cec5fd87d09c7b038465e7f Cr-Commit-Position: refs/heads/master@{#341405} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
