Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11192024:
Add chromeos::NetworkStateManager (Closed)
|
Created:
8 years, 2 months ago by stevenjb Modified:
8 years, 1 month ago CC:
tfarina, Ben Chan Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd chromeos::NetworkStateManager to src/chromeos/network.
The intention for this class is to provide up-to-date state information about the network for the majority of the network related UI. It should also serve as a foundation for classes monitoring the state of the active network.
This code is not designed to provide detailed information about specific network service or device properties, not should it provide the ability to set (configure) those properties.
BUG=154072
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=166375
Patch Set 1 #Patch Set 2 : Cleanup, comment, and add unit tests #
Total comments: 70
Patch Set 3 : Respond to feedback #
Total comments: 49
Patch Set 4 : Respond to feedback Round 2 #
Total comments: 99
Patch Set 5 : Separate out NetworkStateHandlerImpl, address feedback #
Total comments: 13
Patch Set 6 : NetworkStateHandlerImpl -> ShillPropertyHandler #Patch Set 7 : Improve ShillPropertyHandler interface, add unit tests #Patch Set 8 : Clang fixes #
Total comments: 4
Patch Set 9 : Rebase with dbus tests in trunk #
Total comments: 55
Patch Set 10 : Address feedback #Patch Set 11 : Address feedback #
Total comments: 8
Patch Set 12 : Delegate -> Listener, ShillServiceObserver -> internal #Patch Set 13 : Address feedback from gauravesh #Patch Set 14 : Address feedback from hashimoto #
Total comments: 2
Patch Set 15 : Fix .gyp ordering #
Total comments: 2
Patch Set 16 : Fix comment #Messages
Total messages: 41 (0 generated)
This is just an early preview. It needs organizing, comments, and unit tests (at minimum). That said, I wanted to make this available for discussion. There does seem to be a fair bit of overlap with the Connectivity State doc.
OK, I've done a cleanup pass and written a modest set of unit tests. The code is now in a reviewable state. Even though this is a working implementation, I am entirely receptive to feedback on the API and even the basic approach. It is important that we are happy with the API because we will likely be living with it for quite some time. I plan to put together a design document that we can also use as a basis of discussion.
http://codereview.chromium.org/11192024/diff/3001/chromeos/network/device_sta... File chromeos/network/device_state.h (right): http://codereview.chromium.org/11192024/diff/3001/chromeos/network/device_sta... chromeos/network/device_state.h:10: // Simple class to provide device state information. Similar to NetworkState; I'd move this atop of class declaration. http://codereview.chromium.org/11192024/diff/3001/chromeos/network/device_sta... chromeos/network/device_state.h:30: private: nit: indent one more space.
http://codereview.chromium.org/11192024/diff/3001/chromeos/network/network_st... File chromeos/network/network_state_manager.h (right): http://codereview.chromium.org/11192024/diff/3001/chromeos/network/network_st... chromeos/network/network_state_manager.h:40: class CHROMEOS_EXPORT NetworkStateManager Per discussion, will rename this NetworkStateHandler
http://codereview.chromium.org/11192024/diff/3001/chromeos/network/network_st... File chromeos/network/network_state_manager.cc (right): http://codereview.chromium.org/11192024/diff/3001/chromeos/network/network_st... chromeos/network/network_state_manager.cc:27: const std::string& error_message) { Add TODO: Add error logging, including the API (e.g. Manager.EnableTechnology) and path. http://codereview.chromium.org/11192024/diff/3001/chromeos/network/network_st... File chromeos/network/network_state_manager.h (right): http://codereview.chromium.org/11192024/diff/3001/chromeos/network/network_st... chromeos/network/network_state_manager.h:82: void RequestScan() const; Will remove and make implicit in GetNetworkList(). http://codereview.chromium.org/11192024/diff/3001/chromeos/network/network_st... chromeos/network/network_state_manager.h:87: void SetTechnologyEnabled(const std::string& technology, bool enabled); Add TODO: Add an error handler callback for SetTechnologyEnabled. This is the only setter here and I don't think we need error handling for getters. I think that we should develop a standard error callback when we add NetworkConnectionHandler, then use that here (and elsewhere). http://codereview.chromium.org/11192024/diff/3001/chromeos/network/network_st... chromeos/network/network_state_manager.h:115: const NetworkStateList& network_list() const { return network_list_; } Will change to GetNetworkList() and have this request a scan (which will eventually be automatic in Shill)
http://codereview.chromium.org/11192024/diff/3001/chromeos/network/managed_st... File chromeos/network/managed_state.h (right): http://codereview.chromium.org/11192024/diff/3001/chromeos/network/managed_st... chromeos/network/managed_state.h:22: class ManagedState { Add a class comment for this class. http://codereview.chromium.org/11192024/diff/3001/chromeos/network/network_se... File chromeos/network/network_service_observer.h (right): http://codereview.chromium.org/11192024/diff/3001/chromeos/network/network_se... chromeos/network/network_service_observer.h:15: // OnPropertyChanged is called. Move this down to just before the class definition. http://codereview.chromium.org/11192024/diff/3001/chromeos/network/network_st... File chromeos/network/network_state.h (right): http://codereview.chromium.org/11192024/diff/3001/chromeos/network/network_st... chromeos/network/network_state.h:10: // Simple class to provide network state information about a network service. Move inside of namespace just before class declaration. http://codereview.chromium.org/11192024/diff/3001/chromeos/network/network_st... chromeos/network/network_state.h:20: enum DataLeft { nit: Maybe "DataPlanRemaining"? http://codereview.chromium.org/11192024/diff/3001/chromeos/network/network_st... chromeos/network/network_state.h:31: virtual bool PropertyChanged(const std::string& key, Add which interface this overrides to the comment: e.g. "ManagedState Overrides" http://codereview.chromium.org/11192024/diff/3001/chromeos/network/network_st... File chromeos/network/network_state_manager.cc (right): http://codereview.chromium.org/11192024/diff/3001/chromeos/network/network_st... chromeos/network/network_state_manager.cc:429: VLOG(2) << "GetPropertiesCallback: " << type << " : " << path; I wonder if there should be a VLOG(2) section here that prints out the actual properties? http://codereview.chromium.org/11192024/diff/3001/chromeos/network/network_st... File chromeos/network/network_state_manager.h (right): http://codereview.chromium.org/11192024/diff/3001/chromeos/network/network_st... chromeos/network/network_state_manager.h:21: // Class for tracking the list of visible networks and their state. Move down to class declaration. http://codereview.chromium.org/11192024/diff/3001/chromeos/network/network_st... chromeos/network/network_state_manager.h:49: class Observer { It may be better to move this outside of the class and put in a separate file for the reasons described at: http://code.google.com/p/chromium/issues/detail?id=141261 Which seems to outline the "right" way to do this. http://codereview.chromium.org/11192024/diff/3001/chromeos/network/network_st... chromeos/network/network_state_manager.h:119: const base::Value& value); Add OVERRIDE http://codereview.chromium.org/11192024/diff/3001/chromeos/network/network_st... chromeos/network/network_state_manager.h:125: NetworkServiceObservers; nit: NetworkServiceObserverMap instead?
My major question is, how do we expect to map the ManagedState objects to JS? This could perhaps be a bug: http://codereview.chromium.org/11192024/diff/3001/chromeos/network/network_st... http://codereview.chromium.org/11192024/diff/3001/chromeos/network/device_sta... File chromeos/network/device_state.cc (right): http://codereview.chromium.org/11192024/diff/3001/chromeos/network/device_sta... chromeos/network/device_state.cc:68: } else return false; http://codereview.chromium.org/11192024/diff/3001/chromeos/network/device_sta... File chromeos/network/device_state.h (right): http://codereview.chromium.org/11192024/diff/3001/chromeos/network/device_sta... chromeos/network/device_state.h:8: #include "chromeos/network/managed_state.h" please add here and in other files where necessary: #include "base/compiler_specific.h" for OVERRIDE #include "base/basisctypes.h" for DISALLOW_COPY_AND_ASSIGN http://codereview.chromium.org/11192024/diff/3001/chromeos/network/device_sta... chromeos/network/device_state.h:9: forward declare base::Value #include <string> http://codereview.chromium.org/11192024/diff/3001/chromeos/network/network_se... File chromeos/network/network_service_observer.h (right): http://codereview.chromium.org/11192024/diff/3001/chromeos/network/network_se... chromeos/network/network_service_observer.h:15: // OnPropertyChanged is called. But when is that called? Only if the network is connected/connecting or also if it's unconnected? If it's the latter, why does the NetworkStateHandler only register observers for connected/-ing networks? http://codereview.chromium.org/11192024/diff/3001/chromeos/network/network_se... chromeos/network/network_service_observer.h:16: forward declare base::Value http://codereview.chromium.org/11192024/diff/3001/chromeos/network/network_se... chromeos/network/network_service_observer.h:25: NetworkServiceObserver(const std::string& service_path, Handler handler); according to comments in callback.h, callbacks should be passed as const& http://codereview.chromium.org/11192024/diff/3001/chromeos/network/network_st... File chromeos/network/network_state_manager.cc (right): http://codereview.chromium.org/11192024/diff/3001/chromeos/network/network_st... chromeos/network/network_state_manager.cc:290: // Use reinterpret_cast here to avoid copying the list to a list of identical We shouldn't be concerned about the copying. Here you cast in order to generalize UpdateManagedList. But, since that function modifies that list, we lose all type safety. http://codereview.chromium.org/11192024/diff/3001/chromeos/network/network_st... chromeos/network/network_state_manager.cc:294: return reinterpret_cast<ManagedStateList*>(&network_list_); Is it worth to include such an unsafe cast for this functionality? Why not make UpdateManagedList a template to ensure type safety: template<class Type> void UpdateManagedList( ManagedType type, const base::ListValue& entries, std::list<Type> managed_list) { ... managed_list->push_back(new Type(path)); } The code duplication is negligible in this case. http://codereview.chromium.org/11192024/diff/3001/chromeos/network/network_st... chromeos/network/network_state_manager.cc:348: break; // Connected and connecting networks are listed first. Hm. Are you sure that the NetworkState of disconnected networks is updated correctly? These NetworkStates can still be accessed by GetNetworkState, right? Please explain somewhere, what you do depending on the connect-state (observe or not, but always store in network list). I find it a bit difficult to read that from the code. http://codereview.chromium.org/11192024/diff/3001/chromeos/network/network_st... File chromeos/network/network_state_manager.h (right): http://codereview.chromium.org/11192024/diff/3001/chromeos/network/network_st... chromeos/network/network_state_manager.h:23: // each visible network. It is not used to change state, and should not be Actually, SetTechnologyEnabled does change state. Please adapt the comment and emphasize at each state-changing method that it does so. http://codereview.chromium.org/11192024/diff/3001/chromeos/network/network_st... chromeos/network/network_state_manager.h:25: // This class must not outlive the ShillManagerClient instance. Maybe give also a comment on what the class guarantees and what not: - Events/State changes may be missed (?) - State getters are non-blocking and provide the last seen (maybe outdated) state. http://codereview.chromium.org/11192024/diff/3001/chromeos/network/network_st... chromeos/network/network_state_manager.h:50: public: You could disallow copy/assign here, otherwise there will never be compiler errors about copies/assigns to references of type Observer. Then an empty constructor is also required. http://codereview.chromium.org/11192024/diff/3001/chromeos/network/network_st... chromeos/network/network_state_manager.h:84: // Manage the state of enabled / available technologies. A bit misleading. Better split into separate comments for enabled and available. http://codereview.chromium.org/11192024/diff/3001/chromeos/network/network_st... chromeos/network/network_state_manager.h:87: void SetTechnologyEnabled(const std::string& technology, bool enabled); Emphasize, that this does not change which technologies are tracked, but actually tells shill which technologies to use. (perhaps a reference to manager-api.txt is sufficient). I thought you wanted to expose the asynchronous calls up to the UI (e.g. webui)? But SetTechnologyEnabled doesn't notify about success/failure. http://codereview.chromium.org/11192024/diff/3001/chromeos/network/network_st... chromeos/network/network_state_manager.h:121: private: Since this is a rather central manager. Should the implementation perhaps be put completely into the .cc file to avoid recompilation on implementation changes? http://codereview.chromium.org/11192024/diff/3001/chromeos/network/network_st... chromeos/network/network_state_manager.h:128: DeviceState* GetModifiableDeviceState(const std::string& path); Make these two methods const (using const_iterator), then the const_cast should be unnecessary. http://codereview.chromium.org/11192024/diff/3001/chromeos/network/network_st... chromeos/network/network_state_manager.h:187: // Convenince pointer for ShillManagerClient nit: Convenience http://codereview.chromium.org/11192024/diff/3001/chromeos/network/network_st... chromeos/network/network_state_manager.h:194: NetworkStateList network_list_; These lists are never modified, but only new ones created by UpdateManagedList. Why not use vector<>? Or even scoped_vector<> to ensure correct deletion and document ownership?
http://codereview.chromium.org/11192024/diff/3001/chromeos/network/network_st... File chromeos/network/network_state_manager.h (right): http://codereview.chromium.org/11192024/diff/3001/chromeos/network/network_st... chromeos/network/network_state_manager.h:87: void SetTechnologyEnabled(const std::string& technology, bool enabled); On 2012/10/23 23:12:09, stevenjb (chromium) wrote: > Add TODO: Add an error handler callback for SetTechnologyEnabled. As a general rule, how will we know about DBus call/Shill failures for the methods that return cached data? For instance, if you can't get device state because the (earlier cache-filling) shill call failed for some reason, how will I know what the error is at the UI level?
Note: I'll be doing another update to observer all services in the watchlist so that we notice when services go online. I'm also going to make sure we request an update for a service when we start observing it to ensure the state is always accurate. http://codereview.chromium.org/11192024/diff/3001/chromeos/network/device_sta... File chromeos/network/device_state.cc (right): http://codereview.chromium.org/11192024/diff/3001/chromeos/network/device_sta... chromeos/network/device_state.cc:68: } On 2012/10/24 14:41:42, pneubeck wrote: > else return false; Done. http://codereview.chromium.org/11192024/diff/3001/chromeos/network/device_sta... File chromeos/network/device_state.h (right): http://codereview.chromium.org/11192024/diff/3001/chromeos/network/device_sta... chromeos/network/device_state.h:9: On 2012/10/24 14:41:42, pneubeck wrote: > forward declare base::Value > #include <string> These are all guaranteed to be in managed_state.h, why repeat them here? http://codereview.chromium.org/11192024/diff/3001/chromeos/network/device_sta... chromeos/network/device_state.h:10: // Simple class to provide device state information. Similar to NetworkState; On 2012/10/20 02:34:17, tfarina wrote: > I'd move this atop of class declaration. Done. http://codereview.chromium.org/11192024/diff/3001/chromeos/network/device_sta... chromeos/network/device_state.h:30: private: On 2012/10/20 02:34:17, tfarina wrote: > nit: indent one more space. Done. http://codereview.chromium.org/11192024/diff/3001/chromeos/network/managed_st... File chromeos/network/managed_state.h (right): http://codereview.chromium.org/11192024/diff/3001/chromeos/network/managed_st... chromeos/network/managed_state.h:22: class ManagedState { On 2012/10/23 23:19:10, Greg Spencer (Chromium) wrote: > Add a class comment for this class. Done. http://codereview.chromium.org/11192024/diff/3001/chromeos/network/network_se... File chromeos/network/network_service_observer.h (right): http://codereview.chromium.org/11192024/diff/3001/chromeos/network/network_se... chromeos/network/network_service_observer.h:15: // OnPropertyChanged is called. On 2012/10/23 23:19:10, Greg Spencer (Chromium) wrote: > Move this down to just before the class definition. Done. http://codereview.chromium.org/11192024/diff/3001/chromeos/network/network_se... chromeos/network/network_service_observer.h:15: // OnPropertyChanged is called. On 2012/10/24 14:41:42, pneubeck wrote: > But when is that called? Only if the network is connected/connecting or also if > it's unconnected? > If it's the latter, why does the NetworkStateHandler only register observers for > connected/-ing networks? This should get called when any property for an observed service changes. However, your comment made me realize that there is a flaw in how NetworkStateManager determines which services to observe (see the email I just sent). http://codereview.chromium.org/11192024/diff/3001/chromeos/network/network_se... chromeos/network/network_service_observer.h:16: On 2012/10/24 14:41:42, pneubeck wrote: > forward declare base::Value In the ShillPropertyChangedObserver signature so unnecessary. http://codereview.chromium.org/11192024/diff/3001/chromeos/network/network_se... chromeos/network/network_service_observer.h:25: NetworkServiceObserver(const std::string& service_path, Handler handler); On 2012/10/24 14:41:42, pneubeck wrote: > according to comments in callback.h, > callbacks should be passed as const& Done. http://codereview.chromium.org/11192024/diff/3001/chromeos/network/network_st... File chromeos/network/network_state.h (right): http://codereview.chromium.org/11192024/diff/3001/chromeos/network/network_st... chromeos/network/network_state.h:10: // Simple class to provide network state information about a network service. On 2012/10/23 23:19:10, Greg Spencer (Chromium) wrote: > Move inside of namespace just before class declaration. Done. http://codereview.chromium.org/11192024/diff/3001/chromeos/network/network_st... chromeos/network/network_state.h:20: enum DataLeft { On 2012/10/23 23:19:10, Greg Spencer (Chromium) wrote: > nit: Maybe "DataPlanRemaining"? Compromise: DataRemaing http://codereview.chromium.org/11192024/diff/3001/chromeos/network/network_st... chromeos/network/network_state.h:31: virtual bool PropertyChanged(const std::string& key, On 2012/10/23 23:19:10, Greg Spencer (Chromium) wrote: > Add which interface this overrides to the comment: e.g. "ManagedState Overrides" Done. http://codereview.chromium.org/11192024/diff/3001/chromeos/network/network_st... File chromeos/network/network_state_manager.cc (right): http://codereview.chromium.org/11192024/diff/3001/chromeos/network/network_st... chromeos/network/network_state_manager.cc:290: // Use reinterpret_cast here to avoid copying the list to a list of identical On 2012/10/24 14:41:42, pneubeck wrote: > We shouldn't be concerned about the copying. Here you cast in order to > generalize UpdateManagedList. But, since that function modifies that list, we > lose all type safety. I am concerned about copying, we do this fairly often. We're never upcasting here, what type safety issues are you concerned with? http://codereview.chromium.org/11192024/diff/3001/chromeos/network/network_st... chromeos/network/network_state_manager.cc:294: return reinterpret_cast<ManagedStateList*>(&network_list_); On 2012/10/24 14:41:42, pneubeck wrote: > Is it worth to include such an unsafe cast for this functionality? > Why not make UpdateManagedList a template to ensure type safety: > > template<class Type> > void UpdateManagedList( > ManagedType type, > const base::ListValue& entries, > std::list<Type> managed_list) { > > ... managed_list->push_back(new Type(path)); > } > > The code duplication is negligible in this case. We are downcasting the pointers here, so it's not really unsafe, and function templates are a pain to debug. http://codereview.chromium.org/11192024/diff/3001/chromeos/network/network_st... chromeos/network/network_state_manager.cc:348: break; // Connected and connecting networks are listed first. On 2012/10/24 14:41:42, pneubeck wrote: > Hm. Are you sure that the NetworkState of disconnected networks is updated > correctly? > These NetworkStates can still be accessed by GetNetworkState, right? > > Please explain somewhere, what you do depending on the connect-state (observe or > not, but always store in network list). I find it a bit difficult to read that > from the code. Will comment this better when updated in response to my email query. http://codereview.chromium.org/11192024/diff/3001/chromeos/network/network_st... chromeos/network/network_state_manager.cc:429: VLOG(2) << "GetPropertiesCallback: " << type << " : " << path; On 2012/10/23 23:19:10, Greg Spencer (Chromium) wrote: > I wonder if there should be a VLOG(2) section here that prints out the actual > properties? My pattern is to add VLOG(2) as I need it and not try to anticipate too much. http://codereview.chromium.org/11192024/diff/3001/chromeos/network/network_st... File chromeos/network/network_state_manager.h (right): http://codereview.chromium.org/11192024/diff/3001/chromeos/network/network_st... chromeos/network/network_state_manager.h:21: // Class for tracking the list of visible networks and their state. On 2012/10/23 23:19:10, Greg Spencer (Chromium) wrote: > Move down to class declaration. Done. http://codereview.chromium.org/11192024/diff/3001/chromeos/network/network_st... chromeos/network/network_state_manager.h:23: // each visible network. It is not used to change state, and should not be On 2012/10/24 14:41:42, pneubeck wrote: > Actually, SetTechnologyEnabled does change state. > Please adapt the comment and emphasize at each state-changing method that it > does so. Done. http://codereview.chromium.org/11192024/diff/3001/chromeos/network/network_st... chromeos/network/network_state_manager.h:25: // This class must not outlive the ShillManagerClient instance. On 2012/10/24 14:41:42, pneubeck wrote: > Maybe give also a comment on what the class guarantees and what not: > - Events/State changes may be missed (?) We will ensure this is not true. > - State getters are non-blocking and provide the last seen (maybe outdated) > state. Comments changed. http://codereview.chromium.org/11192024/diff/3001/chromeos/network/network_st... chromeos/network/network_state_manager.h:40: class CHROMEOS_EXPORT NetworkStateManager On 2012/10/23 22:59:38, stevenjb (chromium) wrote: > Per discussion, will rename this NetworkStateHandler Done. http://codereview.chromium.org/11192024/diff/3001/chromeos/network/network_st... chromeos/network/network_state_manager.h:49: class Observer { On 2012/10/23 23:19:10, Greg Spencer (Chromium) wrote: > It may be better to move this outside of the class and put in a separate file > for the reasons described at: > > http://code.google.com/p/chromium/issues/detail?id=141261 > > Which seems to outline the "right" way to do this. Done. http://codereview.chromium.org/11192024/diff/3001/chromeos/network/network_st... chromeos/network/network_state_manager.h:50: public: On 2012/10/24 14:41:42, pneubeck wrote: > You could disallow copy/assign here, otherwise there will never be compiler > errors about copies/assigns to references of type Observer. > Then an empty constructor is also required. Done http://codereview.chromium.org/11192024/diff/3001/chromeos/network/network_st... chromeos/network/network_state_manager.h:82: void RequestScan() const; On 2012/10/23 23:12:09, stevenjb (chromium) wrote: > Will remove and make implicit in GetNetworkList(). Done. http://codereview.chromium.org/11192024/diff/3001/chromeos/network/network_st... chromeos/network/network_state_manager.h:87: void SetTechnologyEnabled(const std::string& technology, bool enabled); On 2012/10/24 22:27:42, Greg Spencer (Chromium) wrote: > On 2012/10/23 23:12:09, stevenjb (chromium) wrote: > > Add TODO: Add an error handler callback for SetTechnologyEnabled. > > As a general rule, how will we know about DBus call/Shill failures for the > methods that return cached data? For instance, if you can't get device state > because the (earlier cache-filling) shill call failed for some reason, how will > I know what the error is at the UI level? It is the responsibility of this class to maintain accurate state. If there are failures communicating with Shill it should handle those and log them. I'm not sure there is any real value to adding an "unknown" state for things like Technlogy[Enabled|Available]. http://codereview.chromium.org/11192024/diff/3001/chromeos/network/network_st... chromeos/network/network_state_manager.h:115: const NetworkStateList& network_list() const { return network_list_; } On 2012/10/23 23:12:09, stevenjb (chromium) wrote: > Will change to GetNetworkList() and have this request a scan (which will > eventually be automatic in Shill) Done. http://codereview.chromium.org/11192024/diff/3001/chromeos/network/network_st... chromeos/network/network_state_manager.h:119: const base::Value& value); On 2012/10/23 23:19:10, Greg Spencer (Chromium) wrote: > Add OVERRIDE Done. http://codereview.chromium.org/11192024/diff/3001/chromeos/network/network_st... chromeos/network/network_state_manager.h:121: private: On 2012/10/24 14:41:42, pneubeck wrote: > Since this is a rather central manager. Should the implementation perhaps be put > completely into the .cc file to avoid recompilation on implementation changes? In general in Chrome we rely on fast compilation tools rather than implementation abstractions. I don't expect that there will be frequent header changes once initial development completes. http://codereview.chromium.org/11192024/diff/3001/chromeos/network/network_st... chromeos/network/network_state_manager.h:125: NetworkServiceObservers; On 2012/10/23 23:19:10, Greg Spencer (Chromium) wrote: > nit: NetworkServiceObserverMap instead? Done. http://codereview.chromium.org/11192024/diff/3001/chromeos/network/network_st... chromeos/network/network_state_manager.h:128: DeviceState* GetModifiableDeviceState(const std::string& path); On 2012/10/24 14:41:42, pneubeck wrote: > Make these two methods const (using const_iterator), then the const_cast should > be unnecessary. The const cast is for the return values, to avoid duplicate code. They would still be required if these methods where const. It is generally preferred (at least by Ben) that classes returning modifiable members are not const. http://codereview.chromium.org/11192024/diff/3001/chromeos/network/network_st... chromeos/network/network_state_manager.h:187: // Convenince pointer for ShillManagerClient On 2012/10/24 14:41:42, pneubeck wrote: > nit: Convenience Done. http://codereview.chromium.org/11192024/diff/3001/chromeos/network/network_st... chromeos/network/network_state_manager.h:194: NetworkStateList network_list_; On 2012/10/24 14:41:42, pneubeck wrote: > These lists are never modified, but only new ones created by UpdateManagedList. > Why not use vector<>? Or even scoped_vector<> to ensure correct deletion and > document ownership? Evolutionary artifact. scoped_vector<> is high overhead, but vector<> is fine.
I still wonder we expect to map the state objects to JS? http://codereview.chromium.org/11192024/diff/13002/chromeos/network/managed_s... File chromeos/network/managed_state.h (right): http://codereview.chromium.org/11192024/diff/13002/chromeos/network/managed_s... chromeos/network/managed_state.h:13: #include "third_party/cros_system_api/dbus/service_constants.h" unused? http://codereview.chromium.org/11192024/diff/13002/chromeos/network/managed_s... chromeos/network/managed_state.h:33: } virtual destructor http://codereview.chromium.org/11192024/diff/13002/chromeos/network/managed_s... chromeos/network/managed_state.h:48: std::string path_; disallow copy/assign http://codereview.chromium.org/11192024/diff/13002/chromeos/network/network_s... File chromeos/network/network_service_observer.h (right): http://codereview.chromium.org/11192024/diff/13002/chromeos/network/network_s... chromeos/network/network_service_observer.h:11: #include "chromeos/dbus/shill_property_changed_observer.h" Btw. shill_property_changed_observer.h only forward declares base::Value. Perhaps a point against your argument in the other file... http://codereview.chromium.org/11192024/diff/13002/chromeos/network/network_s... chromeos/network/network_service_observer.h:18: class NetworkServiceObserver : public ShillPropertyChangedObserver { You could call this ShillServiceObserver to make the difference to NetworkState and other Network* clearer. http://codereview.chromium.org/11192024/diff/13002/chromeos/network/network_s... File chromeos/network/network_state.cc (right): http://codereview.chromium.org/11192024/diff/13002/chromeos/network/network_s... chromeos/network/network_state.cc:8: #include "base/values.h" logging.h unused missing include service_constants. http://codereview.chromium.org/11192024/diff/13002/chromeos/network/network_s... File chromeos/network/network_state.h (right): http://codereview.chromium.org/11192024/diff/13002/chromeos/network/network_s... chromeos/network/network_state.h:38: const std::string& ip_config() const { return ip_config_; } Which format has this? If this class is directly accessed by the UI, then we should settle with ONC's definition of ip_config and provide a dictionary. If the UI however does not need the whole ipconfig but only, say, the ipaddress, then we should rename this method and ONC also wouldn't matter. http://codereview.chromium.org/11192024/diff/13002/chromeos/network/network_s... File chromeos/network/network_state_handler.cc (right): http://codereview.chromium.org/11192024/diff/13002/chromeos/network/network_s... chromeos/network/network_state_handler.cc:7: #include <algorithm> unused? http://codereview.chromium.org/11192024/diff/13002/chromeos/network/network_s... chromeos/network/network_state_handler.cc:104: return const_cast<NetworkStateHandler*>(this)->GetModifiableDeviceState(path); > It is generally preferred (at least by Ben) that classes > returning modifiable members are not const. I understand that for public functions. But why would that be a problem here? The GetModifiable*State methods are private methods and not part of the public interface or to derived classes (which we don't expect). > The const cast is for the return values, to avoid > duplicate code. They would still be required if these > methods where const. The const_cast is really not necessary: const DeviceState* GetDeviceState( const std::string& path) const { return GetModifiableDeviceState(path); } DeviceState* GetModifiableDeviceState( const std::string& path) const { for (DeviceStateList::const_iterator iter = ...) ... } http://codereview.chromium.org/11192024/diff/13002/chromeos/network/network_s... chromeos/network/network_state_handler.cc:192: RequestScan(); Why not expose RequestScan publicly and remove it here? The list you return here is not updated by the scan on return time. So the caller has to observe NetworkList changes in anyway. The current implementation only provokes unintentional scan requests. http://codereview.chromium.org/11192024/diff/13002/chromeos/network/network_s... chromeos/network/network_state_handler.cc:236: for (base::DictionaryValue::key_iterator iter = properties.begin_keys(); You could use DictionaryValue::Iterator in this case. Improves readability as GetWithoutPathExpansion and the if can be removed. Applies to most cases in this file. http://codereview.chromium.org/11192024/diff/13002/chromeos/network/network_s... chromeos/network/network_state_handler.cc:287: // Use reinterpret_cast here to avoid copying the list to a list of identical > I am concerned about copying, we do this fairly often. Copying a vector<*> of about 10 entries should be very very fast, the one alloc will take the most time. Why should that be a problem? How often should this function be called? Do we have any measurements? Although, I assume that Dbus will make problems much earlier. Anyways, in the template version there is also no copy necessary. > We're never upcasting here, what type safety issues are > you concerned with? This is neither a downcast nor a upcast. list<A> and list<B> (A != B) are in no subtype relation. As list<A> and list<B> don't share the same code, are you sure that the compiler even compiles both code instantiations of list<T> equally? And even if the cast would work, the resulting list is modifiable. So the compiler doesn't complain if you insert the wrong elements into the list, e.g you can add a NetworkState to the DeviceState list. That is what I meant with losing type-safety. To summarize: this cast is unchecked and by definition incorrect and because of that, operations on the resulting list are unsafe. About the template version. Why should the debugging be a pain in that case? http://codereview.chromium.org/11192024/diff/13002/chromeos/network/network_s... chromeos/network/network_state_handler.cc:344: if (!network->IsConnectedState() && !network->IsConnectingState()) Doesn't this also mean, that we cannot observe e.g. the signal strength of a idle network anymore? I'm still not getting the point of the "not observing idle networks". If a network is not connected, it actually shouldn't change a lot and thus we will not get many state updates. http://codereview.chromium.org/11192024/diff/13002/chromeos/network/network_s... chromeos/network/network_state_handler.cc:436: for (ManagedStateList::iterator iter = list->begin(); Here, you could use ManagedState* managed = GetModifiable*State(path); http://codereview.chromium.org/11192024/diff/13002/chromeos/network/network_s... chromeos/network/network_state_handler.cc:457: for (base::DictionaryValue::key_iterator iter = properties.begin_keys(); DictionaryValue::Iterator http://codereview.chromium.org/11192024/diff/13002/chromeos/network/network_s... chromeos/network/network_state_handler.cc:462: // better handled here than in NetworkService::PropertyChanged (e.g. NetworkState::PropertyChanged? http://codereview.chromium.org/11192024/diff/13002/chromeos/network/network_s... chromeos/network/network_state_handler.cc:468: DBusThreadManager::Get()->GetShillIPConfigClient()->GetProperties( This means, that the state can even be inconsistent! Some properties are already updated and ipconfig may not be. Please mention that in the class comment. We should get Shill to send us ipconfig as part of the property dictionary. Actually, the current shill interface allows only to observe property changes one-by-one. So, properties that are not encapsulated in a dictionary will potentially be "inconsistent" (one value already new, the other still old). We should get Shill to support observing a dictionary of property changes, similar to a transaction. http://codereview.chromium.org/11192024/diff/13002/chromeos/network/network_s... chromeos/network/network_state_handler.cc:503: const std::string& path, this could be both ipconfig path or service path, please give it an unambiguous name. http://codereview.chromium.org/11192024/diff/13002/chromeos/network/network_s... chromeos/network/network_state_handler.cc:511: for (base::DictionaryValue::key_iterator iter = properties.begin_keys(); DictionaryValue::Iterator http://codereview.chromium.org/11192024/diff/13002/chromeos/network/network_s... chromeos/network/network_state_handler.cc:533: shill_manager_->RequestScan(flimflam::kTypeWifi, According to manager-api.txt you can simply use "" instead of a particular technology: void RequestScan(string type) Request a scan for the specified technology. If type is the string "" then a scan request is made for each technology. http://codereview.chromium.org/11192024/diff/13002/chromeos/network/network_s... File chromeos/network/network_state_handler.h (right): http://codereview.chromium.org/11192024/diff/13002/chromeos/network/network_s... chromeos/network/network_state_handler.h:48: : public ShillPropertyChangedObserver { If you want to make the public interface cleaner, you could move the Observer implementation into a private delegate (as private inheritance is not wanted by the styleguide). This delegate could be near identical to NetworkServiceObserver. Then the OnPropertyChanged method would vanish from the public interface. http://codereview.chromium.org/11192024/diff/13002/chromeos/network/network_s... chromeos/network/network_state_handler.h:50: typedef std::vector<ManagedState*> ManagedStateList; On 2012/10/25 00:41:58, stevenjb (chromium) wrote: > On 2012/10/24 14:41:42, pneubeck wrote: > > These lists are never modified, but only new ones created by > UpdateManagedList. > > Why not use vector<>? Or even scoped_vector<> to ensure correct deletion and > > document ownership? > Evolutionary artifact. scoped_vector<> is high overhead, but vector<> is fine. Why is scoped_vector<> high overhead? According to the implementation there is no overhead except the deletion of elements when necessary (.clear, .erase). And the smart-point-guideline mentions "For the common case of a vector that owns pointers, use ScopedVector. For other cases, we often use raw pointers and clean up manually." http://dev.chromium.org/developers/smart-pointer-guidelines http://codereview.chromium.org/11192024/diff/13002/chromeos/network/network_s... File chromeos/network/network_state_handler_observer.h (right): http://codereview.chromium.org/11192024/diff/13002/chromeos/network/network_s... chromeos/network/network_state_handler_observer.h:25: // If |property| is non empty than that property has changed. Otherwise Nit: than -> then The "otherwise" doesn't sound sophisticated. Should we perhaps provide a vector of the changed properties? http://codereview.chromium.org/11192024/diff/13002/chromeos/network/network_s... chromeos/network/network_state_handler_observer.h:27: virtual void NetworkManagerChanged(const std::string& property); I'm rather unhappy about this mixture of property names/keys (here |property|) with the accessors in the State classes. That means, every user of the state classes has to know the mapping from the method names (like security()) to the shill constants (flimflam::kSecurityProperty) which he has to look up in the incomplete shill documentation. Why that redundancy and mental-complication? Wouldn't it be easier to agree on a well-defined state format (similar to ONC) and put it into a dictionary? e.g. a network state could look like: { type: "network", path: "service/123", network : { type: "wifi", name: "my wifi", ssid: "my ssid", signal_strength: 123 } } Then we also get the translation to json/JS for free! And changes could simply be a subset of that dictionary.
http://codereview.chromium.org/11192024/diff/3001/chromeos/network/network_st... File chromeos/network/network_state_manager.h (right): http://codereview.chromium.org/11192024/diff/3001/chromeos/network/network_st... chromeos/network/network_state_manager.h:87: void SetTechnologyEnabled(const std::string& technology, bool enabled); On 2012/10/25 00:41:58, stevenjb (chromium) wrote: > It is the responsibility of this class to maintain accurate state. If there are > failures communicating with Shill it should handle those and log them. I'm not > sure there is any real value to adding an "unknown" state for things like > Technlogy[Enabled|Available]. Well, I was more thinking about errors returned by Shill. Perhaps there should be an error return for the things that can have errors, and that if there was an error fetching things earlier, then we just give out the last error that occurred, if any, and reset it when we succeed or get a new failure. But I guess most of the errors that could be generated in this class are going to be communication errors on DBus instead of Shill API errors, so it's probably OK to just let the class deal with it internally. http://codereview.chromium.org/11192024/diff/3001/chromeos/network/network_st... chromeos/network/network_state_manager.h:98: // Returns the "active" network (first network in the list if connected). What does this return if not connected? (I'm assuming NULL, but it's not documented)
http://codereview.chromium.org/11192024/diff/3001/chromeos/network/network_st... File chromeos/network/network_state_manager.h (right): http://codereview.chromium.org/11192024/diff/3001/chromeos/network/network_st... chromeos/network/network_state_manager.h:98: // Returns the "active" network (first network in the list if connected). On 2012/10/25 21:51:42, Greg Spencer (Chromium) wrote: > What does this return if not connected? (I'm assuming NULL, but it's not > documented) Done. http://codereview.chromium.org/11192024/diff/13002/chromeos/network/managed_s... File chromeos/network/managed_state.h (right): http://codereview.chromium.org/11192024/diff/13002/chromeos/network/managed_s... chromeos/network/managed_state.h:13: #include "third_party/cros_system_api/dbus/service_constants.h" On 2012/10/25 14:42:17, pneubeck wrote: > unused? PropertyChanged keys are defined here. I separated the include with a comment. http://codereview.chromium.org/11192024/diff/13002/chromeos/network/managed_s... chromeos/network/managed_state.h:33: } On 2012/10/25 14:42:17, pneubeck wrote: > virtual destructor Done. http://codereview.chromium.org/11192024/diff/13002/chromeos/network/managed_s... chromeos/network/managed_state.h:48: std::string path_; On 2012/10/25 14:42:17, pneubeck wrote: > disallow copy/assign Done. http://codereview.chromium.org/11192024/diff/13002/chromeos/network/network_s... File chromeos/network/network_service_observer.h (right): http://codereview.chromium.org/11192024/diff/13002/chromeos/network/network_s... chromeos/network/network_service_observer.h:11: #include "chromeos/dbus/shill_property_changed_observer.h" On 2012/10/25 14:42:17, pneubeck wrote: > Btw. shill_property_changed_observer.h only forward declares base::Value. > Perhaps a point against your argument in the other file... Forward declaration is sufficient here though, yes? Not sure I understand your meaning. http://codereview.chromium.org/11192024/diff/13002/chromeos/network/network_s... chromeos/network/network_service_observer.h:18: class NetworkServiceObserver : public ShillPropertyChangedObserver { On 2012/10/25 14:42:17, pneubeck wrote: > You could call this ShillServiceObserver to make the difference to NetworkState > and other Network* clearer. Yeah, that's better, done. http://codereview.chromium.org/11192024/diff/13002/chromeos/network/network_s... File chromeos/network/network_state.cc (right): http://codereview.chromium.org/11192024/diff/13002/chromeos/network/network_s... chromeos/network/network_state.cc:8: #include "base/values.h" On 2012/10/25 14:42:17, pneubeck wrote: > logging.h unused > missing include service_constants. logging.h removed (although we'll probably need it again eventually). service_constants.h is in managed_state.h as explained. http://codereview.chromium.org/11192024/diff/13002/chromeos/network/network_s... File chromeos/network/network_state.h (right): http://codereview.chromium.org/11192024/diff/13002/chromeos/network/network_s... chromeos/network/network_state.h:38: const std::string& ip_config() const { return ip_config_; } On 2012/10/25 14:42:17, pneubeck wrote: > Which format has this? > If this class is directly accessed by the UI, then we should settle with ONC's > definition of ip_config and provide a dictionary. > If the UI however does not need the whole ipconfig but only, say, the ipaddress, > then we should rename this method and ONC also wouldn't matter. You're absolutely correct, this should be ip_address. The vast majority of the UI only cares about ip address. Changed. http://codereview.chromium.org/11192024/diff/13002/chromeos/network/network_s... File chromeos/network/network_state_handler.cc (right): http://codereview.chromium.org/11192024/diff/13002/chromeos/network/network_s... chromeos/network/network_state_handler.cc:7: #include <algorithm> On 2012/10/25 14:42:17, pneubeck wrote: > unused? Removed http://codereview.chromium.org/11192024/diff/13002/chromeos/network/network_s... chromeos/network/network_state_handler.cc:104: return const_cast<NetworkStateHandler*>(this)->GetModifiableDeviceState(path); Yeah, I see your point. It seems a bit awkward, but so is const_cast. I don't feel strongly either way, so I'll change it. I added a comment in the header for GetModifiable*. On 2012/10/25 14:42:17, pneubeck wrote: > > It is generally preferred (at least by Ben) that classes > > returning modifiable members are not const. > I understand that for public functions. But why would that be a problem here? > The GetModifiable*State methods are private methods and not part of the public > interface or to derived classes (which we don't expect). > > > The const cast is for the return values, to avoid > > duplicate code. They would still be required if these > > methods where const. > The const_cast is really not necessary: > > const DeviceState* GetDeviceState( > const std::string& path) const { > return GetModifiableDeviceState(path); GetModifiableDeviceState() > } > > DeviceState* GetModifiableDeviceState( > const std::string& path) const { > for (DeviceStateList::const_iterator iter = ...) > ... > } http://codereview.chromium.org/11192024/diff/13002/chromeos/network/network_s... chromeos/network/network_state_handler.cc:192: RequestScan(); On 2012/10/25 14:42:17, pneubeck wrote: > Why not expose RequestScan publicly and remove it here? The list you return here > is not updated by the scan on return time. So the caller has to observe > NetworkList changes in anyway. > The current implementation only provokes unintentional scan requests. We decidedly to explicitly not expose RequestScan. Eventually this is something that will be triggered implicitly in Shill and the RequestScan will be a no-op. The assumption is that if the UI wants the list of networks, it wants to know their latest state also, i.e. the scan is not unintentional. http://codereview.chromium.org/11192024/diff/13002/chromeos/network/network_s... chromeos/network/network_state_handler.cc:236: for (base::DictionaryValue::key_iterator iter = properties.begin_keys(); On 2012/10/25 14:42:17, pneubeck wrote: > You could use DictionaryValue::Iterator in this case. > Improves readability as GetWithoutPathExpansion and the if can be removed. > > Applies to most cases in this file. I'd never seen that before. Nice. Done. http://codereview.chromium.org/11192024/diff/13002/chromeos/network/network_s... chromeos/network/network_state_handler.cc:287: // Use reinterpret_cast here to avoid copying the list to a list of identical I have had difficulty stepping through templated functions in the past. It's one of the reasons I prefer to avoid them. I also find them a pain in the ass to read. <A> and <B> are pointers, and we're downcasting the pointers. There is no typesafe way to do this in C++, but we *know* that network_list_ and device_list_ contain pointers to ManagedState, so treating them as lists to baseclass pointers is safe. So, yes, in theory, there are plenty of things the compiler can't check for. However, this is a private method, and misuse of it would be, well, a bug. If you really have a problem with this, I'll do it another way, but not with function templates. Maybe with a templated wrapper class for the lists, I'll have to think on it. http://codereview.chromium.org/11192024/diff/13002/chromeos/network/network_s... chromeos/network/network_state_handler.cc:344: if (!network->IsConnectedState() && !network->IsConnectingState()) On 2012/10/25 14:42:17, pneubeck wrote: > Doesn't this also mean, that we cannot observe e.g. the signal strength of a > idle network anymore? > > I'm still not getting the point of the "not observing idle networks". If a > network is not connected, it actually shouldn't change a lot and thus we will > not get many state updates. Shill does not update the strength or other properties of idle networks, so observing them is just extra overhead, except for the case where they come out of idle, which would have been a bug as described in the email thread. Keep in mind that in a busy area (like my condo), there can be O(100) idle networks. This will switch to observing members of the watch list (all networks for now), and we will rely on Shill to pair this down to "interesting" networks. I will put that in a separate patch. http://codereview.chromium.org/11192024/diff/13002/chromeos/network/network_s... chromeos/network/network_state_handler.cc:436: for (ManagedStateList::iterator iter = list->begin(); On 2012/10/25 14:42:17, pneubeck wrote: > Here, you could use > ManagedState* managed = GetModifiable*State(path); I've been trying to keep Type specific code out of these (i.e. that would require an if() to call the right method. Let me think on a better way to handle the common abstraction that will also work for Get*State. http://codereview.chromium.org/11192024/diff/13002/chromeos/network/network_s... chromeos/network/network_state_handler.cc:457: for (base::DictionaryValue::key_iterator iter = properties.begin_keys(); On 2012/10/25 14:42:17, pneubeck wrote: > DictionaryValue::Iterator Done. http://codereview.chromium.org/11192024/diff/13002/chromeos/network/network_s... chromeos/network/network_state_handler.cc:462: // better handled here than in NetworkService::PropertyChanged (e.g. On 2012/10/25 14:42:17, pneubeck wrote: > NetworkState::PropertyChanged? Done. http://codereview.chromium.org/11192024/diff/13002/chromeos/network/network_s... chromeos/network/network_state_handler.cc:468: DBusThreadManager::Get()->GetShillIPConfigClient()->GetProperties( On 2012/10/25 14:42:17, pneubeck wrote: > This means, that the state can even be inconsistent! > Some properties are already updated and ipconfig may not be. > Please mention that in the class comment. > That's always true; shill will send us single property updates. I'll add that to the header comments. > We should get Shill to send us ipconfig as part of the property dictionary. > Agreed. I think there may be a TODO for that already. > Actually, the current shill interface allows only to observe property changes > one-by-one. So, properties that are not encapsulated in a dictionary will > potentially be "inconsistent" (one value already new, the other still old). > > We should get Shill to support observing a dictionary of property changes, > similar to a transaction. When we implement NetworkConfigurationHandler, we will want to gather all state (i.e. wait for IPConfigs to complete) before returning the complete dictionary in a callback, so that the result will be consistent (at the time the request was made). For most of the UI code however, we want to know what the current known state is, and receive notifications when that changes. That said, I realized I was sending the wrong notification when we receive the IP address. http://codereview.chromium.org/11192024/diff/13002/chromeos/network/network_s... chromeos/network/network_state_handler.cc:503: const std::string& path, On 2012/10/25 14:42:17, pneubeck wrote: > this could be both ipconfig path or service path, please give it an unambiguous > name. Done. http://codereview.chromium.org/11192024/diff/13002/chromeos/network/network_s... chromeos/network/network_state_handler.cc:511: for (base::DictionaryValue::key_iterator iter = properties.begin_keys(); On 2012/10/25 14:42:17, pneubeck wrote: > DictionaryValue::Iterator Actually, no need to iterate, this can just be properties.GetStringWithoutPathExpansion. http://codereview.chromium.org/11192024/diff/13002/chromeos/network/network_s... chromeos/network/network_state_handler.cc:533: shill_manager_->RequestScan(flimflam::kTypeWifi, On 2012/10/25 14:42:17, pneubeck wrote: > According to manager-api.txt you can simply use "" instead of a particular > technology: > > void RequestScan(string type) > Request a scan for the specified technology. If > type is the string "" then a scan request is > made for each technology. Ah, that's new. This was copied from NetworkLibrary. Thanks. http://codereview.chromium.org/11192024/diff/13002/chromeos/network/network_s... File chromeos/network/network_state_handler.h (right): http://codereview.chromium.org/11192024/diff/13002/chromeos/network/network_s... chromeos/network/network_state_handler.h:48: : public ShillPropertyChangedObserver { On 2012/10/25 14:42:17, pneubeck wrote: > If you want to make the public interface cleaner, you could move the Observer > implementation into a private delegate (as private inheritance is not wanted by > the styleguide). This delegate could be near identical to > NetworkServiceObserver. > Then the OnPropertyChanged method would vanish from the public interface. Agreed, will do that in a followup. http://codereview.chromium.org/11192024/diff/13002/chromeos/network/network_s... chromeos/network/network_state_handler.h:50: typedef std::vector<ManagedState*> ManagedStateList; On 2012/10/25 14:42:17, pneubeck wrote: > On 2012/10/25 00:41:58, stevenjb (chromium) wrote: > > On 2012/10/24 14:41:42, pneubeck wrote: > > > These lists are never modified, but only new ones created by > > UpdateManagedList. > > > Why not use vector<>? Or even scoped_vector<> to ensure correct deletion and > > > document ownership? > > Evolutionary artifact. scoped_vector<> is high overhead, but vector<> is fine. > > Why is scoped_vector<> high overhead? According to the implementation there is > no overhead except the deletion of elements when necessary (.clear, .erase). > And the smart-point-guideline mentions > "For the common case of a vector that owns pointers, use ScopedVector. For other > cases, we often use raw pointers and clean up manually." > http://dev.chromium.org/developers/smart-pointer-guidelines Yes, you're right, I was thinking of vector<scoped_ptr>, not scoped_vector. The reason not to use ScopedVector is that in UpdateManagedList, we move ownership of the pointers from the vector, to a map, then back to the vector. This is so that we don't end up reconstructing each entry every time the list changes (which can get expensive, I saw that in the early days of NetworkLibrary). http://codereview.chromium.org/11192024/diff/13002/chromeos/network/network_s... File chromeos/network/network_state_handler_observer.h (right): http://codereview.chromium.org/11192024/diff/13002/chromeos/network/network_s... chromeos/network/network_state_handler_observer.h:25: // If |property| is non empty than that property has changed. Otherwise On 2012/10/25 14:42:17, pneubeck wrote: > Nit: than -> then > The "otherwise" doesn't sound sophisticated. Should we perhaps provide a vector > of the changed properties? This wasn't meant to be sophisticated. That would require a lot of bookkeeping and I can think of where that would be useful. I did update the comment. http://codereview.chromium.org/11192024/diff/13002/chromeos/network/network_s... chromeos/network/network_state_handler_observer.h:27: virtual void NetworkManagerChanged(const std::string& property); On 2012/10/25 14:42:17, pneubeck wrote: > I'm rather unhappy about this mixture of property names/keys (here |property|) > with the accessors in the State classes. > > That means, every user of the state classes has to know the mapping from the > method names (like security()) to the shill constants > (flimflam::kSecurityProperty) which he has to look up in the incomplete shill > documentation. > > Why that redundancy and mental-complication? Wouldn't it be easier to agree on a > well-defined state format (similar to ONC) and put it into a dictionary? > > e.g. a network state could look like: > > { type: "network", > path: "service/123", > network : { > type: "wifi", > name: "my wifi", > ssid: "my ssid", > signal_strength: 123 > } > } > > Then we also get the translation to json/JS for free! > And changes could simply be a subset of that dictionary. I agree that this is not ideal, and I've been considering just storing the states into a DictionaryValue. There are two reasons I have resisted that: a) Most of the UI code will never use the keys at all, and just needs easy access to the named properties. b) A couple of the properties are not direct mappings to shill dictionary values: ip_address, home_provider_id. I expect a few more may crop up for mobile. Most of the observer implementations will ignore the property argument, it's provided primarily for the code that does want to optimize. That said, with this implementation we will need to convert these values to JSON for the JS api. However, doing so will be pretty trivial. I think this code may be an evolutionary step which we can eventually replaced with just ONC data, but I feel like there are still too many questions that still need to be resolved there, so the intermediate step seems worthwhile, but I may be able to be convinced otherwise.
Still going through network_state_handler.cc. But I wanted to get these out soon before rietveld eats up my comments again. http://codereview.chromium.org/11192024/diff/23004/chromeos/network/device_st... File chromeos/network/device_state.cc (right): http://codereview.chromium.org/11192024/diff/23004/chromeos/network/device_st... chromeos/network/device_state.cc:17: "CUBIC", Where does this list come from? Should it be in service_constants? http://codereview.chromium.org/11192024/diff/23004/chromeos/network/device_st... chromeos/network/device_state.cc:33: if (key == flimflam::kNameProperty) { Do you expect to add more properties, or does this about cover it? http://codereview.chromium.org/11192024/diff/23004/chromeos/network/device_st... chromeos/network/device_state.cc:39: LOG(ERROR) << "Hardware Address: " << mac_address_; Why is this a LOG(ERROR)? http://codereview.chromium.org/11192024/diff/23004/chromeos/network/device_st... chromeos/network/device_state.cc:53: for (size_t i = 0; i < arraysize(kAlwaysInRoamingOperators); i++) { You could also consider using find() here. http://codereview.chromium.org/11192024/diff/23004/chromeos/network/device_st... chromeos/network/device_state.cc:70: return false; LOG a warning with the unknown key? http://codereview.chromium.org/11192024/diff/23004/chromeos/network/device_st... File chromeos/network/device_state.h (right): http://codereview.chromium.org/11192024/diff/23004/chromeos/network/device_st... chromeos/network/device_state.h:22: // Accessors NIT: . at the end http://codereview.chromium.org/11192024/diff/23004/chromeos/network/device_st... chromeos/network/device_state.h:30: // Device Properties NIT: . at the end. http://codereview.chromium.org/11192024/diff/23004/chromeos/network/device_st... chromeos/network/device_state.h:34: std::string home_provider_id_; Suggestion: separate out the properties that are used for WiFi vs. Cellular devices. http://codereview.chromium.org/11192024/diff/23004/chromeos/network/device_st... chromeos/network/device_state.h:35: bool always_in_roaming_; Can you add a comment about what this tracks. Without knowing much about cellular, I can't understand what this is useful for tracking. http://codereview.chromium.org/11192024/diff/23004/chromeos/network/managed_s... File chromeos/network/managed_state.h (right): http://codereview.chromium.org/11192024/diff/23004/chromeos/network/managed_s... chromeos/network/managed_state.h:16: } NIT: // namespace base http://codereview.chromium.org/11192024/diff/23004/chromeos/network/managed_s... chromeos/network/managed_state.h:21: // with a path. Can you add more context for what this path is in the comment? http://codereview.chromium.org/11192024/diff/23004/chromeos/network/managed_s... chromeos/network/managed_state.h:29: ManagedState(ManagedType type, const std::string& path); I am assuming this constructor is never meant to be called directly. This should be private then? http://codereview.chromium.org/11192024/diff/23004/chromeos/network/managed_s... chromeos/network/managed_state.h:38: virtual bool PropertyChanged(const std::string& key, What does the return value signal? http://codereview.chromium.org/11192024/diff/23004/chromeos/network/network_s... File chromeos/network/network_state.cc (right): http://codereview.chromium.org/11192024/diff/23004/chromeos/network/network_s... chromeos/network/network_state.cc:14: strength_(0) { If you explicitly initializing this, I'd explicitly initalize data_remaining_ to DATA_UKNOWN too lest we end up changing the order of the enum in the future. http://codereview.chromium.org/11192024/diff/23004/chromeos/network/network_s... File chromeos/network/network_state.h (right): http://codereview.chromium.org/11192024/diff/23004/chromeos/network/network_s... chromeos/network/network_state.h:14: // on to. Use path() (in ManagedState) for storing network identifiers and I don't understand the second path of the comment. path() is an accessor and I don't see a setter for path defines. Should this read "Use path() (in ..) for reading the network identifier path"? http://codereview.chromium.org/11192024/diff/23004/chromeos/network/network_s... chromeos/network/network_state.h:64: std::string type_; Just like my comment in device_state.h, this would be better split into properties specific to WiFi vs. Cellular. http://codereview.chromium.org/11192024/diff/23004/chromeos/network/network_s... chromeos/network/network_state.h:71: std::string activation_; What does this track? Should this be a bool? Should it be called activation_state_? http://codereview.chromium.org/11192024/diff/23004/chromeos/network/network_s... chromeos/network/network_state.h:73: int strength_; signal_strength_ would be a better name. http://codereview.chromium.org/11192024/diff/23004/chromeos/network/network_s... File chromeos/network/network_state_handler.h (right): http://codereview.chromium.org/11192024/diff/23004/chromeos/network/network_s... chromeos/network/network_state_handler.h:8: #include <list> Is this used anywhere? http://codereview.chromium.org/11192024/diff/23004/chromeos/network/network_s... chromeos/network/network_state_handler.h:43: // It will send invoke its own more specific observer methods when the send invoke -> invoke http://codereview.chromium.org/11192024/diff/23004/chromeos/network/network_s... chromeos/network/network_state_handler.h:60: // Add remove observers. NIT: Add/remove http://codereview.chromium.org/11192024/diff/23004/chromeos/network/network_s... chromeos/network/network_state_handler.h:73: const DeviceState* GetDeviceState(const std::string& path) const; Suggest calling this device_path to distinguish between service and device paths. http://codereview.chromium.org/11192024/diff/23004/chromeos/network/network_s... chromeos/network/network_state_handler.h:75: // Finds and returns a device by |type|. Returns NULL if not found. device -> device state http://codereview.chromium.org/11192024/diff/23004/chromeos/network/network_s... chromeos/network/network_state_handler.h:78: // Finds and returns a network by |path|. Returns NULL if not found. network state http://codereview.chromium.org/11192024/diff/23004/chromeos/network/network_s... chromeos/network/network_state_handler.h:90: // An empty type will return any connected non-ethernet network. Why do we need to special-case this to only return non-ethernet networks? Why not just return any connected network? http://codereview.chromium.org/11192024/diff/23004/chromeos/network/network_s... chromeos/network/network_state_handler.h:94: // An empty type will return any connecting non-ethernet network. See question about special-casing ethernet. http://codereview.chromium.org/11192024/diff/23004/chromeos/network/network_s... chromeos/network/network_state_handler.h:104: const NetworkStateList& GetNetworkList() const; Is this synchronous? Are the list of networks returned *after* Shill has completed the scan? I'd clarify here. http://codereview.chromium.org/11192024/diff/23004/chromeos/network/network_s... chromeos/network/network_state_handler.h:141: void UpdateAvailableTechnologies(const base::ListValue& technologies); Should this just be called SetAvailableTechnologies - Update implies either that it is asking Shill to do something, or that it merging in newly received information. http://codereview.chromium.org/11192024/diff/23004/chromeos/network/network_s... chromeos/network/network_state_handler.h:163: void NetworkServicePropertyChanged(const std::string& path, NIT: service_path http://codereview.chromium.org/11192024/diff/23004/chromeos/network/network_s... chromeos/network/network_state_handler.h:170: void GetIPConfigCallback(const std::string& service_path, I think it would be useful to group the ordering of these declearations such that all Shill callbacks are in a single place, and so on. I see 3 types of methods: 1) Things that update internal state directly. 2) Things that update internal state in response to a shill callback being finished. 3) Things that ask shill to do something (and presumably return the result in the form of a call back). http://codereview.chromium.org/11192024/diff/23004/chromeos/network/network_s... File chromeos/network/network_state_handler_observer.h (right): http://codereview.chromium.org/11192024/diff/23004/chromeos/network/network_s... chromeos/network/network_state_handler_observer.h:10: #include "base/basictypes.h" NIT: new line before this. http://codereview.chromium.org/11192024/diff/23004/chromeos/network/network_s... chromeos/network/network_state_handler_observer.h:26: // If |property| is non empty than that property has changed. Otherwise empty than -> empty, then http://codereview.chromium.org/11192024/diff/23004/chromeos/network/network_s... chromeos/network/network_state_handler_observer.h:30: virtual void NetworkListChanged(const NetworkStateList& networks); Suggest adding new lines to split declarations - easier to read that way. http://codereview.chromium.org/11192024/diff/23004/chromeos/network/shill_ser... File chromeos/network/shill_service_observer.h (right): http://codereview.chromium.org/11192024/diff/23004/chromeos/network/shill_ser... chromeos/network/shill_service_observer.h:16: // added on construction and removed on destruction. Runs the handler when I'd move the 2nd sentence as a comment into the class (OnPropertyChanged or where handler_ is defined.) http://codereview.chromium.org/11192024/diff/23004/chromeos/network/shill_ser... chromeos/network/shill_service_observer.h:31: const base::Value& value); OVERRIDE
My biggest high level feedback is to move the state list container management out of NetworkStateHandler, and letting a separate class deal with updating/searching/modifying the state list. http://codereview.chromium.org/11192024/diff/23004/chromeos/network/network_s... File chromeos/network/network_state_handler.cc (right): http://codereview.chromium.org/11192024/diff/23004/chromeos/network/network_s... chromeos/network/network_state_handler.cc:137: break; // Connected networks are listed first. I think we should not make this assumption. It ties too much into how shill deals with service lists. I'd rather have us keep a list of connected services separately, or something similar and update it in response to property changed events from shill if optimization is a concern here. http://codereview.chromium.org/11192024/diff/23004/chromeos/network/network_s... chromeos/network/network_state_handler.cc:176: std::string address = HardwareAddress(type); This should have an associated unit test. http://codereview.chromium.org/11192024/diff/23004/chromeos/network/network_s... chromeos/network/network_state_handler.cc:180: for (size_t i = 0; i < address.size(); ++i) { You may consider changing this to i+=2, and also push_back(address[i+1]). This way you don't need the div by 2 check below. It would make the code look clearer IMO. http://codereview.chromium.org/11192024/diff/23004/chromeos/network/network_s... chromeos/network/network_state_handler.cc:189: NetworkStateHandler::GetNetworkList() const { See my comment in .h about the potentially confusing semantics of this method. http://codereview.chromium.org/11192024/diff/23004/chromeos/network/network_s... chromeos/network/network_state_handler.cc:283: return reinterpret_cast<ManagedStateList*>(&network_list_); This makes me iffy too. I wouldn't be surprised if this failed in mysterious ways. There are 2 places ManagedStateList is used - UpdateManagedList and GetPropertiesCallback. It looks to me that managing the list of state like this (c.f. the code for UpdateManagedList below) adds a substantial amount of complexity of the class. Wouldn't it be better to have a separate ManagedStateList class - and have the StateHandler object have 2 members of that - one for managing the list of device states, and the other for network states. This way a substantial bit of implementation logic to deal with these containers can be moved out of this class. And it would also make it easy to write unit tests for the core state management logic. http://codereview.chromium.org/11192024/diff/23004/chromeos/network/network_s... chromeos/network/network_state_handler.cc:375: if (technology.empty()) Should this log a warning? http://codereview.chromium.org/11192024/diff/23004/chromeos/network/network_s... chromeos/network/network_state_handler.cc:388: if (technology.empty()) Should this log a warning? http://codereview.chromium.org/11192024/diff/23004/chromeos/network/network_s... chromeos/network/network_state_handler.cc:484: if (network == network_list_.front() && key == flimflam::kStateProperty) { Is there a guarantee that the manager update with a new list of services would have already been processed (i.e. network_list_ updates) such that this check succeeds when it should? This is the kind of race scenario that I am worried about. We shouldn't rely on order of networks in network_list to keep track of the active/connected service and instead manage them separately. http://codereview.chromium.org/11192024/diff/23004/chromeos/network/network_s... chromeos/network/network_state_handler.cc:486: ActiveNetworkStateChanged(network)); Where do you notify the observes when we transition from have an active network -> no active network? http://codereview.chromium.org/11192024/diff/23004/chromeos/network/network_s... File chromeos/network/network_state_handler.h (right): http://codereview.chromium.org/11192024/diff/23004/chromeos/network/network_s... chromeos/network/network_state_handler.h:99: std::string HardwareAddress(const std::string& type) const; I don't quite understand why we need this. Isn't this information that can already be derived indirectly from NetworkState returned by ConnectedNetworkByType? I'd much rather prefer to keep the API simple even if that means the consumers have to do a little bit of extra work. And if this is only meant to return for connected networks, that should be reflected in the name. http://codereview.chromium.org/11192024/diff/23004/chromeos/network/network_s... chromeos/network/network_state_handler.h:101: std::string HardwareAddressFormatted(const std::string& type) const; Since everything is a string - please change type->technology_type to make that clearer.
Only a few comments left. I'm still unhappy with the redundancy and missing self-containedness of the property keys and accessor methods. @Steve, as you mentioned in the comment, please think about to improve that rather earlier than later. I understand the evolution argument. However, what ever we commit next will stay there likely for more than a year. I will also think about an alternative when I have a quiet moment. http://codereview.chromium.org/11192024/diff/13002/chromeos/network/network_s... File chromeos/network/network_state_handler.h (right): http://codereview.chromium.org/11192024/diff/13002/chromeos/network/network_s... chromeos/network/network_state_handler.h:50: typedef std::vector<ManagedState*> ManagedStateList; On 2012/10/25 22:05:22, stevenjb (chromium) wrote: > On 2012/10/25 14:42:17, pneubeck wrote: > > On 2012/10/25 00:41:58, stevenjb (chromium) wrote: > > > On 2012/10/24 14:41:42, pneubeck wrote: > > > > These lists are never modified, but only new ones created by > > > UpdateManagedList. > > > > Why not use vector<>? Or even scoped_vector<> to ensure correct deletion > and > > > > document ownership? > > > Evolutionary artifact. scoped_vector<> is high overhead, but vector<> is > fine. > > > > Why is scoped_vector<> high overhead? According to the implementation there is > > no overhead except the deletion of elements when necessary (.clear, .erase). > > And the smart-point-guideline mentions > > "For the common case of a vector that owns pointers, use ScopedVector. For > other > > cases, we often use raw pointers and clean up manually." > > http://dev.chromium.org/developers/smart-pointer-guidelines > > Yes, you're right, I was thinking of vector<scoped_ptr>, not scoped_vector. > > The reason not to use ScopedVector is that in UpdateManagedList, we move > ownership of the pointers from the vector, to a map, then back to the vector. > This is so that we don't end up reconstructing each entry every time the list > changes (which can get expensive, I saw that in the early days of > NetworkLibrary). I added comments for the two changes that are necessary in the .cc http://codereview.chromium.org/11192024/diff/23004/chromeos/network/managed_s... File chromeos/network/managed_state.h (right): http://codereview.chromium.org/11192024/diff/23004/chromeos/network/managed_s... chromeos/network/managed_state.h:16: } On 2012/10/25 23:41:05, gauravsh wrote: > NIT: // namespace base I think there was agreement that this closing comment isn't needed for forward-declarations. http://codereview.chromium.org/11192024/diff/23004/chromeos/network/network_s... File chromeos/network/network_state_handler.cc (right): http://codereview.chromium.org/11192024/diff/23004/chromeos/network/network_s... chromeos/network/network_state_handler.cc:52: STLDeleteContainerPointers(network_list_.begin(), network_list_.end()); This line can be removed for scoped_vector http://codereview.chromium.org/11192024/diff/23004/chromeos/network/network_s... chromeos/network/network_state_handler.cc:58: } The second STLDeleteContainerPointers(device_list_, ...) is missing. That leads to a minor memory leak (during shutdown). But it wouldn't be necessary and hadn't happen if we used scoped_vector :-) http://codereview.chromium.org/11192024/diff/23004/chromeos/network/network_s... chromeos/network/network_state_handler.cc:137: break; // Connected networks are listed first. On 2012/10/26 01:15:01, gauravsh wrote: > I think we should not make this assumption. It ties too much into how shill > deals with service lists. I'd rather have us keep a list of connected services > separately, or something similar and update it in response to property changed > events from shill if optimization is a concern here. Optimization shouldn't be a concern if we don't have evidence for its need, i.e measured data. Robustness, readability and decoupled code should be more important. In this case, I think keeping the one list is the simplest solution. And I agree with Gaurav that we shouldn't rely on the lists order if not necessary: if (network->type() == type && network->IsConnectedState()) return network; http://codereview.chromium.org/11192024/diff/23004/chromeos/network/network_s... chromeos/network/network_state_handler.cc:302: managed_list->clear(); This will be weak_clear() for scoped_vector, so there is no reason to not use it.
OK, big set of changes here. Happy Friday! (Please go ahead and wait until Monday to review this :) ) Major chanegs: * Observing all entries in ServiceWatchList (which is currently all services) * Changed the lists to ManagedState* vectors and added safe casting with type checking where needed. * Split up NetworkStateHandler, moving the Shill implementation details to NetworkStateHandlerImpl * Removed property key names from observer functions to avoid confusion. This was convenient for testing but not really necessary, and I can't think of any uses cases where it would be more than a minor optimization. If a use case comes up we can address that then. (I recognize this doesn't fully address the concerns about storing properties as members here, but I'm still not convinced that ONC or a ValueDictionary is the right mechanism for this particular handler, at least not yet). I tried to address all of the feedback so far (it is very much appreciated), but it was challenging to keep up with it, so huge apologies if I missed anything. http://codereview.chromium.org/11192024/diff/23004/chromeos/network/device_st... File chromeos/network/device_state.cc (right): http://codereview.chromium.org/11192024/diff/23004/chromeos/network/device_st... chromeos/network/device_state.cc:17: "CUBIC", On 2012/10/25 23:41:05, gauravsh wrote: > Where does this list come from? Should it be in service_constants? Actually, this was recently replaced in NetworkLibrary with kProviderRequiresRoamingProperty; replaced here. http://codereview.chromium.org/11192024/diff/23004/chromeos/network/device_st... chromeos/network/device_state.cc:33: if (key == flimflam::kNameProperty) { On 2012/10/25 23:41:05, gauravsh wrote: > Do you expect to add more properties, or does this about cover it? Don't know. This covers (almost all of) the status area, which should include most everything, but we may require some other properties elsewhere. http://codereview.chromium.org/11192024/diff/23004/chromeos/network/device_st... chromeos/network/device_state.cc:39: LOG(ERROR) << "Hardware Address: " << mac_address_; On 2012/10/25 23:41:05, gauravsh wrote: > Why is this a LOG(ERROR)? Leftover debugging. Removed. http://codereview.chromium.org/11192024/diff/23004/chromeos/network/device_st... chromeos/network/device_state.cc:53: for (size_t i = 0; i < arraysize(kAlwaysInRoamingOperators); i++) { On 2012/10/25 23:41:05, gauravsh wrote: > You could also consider using find() here. Removed. http://codereview.chromium.org/11192024/diff/23004/chromeos/network/device_st... chromeos/network/device_state.cc:70: return false; On 2012/10/25 23:41:05, gauravsh wrote: > LOG a warning with the unknown key? No warning necessary, this is not intended to handle all keys. http://codereview.chromium.org/11192024/diff/23004/chromeos/network/device_st... File chromeos/network/device_state.h (right): http://codereview.chromium.org/11192024/diff/23004/chromeos/network/device_st... chromeos/network/device_state.h:22: // Accessors On 2012/10/25 23:41:05, gauravsh wrote: > NIT: . at the end Actually, technically only sentences should have a '.'. Fixed 'ManagedState overrides' above and did a consistency pass. http://codereview.chromium.org/11192024/diff/23004/chromeos/network/device_st... chromeos/network/device_state.h:30: // Device Properties On 2012/10/25 23:41:05, gauravsh wrote: > NIT: . at the end. ditto http://codereview.chromium.org/11192024/diff/23004/chromeos/network/device_st... chromeos/network/device_state.h:35: bool always_in_roaming_; On 2012/10/25 23:41:05, gauravsh wrote: > Can you add a comment about what this tracks. Without knowing much about > cellular, I can't understand what this is useful for tracking. Done. http://codereview.chromium.org/11192024/diff/23004/chromeos/network/managed_s... File chromeos/network/managed_state.h (right): http://codereview.chromium.org/11192024/diff/23004/chromeos/network/managed_s... chromeos/network/managed_state.h:21: // with a path. On 2012/10/25 23:41:05, gauravsh wrote: > Can you add more context for what this path is in the comment? Done. http://codereview.chromium.org/11192024/diff/23004/chromeos/network/managed_s... chromeos/network/managed_state.h:29: ManagedState(ManagedType type, const std::string& path); On 2012/10/25 23:41:05, gauravsh wrote: > I am assuming this constructor is never meant to be called directly. This should > be private then? Protected. Done. http://codereview.chromium.org/11192024/diff/23004/chromeos/network/managed_s... chromeos/network/managed_state.h:38: virtual bool PropertyChanged(const std::string& key, On 2012/10/25 23:41:05, gauravsh wrote: > What does the return value signal? Done. http://codereview.chromium.org/11192024/diff/23004/chromeos/network/network_s... File chromeos/network/network_state.cc (right): http://codereview.chromium.org/11192024/diff/23004/chromeos/network/network_s... chromeos/network/network_state.cc:14: strength_(0) { On 2012/10/25 23:41:05, gauravsh wrote: > If you explicitly initializing this, I'd explicitly initalize data_remaining_ to > DATA_UKNOWN too lest we end up changing the order of the enum in the future. I realized this hasn't been implemented yet (it's a bit complex, worthy of a separate CL, maybe separate handler); removing for now. http://codereview.chromium.org/11192024/diff/23004/chromeos/network/network_s... File chromeos/network/network_state.h (right): http://codereview.chromium.org/11192024/diff/23004/chromeos/network/network_s... chromeos/network/network_state.h:14: // on to. Use path() (in ManagedState) for storing network identifiers and On 2012/10/25 23:41:05, gauravsh wrote: > I don't understand the second path of the comment. path() is an accessor and I > don't see a setter for path defines. Should this read "Use path() (in ..) for > reading the network identifier path"? Clarified. http://codereview.chromium.org/11192024/diff/23004/chromeos/network/network_s... chromeos/network/network_state.h:64: std::string type_; On 2012/10/25 23:41:05, gauravsh wrote: > Just like my comment in device_state.h, this would be better split into > properties specific to WiFi vs. Cellular. Done. http://codereview.chromium.org/11192024/diff/23004/chromeos/network/network_s... chromeos/network/network_state.h:71: std::string activation_; On 2012/10/25 23:41:05, gauravsh wrote: > What does this track? Should this be a bool? Should it be called > activation_state_? Renamed http://codereview.chromium.org/11192024/diff/23004/chromeos/network/network_s... chromeos/network/network_state.h:73: int strength_; On 2012/10/25 23:41:05, gauravsh wrote: > signal_strength_ would be a better name. Done. http://codereview.chromium.org/11192024/diff/23004/chromeos/network/network_s... File chromeos/network/network_state_handler.cc (right): http://codereview.chromium.org/11192024/diff/23004/chromeos/network/network_s... chromeos/network/network_state_handler.cc:58: } On 2012/10/26 08:17:46, pneubeck wrote: > The second STLDeleteContainerPointers(device_list_, ...) is missing. > That leads to a minor memory leak (during shutdown). > But it wouldn't be necessary and hadn't happen if we used scoped_vector :-) Fixed. http://codereview.chromium.org/11192024/diff/23004/chromeos/network/network_s... chromeos/network/network_state_handler.cc:137: break; // Connected networks are listed first. On 2012/10/26 08:17:46, pneubeck wrote: > On 2012/10/26 01:15:01, gauravsh wrote: > > I think we should not make this assumption. It ties too much into how shill > > deals with service lists. I'd rather have us keep a list of connected services > > separately, or something similar and update it in response to property changed > > events from shill if optimization is a concern here. > > Optimization shouldn't be a concern if we don't have evidence for its need, i.e > measured data. > Robustness, readability and decoupled code should be more important. > > In this case, I think keeping the one list is the simplest solution. And I agree > with Gaurav that we shouldn't rely on the lists order if not necessary: > > if (network->type() == type && network->IsConnectedState()) > return network; This list *can* be quite long, O(100), and I prefer to avoid O(N) code in UI accessors. This entire code is tightly bound to Shill behavior, so I think the optimization is valid. I definitely want to avoid multiple lists; maintaining them in NetworkLibrary has been a nightmare. http://codereview.chromium.org/11192024/diff/23004/chromeos/network/network_s... chromeos/network/network_state_handler.cc:180: for (size_t i = 0; i < address.size(); ++i) { On 2012/10/26 01:15:01, gauravsh wrote: > You may consider changing this to i+=2, and also push_back(address[i+1]). This > way you don't need the div by 2 check below. It would make the code look clearer > IMO. I copied this from NetworkLibrary; I'm not 100% certain I follow your suggestion. We could optimize and test this, although it doesn't feel high priority to me. http://codereview.chromium.org/11192024/diff/23004/chromeos/network/network_s... chromeos/network/network_state_handler.cc:283: return reinterpret_cast<ManagedStateList*>(&network_list_); On 2012/10/26 01:15:01, gauravsh wrote: > This makes me iffy too. I wouldn't be surprised if this failed in mysterious > ways. There are 2 places ManagedStateList is used - UpdateManagedList and > GetPropertiesCallback. > > It looks to me that managing the list of state like this (c.f. the code for > UpdateManagedList below) adds a substantial amount of complexity of the class. > Wouldn't it be better to have a separate ManagedStateList class - and have the > StateHandler object have 2 members of that - one for managing the list of device > states, and the other for network states. > > This way a substantial bit of implementation logic to deal with these containers > can be moved out of this class. And it would also make it easy to write unit > tests for the core state management logic. This has been refactored. http://codereview.chromium.org/11192024/diff/23004/chromeos/network/network_s... chromeos/network/network_state_handler.cc:302: managed_list->clear(); On 2012/10/26 08:17:46, pneubeck wrote: > This will be weak_clear() for scoped_vector, so there is no reason to not use > it. Personally I think that as soon as you use weak_clear(), you shouldn't be using ScopedVector; I'd rather do the explicit deletes :) http://codereview.chromium.org/11192024/diff/23004/chromeos/network/network_s... chromeos/network/network_state_handler.cc:375: if (technology.empty()) On 2012/10/26 01:15:01, gauravsh wrote: > Should this log a warning? Should never happen and wouldn't hurt anything, I'll make it a DCHECK instead. http://codereview.chromium.org/11192024/diff/23004/chromeos/network/network_s... chromeos/network/network_state_handler.cc:388: if (technology.empty()) On 2012/10/26 01:15:01, gauravsh wrote: > Should this log a warning? Same as above. http://codereview.chromium.org/11192024/diff/23004/chromeos/network/network_s... chromeos/network/network_state_handler.cc:484: if (network == network_list_.front() && key == flimflam::kStateProperty) { On 2012/10/26 01:15:01, gauravsh wrote: > Is there a guarantee that the manager update with a new list of services would > have already been processed (i.e. network_list_ updates) such that this check > succeeds when it should? > > This is the kind of race scenario that I am worried about. We shouldn't rely on > order of networks in network_list to keep track of the active/connected service > and instead manage them separately. I think that separate management would add the kind complexity I am trying to avoid. If the order of network_list_ changes, then we will call ActiveNetworkChanged(), which any client would also need to observe. Shill processes requests in-order, so it should either complete the service requests before sending an update, or vice-versa. If that were not true, there is no way we could consistently track state (or it would be extremely complicated). http://codereview.chromium.org/11192024/diff/23004/chromeos/network/network_s... chromeos/network/network_state_handler.cc:486: ActiveNetworkStateChanged(network)); On 2012/10/26 01:15:01, gauravsh wrote: > Where do you notify the observes when we transition from have an active network > -> no active network? In UpdateObservedNetworkServices() (now NotifyNetworkServiceObservers()) http://codereview.chromium.org/11192024/diff/23004/chromeos/network/network_s... File chromeos/network/network_state_handler.h (right): http://codereview.chromium.org/11192024/diff/23004/chromeos/network/network_s... chromeos/network/network_state_handler.h:8: #include <list> On 2012/10/25 23:41:05, gauravsh wrote: > Is this used anywhere? Fixed http://codereview.chromium.org/11192024/diff/23004/chromeos/network/network_s... chromeos/network/network_state_handler.h:43: // It will send invoke its own more specific observer methods when the On 2012/10/25 23:41:05, gauravsh wrote: > send invoke -> invoke Done. http://codereview.chromium.org/11192024/diff/23004/chromeos/network/network_s... chromeos/network/network_state_handler.h:60: // Add remove observers. On 2012/10/25 23:41:05, gauravsh wrote: > NIT: Add/remove Done. http://codereview.chromium.org/11192024/diff/23004/chromeos/network/network_s... chromeos/network/network_state_handler.h:73: const DeviceState* GetDeviceState(const std::string& path) const; On 2012/10/25 23:41:05, gauravsh wrote: > Suggest calling this device_path to distinguish between service and device > paths. Done. http://codereview.chromium.org/11192024/diff/23004/chromeos/network/network_s... chromeos/network/network_state_handler.h:75: // Finds and returns a device by |type|. Returns NULL if not found. On 2012/10/25 23:41:05, gauravsh wrote: > device -> device state Done. http://codereview.chromium.org/11192024/diff/23004/chromeos/network/network_s... chromeos/network/network_state_handler.h:78: // Finds and returns a network by |path|. Returns NULL if not found. On 2012/10/25 23:41:05, gauravsh wrote: > network state Done. http://codereview.chromium.org/11192024/diff/23004/chromeos/network/network_s... chromeos/network/network_state_handler.h:90: // An empty type will return any connected non-ethernet network. On 2012/10/25 23:41:05, gauravsh wrote: > Why do we need to special-case this to only return non-ethernet networks? Why > not just return any connected network? Oops, not true, comment copied from connecting. http://codereview.chromium.org/11192024/diff/23004/chromeos/network/network_s... chromeos/network/network_state_handler.h:94: // An empty type will return any connecting non-ethernet network. On 2012/10/25 23:41:05, gauravsh wrote: > See question about special-casing ethernet. Ethernet is rarely if ever in a "connecting" state; this avoids occasionally briefly flashing a connecting ethernet icon. http://codereview.chromium.org/11192024/diff/23004/chromeos/network/network_s... chromeos/network/network_state_handler.h:99: std::string HardwareAddress(const std::string& type) const; On 2012/10/26 01:15:01, gauravsh wrote: > I don't quite understand why we need this. Isn't this information that can > already be derived indirectly from NetworkState returned by > ConnectedNetworkByType? I'd much rather prefer to keep the API simple even if > that means the consumers have to do a little bit of extra work. > > And if this is only meant to return for connected networks, that should be > reflected in the name. I improved the names, but feel that: a) We will be calling this in several places in the UI b) We only ever care about connected devices except perhaps in an advanced device inspection view (which I don't think we currently have). It's in the comment, adding that to the name feels awkward. c) The hardware address is stored in the device. Having the UI do the network + device lookup seems like it puts too much burden in the UI code. http://codereview.chromium.org/11192024/diff/23004/chromeos/network/network_s... chromeos/network/network_state_handler.h:101: std::string HardwareAddressFormatted(const std::string& type) const; On 2012/10/26 01:15:01, gauravsh wrote: > Since everything is a string - please change type->technology_type to make that > clearer. I think that would be more confusing. We use 'type' consistently to differentiate between technologies. Converting that to technology_type everywhere seems unnecessarily verbose. http://codereview.chromium.org/11192024/diff/23004/chromeos/network/network_s... chromeos/network/network_state_handler.h:104: const NetworkStateList& GetNetworkList() const; On 2012/10/25 23:41:05, gauravsh wrote: > Is this synchronous? Are the list of networks returned *after* Shill has > completed the scan? I'd clarify here. Comments here have been expanded. http://codereview.chromium.org/11192024/diff/23004/chromeos/network/network_s... chromeos/network/network_state_handler.h:141: void UpdateAvailableTechnologies(const base::ListValue& technologies); On 2012/10/25 23:41:05, gauravsh wrote: > Should this just be called SetAvailableTechnologies - Update implies either that > it is asking Shill to do something, or that it merging in newly received > information. It's named update because it's more involved than a typical setter, but not much more. Renamed. http://codereview.chromium.org/11192024/diff/23004/chromeos/network/network_s... chromeos/network/network_state_handler.h:163: void NetworkServicePropertyChanged(const std::string& path, On 2012/10/25 23:41:05, gauravsh wrote: > NIT: service_path Done. http://codereview.chromium.org/11192024/diff/23004/chromeos/network/network_s... chromeos/network/network_state_handler.h:170: void GetIPConfigCallback(const std::string& service_path, On 2012/10/25 23:41:05, gauravsh wrote: > I think it would be useful to group the ordering of these declearations such > that all Shill callbacks are in a single place, and so on. I see 3 types of > methods: > 1) Things that update internal state directly. > 2) Things that update internal state in response to a shill callback being > finished. > 3) Things that ask shill to do something (and presumably return the result in > the form of a call back). This has been refactored http://codereview.chromium.org/11192024/diff/23004/chromeos/network/network_s... File chromeos/network/network_state_handler_observer.h (right): http://codereview.chromium.org/11192024/diff/23004/chromeos/network/network_s... chromeos/network/network_state_handler_observer.h:10: #include "base/basictypes.h" On 2012/10/25 23:41:05, gauravsh wrote: > NIT: new line before this. Done. http://codereview.chromium.org/11192024/diff/23004/chromeos/network/network_s... chromeos/network/network_state_handler_observer.h:26: // If |property| is non empty than that property has changed. Otherwise On 2012/10/25 23:41:05, gauravsh wrote: > empty than -> empty, then Done. http://codereview.chromium.org/11192024/diff/23004/chromeos/network/network_s... chromeos/network/network_state_handler_observer.h:30: virtual void NetworkListChanged(const NetworkStateList& networks); On 2012/10/25 23:41:05, gauravsh wrote: > Suggest adding new lines to split declarations - easier to read that way. Done. http://codereview.chromium.org/11192024/diff/23004/chromeos/network/shill_ser... File chromeos/network/shill_service_observer.h (right): http://codereview.chromium.org/11192024/diff/23004/chromeos/network/shill_ser... chromeos/network/shill_service_observer.h:16: // added on construction and removed on destruction. Runs the handler when On 2012/10/25 23:41:05, gauravsh wrote: > I'd move the 2nd sentence as a comment into the class (OnPropertyChanged or > where handler_ is defined.) Since this has no public facing API, it seemed better to put all the comments at top. http://codereview.chromium.org/11192024/diff/23004/chromeos/network/shill_ser... chromeos/network/shill_service_observer.h:31: const base::Value& value); On 2012/10/25 23:41:05, gauravsh wrote: > OVERRIDE Done.
http://codereview.chromium.org/11192024/diff/23004/chromeos/network/network_s... File chromeos/network/network_state_handler.cc (right): http://codereview.chromium.org/11192024/diff/23004/chromeos/network/network_s... chromeos/network/network_state_handler.cc:137: break; // Connected networks are listed first. On 2012/10/26 21:36:39, stevenjb (chromium) wrote: > On 2012/10/26 08:17:46, pneubeck wrote: > > On 2012/10/26 01:15:01, gauravsh wrote: > > > I think we should not make this assumption. It ties too much into how shill > > > deals with service lists. I'd rather have us keep a list of connected > services > > > separately, or something similar and update it in response to property > changed > > > events from shill if optimization is a concern here. > > > > Optimization shouldn't be a concern if we don't have evidence for its need, > i.e > > measured data. > > Robustness, readability and decoupled code should be more important. > > > > In this case, I think keeping the one list is the simplest solution. And I > agree > > with Gaurav that we shouldn't rely on the lists order if not necessary: > > > > if (network->type() == type && network->IsConnectedState()) > > return network; > > This list *can* be quite long, O(100), and I prefer to avoid O(N) code in UI > accessors. > This entire code is tightly bound to Shill behavior, so I think the optimization > is valid. > I definitely want to avoid multiple lists; maintaining them in NetworkLibrary > has been a nightmare. > To be fair, the problem with NetworkLibrary is that that there is no single place/entity responsible for doing this management. It's fairly ad-hoc. One of the goals of this re-factor is to allow us to easily change Shill's API contract going forward. In that regard, mirroring Shill's behavior with regards to how service state is handled here is problematic. http://codereview.chromium.org/11192024/diff/23004/chromeos/network/network_s... chromeos/network/network_state_handler.cc:180: for (size_t i = 0; i < address.size(); ++i) { On 2012/10/26 21:36:39, stevenjb (chromium) wrote: > On 2012/10/26 01:15:01, gauravsh wrote: > > You may consider changing this to i+=2, and also push_back(address[i+1]). This > > way you don't need the div by 2 check below. It would make the code look > clearer > > IMO. > I copied this from NetworkLibrary; I'm not 100% certain I follow your > suggestion. We could optimize and test this, although it doesn't feel high > priority to me. The optimization was just a suggestion to improve readability, not performance. However, we are required to have unit tests for all non-trivial methods. http://codereview.chromium.org/11192024/diff/23004/chromeos/network/network_s... chromeos/network/network_state_handler.cc:484: if (network == network_list_.front() && key == flimflam::kStateProperty) { On 2012/10/26 21:36:39, stevenjb (chromium) wrote: > On 2012/10/26 01:15:01, gauravsh wrote: > > Is there a guarantee that the manager update with a new list of services would > > have already been processed (i.e. network_list_ updates) such that this check > > succeeds when it should? > > > > This is the kind of race scenario that I am worried about. We shouldn't rely > on > > order of networks in network_list to keep track of the active/connected > service > > and instead manage them separately. > > I think that separate management would add the kind complexity I am trying to > avoid. > > If the order of network_list_ changes, then we will call ActiveNetworkChanged(), > which any client would also need to observe. > > Shill processes requests in-order, so it should either complete the service > requests before sending an update, or vice-versa. If that were not true, there > is no way we could consistently track state (or it would be extremely > complicated). > Since dbus signals are handled asynchronously, this still depends on the race between which signal is handled first. I don't think managing the active network explicitly will add to the complexity. The bigger problem is that false positives on active network changes is disruptive to NCN - as it impacts existing connections. We should minimize that. http://codereview.chromium.org/11192024/diff/23004/chromeos/network/network_s... File chromeos/network/network_state_handler.h (right): http://codereview.chromium.org/11192024/diff/23004/chromeos/network/network_s... chromeos/network/network_state_handler.h:94: // An empty type will return any connecting non-ethernet network. On 2012/10/26 21:36:39, stevenjb (chromium) wrote: > On 2012/10/25 23:41:05, gauravsh wrote: > > See question about special-casing ethernet. > Ethernet is rarely if ever in a "connecting" state; this avoids occasionally > briefly flashing a connecting ethernet icon. But why not handle this damping at the UI layer insted of hardcoding here? http://codereview.chromium.org/11192024/diff/34001/chromeos/network/device_st... File chromeos/network/device_state.h (right): http://codereview.chromium.org/11192024/diff/34001/chromeos/network/device_st... chromeos/network/device_state.h:32: bool always_in_roaming_; Still no comment on what this tracks. http://codereview.chromium.org/11192024/diff/34001/chromeos/network/network_s... File chromeos/network/network_state_handler_impl.h (right): http://codereview.chromium.org/11192024/diff/34001/chromeos/network/network_s... chromeos/network/network_state_handler_impl.h:35: class NetworkStateHandlerImpl : public ShillPropertyChangedObserver { Please think of a better name for this class. It's a class used by the NetworkStateHandler implementation, but it does not implement it.
http://codereview.chromium.org/11192024/diff/34001/chromeos/network/device_st... File chromeos/network/device_state.h (right): http://codereview.chromium.org/11192024/diff/34001/chromeos/network/device_st... chromeos/network/device_state.h:32: bool always_in_roaming_; On 2012/10/27 00:26:33, gauravsh wrote: > Still no comment on what this tracks. Renamed to match shill property name. http://codereview.chromium.org/11192024/diff/34001/chromeos/network/network_s... File chromeos/network/network_state_handler_impl.h (right): http://codereview.chromium.org/11192024/diff/34001/chromeos/network/network_s... chromeos/network/network_state_handler_impl.h:35: class NetworkStateHandlerImpl : public ShillPropertyChangedObserver { On 2012/10/27 00:26:33, gauravsh wrote: > Please think of a better name for this class. It's a class used by the > NetworkStateHandler implementation, but it does not implement it. Fair enough. How about ShillPropertyHandler?
http://codereview.chromium.org/11192024/diff/34001/chromeos/network/network_s... File chromeos/network/network_state_handler_impl.h (right): http://codereview.chromium.org/11192024/diff/34001/chromeos/network/network_s... chromeos/network/network_state_handler_impl.h:35: class NetworkStateHandlerImpl : public ShillPropertyChangedObserver { On 2012/10/29 18:34:07, stevenjb (chromium) wrote: > On 2012/10/27 00:26:33, gauravsh wrote: > > Please think of a better name for this class. It's a class used by the > > NetworkStateHandler implementation, but it does not implement it. > > Fair enough. How about ShillPropertyHandler? SGTM
Not much left from my side. http://codereview.chromium.org/11192024/diff/34001/chromeos/network/network_s... File chromeos/network/network_state_handler.cc (right): http://codereview.chromium.org/11192024/diff/34001/chromeos/network/network_s... chromeos/network/network_state_handler.cc:314: void NetworkStateHandler::SetServiceIPAddress(const std::string& service_path, SetNetworkServiceIPAddress? http://codereview.chromium.org/11192024/diff/34001/chromeos/network/network_s... File chromeos/network/network_state_handler_impl.cc (right): http://codereview.chromium.org/11192024/diff/34001/chromeos/network/network_s... chromeos/network/network_state_handler_impl.cc:87: ++pending_updates_[type]; Shill/DBus guarantees x answers to x requests for the same path? http://codereview.chromium.org/11192024/diff/34001/chromeos/network/network_s... chromeos/network/network_state_handler_impl.cc:221: base_->ParseProperties(type, path, properties); "Parse" is not informative. Perhaps rename it to SetServiceProperties. http://codereview.chromium.org/11192024/diff/34001/chromeos/network/network_s... chromeos/network/network_state_handler_impl.cc:224: pending_updates_[type] == 0) { Maybe, moving pending_updates_ back to NetworkStateHandler makes the coupling of both classes a bit clearer? One can understand pending_updates_ to be controlling the NetworkStateHandler's observer notifications.
http://codereview.chromium.org/11192024/diff/34001/chromeos/network/network_s... File chromeos/network/network_state_handler.cc (right): http://codereview.chromium.org/11192024/diff/34001/chromeos/network/network_s... chromeos/network/network_state_handler.cc:314: void NetworkStateHandler::SetServiceIPAddress(const std::string& service_path, On 2012/10/29 20:27:49, pneubeck wrote: > SetNetworkServiceIPAddress? Done. http://codereview.chromium.org/11192024/diff/34001/chromeos/network/network_s... File chromeos/network/network_state_handler_impl.cc (right): http://codereview.chromium.org/11192024/diff/34001/chromeos/network/network_s... chromeos/network/network_state_handler_impl.cc:87: ++pending_updates_[type]; On 2012/10/29 20:27:49, pneubeck wrote: > Shill/DBus guarantees x answers to x requests for the same path? Yes. Even if there is a timeout or an error the callback should be triggered. This is why I track pending updates here instead of NetworkStateHandler; it is really a detail of the communication with Shill, not something the handler should have to track. http://codereview.chromium.org/11192024/diff/34001/chromeos/network/network_s... chromeos/network/network_state_handler_impl.cc:221: base_->ParseProperties(type, path, properties); On 2012/10/29 20:27:49, pneubeck wrote: > "Parse" is not informative. Perhaps rename it to SetServiceProperties. Well, it is parsing the dictionary, not blindly setting properties, perhaps ParseManagedStateProperties? http://codereview.chromium.org/11192024/diff/34001/chromeos/network/network_s... chromeos/network/network_state_handler_impl.cc:224: pending_updates_[type] == 0) { On 2012/10/29 20:27:49, pneubeck wrote: > Maybe, moving pending_updates_ back to NetworkStateHandler makes the coupling of > both classes a bit clearer? One can understand pending_updates_ to be > controlling the NetworkStateHandler's observer notifications. See previous comment. Renamed the function and changed the logic however to 'ManagedStateListUpdated(type)' since notifying observers is a detail of NetworkStateManager.
Note: I didn't like some of the changes in patchset #7 so I removed it. Apologies if anyone happened to be reviewing it. I plan to have another update later today that has some changes to when / how this notifies observers.
On 2012/10/30 22:13:09, stevenjb (chromium) wrote: > Note: I didn't like some of the changes in patchset #7 so I removed it. > Apologies if anyone happened to be reviewing it. > > I plan to have another update later today that has some changes to when / how > this notifies observers. Hey: Rietveld doesn't notify when you upload a new patch set. Can you send a message when your new patchset is up and ready for review?
OK, some major changes since patchset #5: * NetworkStateHandlerImpl is now ShillPropertyHandler with a proper Delegate interface so it has no NetworkStateHandler dependencies. * ShillPropertyHandler is pretty well covered by unit tests (functional tests really). This CL currently includes changes to src/chromeos/dbus from http://codereview.chromium.org/11365022/ which provides a test interface to the shill client stub implementations. * Function names in NetworkStateHandler are better named and better organized with the Delegate implemenation. * Added NetworkStateHandler tests for changes to the active network. The code is ready for another round of review.
http://codereview.chromium.org/11192024/diff/63004/chromeos/network/shill_pro... File chromeos/network/shill_property_handler.h (right): http://codereview.chromium.org/11192024/diff/63004/chromeos/network/shill_pro... chromeos/network/shill_property_handler.h:37: class CHROMEOS_EXPORT ShillPropertyHandler It looks odd to me that classes of the internal namespace are exported. Is that necessary because of the inheritance in NetworkStateHandler? http://codereview.chromium.org/11192024/diff/63004/chromeos/network/shill_pro... chromeos/network/shill_property_handler.h:42: class CHROMEOS_EXPORT Delegate { Wasn't ShillPropertyHandler the Delegate of NetworkStateHandler? That is also indicated by the ownership. Perhaps "Delegator" would fit here better?
https://codereview.chromium.org/11192024/diff/63004/chromeos/network/shill_pr... File chromeos/network/shill_property_handler.h (right): https://codereview.chromium.org/11192024/diff/63004/chromeos/network/shill_pr... chromeos/network/shill_property_handler.h:37: class CHROMEOS_EXPORT ShillPropertyHandler On 2012/11/02 09:20:53, pneubeck wrote: > It looks odd to me that classes of the internal namespace are exported. Is that > necessary because of the inheritance in NetworkStateHandler? Tests require it. 'internal' really just indicate "if you're calling this outside the module, you're doing it wrong." https://codereview.chromium.org/11192024/diff/63004/chromeos/network/shill_pr... chromeos/network/shill_property_handler.h:42: class CHROMEOS_EXPORT Delegate { On 2012/11/02 09:20:53, pneubeck wrote: > Wasn't ShillPropertyHandler the Delegate of NetworkStateHandler? That is also > indicated by the ownership. > > Perhaps "Delegator" would fit here better? ShillPropertyHandler is wholly owned and exposed to NetworkStateHandler; ShillPropertyHandler (now) only knows it is talking to a Delegate (i.e it doesn't know any specifics of NetworkStateHandler). This is the same pattern we use for Delegate elsewhere, e.g. a Widget has a WidgetDelegate* that typically points to the class that owns it. Does that make sense? It's confusing because of the way this code evolved, an artifact of soliciting feedback early, but the feedback has been very valuable, so worth it I think.
hashimoto@ - can you review the src/chromeos/dbus changes here (some additions to the previous CL that were added to support the unit tests for the new code). Feel free to review the other changes also, but do not feel obligated. Thanks!
https://codereview.chromium.org/11192024/diff/54061/chromeos/chromeos.gyp File chromeos/chromeos.gyp (right): https://codereview.chromium.org/11192024/diff/54061/chromeos/chromeos.gyp#new... chromeos/chromeos.gyp:121: 'network/device_state.h', '.cc' comes before '.h' https://codereview.chromium.org/11192024/diff/54061/chromeos/dbus/shill_devic... File chromeos/dbus/shill_device_client.cc (right): https://codereview.chromium.org/11192024/diff/54061/chromeos/dbus/shill_devic... chromeos/dbus/shill_device_client.cc:236: observer_list_.AddObserver(observer); You might need to change the type of observer_list_ to std::map<dbus::ObjectPath, ObserverList<ShillPropertyChangedObserver> >. With the current code, an observer gets notified about all devices regardless of which device it is interested in. This is my fault, I should have caught this in the previous code review, sorry. https://codereview.chromium.org/11192024/diff/54061/chromeos/dbus/shill_devic... chromeos/dbus/shill_device_client.cc:299: PostVoidCallback(callback, DBUS_METHOD_CALL_SUCCESS); Is there no need to notify observers about the cleared property? What does the real Shill service do on ClearProperty? https://codereview.chromium.org/11192024/diff/54061/chromeos/dbus/shill_manag... File chromeos/dbus/shill_manager_client.cc (right): https://codereview.chromium.org/11192024/diff/54061/chromeos/dbus/shill_manag... chromeos/dbus/shill_manager_client.cc:175: namespace { We are already in the unnamed namespace, no need to go deeper here. https://codereview.chromium.org/11192024/diff/54061/chromeos/dbus/shill_manag... chromeos/dbus/shill_manager_client.cc:177: struct ValueEquals { Could you add some comment about how this struct is used? https://codereview.chromium.org/11192024/diff/54061/chromeos/dbus/shill_manag... chromeos/dbus/shill_manager_client.cc:388: stub_properties_.Clear(); No need to notify observers here? If so, please add comments to describe why. https://codereview.chromium.org/11192024/diff/54061/chromeos/dbus/shill_manag... chromeos/dbus/shill_manager_client.cc:446: bool initialized_; Please add comments to describe that we need this variable even if no one can add itself as an observer until the ShillManagerClient finishes its construction because we notify observers asynchronously. https://codereview.chromium.org/11192024/diff/54061/chromeos/dbus/shill_manag... File chromeos/dbus/shill_manager_client.h (right): https://codereview.chromium.org/11192024/diff/54061/chromeos/dbus/shill_manag... chromeos/dbus/shill_manager_client.h:43: virtual void InsertService(const std::string& service_path, Please describe the difference between AddService and InsertService with comments. Perhaps, this might be better renamed to AddServiceWithIndex or something? https://codereview.chromium.org/11192024/diff/54061/chromeos/dbus/shill_manag... chromeos/dbus/shill_manager_client.h:44: size_t index) = 0; We might need to have add_to_watch_list here too? https://codereview.chromium.org/11192024/diff/54061/chromeos/dbus/shill_servi... File chromeos/dbus/shill_service_client.cc (right): https://codereview.chromium.org/11192024/diff/54061/chromeos/dbus/shill_servi... chromeos/dbus/shill_service_client.cc:201: observer_list_.AddObserver(observer); You might want to prepare observer lists for each service_path. Please see my comment in shill_device_client.cc
LGTM with one nit. http://codereview.chromium.org/11192024/diff/54061/chromeos/dbus/shill_manage... File chromeos/dbus/shill_manager_client.cc (right): http://codereview.chromium.org/11192024/diff/54061/chromeos/dbus/shill_manage... chromeos/dbus/shill_manager_client.cc:414: int delay_seconds) { Is seconds enough granularity? Seems like milliseconds would be more flexible, even if sub millisecond values are not used at the moment.
http://codereview.chromium.org/11192024/diff/54061/chromeos/chromeos.gyp File chromeos/chromeos.gyp (right): http://codereview.chromium.org/11192024/diff/54061/chromeos/chromeos.gyp#newc... chromeos/chromeos.gyp:121: 'network/device_state.h', On 2012/11/05 04:03:03, hashimoto wrote: > '.cc' comes before '.h' Done. http://codereview.chromium.org/11192024/diff/54061/chromeos/dbus/shill_device... File chromeos/dbus/shill_device_client.cc (right): http://codereview.chromium.org/11192024/diff/54061/chromeos/dbus/shill_device... chromeos/dbus/shill_device_client.cc:236: observer_list_.AddObserver(observer); Good catch, fixed. http://codereview.chromium.org/11192024/diff/54061/chromeos/dbus/shill_device... chromeos/dbus/shill_device_client.cc:299: PostVoidCallback(callback, DBUS_METHOD_CALL_SUCCESS); On 2012/11/05 04:03:03, hashimoto wrote: > Is there no need to notify observers about the cleared property? What does the > real Shill service do on ClearProperty? The dbus impl calls ShillClientHelper::CallVoidMethod() which does trigger the callback on completion, so we should be consistent and do that here too. http://codereview.chromium.org/11192024/diff/54061/chromeos/dbus/shill_manage... File chromeos/dbus/shill_manager_client.cc (right): http://codereview.chromium.org/11192024/diff/54061/chromeos/dbus/shill_manage... chromeos/dbus/shill_manager_client.cc:175: namespace { On 2012/11/05 04:03:03, hashimoto wrote: > We are already in the unnamed namespace, no need to go deeper here. Oops. Done. http://codereview.chromium.org/11192024/diff/54061/chromeos/dbus/shill_manage... chromeos/dbus/shill_manager_client.cc:177: struct ValueEquals { On 2012/11/05 04:03:03, hashimoto wrote: > Could you add some comment about how this struct is used? Done. http://codereview.chromium.org/11192024/diff/54061/chromeos/dbus/shill_manage... chromeos/dbus/shill_manager_client.cc:388: stub_properties_.Clear(); On 2012/11/05 04:03:03, hashimoto wrote: > No need to notify observers here? If so, please add comments to describe why. Comment added to header. http://codereview.chromium.org/11192024/diff/54061/chromeos/dbus/shill_manage... chromeos/dbus/shill_manager_client.cc:414: int delay_seconds) { On 2012/11/05 22:07:23, Greg Spencer (Chromium) wrote: > Is seconds enough granularity? Seems like milliseconds would be more flexible, > even if sub millisecond values are not used at the moment. Done. http://codereview.chromium.org/11192024/diff/54061/chromeos/dbus/shill_manage... chromeos/dbus/shill_manager_client.cc:446: bool initialized_; On 2012/11/05 04:03:03, hashimoto wrote: > Please add comments to describe that we need this variable even if no one can > add itself as an observer until the ShillManagerClient finishes its construction > because we notify observers asynchronously. Good point. Removed this. http://codereview.chromium.org/11192024/diff/54061/chromeos/dbus/shill_manage... File chromeos/dbus/shill_manager_client.h (right): http://codereview.chromium.org/11192024/diff/54061/chromeos/dbus/shill_manage... chromeos/dbus/shill_manager_client.h:43: virtual void InsertService(const std::string& service_path, On 2012/11/05 04:03:03, hashimoto wrote: > Please describe the difference between AddService and InsertService with > comments. > Perhaps, this might be better renamed to AddServiceWithIndex or something? Renamed AddServiceAtIndex. http://codereview.chromium.org/11192024/diff/54061/chromeos/dbus/shill_manage... chromeos/dbus/shill_manager_client.h:44: size_t index) = 0; On 2012/11/05 04:03:03, hashimoto wrote: > We might need to have add_to_watch_list here too? Done. http://codereview.chromium.org/11192024/diff/54061/chromeos/dbus/shill_servic... File chromeos/dbus/shill_service_client.cc (right): http://codereview.chromium.org/11192024/diff/54061/chromeos/dbus/shill_servic... chromeos/dbus/shill_service_client.cc:201: observer_list_.AddObserver(observer); On 2012/11/05 04:03:03, hashimoto wrote: > You might want to prepare observer lists for each service_path. > Please see my comment in shill_device_client.cc Done.
lgtm with nits https://codereview.chromium.org/11192024/diff/63006/chromeos/chromeos.gyp File chromeos/chromeos.gyp (right): https://codereview.chromium.org/11192024/diff/63006/chromeos/chromeos.gyp#new... chromeos/chromeos.gyp:127: 'network/managed_state.cc', managed_state.h missing? https://codereview.chromium.org/11192024/diff/63006/chromeos/dbus/shill_devic... File chromeos/dbus/shill_device_client.cc (right): https://codereview.chromium.org/11192024/diff/63006/chromeos/dbus/shill_devic... chromeos/dbus/shill_device_client.cc:458: PropertyObserverList& GetObserverList(const dbus::ObjectPath& device_path) { nit: Please use pointer instead of reference if there is no strong reason. I think our style guide prefers pointers to references when they are non-const. https://codereview.chromium.org/11192024/diff/63006/chromeos/dbus/shill_manag... File chromeos/dbus/shill_manager_client.cc (right): https://codereview.chromium.org/11192024/diff/63006/chromeos/dbus/shill_manag... chromeos/dbus/shill_manager_client.cc:418: if (observer_list_.size() == 0) nit: Please leave a comment to describe why we need this check. https://codereview.chromium.org/11192024/diff/63006/chromeos/dbus/shill_servi... File chromeos/dbus/shill_service_client.cc (right): https://codereview.chromium.org/11192024/diff/63006/chromeos/dbus/shill_servi... chromeos/dbus/shill_service_client.cc:399: PropertyObserverList& GetObserverList(const dbus::ObjectPath& device_path) { nit: Please use pointer instead of reference if there is no strong reason.
The overall design looks fine to me. I will take on the AI to add a handler for the default service property that the manager emit and cross-check with it the managed active network path.(http://code.google.com/p/chromium/issues/detail?id=159540). http://codereview.chromium.org/11192024/diff/54061/chromeos/network/device_st... File chromeos/network/device_state.cc (right): http://codereview.chromium.org/11192024/diff/54061/chromeos/network/device_st... chromeos/network/device_state.cc:55: } Don't you need to return true here? http://codereview.chromium.org/11192024/diff/54061/chromeos/network/managed_s... File chromeos/network/managed_state.h (right): http://codereview.chromium.org/11192024/diff/54061/chromeos/network/managed_s... chromeos/network/managed_state.h:62: // Helper methods that log warnings if parsing failed. If parsing fails, they also return false, correct? That seems more worth mentioning then the part about logging. Would letting the caller do the logging in case of failure make more sense? will these messages be too spammy, and should perhaps be logged at a higher verbosity level? http://codereview.chromium.org/11192024/diff/54061/chromeos/network/managed_s... chromeos/network/managed_state.h:75: std::string path_; Either add comments that describe what these are meant to represent, or make the names more descriptive. path, name and type are very generic names. http://codereview.chromium.org/11192024/diff/54061/chromeos/network/network_s... File chromeos/network/network_state.h (right): http://codereview.chromium.org/11192024/diff/54061/chromeos/network/network_s... chromeos/network/network_state.h:40: // Helpers (used e.g. when a state is cached) Are there any places where one might be interested in querying if the state is failure? Should there be a StateIsFailure helper? http://codereview.chromium.org/11192024/diff/54061/chromeos/network/network_s... File chromeos/network/network_state_handler.cc (right): http://codereview.chromium.org/11192024/diff/54061/chromeos/network/network_s... chromeos/network/network_state_handler.cc:181: if (path.empty()) This ignores an error in GetAsString() or having an empty path, both of which seem like conditions that should log an error at the very least. If this is expected in normal circumstances, add a comment explaining why it's ok to ignore. http://codereview.chromium.org/11192024/diff/54061/chromeos/network/network_s... chromeos/network/network_state_handler.cc:226: (*iter)->GetAsString(&technology); See my comment in UpdateManagedList. Why is it ok to ignore an error here? http://codereview.chromium.org/11192024/diff/54061/chromeos/network/network_s... chromeos/network/network_state_handler.cc:250: property_changed = true; Since MANAGED_TYPE_DEVICE type never causes observers to be notified, does this need to be set? Perhaps it should be called network_property_changed? http://codereview.chromium.org/11192024/diff/54061/chromeos/network/network_s... chromeos/network/network_state_handler.cc:372: if (key == shill::kIPConfigProperty) { Is this something that should be re-factored at some point? If so, add a TODO. If not, add a comment explaining why this needs to be done here. http://codereview.chromium.org/11192024/diff/54061/chromeos/network/network_s... File chromeos/network/network_state_handler.h (right): http://codereview.chromium.org/11192024/diff/54061/chromeos/network/network_s... chromeos/network/network_state_handler.h:92: std::string HardwareAddressForType(const std::string& type) const; There is nothing in the method name that indicates that this only returns a value iff there is a connected network for |type|. Since this is a public method, I'd suggest something that explicitly called this out in the name. (MACForConnectedType perhaps?) http://codereview.chromium.org/11192024/diff/54061/chromeos/network/network_s... chromeos/network/network_state_handler.h:160: // Casts the list specified by |type| to a list of base ManagedState pointers. This comment is no longer valid - this now just returns the network_list_ or device_list_ based on |type|. http://codereview.chromium.org/11192024/diff/54061/chromeos/network/network_s... File chromeos/network/network_state_handler_observer.h (right): http://codereview.chromium.org/11192024/diff/54061/chromeos/network/network_s... chromeos/network/network_state_handler_observer.h:27: // If |property| is non empty then that property has changed. Otherwise stale comment? http://codereview.chromium.org/11192024/diff/54061/chromeos/network/network_s... File chromeos/network/network_state_handler_unittest.cc (right): http://codereview.chromium.org/11192024/diff/54061/chromeos/network/network_s... chromeos/network/network_state_handler_unittest.cc:24: namespace chromeos { Can this be moved to 95? http://codereview.chromium.org/11192024/diff/54061/chromeos/network/shill_pro... File chromeos/network/shill_property_handler.h (right): http://codereview.chromium.org/11192024/diff/54061/chromeos/network/shill_pro... chromeos/network/shill_property_handler.h:42: class CHROMEOS_EXPORT Delegate { I agree with Steven that ChangeListener/Listener would be a better name than Delegator (or the current Delegate). We don't have to go all academic design patterns, which is why I don't particularly like Delegator - I don't think it's used anywhere in the Chromium code base proper. Listener clarifies the intent - all property handler changes will be communicated to NetworkStateHandler. http://codereview.chromium.org/11192024/diff/54061/chromeos/network/shill_pro... chromeos/network/shill_property_handler.h:69: virtual void ManagerPropertyChanged() = 0; If the intention is to only have this be called when the technology list changed (which is what the logic in NetworkStateHandler implementation does) - then the name should reflect that. http://codereview.chromium.org/11192024/diff/54061/chromeos/network/shill_ser... File chromeos/network/shill_service_observer.h (right): http://codereview.chromium.org/11192024/diff/54061/chromeos/network/shill_ser... chromeos/network/shill_service_observer.h:18: class ShillServiceObserver : public ShillPropertyChangedObserver { Would it be make sense to call this ShillServicePropertyHandler to be analogous to ShillPropertyHandler (which is what internally uses it)?
PTAL https://codereview.chromium.org/11192024/diff/54061/chromeos/network/device_s... File chromeos/network/device_state.cc (right): https://codereview.chromium.org/11192024/diff/54061/chromeos/network/device_s... chromeos/network/device_state.cc:55: } On 2012/11/06 01:56:59, gauravsh wrote: > Don't you need to return true here? Done. https://codereview.chromium.org/11192024/diff/54061/chromeos/network/managed_... File chromeos/network/managed_state.h (right): https://codereview.chromium.org/11192024/diff/54061/chromeos/network/managed_... chromeos/network/managed_state.h:62: // Helper methods that log warnings if parsing failed. On 2012/11/06 01:56:59, gauravsh wrote: > If parsing fails, they also return false, correct? That seems more worth > mentioning then the part about logging. Done. > > Would letting the caller do the logging in case of failure make more sense? will > these messages be too spammy, and should perhaps be logged at a higher verbosity > level? These are never expected to fail; if they do then we have malformed data in a base::Value, or are calling the wrong method. https://codereview.chromium.org/11192024/diff/54061/chromeos/network/managed_... chromeos/network/managed_state.h:75: std::string path_; On 2012/11/06 01:56:59, gauravsh wrote: > Either add comments that describe what these are meant to represent, or make the > names more descriptive. path, name and type are very generic names. Added comments. https://codereview.chromium.org/11192024/diff/54061/chromeos/network/network_... File chromeos/network/network_state.h (right): https://codereview.chromium.org/11192024/diff/54061/chromeos/network/network_... chromeos/network/network_state.h:40: // Helpers (used e.g. when a state is cached) On 2012/11/06 01:56:59, gauravsh wrote: > Are there any places where one might be interested in querying if the state is > failure? Should there be a StateIsFailure helper? There is only a single error state "kStateFailure" whereas Shill provides more granularity for Connected and Connecting states. IsErrorState() may prove useful but I think I would rather wait to add it until we have a use case. https://codereview.chromium.org/11192024/diff/54061/chromeos/network/network_... File chromeos/network/network_state_handler.cc (right): https://codereview.chromium.org/11192024/diff/54061/chromeos/network/network_... chromeos/network/network_state_handler.cc:181: if (path.empty()) Shouldn't happen, changed to DCHECK https://codereview.chromium.org/11192024/diff/54061/chromeos/network/network_... chromeos/network/network_state_handler.cc:226: (*iter)->GetAsString(&technology); On 2012/11/06 01:56:59, gauravsh wrote: > See my comment in UpdateManagedList. Why is it ok to ignore an error here? Should never happen which is why it's a DCHECK. https://codereview.chromium.org/11192024/diff/54061/chromeos/network/network_... chromeos/network/network_state_handler.cc:250: property_changed = true; Renamed https://codereview.chromium.org/11192024/diff/54061/chromeos/network/network_... chromeos/network/network_state_handler.cc:372: if (key == shill::kIPConfigProperty) { Moved the comment inside the if() and clarified it. Also replaced the NOTREACHED with a DCHECK and eliminated property_changed = true, since no property change will occur until RequestIPConfig completes. https://codereview.chromium.org/11192024/diff/54061/chromeos/network/network_... File chromeos/network/network_state_handler.h (right): https://codereview.chromium.org/11192024/diff/54061/chromeos/network/network_... chromeos/network/network_state_handler.h:92: std::string HardwareAddressForType(const std::string& type) const; On 2012/11/06 01:56:59, gauravsh wrote: > There is nothing in the method name that indicates that this only returns a > value iff there is a connected network for |type|. Since this is a public > method, I'd suggest something that explicitly called this out in the name. > (MACForConnectedType perhaps?) That strikes me as the kind of detail that is better kept in the comment. e.g. 'First' isn't in the name either. https://codereview.chromium.org/11192024/diff/54061/chromeos/network/network_... chromeos/network/network_state_handler.h:160: // Casts the list specified by |type| to a list of base ManagedState pointers. On 2012/11/06 01:56:59, gauravsh wrote: > This comment is no longer valid - this now just returns the network_list_ or > device_list_ based on |type|. Done. https://codereview.chromium.org/11192024/diff/54061/chromeos/network/network_... File chromeos/network/network_state_handler_observer.h (right): https://codereview.chromium.org/11192024/diff/54061/chromeos/network/network_... chromeos/network/network_state_handler_observer.h:27: // If |property| is non empty then that property has changed. Otherwise On 2012/11/06 01:56:59, gauravsh wrote: > stale comment? Fixed. https://codereview.chromium.org/11192024/diff/54061/chromeos/network/network_... File chromeos/network/network_state_handler_unittest.cc (right): https://codereview.chromium.org/11192024/diff/54061/chromeos/network/network_... chromeos/network/network_state_handler_unittest.cc:24: namespace chromeos { On 2012/11/06 01:56:59, gauravsh wrote: > Can this be moved to 95? Done. https://codereview.chromium.org/11192024/diff/54061/chromeos/network/shill_pr... File chromeos/network/shill_property_handler.h (right): https://codereview.chromium.org/11192024/diff/54061/chromeos/network/shill_pr... chromeos/network/shill_property_handler.h:42: class CHROMEOS_EXPORT Delegate { On 2012/11/06 01:56:59, gauravsh wrote: > I agree with Steven that ChangeListener/Listener would be a better name than > Delegator (or the current Delegate). > > We don't have to go all academic design patterns, which is why I don't > particularly like Delegator - I don't think it's used anywhere in the Chromium > code base proper. Listener clarifies the intent - all property handler changes > will be communicated to NetworkStateHandler. Done. https://codereview.chromium.org/11192024/diff/54061/chromeos/network/shill_pr... chromeos/network/shill_property_handler.h:69: virtual void ManagerPropertyChanged() = 0; Changed the comment; at the moment this gets called only for the technology list, but as soon as we implement "airplane mode" (which I hope to soon) this will include changes to that property also. These changes are infrequent and I do not anticipate any need for separate callbacks. https://codereview.chromium.org/11192024/diff/54061/chromeos/network/shill_se... File chromeos/network/shill_service_observer.h (right): https://codereview.chromium.org/11192024/diff/54061/chromeos/network/shill_se... chromeos/network/shill_service_observer.h:18: class ShillServiceObserver : public ShillPropertyChangedObserver { On 2012/11/06 01:56:59, gauravsh wrote: > Would it be make sense to call this ShillServicePropertyHandler to be analogous > to ShillPropertyHandler (which is what internally uses it)? This is specifically just an Observer. Originally it was ShillPropertyHandler::ServiceObserver, but the official preference is to avoid embedded classes. ShillPropertyHandlerServiceObserver seemed pretty lengthy, especially since it doesn't have any explicit ShillPropertyHandler dependencies. I've moved it to the internal:: namespace to make it clear that it's not a public interface. I could possibly see renaming it ShillServicePropertyObserver? That just seems a little redundant since, what else would we observe? https://codereview.chromium.org/11192024/diff/63006/chromeos/chromeos.gyp File chromeos/chromeos.gyp (right): https://codereview.chromium.org/11192024/diff/63006/chromeos/chromeos.gyp#new... chromeos/chromeos.gyp:127: 'network/managed_state.cc', On 2012/11/06 01:51:24, hashimoto wrote: > managed_state.h missing? Oops. Fixed. https://codereview.chromium.org/11192024/diff/63006/chromeos/dbus/shill_devic... File chromeos/dbus/shill_device_client.cc (right): https://codereview.chromium.org/11192024/diff/63006/chromeos/dbus/shill_devic... chromeos/dbus/shill_device_client.cc:458: PropertyObserverList& GetObserverList(const dbus::ObjectPath& device_path) { On 2012/11/06 01:51:24, hashimoto wrote: > nit: Please use pointer instead of reference if there is no strong reason. I > think our style guide prefers pointers to references when they are non-const. I originally returned a pointer but then I had '*GetObserverList()' in FOR_EACH_OBSERVER which looks odd. It's kind of awkward either way, but this seemed slightly better since it makes it clear that this can never return NULL. Also, it's a private function so returning a reference seems reasonable. https://codereview.chromium.org/11192024/diff/63006/chromeos/dbus/shill_manag... File chromeos/dbus/shill_manager_client.cc (right): https://codereview.chromium.org/11192024/diff/63006/chromeos/dbus/shill_manag... chromeos/dbus/shill_manager_client.cc:418: if (observer_list_.size() == 0) On 2012/11/06 01:51:24, hashimoto wrote: > nit: Please leave a comment to describe why we need this check. Done. https://codereview.chromium.org/11192024/diff/63006/chromeos/dbus/shill_servi... File chromeos/dbus/shill_service_client.cc (right): https://codereview.chromium.org/11192024/diff/63006/chromeos/dbus/shill_servi... chromeos/dbus/shill_service_client.cc:399: PropertyObserverList& GetObserverList(const dbus::ObjectPath& device_path) { On 2012/11/06 01:51:24, hashimoto wrote: > nit: Please use pointer instead of reference if there is no strong reason. See other comment.
lgtm, with one nit. https://codereview.chromium.org/11192024/diff/53008/chromeos/chromeos.gyp File chromeos/chromeos.gyp (right): https://codereview.chromium.org/11192024/diff/53008/chromeos/chromeos.gyp#new... chromeos/chromeos.gyp:127: 'network/managed_state.h', nit: '.cc' comes before '.h'
lgtm https://codereview.chromium.org/11192024/diff/54061/chromeos/network/network_... File chromeos/network/network_state_handler.cc (right): https://codereview.chromium.org/11192024/diff/54061/chromeos/network/network_... chromeos/network/network_state_handler.cc:181: if (path.empty()) On 2012/11/06 03:17:03, stevenjb (chromium) wrote: > Shouldn't happen, changed to DCHECK A general question about the usage of DCHECK/NOTREACHED: I thought for release builds, we should ensure that in case of an error either - we return to a consistent state and continue execution - or we ignore the error here, knowing that a few statements later it will throw an exception (e.g. segmentation fault) anyways - or we explicitly CHECK In this case, only DCHECKing means, that if the error occurs on release, we will create a managed state with an empty path in line 189. To ensure a consistent state, we could go with if (path.empty()) { NOTREACHED(); continue; }
https://codereview.chromium.org/11192024/diff/53008/chromeos/chromeos.gyp File chromeos/chromeos.gyp (right): https://codereview.chromium.org/11192024/diff/53008/chromeos/chromeos.gyp#new... chromeos/chromeos.gyp:127: 'network/managed_state.h', On 2012/11/06 03:37:34, hashimoto wrote: > nit: '.cc' comes before '.h' DOH. Done.
On 2012/11/06 09:18:18, pneubeck wrote: > lgtm > > https://codereview.chromium.org/11192024/diff/54061/chromeos/network/network_... > File chromeos/network/network_state_handler.cc (right): > > https://codereview.chromium.org/11192024/diff/54061/chromeos/network/network_... > chromeos/network/network_state_handler.cc:181: if (path.empty()) > On 2012/11/06 03:17:03, stevenjb (chromium) wrote: > > Shouldn't happen, changed to DCHECK > A general question about the usage of DCHECK/NOTREACHED: > I thought for release builds, we should ensure that in case of an error either > - we return to a consistent state and continue execution > - or we ignore the error here, knowing that a few statements later it will > throw an exception (e.g. segmentation fault) anyways > - or we explicitly CHECK > > In this case, only DCHECKing means, that if the error occurs on release, we will > create a managed state with an empty path in line 189. > To ensure a consistent state, we could go with > if (path.empty()) { > NOTREACHED(); > continue; > } The style guide says: "you should not handle DCHECK() failures, even if failure would result in a crash. Attempting to handle a DCHECK() failure is a statement that the DCHECK() can fail, which contradicts the point of writing the DCHECK()." So, we shouldn't handle the case of an empty path, regardless of whether or not it results in a crash. Shill should never be sending us empty paths, the DCHECK is just documenting that this is not behavior that we handle. If we did handle it (i.e. with a 'continue') we would want to log a WARNING instead of calling NOTREACHED().
lgtm with a nit http://codereview.chromium.org/11192024/diff/54061/chromeos/network/network_s... File chromeos/network/network_state.h (right): http://codereview.chromium.org/11192024/diff/54061/chromeos/network/network_s... chromeos/network/network_state.h:40: // Helpers (used e.g. when a state is cached) On 2012/11/06 03:17:03, stevenjb (chromium) wrote: > On 2012/11/06 01:56:59, gauravsh wrote: > > Are there any places where one might be interested in querying if the state is > > failure? Should there be a StateIsFailure helper? > > There is only a single error state "kStateFailure" whereas Shill provides more > granularity for Connected and Connecting states. IsErrorState() may prove useful > but I think I would rather wait to add it until we have a use case. Cool, there are a few other error states but not sure how often they are actually used. http://codereview.chromium.org/11192024/diff/57035/chromeos/network/shill_pro... File chromeos/network/shill_property_handler.h (right): http://codereview.chromium.org/11192024/diff/57035/chromeos/network/shill_pro... chromeos/network/shill_property_handler.h:34: // requests properties for new devices/networks.It takes a Listener in its NIT: space before It
http://codereview.chromium.org/11192024/diff/54061/chromeos/network/network_s... File chromeos/network/network_state.h (right): http://codereview.chromium.org/11192024/diff/54061/chromeos/network/network_s... chromeos/network/network_state.h:40: // Helpers (used e.g. when a state is cached) On 2012/11/06 22:51:41, gauravsh wrote: > On 2012/11/06 03:17:03, stevenjb (chromium) wrote: > > On 2012/11/06 01:56:59, gauravsh wrote: > > > Are there any places where one might be interested in querying if the state > is > > > failure? Should there be a StateIsFailure helper? > > > > There is only a single error state "kStateFailure" whereas Shill provides more > > granularity for Connected and Connecting states. IsErrorState() may prove > useful > > but I think I would rather wait to add it until we have a use case. > > Cool, there are a few other error states but not sure how often they are > actually used. The only other one I see in service_constants.h is kStateActivationFailure which is kind of specialized, but may still be worth adding IsErrorState() for. I'll keep that in mind in the next CL :) If we have other error states we should be sure to add them to service_constants.h. http://codereview.chromium.org/11192024/diff/57035/chromeos/network/shill_pro... File chromeos/network/shill_property_handler.h (right): http://codereview.chromium.org/11192024/diff/57035/chromeos/network/shill_pro... chromeos/network/shill_property_handler.h:34: // requests properties for new devices/networks.It takes a Listener in its On 2012/11/06 22:51:41, gauravsh wrote: > NIT: space before It Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/stevenjb@chromium.org/11192024/52028
Change committed as 166375 |
