Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
64683014:
Mac OS X-specific implementation of Networking Private API. (Closed)
|
Created:
7 years, 1 month ago by mef Modified:
6 years, 10 months ago CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionMac OS X-specific implementation of Networking Private API.
Based on infrastructure in http://crrev.com/54323003
BUG=330255
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=247659
Patch Set 1 #Patch Set 2 : Complete basic implementation of WiFiService on OS X. #Patch Set 3 : Made networkingPrivateApi available on OS X. #Patch Set 4 : Explicitly load CoreWLAN and retain CWInterface. #Patch Set 5 : First successful setup run. #Patch Set 6 : Sync to r240704 #Patch Set 7 : GetProperties now use |networks_| information cached by GetVisibleNetworks. #Patch Set 8 : Use Reachability to determine when WiFi network connection is complete. #Patch Set 9 : Fix compilation errors. Add WLANListener. #Patch Set 10 : Fix linker error. #Patch Set 11 : Fix Win compilation error. #Patch Set 12 : Better comments. #
Total comments: 16
Patch Set 13 : Address stevenjb's comments. #
Total comments: 4
Patch Set 14 : Address stevenjb's comment. #
Total comments: 109
Patch Set 15 : Address codereview comments. #
Total comments: 14
Patch Set 16 : Report connection_state as kConnecting until network is reachable. #Patch Set 17 : Address Toni's comments. #Patch Set 18 : In my testing this query doesn't return any items: #Patch Set 19 : Remove ScopedNSAutoreleasePool. #
Total comments: 64
Patch Set 20 : Address codereview comments. #Patch Set 21 : Address Toni's comments. #
Total comments: 16
Patch Set 22 : Address Roberts's comments. #
Total comments: 2
Patch Set 23 : Added NetworkChangeDetector that listens to NetworkChangeNotifier. #
Total comments: 8
Patch Set 24 : Explicitly track connected_network_guid and use it to notify network disconnect. #
Total comments: 16
Patch Set 25 : Address codereview comments. #
Total comments: 4
Patch Set 26 : Address Toni's comments. #Patch Set 27 : Observe OnNetworkChanged in NPServiceClient and post them to WiFiService. #
Total comments: 22
Patch Set 28 : Address Toni's comments. #Patch Set 29 : Fix trybot compilation error. #
Total comments: 4
Patch Set 30 : Address codereview comments. #Messages
Total messages: 43 (0 generated)
Hi, whenever you have a change (probably after New Year), please take a look at Mac OS X-specific implementation of Networking Private API to support Chromecast setup in Chrome Google Cast extension... tbarzic@ - overall. stevenjb@ - overall, networking_private*. arw@ - Mac OS X specifics. afontan@ - FYI. thanks, -m
https://codereview.chromium.org/64683014/diff/500001/chrome/browser/extension... File chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc (right): https://codereview.chromium.org/64683014/diff/500001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc:397: const std::string& error_name, const std::string& error) { one arg per line https://codereview.chromium.org/64683014/diff/500001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc:450: const std::string& error_name, const std::string& error) { one arg per line https://codereview.chromium.org/64683014/diff/500001/components/wifi/wifi_ser... File components/wifi/wifi_service_mac.mm (right): https://codereview.chromium.org/64683014/diff/500001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:367: if (network != nil) { if (network == nil) { *error = "Error.NotFound"; return; } https://codereview.chromium.org/64683014/diff/500001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:393: for (int i = 0; i < 3; ++i) { Avoid magic numbers; make 3 a constant and comment why 3. Maybe posting a delayed task would be a better approach? i.e. add a WaitForNetworkAssociate method similar to WaitForNetworkConnect? https://codereview.chromium.org/64683014/diff/500001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:429: //[interface_ setPower:YES error:nil]; Remove (or comment better). https://codereview.chromium.org/64683014/diff/500001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:510: std::map<std::string, NetworkProperties&> network_guids; Avoid using a & in a map, it's hard to read; use a * instead. Also, name this something like network_property_map. https://codereview.chromium.org/64683014/diff/500001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:521: network_properties.guid, network_list->back())); network_guids[network_properties.guid] = &network_list->back(); https://codereview.chromium.org/64683014/diff/500001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:585: default: nit: Omit the default so that the compiler will catch any missing modes.
Thanks, Steven! https://codereview.chromium.org/64683014/diff/500001/chrome/browser/extension... File chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc (right): https://codereview.chromium.org/64683014/diff/500001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc:397: const std::string& error_name, const std::string& error) { On 2013/12/27 19:53:44, stevenjb wrote: > one arg per line Done. https://codereview.chromium.org/64683014/diff/500001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc:450: const std::string& error_name, const std::string& error) { On 2013/12/27 19:53:44, stevenjb wrote: > one arg per line Done. https://codereview.chromium.org/64683014/diff/500001/components/wifi/wifi_ser... File components/wifi/wifi_service_mac.mm (right): https://codereview.chromium.org/64683014/diff/500001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:367: if (network != nil) { On 2013/12/27 19:53:44, stevenjb wrote: > if (network == nil) { > *error = "Error.NotFound"; > return; > } Done. https://codereview.chromium.org/64683014/diff/500001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:393: for (int i = 0; i < 3; ++i) { On 2013/12/27 19:53:44, stevenjb wrote: > Avoid magic numbers; make 3 a constant and comment why 3. Maybe posting a > delayed task would be a better approach? i.e. add a WaitForNetworkAssociate > method similar to WaitForNetworkConnect? I've 'borrowed' this from ChromeCast setup app (AssociateToNetworkAsyncOperation.m:204). I don't think posting task is needed here as there is no delay between retries. I've added a constant and a comment. https://codereview.chromium.org/64683014/diff/500001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:429: //[interface_ setPower:YES error:nil]; On 2013/12/27 19:53:44, stevenjb wrote: > Remove (or comment better). Done. https://codereview.chromium.org/64683014/diff/500001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:510: std::map<std::string, NetworkProperties&> network_guids; On 2013/12/27 19:53:44, stevenjb wrote: > Avoid using a & in a map, it's hard to read; use a * instead. Also, name this > something like network_property_map. Done. https://codereview.chromium.org/64683014/diff/500001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:521: network_properties.guid, network_list->back())); On 2013/12/27 19:53:44, stevenjb wrote: > network_guids[network_properties.guid] = &network_list->back(); Done. https://codereview.chromium.org/64683014/diff/500001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:585: default: On 2013/12/27 19:53:44, stevenjb wrote: > nit: Omit the default so that the compiler will catch any missing modes. Done.
Non Mac specific bits lgtm https://codereview.chromium.org/64683014/diff/560001/components/wifi/wifi_ser... File components/wifi/wifi_service_mac.mm (right): https://codereview.chromium.org/64683014/diff/560001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:217: static const int kMaxAssociationAttempts = 3; Since this is only used once, define this in the function that uses it, right below the comment explaining why we retry.
Steven, thanks! Everybody else: please take a look. https://codereview.chromium.org/64683014/diff/560001/components/wifi/wifi_ser... File components/wifi/wifi_service_mac.mm (right): https://codereview.chromium.org/64683014/diff/560001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:217: static const int kMaxAssociationAttempts = 3; On 2013/12/27 21:22:05, stevenjb wrote: > Since this is only used once, define this in the function that uses it, right > below the comment explaining why we retry. Done.
lgtm Took a look at the Mac portion. Looks good, just one small comment. https://codereview.chromium.org/64683014/diff/560001/components/wifi/wifi_ser... File components/wifi/wifi_service_mac.mm (right): https://codereview.chromium.org/64683014/diff/560001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:374: std::string connected_network_guid = GUIDFromSSID([interface_ ssid]); Can this check and return logic be moved higher up? Seems like if we're already connected to the desired network, there should be no reason to perform the network scan above to resolve network_guid to a CWNetwork. I'm going to guess something depends on this... but does NotifyNetworkChanged need to be called if the network hasn't actually changed?
Drive-by Mac internals review. https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... File components/wifi/wifi_service_mac.mm (right): https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:13: #import <netinet/in.h> These headers come below the .h for the .mm and before other project includes. https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:19: const char* kCWSSIDDidChangeNotification_chrome = kCWSSIDDidChangeNotification is marked as available since 10.6? https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:29: - (BOOL)associateToNetwork:(CWNetwork *)network nit: here and throughout, there's no space between typename and the *; just |CFNetwork*| https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:37: kCWChannelBandUnknown = 0, nit: only indent 2 spaces https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:75: [[NSNotificationCenter defaultCenter] You can remove this entire ObjC class if you use the block-based NSNotificationCenter API. https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:93: class WiFiServiceImpl : public WiFiService { Why not call this WiFiServiceMac ? https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:144: bool CheckError(NSError* nsError, naming: In C++, variables do not use camelCase. https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:149: NSString* SSIDFromGUID(const std::string& network_guid) const { I think you can remove this and the method below entirely if you use base/strings/sys_string_conversions.h. https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:226: // As the WLAN api binding runs on its own thread, we need to provide our own I don't understand this. What's backing this SequencedTaskRunner and why isn't it providing the autorelease pool automatically (for reference, SequencedWorkerPool does)? https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:235: initWithPath:@"/System/Library/Frameworks/CoreWLAN.framework"]); Why do you dynamically load CoreWLAN? https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:248: WLANNotificationObserver* observer = [[WLANNotificationObserver alloc] init]; Put this in the scoper to start with, rather than .reset()ing below. https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:266: NSError* nsError = GetVisibleNetworkList(std::string(), &networks_); naming: nsError https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:293: *error = "Error.NotImplemented"; Are these going to be implemented? https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:327: DVLOG(1) << __FUNCTION__; Remove unnecessary logging; here and throughout. https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:347: NSError* nsError = GetVisibleNetworkList(std::string(), &networks); naming: nsError. Here and throughout. https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:363: if (CheckError(nsError, "Error.scanForNetworksWithName", error)) Should these errors be made into constants? https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:382: NSString* nsPassword = nil; naming https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:387: nsPassword = [NSString stringWithUTF8String:passphrase.c_str()]; base/strings/sys_string_conversions.h https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:401: if ([interface_ associateToNetwork:network What if this is running on 10.6? This method is only declared on 10.7+. It seems that this method used to be |- (BOOL)associateToNetwork:(CWNetwork*)network parameters:(NSDictionary*)parameters error:(NSError**)error| in 10.6 https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:413: long error_code = [nsError code]; long -> NSInteger https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:432: *error = "not-connected"; Why doesn't this error start with |Error.|? https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:459: struct sockaddr_in localWiFiAddress; naming https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:465: SCNetworkReachabilityRef reachability = Use a ScopedCFTypeRef. https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:468: (const struct sockaddr*)&localWiFiAddress); C-style casts are banned. https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:498: NSString* networkName = nil; naming https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:515: for(network in networks) { nit: space before ( https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:549: if (NSOrderedSame == [[network ssid] compare:[interface_ ssid]]) Preference is for |expr == NSOrderedSame| http://dev.chromium.org/developers/coding-style#Code_Formatting https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:554: properties->ssid = [[network ssid] UTF8String]; sys_string_conversions.h. This looks like you're assigning a weak C-string pointer to something that will outlive this function. https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:560: if ([[network wlanChannel] channelBand] == kCWChannelBand2GHz) Again, what if this is running on a 10.6 machine? https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:592: void WiFiServiceImpl::SortNetworks(NetworkList* networks) { This is called exactly once. Why does it need its own method? https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:596: nit: double blank line https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:616: if (network_list_changed_observer_.is_null()) Why listen for network change events if nothing is going to use that data? https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:632: if (!enable_notify_network_changed_ || networks_changed_observer_.is_null()) Same question. https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:641: "// static"
https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... File components/wifi/wifi_service_mac.mm (right): https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:52: // Observe CoreWLAN notifications and call |WiFiServiceImpl| on worker thread. This seems to observe only "connected network ssid" changes, right? Can you add a comment about this. https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:177: // Ethernet, WiFi, Cellular, VPN missing . at the end of the comment https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:180: // Handle notification from |WLANListener|; Hm, I can't see anything named |WLANListener| here. Did you mean |wlan_observer_| https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:213: static const int kMaxAttempts = 200; the name could be more descriptive (e.g. kMaxConnectionAttempts) https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:392: // have valid IP address). This seems wrong.. what if another network gets connected while we're waiting for this one to become accessible? Also, the network might change by means other than this API, and this suggests that in these they would erroneously be reported as "Connected". Can we just set the network status to "Connecting" if it's 'connected' but not 'accessible'? https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:407: // Start waiting for network connection state change. Add a comment that WaiForNetworkConnect is async and that it'll reset enable_notify_network_changed_ https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:430: NotifyNetworkChanged(network_guid); Shouldn't this be reported from |OnListenerNotification| https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:601: // Find previously connected network and notify that it is disconnected. When a connected network changes from A to B, will there always be a notification with empty interface_ ssid? If not, we may want to do this even if |connected_network_guid| is not empty.
Thanks a lot for your comments, I've addressed most of them, and added follow-up questions to remaining items, PTAL. Robert, thanks a lot for thorough review, I appreciate that. I'm not an everyday Mac OS X developer, and this is my first Mac-specific change in Chrome, so I apologize for some missteps. Could you suggest a proper way to deal with CoreWLAN loading (should I link to it or load dynamically?) and 10.6-10.7 API changes. thanks, -m https://codereview.chromium.org/64683014/diff/560001/components/wifi/wifi_ser... File components/wifi/wifi_service_mac.mm (right): https://codereview.chromium.org/64683014/diff/560001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:374: std::string connected_network_guid = GUIDFromSSID([interface_ ssid]); On 2014/01/06 23:08:54, arw wrote: > Can this check and return logic be moved higher up? Seems like if we're already > connected to the desired network, there should be no reason to perform the > network scan above to resolve network_guid to a CWNetwork. > > I'm going to guess something depends on this... but does NotifyNetworkChanged > need to be called if the network hasn't actually changed? Done. https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... File components/wifi/wifi_service_mac.mm (right): https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:13: #import <netinet/in.h> On 2014/01/07 16:19:05, rsesek wrote: > These headers come below the .h for the .mm and before other project includes. Done. https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:19: const char* kCWSSIDDidChangeNotification_chrome = On 2014/01/07 16:19:05, rsesek wrote: > kCWSSIDDidChangeNotification is marked as available since 10.6? Is it ok to link to CoreWLAN.framework, or should it be loaded dynamically via dlopen? https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:29: - (BOOL)associateToNetwork:(CWNetwork *)network On 2014/01/07 16:19:05, rsesek wrote: > nit: here and throughout, there's no space between typename and the *; just > |CFNetwork*| Done. https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:37: kCWChannelBandUnknown = 0, On 2014/01/07 16:19:05, rsesek wrote: > nit: only indent 2 spaces Done. https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:52: // Observe CoreWLAN notifications and call |WiFiServiceImpl| on worker thread. On 2014/01/07 20:05:33, tbarzic wrote: > This seems to observe only "connected network ssid" changes, right? > Can you add a comment about this. Done. https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:75: [[NSNotificationCenter defaultCenter] On 2014/01/07 16:19:05, rsesek wrote: > You can remove this entire ObjC class if you use the block-based > NSNotificationCenter API. Done. https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:93: class WiFiServiceImpl : public WiFiService { On 2014/01/07 16:19:05, rsesek wrote: > Why not call this WiFiServiceMac ? Done. https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:144: bool CheckError(NSError* nsError, On 2014/01/07 16:19:05, rsesek wrote: > naming: In C++, variables do not use camelCase. Done. https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:149: NSString* SSIDFromGUID(const std::string& network_guid) const { On 2014/01/07 16:19:05, rsesek wrote: > I think you can remove this and the method below entirely if you use > base/strings/sys_string_conversions.h. I'd like to have explicit SSID / GUID convertors in case if we decide that GUID shouldn't be equal to SSID. https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:177: // Ethernet, WiFi, Cellular, VPN On 2014/01/07 20:05:33, tbarzic wrote: > missing . at the end of the comment Done. https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:180: // Handle notification from |WLANListener|; On 2014/01/07 20:05:33, tbarzic wrote: > Hm, I can't see anything named |WLANListener| here. Did you mean > |wlan_observer_| Done. https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:213: static const int kMaxAttempts = 200; On 2014/01/07 20:05:33, tbarzic wrote: > the name could be more descriptive (e.g. kMaxConnectionAttempts) Done. https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:226: // As the WLAN api binding runs on its own thread, we need to provide our own On 2014/01/07 16:19:05, rsesek wrote: > I don't understand this. What's backing this SequencedTaskRunner and why isn't > it providing the autorelease pool automatically (for reference, > SequencedWorkerPool does)? I'm not sure I understand the question, could you elaborate? I've copied this comment from https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/ge... https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:235: initWithPath:@"/System/Library/Frameworks/CoreWLAN.framework"]); On 2014/01/07 16:19:05, rsesek wrote: > Why do you dynamically load CoreWLAN? To mimic geolocation code, but I guess we can actually link it directly as we don't support 10.5 any more, right? https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:248: WLANNotificationObserver* observer = [[WLANNotificationObserver alloc] init]; On 2014/01/07 16:19:05, rsesek wrote: > Put this in the scoper to start with, rather than .reset()ing below. Done. https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:266: NSError* nsError = GetVisibleNetworkList(std::string(), &networks_); On 2014/01/07 16:19:05, rsesek wrote: > naming: nsError Done. https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:293: *error = "Error.NotImplemented"; On 2014/01/07 16:19:05, rsesek wrote: > Are these going to be implemented? At some point, not in current release. https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:327: DVLOG(1) << __FUNCTION__; On 2014/01/07 16:19:05, rsesek wrote: > Remove unnecessary logging; here and throughout. Why? https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:347: NSError* nsError = GetVisibleNetworkList(std::string(), &networks); On 2014/01/07 16:19:05, rsesek wrote: > naming: nsError. Here and throughout. Done. https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:363: if (CheckError(nsError, "Error.scanForNetworksWithName", error)) On 2014/01/07 16:19:05, rsesek wrote: > Should these errors be made into constants? Done. https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:382: NSString* nsPassword = nil; On 2014/01/07 16:19:05, rsesek wrote: > naming Done. https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:387: nsPassword = [NSString stringWithUTF8String:passphrase.c_str()]; On 2014/01/07 16:19:05, rsesek wrote: > base/strings/sys_string_conversions.h Done. https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:392: // have valid IP address). On 2014/01/07 20:05:33, tbarzic wrote: > This seems wrong.. > what if another network gets connected while we're waiting for this one to > become accessible? > > Also, the network might change by means other than this API, and this suggests > that in these they would erroneously be reported as "Connected". Can we just set > the network status to "Connecting" if it's 'connected' but not 'accessible'? I think I should change 'GetProperties' to check reachability of network [interface_ ssid], and report state as 'Connecting' if network is not reachable yet. Does this make sense? https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:401: if ([interface_ associateToNetwork:network On 2014/01/07 16:19:05, rsesek wrote: > What if this is running on 10.6? This method is only declared on 10.7+. > > It seems that this method used to be |- > (BOOL)associateToNetwork:(CWNetwork*)network > parameters:(NSDictionary*)parameters error:(NSError**)error| in 10.6 Couple of followup questions: - What is suggested way of dealing with API differences? - Where can I find a 10.6 API documentation? https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:407: // Start waiting for network connection state change. On 2014/01/07 20:05:33, tbarzic wrote: > Add a comment that WaiForNetworkConnect is async and that it'll reset > enable_notify_network_changed_ Done. https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:413: long error_code = [nsError code]; On 2014/01/07 16:19:05, rsesek wrote: > long -> NSInteger Done. https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:430: NotifyNetworkChanged(network_guid); On 2014/01/07 20:05:33, tbarzic wrote: > Shouldn't this be reported from |OnListenerNotification| Done. https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:432: *error = "not-connected"; On 2014/01/07 16:19:05, rsesek wrote: > Why doesn't this error start with |Error.|? Done. https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:459: struct sockaddr_in localWiFiAddress; On 2014/01/07 16:19:05, rsesek wrote: > naming Done. https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:465: SCNetworkReachabilityRef reachability = On 2014/01/07 16:19:05, rsesek wrote: > Use a ScopedCFTypeRef. Done. https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:468: (const struct sockaddr*)&localWiFiAddress); On 2014/01/07 16:19:05, rsesek wrote: > C-style casts are banned. Done. https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:498: NSString* networkName = nil; On 2014/01/07 16:19:05, rsesek wrote: > naming Done. https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:515: for(network in networks) { On 2014/01/07 16:19:05, rsesek wrote: > nit: space before ( Done. https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:549: if (NSOrderedSame == [[network ssid] compare:[interface_ ssid]]) On 2014/01/07 16:19:05, rsesek wrote: > Preference is for |expr == NSOrderedSame| > > http://dev.chromium.org/developers/coding-style#Code_Formatting Done. https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:554: properties->ssid = [[network ssid] UTF8String]; On 2014/01/07 16:19:05, rsesek wrote: > sys_string_conversions.h. This looks like you're assigning a weak C-string > pointer to something that will outlive this function. |ssid| and other properties are std::string. https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:560: if ([[network wlanChannel] channelBand] == kCWChannelBand2GHz) On 2014/01/07 16:19:05, rsesek wrote: > Again, what if this is running on a 10.6 machine? Is there some other way to get this info on 10.6 machine? I especially need bssid, but band would also be nice to have. https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:592: void WiFiServiceImpl::SortNetworks(NetworkList* networks) { On 2014/01/07 16:19:05, rsesek wrote: > This is called exactly once. Why does it need its own method? Done. https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:596: On 2014/01/07 16:19:05, rsesek wrote: > nit: double blank line Done. https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:601: // Find previously connected network and notify that it is disconnected. On 2014/01/07 20:05:33, tbarzic wrote: > When a connected network changes from A to B, will there always be a > notification with empty interface_ ssid? > If not, we may want to do this even if |connected_network_guid| is not empty. In my testing it appears to always get notification for disconnect, so [interface_ ssid] is empty first. https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:616: if (network_list_changed_observer_.is_null()) On 2014/01/07 16:19:05, rsesek wrote: > Why listen for network change events if nothing is going to use that data? Because caller could've done |RequestNetworkScan| without setting |network_list_changed_observer_|. https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:632: if (!enable_notify_network_changed_ || networks_changed_observer_.is_null()) On 2014/01/07 16:19:05, rsesek wrote: > Same question. Done. I've moved 'NSNotificationCenter addObserverForName' into |WiFiServiceMac::SetEventObservers|. https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:641: On 2014/01/07 16:19:05, rsesek wrote: > "// static" Done.
https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... File components/wifi/wifi_service_mac.mm (right): https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:392: // have valid IP address). On 2014/01/08 21:00:03, mef wrote: > On 2014/01/07 20:05:33, tbarzic wrote: > > This seems wrong.. > > what if another network gets connected while we're waiting for this one to > > become accessible? > > > > Also, the network might change by means other than this API, and this suggests > > that in these they would erroneously be reported as "Connected". Can we just > set > > the network status to "Connecting" if it's 'connected' but not 'accessible'? > I think I should change 'GetProperties' to check reachability of network > [interface_ ssid], and report state as 'Connecting' if network is not reachable > yet. Does this make sense? sounds good https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:601: // Find previously connected network and notify that it is disconnected. On 2014/01/08 21:00:03, mef wrote: > On 2014/01/07 20:05:33, tbarzic wrote: > > When a connected network changes from A to B, will there always be a > > notification with empty interface_ ssid? > > If not, we may want to do this even if |connected_network_guid| is not empty. > In my testing it appears to always get notification for disconnect, so > [interface_ ssid] is empty first. You don't seem too sure about this :) Can you add a comment? https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:616: if (network_list_changed_observer_.is_null()) On 2014/01/08 21:00:03, mef wrote: > On 2014/01/07 16:19:05, rsesek wrote: > > Why listen for network change events if nothing is going to use that data? > Because caller could've done |RequestNetworkScan| without setting > |network_list_changed_observer_|. I agree that there is no real need to observe network changes if there are no listeners. https://codereview.chromium.org/64683014/diff/740001/components/wifi/wifi_ser... File components/wifi/wifi_service_mac.mm (right): https://codereview.chromium.org/64683014/diff/740001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:1: // Copyright 2013 The Chromium Authors. All rights reserved. the copyright header is out of date :P https://codereview.chromium.org/64683014/diff/740001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:145: // Handle notification from |wlan_observer_|; end comment with . instead of ; Here and throughout: Method comments should be descriptive rather than imperative. https://codereview.chromium.org/64683014/diff/740001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:224: if (networks_.empty()) { I don't think this block is needed (at least if RequestNetworkScan is changed to update networks_). If a network is not in networks_, the extension using the API should not know about it. https://codereview.chromium.org/64683014/diff/740001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:234: if (it->guid == network_guid) { the network's properties should probably be rescaned here, to make sure the extension gets up to date information. https://codereview.chromium.org/64683014/diff/740001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:466: NSError* WiFiServiceMac::GetVisibleNetworkList(const std::string& network_guid, Instead of returning network_list here, why don't you rename the method to |UpdateNetworks| and update networks_ (and trigger network list changed event). If |network_guid| is set, you could only update the properties for said network (and add the network to the list if needed). https://codereview.chromium.org/64683014/diff/740001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:487: for (network in networks) { should network_list be cleared before the loop? https://codereview.chromium.org/64683014/diff/740001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:578: NotifyNetworkChanged(connected_network_guid); You should make sure that networks_ contains the connected network, and if it does not, update the network list before triggering this event. It would be ok to scan for the connected network only (and add it to network list).
Thanks Toni, PTAL! https://codereview.chromium.org/64683014/diff/740001/components/wifi/wifi_ser... File components/wifi/wifi_service_mac.mm (right): https://codereview.chromium.org/64683014/diff/740001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:1: // Copyright 2013 The Chromium Authors. All rights reserved. On 2014/01/08 23:06:23, tbarzic wrote: > the copyright header is out of date :P Done. https://codereview.chromium.org/64683014/diff/740001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:145: // Handle notification from |wlan_observer_|; On 2014/01/08 23:06:23, tbarzic wrote: > end comment with . instead of ; > > Here and throughout: Method comments should be descriptive rather than > imperative. Done. https://codereview.chromium.org/64683014/diff/740001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:224: if (networks_.empty()) { On 2014/01/08 23:06:23, tbarzic wrote: > I don't think this block is needed (at least if RequestNetworkScan is changed to > update networks_). If a network is not in networks_, the extension using the API > should not know about it. Done. https://codereview.chromium.org/64683014/diff/740001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:234: if (it->guid == network_guid) { On 2014/01/08 23:06:23, tbarzic wrote: > the network's properties should probably be rescaned here, to make sure the > extension gets up to date information. > Done. https://codereview.chromium.org/64683014/diff/740001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:466: NSError* WiFiServiceMac::GetVisibleNetworkList(const std::string& network_guid, On 2014/01/08 23:06:23, tbarzic wrote: > Instead of returning network_list here, why don't you rename the method to > |UpdateNetworks| and update networks_ (and trigger network list changed event). > > If |network_guid| is set, you could only update the properties for said network > (and add the network to the list if needed). > Done. https://codereview.chromium.org/64683014/diff/740001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:487: for (network in networks) { On 2014/01/08 23:06:23, tbarzic wrote: > should network_list be cleared before the loop? Done. https://codereview.chromium.org/64683014/diff/740001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:578: NotifyNetworkChanged(connected_network_guid); On 2014/01/08 23:06:23, tbarzic wrote: > You should make sure that networks_ contains the connected network, and if it > does not, update the network list before triggering this event. It would be ok > to scan for the connected network only (and add it to network list). Done.
I'm sorry I didn't get to do a full pass today, but I will tomorrow. Answers to questions at least! https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... File components/wifi/wifi_service_mac.mm (right): https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:19: const char* kCWSSIDDidChangeNotification_chrome = On 2014/01/08 21:00:03, mef wrote: > On 2014/01/07 16:19:05, rsesek wrote: > > kCWSSIDDidChangeNotification is marked as available since 10.6? > Is it ok to link to CoreWLAN.framework, or should it be loaded dynamically via > dlopen? I think it's fine to link it directly in the link_settings. https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:226: // As the WLAN api binding runs on its own thread, we need to provide our own On 2014/01/08 21:00:03, mef wrote: > On 2014/01/07 16:19:05, rsesek wrote: > > I don't understand this. What's backing this SequencedTaskRunner and why isn't > > it providing the autorelease pool automatically (for reference, > > SequencedWorkerPool does)? > > I'm not sure I understand the question, could you elaborate? I've copied this > comment from > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/ge... I don't think you need to supply your own autorelease pool at all in this CL, since the thread running the MessageLoop should be doing that for you. Why did you add this in the first place? Were you seeing console errors being printed around objects being leaked because no pool was in place? Unless you're running your own base::Thread, there should be an autorelease pool around the main event loop automatically. https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:235: initWithPath:@"/System/Library/Frameworks/CoreWLAN.framework"]); On 2014/01/08 21:00:03, mef wrote: > On 2014/01/07 16:19:05, rsesek wrote: > > Why do you dynamically load CoreWLAN? > To mimic geolocation code, but I guess we can actually link it directly as we > don't support 10.5 any more, right? Yes. I think so. https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:327: DVLOG(1) << __FUNCTION__; On 2014/01/08 21:00:03, mef wrote: > On 2014/01/07 16:19:05, rsesek wrote: > > Remove unnecessary logging; here and throughout. > Why? It needlessly clutters the code. https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:401: if ([interface_ associateToNetwork:network On 2014/01/08 21:00:03, mef wrote: > On 2014/01/07 16:19:05, rsesek wrote: > > What if this is running on 10.6? This method is only declared on 10.7+. > > > > It seems that this method used to be |- > > (BOOL)associateToNetwork:(CWNetwork*)network > > parameters:(NSDictionary*)parameters error:(NSError**)error| in 10.6 > Couple of followup questions: > - What is suggested way of dealing with API differences? > - Where can I find a 10.6 API documentation? This is the first time I've ever seen Apple actually _remove_ deprecated methods from the API headers, which is really weird. Typically you do this using: if ([obj respondsToSelector:@selector(methodName:withArg:arg2:)] { [obj methodName:…]; } I don't know how to dig up old API documentation, but I can provide you a link to the old 10.6 SDK where you can look at the header comments. https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:554: properties->ssid = [[network ssid] UTF8String]; On 2014/01/08 21:00:03, mef wrote: > On 2014/01/07 16:19:05, rsesek wrote: > > sys_string_conversions.h. This looks like you're assigning a weak C-string > > pointer to something that will outlive this function. > |ssid| and other properties are std::string. Yes, but I had to look that up. It looks like you're assigning to a const char*. https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:560: if ([[network wlanChannel] channelBand] == kCWChannelBand2GHz) On 2014/01/08 21:00:03, mef wrote: > On 2014/01/07 16:19:05, rsesek wrote: > > Again, what if this is running on a 10.6 machine? > Is there some other way to get this info on 10.6 machine? > I especially need bssid, but band would also be nice to have. That I do not know, unfortunately :|.
Thanks, PTAL. https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... File components/wifi/wifi_service_mac.mm (right): https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:19: const char* kCWSSIDDidChangeNotification_chrome = On 2014/01/09 23:12:45, rsesek wrote: > On 2014/01/08 21:00:03, mef wrote: > > On 2014/01/07 16:19:05, rsesek wrote: > > > kCWSSIDDidChangeNotification is marked as available since 10.6? > > Is it ok to link to CoreWLAN.framework, or should it be loaded dynamically via > > dlopen? > > I think it's fine to link it directly in the link_settings. Done. https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:226: // As the WLAN api binding runs on its own thread, we need to provide our own On 2014/01/09 23:12:45, rsesek wrote: > On 2014/01/08 21:00:03, mef wrote: > > On 2014/01/07 16:19:05, rsesek wrote: > > > I don't understand this. What's backing this SequencedTaskRunner and why > isn't > > > it providing the autorelease pool automatically (for reference, > > > SequencedWorkerPool does)? > > > > I'm not sure I understand the question, could you elaborate? I've copied this > > comment from > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/ge... > > I don't think you need to supply your own autorelease pool at all in this CL, > since the thread running the MessageLoop should be doing that for you. Why did > you add this in the first place? Were you seeing console errors being printed > around objects being leaked because no pool was in place? Unless you're running > your own base::Thread, there should be an autorelease pool around the main event > loop automatically. FYI: WiFiService calls are posted from UI thread to sequenced task runner on worker pool, so they run on random worker threads. I didn't observe any errors, but adding local autorelease pool seemed to make sense. I'll be happy to remove it though if its not necessary. Please advise. https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:235: initWithPath:@"/System/Library/Frameworks/CoreWLAN.framework"]); On 2014/01/09 23:12:45, rsesek wrote: > On 2014/01/08 21:00:03, mef wrote: > > On 2014/01/07 16:19:05, rsesek wrote: > > > Why do you dynamically load CoreWLAN? > > To mimic geolocation code, but I guess we can actually link it directly as we > > don't support 10.5 any more, right? > > Yes. I think so. Done. https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:401: if ([interface_ associateToNetwork:network On 2014/01/09 23:12:45, rsesek wrote: > On 2014/01/08 21:00:03, mef wrote: > > On 2014/01/07 16:19:05, rsesek wrote: > > > What if this is running on 10.6? This method is only declared on 10.7+. > > > > > > It seems that this method used to be |- > > > (BOOL)associateToNetwork:(CWNetwork*)network > > > parameters:(NSDictionary*)parameters error:(NSError**)error| in 10.6 > > Couple of followup questions: > > - What is suggested way of dealing with API differences? > > - Where can I find a 10.6 API documentation? > > > This is the first time I've ever seen Apple actually _remove_ deprecated methods > from the API headers, which is really weird. Typically you do this using: > > if ([obj respondsToSelector:@selector(methodName:withArg:arg2:)] { > [obj methodName:…]; > } > > I don't know how to dig up old API documentation, but I can provide you a link > to the old 10.6 SDK where you can look at the header comments. Done. https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:554: properties->ssid = [[network ssid] UTF8String]; On 2014/01/09 23:12:45, rsesek wrote: > On 2014/01/08 21:00:03, mef wrote: > > On 2014/01/07 16:19:05, rsesek wrote: > > > sys_string_conversions.h. This looks like you're assigning a weak C-string > > > pointer to something that will outlive this function. > > |ssid| and other properties are std::string. > > Yes, but I had to look that up. It looks like you're assigning to a const char*. Any suggestions on making it more apparent? https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:560: if ([[network wlanChannel] channelBand] == kCWChannelBand2GHz) On 2014/01/09 23:12:45, rsesek wrote: > On 2014/01/08 21:00:03, mef wrote: > > On 2014/01/07 16:19:05, rsesek wrote: > > > Again, what if this is running on a 10.6 machine? > > Is there some other way to get this info on 10.6 machine? > > I especially need bssid, but band would also be nice to have. > > That I do not know, unfortunately :|. It seems that native ChromeCast App only supports 10.7+, so it should be ok to just fail gracefully. https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:601: // Find previously connected network and notify that it is disconnected. On 2014/01/08 23:06:23, tbarzic wrote: > On 2014/01/08 21:00:03, mef wrote: > > On 2014/01/07 20:05:33, tbarzic wrote: > > > When a connected network changes from A to B, will there always be a > > > notification with empty interface_ ssid? > > > If not, we may want to do this even if |connected_network_guid| is not > empty. > > In my testing it appears to always get notification for disconnect, so > > [interface_ ssid] is empty first. > > You don't seem too sure about this :) > Can you add a comment? Done.
https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... File components/wifi/wifi_service_mac.mm (right): https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:226: // As the WLAN api binding runs on its own thread, we need to provide our own On 2014/01/10 17:01:52, mef wrote: > On 2014/01/09 23:12:45, rsesek wrote: > > On 2014/01/08 21:00:03, mef wrote: > > > On 2014/01/07 16:19:05, rsesek wrote: > > > > I don't understand this. What's backing this SequencedTaskRunner and why > > isn't > > > > it providing the autorelease pool automatically (for reference, > > > > SequencedWorkerPool does)? > > > > > > I'm not sure I understand the question, could you elaborate? I've copied > this > > > comment from > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/ge... > > > > I don't think you need to supply your own autorelease pool at all in this CL, > > since the thread running the MessageLoop should be doing that for you. Why did > > you add this in the first place? Were you seeing console errors being printed > > around objects being leaked because no pool was in place? Unless you're > running > > your own base::Thread, there should be an autorelease pool around the main > event > > loop automatically. > FYI: WiFiService calls are posted from UI thread to sequenced task runner on > worker pool, so they run on random worker threads. I didn't observe any errors, > but adding local autorelease pool seemed to make sense. I'll be happy to remove > it though if its not necessary. Please advise. As I wrote above, it is not necessary. Worker threads create the autorelease pool around the main MessageLoop. https://code.google.com/p/chromium/codesearch#chromium/src/base/threading/seq... https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:554: properties->ssid = [[network ssid] UTF8String]; On 2014/01/10 17:01:52, mef wrote: > On 2014/01/09 23:12:45, rsesek wrote: > > On 2014/01/08 21:00:03, mef wrote: > > > On 2014/01/07 16:19:05, rsesek wrote: > > > > sys_string_conversions.h. This looks like you're assigning a weak C-string > > > > pointer to something that will outlive this function. > > > |ssid| and other properties are std::string. > > > > Yes, but I had to look that up. It looks like you're assigning to a const > char*. > Any suggestions on making it more apparent? Yes, the string conversion function to make it explicit for operator=(const std::string&) https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:560: if ([[network wlanChannel] channelBand] == kCWChannelBand2GHz) On 2014/01/10 17:01:52, mef wrote: > On 2014/01/09 23:12:45, rsesek wrote: > > On 2014/01/08 21:00:03, mef wrote: > > > On 2014/01/07 16:19:05, rsesek wrote: > > > > Again, what if this is running on a 10.6 machine? > > > Is there some other way to get this info on 10.6 machine? > > > I especially need bssid, but band would also be nice to have. > > > > That I do not know, unfortunately :|. > It seems that native ChromeCast App only supports 10.7+, so it should be ok to > just fail gracefully. Chrome needs to run on 10.6. If you call this as is, it will crash on every 10.6 machine.
Thanks a lot. At this point I believe I've addressed all open issues, PTAL. https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... File components/wifi/wifi_service_mac.mm (right): https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:226: // As the WLAN api binding runs on its own thread, we need to provide our own On 2014/01/10 18:59:51, rsesek wrote: > On 2014/01/10 17:01:52, mef wrote: > > On 2014/01/09 23:12:45, rsesek wrote: > > > On 2014/01/08 21:00:03, mef wrote: > > > > On 2014/01/07 16:19:05, rsesek wrote: > > > > > I don't understand this. What's backing this SequencedTaskRunner and why > > > isn't > > > > > it providing the autorelease pool automatically (for reference, > > > > > SequencedWorkerPool does)? > > > > > > > > I'm not sure I understand the question, could you elaborate? I've copied > > this > > > > comment from > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/ge... > > > > > > I don't think you need to supply your own autorelease pool at all in this > CL, > > > since the thread running the MessageLoop should be doing that for you. Why > did > > > you add this in the first place? Were you seeing console errors being > printed > > > around objects being leaked because no pool was in place? Unless you're > > running > > > your own base::Thread, there should be an autorelease pool around the main > > event > > > loop automatically. > > FYI: WiFiService calls are posted from UI thread to sequenced task runner on > > worker pool, so they run on random worker threads. I didn't observe any > errors, > > but adding local autorelease pool seemed to make sense. I'll be happy to > remove > > it though if its not necessary. Please advise. > > As I wrote above, it is not necessary. Worker threads create the autorelease > pool around the main MessageLoop. > > https://code.google.com/p/chromium/codesearch#chromium/src/base/threading/seq... Done. https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:392: // have valid IP address). On 2014/01/08 23:06:23, tbarzic wrote: > On 2014/01/08 21:00:03, mef wrote: > > On 2014/01/07 20:05:33, tbarzic wrote: > > > This seems wrong.. > > > what if another network gets connected while we're waiting for this one to > > > become accessible? > > > > > > Also, the network might change by means other than this API, and this > suggests > > > that in these they would erroneously be reported as "Connected". Can we just > > set > > > the network status to "Connecting" if it's 'connected' but not 'accessible'? > > I think I should change 'GetProperties' to check reachability of network > > [interface_ ssid], and report state as 'Connecting' if network is not > reachable > > yet. Does this make sense? > > sounds good Done. https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:554: properties->ssid = [[network ssid] UTF8String]; On 2014/01/10 18:59:51, rsesek wrote: > On 2014/01/10 17:01:52, mef wrote: > > On 2014/01/09 23:12:45, rsesek wrote: > > > On 2014/01/08 21:00:03, mef wrote: > > > > On 2014/01/07 16:19:05, rsesek wrote: > > > > > sys_string_conversions.h. This looks like you're assigning a weak > C-string > > > > > pointer to something that will outlive this function. > > > > |ssid| and other properties are std::string. > > > > > > Yes, but I had to look that up. It looks like you're assigning to a const > > char*. > > Any suggestions on making it more apparent? > > Yes, the string conversion function to make it explicit for operator=(const > std::string&) Done. Use SysNSStringToUTF8. https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:560: if ([[network wlanChannel] channelBand] == kCWChannelBand2GHz) On 2014/01/10 18:59:51, rsesek wrote: > On 2014/01/10 17:01:52, mef wrote: > > On 2014/01/09 23:12:45, rsesek wrote: > > > On 2014/01/08 21:00:03, mef wrote: > > > > On 2014/01/07 16:19:05, rsesek wrote: > > > > > Again, what if this is running on a 10.6 machine? > > > > Is there some other way to get this info on 10.6 machine? > > > > I especially need bssid, but band would also be nice to have. > > > > > > That I do not know, unfortunately :|. > > It seems that native ChromeCast App only supports 10.7+, so it should be ok to > > just fail gracefully. > > Chrome needs to run on 10.6. If you call this as is, it will crash on every 10.6 > machine. Done. Added respondsToSelector:@selector(associateToNetwork:password:error:) to WiFiServiceMac::Initialize, and release interface_ if it is not supported. https://codereview.chromium.org/64683014/diff/670001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:616: if (network_list_changed_observer_.is_null()) On 2014/01/08 23:06:23, tbarzic wrote: > On 2014/01/08 21:00:03, mef wrote: > > On 2014/01/07 16:19:05, rsesek wrote: > > > Why listen for network change events if nothing is going to use that data? > > Because caller could've done |RequestNetworkScan| without setting > > |network_list_changed_observer_|. > > I agree that there is no real need to observe network changes if there are no > listeners. Done.
Mostly nits, but some comments about control flow. https://codereview.chromium.org/64683014/diff/940001/components/wifi/wifi_ser... File components/wifi/wifi_service_mac.mm (right): https://codereview.chromium.org/64683014/diff/940001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:195: void WiFiServiceMac::UnInitialize() { Should you interface_.reset() here too? https://codereview.chromium.org/64683014/diff/940001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:200: base::DictionaryValue* properties, nit: alignment https://codereview.chromium.org/64683014/diff/940001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:214: *error = kErrorNotFound; Is it assumed that *error == "" at entry? https://codereview.chromium.org/64683014/diff/940001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:225: base::DictionaryValue* properties, nit: alignment https://codereview.chromium.org/64683014/diff/940001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:247: base::ListValue* network_list) { nit: alignment https://codereview.chromium.org/64683014/diff/940001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:255: RequestNetworkScan(); This will trigger a NetworkListChanged callback, right? Is that expected to happen from within GetVisibleNewtworks? https://codereview.chromium.org/64683014/diff/940001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:271: std::string* error) { nit: alignment https://codereview.chromium.org/64683014/diff/940001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:277: // Check, whether desired network is already connected. nit: remove , https://codereview.chromium.org/64683014/diff/940001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:296: // Check whether WiFi Password is set in |network_properties_| nit: end with punctuation https://codereview.chromium.org/64683014/diff/940001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:338: std::string* error) { nit: alignment https://codereview.chromium.org/64683014/diff/940001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:357: wlan_observer_.reset([[NSNotificationCenter defaultCenter] If SetEventObservers() is called twice, then you'll leak the previous observer. But also note that you do not own the reference returned from -addObserver… so this should not be in a scoper. There shouldn't be an observer if the callbacks is_null(), either. https://codereview.chromium.org/64683014/diff/940001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:362: task_runner_->PostTask( nit: this is supposed to be indented 4 spaces from the line that opened the block. Since that will cause wrapping issues, pull the block out into a local: http://google-styleguide.googlecode.com/svn/trunk/objcguide.xml?showone=Block... https://codereview.chromium.org/64683014/diff/940001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:370: int attempt) { nit: alignment https://codereview.chromium.org/64683014/diff/940001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:384: // Continue waiting for network connection state change. Do you need to poll for this, or can you use SCNetworkReachability to be notified of reachability changes? https://codereview.chromium.org/64683014/diff/940001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:396: const std::string& network_guid) const { nit: indent 2 more spaces https://codereview.chromium.org/64683014/diff/940001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:397: if(network_guid != GUIDFromSSID([interface_ ssid])) nit: space before ( https://codereview.chromium.org/64683014/diff/940001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:423: nit: remove https://codereview.chromium.org/64683014/diff/940001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:434: CWNetwork* cw_network; You can move this into the for() loop expression. https://codereview.chromium.org/64683014/diff/940001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:442: network_properties_map.end()) { nit: indent 4 spaces more https://codereview.chromium.org/64683014/diff/940001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:445: } else { Do you need to update the networks_ list in case network_properties is different? https://codereview.chromium.org/64683014/diff/940001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:456: const char* error_name, nit: alignment
https://codereview.chromium.org/64683014/diff/940001/components/wifi/wifi_ser... File components/wifi/wifi_service_mac.mm (right): https://codereview.chromium.org/64683014/diff/940001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:267: NotifyNetworkListChanged(networks_); NotifyNetworkListChanged should be part of UpdateNetworks. And potentially, we should check if the network list really changed before sending the event https://codereview.chromium.org/64683014/diff/940001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:279: NotifyNetworkChanged(connected_network_guid); no need for this, as there are no changes to the network here. https://codereview.chromium.org/64683014/diff/940001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:283: NSSet* networks = [interface_ If the scan does not find the network, if should be removed from |networks_| and NetworkListChanged should be sent. Also, what do you think about checking whether the network in contained in |networks_| before the scan; and failing if it's not? I can think of two cases where this may happen: 1. A bug in the calling code where wrong guid is used for StartConnect. 2. |networks_| has just been updated, but the observer hasn't yet received NetworkListChanged notification, in which case there's a reason the network was removed from |networks_|. https://codereview.chromium.org/64683014/diff/940001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:318: // Notify that previously connected network has changed. Can't this be done in OnWlanObserverNotification (there should be a wlan notification on success, right?). https://codereview.chromium.org/64683014/diff/940001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:322: // is async and it'll reset enable_notify_network_changed_. the comment is outdated; you've gotten rid of enable_notify_network_changed_ https://codereview.chromium.org/64683014/diff/940001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:432: networks_.clear(); it would be nice to gather the list of networks whose properties have changed ans trigger NetworksChanged notification for them. https://codereview.chromium.org/64683014/diff/940001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:541: NotifyNetworkChanged(connected_network_guid); WaitForNetworkConnect should probably be done here (instead of in start connect), so we send an event when the network's connection state changes to 'Connected' (in case it's initially 'Connecting')
Robert, thanks! I'll try to use Reachability notification instead of polling, and this may make it easier to address Toni's suggestions. thanks, -m https://codereview.chromium.org/64683014/diff/940001/components/wifi/wifi_ser... File components/wifi/wifi_service_mac.mm (right): https://codereview.chromium.org/64683014/diff/940001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:195: void WiFiServiceMac::UnInitialize() { On 2014/01/13 19:10:28, rsesek wrote: > Should you interface_.reset() here too? Done. https://codereview.chromium.org/64683014/diff/940001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:200: base::DictionaryValue* properties, On 2014/01/13 19:10:28, rsesek wrote: > nit: alignment Done. https://codereview.chromium.org/64683014/diff/940001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:214: *error = kErrorNotFound; On 2014/01/13 19:10:28, rsesek wrote: > Is it assumed that *error == "" at entry? Yes. Should I explicitly set it this way in public interface methods? https://codereview.chromium.org/64683014/diff/940001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:225: base::DictionaryValue* properties, On 2014/01/13 19:10:28, rsesek wrote: > nit: alignment Done. I think those are artifacts of Renaming WiFiServiceImpl -> WiFiServiceMac... https://codereview.chromium.org/64683014/diff/940001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:247: base::ListValue* network_list) { On 2014/01/13 19:10:28, rsesek wrote: > nit: alignment Done. https://codereview.chromium.org/64683014/diff/940001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:255: RequestNetworkScan(); On 2014/01/13 19:10:28, rsesek wrote: > This will trigger a NetworkListChanged callback, right? Is that expected to > happen from within GetVisibleNewtworks? Good question. Toni? https://codereview.chromium.org/64683014/diff/940001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:271: std::string* error) { On 2014/01/13 19:10:28, rsesek wrote: > nit: alignment Done. https://codereview.chromium.org/64683014/diff/940001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:277: // Check, whether desired network is already connected. On 2014/01/13 19:10:28, rsesek wrote: > nit: remove , Done. https://codereview.chromium.org/64683014/diff/940001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:296: // Check whether WiFi Password is set in |network_properties_| On 2014/01/13 19:10:28, rsesek wrote: > nit: end with punctuation Done. https://codereview.chromium.org/64683014/diff/940001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:338: std::string* error) { On 2014/01/13 19:10:28, rsesek wrote: > nit: alignment Done. https://codereview.chromium.org/64683014/diff/940001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:357: wlan_observer_.reset([[NSNotificationCenter defaultCenter] On 2014/01/13 19:10:28, rsesek wrote: > If SetEventObservers() is called twice, then you'll leak the previous observer. > But also note that you do not own the reference returned from -addObserver… so > this should not be in a scoper. > > There shouldn't be an observer if the callbacks is_null(), either. Done. https://codereview.chromium.org/64683014/diff/940001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:362: task_runner_->PostTask( On 2014/01/13 19:10:28, rsesek wrote: > nit: this is supposed to be indented 4 spaces from the line that opened the > block. Since that will cause wrapping issues, pull the block out into a local: > > http://google-styleguide.googlecode.com/svn/trunk/objcguide.xml?showone=Block... Done. I think. https://codereview.chromium.org/64683014/diff/940001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:370: int attempt) { On 2014/01/13 19:10:28, rsesek wrote: > nit: alignment Done. https://codereview.chromium.org/64683014/diff/940001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:384: // Continue waiting for network connection state change. On 2014/01/13 19:10:28, rsesek wrote: > Do you need to poll for this, or can you use SCNetworkReachability to be > notified of reachability changes? I think reachability is better, I'll try to use it. https://codereview.chromium.org/64683014/diff/940001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:396: const std::string& network_guid) const { On 2014/01/13 19:10:28, rsesek wrote: > nit: indent 2 more spaces Done. https://codereview.chromium.org/64683014/diff/940001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:397: if(network_guid != GUIDFromSSID([interface_ ssid])) On 2014/01/13 19:10:28, rsesek wrote: > nit: space before ( Done. https://codereview.chromium.org/64683014/diff/940001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:423: On 2014/01/13 19:10:28, rsesek wrote: > nit: remove Done. https://codereview.chromium.org/64683014/diff/940001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:434: CWNetwork* cw_network; On 2014/01/13 19:10:28, rsesek wrote: > You can move this into the for() loop expression. Done. https://codereview.chromium.org/64683014/diff/940001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:442: network_properties_map.end()) { On 2014/01/13 19:10:28, rsesek wrote: > nit: indent 4 spaces more Done. https://codereview.chromium.org/64683014/diff/940001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:445: } else { On 2014/01/13 19:10:28, rsesek wrote: > Do you need to update the networks_ list in case network_properties is > different? I'm just trying to coalesce all networks with same SSID into single NetworkProperies object with multiple frequencies. https://codereview.chromium.org/64683014/diff/940001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:456: const char* error_name, On 2014/01/13 19:10:28, rsesek wrote: > nit: alignment Done.
Toni, thanks! PTAL, and couple of followup questions... thanks, -m https://codereview.chromium.org/64683014/diff/940001/components/wifi/wifi_ser... File components/wifi/wifi_service_mac.mm (right): https://codereview.chromium.org/64683014/diff/940001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:267: NotifyNetworkListChanged(networks_); On 2014/01/13 19:39:30, tbarzic wrote: > NotifyNetworkListChanged should be part of UpdateNetworks. And potentially, we > should check if the network list really changed before sending the event Done. https://codereview.chromium.org/64683014/diff/940001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:279: NotifyNetworkChanged(connected_network_guid); On 2014/01/13 19:39:30, tbarzic wrote: > no need for this, as there are no changes to the network here. From my observations the extension didn't seem to recognize that mac is already connected to the network unless I've issued this notification. https://codereview.chromium.org/64683014/diff/940001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:283: NSSet* networks = [interface_ On 2014/01/13 19:39:30, tbarzic wrote: > If the scan does not find the network, if should be removed from |networks_| and > NetworkListChanged should be sent. Sounds good. > Also, what do you think about checking whether the network in contained in > |networks_| before the scan; and failing if it's not? > I can think of two cases where this may happen: > 1. A bug in the calling code where wrong guid is used for StartConnect. > 2. |networks_| has just been updated, but the observer hasn't yet received > NetworkListChanged notification, in which case there's a reason the network was > removed from |networks_|. 3. Hidden network will not show up in |networks_|. Although proper handling of that should probably involve implementation of |CreateNetwork| to at least add its guid as valid. https://codereview.chromium.org/64683014/diff/940001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:318: // Notify that previously connected network has changed. On 2014/01/13 19:39:30, tbarzic wrote: > Can't this be done in OnWlanObserverNotification (there should be a wlan > notification on success, right?). Done. https://codereview.chromium.org/64683014/diff/940001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:322: // is async and it'll reset enable_notify_network_changed_. On 2014/01/13 19:39:30, tbarzic wrote: > the comment is outdated; you've gotten rid of enable_notify_network_changed_ Done. https://codereview.chromium.org/64683014/diff/940001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:432: networks_.clear(); On 2014/01/13 19:39:30, tbarzic wrote: > it would be nice to gather the list of networks whose properties have changed > ans trigger NetworksChanged notification for them. I should be able to do that, just what about networks that have appeared or disappeared? Should they also be included in NetworksChanged notification, or just NetworkListChanged? https://codereview.chromium.org/64683014/diff/940001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:541: NotifyNetworkChanged(connected_network_guid); On 2014/01/13 19:39:30, tbarzic wrote: > WaitForNetworkConnect should probably be done here (instead of in start > connect), so we send an event when the network's connection state changes to > 'Connected' (in case it's initially 'Connecting') Done.
https://codereview.chromium.org/64683014/diff/1030001/components/wifi/wifi_se... File components/wifi/wifi_service_mac.mm (right): https://codereview.chromium.org/64683014/diff/1030001/components/wifi/wifi_se... components/wifi/wifi_service_mac.mm:157: NSObject* wlan_observer_; This should be |id|, not |NSObject*|. https://codereview.chromium.org/64683014/diff/1030001/components/wifi/wifi_se... components/wifi/wifi_service_mac.mm:177: WiFiServiceMac::WiFiServiceMac() : wlan_observer_(NULL) { NULL -> nil for ObjC objects. https://codereview.chromium.org/64683014/diff/1030001/components/wifi/wifi_se... components/wifi/wifi_service_mac.mm:201: [[NSNotificationCenter defaultCenter] removeObserver:wlan_observer_]; You should guard this with |if (wlan_observer_)| since it is possible for it to be nil if there are no observers. https://codereview.chromium.org/64683014/diff/1030001/components/wifi/wifi_se... components/wifi/wifi_service_mac.mm:213: DVLOG(1) << *properties; This logging seems like it's just noise. https://codereview.chromium.org/64683014/diff/1030001/components/wifi/wifi_se... components/wifi/wifi_service_mac.mm:217: *error = kErrorNotFound; Flip this to an early return? https://codereview.chromium.org/64683014/diff/1030001/components/wifi/wifi_se... components/wifi/wifi_service_mac.mm:340: password:ns_password nit: align colons https://codereview.chromium.org/64683014/diff/1030001/components/wifi/wifi_se... components/wifi/wifi_service_mac.mm:356: // Power-cycle the interface to disconnect from current network and connect Why did this change instead of calling |-disassociate|? https://codereview.chromium.org/64683014/diff/1030001/components/wifi/wifi_se... components/wifi/wifi_service_mac.mm:379: wlan_observer_ = NULL; NULL -> nil
Robert, thanks! https://codereview.chromium.org/64683014/diff/1030001/components/wifi/wifi_se... File components/wifi/wifi_service_mac.mm (right): https://codereview.chromium.org/64683014/diff/1030001/components/wifi/wifi_se... components/wifi/wifi_service_mac.mm:157: NSObject* wlan_observer_; On 2014/01/15 16:26:35, rsesek wrote: > This should be |id|, not |NSObject*|. Done. https://codereview.chromium.org/64683014/diff/1030001/components/wifi/wifi_se... components/wifi/wifi_service_mac.mm:177: WiFiServiceMac::WiFiServiceMac() : wlan_observer_(NULL) { On 2014/01/15 16:26:35, rsesek wrote: > NULL -> nil for ObjC objects. Done. https://codereview.chromium.org/64683014/diff/1030001/components/wifi/wifi_se... components/wifi/wifi_service_mac.mm:201: [[NSNotificationCenter defaultCenter] removeObserver:wlan_observer_]; On 2014/01/15 16:26:35, rsesek wrote: > You should guard this with |if (wlan_observer_)| since it is possible for it to > be nil if there are no observers. Done. https://codereview.chromium.org/64683014/diff/1030001/components/wifi/wifi_se... components/wifi/wifi_service_mac.mm:213: DVLOG(1) << *properties; On 2014/01/15 16:26:35, rsesek wrote: > This logging seems like it's just noise. It helps me to see interactions with extension (including connection_state of network). https://codereview.chromium.org/64683014/diff/1030001/components/wifi/wifi_se... components/wifi/wifi_service_mac.mm:217: *error = kErrorNotFound; On 2014/01/15 16:26:35, rsesek wrote: > Flip this to an early return? Done. https://codereview.chromium.org/64683014/diff/1030001/components/wifi/wifi_se... components/wifi/wifi_service_mac.mm:340: password:ns_password On 2014/01/15 16:26:35, rsesek wrote: > nit: align colons Done. https://codereview.chromium.org/64683014/diff/1030001/components/wifi/wifi_se... components/wifi/wifi_service_mac.mm:356: // Power-cycle the interface to disconnect from current network and connect On 2014/01/15 16:26:35, rsesek wrote: > Why did this change instead of calling |-disassociate|? My testing showed that extension calls StartDisconnect when user cancels the setup. Calling |-disassociate| was leaving computer disconnected, while power-cycling re-connects it to default network. https://codereview.chromium.org/64683014/diff/1030001/components/wifi/wifi_se... components/wifi/wifi_service_mac.mm:379: wlan_observer_ = NULL; On 2014/01/15 16:26:35, rsesek wrote: > NULL -> nil Done.
This is in pretty good shape now. Any updates on using SCReachabiilty? Also, I have a question about the threading model of this class. From which threads is it safe to call the WiFi service methods/does this object live on a given thread? (The header specifies that it can be created on any thread but not on which thread it is safe to call). If so, is that thread different from the task_runner_ passed to it in Initialize()? If yes, then how can you modify the member variables safely on the task runner while reading (and maybe updating) on a different thread? Also, are you doing any CoreWLAN stuff (scanning or connecting) on a BrowserThread?
On 2014/01/15 22:20:27, rsesek wrote: > This is in pretty good shape now. Any updates on using SCReachabiilty? > > Also, I have a question about the threading model of this class. From which > threads is it safe to call the WiFi service methods/does this object live on a > given thread? (The header specifies that it can be created on any thread but not > on which thread it is safe to call). If so, is that thread different from the > task_runner_ passed to it in Initialize()? If yes, then how can you modify the > member variables safely on the task runner while reading (and maybe updating) on > a different thread? Also, are you doing any CoreWLAN stuff (scanning or > connecting) on a BrowserThread? That's great! In regards to threading - WiFiService object is owned by NetworkingPrivateServiceClient and is called on a random thread using SequencedTaskRunner from BlockingPool: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... Thread safety is provided by SequencedTaskRunner. CoreWLAN is accessed on a random thread from BlockingPool. Coincidentally I haven't added NetworkChangeObserver interface yet because it says that it is going to be called on the same thread that have registered for notifications, and that may happen concurrently with other thread running a task posted on the task runner. I'll appreciate any suggestions.
I would like to humbly ask for OWNER review of these changes: erg@ - chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc (extensions::NetworkingPrivateEventRouterFactory is now available on OS_MACOSX) finnur@ - chrome/common/extensions/api/*.json (make it available on "mac"). thanks, -m
lgtm
OWNERS LGTM
On 2014/01/15 23:09:05, mef wrote: > On 2014/01/15 22:20:27, rsesek wrote: > > This is in pretty good shape now. Any updates on using SCReachabiilty? > > > > Also, I have a question about the threading model of this class. From which > > threads is it safe to call the WiFi service methods/does this object live on a > > given thread? (The header specifies that it can be created on any thread but > not > > on which thread it is safe to call). If so, is that thread different from the > > task_runner_ passed to it in Initialize()? If yes, then how can you modify the > > member variables safely on the task runner while reading (and maybe updating) > on > > a different thread? Also, are you doing any CoreWLAN stuff (scanning or > > connecting) on a BrowserThread? > > That's great! > > In regards to threading - WiFiService object is owned by > NetworkingPrivateServiceClient and is called on a random thread using > SequencedTaskRunner from BlockingPool: > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... Ah, got it. I would definitely make this more clear in wifi_service.h, since it currently states that methods should be called "on worker thread" which is not really an accurate description of the calling semantics. Calling out NetworkingPrivateServiceClient as the UI-thread-accessible wrapper may be a good idea, too. > Thread safety is provided by SequencedTaskRunner. CoreWLAN is accessed on a > random thread from BlockingPool. > > Coincidentally I haven't added NetworkChangeObserver interface yet because it > says that it is going to be called on the same thread that have registered for > notifications, and that may happen concurrently with other thread running a task > posted on the task runner. > > I'll appreciate any suggestions. Yes, that does appear problematic, since it grabs the MessageLoop::current() directly. Do other OSes do this polling when connecting to the network?
mostly looks ok. https://codereview.chromium.org/64683014/diff/940001/components/wifi/wifi_ser... File components/wifi/wifi_service_mac.mm (right): https://codereview.chromium.org/64683014/diff/940001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:255: RequestNetworkScan(); On 2014/01/13 22:05:34, mef wrote: > On 2014/01/13 19:10:28, rsesek wrote: > > This will trigger a NetworkListChanged callback, right? Is that expected to > > happen from within GetVisibleNewtworks? > Good question. Toni? It's not required, but it's ok if rescan happens here (and if we rescan, the event should be sent). This seems more like lazy initialization of the network list. Maybe it could be moved when the service starts? I'm fine with this being here, though. https://codereview.chromium.org/64683014/diff/940001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:279: NotifyNetworkChanged(connected_network_guid); On 2014/01/14 22:10:31, mef wrote: > On 2014/01/13 19:39:30, tbarzic wrote: > > no need for this, as there are no changes to the network here. > From my observations the extension didn't seem to recognize that mac is already > connected to the network unless I've issued this notification. hm, that looks like a bug. either in the extension or here. Can you see what's the network's ConnectionState in this case (as seen by the extension)? The fact that interface_ already has the targeted ssid, means that a network changed event should have already been sent (and the extension hasn't yet received it or processed it, but in either way, it should really soon detect that the connection status changed). So it seems that the NetworkChanged event was not sent when the network's connection properties changed to 'Connected'/'Connecting'. Maybe the connection state is not correctly set for the network that's connected (either reachable or not) when the wifi service is started? Other option is that the extension sent startConnect request even though the network's connection state is 'Connected'. If you suspect a bug in the extension, feel free to send me the extension logs (and leave this as it is for now). https://codereview.chromium.org/64683014/diff/940001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:283: NSSet* networks = [interface_ On 2014/01/14 22:10:31, mef wrote: > On 2014/01/13 19:39:30, tbarzic wrote: > > If the scan does not find the network, if should be removed from |networks_| > and > > NetworkListChanged should be sent. > Sounds good. > > > Also, what do you think about checking whether the network in contained in > > |networks_| before the scan; and failing if it's not? > > I can think of two cases where this may happen: > > 1. A bug in the calling code where wrong guid is used for StartConnect. > > 2. |networks_| has just been updated, but the observer hasn't yet received > > NetworkListChanged notification, in which case there's a reason the network > was > > removed from |networks_|. > 3. Hidden network will not show up in |networks_|. Although proper handling of > that should probably involve implementation of |CreateNetwork| to at least add > its guid as valid. Yeah, proper handling in this case would be to create network first (as the API generaly does not assume ssid == guid), so it cannot really know the guid for a non-visible network :) https://codereview.chromium.org/64683014/diff/940001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:432: networks_.clear(); On 2014/01/14 22:10:31, mef wrote: > On 2014/01/13 19:39:30, tbarzic wrote: > > it would be nice to gather the list of networks whose properties have changed > > ans trigger NetworksChanged notification for them. > I should be able to do that, just what about networks that have appeared or > disappeared? Should they also be included in NetworksChanged notification, or > just NetworkListChanged? hm, I think that ChromeOS version does send NetworksChanged in these cases, but I'm not sure. From the cast extension's point of view, it's fine either way (it uses primarily onNetworkListChange to detect network addition/removal). I'm also fine if you leave this for a follow up. https://codereview.chromium.org/64683014/diff/1070001/components/wifi/wifi_se... File components/wifi/wifi_service_mac.mm (right): https://codereview.chromium.org/64683014/diff/1070001/components/wifi/wifi_se... components/wifi/wifi_service_mac.mm:576: UpdateNetworks(); will this find a connected hidden network? Should this be more targeted (e.g. UpdateNetwork(ssid), which does general scan when ssid.empty())
Hi, PTAL. I've added NetworkChangeObserver. I start and stop it on IO thread and post notifications to task runner. I think it is somewhat more reliable than polling, but I had to add "+net/base", and "+content/public/browser" to DEPS. I'm not sure how critical is that. I didn't remove the WaitForNetworkConnect yet, but I'll remove it if everything looks good. I'll do more testing tomorrow, one strange thing I'm observing is that contrary to my expectations extension doesn't always call GetProperties after getting NetworksChanged notification, and seems to have stale information about currently connected network because of that. BTW, I've implemented |CreateNetwork| for connection to hidden networks. thanks, -m https://codereview.chromium.org/64683014/diff/940001/components/wifi/wifi_ser... File components/wifi/wifi_service_mac.mm (right): https://codereview.chromium.org/64683014/diff/940001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:255: RequestNetworkScan(); On 2014/01/16 19:41:48, tbarzic wrote: > On 2014/01/13 22:05:34, mef wrote: > > On 2014/01/13 19:10:28, rsesek wrote: > > > This will trigger a NetworkListChanged callback, right? Is that expected to > > > happen from within GetVisibleNewtworks? > > Good question. Toni? > > It's not required, but it's ok if rescan happens here (and if we rescan, the > event should be sent). This seems more like lazy initialization of the network > list. Maybe it could be moved when the service starts? I'm fine with this being > here, though. Done. https://codereview.chromium.org/64683014/diff/940001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:279: NotifyNetworkChanged(connected_network_guid); On 2014/01/16 19:41:48, tbarzic wrote: > On 2014/01/14 22:10:31, mef wrote: > > On 2014/01/13 19:39:30, tbarzic wrote: > > > no need for this, as there are no changes to the network here. > > From my observations the extension didn't seem to recognize that mac is > already > > connected to the network unless I've issued this notification. > > hm, that looks like a bug. either in the extension or here. > Can you see what's the network's ConnectionState in this case (as seen by the > extension)? > The fact that interface_ already has the targeted ssid, means that a network > changed event should have already been sent (and the extension hasn't yet > received it or processed it, but in either way, it should really soon detect > that the connection status changed). So it seems that the NetworkChanged event > was not sent when the network's connection properties changed to > 'Connected'/'Connecting'. Maybe the connection state is not correctly set for > the network that's connected (either reachable or not) when the wifi service is > started? > > Other option is that the extension sent startConnect request even though the > network's connection state is 'Connected'. > > If you suspect a bug in the extension, feel free to send me the extension logs > (and leave this as it is for now). I'll try to debug this. https://codereview.chromium.org/64683014/diff/940001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:283: NSSet* networks = [interface_ On 2014/01/16 19:41:48, tbarzic wrote: > On 2014/01/14 22:10:31, mef wrote: > > On 2014/01/13 19:39:30, tbarzic wrote: > > > If the scan does not find the network, if should be removed from |networks_| > > and > > > NetworkListChanged should be sent. > > Sounds good. > > > > > Also, what do you think about checking whether the network in contained in > > > |networks_| before the scan; and failing if it's not? > > > I can think of two cases where this may happen: > > > 1. A bug in the calling code where wrong guid is used for StartConnect. > > > 2. |networks_| has just been updated, but the observer hasn't yet received > > > NetworkListChanged notification, in which case there's a reason the network > > was > > > removed from |networks_|. > > 3. Hidden network will not show up in |networks_|. Although proper handling of > > that should probably involve implementation of |CreateNetwork| to at least add > > its guid as valid. > > Yeah, proper handling in this case would be to create network first (as the API > generaly does not assume ssid == guid), so it cannot really know the guid for a > non-visible network :) Done. Implemented |CreateNetwork| to store network properties and return network guid. https://codereview.chromium.org/64683014/diff/940001/components/wifi/wifi_ser... components/wifi/wifi_service_mac.mm:432: networks_.clear(); On 2014/01/16 19:41:48, tbarzic wrote: > On 2014/01/14 22:10:31, mef wrote: > > On 2014/01/13 19:39:30, tbarzic wrote: > > > it would be nice to gather the list of networks whose properties have > changed > > > ans trigger NetworksChanged notification for them. > > I should be able to do that, just what about networks that have appeared or > > disappeared? Should they also be included in NetworksChanged notification, or > > just NetworkListChanged? > > hm, I think that ChromeOS version does send NetworksChanged in these cases, but > I'm not sure. From the cast extension's point of view, it's fine either way (it > uses primarily onNetworkListChange to detect network addition/removal). > > I'm also fine if you leave this for a follow up. Sounds good. I'll add that to my TODO list for a follow up. https://codereview.chromium.org/64683014/diff/1070001/components/wifi/wifi_se... File components/wifi/wifi_service_mac.mm (right): https://codereview.chromium.org/64683014/diff/1070001/components/wifi/wifi_se... components/wifi/wifi_service_mac.mm:576: UpdateNetworks(); On 2014/01/16 19:41:49, tbarzic wrote: > will this find a connected hidden network? Should this be more targeted (e.g. > UpdateNetwork(ssid), which does general scan when ssid.empty()) UpdateNetworks() will not see the hidden network, however StartConnect() uses scanForNetworksWithName:SSIDFromGUID(network_guid), which should fine one if it is around (to be verified).
Yeah, the extension doesn't always call getProperties when it gets NetworksChanged notification. But it should not have stale information about connected network, as that is actually the one property the extension cares about. It will refetch properties when if gets NetworksChanged notif in the following situations: 1. there is no connected wifi network. 2. there currently connected wifi network's changed. (plus after startConnect is called for a network, if always refetches that network's properties) So, if the extension has wrong information about the connected network, maybe a NetworksChanged event was not sent when the previously connected network was disconnected (or maybe it's 'ConnectionState' property was not properly updated)?
Hi guys, PTAL. It turned out that system events observers were posting tasks to NotifyNetworkChanged, but sometimes by the time those tasks were processed the [interface_ ssid] was already not empty, so the disconnected network detection didn't work properly, and extension wasn't getting disconnect notifications. I've changed it to explicitly track connected_network_guid_ and now setup completes successfully. Also please comment on Patch #23 (Using NetworkChangeNotified), whether additional complexity and dependencies are fair price to pay to get rid of polling. thanks, -m
I don't think the NetworkChangeObserver adds too much complexity, but whether you want to suck in that DEPS is up to the OWNERS of this component. https://codereview.chromium.org/64683014/diff/1320001/components/wifi/wifi_se... File components/wifi/wifi_service_mac.mm (right): https://codereview.chromium.org/64683014/diff/1320001/components/wifi/wifi_se... components/wifi/wifi_service_mac.mm:60: nit: extra blank line https://codereview.chromium.org/64683014/diff/1320001/components/wifi/wifi_se... components/wifi/wifi_service_mac.mm:176: // Convert s|CWSecurityMode| into onc::wifi::k{WPA|WEP}* security constant. nit: ^T[ s] https://codereview.chromium.org/64683014/diff/1320001/components/wifi/wifi_se... components/wifi/wifi_service_mac.mm:480: nit: remove blank line
Hi Robert, thanks a lot, PTAL. BTW, is there a better thread than BrowserThread::IO for NetworkChangeDetector to avoid adding "content/public/browser" to DEPS? https://codereview.chromium.org/64683014/diff/1320001/components/wifi/wifi_se... File components/wifi/wifi_service_mac.mm (right): https://codereview.chromium.org/64683014/diff/1320001/components/wifi/wifi_se... components/wifi/wifi_service_mac.mm:60: On 2014/01/27 18:06:56, rsesek wrote: > nit: extra blank line Done. https://codereview.chromium.org/64683014/diff/1320001/components/wifi/wifi_se... components/wifi/wifi_service_mac.mm:176: // Convert s|CWSecurityMode| into onc::wifi::k{WPA|WEP}* security constant. On 2014/01/27 18:06:56, rsesek wrote: > nit: ^T[ s] Done. https://codereview.chromium.org/64683014/diff/1320001/components/wifi/wifi_se... components/wifi/wifi_service_mac.mm:480: On 2014/01/27 18:06:56, rsesek wrote: > nit: remove blank line Done.
https://codereview.chromium.org/64683014/diff/1230001/components/wifi/wifi_se... File components/wifi/wifi_service_mac.mm (right): https://codereview.chromium.org/64683014/diff/1230001/components/wifi/wifi_se... components/wifi/wifi_service_mac.mm:81: net::NetworkChangeNotifier::RemoveNetworkChangeObserver(this); is it ok if this doesn't get called (which is possible, since IOThread may be stopped when |UnInitialize| is called on the worker thread)? https://codereview.chromium.org/64683014/diff/1230001/components/wifi/wifi_se... components/wifi/wifi_service_mac.mm:85: friend struct content::BrowserThread::DeleteOnThread< As long as you always call StopOnIOThread, you shouldn't need this. https://codereview.chromium.org/64683014/diff/1230001/components/wifi/wifi_se... components/wifi/wifi_service_mac.mm:259: content::BrowserThread::IO, if you wish, you could get rid of dependency on content/ by pass IO message loop to initialize (as you do with task_runner_). https://codereview.chromium.org/64683014/diff/1230001/components/wifi/wifi_se... components/wifi/wifi_service_mac.mm:475: nuke the line https://codereview.chromium.org/64683014/diff/1320001/chrome/browser/extensio... File chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc (right): https://codereview.chromium.org/64683014/diff/1320001/chrome/browser/extensio... chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc:400: DLOG(ERROR) << error_name; Won't the errors from the extension api calls get logged in Debug build, even without this? https://codereview.chromium.org/64683014/diff/1320001/components/wifi/wifi_se... File components/wifi/wifi_service.h (right): https://codereview.chromium.org/64683014/diff/1320001/components/wifi/wifi_se... components/wifi/wifi_service.h:24: // posted to worker pool using SequencedTaskRunner |task_runner_|. You don't need the second part of the sentence. (especially since |task_runner_| is not part of the |WifiService| interface) https://codereview.chromium.org/64683014/diff/1320001/components/wifi/wifi_se... File components/wifi/wifi_service_mac.mm (right): https://codereview.chromium.org/64683014/diff/1320001/components/wifi/wifi_se... components/wifi/wifi_service_mac.mm:638: // There are 2 notifications for every network switch: remove this comment, looks like it turned out to be false :) https://codereview.chromium.org/64683014/diff/1320001/components/wifi/wifi_se... components/wifi/wifi_service_mac.mm:647: it->connection_state = onc::connection_state::kNotConnected; indent-=2 https://codereview.chromium.org/64683014/diff/1320001/components/wifi/wifi_se... components/wifi/wifi_service_mac.mm:649: NotifyNetworkChanged(connected_network_guid_); this should be called only if (it != networks_.end()) (otherwise, the networks was removed, and the event should already been sent to the extension) https://codereview.chromium.org/64683014/diff/1460001/components/wifi/wifi_se... File components/wifi/wifi_service_mac.mm (right): https://codereview.chromium.org/64683014/diff/1460001/components/wifi/wifi_se... components/wifi/wifi_service_mac.mm:75: net::NetworkChangeNotifier::AddNetworkChangeObserver(this); maybe trigger OnNetworkChange (to initialize connected_network_guid_). https://codereview.chromium.org/64683014/diff/1460001/components/wifi/wifi_se... components/wifi/wifi_service_mac.mm:96: if (!on_network_changed_.is_null()) { I'd consider making this CHECK If you keep it like this, {} are not needed
Hi Toni, thanks for your comments! I've addressed most of them and have followup questions to couple of others. Would it make sense to move NetworkChangeDetector out of WiFiService into NetworkingPrivateServiceClient? I could allocate it on UI thread pass it to WiFiService in the Initialize? thanks, -m https://codereview.chromium.org/64683014/diff/1230001/components/wifi/wifi_se... File components/wifi/wifi_service_mac.mm (right): https://codereview.chromium.org/64683014/diff/1230001/components/wifi/wifi_se... components/wifi/wifi_service_mac.mm:81: net::NetworkChangeNotifier::RemoveNetworkChangeObserver(this); On 2014/01/27 20:17:24, tbarzic wrote: > is it ok if this doesn't get called (which is possible, since IOThread may be > stopped when |UnInitialize| is called on the worker thread)? Good question, it seems that it may leak. Any suggestions? https://codereview.chromium.org/64683014/diff/1230001/components/wifi/wifi_se... components/wifi/wifi_service_mac.mm:85: friend struct content::BrowserThread::DeleteOnThread< On 2014/01/27 20:17:24, tbarzic wrote: > As long as you always call StopOnIOThread, you shouldn't need this. Without this I get compiler errors about inaccessible private destructor. https://codereview.chromium.org/64683014/diff/1230001/components/wifi/wifi_se... components/wifi/wifi_service_mac.mm:259: content::BrowserThread::IO, On 2014/01/27 20:17:24, tbarzic wrote: > if you wish, you could get rid of dependency on content/ by pass IO message loop > to initialize (as you do with task_runner_). Hmm, I'm wondering whether I should subscribe to notifications in |SetEventObservers| instead of here. The (unrelated) password slurping CL would instantiate WiFiService inside of Utility Process and wouldn't care about any notifications. https://codereview.chromium.org/64683014/diff/1230001/components/wifi/wifi_se... components/wifi/wifi_service_mac.mm:475: On 2014/01/27 20:17:24, tbarzic wrote: > nuke the line Done. https://codereview.chromium.org/64683014/diff/1320001/chrome/browser/extensio... File chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc (right): https://codereview.chromium.org/64683014/diff/1320001/chrome/browser/extensio... chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc:400: DLOG(ERROR) << error_name; On 2014/01/27 20:17:24, tbarzic wrote: > Won't the errors from the extension api calls get logged in Debug build, even > without this? Done. https://codereview.chromium.org/64683014/diff/1320001/components/wifi/wifi_se... File components/wifi/wifi_service.h (right): https://codereview.chromium.org/64683014/diff/1320001/components/wifi/wifi_se... components/wifi/wifi_service.h:24: // posted to worker pool using SequencedTaskRunner |task_runner_|. On 2014/01/27 20:17:24, tbarzic wrote: > You don't need the second part of the sentence. (especially since |task_runner_| > is not part of the |WifiService| interface) Done. https://codereview.chromium.org/64683014/diff/1320001/components/wifi/wifi_se... File components/wifi/wifi_service_mac.mm (right): https://codereview.chromium.org/64683014/diff/1320001/components/wifi/wifi_se... components/wifi/wifi_service_mac.mm:638: // There are 2 notifications for every network switch: On 2014/01/27 20:17:24, tbarzic wrote: > remove this comment, looks like it turned out to be false :) Done. https://codereview.chromium.org/64683014/diff/1320001/components/wifi/wifi_se... components/wifi/wifi_service_mac.mm:647: it->connection_state = onc::connection_state::kNotConnected; On 2014/01/27 20:17:24, tbarzic wrote: > indent-=2 Done. https://codereview.chromium.org/64683014/diff/1320001/components/wifi/wifi_se... components/wifi/wifi_service_mac.mm:649: NotifyNetworkChanged(connected_network_guid_); On 2014/01/27 20:17:24, tbarzic wrote: > this should be called only if (it != networks_.end()) > (otherwise, the networks was removed, and the event should already been sent to > the extension) Done. https://codereview.chromium.org/64683014/diff/1460001/components/wifi/wifi_se... File components/wifi/wifi_service_mac.mm (right): https://codereview.chromium.org/64683014/diff/1460001/components/wifi/wifi_se... components/wifi/wifi_service_mac.mm:75: net::NetworkChangeNotifier::AddNetworkChangeObserver(this); On 2014/01/27 20:17:24, tbarzic wrote: > maybe trigger OnNetworkChange (to initialize connected_network_guid_). Done. https://codereview.chromium.org/64683014/diff/1460001/components/wifi/wifi_se... components/wifi/wifi_service_mac.mm:96: if (!on_network_changed_.is_null()) { On 2014/01/27 20:17:24, tbarzic wrote: > I'd consider making this CHECK > If you keep it like this, {} are not needed Done.
Hi guys, PTAL. I've moved net::NetworkChangeNotifier observer implementation to NetworkingPrivateServiceClient (notified on UI thread) and post notifications to WiFiService on worker thread. This seems to simplify things and resolve questions about lifetime / ownership of NetworkChangeDetector. thanks, -m
https://codereview.chromium.org/64683014/diff/1500001/chrome/browser/extensio... File chrome/browser/extensions/api/networking_private/networking_private_service_client.h (right): https://codereview.chromium.org/64683014/diff/1500001/chrome/browser/extensio... chrome/browser/extensions/api/networking_private/networking_private_service_client.h:40: // them on worker thread. Always used from UI thread only. Can you update comment and mention observing NetworkChangeNotifier (and add a reason why this is not done in wifi_service; this part may also go to wifi_service.h). https://codereview.chromium.org/64683014/diff/1500001/chrome/browser/extensio... chrome/browser/extensions/api/networking_private/networking_private_service_client.h:41: class NetworkingPrivateServiceClient : public BrowserContextKeyedService, public BCKeyedService to a new line https://codereview.chromium.org/64683014/diff/1500001/components/wifi/wifi_se... File components/wifi/wifi_service.h (right): https://codereview.chromium.org/64683014/diff/1500001/components/wifi/wifi_se... components/wifi/wifi_service.h:112: virtual void OnNetworkChangeNotification() = 0; Maybe UpdateConnectedNetwork/RequestConnectedNetworkUpdate/SomethingMoreDescriptive (WifiService does not actually listen to an OnNetworkChange notification, so the name seems a bit out of place)? https://codereview.chromium.org/64683014/diff/1500001/components/wifi/wifi_se... File components/wifi/wifi_service_mac.mm (right): https://codereview.chromium.org/64683014/diff/1500001/components/wifi/wifi_se... components/wifi/wifi_service_mac.mm:159: // Observer to get notified when network list has changed (scan complete). I'd drop "scan complete" part https://codereview.chromium.org/64683014/diff/1500001/components/wifi/wifi_se... components/wifi/wifi_service_mac.mm:161: // MessageLoopProxy to post events on UI thread. MessageLoopProxy to which events should be posted. (wifi_service should not care which message loop that really is) https://codereview.chromium.org/64683014/diff/1500001/components/wifi/wifi_se... components/wifi/wifi_service_mac.mm:165: // Cache of network list collected by GetVisibleNetworks. Cached list of visible networks. Updated by |UpdateNetworks|. https://codereview.chromium.org/64683014/diff/1500001/components/wifi/wifi_se... components/wifi/wifi_service_mac.mm:172: base::WeakPtrFactory<WiFiServiceMac> weak_factory_; not needed anymore https://codereview.chromium.org/64683014/diff/1500001/components/wifi/wifi_se... components/wifi/wifi_service_mac.mm:298: NotifyNetworkChanged(connected_network_guid); can this just return now? https://codereview.chromium.org/64683014/diff/1500001/components/wifi/wifi_se... components/wifi/wifi_service_mac.mm:381: if (wlan_observer_) { I'd rather do a CHECK (as calling SetEventObservers twice would break the expectation of the first observer). But if you want to keep it this way, that's OK. https://codereview.chromium.org/64683014/diff/1500001/components/wifi/wifi_se... components/wifi/wifi_service_mac.mm:449: NetworkProperties network_properties; if the network is connected, you should make sure network_properties contain the properties for the connected BSS. https://codereview.chromium.org/64683014/diff/1500001/components/wifi/wifi_se... components/wifi/wifi_service_mac.mm:452: if (network_properties_map.find(network_properties.guid) == you could check this before NetworkPropertiesFromCWNetwork, and if properties are found, pass the pointer to thoes to NetworkPropertiesFromCWNetwork (that way it would be easier to control if existing properties should be overwritten or not).
Toni, thanks, PTAL! https://codereview.chromium.org/64683014/diff/1500001/chrome/browser/extensio... File chrome/browser/extensions/api/networking_private/networking_private_service_client.h (right): https://codereview.chromium.org/64683014/diff/1500001/chrome/browser/extensio... chrome/browser/extensions/api/networking_private/networking_private_service_client.h:40: // them on worker thread. Always used from UI thread only. On 2014/01/28 02:05:03, tbarzic wrote: > Can you update comment and mention observing NetworkChangeNotifier (and add a > reason why this is not done in wifi_service; this part may also go to > wifi_service.h). Done. https://codereview.chromium.org/64683014/diff/1500001/chrome/browser/extensio... chrome/browser/extensions/api/networking_private/networking_private_service_client.h:41: class NetworkingPrivateServiceClient : public BrowserContextKeyedService, On 2014/01/28 02:05:03, tbarzic wrote: > public BCKeyedService to a new line Done. https://codereview.chromium.org/64683014/diff/1500001/components/wifi/wifi_se... File components/wifi/wifi_service.h (right): https://codereview.chromium.org/64683014/diff/1500001/components/wifi/wifi_se... components/wifi/wifi_service.h:112: virtual void OnNetworkChangeNotification() = 0; On 2014/01/28 02:05:03, tbarzic wrote: > Maybe > UpdateConnectedNetwork/RequestConnectedNetworkUpdate/SomethingMoreDescriptive > (WifiService does not actually listen to an OnNetworkChange notification, so the > name seems a bit out of place)? Done. https://codereview.chromium.org/64683014/diff/1500001/components/wifi/wifi_se... File components/wifi/wifi_service_mac.mm (right): https://codereview.chromium.org/64683014/diff/1500001/components/wifi/wifi_se... components/wifi/wifi_service_mac.mm:159: // Observer to get notified when network list has changed (scan complete). On 2014/01/28 02:05:03, tbarzic wrote: > I'd drop "scan complete" part Done. https://codereview.chromium.org/64683014/diff/1500001/components/wifi/wifi_se... components/wifi/wifi_service_mac.mm:161: // MessageLoopProxy to post events on UI thread. On 2014/01/28 02:05:03, tbarzic wrote: > MessageLoopProxy to which events should be posted. > > (wifi_service should not care which message loop that really is) Done. https://codereview.chromium.org/64683014/diff/1500001/components/wifi/wifi_se... components/wifi/wifi_service_mac.mm:165: // Cache of network list collected by GetVisibleNetworks. On 2014/01/28 02:05:03, tbarzic wrote: > Cached list of visible networks. Updated by |UpdateNetworks|. Done. https://codereview.chromium.org/64683014/diff/1500001/components/wifi/wifi_se... components/wifi/wifi_service_mac.mm:172: base::WeakPtrFactory<WiFiServiceMac> weak_factory_; On 2014/01/28 02:05:03, tbarzic wrote: > not needed anymore Done. https://codereview.chromium.org/64683014/diff/1500001/components/wifi/wifi_se... components/wifi/wifi_service_mac.mm:298: NotifyNetworkChanged(connected_network_guid); On 2014/01/28 02:05:03, tbarzic wrote: > can this just return now? Done. https://codereview.chromium.org/64683014/diff/1500001/components/wifi/wifi_se... components/wifi/wifi_service_mac.mm:381: if (wlan_observer_) { On 2014/01/28 02:05:03, tbarzic wrote: > I'd rather do a CHECK (as calling SetEventObservers twice would break the > expectation of the first observer). > But if you want to keep it this way, that's OK. Ok https://codereview.chromium.org/64683014/diff/1500001/components/wifi/wifi_se... components/wifi/wifi_service_mac.mm:449: NetworkProperties network_properties; On 2014/01/28 02:05:03, tbarzic wrote: > if the network is connected, you should make sure network_properties contain the > properties for the connected BSS. > > Done. https://codereview.chromium.org/64683014/diff/1500001/components/wifi/wifi_se... components/wifi/wifi_service_mac.mm:452: if (network_properties_map.find(network_properties.guid) == On 2014/01/28 02:05:03, tbarzic wrote: > you could check this before NetworkPropertiesFromCWNetwork, and if properties > are found, pass the pointer to thoes to NetworkPropertiesFromCWNetwork (that way > it would be easier to control if existing properties should be overwritten or > not). Done.
lgtm https://codereview.chromium.org/64683014/diff/1540001/components/wifi/wifi_se... File components/wifi/wifi_service_mac.mm (right): https://codereview.chromium.org/64683014/diff/1540001/components/wifi/wifi_se... components/wifi/wifi_service_mac.mm:297: if (network_guid == connected_network_guid) { nit: drop {}
LGTM https://codereview.chromium.org/64683014/diff/1540001/chrome/browser/extensio... File chrome/browser/extensions/api/networking_private/networking_private_service_client.cc (right): https://codereview.chromium.org/64683014/diff/1540001/chrome/browser/extensio... chrome/browser/extensions/api/networking_private/networking_private_service_client.cc:170: FROM_HERE, nit: indent 2 more
Thanks! https://codereview.chromium.org/64683014/diff/1540001/chrome/browser/extensio... File chrome/browser/extensions/api/networking_private/networking_private_service_client.cc (right): https://codereview.chromium.org/64683014/diff/1540001/chrome/browser/extensio... chrome/browser/extensions/api/networking_private/networking_private_service_client.cc:170: FROM_HERE, On 2014/01/28 18:28:20, rsesek wrote: > nit: indent 2 more Done. https://codereview.chromium.org/64683014/diff/1540001/components/wifi/wifi_se... File components/wifi/wifi_service_mac.mm (right): https://codereview.chromium.org/64683014/diff/1540001/components/wifi/wifi_se... components/wifi/wifi_service_mac.mm:297: if (network_guid == connected_network_guid) { On 2014/01/28 18:10:43, tbarzic wrote: > nit: drop {} Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mef@chromium.org/64683014/1550001
Message was sent while issue was closed.
Change committed as 247659 |
