Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
675993005:
components: add wifi_sync component (Closed)
|
Created:
6 years, 2 months ago by mukesh agrawal Modified:
6 years, 1 month ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@local-master Project:
chromium Visibility:
Public. |
DescriptionAdd a new component for WiFi credential sync. Populate the new component with utilities related to WiFi security classes.
The WiFi security class utility code consists of a) an enum that provides an internal representation of WiFi security classes (e.g. WEP, PSK), and b) functions that convert between shill SecurityClass values, and the enum.
BUG=chromium:426693
BUG=chromium:426696
TEST=components_unittests --gtest_filter="Wifi*"
Patch Set 1 #
Total comments: 8
Patch Set 2 : add ifdefs for chromeos-specific code #Patch Set 3 : rename component, and add stub wifi_credential_syncable_service #
Total comments: 9
Patch Set 4 : rebase #Patch Set 5 : fix style issues, and unnecessary virtual functions #
Total comments: 33
Patch Set 6 : add/update GN files, add comments, clean up includes, move chromeos-only code to separate files #
Total comments: 1
Patch Set 7 : remove stub wifi_credential_syncable_service #
Total comments: 9
Patch Set 8 : fix nits #Patch Set 9 : rebase #
Messages
Total messages: 35 (6 generated)
quiche@chromium.org changed reviewers: + pstew@chromium.org, stevenjb@chromium.org
I'd kind of like to see more of this component before we add it, just to do a sanity check that we really need it (realizing of course that I am the one that suggested it). Some other 'network configuration' code that I thought we might want to put into a component actually ended up in a ui component, i.e. src/ui/network. (See https://codereview.chromium.org/686503002/). https://codereview.chromium.org/675993005/diff/1/components/network_configura... File components/network_configuration/wifi_security_class.cc (right): https://codereview.chromium.org/675993005/diff/1/components/network_configura... components/network_configuration/wifi_security_class.cc:8: #include "third_party/cros_system_api/dbus/service_constants.h" #if defined(OS_CHROMEOS) https://codereview.chromium.org/675993005/diff/1/components/network_configura... components/network_configuration/wifi_security_class.cc:11: #if defined(OS_CHROMEOS) https://codereview.chromium.org/675993005/diff/1/components/network_configura... File components/network_configuration/wifi_security_class.h (right): https://codereview.chromium.org/675993005/diff/1/components/network_configura... components/network_configuration/wifi_security_class.h:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. nit: no (c) https://codereview.chromium.org/675993005/diff/1/components/network_configura... components/network_configuration/wifi_security_class.h:24: WifiSecurityClass security_class_in, std::string* security_class_out); These functions should probably be inside #if defined(OS_CHROMEOS)
What's the best way to show the other code that goes in this component? Should I upload the individual CLs for the other classes? One large CL with the end result? (Not for submitting, of course. But to show where this ends up.) FWIW, the other classes I plan to add to this component are: - WifiCredentialSyncableService (implements the SyncableService API) - WifiCredentialSyncableServiceFactory - SyncJob (holds a sync request, until the network connection is successful, and sync is running) - SyncQueue (holds all pending wifi credential sync requests) On 2014/10/27 20:25:15, stevenjb wrote: > I'd kind of like to see more of this component before we add it, just to do a > sanity check that we really need it (realizing of course that I am the one that > suggested it). > > Some other 'network configuration' code that I thought we might want to put into > a component actually ended up in a ui component, i.e. src/ui/network. (See > https://codereview.chromium.org/686503002/).
https://codereview.chromium.org/675993005/diff/1/components/network_configura... File components/network_configuration/wifi_security_class.cc (right): https://codereview.chromium.org/675993005/diff/1/components/network_configura... components/network_configuration/wifi_security_class.cc:8: #include "third_party/cros_system_api/dbus/service_constants.h" On 2014/10/27 20:25:15, stevenjb wrote: > #if defined(OS_CHROMEOS) Done. https://codereview.chromium.org/675993005/diff/1/components/network_configura... components/network_configuration/wifi_security_class.cc:11: On 2014/10/27 20:25:15, stevenjb wrote: > #if defined(OS_CHROMEOS) Done. https://codereview.chromium.org/675993005/diff/1/components/network_configura... File components/network_configuration/wifi_security_class.h (right): https://codereview.chromium.org/675993005/diff/1/components/network_configura... components/network_configuration/wifi_security_class.h:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. On 2014/10/27 20:25:15, stevenjb wrote: > nit: no (c) Done. https://codereview.chromium.org/675993005/diff/1/components/network_configura... components/network_configuration/wifi_security_class.h:24: WifiSecurityClass security_class_in, std::string* security_class_out); On 2014/10/27 20:25:15, stevenjb wrote: > These functions should probably be inside #if defined(OS_CHROMEOS) Done.
On 2014/10/27 21:06:07, mukesh agrawal wrote: > What's the best way to show the other code that goes in this component? Should I > upload the individual CLs for the other classes? One large CL with the end > result? (Not for submitting, of course. But to show where this ends up.) > > FWIW, the other classes I plan to add to this component are: > - WifiCredentialSyncableService (implements the SyncableService API) > - WifiCredentialSyncableServiceFactory > - SyncJob (holds a sync request, until the network connection is successful, and > sync is running) > - SyncQueue (holds all pending wifi credential sync requests) > > On 2014/10/27 20:25:15, stevenjb wrote: > > I'd kind of like to see more of this component before we add it, just to do a > > sanity check that we really need it (realizing of course that I am the one > that > > suggested it). > > > > Some other 'network configuration' code that I thought we might want to put > into > > a component actually ended up in a ui component, i.e. src/ui/network. (See > > https://codereview.chromium.org/686503002/). Maybe merge this with the WifiCredentialSyncableService CL? I'm wondering if maybe we should just call this 'wifi_sync' instead, given that I'm no longer certain that there is any additional "network configuration" code to be added. (Apologies for the potentially bad suggestion).
sgtm. i'll give that a go. On 2014/10/27 21:40:54, stevenjb wrote: > Maybe merge this with the WifiCredentialSyncableService CL? > I'm wondering if maybe we should just call this 'wifi_sync' instead, given that > I'm no longer certain that there is any additional "network configuration" code > to be added. (Apologies for the potentially bad suggestion).
ready for another look. On 2014/10/27 21:46:21, mukesh agrawal wrote: > sgtm. i'll give that a go. > > On 2014/10/27 21:40:54, stevenjb wrote: > > Maybe merge this with the WifiCredentialSyncableService CL? > > I'm wondering if maybe we should just call this 'wifi_sync' instead, given > that > > I'm no longer certain that there is any additional "network configuration" > code > > to be added. (Apologies for the potentially bad suggestion).
quiche@chromium.org changed reviewers: + pavely@chromium.org, zea@chromium.org
quiche@chromium.org changed required reviewers: + stevenjb@chromium.org
primary reviewer: stevenjb@ cc: pavely@, zea@ to look at stub versions of WifiCredentialSyncableService and WifiCredentialSyncableServiceFactory.
https://codereview.chromium.org/675993005/diff/40001/components/wifi_sync/wif... File components/wifi_sync/wifi_credential_syncable_service.cc (right): https://codereview.chromium.org/675993005/diff/40001/components/wifi_sync/wif... components/wifi_sync/wifi_credential_syncable_service.cc:13: } Are both of these google-clang-formatted? (It just looks strange to me having one {} split and not the other. https://codereview.chromium.org/675993005/diff/40001/components/wifi_sync/wif... File components/wifi_sync/wifi_credential_syncable_service.h (right): https://codereview.chromium.org/675993005/diff/40001/components/wifi_sync/wif... components/wifi_sync/wifi_credential_syncable_service.h:29: virtual ~WifiCredentialSyncableService(); new style is: ~WifiCredentialSyncableService() override; Also, no 'virtual' for overrides below. https://codereview.chromium.org/675993005/diff/40001/components/wifi_sync/wif... components/wifi_sync/wifi_credential_syncable_service.h:45: virtual bool IsRunning(); nit: New virtuals should probably be declared first, with a brief explanation of why they are virtual (for tests?). https://codereview.chromium.org/675993005/diff/40001/components/wifi_sync/wif... File components/wifi_sync/wifi_credential_syncable_service_factory.h (right): https://codereview.chromium.org/675993005/diff/40001/components/wifi_sync/wif... components/wifi_sync/wifi_credential_syncable_service_factory.h:36: virtual ~WifiCredentialSyncableServiceFactory(); New formatting for overrides
Thanks, Steven. Please take another look. https://codereview.chromium.org/675993005/diff/40001/components/wifi_sync/wif... File components/wifi_sync/wifi_credential_syncable_service.cc (right): https://codereview.chromium.org/675993005/diff/40001/components/wifi_sync/wif... components/wifi_sync/wifi_credential_syncable_service.cc:13: } On 2014/10/28 21:17:31, stevenjb wrote: > Are both of these google-clang-formatted? (It just looks strange to me having > one {} split and not the other. Done. (Ran "git cl format" over the patch.) https://codereview.chromium.org/675993005/diff/40001/components/wifi_sync/wif... File components/wifi_sync/wifi_credential_syncable_service.h (right): https://codereview.chromium.org/675993005/diff/40001/components/wifi_sync/wif... components/wifi_sync/wifi_credential_syncable_service.h:29: virtual ~WifiCredentialSyncableService(); On 2014/10/28 21:17:31, stevenjb wrote: > new style is: > ~WifiCredentialSyncableService() override; > > Also, no 'virtual' for overrides below. Done. https://codereview.chromium.org/675993005/diff/40001/components/wifi_sync/wif... components/wifi_sync/wifi_credential_syncable_service.h:45: virtual bool IsRunning(); I actually don't know why I made these virtual, so I removed that. Since they're no longer virtual, I left them where they were. Let me know if they should still be moved. https://codereview.chromium.org/675993005/diff/40001/components/wifi_sync/wif... File components/wifi_sync/wifi_credential_syncable_service_factory.h (right): https://codereview.chromium.org/675993005/diff/40001/components/wifi_sync/wif... components/wifi_sync/wifi_credential_syncable_service_factory.h:36: virtual ~WifiCredentialSyncableServiceFactory(); On 2014/10/28 21:17:31, stevenjb wrote: > New formatting for overrides Done. (Also for the virtual functions below.)
quiche@chromium.org changed reviewers: + erg@chromium.org, erikwright@chromium.org
quiche@chromium.org changed required reviewers: + erg@chromium.org, erikwright@chromium.org, pavely@chromium.org, pstew@chromium.org
Requesting additional reviewers, per OWNERS complaints on pre-submit check: - erikwright@: components.gyp - erg@: DEPS on components/keyed_service - pavely@: DEPS on sync/api - pstew@: DEPS on cros_system_api
lgtm wrt cros_system_api
lgtm https://codereview.chromium.org/675993005/diff/40001/components/wifi_sync/wif... File components/wifi_sync/wifi_credential_syncable_service.h (right): https://codereview.chromium.org/675993005/diff/40001/components/wifi_sync/wif... components/wifi_sync/wifi_credential_syncable_service.h:45: virtual bool IsRunning(); On 2014/10/29 00:41:34, mukesh agrawal wrote: > I actually don't know why I made these virtual, so I removed that. Since they're > no longer virtual, I left them where they were. Let me know if they should still > be moved. I personally tend to put new public methods above overrides, but I don't think that's part of the style guide, just my own style, so they're fine here.
Please add an OWNERS file in components/wifi_sync As long as this remains a stub, the entries should be commented out and a components/OWNER will be required. Once the implementation approaches completion those owners may take over. https://codereview.chromium.org/675993005/diff/80001/components/components_te... File components/components_tests.gyp (right): https://codereview.chromium.org/675993005/diff/80001/components/components_te... components/components_tests.gyp:21: # Note: sources list duplicated in GN build. In the GN build, Do you need to update GN files? https://codereview.chromium.org/675993005/diff/80001/components/wifi_sync.gypi File components/wifi_sync.gypi (right): https://codereview.chromium.org/675993005/diff/80001/components/wifi_sync.gyp... components/wifi_sync.gypi:8: # GN version: //components/wifi_sync missing? https://codereview.chromium.org/675993005/diff/80001/components/wifi_sync/wif... File components/wifi_sync/wifi_credential_syncable_service.cc (right): https://codereview.chromium.org/675993005/diff/80001/components/wifi_sync/wif... components/wifi_sync/wifi_credential_syncable_service.cc:24: NOTIMPLEMENTED(); Is this a work-in-progress, or ready for review? https://codereview.chromium.org/675993005/diff/80001/components/wifi_sync/wif... File components/wifi_sync/wifi_credential_syncable_service.h (right): https://codereview.chromium.org/675993005/diff/80001/components/wifi_sync/wif... components/wifi_sync/wifi_credential_syncable_service.h:12: #include "components/wifi_sync/wifi_security_class.h" forward-decl for many of these. You do not need to include anything that is only referenced in the declaration of an override of the base class (by definition, the type must be sufficiently declared or forward-declared for the method declaration to parse). You still need to include them in the .cc, since you cannot assume that syncable_service.h includes complete definitions. https://codereview.chromium.org/675993005/diff/80001/components/wifi_sync/wif... components/wifi_sync/wifi_credential_syncable_service.h:25: class WifiCredentialSyncableService : public syncer::SyncableService, class comments? https://codereview.chromium.org/675993005/diff/80001/components/wifi_sync/wif... components/wifi_sync/wifi_credential_syncable_service.h:28: explicit WifiCredentialSyncableService(content::BrowserContext* context); comment https://codereview.chromium.org/675993005/diff/80001/components/wifi_sync/wif... components/wifi_sync/wifi_credential_syncable_service.h:44: bool IsRunning(); method comments? https://codereview.chromium.org/675993005/diff/80001/components/wifi_sync/wif... components/wifi_sync/wifi_credential_syncable_service.h:56: DISALLOW_COPY_AND_ASSIGN(WifiCredentialSyncableService); #include "base/macros.h" https://codereview.chromium.org/675993005/diff/80001/components/wifi_sync/wif... File components/wifi_sync/wifi_credential_syncable_service_factory.h (right): https://codereview.chromium.org/675993005/diff/80001/components/wifi_sync/wif... components/wifi_sync/wifi_credential_syncable_service_factory.h:8: #include "base/basictypes.h" I believe "macros.h" is now the direct line for DISALLOW_... https://codereview.chromium.org/675993005/diff/80001/components/wifi_sync/wif... components/wifi_sync/wifi_credential_syncable_service_factory.h:36: ~WifiCredentialSyncableServiceFactory() override; I don't believe override is technically correct here. And explicitly mark 'virtual'. https://codereview.chromium.org/675993005/diff/80001/components/wifi_sync/wif... File components/wifi_sync/wifi_security_class.cc (right): https://codereview.chromium.org/675993005/diff/80001/components/wifi_sync/wif... components/wifi_sync/wifi_security_class.cc:8: #if defined(OS_CHROMEOS) this whole file is chrome_os only. Can you use a file suffix to exclude it, instead of ifdef? https://codereview.chromium.org/675993005/diff/80001/components/wifi_sync/wif... File components/wifi_sync/wifi_security_class.h (right): https://codereview.chromium.org/675993005/diff/80001/components/wifi_sync/wif... components/wifi_sync/wifi_security_class.h:21: WifiSecurityClass WifiSecurityClassFromShillString( comments for these methods https://codereview.chromium.org/675993005/diff/80001/components/wifi_sync/wif... File components/wifi_sync/wifi_security_class_unittest.cc (right): https://codereview.chromium.org/675993005/diff/80001/components/wifi_sync/wif... components/wifi_sync/wifi_security_class_unittest.cc:8: #if defined(OS_CHROMEOS) same question about file suffix
https://codereview.chromium.org/675993005/diff/80001/components/wifi_sync/wif... File components/wifi_sync/wifi_credential_syncable_service_factory.cc (right): https://codereview.chromium.org/675993005/diff/80001/components/wifi_sync/wif... components/wifi_sync/wifi_credential_syncable_service_factory.cc:14: NOTIMPLEMENTED(); I don't understand why you've written this stub. Is this actually ready for review? https://codereview.chromium.org/675993005/diff/80001/components/wifi_sync/wif... File components/wifi_sync/wifi_credential_syncable_service_factory.h (right): https://codereview.chromium.org/675993005/diff/80001/components/wifi_sync/wif... components/wifi_sync/wifi_credential_syncable_service_factory.h:36: ~WifiCredentialSyncableServiceFactory() override; On 2014/10/29 18:18:25, erikwright wrote: > I don't believe override is technically correct here. > > And explicitly mark 'virtual'. override is the correct thing here. You should not mark this as virtual. This is currently being mass-converted across the code base.
lgtm https://codereview.chromium.org/675993005/diff/80001/components/wifi_sync/wif... File components/wifi_sync/wifi_credential_syncable_service_factory.h (right): https://codereview.chromium.org/675993005/diff/80001/components/wifi_sync/wif... components/wifi_sync/wifi_credential_syncable_service_factory.h:29: static KeyedService* BuildInstanceFor( You've stubbed more functions than absolutely necessary, In the future it is easier to forget removing unused functions than to forget adding necessary ones. I would start this class with just GetInstance/GetForProfile, ctor/dtor and BuildServiceInstanceFor.
Thanks, all. erikwright@, erg@, PTAL. https://codereview.chromium.org/675993005/diff/80001/components/components_te... File components/components_tests.gyp (right): https://codereview.chromium.org/675993005/diff/80001/components/components_te... components/components_tests.gyp:21: # Note: sources list duplicated in GN build. In the GN build, On 2014/10/29 18:18:24, erikwright wrote: > Do you need to update GN files? Done. https://codereview.chromium.org/675993005/diff/80001/components/wifi_sync.gypi File components/wifi_sync.gypi (right): https://codereview.chromium.org/675993005/diff/80001/components/wifi_sync.gyp... components/wifi_sync.gypi:8: # GN version: //components/wifi_sync On 2014/10/29 18:18:25, erikwright wrote: > missing? Done. https://codereview.chromium.org/675993005/diff/80001/components/wifi_sync/wif... File components/wifi_sync/wifi_credential_syncable_service.cc (right): https://codereview.chromium.org/675993005/diff/80001/components/wifi_sync/wif... components/wifi_sync/wifi_credential_syncable_service.cc:24: NOTIMPLEMENTED(); On 2014/10/29 18:18:25, erikwright wrote: > Is this a work-in-progress, or ready for review? While these implementations are just stubs, I think it makes sense to land these stubs. The values of these stubs is: 1. Having them in place will make it possible for me to iterate on the two halves of WiFi credential syncing separately. (One half is implementing this class, the other is the code that calls this class.) 2. The full implementation + tests is too large for a single CL. Getting the stubs in place let's me add other pieces in small CLs. 3. There was some concern that adding a new component just for WifiSecurityClass would be hard to justify. Adding this stub class makes the point of having wifi_sync clearer. How does that sound? https://codereview.chromium.org/675993005/diff/80001/components/wifi_sync/wif... File components/wifi_sync/wifi_credential_syncable_service.h (right): https://codereview.chromium.org/675993005/diff/80001/components/wifi_sync/wif... components/wifi_sync/wifi_credential_syncable_service.h:12: #include "components/wifi_sync/wifi_security_class.h" On 2014/10/29 18:18:25, erikwright wrote: > forward-decl for many of these. You do not need to include anything that is only > referenced in the declaration of an override of the base class (by definition, > the type must be sufficiently declared or forward-declared for the method > declaration to parse). > > You still need to include them in the .cc, since you cannot assume that > syncable_service.h includes complete definitions. Done. https://codereview.chromium.org/675993005/diff/80001/components/wifi_sync/wif... components/wifi_sync/wifi_credential_syncable_service.h:25: class WifiCredentialSyncableService : public syncer::SyncableService, On 2014/10/29 18:18:25, erikwright wrote: > class comments? Done. https://codereview.chromium.org/675993005/diff/80001/components/wifi_sync/wif... components/wifi_sync/wifi_credential_syncable_service.h:28: explicit WifiCredentialSyncableService(content::BrowserContext* context); On 2014/10/29 18:18:25, erikwright wrote: > comment Done. https://codereview.chromium.org/675993005/diff/80001/components/wifi_sync/wif... components/wifi_sync/wifi_credential_syncable_service.h:44: bool IsRunning(); On 2014/10/29 18:18:25, erikwright wrote: > method comments? Done. https://codereview.chromium.org/675993005/diff/80001/components/wifi_sync/wif... components/wifi_sync/wifi_credential_syncable_service.h:56: DISALLOW_COPY_AND_ASSIGN(WifiCredentialSyncableService); On 2014/10/29 18:18:25, erikwright wrote: > #include "base/macros.h" Done. https://codereview.chromium.org/675993005/diff/80001/components/wifi_sync/wif... File components/wifi_sync/wifi_credential_syncable_service_factory.cc (right): https://codereview.chromium.org/675993005/diff/80001/components/wifi_sync/wif... components/wifi_sync/wifi_credential_syncable_service_factory.cc:14: NOTIMPLEMENTED(); On 2014/10/29 19:41:17, Elliot Glaysher wrote: > I don't understand why you've written this stub. > > Is this actually ready for review? Fair point. This one can wait until later. https://codereview.chromium.org/675993005/diff/80001/components/wifi_sync/wif... File components/wifi_sync/wifi_credential_syncable_service_factory.h (right): https://codereview.chromium.org/675993005/diff/80001/components/wifi_sync/wif... components/wifi_sync/wifi_credential_syncable_service_factory.h:8: #include "base/basictypes.h" On 2014/10/29 18:18:25, erikwright wrote: > I believe "macros.h" is now the direct line for DISALLOW_... Dropping this file for now, per comment from erg@. https://codereview.chromium.org/675993005/diff/80001/components/wifi_sync/wif... components/wifi_sync/wifi_credential_syncable_service_factory.h:29: static KeyedService* BuildInstanceFor( On 2014/10/30 00:25:38, pavely wrote: > You've stubbed more functions than absolutely necessary, In the future it is > easier to forget removing unused functions than to forget adding necessary ones. > I would start this class with just GetInstance/GetForProfile, ctor/dtor and > BuildServiceInstanceFor. Acknowledged. https://codereview.chromium.org/675993005/diff/80001/components/wifi_sync/wif... components/wifi_sync/wifi_credential_syncable_service_factory.h:36: ~WifiCredentialSyncableServiceFactory() override; On 2014/10/29 19:41:18, Elliot Glaysher wrote: > On 2014/10/29 18:18:25, erikwright wrote: > > I don't believe override is technically correct here. > > > > And explicitly mark 'virtual'. > > override is the correct thing here. You should not mark this as virtual. This is > currently being mass-converted across the code base. Acknowledged. https://codereview.chromium.org/675993005/diff/80001/components/wifi_sync/wif... File components/wifi_sync/wifi_security_class.cc (right): https://codereview.chromium.org/675993005/diff/80001/components/wifi_sync/wif... components/wifi_sync/wifi_security_class.cc:8: #if defined(OS_CHROMEOS) On 2014/10/29 18:18:25, erikwright wrote: > this whole file is chrome_os only. Can you use a file suffix to exclude it, > instead of ifdef? Done. https://codereview.chromium.org/675993005/diff/80001/components/wifi_sync/wif... File components/wifi_sync/wifi_security_class.h (right): https://codereview.chromium.org/675993005/diff/80001/components/wifi_sync/wif... components/wifi_sync/wifi_security_class.h:21: WifiSecurityClass WifiSecurityClassFromShillString( On 2014/10/29 18:18:25, erikwright wrote: > comments for these methods Done. https://codereview.chromium.org/675993005/diff/80001/components/wifi_sync/wif... File components/wifi_sync/wifi_security_class_unittest.cc (right): https://codereview.chromium.org/675993005/diff/80001/components/wifi_sync/wif... components/wifi_sync/wifi_security_class_unittest.cc:8: #if defined(OS_CHROMEOS) On 2014/10/29 18:18:25, erikwright wrote: > same question about file suffix Done. https://codereview.chromium.org/675993005/diff/100001/components/wifi_sync/BU... File components/wifi_sync/BUILD.gn (right): https://codereview.chromium.org/675993005/diff/100001/components/wifi_sync/BU... components/wifi_sync/BUILD.gn:22: source_set("unit_tests") { Note that this clause isn't verified yet, because components/BUILD.gn wraps its unit test clause in "if (false)". If you prefer, I can remove this until the components GN file is ready to build unit tests.
https://codereview.chromium.org/675993005/diff/80001/components/wifi_sync/wif... File components/wifi_sync/wifi_credential_syncable_service.cc (right): https://codereview.chromium.org/675993005/diff/80001/components/wifi_sync/wif... components/wifi_sync/wifi_credential_syncable_service.cc:24: NOTIMPLEMENTED(); One more thought... Since these are just stubs, you might ask if the stubbed interface is correct. The answer to that is that each method is either a) specified by the SyncableService API, b) a stub extracted from an implementation that I've already written, or c) both. (For methods in case b, I can't fit the implementations in this CL, and still keep the CL reasonably sized.)
https://codereview.chromium.org/675993005/diff/80001/components/wifi_sync/wif... File components/wifi_sync/wifi_credential_syncable_service.cc (right): https://codereview.chromium.org/675993005/diff/80001/components/wifi_sync/wif... components/wifi_sync/wifi_credential_syncable_service.cc:24: NOTIMPLEMENTED(); On 2014/10/30 20:44:54, mukesh agrawal wrote: > One more thought... Since these are just stubs, you might ask if the stubbed > interface is correct. > > The answer to that is that each method is either a) specified by the > SyncableService API, b) a stub extracted from an implementation that I've > already written, or c) both. > > (For methods in case b, I can't fit the implementations in this CL, and still > keep the CL reasonably sized.) I think you should just remove this file, and come back and add a real implementation later.
On 2014/10/31 20:39:07, Elliot Glaysher wrote: > I think you should just remove this file, and come back and add a real > implementation later. stevenjb@, is that alright with you?
quiche@chromium.org changed required reviewers: - erg@chromium.org, pavely@chromium.org
pavely@ and erg@ no longer required reviewers, due to revised DEPS. (But feel free to share any concerns you have.) erikwright@, stevenjb@: PTAL. https://codereview.chromium.org/675993005/diff/120001/components/wifi_sync.gypi File components/wifi_sync.gypi (right): https://codereview.chromium.org/675993005/diff/120001/components/wifi_sync.gy... components/wifi_sync.gypi:8: # GN version: //components/wifi_sync Please ignore the "OLD" half of this diff. This was a false positive on git's copy detection.
lgtm w/nits https://codereview.chromium.org/675993005/diff/120001/components/wifi_sync/wi... File components/wifi_sync/wifi_security_class.h (right): https://codereview.chromium.org/675993005/diff/120001/components/wifi_sync/wi... components/wifi_sync/wifi_security_class.h:23: // nit: no separation within function comments https://codereview.chromium.org/675993005/diff/120001/components/wifi_sync/wi... components/wifi_sync/wifi_security_class.h:35: // |security_class_in| was recognized and valid. This can be simplified to "Returns false if |security_class_in| is SECURITY_CLASS_INVALID, or outside the range of the WifiSecurityClass enum." (The "returns true otherwise" part is implicit). https://codereview.chromium.org/675993005/diff/120001/components/wifi_sync/wi... File components/wifi_sync/wifi_security_class_chromeos.cc (right): https://codereview.chromium.org/675993005/diff/120001/components/wifi_sync/wi... components/wifi_sync/wifi_security_class_chromeos.cc:23: return SECURITY_CLASS_INVALID; No final else clause needed. Also, {} are unnecessary. https://codereview.chromium.org/675993005/diff/120001/components/wifi_sync/wi... components/wifi_sync/wifi_security_class_chromeos.cc:44: default: Rather than putting a default here, let the compiler fail if an enum is added, and put the CHECK outside the switch.
https://codereview.chromium.org/675993005/diff/120001/components/wifi_sync/wi... File components/wifi_sync/wifi_security_class.h (right): https://codereview.chromium.org/675993005/diff/120001/components/wifi_sync/wi... components/wifi_sync/wifi_security_class.h:23: // On 2014/11/04 17:21:21, stevenjb wrote: > nit: no separation within function comments Done. https://codereview.chromium.org/675993005/diff/120001/components/wifi_sync/wi... components/wifi_sync/wifi_security_class.h:35: // |security_class_in| was recognized and valid. On 2014/11/04 17:21:21, stevenjb wrote: > This can be simplified to "Returns false if |security_class_in| is > SECURITY_CLASS_INVALID, or outside the range of the WifiSecurityClass enum." > > (The "returns true otherwise" part is implicit). Done. https://codereview.chromium.org/675993005/diff/120001/components/wifi_sync/wi... File components/wifi_sync/wifi_security_class_chromeos.cc (right): https://codereview.chromium.org/675993005/diff/120001/components/wifi_sync/wi... components/wifi_sync/wifi_security_class_chromeos.cc:23: return SECURITY_CLASS_INVALID; On 2014/11/04 17:21:21, stevenjb wrote: > No final else clause needed. Also, {} are unnecessary. Done. https://codereview.chromium.org/675993005/diff/120001/components/wifi_sync/wi... components/wifi_sync/wifi_security_class_chromeos.cc:44: default: On 2014/11/04 17:21:21, stevenjb wrote: > Rather than putting a default here, let the compiler fail if an enum is added, > and put the CHECK outside the switch. Done. I also changed the CHECK to a LOG(ERROR), to match the documentation in the header file. (Aborting on bad data here does seem like overkill.)
Cheers. Still lgtm.
This looks fine but I'd like to see the code that actually uses this CL before we introduce a new component that may just end up being dead code.
On 2014/11/05 15:26:42, erikwright wrote:
> This looks fine but I'd like to see the code that actually uses this CL before
> we introduce a new component that may just end up being dead code.
Ok, I've uploaded the next six patches in this series. The patch series is as
follows. ("->" denotes this CL.)
-> CL:675993005 components: add wifi_sync component
CL:709683004 wifi_sync: add a stub SyncableService
CL:713583002 wifi_sync: add utility to check if a network allows passwords
CL:706233004 wifi_sync: implement ACTION_ADD for
WifiCredentialSyncableService
CL:687803012 wifi_sync: add password caching to WifiCredentialSyncableService
CL:708223002 wifi_sync: implement ACTION_UPDATE for
WifiCredentialSyncableService
CL:706423002 wifi_sync: implement ACTION_DELETE for
WifiCredentialSyncableService
D'oh! Let's try linkifying again: * crrev.com/675993005 components: add wifi_sync component crrev.com/709683004 wifi_sync: add a stub SyncableService crrev.com/713583002 wifi_sync: add utility to check if a network allows passwords crrev.com/706233004 wifi_sync: implement ACTION_ADD for WifiCredentialSyncableService crrev.com/687803012 wifi_sync: add password caching to WifiCredentialSyncableService crrev.com/708223002 wifi_sync: implement ACTION_UPDATE for WifiCredentialSyncableService crrev.com/706423002 wifi_sync: implement ACTION_DELETE for WifiCredentialSyncableService * denotes this CL.
One more try: * http://crrev.com/675993005 components: add wifi_sync component http://crrev.com/709683004 wifi_sync: add a stub SyncableService http://crrev.com/713583002 wifi_sync: add utility to check if a network allows passwords http://crrev.com/706233004 wifi_sync: implement ACTION_ADD for WifiCredentialSyncableService http://crrev.com/687803012 wifi_sync: add password caching to WifiCredentialSyncableService http://crrev.com/708223002 wifi_sync: implement ACTION_UPDATE for WifiCredentialSyncableService http://crrev.com/706423002 wifi_sync: implement ACTION_DELETE for WifiCredentialSyncableService* * Denotes this CL.
Closing this CL, as the useful bits have been absorbed into https://codereview.chromium.org/709683004/. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
