Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
836303004:
wifi_sync: implement ACTION_ADD in WifiCredentialSyncableService (Closed)
|
Created:
5 years, 11 months ago by mukesh agrawal Modified:
5 years, 10 months ago CC:
chromium-reviews, tim+watch_chromium.org, pvalenzuela+watch_chromium.org, maxbogue+watch_chromium.org, zea+watch_chromium.org, maniscalco+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@submit-4.2-wifi-config-delegate Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionwifi_sync: implement ACTION_ADD in WifiCredentialSyncableService
Implement support for adding networks. Specifically:
1) When an ACTION_ADD change is received from sync, apply that
change to the platform's network configruation.
2) Provide a method that upper layers can call, to add a network
to Chrome Sync.
BUG=chromium:431435
TEST=components_unittests --gtest_filter="Wifi*"
Committed: https://crrev.com/c96b5d5783a331aeea09e91e87f2f3678909c37f
Cr-Commit-Position: refs/heads/master@{#313109}
Patch Set 1 #Patch Set 2 : rebase #
Total comments: 38
Patch Set 3 : add more unit tests for ProcessSyncChanges #
Total comments: 4
Patch Set 4 : rebase to ToT #
Total comments: 7
Patch Set 5 : address comments on PS3 (stevenjb) and PS4 (pavely) #
Total comments: 1
Patch Set 6 : apply git cl format #
Total comments: 34
Patch Set 7 : address comments on PS6 #
Total comments: 1
Patch Set 8 : virtual -> override #
Messages
Total messages: 26 (5 generated)
quiche@chromium.org changed reviewers: + erikwright@chromium.org, pavely@chromium.org, pstew@chromium.org, stevenjb@chromium.org
quiche@chromium.org changed required reviewers: + erikwright@chromium.org, pavely@chromium.org, stevenjb@chromium.org
The build failure is due to this being an incremental CL. (I didn't realize the trybots don't have logic to pull in dependent CLs.)
FYI: While 'git cl try' can only submit what is uploaded to Rietveld, 'git try' has options that allow you to submit the entire diff from origin/master to HEAD, allowing you to run your full set of CLs leading up to this one on the try bots. https://codereview.chromium.org/836303004/diff/20001/components/wifi_sync/wif... File components/wifi_sync/wifi_credential_syncable_service_factory.cc (right): https://codereview.chromium.org/836303004/diff/20001/components/wifi_sync/wif... components/wifi_sync/wifi_credential_syncable_service_factory.cc:43: NOTIMPLEMENTED(); Do you intend for this to CHECK on other platforms? Presumably you don't expect it to ever be called? https://codereview.chromium.org/836303004/diff/20001/components/wifi_sync/wif... File components/wifi_sync/wifi_credential_syncable_service_unittest.cc (right): https://codereview.chromium.org/836303004/diff/20001/components/wifi_sync/wif... components/wifi_sync/wifi_credential_syncable_service_unittest.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. 2015 https://codereview.chromium.org/836303004/diff/20001/components/wifi_sync/wif... components/wifi_sync/wifi_credential_syncable_service_unittest.cc:29: auto config_delegate = make_scoped_ptr(new FakeWifiConfigDelegate()); not a case for auto. https://codereview.chromium.org/836303004/diff/20001/components/wifi_sync/wif... components/wifi_sync/wifi_credential_syncable_service_unittest.cc:30: config_delegate_ = config_delegate.get(); I'm not sure if there's a hard convention or rule, but I would think that all primitive members should be initialized in the initializer list even if they are definitely assigned in the constructor body. https://codereview.chromium.org/836303004/diff/20001/components/wifi_sync/wif... components/wifi_sync/wifi_credential_syncable_service_unittest.cc:92: auto sync_error(ProcessSyncChanges(FROM_HERE, syncer::SyncChangeList())); not a case for auto, here and below https://codereview.chromium.org/836303004/diff/20001/components/wifi_sync/wif... components/wifi_sync/wifi_credential_syncable_service_unittest.cc:108: // The success cases for ProcessSyncChanges will be tested as part of Is this consistent with other syncable services?
https://codereview.chromium.org/836303004/diff/20001/components/wifi_sync/wif... File components/wifi_sync/wifi_credential_syncable_service.cc (right): https://codereview.chromium.org/836303004/diff/20001/components/wifi_sync/wif... components/wifi_sync/wifi_credential_syncable_service.cc:165: for (const auto &sync_change : change_list) { auto& syc_change https://codereview.chromium.org/836303004/diff/20001/components/wifi_sync/wif... components/wifi_sync/wifi_credential_syncable_service.cc:222: // TODO(quiche): Handle case where network already exists. I believe that the sync code will crash if we attempt to ADD an existing item; we probably need to avoid that in this CL. This can be done simply by keeping track of the synced networks in this class (e.g. set of item_id's or a map of ids to WifiCredential which may be useful later). https://codereview.chromium.org/836303004/diff/20001/components/wifi_sync/wif... File components/wifi_sync/wifi_credential_syncable_service.h (right): https://codereview.chromium.org/836303004/diff/20001/components/wifi_sync/wif... components/wifi_sync/wifi_credential_syncable_service.h:37: WifiCredentialSyncableService( explicit https://codereview.chromium.org/836303004/diff/20001/components/wifi_sync/wif... components/wifi_sync/wifi_credential_syncable_service.h:67: const scoped_ptr<WifiConfigDelegate> network_config_delegate_; nit: WS https://codereview.chromium.org/836303004/diff/20001/components/wifi_sync/wif... File components/wifi_sync/wifi_credential_syncable_service_factory.cc (right): https://codereview.chromium.org/836303004/diff/20001/components/wifi_sync/wif... components/wifi_sync/wifi_credential_syncable_service_factory.cc:37: // ManagedNetworkConfigurationHandler singleton. This comment is overly complicated. All you need to say is something like: "Note: NetworkHandler is a singleton that is managed by ChromeBrowserMainPartsChromeos and destroyed after all KeyedService instances are destroyed". (In particular, the relationship between NetworkHandler, DBusServices, and ChromeBrowserMainPartsChromeos is an external detail that might change at some point). https://codereview.chromium.org/836303004/diff/20001/components/wifi_sync/wif... File components/wifi_sync/wifi_credential_syncable_service_unittest.cc (right): https://codereview.chromium.org/836303004/diff/20001/components/wifi_sync/wif... components/wifi_sync/wifi_credential_syncable_service_unittest.cc:30: config_delegate_ = config_delegate.get(); On 2015/01/13 19:21:59, erikwright wrote: > I'm not sure if there's a hard convention or rule, but I would think that all > primitive members should be initialized in the initializer list even if they are > definitely assigned in the constructor body. That is correct, config_delegate_ should be initialized to nullptr in the constructor. That said, I think this is awkward at best. I would either add a config_delegate_for_test() accessor to WifiCredentialSyncableService, or remove the local scoped ptr and just assign 'new FakeWifiConfigDelegate()' directly to config_delegate_ with a comment that ownership is passed to syncable_service_. https://codereview.chromium.org/836303004/diff/20001/components/wifi_sync/wif... components/wifi_sync/wifi_credential_syncable_service_unittest.cc:37: // methods they call. I don't think noting the sorting order of these methods is particularly helpful. https://codereview.chromium.org/836303004/diff/20001/components/wifi_sync/wif... components/wifi_sync/wifi_credential_syncable_service_unittest.cc:42: } WS https://codereview.chromium.org/836303004/diff/20001/components/wifi_sync/wif... components/wifi_sync/wifi_credential_syncable_service_unittest.cc:53: } WS https://codereview.chromium.org/836303004/diff/20001/components/wifi_sync/wif... components/wifi_sync/wifi_credential_syncable_service_unittest.cc:58: } WS https://codereview.chromium.org/836303004/diff/20001/components/wifi_sync/wif... components/wifi_sync/wifi_credential_syncable_service_unittest.cc:70: } WS
https://codereview.chromium.org/836303004/diff/20001/components/wifi_sync/wif... File components/wifi_sync/wifi_credential_syncable_service_unittest.cc (right): https://codereview.chromium.org/836303004/diff/20001/components/wifi_sync/wif... components/wifi_sync/wifi_credential_syncable_service_unittest.cc:30: config_delegate_ = config_delegate.get(); On 2015/01/14 00:12:49, stevenjb wrote: > On 2015/01/13 19:21:59, erikwright wrote: > > I'm not sure if there's a hard convention or rule, but I would think that all > > primitive members should be initialized in the initializer list even if they > are > > definitely assigned in the constructor body. > > That is correct, config_delegate_ should be initialized to nullptr in the > constructor. > That said, I think this is awkward at best. I would either add a > config_delegate_for_test() accessor to WifiCredentialSyncableService, or remove > the local scoped ptr and just assign 'new FakeWifiConfigDelegate()' directly to > config_delegate_ with a comment that ownership is passed to syncable_service_. This pattern is common and reasonable in unit tests. There is no reason to complicate the public API of the class under test. An alternative approach is to split the mock object in two. You create a data object (in this case, just an int) and pass a pointer to that in the mock's constructor. Then you pass the mock to the class under test and use the data object to verify the behaviour. For example: int call_count = 0; // really, should be a member and initialized in initializer list syncable_service_.reset(new WifiCredentialSyncableService(make_scoped_ptr(new FakeWifiConfigDelegate(&call_count)))); Naturally, if the state of the mock becomes more complex you would introduce a struct or class to manage the data instead of a primitive. I have three different approaches, depending on the complexity of the situation: (1) As coded in this patchset, when the lifetime of the mock object is well-known (i.e. I can safely avoid using my raw pointer after the class under test destroys it) (2) As above in this comment, if I can at least guarantee that the shared data object (or primitive) will outlive the mock, or if multiple mock instances need to collect data together. (3) Similar to (2), but refcounting the data object, if the lifetime of the mock is obscured somehow and might otherwise outlive the data object.
Thanks, and PTAL. https://codereview.chromium.org/836303004/diff/20001/components/components_te... File components/components_tests.gyp (right): https://codereview.chromium.org/836303004/diff/20001/components/components_te... components/components_tests.gyp:817: 'wifi_sync/fake_wifi_config_delegate.cc', Removed due to rolling the Fake into the WCSS unittest. https://codereview.chromium.org/836303004/diff/20001/components/wifi_sync/BUI... File components/wifi_sync/BUILD.gn (right): https://codereview.chromium.org/836303004/diff/20001/components/wifi_sync/BUI... components/wifi_sync/BUILD.gn:41: "fake_wifi_config_delegate.cc", Removed due to rolling the Fake into the WCSS unittest. https://codereview.chromium.org/836303004/diff/20001/components/wifi_sync/wif... File components/wifi_sync/wifi_credential_syncable_service.cc (right): https://codereview.chromium.org/836303004/diff/20001/components/wifi_sync/wif... components/wifi_sync/wifi_credential_syncable_service.cc:165: for (const auto &sync_change : change_list) { On 2015/01/14 00:12:48, stevenjb wrote: > auto& syc_change Done. https://codereview.chromium.org/836303004/diff/20001/components/wifi_sync/wif... components/wifi_sync/wifi_credential_syncable_service.cc:222: // TODO(quiche): Handle case where network already exists. On 2015/01/14 00:12:48, stevenjb wrote: > I believe that the sync code will crash if we attempt to ADD an existing item; > we probably need to avoid that in this CL. This can be done simply by keeping > track of the synced networks in this class (e.g. set of item_id's or a map of > ids to WifiCredential which may be useful later). > If it's alright with you, I'd like to take care of this separately (crbug.com/449726). (The feature is still off by default, so waiting a little bit shouldn't cause problems for users.) https://codereview.chromium.org/836303004/diff/20001/components/wifi_sync/wif... File components/wifi_sync/wifi_credential_syncable_service.h (right): https://codereview.chromium.org/836303004/diff/20001/components/wifi_sync/wif... components/wifi_sync/wifi_credential_syncable_service.h:37: WifiCredentialSyncableService( On 2015/01/14 00:12:48, stevenjb wrote: > explicit Done. https://codereview.chromium.org/836303004/diff/20001/components/wifi_sync/wif... components/wifi_sync/wifi_credential_syncable_service.h:67: const scoped_ptr<WifiConfigDelegate> network_config_delegate_; On 2015/01/14 00:12:48, stevenjb wrote: > nit: WS Done. https://codereview.chromium.org/836303004/diff/20001/components/wifi_sync/wif... File components/wifi_sync/wifi_credential_syncable_service_factory.cc (right): https://codereview.chromium.org/836303004/diff/20001/components/wifi_sync/wif... components/wifi_sync/wifi_credential_syncable_service_factory.cc:37: // ManagedNetworkConfigurationHandler singleton. On 2015/01/14 00:12:49, stevenjb wrote: > This comment is overly complicated. All you need to say is something like: > "Note: NetworkHandler is a singleton that is managed by > ChromeBrowserMainPartsChromeos and destroyed after all KeyedService instances > are destroyed". > > (In particular, the relationship between NetworkHandler, DBusServices, and > ChromeBrowserMainPartsChromeos is an external detail that might change at some > point). Done. https://codereview.chromium.org/836303004/diff/20001/components/wifi_sync/wif... components/wifi_sync/wifi_credential_syncable_service_factory.cc:43: NOTIMPLEMENTED(); On 2015/01/13 19:21:59, erikwright wrote: > Do you intend for this to CHECK on other platforms? Presumably you don't expect > it to ever be called? True -- I don't expect this to be called at all on other platforms. So I've changed this to NOTREACHED(). (While NOTREACHED is a DCHECK rather than a CHECK, I don't see any reason to make this particular NOTREACHED a CHECK. Let me know if you disagree.) https://codereview.chromium.org/836303004/diff/20001/components/wifi_sync/wif... File components/wifi_sync/wifi_credential_syncable_service_unittest.cc (right): https://codereview.chromium.org/836303004/diff/20001/components/wifi_sync/wif... components/wifi_sync/wifi_credential_syncable_service_unittest.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2015/01/13 19:21:59, erikwright (OOO Jan 19) wrote: > 2015 Done. https://codereview.chromium.org/836303004/diff/20001/components/wifi_sync/wif... components/wifi_sync/wifi_credential_syncable_service_unittest.cc:29: auto config_delegate = make_scoped_ptr(new FakeWifiConfigDelegate()); On 2015/01/13 19:21:59, erikwright (OOO Jan 19) wrote: > not a case for auto. Done. https://codereview.chromium.org/836303004/diff/20001/components/wifi_sync/wif... components/wifi_sync/wifi_credential_syncable_service_unittest.cc:30: config_delegate_ = config_delegate.get(); On 2015/01/14 14:59:27, erikwright (OOO Jan 19) wrote: > On 2015/01/14 00:12:49, stevenjb wrote: > > On 2015/01/13 19:21:59, erikwright wrote: > > > I'm not sure if there's a hard convention or rule, but I would think that > all > > > primitive members should be initialized in the initializer list even if they > > are > > > definitely assigned in the constructor body. > > > > That is correct, config_delegate_ should be initialized to nullptr in the > > constructor. > > That said, I think this is awkward at best. I would either add a > > config_delegate_for_test() accessor to WifiCredentialSyncableService, or > remove > > the local scoped ptr and just assign 'new FakeWifiConfigDelegate()' directly > to > > config_delegate_ with a comment that ownership is passed to syncable_service_. > > This pattern is common and reasonable in unit tests. There is no reason to > complicate the public API of the class under test. > > An alternative approach is to split the mock object in two. You create a data > object (in this case, just an int) and pass a pointer to that in the mock's > constructor. Then you pass the mock to the class under test and use the data > object to verify the behaviour. > > For example: > > int call_count = 0; // really, should be a member and initialized in > initializer list > syncable_service_.reset(new WifiCredentialSyncableService(make_scoped_ptr(new > FakeWifiConfigDelegate(&call_count)))); > > Naturally, if the state of the mock becomes more complex you would introduce a > struct or class to manage the data instead of a primitive. > > I have three different approaches, depending on the complexity of the situation: > > (1) As coded in this patchset, when the lifetime of the mock object is > well-known (i.e. I can safely avoid using my raw pointer after the class under > test destroys it) > (2) As above in this comment, if I can at least guarantee that the shared data > object (or primitive) will outlive the mock, or if multiple mock instances need > to collect data together. > (3) Similar to (2), but refcounting the data object, if the lifetime of the mock > is obscured somehow and might otherwise outlive the data object. I've moved the allocation of FakeWifiConfigDelegate into the initializer list. This takes care of initialize the field in the initializer list, while making the overall code slightly simpler. Since we don't allow exceptions, this code should still guarantee that we don't leak the Fake. https://codereview.chromium.org/836303004/diff/20001/components/wifi_sync/wif... components/wifi_sync/wifi_credential_syncable_service_unittest.cc:37: // methods they call. On 2015/01/14 00:12:49, stevenjb wrote: > I don't think noting the sorting order of these methods is particularly helpful. Done. https://codereview.chromium.org/836303004/diff/20001/components/wifi_sync/wif... components/wifi_sync/wifi_credential_syncable_service_unittest.cc:42: } On 2015/01/14 00:12:49, stevenjb wrote: > WS I believe this case is correct. AddToSyncedNetworks is also covered by the "// comment" at line 35, so no whitespace is warranted. Let me know if I've misunderstood. https://codereview.chromium.org/836303004/diff/20001/components/wifi_sync/wif... components/wifi_sync/wifi_credential_syncable_service_unittest.cc:53: } On 2015/01/14 00:12:49, stevenjb wrote: > WS Done. https://codereview.chromium.org/836303004/diff/20001/components/wifi_sync/wif... components/wifi_sync/wifi_credential_syncable_service_unittest.cc:58: } On 2015/01/14 00:12:49, stevenjb wrote: > WS Done. https://codereview.chromium.org/836303004/diff/20001/components/wifi_sync/wif... components/wifi_sync/wifi_credential_syncable_service_unittest.cc:70: } On 2015/01/14 00:12:49, stevenjb wrote: > WS Done. https://codereview.chromium.org/836303004/diff/20001/components/wifi_sync/wif... components/wifi_sync/wifi_credential_syncable_service_unittest.cc:92: auto sync_error(ProcessSyncChanges(FROM_HERE, syncer::SyncChangeList())); On 2015/01/13 19:21:59, erikwright (OOO Jan 19) wrote: > not a case for auto, here and below Done. https://codereview.chromium.org/836303004/diff/20001/components/wifi_sync/wif... components/wifi_sync/wifi_credential_syncable_service_unittest.cc:108: // The success cases for ProcessSyncChanges will be tested as part of On 2015/01/13 19:21:59, erikwright (OOO Jan 19) wrote: > Is this consistent with other syncable services? Thanks for asking. Taking a closer look at some of the other SyncableServices, it turns out that there is a solution for the problem I ran into. (I needed the AttachmentServiceProxyForTest.)
https://codereview.chromium.org/836303004/diff/20001/components/wifi_sync/wif... File components/wifi_sync/wifi_credential_syncable_service.cc (right): https://codereview.chromium.org/836303004/diff/20001/components/wifi_sync/wif... components/wifi_sync/wifi_credential_syncable_service.cc:222: // TODO(quiche): Handle case where network already exists. On 2015/01/20 21:13:15, mukesh agrawal wrote: > On 2015/01/14 00:12:48, stevenjb wrote: > > I believe that the sync code will crash if we attempt to ADD an existing item; > > we probably need to avoid that in this CL. This can be done simply by keeping > > track of the synced networks in this class (e.g. set of item_id's or a map of > > ids to WifiCredential which may be useful later). > > > > If it's alright with you, I'd like to take care of this separately > (crbug.com/449726). (The feature is still off by default, so waiting a little > bit shouldn't cause problems for users.) I guess we can wait, although I personally think that is a bit too incremental. At minimum we should add a comment to the header that the code will crash if this is called with an item that has already been synced. https://codereview.chromium.org/836303004/diff/40001/components/wifi_sync/wif... File components/wifi_sync/wifi_credential_syncable_service.cc (right): https://codereview.chromium.org/836303004/diff/40001/components/wifi_sync/wif... components/wifi_sync/wifi_credential_syncable_service.cc:28: sync_pb::EntitySpecifics* out_buffer) { Is this clang formatted? It looks like these will fit aligned after the ( https://codereview.chromium.org/836303004/diff/40001/components/wifi_sync/wif... components/wifi_sync/wifi_credential_syncable_service.cc:176: std::string passphrase; nit: maybe use a struct for these to simplify the function calls a bit?
lgtm with small comments https://codereview.chromium.org/836303004/diff/60001/components/wifi_sync/DEPS File components/wifi_sync/DEPS (right): https://codereview.chromium.org/836303004/diff/60001/components/wifi_sync/DEP... components/wifi_sync/DEPS:9: "+sync/internal_api/public", could you put "+sync/internal_api/public/attachments" here instead? https://codereview.chromium.org/836303004/diff/60001/components/wifi_sync/wif... File components/wifi_sync/wifi_credential_syncable_service.cc (right): https://codereview.chromium.org/836303004/diff/60001/components/wifi_sync/wif... components/wifi_sync/wifi_credential_syncable_service.cc:116: : network_config_delegate_(network_config_delegate.release()) { You can use Pass() instead of release() here. https://codereview.chromium.org/836303004/diff/60001/components/wifi_sync/wif... components/wifi_sync/wifi_credential_syncable_service.cc:165: if (!sync_change.IsValid()) { I think DCHECK(sync_change.IsValid()) is sufficient here. It is not expected to receive invalid sync change here. With DCHECK you will catch issues during testing while returning SyncError can go unnoticed.
https://codereview.chromium.org/836303004/diff/20001/components/wifi_sync/wif... File components/wifi_sync/wifi_credential_syncable_service.cc (right): https://codereview.chromium.org/836303004/diff/20001/components/wifi_sync/wif... components/wifi_sync/wifi_credential_syncable_service.cc:222: // TODO(quiche): Handle case where network already exists. On 2015/01/20 22:20:27, stevenjb wrote: > On 2015/01/20 21:13:15, mukesh agrawal wrote: > > On 2015/01/14 00:12:48, stevenjb wrote: > > > I believe that the sync code will crash if we attempt to ADD an existing > item; > > > we probably need to avoid that in this CL. This can be done simply by > keeping > > > track of the synced networks in this class (e.g. set of item_id's or a map > of > > > ids to WifiCredential which may be useful later). > > > > > > > If it's alright with you, I'd like to take care of this separately > > (crbug.com/449726). (The feature is still off by default, so waiting a little > > bit shouldn't cause problems for users.) > > I guess we can wait, although I personally think that is a bit too incremental. > At minimum we should add a comment to the header that the code will crash if > this is called with an item that has already been synced. Done. Yeah, the documentation in the header file was misleading. Sorry about that. https://codereview.chromium.org/836303004/diff/40001/components/wifi_sync/wif... File components/wifi_sync/wifi_credential_syncable_service.cc (right): https://codereview.chromium.org/836303004/diff/40001/components/wifi_sync/wif... components/wifi_sync/wifi_credential_syncable_service.cc:28: sync_pb::EntitySpecifics* out_buffer) { On 2015/01/20 22:20:27, stevenjb wrote: > Is this clang formatted? It looks like these will fit aligned after the ( I'll reformat in PS6. (That should make the diff for PS4 -> PS5 easier to follow.) https://codereview.chromium.org/836303004/diff/40001/components/wifi_sync/wif... components/wifi_sync/wifi_credential_syncable_service.cc:176: std::string passphrase; On 2015/01/20 22:20:27, stevenjb wrote: > nit: maybe use a struct for these to simplify the function calls a bit? Done. https://codereview.chromium.org/836303004/diff/60001/components/wifi_sync/DEPS File components/wifi_sync/DEPS (right): https://codereview.chromium.org/836303004/diff/60001/components/wifi_sync/DEP... components/wifi_sync/DEPS:9: "+sync/internal_api/public", On 2015/01/21 23:08:40, pavely wrote: > could you put "+sync/internal_api/public/attachments" here instead? Done. https://codereview.chromium.org/836303004/diff/60001/components/wifi_sync/wif... File components/wifi_sync/wifi_credential_syncable_service.cc (right): https://codereview.chromium.org/836303004/diff/60001/components/wifi_sync/wif... components/wifi_sync/wifi_credential_syncable_service.cc:116: : network_config_delegate_(network_config_delegate.release()) { On 2015/01/21 23:08:40, pavely wrote: > You can use Pass() instead of release() here. Done. https://codereview.chromium.org/836303004/diff/60001/components/wifi_sync/wif... components/wifi_sync/wifi_credential_syncable_service.cc:165: if (!sync_change.IsValid()) { On 2015/01/21 23:08:40, pavely wrote: > I think DCHECK(sync_change.IsValid()) is sufficient here. It is not expected to > receive invalid sync change here. > With DCHECK you will catch issues during testing while returning SyncError can > go unnoticed. Done. https://codereview.chromium.org/836303004/diff/60001/components/wifi_sync/wif... File components/wifi_sync/wifi_credential_syncable_service_unittest.cc (right): https://codereview.chromium.org/836303004/diff/60001/components/wifi_sync/wif... components/wifi_sync/wifi_credential_syncable_service_unittest.cc:173: TEST_F(WifiCredentialSyncableServiceTest, ProcessSyncChangesInvalidChange) { Removed test as this case is now handled with a DCHECK. (Per pavely@, Sync should never give us an invalid change.)
Thanks, all. stevenjb + erikwright: PTAL (Note: PS6 simply applies "git cl format" to PS5. No other changes in PS6.)
https://codereview.chromium.org/836303004/diff/80001/components/wifi_sync/wif... File components/wifi_sync/wifi_credential_syncable_service.cc (right): https://codereview.chromium.org/836303004/diff/80001/components/wifi_sync/wif... components/wifi_sync/wifi_credential_syncable_service.cc:31: const WifiCredential& credential, I went with WifiCredential here, instead of raw_credential_data, because I didn't see any benefit to converting from the more-validated type to the less-validated type.
https://codereview.chromium.org/836303004/diff/100001/components/wifi_sync/wi... File components/wifi_sync/wifi_credential_syncable_service.cc (right): https://codereview.chromium.org/836303004/diff/100001/components/wifi_sync/wi... components/wifi_sync/wifi_credential_syncable_service.cc:24: struct raw_credential_data { This should still be named in camel case unless there is some convention I'm unaware of. https://codereview.chromium.org/836303004/diff/100001/components/wifi_sync/wi... components/wifi_sync/wifi_credential_syncable_service.cc:47: struct raw_credential_data* raw_credential) { you don't need 'struct' here. https://codereview.chromium.org/836303004/diff/100001/components/wifi_sync/wi... components/wifi_sync/wifi_credential_syncable_service.cc:86: struct raw_credential_data* raw_credential) { - struct https://codereview.chromium.org/836303004/diff/100001/components/wifi_sync/wi... components/wifi_sync/wifi_credential_syncable_service.cc:157: for (const auto& sync_change : change_list) { I'm not sure what type this is. Is it particularly verbose? The auto keyword here makes it that much harder for me to understand the code (now I need to figure out where SyncChangeList is declared, to figure out what type it returns, etc.). https://codereview.chromium.org/836303004/diff/100001/components/wifi_sync/wi... components/wifi_sync/wifi_credential_syncable_service.cc:159: struct raw_credential_data raw_credential; -struct https://codereview.chromium.org/836303004/diff/100001/components/wifi_sync/wi... components/wifi_sync/wifi_credential_syncable_service.cc:200: LOG(WARNING) << "WifiCredentials syncable service is not started."; Is this standard (as opposed to queueing data for subsequent syncing? https://codereview.chromium.org/836303004/diff/100001/components/wifi_sync/wi... File components/wifi_sync/wifi_credential_syncable_service_unittest.cc (right): https://codereview.chromium.org/836303004/diff/100001/components/wifi_sync/wi... components/wifi_sync/wifi_credential_syncable_service_unittest.cc:32: // Fake implementation of WifiConfigDelegate, which provides the You can start your anon namespace here. https://codereview.chromium.org/836303004/diff/100001/components/wifi_sync/wi... components/wifi_sync/wifi_credential_syncable_service_unittest.cc:47: int GetAddCount() const { return add_count_; } accessor naming (add_count()) https://codereview.chromium.org/836303004/diff/100001/components/wifi_sync/wi... components/wifi_sync/wifi_credential_syncable_service_unittest.cc:51: int add_count_; unsigned int https://codereview.chromium.org/836303004/diff/100001/components/wifi_sync/wi... components/wifi_sync/wifi_credential_syncable_service_unittest.cc:63: new WifiCredentialSyncableService(make_scoped_ptr(config_delegate_))); This can appear in the initializer list. https://codereview.chromium.org/836303004/diff/100001/components/wifi_sync/wi... components/wifi_sync/wifi_credential_syncable_service_unittest.cc:79: int GetChangeProcessorChangeCount() { accessor naming https://codereview.chromium.org/836303004/diff/100001/components/wifi_sync/wi... components/wifi_sync/wifi_credential_syncable_service_unittest.cc:86: int GetWifiDelegateAddCount() { return config_delegate_->GetAddCount(); } ditto https://codereview.chromium.org/836303004/diff/100001/components/wifi_sync/wi... components/wifi_sync/wifi_credential_syncable_service_unittest.cc:153: } } // namespace https://codereview.chromium.org/836303004/diff/100001/components/wifi_sync/wi... components/wifi_sync/wifi_credential_syncable_service_unittest.cc:157: ProcessSyncChanges(FROM_HERE, syncer::SyncChangeList())); Unless you actually test something related to the location, why not just pull the "FROM_HERE" inside the helper method and simplify the signature? https://codereview.chromium.org/836303004/diff/100001/components/wifi_sync/wi... components/wifi_sync/wifi_credential_syncable_service_unittest.cc:172: EXPECT_FALSE(sync_error.IsSet()); // Just ignore bad items. // Bad items are ignored. The current comment seems to describe what the test does (it confused me). My suggestion more clearly describes what the code under test does. Do this below, too.
https://codereview.chromium.org/836303004/diff/100001/components/wifi_sync/wi... File components/wifi_sync/wifi_credential_syncable_service.cc (right): https://codereview.chromium.org/836303004/diff/100001/components/wifi_sync/wi... components/wifi_sync/wifi_credential_syncable_service.cc:24: struct raw_credential_data { On 2015/01/22 15:59:09, erikwright (OOO Jan 19) wrote: > This should still be named in camel case unless there is some convention I'm > unaware of. +1 https://codereview.chromium.org/836303004/diff/100001/components/wifi_sync/wi... components/wifi_sync/wifi_credential_syncable_service.cc:157: for (const auto& sync_change : change_list) { On 2015/01/22 15:59:09, erikwright (OOO Jan 19) wrote: > I'm not sure what type this is. Is it particularly verbose? > > The auto keyword here makes it that much harder for me to understand the code > (now I need to figure out where SyncChangeList is declared, to figure out what > type it returns, etc.). We use this pattern pretty widely now in for loops, especially with STL containers, which are usually typedefed. While I agree that we shouldn't overuse auto, this seems like a pretty straightforward use to me. SyncChangeList is pretty much what it is named, vector<SyncChange>. https://codereview.chromium.org/836303004/diff/100001/components/wifi_sync/wi... components/wifi_sync/wifi_credential_syncable_service.cc:200: LOG(WARNING) << "WifiCredentials syncable service is not started."; On 2015/01/22 15:59:09, erikwright (OOO Jan 19) wrote: > Is this standard (as opposed to queueing data for subsequent syncing? Generally when sync is started it would query for any new changes. In this case we can't because we don't store credentials in Chrome, but at the same time we don't really want to queue the credentials locally here either. This is pretty unlikely (but possible if sync is slow to start for some reason). I think it is OK not to handle that, but we should add a comment to that effect here.
erikwright, stevenjb: Thanks and PTAL. https://codereview.chromium.org/836303004/diff/100001/components/wifi_sync/wi... File components/wifi_sync/wifi_credential_syncable_service.cc (right): https://codereview.chromium.org/836303004/diff/100001/components/wifi_sync/wi... components/wifi_sync/wifi_credential_syncable_service.cc:24: struct raw_credential_data { On 2015/01/22 17:44:56, stevenjb wrote: > On 2015/01/22 15:59:09, erikwright (OOO Jan 19) wrote: > > This should still be named in camel case unless there is some convention I'm > > unaware of. > +1 Done. (Sorry -- when I think about structs, my old-school C habits kick in.) https://codereview.chromium.org/836303004/diff/100001/components/wifi_sync/wi... components/wifi_sync/wifi_credential_syncable_service.cc:47: struct raw_credential_data* raw_credential) { On 2015/01/22 15:59:09, erikwright wrote: > you don't need 'struct' here. Done. https://codereview.chromium.org/836303004/diff/100001/components/wifi_sync/wi... components/wifi_sync/wifi_credential_syncable_service.cc:86: struct raw_credential_data* raw_credential) { On 2015/01/22 15:59:09, erikwright wrote: > - struct Done. https://codereview.chromium.org/836303004/diff/100001/components/wifi_sync/wi... components/wifi_sync/wifi_credential_syncable_service.cc:157: for (const auto& sync_change : change_list) { On 2015/01/22 17:44:56, stevenjb wrote: > On 2015/01/22 15:59:09, erikwright (OOO Jan 19) wrote: > > I'm not sure what type this is. Is it particularly verbose? > > > > The auto keyword here makes it that much harder for me to understand the code > > (now I need to figure out where SyncChangeList is declared, to figure out what > > type it returns, etc.). > > We use this pattern pretty widely now in for loops, especially with STL > containers, which are usually typedefed. While I agree that we shouldn't overuse > auto, this seems like a pretty straightforward use to me. SyncChangeList is > pretty much what it is named, vector<SyncChange>. Thanks, Steven. I think that explains my thinking when wrote this with auto. That said, since it proved to be confusing, I've changed the type to be explicit. https://codereview.chromium.org/836303004/diff/100001/components/wifi_sync/wi... components/wifi_sync/wifi_credential_syncable_service.cc:159: struct raw_credential_data raw_credential; On 2015/01/22 15:59:09, erikwright wrote: > -struct Done. https://codereview.chromium.org/836303004/diff/100001/components/wifi_sync/wi... components/wifi_sync/wifi_credential_syncable_service.cc:200: LOG(WARNING) << "WifiCredentials syncable service is not started."; On 2015/01/22 17:44:56, stevenjb wrote: > On 2015/01/22 15:59:09, erikwright (OOO Jan 19) wrote: > > Is this standard (as opposed to queueing data for subsequent syncing? > > Generally when sync is started it would query for any new changes. In this case > we can't because we don't store credentials in Chrome, but at the same time we > don't really want to queue the credentials locally here either. This is pretty > unlikely (but possible if sync is slow to start for some reason). I think it is > OK not to handle that, but we should add a comment to that effect here. ==stevenjb. For most datatypes, the local data is managed by Chrome. So changes can effectively be queued by 1) updating the persistent copy of that data, and 2) consulting the persistent data to identify new items during MergeDataAndStartSyncing. The WifiCredential data type is odd, in that the local data is managed out of process, by a ChromeOS daemon (called "shill"). Furthermore, shill doesn't provide read access to the passwords. So we can't use the same approach to merge new items during MergeDataAndStartSyncing. That said, we could, as you suggest, queue the credentials inside the SyncableService. But I agree with Steven that we're better off not doing so. The reason I argue against a queue in this SyncableService is that the caller of AddToSyncedNetworks will need to cope with the possibility that the SyncableService doesn't even exist. So the calling code needs to have a queue anyway. Given that, it seems simpler to ask the caller to also use that queue when the SyncableService exists, but isn't ready. (Why might the SyncableService not exist? The SyncableService can't exist at the ChromeOS login screen, because there is no Google account authenticated. But we'd like a network that was configured at the login screen to get synced into the account of the first user who logs in. Hence the need for a queue in the calling code.) You might also be concerned about how the calling code knows when it is safe to call AddToSyncedNetworks. E.g., does it have to periodically retry its queue? The answer to that is in MergeDataAndStartSyncing method: "// TODO(quiche): Notify upper layers that sync is ready." So no, the calling code won't have to poll/retry to see when the SyncableService is ready. https://codereview.chromium.org/836303004/diff/100001/components/wifi_sync/wi... File components/wifi_sync/wifi_credential_syncable_service_unittest.cc (right): https://codereview.chromium.org/836303004/diff/100001/components/wifi_sync/wi... components/wifi_sync/wifi_credential_syncable_service_unittest.cc:32: // Fake implementation of WifiConfigDelegate, which provides the On 2015/01/22 15:59:10, erikwright wrote: > You can start your anon namespace here. Done. https://codereview.chromium.org/836303004/diff/100001/components/wifi_sync/wi... components/wifi_sync/wifi_credential_syncable_service_unittest.cc:47: int GetAddCount() const { return add_count_; } On 2015/01/22 15:59:10, erikwright wrote: > accessor naming (add_count()) Done. https://codereview.chromium.org/836303004/diff/100001/components/wifi_sync/wi... components/wifi_sync/wifi_credential_syncable_service_unittest.cc:51: int add_count_; On 2015/01/22 15:59:09, erikwright wrote: > unsigned int Done. https://codereview.chromium.org/836303004/diff/100001/components/wifi_sync/wi... components/wifi_sync/wifi_credential_syncable_service_unittest.cc:63: new WifiCredentialSyncableService(make_scoped_ptr(config_delegate_))); On 2015/01/22 15:59:10, erikwright wrote: > This can appear in the initializer list. In order to place this in the initializer list, I'll need to change the declaration order. Currently, I have: scoped_ptr<WifiCredentialSyncableService> syncable_service_; FakeWifiConfigDelegate* config_delegate_; // Owned by |syncable_service_| FakeSyncChangeProcessor* change_processor_; // Owned by |syncable_service_| With this declaration order, placing syncable_service_ in the initializer list would mean that I read from config_delegate_ before config_delegate_ has been initialized. I think the current declaration order is preferable, as it introduces |syncable_service_| before the comments that reference it. If you disagree, let me know, and I'll change it. https://codereview.chromium.org/836303004/diff/100001/components/wifi_sync/wi... components/wifi_sync/wifi_credential_syncable_service_unittest.cc:79: int GetChangeProcessorChangeCount() { On 2015/01/22 15:59:10, erikwright wrote: > accessor naming Done. (I couldn't find any guidance, in the style guide, for naming an accessor which follows references. So I simply concatenated the names.) https://codereview.chromium.org/836303004/diff/100001/components/wifi_sync/wi... components/wifi_sync/wifi_credential_syncable_service_unittest.cc:86: int GetWifiDelegateAddCount() { return config_delegate_->GetAddCount(); } On 2015/01/22 15:59:10, erikwright wrote: > ditto Done. https://codereview.chromium.org/836303004/diff/100001/components/wifi_sync/wi... components/wifi_sync/wifi_credential_syncable_service_unittest.cc:153: } On 2015/01/22 15:59:09, erikwright wrote: > } // namespace Done. https://codereview.chromium.org/836303004/diff/100001/components/wifi_sync/wi... components/wifi_sync/wifi_credential_syncable_service_unittest.cc:157: ProcessSyncChanges(FROM_HERE, syncer::SyncChangeList())); On 2015/01/22 15:59:09, erikwright wrote: > Unless you actually test something related to the location, why not just pull > the "FROM_HERE" inside the helper method and simplify the signature? Done. (Made the same change for MakeActionAdd.) https://codereview.chromium.org/836303004/diff/100001/components/wifi_sync/wi... components/wifi_sync/wifi_credential_syncable_service_unittest.cc:172: EXPECT_FALSE(sync_error.IsSet()); // Just ignore bad items. On 2015/01/22 15:59:10, erikwright wrote: > // Bad items are ignored. > > The current comment seems to describe what the test does (it confused me). My > suggestion more clearly describes what the code under test does. > > Do this below, too. Done. https://codereview.chromium.org/836303004/diff/120001/components/wifi_sync/wi... File components/wifi_sync/wifi_credential_syncable_service.h (right): https://codereview.chromium.org/836303004/diff/120001/components/wifi_sync/wi... components/wifi_sync/wifi_credential_syncable_service.h:55: // is an error to add a network that already exists. It is also an I meant for this change to be included in PS5. Sorry about losing track of it.
lgtm https://codereview.chromium.org/836303004/diff/100001/components/wifi_sync/wi... File components/wifi_sync/wifi_credential_syncable_service.cc (right): https://codereview.chromium.org/836303004/diff/100001/components/wifi_sync/wi... components/wifi_sync/wifi_credential_syncable_service.cc:157: for (const auto& sync_change : change_list) { On 2015/01/22 23:36:02, mukesh agrawal wrote: > On 2015/01/22 17:44:56, stevenjb wrote: > > On 2015/01/22 15:59:09, erikwright (OOO Jan 19) wrote: > > > I'm not sure what type this is. Is it particularly verbose? > > > > > > The auto keyword here makes it that much harder for me to understand the > code > > > (now I need to figure out where SyncChangeList is declared, to figure out > what > > > type it returns, etc.). > > > > We use this pattern pretty widely now in for loops, especially with STL > > containers, which are usually typedefed. While I agree that we shouldn't > overuse > > auto, this seems like a pretty straightforward use to me. SyncChangeList is > > pretty much what it is named, vector<SyncChange>. > > Thanks, Steven. I think that explains my thinking when wrote this with auto. > That said, since it proved to be confusing, I've changed the type to be > explicit. Huh, I didn't realize you could use for ( : ) without auto; good to know, I do think this is a bit more clear.
LGTM.
The CQ bit was checked by quiche@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/836303004/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by quiche@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/836303004/140001
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/c96b5d5783a331aeea09e91e87f2f3678909c37f Cr-Commit-Position: refs/heads/master@{#313109} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
