Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1256803002:
[Smart Lock, Prefs reconciliation] Prefs migration logic for desktop platforms. (Closed)
|
Created:
5 years, 5 months ago by melandory Modified:
5 years, 2 months ago Reviewers:
pval...(no longer on Chromium), Mike Lerman, engedy, vabr (Chromium), Nicolas Zea, battre CC:
chromium-reviews, gcasto+watchlist_chromium.org, vabr+watchlist_chromium.org, mkwst+watchlist-passwords_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Smart Lock, Prefs reconciliation] Prefs migration logic for desktop platforms.
Currently password manager in chrome is controlled by preference profile.password_manager_enabled. But Smart Lock behavior on Android is controlled by priority preference credentials_enable_service, which is also surfaced at passwords.google.com. So if users is a Smart Lock user and turn feature on Android (so passwords will not be saved and user will not be autosigned in), then it will not take any effect on the Chrome password manager.
This behavior should be changed and Smart Lock users should be able to control Smart Lock behavior on Android and Chrome password manager behavior with one setting.
This change implements the platforms.
BUG=517087
Committed: https://crrev.com/fac58ec68f43cb21f1dc6a18e8878e03a63797fc
Cr-Commit-Position: refs/heads/master@{#350162}
Patch Set 1 : #Patch Set 2 : More tests #Patch Set 3 : Cleanups #Patch Set 4 : More tests #Patch Set 5 : Fix mac #
Total comments: 48
Patch Set 6 : Refactor tests and comments for tests #Patch Set 7 : Change comments style in tests. #Patch Set 8 : Remove init from chrome_browser_main #Patch Set 9 : :%s/PasswordSettingsMigrationService/PasswordManagerSettingMigraterService/g #Patch Set 10 : #
Total comments: 24
Patch Set 11 : Basic two clients sync tests and PasswordSettingsMigrationService and PasswordSettingsMigrationServ… #Patch Set 12 : More sync tests #Patch Set 13 : #
Total comments: 26
Patch Set 14 : More two client tets #
Total comments: 2
Patch Set 15 : WIP #Patch Set 16 : WIP #Patch Set 17 : More tests #Patch Set 18 : More tetst #Patch Set 19 : WIP. New logic. #Patch Set 20 : More structure to the tests #Patch Set 21 : Cover table with a tests. #Patch Set 22 : Sort tests. #
Total comments: 10
Patch Set 23 : #
Total comments: 8
Patch Set 24 : Follow one variable per declaration rule. #
Total comments: 23
Patch Set 25 : #
Total comments: 34
Patch Set 26 : #
Total comments: 10
Patch Set 27 : #Patch Set 28 : Add guard #
Total comments: 3
Patch Set 29 : #
Total comments: 4
Patch Set 30 : git grep -l 'MigrationFieldTrial' | xargs sed -i 's/MigrationFieldTrial/GetMigrationFieldTrial/g' #Patch Set 31 : git diff HEAD^.. --name-only | grep migrater | while read file ; do ./tools/git/move_source_file.py… #Patch Set 32 : git grep -l 'PasswordManagerSettingMigraterService' | xargs sed -i 's/PasswordManagerSettingMigrate… #
Total comments: 45
Patch Set 33 : #Patch Set 34 : #
Total comments: 21
Patch Set 35 : #Patch Set 36 : #Patch Set 37 : #
Total comments: 67
Patch Set 38 : #Patch Set 39 : For review. #Patch Set 40 : WIP. #Patch Set 41 : #
Total comments: 127
Patch Set 42 : #Messages
Total messages: 131 (57 generated)
Patchset #1 (id:1) has been deleted
Patchset #3 (id:60001) has been deleted
melandory@chromium.org changed reviewers: + engedy@chromium.org
Hi Balazs, can you look at the CL, please. Thanks in advance. P.S. Ignore failed build on mac, I'll rewrite code with cased it.
Patchset #5 (id:120001) has been deleted
melandory@chromium.org changed reviewers: + vabr@chromium.org
Hi Vaclav, review this CL, please. You own every file except: chrome/browser/chrome_browser_main.cc
On 2015/08/06 06:49:06, melandory wrote: > Hi Vaclav, > > review this CL, please. > > You own every file except: > chrome/browser/chrome_browser_main.cc (Just a status update, I'm looking at the CL, but it may take longer both because of me investigating how to eliminate the //chrome dependencies, and being a sheriff. Will get back to you later today.) Cheers, Vaclav
On 2015/08/06 07:52:42, vabr (Chromium) wrote: > On 2015/08/06 06:49:06, melandory wrote: > > Hi Vaclav, > > > > review this CL, please. > > > > You own every file except: > > chrome/browser/chrome_browser_main.cc > > (Just a status update, I'm looking at the CL, but it may take longer both > because of me investigating how to eliminate the //chrome dependencies, and > being a sheriff. Will get back to you later today.) > > Cheers, > Vaclav Terribly sorry, Tanja, sheriffing was busy today. I started reading the design doc, and will continue with the review tomorrow. Cheers, Vaclav
Because Balázs is having a look as well, I'll post my comments from yesterday. Some might get out of date as soon as I manage to read more of the code and design doc, sorry about that. I'm reviewing further, will notify when I'm done. Cheers, Vaclav https://codereview.chromium.org/1256803002/diff/160001/chrome/browser/chrome_... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/1256803002/diff/160001/chrome/browser/chrome_... chrome/browser/chrome_browser_main.cc:1566: password_manager::PasswordSettingsMigrationService::Factory::GetForProfile( nit: Add a blank line above this line, otherwise it looks like it is part of recording the initial prefs. https://codereview.chromium.org/1256803002/diff/160001/chrome/browser/passwor... File chrome/browser/password_manager/password_manager_migration_service.h (right): https://codereview.chromium.org/1256803002/diff/160001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_migration_service.h:20: // PasswordSettingsMigrationService is responsible for reconcile Chrome password nit: reconciling https://codereview.chromium.org/1256803002/diff/160001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_migration_service.h:38: friend class PasswordSettingsMigrationServiceTest; so far non-actionable note: Friending for tests is rarely the right thing to do. Tests should test the external API of classes, not the implementation, which may be subject to change. There are exceptions, but I have not had chance to have a proper look yet. Feel free to ignore this comment for now, I may post something more actionable soon. https://codereview.chromium.org/1256803002/diff/160001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_migration_service.h:61: // Called on event of changing either PasswordManagerSavingEnabled or nit: blank line above https://codereview.chromium.org/1256803002/diff/160001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_migration_service.h:62: // CredentialEnableService. Change the value of another preference to be in What is CredentialEnableService? https://codereview.chromium.org/1256803002/diff/160001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_migration_service.h:69: void Reconcile(PrefService* prefs); The name does not seem to match the comment. Reconcile sounds more generic than just "turn off when turned off". If it does what the comment says, it should be called accordingly, otherwise the comment should be updated. https://codereview.chromium.org/1256803002/diff/160001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_migration_service.h:73: ProfileSyncService* profile_sync_service_; Please use the parent interface from components/sync_driver/sync_service.h instead. No need to introduce //chrome dependencies. https://codereview.chromium.org/1256803002/diff/160001/components/password_ma... File components/password_manager/core/common/password_manager_pref_names.cc (right): https://codereview.chromium.org/1256803002/diff/160001/components/password_ma... components/password_manager/core/common/password_manager_pref_names.cc:15: const char kCredentialEnableService[] = "credentials_enable_service"; nit: The constant name and value differ (credential vs. credentials). Not a big deal, but usually they are the same. Also, it sounds strange as an English phrase, and the role of "service" is not clear. What about "save_and_fill_credentials"? https://codereview.chromium.org/1256803002/diff/160001/components/password_ma... File components/password_manager/core/common/password_manager_pref_names.h (right): https://codereview.chromium.org/1256803002/diff/160001/components/password_ma... components/password_manager/core/common/password_manager_pref_names.h:24: // The value of this preference controls whenever Password Manager will save and Please explain (in the comment) the relation to kPasswordManagerSavingEnabled. https://codereview.chromium.org/1256803002/diff/160001/components/password_ma... components/password_manager/core/common/password_manager_pref_names.h:25: // fill credentials. Is the part about filling actually true? Currently, there is no way to disable filling other than deleting the credentials, and this CL does not seem to introduce this functionality.
https://codereview.chromium.org/1256803002/diff/160001/components/password_ma... File components/password_manager/core/common/password_manager_pref_names.cc (right): https://codereview.chromium.org/1256803002/diff/160001/components/password_ma... components/password_manager/core/common/password_manager_pref_names.cc:15: const char kCredentialEnableService[] = "credentials_enable_service"; Just to add to the 2nd part of my original comment: > Also, it sounds strange as an English phrase, and the role of "service" is not > clear. What about "save_and_fill_credentials"? I see that this comes from the existing preference on Android. In that case, just keep it, sorry for the noise. (My comment about matching the const name with value still holds, though.)
Looking good overall with mostly minor comments. There are a few edge cases and details I'd like to discuss a bit more given the importance of getting this setting migration right. https://codereview.chromium.org/1256803002/diff/160001/chrome/browser/chrome_... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/1256803002/diff/160001/chrome/browser/chrome_... chrome/browser/chrome_browser_main.cc:1566: password_manager::PasswordSettingsMigrationService::Factory::GetForProfile( It might be just that I have already forgotten why it was impossible, but now it seems to be that it should be possible to rely on ServiceIsCreatedWithBrowserContext() and remove this line. That way, we would add a reference to the factory into ChromeBrowserMainExtraPartsProfiles:: EnsureBrowserContextKeyedServiceFactoriesBuilt(), and given the DependsOn() directives, the settings migrater service would be magically created after the ProfileSyncService. I am not entirely sure, however, which scenarios there are when we do not have a ProfileSyncService, and what the DependsOn directive will result in in those cases. Could you please explain this to me? https://codereview.chromium.org/1256803002/diff/160001/chrome/browser/passwor... File chrome/browser/password_manager/password_manager_migration_service.cc (right): https://codereview.chromium.org/1256803002/diff/160001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_migration_service.cc:47: base::Unretained(this), prefs::kCredentialEnableService)); nit: Include bind_helpers.h for base::Unretained. https://codereview.chromium.org/1256803002/diff/160001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_migration_service.cc:104: DependsOn(TemplateURLServiceFactory::GetInstance()); I think we don't need this dependency. https://codereview.chromium.org/1256803002/diff/160001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_migration_service.cc:122: return false; You may omit the override as this is the default behavior. https://codereview.chromium.org/1256803002/diff/160001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_migration_service.cc:126: PasswordSettingsMigrationService::Factory::GetBrowserContextToUse( You may omit this override as well. I think it's fine not to have this service on an Incognito profile at all, which is the default behavior. On second though, I think I would actually prefer that solution to redirecting. https://codereview.chromium.org/1256803002/diff/160001/chrome/browser/passwor... File chrome/browser/password_manager/password_manager_migration_service.h (right): https://codereview.chromium.org/1256803002/diff/160001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_migration_service.h:22: class PasswordSettingsMigrationService : public KeyedService, I'd also suggest to slightly change the class name to something like PasswordManagerSettingMigraterService, so as to better capture that this is for PM (not for "passwords" in general) and that we are currently only migrating a single setting. (I am open to better name suggestion, too. :)) https://codereview.chromium.org/1256803002/diff/160001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_migration_service.h:31: class Factory : public BrowserContextKeyedServiceFactory { nit: Declare nested types first within a section. See: https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_... https://codereview.chromium.org/1256803002/diff/160001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_migration_service.h:43: // BrowserContextKeyedServiceFactory implementation optional nit: Some people prefer writing just: // BrowserContextKeyedServiceFactory: It's up to you -- just make sure you are consistent with the other section below. https://codereview.chromium.org/1256803002/diff/160001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_migration_service.h:60: ProfileSyncService* profile_sync_service); You could promote this to be the main constructor, and inject the dependencies in the Facgory::BuildServiceInstanceFor, that way, you would not need special casing for tests. https://codereview.chromium.org/1256803002/diff/160001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_migration_service.h:62: // CredentialEnableService. Change the value of another preference to be in On 2015/08/07 08:48:02, vabr (Chromium) wrote: > What is CredentialEnableService? It's the preference name. But, yes, we would probably want to keep the |unix_hacker_style| name for it. https://codereview.chromium.org/1256803002/diff/160001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_migration_service.h:64: void OnPrefChanged(const std::string& other_pref_name, nit: I think we could have two separate methods, OnLegacyPrefChanged, OnNewPrefChanged. In my opinion, that could be more readable. https://codereview.chromium.org/1256803002/diff/160001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_migration_service.h:69: void Reconcile(PrefService* prefs); On 2015/08/07 08:48:02, vabr (Chromium) wrote: > The name does not seem to match the comment. Reconcile sounds more generic than > just "turn off when turned off". If it does what the comment says, it should be > called accordingly, otherwise the comment should be updated. Apologies, I am afraid it was me who originally came up with the name. On second thought, it indeed wasn't the best choice. One naming idea is to call it: "MigrateLegacyOffState". WDYT? https://codereview.chromium.org/1256803002/diff/160001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_migration_service.h:76: base::WeakPtrFactory<PasswordSettingsMigrationService> weak_factory_; nit: Do we actually use the |weak_factory_|? https://codereview.chromium.org/1256803002/diff/160001/components/password_ma... File components/password_manager/core/common/password_manager_pref_names.cc (right): https://codereview.chromium.org/1256803002/diff/160001/components/password_ma... components/password_manager/core/common/password_manager_pref_names.cc:15: const char kCredentialEnableService[] = "credentials_enable_service"; On 2015/08/07 09:26:21, vabr (Chromium) wrote: > Just to add to the 2nd part of my original comment: > > > Also, it sounds strange as an English phrase, and the role of "service" is not > > clear. What about "save_and_fill_credentials"? > > I see that this comes from the existing preference on Android. In that case, > just keep it, sorry for the noise. > > (My comment about matching the const name with value still holds, though.) We could consider doing a mapping in PrefModelAssociator to a more reasonable name that is in the "password_manager.*" namespace, but that's quite obscure, and I am not sure how well it would be received by the Sync owners. If we are not going to do that, we should point out in the comment that the name is as unfortunate as it is because of compatibility reasons with Smart Lock on Android. https://codereview.chromium.org/1256803002/diff/160001/components/password_ma... File components/password_manager/core/common/password_manager_pref_names.h (right): https://codereview.chromium.org/1256803002/diff/160001/components/password_ma... components/password_manager/core/common/password_manager_pref_names.h:25: // fill credentials. On 2015/08/07 08:48:02, vabr (Chromium) wrote: > Is the part about filling actually true? Currently, there is no way to disable > filling other than deleting the credentials, and this CL does not seem to > introduce this functionality. Well, technically, the setting will not control anything as of this CL, that will come later. I think we should add a TODO that we still need to wire in this preference and change the behavior so that it controls both saving and filling. https://codereview.chromium.org/1256803002/diff/160001/components/password_ma... components/password_manager/core/common/password_manager_pref_names.h:26: extern const char kCredentialEnableService[]; I'd actually dare propose that we make use of the extra level of indirection here introduced by having a constant, and give the constant an actually meaningful name, such as |kPasswordManagerEnabledNew|. Then, once it is wired in, we could rename it to |kPasswordManagerEnabled| and change the existing setting to |kPasswordManagerSavingEnabledLegacy|. WDYT?
Some quick responses to @engedy's comments. Vaclav https://codereview.chromium.org/1256803002/diff/160001/chrome/browser/passwor... File chrome/browser/password_manager/password_manager_migration_service.h (right): https://codereview.chromium.org/1256803002/diff/160001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_migration_service.h:69: void Reconcile(PrefService* prefs); On 2015/08/07 11:41:38, engedy wrote: > On 2015/08/07 08:48:02, vabr (Chromium) wrote: > > The name does not seem to match the comment. Reconcile sounds more generic > than > > just "turn off when turned off". If it does what the comment says, it should > be > > called accordingly, otherwise the comment should be updated. > > Apologies, I am afraid it was me who originally came up with the name. On second > thought, it indeed wasn't the best choice. > > One naming idea is to call it: "MigrateLegacyOffState". WDYT? SGTM! https://codereview.chromium.org/1256803002/diff/160001/components/password_ma... File components/password_manager/core/common/password_manager_pref_names.cc (right): https://codereview.chromium.org/1256803002/diff/160001/components/password_ma... components/password_manager/core/common/password_manager_pref_names.cc:15: const char kCredentialEnableService[] = "credentials_enable_service"; On 2015/08/07 11:41:38, engedy wrote: > On 2015/08/07 09:26:21, vabr (Chromium) wrote: > > Just to add to the 2nd part of my original comment: > > > > > Also, it sounds strange as an English phrase, and the role of "service" is > not > > > clear. What about "save_and_fill_credentials"? > > > > I see that this comes from the existing preference on Android. In that case, > > just keep it, sorry for the noise. > > > > (My comment about matching the const name with value still holds, though.) > > We could consider doing a mapping in PrefModelAssociator to a more reasonable > name that is in the "password_manager.*" namespace, but that's quite obscure, > and I am not sure how well it would be received by the Sync owners. Please do not undergo such pain just for the name. Being consistent is also probably better than being an understandable English sentence in this case. > > If we are not going to do that, we should point out in the comment that the name > is as unfortunate as it is because of compatibility reasons with Smart Lock on > Android. +1 https://codereview.chromium.org/1256803002/diff/160001/components/password_ma... File components/password_manager/core/common/password_manager_pref_names.h (right): https://codereview.chromium.org/1256803002/diff/160001/components/password_ma... components/password_manager/core/common/password_manager_pref_names.h:25: // fill credentials. On 2015/08/07 11:41:38, engedy wrote: > On 2015/08/07 08:48:02, vabr (Chromium) wrote: > > Is the part about filling actually true? Currently, there is no way to disable > > filling other than deleting the credentials, and this CL does not seem to > > introduce this functionality. > > Well, technically, the setting will not control anything as of this CL, that > will come later. > > I think we should add a TODO that we still need to wire in this preference and > change the behavior so that it controls both saving and filling. +1 https://codereview.chromium.org/1256803002/diff/160001/components/password_ma... components/password_manager/core/common/password_manager_pref_names.h:26: extern const char kCredentialEnableService[]; On 2015/08/07 11:41:38, engedy wrote: > I'd actually dare propose that we make use of the extra level of indirection > here introduced by having a constant, and give the constant an actually > meaningful name, such as |kPasswordManagerEnabledNew|. Then, once it is wired > in, we could rename it to |kPasswordManagerEnabled| and change the existing > setting to |kPasswordManagerSavingEnabledLegacy|. > > WDYT? This sounds good, but also the current state (with the typo fixed) works for me. One drawback of the suggested name switching (in addition to the once more CL to do the switch) is that |kPasswordManagerEnabled| will mean two different things in code of different age, which might be confusing when investigating problems on a branch etc. So my weak preference is actually to let the constant name match its content.
Patchset #9 (id:220001) has been deleted
https://codereview.chromium.org/1256803002/diff/160001/chrome/browser/chrome_... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/1256803002/diff/160001/chrome/browser/chrome_... chrome/browser/chrome_browser_main.cc:1566: password_manager::PasswordSettingsMigrationService::Factory::GetForProfile( On 2015/08/07 11:41:38, engedy wrote: > It might be just that I have already forgotten why it was impossible, but now it > seems to be that it should be possible to rely on > ServiceIsCreatedWithBrowserContext() and remove this line. > > That way, we would add a reference to the factory into > ChromeBrowserMainExtraPartsProfiles:: > EnsureBrowserContextKeyedServiceFactoriesBuilt(), and given the DependsOn() > directives, the settings migrater service would be magically created after the > ProfileSyncService. > > I am not entirely sure, however, which scenarios there are when we do not have a > ProfileSyncService, and what the DependsOn directive will result in in those > cases. Could you please explain this to me? Before my concern was more emperical, I was getting ProfileSyncServiceFactory::HasProfileSyncService(profile_) false on the time ofPasswordSettingsMigrationService construction. I investigated a bit more, it looks like I'm hitting https://code.google.com/p/chromium/issues/detail?id=171406 I'll check what's can be done here.
Patchset #9 (id:240001) has been deleted
As discussed offline, relying on PROFILE_ADDED seems like the best workaround for now. Let me know when I can take another look at the CL.
Patchset #11 (id:300001) has been deleted
On 2015/08/10 10:51:25, engedy wrote: > As discussed offline, relying on PROFILE_ADDED seems like the best workaround > for now. Let me know when I can take another look at the CL. Hi Vaclav and Balazs, can you take a look? Thanks!
Hi Tanja, My next batch of comments follows. Some of them only make sense if the check for the profile being added is moved to the factory. Also, I did not look at the tests yet, as I think we need to make sure we all agree on the production code design first. One thing I'll check when looking at the tests will be if we could get rid of friending the production code. Cheers, Vaclav https://codereview.chromium.org/1256803002/diff/320001/chrome/browser/passwor... File chrome/browser/password_manager/password_manager_setting_migrater_service.cc (right): https://codereview.chromium.org/1256803002/diff/320001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service.cc:24: PrefServiceSyncable* prefs = PrefServiceSyncable::FromProfile(profile); While PrefServiceSyncable cannot be removed from this code completely, there is still value in not letting the //chrome dependencies take too deep roots in the code. In particular here, instead of passing Profile as an argument, and requesting PrefServiceSyncable, I suggest passing sync_driver::SyncService* as the argument instead of Profile. The SyncService interface should be enough, and while you still have to get the same object from the profile you get now, it will happen lower in the call stack, and spread //chrome dependencies over less code. https://codereview.chromium.org/1256803002/diff/320001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service.cc:52: void PasswordManagerSettingMigraterService::Observe( Would it make sense if instead of the service observing this notification, the factory would check ProfileManager::IsValidProfile in GetForProfile, and return nullptr if the profile is not valid yet? This would have two advantages: (1) You could have the single constructor for the service, and guaranteed that |sync_service| won't change value until your service's shutdown. (2) You could drop the code for observing the profile notification. https://codereview.chromium.org/1256803002/diff/320001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service.cc:56: DCHECK(type == chrome::NOTIFICATION_PROFILE_ADDED); nit: DCHECK_EQ(chrome::NOTIFICATION_PROFILE_ADDED, type); https://codereview.chromium.org/1256803002/diff/320001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service.cc:80: PasswordManagerSettingMigraterService::MigrateLegacyOffState(prefs); Why do we try to set the YOLO preference, when there is no sync service (or it cannot start)? https://codereview.chromium.org/1256803002/diff/320001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service.cc:80: PasswordManagerSettingMigraterService::MigrateLegacyOffState(prefs); nit: Can you drop PasswordManagerSettingMigraterService:: ? https://codereview.chromium.org/1256803002/diff/320001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service.cc:109: void PasswordManagerSettingMigraterService::MigrateLegacyOffState( Should we ensure that this is only called at most once? Also, should we make sure that this is not called after Shutdown() has been called on your service? https://codereview.chromium.org/1256803002/diff/320001/chrome/browser/passwor... File chrome/browser/password_manager/password_manager_setting_migrater_service.h (right): https://codereview.chromium.org/1256803002/diff/320001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service.h:22: namespace password_manager { nit: By convention, code in //chrome mostly does not use namespaces, and password manager code should be in its namespace only in the component. To keep consistency, I suggest not creating the new code in //chrome in namespace password_manager. Feel free to use "using" declarations in the .cc files to avoid lengthy expressions when referencing code from the component. https://codereview.chromium.org/1256803002/diff/320001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service.h:70: PasswordManagerSettingMigraterService(Profile* profile, As engedy@ noted before -- why not make this the only constructor, and inject the production sync service through it as well? Any test-specific changes in production are usually a bad sign, forking production code for test means that the less of the production code gets tested. https://codereview.chromium.org/1256803002/diff/320001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service.h:89: Profile* profile_; The member pointers which are not touched elsewhere than during construction should be marked const: Profile* const profile_; sync_driver::SyncService* const sync_service_; https://codereview.chromium.org/1256803002/diff/320001/components/password_ma... File components/password_manager/core/common/password_manager_pref_names.h (right): https://codereview.chromium.org/1256803002/diff/320001/components/password_ma... components/password_manager/core/common/password_manager_pref_names.h:30: // credentials. Which credentials? Do you mean Credentials API?
Somehow managed not to send my comments. https://codereview.chromium.org/1256803002/diff/160001/chrome/browser/chrome_... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/1256803002/diff/160001/chrome/browser/chrome_... chrome/browser/chrome_browser_main.cc:1566: password_manager::PasswordSettingsMigrationService::Factory::GetForProfile( On 2015/08/07 15:05:18, melandory wrote: > On 2015/08/07 11:41:38, engedy wrote: > > It might be just that I have already forgotten why it was impossible, but now > it > > seems to be that it should be possible to rely on > > ServiceIsCreatedWithBrowserContext() and remove this line. > > > > That way, we would add a reference to the factory into > > ChromeBrowserMainExtraPartsProfiles:: > > EnsureBrowserContextKeyedServiceFactoriesBuilt(), and given the DependsOn() > > directives, the settings migrater service would be magically created after the > > ProfileSyncService. > > > > I am not entirely sure, however, which scenarios there are when we do not have > a > > ProfileSyncService, and what the DependsOn directive will result in in those > > cases. Could you please explain this to me? > Before my concern was more emperical, I was getting > ProfileSyncServiceFactory::HasProfileSyncService(profile_) false on the time > ofPasswordSettingsMigrationService construction. > > I investigated a bit more, it looks like I'm hitting > https://code.google.com/p/chromium/issues/detail?id=171406 > > I'll check what's can be done here. Done. https://codereview.chromium.org/1256803002/diff/160001/chrome/browser/passwor... File chrome/browser/password_manager/password_manager_migration_service.cc (right): https://codereview.chromium.org/1256803002/diff/160001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_migration_service.cc:47: base::Unretained(this), prefs::kCredentialEnableService)); On 2015/08/07 11:41:38, engedy wrote: > nit: Include bind_helpers.h for base::Unretained. Done. https://codereview.chromium.org/1256803002/diff/160001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_migration_service.cc:104: DependsOn(TemplateURLServiceFactory::GetInstance()); On 2015/08/07 11:41:38, engedy wrote: > I think we don't need this dependency. Done. https://codereview.chromium.org/1256803002/diff/160001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_migration_service.cc:122: return false; On 2015/08/07 11:41:38, engedy wrote: > You may omit the override as this is the default behavior. Done. https://codereview.chromium.org/1256803002/diff/160001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_migration_service.cc:126: PasswordSettingsMigrationService::Factory::GetBrowserContextToUse( On 2015/08/07 11:41:38, engedy wrote: > You may omit this override as well. I think it's fine not to have this service > on an Incognito profile at all, which is the default behavior. > > On second though, I think I would actually prefer that solution to redirecting. Done. https://codereview.chromium.org/1256803002/diff/160001/chrome/browser/passwor... File chrome/browser/password_manager/password_manager_migration_service.h (right): https://codereview.chromium.org/1256803002/diff/160001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_migration_service.h:20: // PasswordSettingsMigrationService is responsible for reconcile Chrome password On 2015/08/07 08:48:02, vabr (Chromium) wrote: > nit: reconciling Done. https://codereview.chromium.org/1256803002/diff/160001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_migration_service.h:22: class PasswordSettingsMigrationService : public KeyedService, On 2015/08/07 11:41:38, engedy wrote: > I'd also suggest to slightly change the class name to something like > PasswordManagerSettingMigraterService, so as to better capture that this is for > PM (not for "passwords" in general) and that we are currently only migrating a > single setting. > > (I am open to better name suggestion, too. :)) Done. https://codereview.chromium.org/1256803002/diff/160001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_migration_service.h:31: class Factory : public BrowserContextKeyedServiceFactory { On 2015/08/07 11:41:38, engedy wrote: > nit: Declare nested types first within a section. See: > https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_... Done. https://codereview.chromium.org/1256803002/diff/160001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_migration_service.h:43: // BrowserContextKeyedServiceFactory implementation On 2015/08/07 11:41:38, engedy wrote: > optional nit: Some people prefer writing just: > > // BrowserContextKeyedServiceFactory: > > It's up to you -- just make sure you are consistent with the other section > below. Done. https://codereview.chromium.org/1256803002/diff/160001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_migration_service.h:60: ProfileSyncService* profile_sync_service); On 2015/08/07 11:41:38, engedy wrote: > You could promote this to be the main constructor, and inject the dependencies > in the Facgory::BuildServiceInstanceFor, that way, you would not need special > casing for tests. Unfortunately, can't do this anymore because of ProfileSyncService not available by the time Facgory::BuildServiceInstanceFor called. What I can do is: pass ProfileSyncService* as argument of InitObservers. This way I'll also be able to eliminate constructor, which is used for unittests only. WDYT? https://codereview.chromium.org/1256803002/diff/160001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_migration_service.h:61: // Called on event of changing either PasswordManagerSavingEnabled or On 2015/08/07 08:48:02, vabr (Chromium) wrote: > nit: blank line above Done. https://codereview.chromium.org/1256803002/diff/160001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_migration_service.h:64: void OnPrefChanged(const std::string& other_pref_name, On 2015/08/07 11:41:38, engedy wrote: > nit: I think we could have two separate methods, OnLegacyPrefChanged, > OnNewPrefChanged. In my opinion, that could be more readable. Done with correction that I changed New and Legacy with prefs constant names. Or you think New and Legacy is better? https://codereview.chromium.org/1256803002/diff/160001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_migration_service.h:69: void Reconcile(PrefService* prefs); On 2015/08/07 11:56:19, vabr (Chromium) wrote: > On 2015/08/07 11:41:38, engedy wrote: > > On 2015/08/07 08:48:02, vabr (Chromium) wrote: > > > The name does not seem to match the comment. Reconcile sounds more generic > > than > > > just "turn off when turned off". If it does what the comment says, it should > > be > > > called accordingly, otherwise the comment should be updated. > > > > Apologies, I am afraid it was me who originally came up with the name. On > second > > thought, it indeed wasn't the best choice. > > > > One naming idea is to call it: "MigrateLegacyOffState". WDYT? > > SGTM! Done. https://codereview.chromium.org/1256803002/diff/160001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_migration_service.h:76: base::WeakPtrFactory<PasswordSettingsMigrationService> weak_factory_; On 2015/08/07 11:41:38, engedy wrote: > nit: Do we actually use the |weak_factory_|? Done. https://codereview.chromium.org/1256803002/diff/160001/components/password_ma... File components/password_manager/core/common/password_manager_pref_names.h (right): https://codereview.chromium.org/1256803002/diff/160001/components/password_ma... components/password_manager/core/common/password_manager_pref_names.h:24: // The value of this preference controls whenever Password Manager will save and On 2015/08/07 08:48:02, vabr (Chromium) wrote: > Please explain (in the comment) the relation to kPasswordManagerSavingEnabled. Done. https://codereview.chromium.org/1256803002/diff/160001/components/password_ma... components/password_manager/core/common/password_manager_pref_names.h:26: extern const char kCredentialEnableService[]; On 2015/08/07 11:56:19, vabr (Chromium) wrote: > On 2015/08/07 11:41:38, engedy wrote: > > I'd actually dare propose that we make use of the extra level of indirection > > here introduced by having a constant, and give the constant an actually > > meaningful name, such as |kPasswordManagerEnabledNew|. Then, once it is wired > > in, we could rename it to |kPasswordManagerEnabled| and change the existing > > setting to |kPasswordManagerSavingEnabledLegacy|. > > > > WDYT? > > This sounds good, but also the current state (with the typo fixed) works for me. > One drawback of the suggested name switching (in addition to the once more CL to > do the switch) is that |kPasswordManagerEnabled| will mean two different things > in code of different age, which might be confusing when investigating problems > on a branch etc. > So my weak preference is actually to let the constant name match its content. It's also seems to me that it's better to let the constant name match its content.
Thanks for the off-line discussion! With the one comment below, I now agree with the current design of the production code. Thanks! As for the tests, I understood, that some changes are planned, so I'll have a look when you notify me they are ready. Cheers, Vaclav https://codereview.chromium.org/1256803002/diff/320001/chrome/browser/passwor... File chrome/browser/password_manager/password_manager_setting_migrater_service.cc (right): https://codereview.chromium.org/1256803002/diff/320001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service.cc:104: prefs->RemoveObserver(this); We should make sure that this is called also during Shutdown, if needed.
Patchset #13 (id:360001) has been deleted
Addressed comments and add basic two client tests. Currently, trying to figure out how to test more use cases in two clients tests. https://codereview.chromium.org/1256803002/diff/320001/chrome/browser/passwor... File chrome/browser/password_manager/password_manager_setting_migrater_service.cc (right): https://codereview.chromium.org/1256803002/diff/320001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service.cc:24: PrefServiceSyncable* prefs = PrefServiceSyncable::FromProfile(profile); On 2015/08/12 08:25:50, vabr (Chromium) wrote: > While PrefServiceSyncable cannot be removed from this code completely, there is > still value in not letting the //chrome dependencies take too deep roots in the > code. In particular here, instead of passing Profile as an argument, and > requesting PrefServiceSyncable, I suggest passing sync_driver::SyncService* as > the argument instead of Profile. The SyncService interface should be enough, and > while you still have to get the same object from the profile you get now, it > will happen lower in the call stack, and spread //chrome dependencies over less > code. Done. https://codereview.chromium.org/1256803002/diff/320001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service.cc:52: void PasswordManagerSettingMigraterService::Observe( On 2015/08/12 08:25:50, vabr (Chromium) wrote: > Would it make sense if instead of the service observing this notification, the > factory would check ProfileManager::IsValidProfile in GetForProfile, and return > nullptr if the profile is not valid yet? > > This would have two advantages: > (1) You could have the single constructor for the service, and guaranteed that > |sync_service| won't change value until your service's shutdown. > (2) You could drop the code for observing the profile notification. Resolved by offline discussion https://codereview.chromium.org/1256803002/diff/320001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service.cc:56: DCHECK(type == chrome::NOTIFICATION_PROFILE_ADDED); On 2015/08/12 08:25:50, vabr (Chromium) wrote: > nit: DCHECK_EQ(chrome::NOTIFICATION_PROFILE_ADDED, type); Done. https://codereview.chromium.org/1256803002/diff/320001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service.cc:80: PasswordManagerSettingMigraterService::MigrateLegacyOffState(prefs); On 2015/08/12 08:25:50, vabr (Chromium) wrote: > Why do we try to set the YOLO preference, when there is no sync service (or it > cannot start)? It's point where we migrate non-sync users to the new setting. So idea that every one will be migrated to the yolo setting even not yolo users. https://codereview.chromium.org/1256803002/diff/320001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service.cc:104: prefs->RemoveObserver(this); On 2015/08/12 12:32:09, vabr (Chromium) wrote: > We should make sure that this is called also during Shutdown, if needed. Done. https://codereview.chromium.org/1256803002/diff/320001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service.cc:109: void PasswordManagerSettingMigraterService::MigrateLegacyOffState( On 2015/08/12 08:25:50, vabr (Chromium) wrote: > Should we ensure that this is only called at most once? > Also, should we make sure that this is not called after Shutdown() has been > called on your service? Done. https://codereview.chromium.org/1256803002/diff/320001/chrome/browser/passwor... File chrome/browser/password_manager/password_manager_setting_migrater_service.h (right): https://codereview.chromium.org/1256803002/diff/320001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service.h:22: namespace password_manager { On 2015/08/12 08:25:51, vabr (Chromium) wrote: > nit: By convention, code in //chrome mostly does not use namespaces, and > password manager code should be in its namespace only in the component. To keep > consistency, I suggest not creating the new code in //chrome in namespace > password_manager. Feel free to use "using" declarations in the .cc files to > avoid lengthy expressions when referencing code from the component. Done. https://codereview.chromium.org/1256803002/diff/320001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service.h:70: PasswordManagerSettingMigraterService(Profile* profile, On 2015/08/12 08:25:51, vabr (Chromium) wrote: > As engedy@ noted before -- why not make this the only constructor, and inject > the production sync service through it as well? > > Any test-specific changes in production are usually a bad sign, forking > production code for test means that the less of the production code gets tested. Found another way how to inject mock ProfileSyncService. https://codereview.chromium.org/1256803002/diff/320001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service.h:89: Profile* profile_; On 2015/08/12 08:25:51, vabr (Chromium) wrote: > The member pointers which are not touched elsewhere than during construction > should be marked const: > Profile* const profile_; > sync_driver::SyncService* const sync_service_; Done. Can't do this for sync_service_, because it nit initialized in constructor. https://codereview.chromium.org/1256803002/diff/320001/components/password_ma... File components/password_manager/core/common/password_manager_pref_names.h (right): https://codereview.chromium.org/1256803002/diff/320001/components/password_ma... components/password_manager/core/common/password_manager_pref_names.h:30: // credentials. On 2015/08/12 08:25:51, vabr (Chromium) wrote: > Which credentials? Do you mean Credentials API? I meant username and password pair. I changed comment.
Hey, can you have another look. Thanks!
Hi Tanja, I managed to have a look at patch set 14, I have not seen yet the new tests you just added, will look at them soon. The CL looks good to me, but I'd like to clarify some comments below before approving. Thanks for all the work you put into this! Vaclav https://codereview.chromium.org/1256803002/diff/320001/chrome/browser/passwor... File chrome/browser/password_manager/password_manager_setting_migrater_service.cc (right): https://codereview.chromium.org/1256803002/diff/320001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service.cc:80: PasswordManagerSettingMigraterService::MigrateLegacyOffState(prefs); On 2015/08/13 15:49:15, melandory wrote: > On 2015/08/12 08:25:50, vabr (Chromium) wrote: > > Why do we try to set the YOLO preference, when there is no sync service (or it > > cannot start)? > > It's point where we migrate non-sync users to the new setting. So idea that > every one will be migrated to the yolo setting even not yolo users. Good point, thanks for explanation! Acknowledged. https://codereview.chromium.org/1256803002/diff/320001/chrome/browser/passwor... File chrome/browser/password_manager/password_manager_setting_migrater_service.h (right): https://codereview.chromium.org/1256803002/diff/320001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service.h:70: PasswordManagerSettingMigraterService(Profile* profile, On 2015/08/13 15:49:15, melandory wrote: > On 2015/08/12 08:25:51, vabr (Chromium) wrote: > > As engedy@ noted before -- why not make this the only constructor, and inject > > the production sync service through it as well? > > > > Any test-specific changes in production are usually a bad sign, forking > > production code for test means that the less of the production code gets > tested. > > Found another way how to inject mock ProfileSyncService. Great, I like that it now comes with the notification as in production code. https://codereview.chromium.org/1256803002/diff/320001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service.h:89: Profile* profile_; On 2015/08/13 15:49:15, melandory wrote: > On 2015/08/12 08:25:51, vabr (Chromium) wrote: > > The member pointers which are not touched elsewhere than during construction > > should be marked const: > > Profile* const profile_; > > sync_driver::SyncService* const sync_service_; > > Done. > > Can't do this for sync_service_, because it nit initialized in constructor. Acknowledged. https://codereview.chromium.org/1256803002/diff/400001/chrome/browser/passwor... File chrome/browser/password_manager/password_manager_setting_migrater_service.cc (right): https://codereview.chromium.org/1256803002/diff/400001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service.cc:19: namespace { nit: Blank line below. https://codereview.chromium.org/1256803002/diff/400001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service.cc:83: MigrateLegacyOffState(prefs); I suggest replacing this line with prefs->RemoveObserver(this); Please also make sure that it is OK to call RemoveObserver if we did not call AddObserver (and if that's not OK, then ensure that we only call RO if we called AO). https://codereview.chromium.org/1256803002/diff/400001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service.cc:108: prefs->RemoveObserver(this); nit: I would swap the order of lines 107 and 108. It probably does not change anything, but just in case there were some posting of tasks in the future which could cause re-entering this block, removing the observer before acting on the observed signal prevents that. https://codereview.chromium.org/1256803002/diff/400001/chrome/browser/passwor... File chrome/browser/password_manager/password_manager_setting_migrater_service.h (right): https://codereview.chromium.org/1256803002/diff/400001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service.h:62: // For unit testing only. nit: Blank space above. https://codereview.chromium.org/1256803002/diff/400001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service.h:63: PasswordManagerSettingMigraterService(Profile* profile, You don't seem to use this in the tests any more (good!), could you remove it? https://codereview.chromium.org/1256803002/diff/400001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service.h:67: friend class PasswordManagerSettingMigraterServiceTest; Could you drop this? https://codereview.chromium.org/1256803002/diff/400001/chrome/browser/passwor... File chrome/browser/password_manager/password_manager_setting_migrater_service_unittests.cc (right): https://codereview.chromium.org/1256803002/diff/400001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service_unittests.cc:46: if (pref) { Is this only to handle wrong preference names gracefully? Or could pref be null even after line 42 or 44 is executed? If it cannot be null for the right pref names, I suggest replacing the if with DCHECK(pref) << "Wrong preference name: " << name; or alternatively not checking pref at all, and passing an enum instead of |name|, and deriving the string from the enum. https://codereview.chromium.org/1256803002/diff/400001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service_unittests.cc:55: void StartSyncing(PrefServiceSyncable* prefs, Please comment on what this function does. https://codereview.chromium.org/1256803002/diff/400001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service_unittests.cc:63: syncer::SyncableService* sync = prefs->GetSyncableService(type); nit: Insert ASSERT_NE(syncer::UNSPECIFIED, type) << "Wrong preference name: " << name; or do the change with the enum suggested above. https://codereview.chromium.org/1256803002/diff/400001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service_unittests.cc:84: Profile* profile() { return &profile_; } nit: Please insert blank lines between methods. https://codereview.chromium.org/1256803002/diff/400001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service_unittests.cc:101: EXPECT_EQ(prefs->GetBoolean(prefs::kCredentialsEnableService), value); nit: Swap the order of operands at EXPECT_EQ -- the first one needs to be the expected value, the second on the actual output of the tested code. Please check this throughout the test. (While equivalence is a symmetric relation, the side-effect of failing EXPECT_EQ is not -- the first value is reported as expected, and the second as actual.) https://codereview.chromium.org/1256803002/diff/400001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service_unittests.cc:129: EXPECT_EQ(prefs->GetBoolean(prefs::kCredentialsEnableService), true); Why is the old value always true? https://codereview.chromium.org/1256803002/diff/400001/chrome/browser/sync/te... File chrome/browser/sync/test/integration/two_client_password_manager_setting_migrater_service_sync_test.cc (right): https://codereview.chromium.org/1256803002/diff/400001/chrome/browser/sync/te... chrome/browser/sync/test/integration/two_client_password_manager_setting_migrater_service_sync_test.cc:67: ASSERT_FALSE(GetPrefs(i)->GetBoolean( Should we have a similar test before updating the prefs to document that they are both true in the beginning? Why are they true in the beginning, actually?
Hi, Thanks for the more tests. I looked at patch set 15 and added one more comment. Cheers, Vaclav https://codereview.chromium.org/1256803002/diff/420001/chrome/browser/sync/te... File chrome/browser/sync/test/integration/two_client_password_manager_setting_migrater_service_sync_test.cc (right): https://codereview.chromium.org/1256803002/diff/420001/chrome/browser/sync/te... chrome/browser/sync/test/integration/two_client_password_manager_setting_migrater_service_sync_test.cc:38: void TestPrefChangeOnClient(int index, const char* pref_name) { Please describe what the function does and what are the arguments. It seems strange that |index| is relevant for setting the pref but not for reading it.
https://codereview.chromium.org/1256803002/diff/400001/chrome/browser/passwor... File chrome/browser/password_manager/password_manager_setting_migrater_service.cc (right): https://codereview.chromium.org/1256803002/diff/400001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service.cc:19: namespace { On 2015/08/14 14:11:45, vabr (Chromium) wrote: > nit: Blank line below. Done. https://codereview.chromium.org/1256803002/diff/400001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service.cc:83: MigrateLegacyOffState(prefs); On 2015/08/14 14:11:45, vabr (Chromium) wrote: > I suggest replacing this line with > prefs->RemoveObserver(this); > > Please also make sure that it is OK to call RemoveObserver if we did not call > AddObserver (and if that's not OK, then ensure that we only call RO if we called > AO). Yes, nothing will happen if we remove non added observer https://code.google.com/p/chromium/codesearch#chromium/src/base/observer_list... https://codereview.chromium.org/1256803002/diff/400001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service.cc:108: prefs->RemoveObserver(this); On 2015/08/14 14:11:45, vabr (Chromium) wrote: > nit: I would swap the order of lines 107 and 108. It probably does not change > anything, but just in case there were some posting of tasks in the future which > could cause re-entering this block, removing the observer before acting on the > observed signal prevents that. Done. https://codereview.chromium.org/1256803002/diff/400001/chrome/browser/passwor... File chrome/browser/password_manager/password_manager_setting_migrater_service.h (right): https://codereview.chromium.org/1256803002/diff/400001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service.h:62: // For unit testing only. On 2015/08/14 14:11:45, vabr (on holiday very soon) wrote: > nit: Blank space above. Done. https://codereview.chromium.org/1256803002/diff/400001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service.h:63: PasswordManagerSettingMigraterService(Profile* profile, On 2015/08/14 14:11:45, vabr (Chromium) wrote: > You don't seem to use this in the tests any more (good!), could you remove it? Yeah, I did the logic for unfriending but forgot to remove everything else :) https://codereview.chromium.org/1256803002/diff/400001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service.h:67: friend class PasswordManagerSettingMigraterServiceTest; On 2015/08/14 14:11:45, vabr (Chromium) wrote: > Could you drop this? Sorry, forgot to remove. https://codereview.chromium.org/1256803002/diff/400001/chrome/browser/passwor... File chrome/browser/password_manager/password_manager_setting_migrater_service_unittests.cc (right): https://codereview.chromium.org/1256803002/diff/400001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service_unittests.cc:46: if (pref) { On 2015/08/14 14:11:45, vabr (Chromium) wrote: > Is this only to handle wrong preference names gracefully? Or could pref be null > even after line 42 or 44 is executed? > If it cannot be null for the right pref names, I suggest replacing the if with > DCHECK(pref) << "Wrong preference name: " << name; > or alternatively not checking pref at all, and passing an enum instead of > |name|, and deriving the string from the enum. Done. https://codereview.chromium.org/1256803002/diff/400001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service_unittests.cc:55: void StartSyncing(PrefServiceSyncable* prefs, On 2015/08/14 14:11:45, vabr (on holiday very soon) wrote: > Please comment on what this function does. Done. https://codereview.chromium.org/1256803002/diff/400001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service_unittests.cc:63: syncer::SyncableService* sync = prefs->GetSyncableService(type); On 2015/08/14 14:11:45, vabr (Chromium) wrote: > nit: Insert > > ASSERT_NE(syncer::UNSPECIFIED, type) << "Wrong preference name: " << name; > > or do the change with the enum suggested above. Done. https://codereview.chromium.org/1256803002/diff/400001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service_unittests.cc:84: Profile* profile() { return &profile_; } On 2015/08/14 14:11:45, vabr (Chromium) wrote: > nit: Please insert blank lines between methods. Done. https://codereview.chromium.org/1256803002/diff/400001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service_unittests.cc:101: EXPECT_EQ(prefs->GetBoolean(prefs::kCredentialsEnableService), value); On 2015/08/14 14:11:45, vabr (Chromium) wrote: > nit: Swap the order of operands at EXPECT_EQ -- the first one needs to be the > expected value, the second on the actual output of the tested code. > > Please check this throughout the test. > > (While equivalence is a symmetric relation, the side-effect of failing EXPECT_EQ > is not -- the first value is reported as expected, and the second as actual.) Done. https://codereview.chromium.org/1256803002/diff/400001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service_unittests.cc:129: EXPECT_EQ(prefs->GetBoolean(prefs::kCredentialsEnableService), true); On 2015/08/14 14:11:45, vabr (Chromium) wrote: > Why is the old value always true? Will add another set of tests for the other case. https://codereview.chromium.org/1256803002/diff/400001/chrome/browser/sync/te... File chrome/browser/sync/test/integration/two_client_password_manager_setting_migrater_service_sync_test.cc (right): https://codereview.chromium.org/1256803002/diff/400001/chrome/browser/sync/te... chrome/browser/sync/test/integration/two_client_password_manager_setting_migrater_service_sync_test.cc:67: ASSERT_FALSE(GetPrefs(i)->GetBoolean( On 2015/08/14 14:11:45, vabr (Chromium) wrote: > Should we have a similar test before updating the prefs to document that they > are both true in the beginning? > Why are they true in the beginning, actually? Default value. https://codereview.chromium.org/1256803002/diff/420001/chrome/browser/sync/te... File chrome/browser/sync/test/integration/two_client_password_manager_setting_migrater_service_sync_test.cc (right): https://codereview.chromium.org/1256803002/diff/420001/chrome/browser/sync/te... chrome/browser/sync/test/integration/two_client_password_manager_setting_migrater_service_sync_test.cc:38: void TestPrefChangeOnClient(int index, const char* pref_name) { On 2015/08/14 14:55:20, vabr (on holiday very soon) wrote: > Please describe what the function does and what are the arguments. It seems > strange that |index| is relevant for setting the pref but not for reading it. Done.
Patchset #21 (id:540001) has been deleted
On 2015/08/17 16:28:56, melandory wrote: > https://codereview.chromium.org/1256803002/diff/400001/chrome/browser/passwor... > File > chrome/browser/password_manager/password_manager_setting_migrater_service.cc > (right): > > https://codereview.chromium.org/1256803002/diff/400001/chrome/browser/passwor... > chrome/browser/password_manager/password_manager_setting_migrater_service.cc:19: > namespace { > On 2015/08/14 14:11:45, vabr (Chromium) wrote: > > nit: Blank line below. > > Done. > > https://codereview.chromium.org/1256803002/diff/400001/chrome/browser/passwor... > chrome/browser/password_manager/password_manager_setting_migrater_service.cc:83: > MigrateLegacyOffState(prefs); > On 2015/08/14 14:11:45, vabr (Chromium) wrote: > > I suggest replacing this line with > > prefs->RemoveObserver(this); > > > > Please also make sure that it is OK to call RemoveObserver if we did not call > > AddObserver (and if that's not OK, then ensure that we only call RO if we > called > > AO). > Yes, nothing will happen if we remove non added observer > https://code.google.com/p/chromium/codesearch#chromium/src/base/observer_list... > > https://codereview.chromium.org/1256803002/diff/400001/chrome/browser/passwor... > chrome/browser/password_manager/password_manager_setting_migrater_service.cc:108: > prefs->RemoveObserver(this); > On 2015/08/14 14:11:45, vabr (Chromium) wrote: > > nit: I would swap the order of lines 107 and 108. It probably does not change > > anything, but just in case there were some posting of tasks in the future > which > > could cause re-entering this block, removing the observer before acting on the > > observed signal prevents that. > > Done. > > https://codereview.chromium.org/1256803002/diff/400001/chrome/browser/passwor... > File chrome/browser/password_manager/password_manager_setting_migrater_service.h > (right): > > https://codereview.chromium.org/1256803002/diff/400001/chrome/browser/passwor... > chrome/browser/password_manager/password_manager_setting_migrater_service.h:62: > // For unit testing only. > On 2015/08/14 14:11:45, vabr (on holiday very soon) wrote: > > nit: Blank space above. > > Done. > > https://codereview.chromium.org/1256803002/diff/400001/chrome/browser/passwor... > chrome/browser/password_manager/password_manager_setting_migrater_service.h:63: > PasswordManagerSettingMigraterService(Profile* profile, > On 2015/08/14 14:11:45, vabr (Chromium) wrote: > > You don't seem to use this in the tests any more (good!), could you remove it? > > Yeah, I did the logic for unfriending but forgot to remove everything else :) > > https://codereview.chromium.org/1256803002/diff/400001/chrome/browser/passwor... > chrome/browser/password_manager/password_manager_setting_migrater_service.h:67: > friend class PasswordManagerSettingMigraterServiceTest; > On 2015/08/14 14:11:45, vabr (Chromium) wrote: > > Could you drop this? > > Sorry, forgot to remove. > > https://codereview.chromium.org/1256803002/diff/400001/chrome/browser/passwor... > File > chrome/browser/password_manager/password_manager_setting_migrater_service_unittests.cc > (right): > > https://codereview.chromium.org/1256803002/diff/400001/chrome/browser/passwor... > chrome/browser/password_manager/password_manager_setting_migrater_service_unittests.cc:46: > if (pref) { > On 2015/08/14 14:11:45, vabr (Chromium) wrote: > > Is this only to handle wrong preference names gracefully? Or could pref be > null > > even after line 42 or 44 is executed? > > If it cannot be null for the right pref names, I suggest replacing the if with > > DCHECK(pref) << "Wrong preference name: " << name; > > or alternatively not checking pref at all, and passing an enum instead of > > |name|, and deriving the string from the enum. > > Done. > > https://codereview.chromium.org/1256803002/diff/400001/chrome/browser/passwor... > chrome/browser/password_manager/password_manager_setting_migrater_service_unittests.cc:55: > void StartSyncing(PrefServiceSyncable* prefs, > On 2015/08/14 14:11:45, vabr (on holiday very soon) wrote: > > Please comment on what this function does. > > Done. > > https://codereview.chromium.org/1256803002/diff/400001/chrome/browser/passwor... > chrome/browser/password_manager/password_manager_setting_migrater_service_unittests.cc:63: > syncer::SyncableService* sync = prefs->GetSyncableService(type); > On 2015/08/14 14:11:45, vabr (Chromium) wrote: > > nit: Insert > > > > ASSERT_NE(syncer::UNSPECIFIED, type) << "Wrong preference name: " << name; > > > > or do the change with the enum suggested above. > > Done. > > https://codereview.chromium.org/1256803002/diff/400001/chrome/browser/passwor... > chrome/browser/password_manager/password_manager_setting_migrater_service_unittests.cc:84: > Profile* profile() { return &profile_; } > On 2015/08/14 14:11:45, vabr (Chromium) wrote: > > nit: Please insert blank lines between methods. > > Done. > > https://codereview.chromium.org/1256803002/diff/400001/chrome/browser/passwor... > chrome/browser/password_manager/password_manager_setting_migrater_service_unittests.cc:101: > EXPECT_EQ(prefs->GetBoolean(prefs::kCredentialsEnableService), value); > On 2015/08/14 14:11:45, vabr (Chromium) wrote: > > nit: Swap the order of operands at EXPECT_EQ -- the first one needs to be the > > expected value, the second on the actual output of the tested code. > > > > Please check this throughout the test. > > > > (While equivalence is a symmetric relation, the side-effect of failing > EXPECT_EQ > > is not -- the first value is reported as expected, and the second as actual.) > > Done. > > https://codereview.chromium.org/1256803002/diff/400001/chrome/browser/passwor... > chrome/browser/password_manager/password_manager_setting_migrater_service_unittests.cc:129: > EXPECT_EQ(prefs->GetBoolean(prefs::kCredentialsEnableService), true); > On 2015/08/14 14:11:45, vabr (Chromium) wrote: > > Why is the old value always true? > > Will add another set of tests for the other case. > > https://codereview.chromium.org/1256803002/diff/400001/chrome/browser/sync/te... > File > chrome/browser/sync/test/integration/two_client_password_manager_setting_migrater_service_sync_test.cc > (right): > > https://codereview.chromium.org/1256803002/diff/400001/chrome/browser/sync/te... > chrome/browser/sync/test/integration/two_client_password_manager_setting_migrater_service_sync_test.cc:67: > ASSERT_FALSE(GetPrefs(i)->GetBoolean( > On 2015/08/14 14:11:45, vabr (Chromium) wrote: > > Should we have a similar test before updating the prefs to document that they > > are both true in the beginning? > > Why are they true in the beginning, actually? > > Default value. > > https://codereview.chromium.org/1256803002/diff/420001/chrome/browser/sync/te... > File > chrome/browser/sync/test/integration/two_client_password_manager_setting_migrater_service_sync_test.cc > (right): > > https://codereview.chromium.org/1256803002/diff/420001/chrome/browser/sync/te... > chrome/browser/sync/test/integration/two_client_password_manager_setting_migrater_service_sync_test.cc:38: > void TestPrefChangeOnClient(int index, const char* pref_name) { > On 2015/08/14 14:55:20, vabr (on holiday very soon) wrote: > > Please describe what the function does and what are the arguments. It seems > > strange that |index| is relevant for setting the pref but not for reading it. > > Done. Hi Newton, I've addressed previous round of comments, please take a look.
Thanks, especially the unittests are now much easier to read and provide great coverage. I have some more comments, but this LGTM. To be honest, I got a bit lost in the meeting today about the proposed design and the issues. If the design doc is up to date, and you want me to read it and also check this CL that it implements exactly that design, please let me know. So far I have checked this CL for sanity with respect to what I remembered about the design. Cheers, Vaclav https://codereview.chromium.org/1256803002/diff/600001/chrome/browser/passwor... File chrome/browser/password_manager/password_manager_setting_migrater_service.cc (right): https://codereview.chromium.org/1256803002/diff/600001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service.cc:94: MigrateOffState(prefs); I still think we should call prefs->RemoveObserver(this) and not MigrateOffState. Shutdown is a place to clean-up, not try to do any more work. Is there a good reason to try and do migration here? https://codereview.chromium.org/1256803002/diff/600001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service.cc:110: // In order to preserve off value we need to hold a propagation of the update. nit: What does "hold a propagation" mean? Did you mean "hold on with propagation"? https://codereview.chromium.org/1256803002/diff/600001/chrome/browser/passwor... File chrome/browser/password_manager/password_manager_setting_migrater_service.h (right): https://codereview.chromium.org/1256803002/diff/600001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service.h:89: bool both_pref_off_; Please comment on the meaning of this flag, and how and why it is used. https://codereview.chromium.org/1256803002/diff/600001/chrome/browser/passwor... File chrome/browser/password_manager/password_manager_setting_migrater_service_unittests.cc (right): https://codereview.chromium.org/1256803002/diff/600001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service_unittests.cc:54: // Emulates start of the syncing for the specific sync type. If |name| is old nit: Please reference kPasswordManagerSavingEnabled and kCredentialsEnableService instead of "old" and "new". https://codereview.chromium.org/1256803002/diff/600001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service_unittests.cc:55: // preference, then it's PREFERENCE syncing if |name| is new pref, then it's nit: A missing comma after "syncing".
PTAL https://codereview.chromium.org/1256803002/diff/600001/chrome/browser/passwor... File chrome/browser/password_manager/password_manager_setting_migrater_service.cc (right): https://codereview.chromium.org/1256803002/diff/600001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service.cc:94: MigrateOffState(prefs); On 2015/08/26 14:42:43, vabr (slow start past holiday) wrote: > I still think we should call prefs->RemoveObserver(this) and not > MigrateOffState. Shutdown is a place to clean-up, not try to do any more work. > Is there a good reason to try and do migration here? Done. https://codereview.chromium.org/1256803002/diff/600001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service.cc:110: // In order to preserve off value we need to hold a propagation of the update. On 2015/08/26 14:42:43, vabr (slow start past holiday) wrote: > nit: What does "hold a propagation" mean? Did you mean "hold on with > propagation"? Not applicable any more. https://codereview.chromium.org/1256803002/diff/600001/chrome/browser/passwor... File chrome/browser/password_manager/password_manager_setting_migrater_service.h (right): https://codereview.chromium.org/1256803002/diff/600001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service.h:89: bool both_pref_off_; On 2015/08/26 14:42:43, vabr (slow start past holiday) wrote: > Please comment on the meaning of this flag, and how and why it is used. Not applicable anymore. https://codereview.chromium.org/1256803002/diff/600001/chrome/browser/passwor... File chrome/browser/password_manager/password_manager_setting_migrater_service_unittests.cc (right): https://codereview.chromium.org/1256803002/diff/600001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service_unittests.cc:54: // Emulates start of the syncing for the specific sync type. If |name| is old On 2015/08/26 14:42:43, vabr (slow start past holiday) wrote: > nit: Please reference kPasswordManagerSavingEnabled and > kCredentialsEnableService instead of "old" and "new". Done. https://codereview.chromium.org/1256803002/diff/600001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service_unittests.cc:55: // preference, then it's PREFERENCE syncing if |name| is new pref, then it's On 2015/08/26 14:42:43, vabr (slow start past holiday) wrote: > nit: A missing comma after "syncing". Done.
Patchset #24 (id:620001) has been deleted
Thanks for the update. This LGTM, with some comments. Cheers, Vaclav https://codereview.chromium.org/1256803002/diff/640001/chrome/browser/passwor... File chrome/browser/password_manager/password_manager_setting_migrater_service.cc (right): https://codereview.chromium.org/1256803002/diff/640001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service.cc:36: void UpdatePreferencesValuesIfNeeded(PrefService* prefs, bool new_value) { nit: We have a clash in the meaning of "new" in new_value vs. new_pref_value. I vote for changing new_pref_value, e.g., to current_pref_value. Once you decide on the renaming, please apply it consistently in all the added code. https://codereview.chromium.org/1256803002/diff/640001/chrome/browser/passwor... File chrome/browser/password_manager/password_manager_setting_migrater_service.h (right): https://codereview.chromium.org/1256803002/diff/640001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service.h:85: // based on initial values for bother preferences and values received from typo: bother https://codereview.chromium.org/1256803002/diff/640001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service.h:96: // This vector contains initial value for both preferences. Please comment on the maximal size, and expected order of values in the vectors. https://codereview.chromium.org/1256803002/diff/640001/chrome/browser/passwor... File chrome/browser/password_manager/password_manager_setting_migrater_service_unittests.cc (right): https://codereview.chromium.org/1256803002/diff/640001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service_unittests.cc:101: PasswordManagerSettingMigraterServiceTest() {} //: service_(&profile_) {} Please remove the commented-out code on this line.
https://codereview.chromium.org/1256803002/diff/640001/chrome/browser/passwor... File chrome/browser/password_manager/password_manager_setting_migrater_service.cc (right): https://codereview.chromium.org/1256803002/diff/640001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service.cc:36: void UpdatePreferencesValuesIfNeeded(PrefService* prefs, bool new_value) { On 2015/08/28 16:58:19, vabr (OOO until Tue) wrote: > nit: We have a clash in the meaning of "new" in new_value vs. new_pref_value. I > vote for changing new_pref_value, e.g., to current_pref_value. > > Once you decide on the renaming, please apply it consistently in all the added > code. I prefer to have new_pref_value instead of current_pref_value, but I'll check that I do not use new_value for the new_pref_value. https://codereview.chromium.org/1256803002/diff/640001/chrome/browser/passwor... File chrome/browser/password_manager/password_manager_setting_migrater_service.h (right): https://codereview.chromium.org/1256803002/diff/640001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service.h:85: // based on initial values for bother preferences and values received from On 2015/08/28 16:58:19, vabr (OOO until Tue) wrote: > typo: bother Done. https://codereview.chromium.org/1256803002/diff/640001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service.h:96: // This vector contains initial value for both preferences. On 2015/08/28 16:58:19, vabr (OOO until Tue) wrote: > Please comment on the maximal size, and expected order of values in the vectors. Done. https://codereview.chromium.org/1256803002/diff/640001/chrome/browser/passwor... File chrome/browser/password_manager/password_manager_setting_migrater_service_unittests.cc (right): https://codereview.chromium.org/1256803002/diff/640001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service_unittests.cc:101: PasswordManagerSettingMigraterServiceTest() {} //: service_(&profile_) {} On 2015/08/28 16:58:19, vabr (OOO until Tue) wrote: > Please remove the commented-out code on this line. Ups, sorry
Some initial comments, I am still looking at it. https://codereview.chromium.org/1256803002/diff/700001/chrome/browser/passwor... File chrome/browser/password_manager/password_manager_setting_migrater_service.cc (right): https://codereview.chromium.org/1256803002/diff/700001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service.cc:69: registrar_.Add(this, chrome::NOTIFICATION_PROFILE_ADDED, We should add a comment to summarize the discussion on the CL and explain this cryptic line (i.e., that this is the signal that we use to wait until after somebody deigns to create the ProfileSyncService). This can be either here, or in the class comment in the header where we could add a paragraph describing start-up ordering. https://codereview.chromium.org/1256803002/diff/700001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service.cc:211: DependsOn(ProfileSyncServiceFactory::GetInstance()); We should add a comment to summarize the discussion on the CL and explain this cryptic line. This can be either here, or in the class comment in the header where we could add a paragraph describing start-up ordering. https://codereview.chromium.org/1256803002/diff/700001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service.cc:220: static_cast<Profile*>(context)); nit: Profile::FromBrowserContext https://codereview.chromium.org/1256803002/diff/700001/chrome/browser/passwor... File chrome/browser/password_manager/password_manager_setting_migrater_service.h (right): https://codereview.chromium.org/1256803002/diff/700001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service.h:24: // PasswordManagerSettingMigraterService is responsible for reconciling Chrome Phrasing nit: Given the complexity and the vast number of edge cases, we should give a summary of them in this comment. It could go like this: // Service that is responsible for reconciling the legacy "Offer to save your // web passwords" setting with the new "Enable Smart Lock for Passwords" // setting. // // It works as follows: // // [...] https://codereview.chromium.org/1256803002/diff/700001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service.h:46: // BrowserContextKeyedServiceFactory nit: Add colon (:) at the end. Same below, 3 places altogether. https://codereview.chromium.org/1256803002/diff/700001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service.h:69: // Called on event of changing kCredentialsEnableService preference. Changes Phrasing nit: What do you think about simply saying: // Called when the value of the |kCredentialsEnableService| preference changes, and updates the value of |kPasswordManagerSavingEnabled| accordingly. https://codereview.chromium.org/1256803002/diff/700001/chrome/browser/passwor... File chrome/browser/password_manager/password_manager_setting_migrater_service_unittests.cc (right): https://codereview.chromium.org/1256803002/diff/700001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service_unittests.cc:45: } nit: } else { NOTREACHED() << "Wrong preference name: " << name; } Also, the {} are not needed as everything fits on one line. https://codereview.chromium.org/1256803002/diff/700001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service_unittests.cc:134: void TestEmptySyncDataDoesntAffectPrefValues(bool value) { To make the tests more concise, we could consolidate most of these methods into one, by introducing a Tristate enum class type (with UNSET, FALSE, TRUE). https://codereview.chromium.org/1256803002/diff/700001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service_unittests.cc:262: TEST_F(PasswordManagerSettingMigraterServiceTest, I think we could make these tests a bit easier to read if we consolidated this and the analogous tests below into single test that iterates through an array of |kTestCases|, like here: components/password_manager/core/browser/affiliation_utils_unittest.cc. It could have the initial values, the Sync changes, and then the expectations defined for each case (in form of Tristates and/or bools). https://codereview.chromium.org/1256803002/diff/700001/chrome/browser/sync/te... File chrome/browser/sync/test/integration/two_client_password_manager_setting_migrater_service_sync_test.cc (right): https://codereview.chromium.org/1256803002/diff/700001/chrome/browser/sync/te... chrome/browser/sync/test/integration/two_client_password_manager_setting_migrater_service_sync_test.cc:92: ASSERT_TRUE(SetupSync()); If I understood correctly how Sync works, MergeDataAndStartSyncing would normally try to merge equivalent sets of sync-side and browser-side data, except when the user had not been syncing before and starts syncing now. Still, we should make sure to have some tests that verify that in those corner cases we have identified, there are no false changes generated in these cases.
https://codereview.chromium.org/1256803002/diff/700001/chrome/browser/passwor... File chrome/browser/password_manager/password_manager_setting_migrater_service.cc (right): https://codereview.chromium.org/1256803002/diff/700001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service.cc:29: // TODO(melandory): add histograms in order to track when we can stop Have you though of what metrics we could use to track this? We would also probably need to put this behind a field trial, so that we could migrate a small subset of users initially, and somehow judge based on said metrics that we have done a good job at migrating. https://codereview.chromium.org/1256803002/diff/700001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service.cc:91: &initial_values_); Could we just save to an |initial_new_pref_value_| and |initial_legacy_pref_value_| instead, and get rid of the std::vector?
Patchset #28 (id:720001) has been deleted
Patchset #28 (id:740001) has been deleted
Patchset #28 (id:760001) has been deleted
Patchset #25 (id:660001) has been deleted
Patchset #27 (id:780001) has been deleted
Patchset #25 (id:680001) has been deleted
Patchset #2 (id:40001) has been deleted
Patchset #3 (id:100001) has been deleted
https://codereview.chromium.org/1256803002/diff/700001/chrome/browser/passwor... File chrome/browser/password_manager/password_manager_setting_migrater_service.cc (right): https://codereview.chromium.org/1256803002/diff/700001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service.cc:29: // TODO(melandory): add histograms in order to track when we can stop On 2015/09/01 23:10:58, engedy wrote: > Have you though of what metrics we could use to track this? > > We would also probably need to put this behind a field trial, so that we could > migrate a small subset of users initially, and somehow judge based on said > metrics that we have done a good job at migrating. As per offline discussion: * Log initial values for n days without performing migration, then roll migration for some percentage and log initial and final values. * Log that pref value was changed separately for users who has performed migration and for one who doesn't. P.S. I'll implement this in a separate CL. https://codereview.chromium.org/1256803002/diff/700001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service.cc:69: registrar_.Add(this, chrome::NOTIFICATION_PROFILE_ADDED, On 2015/08/31 18:17:23, engedy wrote: > We should add a comment to summarize the discussion on the CL and explain this > cryptic line (i.e., that this is the signal that we use to wait until after > somebody deigns to create the ProfileSyncService). This can be either here, or > in the class comment in the header where we could add a paragraph describing > start-up ordering. added to header https://codereview.chromium.org/1256803002/diff/700001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service.cc:91: &initial_values_); On 2015/09/01 23:10:58, engedy wrote: > Could we just save to an |initial_new_pref_value_| and > |initial_legacy_pref_value_| instead, and get rid of the std::vector? Done. https://codereview.chromium.org/1256803002/diff/700001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service.cc:211: DependsOn(ProfileSyncServiceFactory::GetInstance()); On 2015/08/31 18:17:23, engedy wrote: > We should add a comment to summarize the discussion on the CL and explain this > cryptic line. This can be either here, or in the class comment in the header > where we could add a paragraph describing start-up ordering. added to header. https://codereview.chromium.org/1256803002/diff/700001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service.cc:220: static_cast<Profile*>(context)); On 2015/08/31 18:17:23, engedy wrote: > nit: Profile::FromBrowserContext Done. https://codereview.chromium.org/1256803002/diff/700001/chrome/browser/passwor... File chrome/browser/password_manager/password_manager_setting_migrater_service.h (right): https://codereview.chromium.org/1256803002/diff/700001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service.h:24: // PasswordManagerSettingMigraterService is responsible for reconciling Chrome On 2015/08/31 18:17:23, engedy wrote: > Phrasing nit: > > Given the complexity and the vast number of edge cases, we should give a summary > of them in this comment. It could go like this: > > // Service that is responsible for reconciling the legacy "Offer to save your > // web passwords" setting with the new "Enable Smart Lock for Passwords" > // setting. > // > // It works as follows: > // > // [...] Done. https://codereview.chromium.org/1256803002/diff/700001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service.h:46: // BrowserContextKeyedServiceFactory On 2015/08/31 18:17:23, engedy wrote: > nit: Add colon (:) at the end. Same below, 3 places altogether. Done. https://codereview.chromium.org/1256803002/diff/700001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service.h:69: // Called on event of changing kCredentialsEnableService preference. Changes On 2015/08/31 18:17:23, engedy wrote: > Phrasing nit: What do you think about simply saying: > > // Called when the value of the |kCredentialsEnableService| preference changes, > and updates the value of |kPasswordManagerSavingEnabled| accordingly. Done. https://codereview.chromium.org/1256803002/diff/700001/chrome/browser/passwor... File chrome/browser/password_manager/password_manager_setting_migrater_service_unittests.cc (right): https://codereview.chromium.org/1256803002/diff/700001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service_unittests.cc:45: } On 2015/08/31 18:17:23, engedy wrote: > nit: > > } else { > NOTREACHED() << "Wrong preference name: " << name; > } > > Also, the {} are not needed as everything fits on one line. Done. https://codereview.chromium.org/1256803002/diff/700001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service_unittests.cc:134: void TestEmptySyncDataDoesntAffectPrefValues(bool value) { On 2015/08/31 18:17:23, engedy wrote: > To make the tests more concise, we could consolidate most of these methods into > one, by introducing a Tristate enum class type (with UNSET, FALSE, TRUE). Done. https://codereview.chromium.org/1256803002/diff/700001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service_unittests.cc:262: TEST_F(PasswordManagerSettingMigraterServiceTest, On 2015/08/31 18:17:23, engedy wrote: > I think we could make these tests a bit easier to read if we consolidated this > and the analogous tests below into single test that iterates through an array of > |kTestCases|, like here: > components/password_manager/core/browser/affiliation_utils_unittest.cc. > > It could have the initial values, the Sync changes, and then the expectations > defined for each case (in form of Tristates and/or bools). Done.
Proofreading of the long comments section. I have added a bunch of phrasing nits. https://codereview.chromium.org/1256803002/diff/800001/chrome/browser/passwor... File chrome/browser/password_manager/password_manager_setting_migrater_service.h (right): https://codereview.chromium.org/1256803002/diff/800001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service.h:25: // web passwords" setting (L, which is states for legacy) with the new nit: (henceforth denoted 'L', for legacy) https://codereview.chromium.org/1256803002/diff/800001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service.h:26: // "Enable Smart Lock for Passwords" setting (N, which is states for new). nit: ('N', for new) https://codereview.chromium.org/1256803002/diff/800001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service.h:30: // Users who are not syncing (this is checked on a service startup by nit: not syncing preferences? https://codereview.chromium.org/1256803002/diff/800001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service.h:31: // calling to SyncService:CanSyncStart()) are migrated on a startup of a nit: on start-up of the https://codereview.chromium.org/1256803002/diff/800001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service.h:36: // Nothing should be done in case 1 and 2 since settings are in sync with each nit: are already in sync https://codereview.chromium.org/1256803002/diff/800001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service.h:37: // other, in case 3 we change value of N to 1. nit: s/,/;/ https://codereview.chromium.org/1256803002/diff/800001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service.h:38: // Service is also observes changes to both preferences, so if one of the nit: The service also https://codereview.chromium.org/1256803002/diff/800001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service.h:42: // For users who are syncing we save the values for new and legacy preference on nit: the new and legacy preferences https://codereview.chromium.org/1256803002/diff/800001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service.h:43: // a service start up (|inital_values_|) and values come from sync during nit: -a https://codereview.chromium.org/1256803002/diff/800001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service.h:43: // a service start up (|inital_values_|) and values come from sync during nit: and the values that https://codereview.chromium.org/1256803002/diff/800001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service.h:45: // On the finish of model association step we derive new value for both settings nit: the new values https://codereview.chromium.org/1256803002/diff/800001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service.h:45: // On the finish of model association step we derive new value for both settings We should explain: -- why it is not enough to let propagation take care of model association step, -- how we handle changes coming in later through ProcessSyncChanges, and that there is not much we can do there apart from propagation, -- the corner case this leads to the wrong (well...) solution. https://codereview.chromium.org/1256803002/diff/800001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service.h:46: // using following table: nit: Newline after this. https://codereview.chromium.org/1256803002/diff/800001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service.h:47: // NL* 0_ 1_ _0 _1 00 01 10 11 nit: Label the axes of this table. :) https://codereview.chromium.org/1256803002/diff/800001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service.h:49: // 00 00 11 00 11 00 11 11 11 With our recent revelations about how Sync works, are most of these combinations even possible during MergeDataAndStartSyncing, unless Sync is being started for the first time right now? https://codereview.chromium.org/1256803002/diff/800001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service.h:53: // Service observes changes to the both preferences (e.g. changes from sync, nit: Newline before this line, also indent -1. https://codereview.chromium.org/1256803002/diff/800001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service.h:53: // Service observes changes to the both preferences (e.g. changes from sync, nit: to both preferences https://codereview.chromium.org/1256803002/diff/800001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service.h:53: // Service observes changes to the both preferences (e.g. changes from sync, nit: The service
melandory@chromium.org changed reviewers: + pvalenzuela@chromium.org
Hi pvalenzuela@, can you please take a look at two clients test? I also have a question, is there a way for the tests to set a snapshot of a server data, which will be then used during model association step. Thanks in advance! https://codereview.chromium.org/1256803002/diff/800001/chrome/browser/passwor... File chrome/browser/password_manager/password_manager_setting_migrater_service.h (right): https://codereview.chromium.org/1256803002/diff/800001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service.h:25: // web passwords" setting (L, which is states for legacy) with the new On 2015/09/02 13:31:20, engedy wrote: > nit: (henceforth denoted 'L', for legacy) Done. https://codereview.chromium.org/1256803002/diff/800001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service.h:26: // "Enable Smart Lock for Passwords" setting (N, which is states for new). On 2015/09/02 13:31:21, engedy wrote: > nit: ('N', for new) Done. https://codereview.chromium.org/1256803002/diff/800001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service.h:31: // calling to SyncService:CanSyncStart()) are migrated on a startup of a On 2015/09/02 13:31:21, engedy wrote: > nit: on start-up of the Done. https://codereview.chromium.org/1256803002/diff/800001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service.h:36: // Nothing should be done in case 1 and 2 since settings are in sync with each On 2015/09/02 13:31:20, engedy wrote: > nit: are already in sync Done. https://codereview.chromium.org/1256803002/diff/800001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service.h:37: // other, in case 3 we change value of N to 1. On 2015/09/02 13:31:20, engedy wrote: > nit: s/,/;/ Done. https://codereview.chromium.org/1256803002/diff/800001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service.h:38: // Service is also observes changes to both preferences, so if one of the On 2015/09/02 13:31:20, engedy wrote: > nit: The service also Done. https://codereview.chromium.org/1256803002/diff/800001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service.h:42: // For users who are syncing we save the values for new and legacy preference on On 2015/09/02 13:31:21, engedy wrote: > nit: the new and legacy preferences Done. https://codereview.chromium.org/1256803002/diff/800001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service.h:43: // a service start up (|inital_values_|) and values come from sync during On 2015/09/02 13:31:21, engedy wrote: > nit: -a Done. https://codereview.chromium.org/1256803002/diff/800001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service.h:43: // a service start up (|inital_values_|) and values come from sync during On 2015/09/02 13:31:20, engedy wrote: > nit: and the values that Done. https://codereview.chromium.org/1256803002/diff/800001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service.h:45: // On the finish of model association step we derive new value for both settings On 2015/09/02 13:31:21, engedy wrote: > nit: the new values Done. https://codereview.chromium.org/1256803002/diff/800001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service.h:45: // On the finish of model association step we derive new value for both settings On 2015/09/02 13:31:21, engedy wrote: > We should explain: > -- why it is not enough to let propagation take care of model association > step, > -- how we handle changes coming in later through ProcessSyncChanges, and that > there is not much we can do there apart from propagation, > -- the corner case this leads to the wrong (well...) solution. Done. https://codereview.chromium.org/1256803002/diff/800001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service.h:46: // using following table: On 2015/09/02 13:31:21, engedy wrote: > nit: Newline after this. Done. https://codereview.chromium.org/1256803002/diff/800001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service.h:49: // 00 00 11 00 11 00 11 11 11 On 2015/09/02 13:31:20, engedy wrote: > With our recent revelations about how Sync works, are most of these combinations > even possible during MergeDataAndStartSyncing, unless Sync is being started for > the first time right now? Yes, some of this combinations are not possible. WDYT, should I remove them? I actually like that the current approach seems to work correctly even in impossible combinations, because you never know... https://codereview.chromium.org/1256803002/diff/800001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service.h:53: // Service observes changes to the both preferences (e.g. changes from sync, On 2015/09/02 13:31:20, engedy wrote: > nit: Newline before this line, also indent -1. Done. https://codereview.chromium.org/1256803002/diff/800001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service.h:53: // Service observes changes to the both preferences (e.g. changes from sync, On 2015/09/02 13:31:21, engedy wrote: > nit: to both preferences Done. https://codereview.chromium.org/1256803002/diff/800001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service.h:53: // Service observes changes to the both preferences (e.g. changes from sync, On 2015/09/02 13:31:21, engedy wrote: > nit: The service Done.
zea@chromium.org changed reviewers: + zea@chromium.org
Couple comments, still trying to wrap my head around whether the truth table values make sense. https://codereview.chromium.org/1256803002/diff/820001/chrome/browser/passwor... File chrome/browser/password_manager/password_manager_setting_migrater_service.cc (right): https://codereview.chromium.org/1256803002/diff/820001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service.cc:60: initial_values->push_back(*new_pref_value); style_comment: it might be good to have a const int kNewPrefIndex and kLegacyPrefIndex defined at the top of the file which you then always use to access/set these values. That might avoid bugs where you accidentally swap the order of the two due to using push_back https://codereview.chromium.org/1256803002/diff/820001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service.cc:137: LOG(ERROR) << "OnIsSyncingChanged"; All these LOG(ERROR)'s should probably be DVLOG(1) https://codereview.chromium.org/1256803002/diff/820001/chrome/browser/passwor... File chrome/browser/password_manager/password_manager_setting_migrater_service.h (right): https://codereview.chromium.org/1256803002/diff/820001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service.h:38: // in case 3 we change value of N to 1. Doesn't case 3 denote that N's value is already 1? https://codereview.chromium.org/1256803002/diff/820001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service.h:60: // NL* 0_ 1_ _0 _1 00 01 10 11 What are the X and Y axes?
melandory@, To answer your question (assuming I understand it correctly), no. FakeServer will act more or less like the real server. However, you can inject data on the server side (prevent repetitive and slow client-side setup). See the FakeServer::InjectEntity method. Let me know if you need more help writing the sorts of tests you want. https://codereview.chromium.org/1256803002/diff/820001/chrome/browser/sync/te... File chrome/browser/sync/test/integration/two_client_password_manager_setting_migrater_service_sync_test.cc (right): https://codereview.chromium.org/1256803002/diff/820001/chrome/browser/sync/te... chrome/browser/sync/test/integration/two_client_password_manager_setting_migrater_service_sync_test.cc:103: NotifyProfileAdded(0); why trigger the notifications before syncing?
On 2015/09/04 00:01:41, pvalenzuela wrote: > melandory@, > > To answer your question (assuming I understand it correctly), no. FakeServer > will act more or less like the real server. > > However, you can inject data on the server side (prevent repetitive and slow > client-side setup). See the FakeServer::InjectEntity method. Let me know if you > need more help writing the sorts of tests you want. It looks exactly like what I need. Thanks! > > https://codereview.chromium.org/1256803002/diff/820001/chrome/browser/sync/te... > File > chrome/browser/sync/test/integration/two_client_password_manager_setting_migrater_service_sync_test.cc > (right): > > https://codereview.chromium.org/1256803002/diff/820001/chrome/browser/sync/te... > chrome/browser/sync/test/integration/two_client_password_manager_setting_migrater_service_sync_test.cc:103: > NotifyProfileAdded(0); > why trigger the notifications before syncing?
Hey pvalenzuela@: I also will add single client tests with fake server data and multiple client test and ask you for review then. Thanks! https://codereview.chromium.org/1256803002/diff/820001/chrome/browser/passwor... File chrome/browser/password_manager/password_manager_setting_migrater_service.cc (right): https://codereview.chromium.org/1256803002/diff/820001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service.cc:60: initial_values->push_back(*new_pref_value); On 2015/09/03 22:44:16, Nicolas Zea wrote: > style_comment: it might be good to have a const int kNewPrefIndex and > kLegacyPrefIndex defined at the top of the file which you then always use to > access/set these values. That might avoid bugs where you accidentally swap the > order of the two due to using push_back I got rid of this vector. https://codereview.chromium.org/1256803002/diff/820001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service.cc:137: LOG(ERROR) << "OnIsSyncingChanged"; On 2015/09/03 22:44:16, Nicolas Zea wrote: > All these LOG(ERROR)'s should probably be DVLOG(1) Sorry, I've accidentally committed it https://codereview.chromium.org/1256803002/diff/820001/chrome/browser/passwor... File chrome/browser/password_manager/password_manager_setting_migrater_service.h (right): https://codereview.chromium.org/1256803002/diff/820001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service.h:38: // in case 3 we change value of N to 1. On 2015/09/03 22:44:16, Nicolas Zea wrote: > Doesn't case 3 denote that N's value is already 1? Yep, it should be "we change value of N to 0." Thanks for noticing. https://codereview.chromium.org/1256803002/diff/820001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service.h:60: // NL* 0_ 1_ _0 _1 00 01 10 11 On 2015/09/03 22:44:16, Nicolas Zea wrote: > What are the X and Y axes? Done. https://codereview.chromium.org/1256803002/diff/820001/chrome/browser/sync/te... File chrome/browser/sync/test/integration/two_client_password_manager_setting_migrater_service_sync_test.cc (right): https://codereview.chromium.org/1256803002/diff/820001/chrome/browser/sync/te... chrome/browser/sync/test/integration/two_client_password_manager_setting_migrater_service_sync_test.cc:103: NotifyProfileAdded(0); On 2015/09/04 00:01:40, pvalenzuela wrote: > why trigger the notifications before syncing? Profiles were added and initialized in SetupClients (https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/syn...), so after that we need to notify migration service about it is we want to have a client with migration code. It definitely make no much sense here, since no change will come from server snapshot during SyncSetup. But I not see why not to activate migration code as early as it make sense.
Hey pvalenzuela@: I have another question: I'm adding data to the fake server for both PRIORITY_PREFERENCE and PREFERENCE data types. But it looks, like client first merges local values with PRIORITY_PREFERENCE value added by me, then client turns sync off, then client merges with PREFERENCE value added by me. Is there a was to merge with values for both datatypes in a same run? P.S. Code I'm talking about is in last patch in single_client test. Thanks in advance. > > I also will add single client tests with fake server data and multiple client > test and ask you for review then. > > Thanks! > > https://codereview.chromium.org/1256803002/diff/820001/chrome/browser/passwor... > File > chrome/browser/password_manager/password_manager_setting_migrater_service.cc > (right): > > https://codereview.chromium.org/1256803002/diff/820001/chrome/browser/passwor... > chrome/browser/password_manager/password_manager_setting_migrater_service.cc:60: > initial_values->push_back(*new_pref_value); > On 2015/09/03 22:44:16, Nicolas Zea wrote: > > style_comment: it might be good to have a const int kNewPrefIndex and > > kLegacyPrefIndex defined at the top of the file which you then always use to > > access/set these values. That might avoid bugs where you accidentally swap the > > order of the two due to using push_back > I got rid of this vector. > > https://codereview.chromium.org/1256803002/diff/820001/chrome/browser/passwor... > chrome/browser/password_manager/password_manager_setting_migrater_service.cc:137: > LOG(ERROR) << "OnIsSyncingChanged"; > On 2015/09/03 22:44:16, Nicolas Zea wrote: > > All these LOG(ERROR)'s should probably be DVLOG(1) > > Sorry, I've accidentally committed it > > https://codereview.chromium.org/1256803002/diff/820001/chrome/browser/passwor... > File chrome/browser/password_manager/password_manager_setting_migrater_service.h > (right): > > https://codereview.chromium.org/1256803002/diff/820001/chrome/browser/passwor... > chrome/browser/password_manager/password_manager_setting_migrater_service.h:38: > // in case 3 we change value of N to 1. > On 2015/09/03 22:44:16, Nicolas Zea wrote: > > Doesn't case 3 denote that N's value is already 1? > > Yep, it should be "we change value of N to 0." Thanks for noticing. > > https://codereview.chromium.org/1256803002/diff/820001/chrome/browser/passwor... > chrome/browser/password_manager/password_manager_setting_migrater_service.h:60: > // NL* 0_ 1_ _0 _1 00 01 10 11 > On 2015/09/03 22:44:16, Nicolas Zea wrote: > > What are the X and Y axes? > > Done. > > https://codereview.chromium.org/1256803002/diff/820001/chrome/browser/sync/te... > File > chrome/browser/sync/test/integration/two_client_password_manager_setting_migrater_service_sync_test.cc > (right): > > https://codereview.chromium.org/1256803002/diff/820001/chrome/browser/sync/te... > chrome/browser/sync/test/integration/two_client_password_manager_setting_migrater_service_sync_test.cc:103: > NotifyProfileAdded(0); > On 2015/09/04 00:01:40, pvalenzuela wrote: > > why trigger the notifications before syncing? > > Profiles were added and initialized in SetupClients > (https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/syn...), > so after that we need to notify migration service about it is we want to have a > client with migration code. > > It definitely make no much sense here, since no change will come from server > snapshot during SyncSetup. But I not see why not to activate migration code as > early as it make sense.
Hey pvalenzuela@ and zea@: pvalenzuela@, I'm happy with multiple client tests and two client tests, but still haven't resolved the issue with single client test and feeding fake data to the server. I'll appreciate you r ideas on it. zea@, is there something on my side I can do in order to help you with understanding the reconciliation algorithm. Thanks! > I have another question: I'm adding data to the fake server for both > PRIORITY_PREFERENCE and PREFERENCE data types. > But it looks, like client first merges local values with PRIORITY_PREFERENCE > value added by me, > then client turns sync off, then client merges with PREFERENCE value added by > me. > > Is there a was to merge with values for both datatypes in a same run? > > P.S. Code I'm talking about is in last patch in single_client test. > > Thanks in advance. > > > > I also will add single client tests with fake server data and multiple client > > test and ask you for review then. > > > > Thanks! > > > > > https://codereview.chromium.org/1256803002/diff/820001/chrome/browser/passwor... > > File > > chrome/browser/password_manager/password_manager_setting_migrater_service.cc > > (right): > > > > > https://codereview.chromium.org/1256803002/diff/820001/chrome/browser/passwor... > > > chrome/browser/password_manager/password_manager_setting_migrater_service.cc:60: > > initial_values->push_back(*new_pref_value); > > On 2015/09/03 22:44:16, Nicolas Zea wrote: > > > style_comment: it might be good to have a const int kNewPrefIndex and > > > kLegacyPrefIndex defined at the top of the file which you then always use to > > > access/set these values. That might avoid bugs where you accidentally swap > the > > > order of the two due to using push_back > > I got rid of this vector. > > > > > https://codereview.chromium.org/1256803002/diff/820001/chrome/browser/passwor... > > > chrome/browser/password_manager/password_manager_setting_migrater_service.cc:137: > > LOG(ERROR) << "OnIsSyncingChanged"; > > On 2015/09/03 22:44:16, Nicolas Zea wrote: > > > All these LOG(ERROR)'s should probably be DVLOG(1) > > > > Sorry, I've accidentally committed it > > > > > https://codereview.chromium.org/1256803002/diff/820001/chrome/browser/passwor... > > File > chrome/browser/password_manager/password_manager_setting_migrater_service.h > > (right): > > > > > https://codereview.chromium.org/1256803002/diff/820001/chrome/browser/passwor... > > > chrome/browser/password_manager/password_manager_setting_migrater_service.h:38: > > // in case 3 we change value of N to 1. > > On 2015/09/03 22:44:16, Nicolas Zea wrote: > > > Doesn't case 3 denote that N's value is already 1? > > > > Yep, it should be "we change value of N to 0." Thanks for noticing. > > > > > https://codereview.chromium.org/1256803002/diff/820001/chrome/browser/passwor... > > > chrome/browser/password_manager/password_manager_setting_migrater_service.h:60: > > // NL* 0_ 1_ _0 _1 00 01 10 11 > > On 2015/09/03 22:44:16, Nicolas Zea wrote: > > > What are the X and Y axes? > > > > Done. > > > > > https://codereview.chromium.org/1256803002/diff/820001/chrome/browser/sync/te... > > File > > > chrome/browser/sync/test/integration/two_client_password_manager_setting_migrater_service_sync_test.cc > > (right): > > > > > https://codereview.chromium.org/1256803002/diff/820001/chrome/browser/sync/te... > > > chrome/browser/sync/test/integration/two_client_password_manager_setting_migrater_service_sync_test.cc:103: > > NotifyProfileAdded(0); > > On 2015/09/04 00:01:40, pvalenzuela wrote: > > > why trigger the notifications before syncing? > > > > Profiles were added and initialized in SetupClients > > > (https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/syn...), > > so after that we need to notify migration service about it is we want to have > a > > client with migration code. > > > > It definitely make no much sense here, since no change will come from server > > snapshot during SyncSetup. But I not see why not to activate migration code as > > early as it make sense.
Patchset #27 (id:840001) has been deleted
Patchset #27 (id:860001) has been deleted
Patchset #27 (id:880001) has been deleted
https://codereview.chromium.org/1256803002/diff/920001/chrome/browser/passwor... File chrome/browser/password_manager/password_manager_setting_migrater_service.h (right): https://codereview.chromium.org/1256803002/diff/920001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service.h:55: // But correct final value in this case is 0. Why is the correct value in the final case 0? Should it not be 1? I think the new synced value should have priority over the legacy value (imagine a case where in the future clients don't sync the old preference anymore). Keep in mind, default-value prefs don't sync, only non-default values, so in this case the user explicitly opted in to credentials service.
https://codereview.chromium.org/1256803002/diff/920001/chrome/browser/passwor... File chrome/browser/password_manager/password_manager_setting_migrater_service.h (right): https://codereview.chromium.org/1256803002/diff/920001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_setting_migrater_service.h:55: // But correct final value in this case is 0. > imagine a case where in the future clients don't sync the old preference anymore Yeah, so flow which breaks everything in this case would be following: * Migration was performed on clients which are online, both preferences are off. * We introduce "future" client where old pref is not syncing. * Client turns "on" new pref on one of the "future" clients. * Some old client performs a migration, which turns everything to off. I need to think about it. > Keep in mind, default-value prefs don't sync, only non-default values, so in > this case the user explicitly opted in to credentials service. As we were told by identity team, their metrics show that users are often "play" with new pref by changing toggle at passwords.google.com and given that default value is 1, it can be not opt-in but from 1->0->1 as such "play" with toggle. But it I feel that it shouldn't affect the final decision. So as I understood from the email thread if user starts Chrome as non-signed in user and then signs in, then new snapshot is pulled from server. So if new preference wasn't changed by user (e.g. at password.google.com), then this snapshot won't contain a value for the new preference?
Re: priority prefs getting processed before normal prefs. I think this is a side-effect of how sync handles these types. I'll sync up with zea@ and see if we can figure it out. https://codereview.chromium.org/1256803002/diff/920001/chrome/browser/sync/te... File chrome/browser/sync/test/integration/single_client_password_manager_setting_migrater_service_sync_test.cc (right): https://codereview.chromium.org/1256803002/diff/920001/chrome/browser/sync/te... chrome/browser/sync/test/integration/single_client_password_manager_setting_migrater_service_sync_test.cc:118: TestReconcileOnMergeDataAndStartSyncing(true, true, false, false, false); Calls like this are pretty confusing to read in the test cases. I understand that this reduces code duplication, but I would recommend splitting up the TestReconcileOnMergeDataAndStartSyncing method so it's more clear what each test case is doing. I would also recommend adding a comment whenever passing boolean params to indicate what they mean (you could also create constants for better readability). For example the test case could look like this: AssertPrefValues(true /* kCredentialsEnableService */, true /* kPasswordManagerSavingEnabled */); InjectNewValues(false /* ... */, false /* ... */); ASSERT_TRUE(SetupSync()); ASSERT_TRUE(sync_integration_test_util::AwaitCommitActivityCompletion(GetSyncService((0)))); AssertPrefValues(false /* kCredentialsEnableService */, false /* kPasswordManagerSavingEnabled */);
On 2015/09/08 19:54:09, melandory wrote: > https://codereview.chromium.org/1256803002/diff/920001/chrome/browser/passwor... > File chrome/browser/password_manager/password_manager_setting_migrater_service.h > (right): > > https://codereview.chromium.org/1256803002/diff/920001/chrome/browser/passwor... > chrome/browser/password_manager/password_manager_setting_migrater_service.h:55: > // But correct final value in this case is 0. > > imagine a case where in the future clients don't sync the old preference > anymore > Yeah, so flow which breaks everything in this case would be following: > * Migration was performed on clients which are online, both preferences are > off. > * We introduce "future" client where old pref is not syncing. > * Client turns "on" new pref on one of the "future" clients. > * Some old client performs a migration, which turns everything to off. > I need to think about it. The old client's migration shouldn't turn everything off in that case, once you sync, right? Assuming it already has the new pref value (via sync), the existence of the new pref should take priority. > > > Keep in mind, default-value prefs don't sync, only non-default values, so in > > this case the user explicitly opted in to credentials service. > As we were told by identity team, their metrics show that users are often "play" > with new pref by changing toggle at http://passwords.google.com and given that default > value is 1, it can be not opt-in but from 1->0->1 as such "play" with toggle. > But it I feel that it shouldn't affect the final decision. > > So as I understood from the email thread if user starts Chrome as non-signed in > user and then signs in, then new snapshot is pulled from server. So if new > preference wasn't changed by user (e.g. at http://password.google.com), then this > snapshot won't contain a value for the new preference? That's true for normal pref values. That said, I forgot about the fact that this can be toggled via password.google.com. I suspect it's still true here, but we should verify manually (and/or check with a server dev).
Some followup questions: 1) Do the single client tests pass? 2) Are you sure the two phases you're seeing don't include the client backup that happens before syncing? I can do some debugging tomorrow if you're still having issues.
On 2015/09/09 00:23:07, pvalenzuela wrote: > Some followup questions: > 1) Do the single client tests pass? No they don't, but I figured out what changes should be made to them. > 2) Are you sure the two phases you're seeing don't include the client backup > that happens before syncing? Fake server works as you explained, it was problem between monitor and keyboard.
On 2015/09/09 00:08:27, Nicolas Zea wrote: > On 2015/09/08 19:54:09, melandory wrote: > > > https://codereview.chromium.org/1256803002/diff/920001/chrome/browser/passwor... > > File > chrome/browser/password_manager/password_manager_setting_migrater_service.h > > (right): > > > > > https://codereview.chromium.org/1256803002/diff/920001/chrome/browser/passwor... > > > chrome/browser/password_manager/password_manager_setting_migrater_service.h:55: > > // But correct final value in this case is 0. > > > imagine a case where in the future clients don't sync the old preference > > anymore > > Yeah, so flow which breaks everything in this case would be following: > > * Migration was performed on clients which are online, both preferences are > > off. > > * We introduce "future" client where old pref is not syncing. > > * Client turns "on" new pref on one of the "future" clients. > > * Some old client performs a migration, which turns everything to off. > > I need to think about it. > > The old client's migration shouldn't turn everything off in that case, once you > sync, right? Assuming it already has the new pref value (via sync), the > existence of the new pref should take priority. > > > > > > Keep in mind, default-value prefs don't sync, only non-default values, so in > > > this case the user explicitly opted in to credentials service. > > As we were told by identity team, their metrics show that users are often > "play" > > with new pref by changing toggle at http://passwords.google.com and given that > default > > value is 1, it can be not opt-in but from 1->0->1 as such "play" with toggle. > > But it I feel that it shouldn't affect the final decision. > > > > So as I understood from the email thread if user starts Chrome as non-signed > in > > user and then signs in, then new snapshot is pulled from server. So if new > > preference wasn't changed by user (e.g. at http://password.google.com), then > this > > snapshot won't contain a value for the new preference? > > That's true for normal pref values. That said, I forgot about the fact that this > can be toggled via http://password.google.com. I suspect it's still true here, but we > should verify manually (and/or check with a server dev). I've checked, you are right it's still true. About test case we discussed: * local N=1, L=1 sync N=0, L=1 comes form sync. I still believe that it should be migrated to 0, because user has opted out from Smart Lock (e.g. at password.google.com). * local N=1, L=1 sync N=1 L=0 here user opted out of password manager, but opted in for Smart Lock. I don't think that being conservative and migrate to 0 is wrong. Since migration is guarded by experiment it gives us ability to migrate most of clients and stop migration (or not stop it completely, but just witch to propagate all changes of one pref to another pref mode), which should mitigate the issue with future clients. WDYT?
On 2015/09/09 20:54:22, melandory wrote: > On 2015/09/09 00:08:27, Nicolas Zea wrote: > > On 2015/09/08 19:54:09, melandory wrote: > > > > > > https://codereview.chromium.org/1256803002/diff/920001/chrome/browser/passwor... > > > File > > chrome/browser/password_manager/password_manager_setting_migrater_service.h > > > (right): > > > > > > > > > https://codereview.chromium.org/1256803002/diff/920001/chrome/browser/passwor... > > > > > > chrome/browser/password_manager/password_manager_setting_migrater_service.h:55: > > > // But correct final value in this case is 0. > > > > imagine a case where in the future clients don't sync the old preference > > > anymore > > > Yeah, so flow which breaks everything in this case would be following: > > > * Migration was performed on clients which are online, both preferences > are > > > off. > > > * We introduce "future" client where old pref is not syncing. > > > * Client turns "on" new pref on one of the "future" clients. > > > * Some old client performs a migration, which turns everything to off. > > > I need to think about it. > > > > The old client's migration shouldn't turn everything off in that case, once > you > > sync, right? Assuming it already has the new pref value (via sync), the > > existence of the new pref should take priority. > > > > > > > > > Keep in mind, default-value prefs don't sync, only non-default values, so > in > > > > this case the user explicitly opted in to credentials service. > > > As we were told by identity team, their metrics show that users are often > > "play" > > > with new pref by changing toggle at http://passwords.google.com and given > that > > default > > > value is 1, it can be not opt-in but from 1->0->1 as such "play" with > toggle. > > > But it I feel that it shouldn't affect the final decision. > > > > > > So as I understood from the email thread if user starts Chrome as non-signed > > in > > > user and then signs in, then new snapshot is pulled from server. So if new > > > preference wasn't changed by user (e.g. at http://password.google.com), then > > this > > > snapshot won't contain a value for the new preference? > > > > That's true for normal pref values. That said, I forgot about the fact that > this > > can be toggled via http://password.google.com. I suspect it's still true here, > but we > > should verify manually (and/or check with a server dev). > I've checked, you are right it's still true. > > About test case we discussed: > * local N=1, L=1 sync N=0, L=1 comes form sync. I still believe that it should > be migrated to 0, because user has opted out from Smart Lock (e.g. at > http://password.google.com). > * local N=1, L=1 sync N=1 L=0 here user opted out of password manager, but > opted in for Smart Lock. I don't think that being conservative and migrate to 0 > is wrong. > Since migration is guarded by experiment it gives us ability to migrate most of > clients and stop migration (or not stop it completely, but just witch to > propagate all changes of one pref to another pref mode), > which should mitigate the issue with future clients. > > WDYT? Agree with the first case, but I'm not sure I agree about the second (and migrating to 0), but that seems more like a product decision rather than a sync one. It's true that gating the migration behind a finch flag will help, as long as your fallback is to trust the new flag only. If your plan is to only have the migration on for a certain amount of time and then beyond that say that old values don't get migrated, I'm okay with that.
Patchset #29 (id:940001) has been deleted
Patchset #29 (id:960001) has been deleted
Patchset #29 (id:980001) has been deleted
Patchset #29 (id:1000001) has been deleted
Should these classes use the spelling "migrator" instead of "migrater?" It looks like migrater is an acceptable alternate spelling (https://en.wiktionary.org/wiki/migrater), but it looked wrong to me on first glance. https://codereview.chromium.org/1256803002/diff/1020001/chrome/browser/sync/t... File chrome/browser/sync/test/integration/password_manager_setting_migrater_helper.h (right): https://codereview.chromium.org/1256803002/diff/1020001/chrome/browser/sync/t... chrome/browser/sync/test/integration/password_manager_setting_migrater_helper.h:15: base::FieldTrial* MigrationFieldTrial(); should this method name be prefixed with Get? https://codereview.chromium.org/1256803002/diff/1020001/chrome/browser/sync/t... chrome/browser/sync/test/integration/password_manager_setting_migrater_helper.h:17: void SendProfileAddNotification(Profile* profile); add method comments here
On 2015/09/10 18:03:25, pvalenzuela wrote: > Should these classes use the spelling "migrator" instead of "migrater?" It looks > like migrater is an acceptable alternate spelling > (https://en.wiktionary.org/wiki/migrater), but it looked wrong to me on first > glance. Sure, I've renamed it. > https://codereview.chromium.org/1256803002/diff/1020001/chrome/browser/sync/t... > File > chrome/browser/sync/test/integration/password_manager_setting_migrater_helper.h > (right): > > https://codereview.chromium.org/1256803002/diff/1020001/chrome/browser/sync/t... > chrome/browser/sync/test/integration/password_manager_setting_migrater_helper.h:15: > base::FieldTrial* MigrationFieldTrial(); > should this method name be prefixed with Get? > > https://codereview.chromium.org/1256803002/diff/1020001/chrome/browser/sync/t... > chrome/browser/sync/test/integration/password_manager_setting_migrater_helper.h:17: > void SendProfileAddNotification(Profile* profile); > add method comments here
https://codereview.chromium.org/1256803002/diff/1020001/chrome/browser/sync/t... File chrome/browser/sync/test/integration/password_manager_setting_migrater_helper.h (right): https://codereview.chromium.org/1256803002/diff/1020001/chrome/browser/sync/t... chrome/browser/sync/test/integration/password_manager_setting_migrater_helper.h:15: base::FieldTrial* MigrationFieldTrial(); On 2015/09/10 18:03:25, pvalenzuela wrote: > should this method name be prefixed with Get? Done. https://codereview.chromium.org/1256803002/diff/1020001/chrome/browser/sync/t... chrome/browser/sync/test/integration/password_manager_setting_migrater_helper.h:17: void SendProfileAddNotification(Profile* profile); On 2015/09/10 18:03:25, pvalenzuela wrote: > add method comments here Done.
FYI I haven't reviewed the code too closely, but the general approach to handling migration LGTM modulo my concerns earlier.
melandory@chromium.org changed reviewers: + mlerman@chromium.org
mlerman@chromium.org: Please review changes in chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc
Profiles extra_parts lgtm
On 2015/09/10 21:38:42, Mike Lerman wrote: > Profiles extra_parts lgtm engedy@ nad vabr@, can you please take another look. Thanks!
This still LGTM, with a couple of nitpicks. Cheers, Vaclav https://codereview.chromium.org/1256803002/diff/1080001/chrome/browser/passwo... File chrome/browser/password_manager/password_manager_setting_migrator_service.cc (right): https://codereview.chromium.org/1256803002/diff/1080001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service.cc:81: LOG(ERROR) << "Sorry, you are not in experiment"; Please do not use LOG here, follow the guidance in http://dev.chromium.org/developers/coding-style#TOC-Logging (My best guess would be to remove the log, because not being part of the migration experiment is not an error state.) https://codereview.chromium.org/1256803002/diff/1080001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service.cc:172: // Only one value has came from sync. This value should be assigned to both nit: has came -> came https://codereview.chromium.org/1256803002/diff/1080001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service.cc:176: bool sync_new_pref_value = sync_data_[0]; optional: DCHECK_EQ(2u, sync_data_.size()); https://codereview.chromium.org/1256803002/diff/1080001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service.cc:177: bool sync_legacy_pref_value = sync_data_[sync_data_.size() - 1]; Here you already know that size() >= 1, so let's use sync_data_[1] instead. https://codereview.chromium.org/1256803002/diff/1080001/chrome/browser/passwo... File chrome/browser/password_manager/password_manager_setting_migrator_service.h (right): https://codereview.chromium.org/1256803002/diff/1080001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service.h:31: // calling to SyncService:CanSyncStart()) are migrated on a startup of a nit: a service -> the service (if you mean PasswordManagerSettingMigratorService) https://codereview.chromium.org/1256803002/diff/1080001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service.h:32: // service. This users can be in following states: typo: This -> These https://codereview.chromium.org/1256803002/diff/1080001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service.h:44: // preference on service start up (|inital_values_|) and the values that come nit: "start up" is a verb, "startup" (or "start-up", but be consistent) a noun https://codereview.chromium.org/1256803002/diff/1080001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service.h:49: // preference has a value equals to 1, new preference was registered for the nit: "has a value equals to 1" -> "has value 1" https://codereview.chromium.org/1256803002/diff/1080001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service.h:51: // which user has on a client contains new preference equals to 0 and old nit: equals -> equal "equals" is a verb form, "equal" is an adjective Also below. https://codereview.chromium.org/1256803002/diff/1080001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service.h:55: // But correct final value in this case is 0. nit: correct -> the correct https://codereview.chromium.org/1256803002/diff/1080001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service.h:57: // On the finish of model association step we derive the new values for both nit: "On the finish" -> "At the end" https://codereview.chromium.org/1256803002/diff/1080001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service.h:58: // settings using following table (first column contains local values for the nit: following -> the following https://codereview.chromium.org/1256803002/diff/1080001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service.h:59: // preferences, first row contains values which has came from sync, where _ that nit: has came -> came https://codereview.chromium.org/1256803002/diff/1080001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service.h:59: // preferences, first row contains values which has came from sync, where _ that nit: "that value doesn't came" -> "means that the value did not come" https://codereview.chromium.org/1256803002/diff/1080001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service.h:70: // changes from UI) and propagate the change to the other preference if needed. nit: propagate -> propagates https://codereview.chromium.org/1256803002/diff/1080001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service.h:72: // After service is created it waits until Profile and profile related services nit: service -> the service https://codereview.chromium.org/1256803002/diff/1080001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service.h:77: // depends on PrefServiceSyncable. Please reference http://crbug.com/522536. https://codereview.chromium.org/1256803002/diff/1080001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service.h:152: std::vector<bool> sync_data_; I would explicitly mention what index corresponds to which preference (N or L) if the size is 1, and that in case the size is 1 this distinction does not matter, because the columns in the table above are the same for _X and X_. https://codereview.chromium.org/1256803002/diff/1080001/chrome/browser/sync/t... File chrome/browser/sync/test/integration/password_manager_setting_migrator_helper.cc (right): https://codereview.chromium.org/1256803002/diff/1080001/chrome/browser/sync/t... chrome/browser/sync/test/integration/password_manager_setting_migrator_helper.cc:43: using namespace password_manager::prefs; This is forbidden by the style guide: "You may not use a using-directive to make all names from a namespace available." http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Namespaces But you can put using password_manager::prefs::kCredentialsEnableService using password_manager::prefs::kPasswordManagerSavingEnabled between lines 17 and 18. https://codereview.chromium.org/1256803002/diff/1080001/chrome/browser/sync/t... File chrome/browser/sync/test/integration/password_manager_setting_migrator_helper.h (right): https://codereview.chromium.org/1256803002/diff/1080001/chrome/browser/sync/t... chrome/browser/sync/test/integration/password_manager_setting_migrator_helper.h:17: // Sends NOTIFICATION_PROFILE_ADDED for the |profile| nit: Please put a full stop (.) at the end of the sentence. https://codereview.chromium.org/1256803002/diff/1080001/chrome/browser/sync/t... File chrome/browser/sync/test/integration/single_client_password_manager_setting_migrator_service_sync_test.cc (right): https://codereview.chromium.org/1256803002/diff/1080001/chrome/browser/sync/t... chrome/browser/sync/test/integration/single_client_password_manager_setting_migrator_service_sync_test.cc:135: ASSERT_TRUE(SetupClients()); optional: All the test cases seem to follow an identical pattern. Extracting the parameters to an array of a test case structures as you do in other tests might improve the readability of the test. https://codereview.chromium.org/1256803002/diff/1080001/components/password_m... File components/password_manager/core/common/password_manager_pref_names.h (right): https://codereview.chromium.org/1256803002/diff/1080001/components/password_m... components/password_manager/core/common/password_manager_pref_names.h:28: // of these two preference are in sync one with another. nit: "in sync one with another" -> "in sync with each other" or just "in sync"
chrome/browser/sync/test/integration lgtm after you fix the two pending comments from vabr@ (comment punctuation and the using declarations) https://codereview.chromium.org/1256803002/diff/1080001/chrome/browser/sync/t... File chrome/browser/sync/test/integration/single_client_password_manager_setting_migrator_service_sync_test.cc (right): https://codereview.chromium.org/1256803002/diff/1080001/chrome/browser/sync/t... chrome/browser/sync/test/integration/single_client_password_manager_setting_migrator_service_sync_test.cc:135: ASSERT_TRUE(SetupClients()); On 2015/09/11 12:44:50, vabr (Chromium) wrote: > optional: All the test cases seem to follow an identical pattern. Extracting the > parameters to an array of a test case structures as you do in other tests might > improve the readability of the test. I think I had recommended the current style of these tests instead of each having a single TestFoo() method with a lot of params. I think these cases, as is, can be easily modified later on.
https://codereview.chromium.org/1256803002/diff/1080001/chrome/browser/sync/t... File chrome/browser/sync/test/integration/single_client_password_manager_setting_migrator_service_sync_test.cc (right): https://codereview.chromium.org/1256803002/diff/1080001/chrome/browser/sync/t... chrome/browser/sync/test/integration/single_client_password_manager_setting_migrator_service_sync_test.cc:135: ASSERT_TRUE(SetupClients()); On 2015/09/11 20:26:12, pvalenzuela wrote: > On 2015/09/11 12:44:50, vabr (Chromium) wrote: > > optional: All the test cases seem to follow an identical pattern. Extracting > the > > parameters to an array of a test case structures as you do in other tests > might > > improve the readability of the test. > > I think I had recommended the current style of these tests instead of each > having a single TestFoo() method with a lot of params. I think these cases, as > is, can be easily modified later on. Fair enough, it's fine with me to leave it as is.
https://codereview.chromium.org/1256803002/diff/1080001/chrome/browser/passwo... File chrome/browser/password_manager/password_manager_setting_migrator_service.cc (right): https://codereview.chromium.org/1256803002/diff/1080001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service.cc:81: LOG(ERROR) << "Sorry, you are not in experiment"; On 2015/09/11 12:44:49, vabr (Chromium) wrote: > Please do not use LOG here, follow the guidance in > http://dev.chromium.org/developers/coding-style#TOC-Logging > > (My best guess would be to remove the log, because not being part of the > migration experiment is not an error state.) Sorry. I still was debugging and decided not to remove with the comment in the email that I will remove it afterwards, but forgot to mention it. https://codereview.chromium.org/1256803002/diff/1080001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service.cc:172: // Only one value has came from sync. This value should be assigned to both On 2015/09/11 12:44:49, vabr (Chromium) wrote: > nit: has came -> came Done. https://codereview.chromium.org/1256803002/diff/1080001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service.cc:176: bool sync_new_pref_value = sync_data_[0]; On 2015/09/11 12:44:49, vabr (Chromium) wrote: > optional: DCHECK_EQ(2u, sync_data_.size()); It's not nessary 2: it can be 4 N=1 change has case from sync => sync_data_ = {1,1} L=0 change has case from sync => sync_data_ = {1,1,0,0} https://codereview.chromium.org/1256803002/diff/1080001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service.cc:177: bool sync_legacy_pref_value = sync_data_[sync_data_.size() - 1]; On 2015/09/11 12:44:49, vabr (Chromium) wrote: > Here you already know that size() >= 1, so let's use sync_data_[1] instead. No, I need first and last, in case when both values came from sync and sync_data_ has size of 4, where first and last elements corresponds to N and L https://codereview.chromium.org/1256803002/diff/1080001/chrome/browser/passwo... File chrome/browser/password_manager/password_manager_setting_migrator_service.h (right): https://codereview.chromium.org/1256803002/diff/1080001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service.h:31: // calling to SyncService:CanSyncStart()) are migrated on a startup of a On 2015/09/11 12:44:49, vabr (Chromium) wrote: > nit: a service -> the service > (if you mean PasswordManagerSettingMigratorService) Done. https://codereview.chromium.org/1256803002/diff/1080001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service.h:32: // service. This users can be in following states: On 2015/09/11 12:44:49, vabr (Chromium) wrote: > typo: This -> These Done. https://codereview.chromium.org/1256803002/diff/1080001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service.h:44: // preference on service start up (|inital_values_|) and the values that come On 2015/09/11 12:44:49, vabr (Chromium) wrote: > nit: "start up" is a verb, "startup" (or "start-up", but be consistent) a noun Done. https://codereview.chromium.org/1256803002/diff/1080001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service.h:49: // preference has a value equals to 1, new preference was registered for the On 2015/09/11 12:44:49, vabr (Chromium) wrote: > nit: "has a value equals to 1" -> "has value 1" Done. https://codereview.chromium.org/1256803002/diff/1080001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service.h:51: // which user has on a client contains new preference equals to 0 and old On 2015/09/11 12:44:50, vabr (Chromium) wrote: > nit: equals -> equal > "equals" is a verb form, "equal" is an adjective > Also below. Done. https://codereview.chromium.org/1256803002/diff/1080001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service.h:55: // But correct final value in this case is 0. On 2015/09/11 12:44:49, vabr (Chromium) wrote: > nit: correct -> the correct Done. https://codereview.chromium.org/1256803002/diff/1080001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service.h:57: // On the finish of model association step we derive the new values for both On 2015/09/11 12:44:49, vabr (Chromium) wrote: > nit: "On the finish" -> "At the end" Done. https://codereview.chromium.org/1256803002/diff/1080001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service.h:58: // settings using following table (first column contains local values for the On 2015/09/11 12:44:49, vabr (Chromium) wrote: > nit: following -> the following Done. https://codereview.chromium.org/1256803002/diff/1080001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service.h:59: // preferences, first row contains values which has came from sync, where _ that On 2015/09/11 12:44:49, vabr (Chromium) wrote: > nit: "that value doesn't came" -> "means that the value did not come" Done. https://codereview.chromium.org/1256803002/diff/1080001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service.h:59: // preferences, first row contains values which has came from sync, where _ that On 2015/09/11 12:44:49, vabr (Chromium) wrote: > nit: has came -> came Done. https://codereview.chromium.org/1256803002/diff/1080001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service.h:70: // changes from UI) and propagate the change to the other preference if needed. On 2015/09/11 12:44:49, vabr (Chromium) wrote: > nit: propagate -> propagates Done. https://codereview.chromium.org/1256803002/diff/1080001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service.h:72: // After service is created it waits until Profile and profile related services On 2015/09/11 12:44:49, vabr (Chromium) wrote: > nit: service -> the service Done. https://codereview.chromium.org/1256803002/diff/1080001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service.h:77: // depends on PrefServiceSyncable. On 2015/09/11 12:44:49, vabr (Chromium) wrote: > Please reference http://crbug.com/522536. Done. https://codereview.chromium.org/1256803002/diff/1080001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service.h:152: std::vector<bool> sync_data_; On 2015/09/11 12:44:49, vabr (Chromium) wrote: > I would explicitly mention what index corresponds to which preference (N or L) > if the size is 1, and that in case the size is 1 this distinction does not > matter, because the columns in the table above are the same for _X and X_. Done. https://codereview.chromium.org/1256803002/diff/1080001/chrome/browser/sync/t... File chrome/browser/sync/test/integration/password_manager_setting_migrator_helper.cc (right): https://codereview.chromium.org/1256803002/diff/1080001/chrome/browser/sync/t... chrome/browser/sync/test/integration/password_manager_setting_migrator_helper.cc:43: using namespace password_manager::prefs; On 2015/09/11 12:44:50, vabr (Chromium) wrote: > This is forbidden by the style guide: > "You may not use a using-directive to make all names from a namespace > available." > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Namespaces > > But you can put > using password_manager::prefs::kCredentialsEnableService > using password_manager::prefs::kPasswordManagerSavingEnabled > between lines 17 and 18. Done. https://codereview.chromium.org/1256803002/diff/1080001/chrome/browser/sync/t... File chrome/browser/sync/test/integration/password_manager_setting_migrator_helper.h (right): https://codereview.chromium.org/1256803002/diff/1080001/chrome/browser/sync/t... chrome/browser/sync/test/integration/password_manager_setting_migrator_helper.h:17: // Sends NOTIFICATION_PROFILE_ADDED for the |profile| On 2015/09/11 12:44:50, vabr (Chromium) wrote: > nit: Please put a full stop (.) at the end of the sentence. Done. https://codereview.chromium.org/1256803002/diff/1080001/components/password_m... File components/password_manager/core/common/password_manager_pref_names.h (right): https://codereview.chromium.org/1256803002/diff/1080001/components/password_m... components/password_manager/core/common/password_manager_pref_names.h:28: // of these two preference are in sync one with another. On 2015/09/11 12:44:50, vabr (Chromium) wrote: > nit: "in sync one with another" -> "in sync with each other" or just "in sync" Done.
Patchset #33 (id:1100001) has been deleted
Patchset #33 (id:1120001) has been deleted
Hi Tanja, I still did not understand the sync_data_ values (as you could tell from my misguided comments in the last round). Could you please do one more pass through its declaration comment? Otherwise LGTM. Cheers, Vaclav https://codereview.chromium.org/1256803002/diff/1170001/chrome/browser/passwo... File chrome/browser/password_manager/password_manager_setting_migrator_service.h (right): https://codereview.chromium.org/1256803002/diff/1170001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service.h:149: // Contains values which has came from sync during model association step. nit: has came -> have come https://codereview.chromium.org/1256803002/diff/1170001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service.h:150: // After both preferences data types are associated, new value for the both nit: preferences -> preferences' https://codereview.chromium.org/1256803002/diff/1170001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service.h:152: // kCredentialsEnableService, last index corresponds to I still don't understand, in particular the vector seems to be twice as long as I expected. Maybe an example would help. https://codereview.chromium.org/1256803002/diff/1170001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service.h:153: // kPasswordManagerSavingEnabled. If |sync_data_| has size 2, then if the size "then if the size" looks out of place
battre@chromium.org changed reviewers: + battre@chromium.org
some drive-by comments (did not do a full review) https://codereview.chromium.org/1256803002/diff/1170001/chrome/browser/passwo... File chrome/browser/password_manager/password_manager_setting_migrator_service.cc (right): https://codereview.chromium.org/1256803002/diff/1170001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service.cc:43: if (legacy_pref_value != new_value) { Why do you perform this check instead of always calling prefs->SetBoolean() ? https://codereview.chromium.org/1256803002/diff/1170001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service.cc:105: PasswordManagerSettingMigratorService::MigrateOffState(prefs); My feeling is that MigrateOffState() does not belong into InitObservers(). https://codereview.chromium.org/1256803002/diff/1170001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service.cc:190: // static Nit: These should go to the top of the file. The function order should roughly correspond to the header file. https://codereview.chromium.org/1256803002/diff/1170001/chrome/browser/passwo... File chrome/browser/password_manager/password_manager_setting_migrator_service.h (right): https://codereview.chromium.org/1256803002/diff/1170001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service.h:159: bool initial_new_pref_value_; these booleans need to be initialized in the constructor because they don't have an implicit default constructor. https://codereview.chromium.org/1256803002/diff/1170001/chrome/browser/sync/t... File chrome/browser/sync/test/integration/password_manager_setting_migrator_helper.h (right): https://codereview.chromium.org/1256803002/diff/1170001/chrome/browser/sync/t... chrome/browser/sync/test/integration/password_manager_setting_migrator_helper.h:22: void AssertPrefValuesOnClient(int index, I think that some documentation of this function would be helpful. https://codereview.chromium.org/1256803002/diff/1170001/components/password_m... File components/password_manager/core/common/password_manager_pref_names.h (right): https://codereview.chromium.org/1256803002/diff/1170001/components/password_m... components/password_manager/core/common/password_manager_pref_names.h:24: // The value of this preference controls whenever Password Manager will save s/whenever/whether the/ https://codereview.chromium.org/1256803002/diff/1170001/components/password_m... components/password_manager/core/common/password_manager_pref_names.h:31: extern const char kCredentialsEnableService[]; Should this be named kCredentialsServiceEnabled ?
Patchset #37 (id:1210001) has been deleted
Patchset #34 (id:1150001) has been deleted
Patchset #35 (id:1190001) has been deleted
Patchset #35 (id:1230001) has been deleted
https://codereview.chromium.org/1256803002/diff/1170001/chrome/browser/passwo... File chrome/browser/password_manager/password_manager_setting_migrator_service.cc (right): https://codereview.chromium.org/1256803002/diff/1170001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service.cc:43: if (legacy_pref_value != new_value) { On 2015/09/14 19:41:26, battre wrote: > Why do you perform this check instead of always calling prefs->SetBoolean() ? I think that in case when pref doesn't have a user set value, the preference change notification will be fired even if new value equals to default one. And I wanted to prevent it. But thanks for catching this, current version of algorithm doesn't require this. https://codereview.chromium.org/1256803002/diff/1170001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service.cc:105: PasswordManagerSettingMigratorService::MigrateOffState(prefs); On 2015/09/14 19:41:26, battre wrote: > My feeling is that MigrateOffState() does not belong into InitObservers(). Done. https://codereview.chromium.org/1256803002/diff/1170001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service.cc:190: // static On 2015/09/14 19:41:26, battre wrote: > Nit: These should go to the top of the file. The function order should roughly > correspond to the header file. Done. https://codereview.chromium.org/1256803002/diff/1170001/chrome/browser/passwo... File chrome/browser/password_manager/password_manager_setting_migrator_service.h (right): https://codereview.chromium.org/1256803002/diff/1170001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service.h:152: // kCredentialsEnableService, last index corresponds to On 2015/09/14 16:22:34, vabr (Chromium) wrote: > I still don't understand, in particular the vector seems to be twice as long as > I expected. Maybe an example would help. Done. https://codereview.chromium.org/1256803002/diff/1170001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service.h:159: bool initial_new_pref_value_; On 2015/09/14 19:41:26, battre wrote: > these booleans need to be initialized in the constructor because they don't have > an implicit default constructor. Done. https://codereview.chromium.org/1256803002/diff/1170001/chrome/browser/sync/t... File chrome/browser/sync/test/integration/password_manager_setting_migrator_helper.h (right): https://codereview.chromium.org/1256803002/diff/1170001/chrome/browser/sync/t... chrome/browser/sync/test/integration/password_manager_setting_migrator_helper.h:22: void AssertPrefValuesOnClient(int index, On 2015/09/14 19:41:26, battre wrote: > I think that some documentation of this function would be helpful. Done. https://codereview.chromium.org/1256803002/diff/1170001/components/password_m... File components/password_manager/core/common/password_manager_pref_names.h (right): https://codereview.chromium.org/1256803002/diff/1170001/components/password_m... components/password_manager/core/common/password_manager_pref_names.h:24: // The value of this preference controls whenever Password Manager will save On 2015/09/14 19:41:27, battre wrote: > s/whenever/whether the/ Done. https://codereview.chromium.org/1256803002/diff/1170001/components/password_m... components/password_manager/core/common/password_manager_pref_names.h:31: extern const char kCredentialsEnableService[]; On 2015/09/14 19:41:26, battre wrote: > Should this be named kCredentialsServiceEnabled ? I would keep current name, because it matches pref name "credentials_enable_service".
https://codereview.chromium.org/1256803002/diff/1170001/components/password_m... File components/password_manager/core/common/password_manager_pref_names.h (right): https://codereview.chromium.org/1256803002/diff/1170001/components/password_m... components/password_manager/core/common/password_manager_pref_names.h:31: extern const char kCredentialsEnableService[]; On 2015/09/15 07:23:17, melandory wrote: > On 2015/09/14 19:41:26, battre wrote: > > Should this be named kCredentialsServiceEnabled ? > > I would keep current name, because it matches pref name > "credentials_enable_service". Oh, is that the preference that exists already? In that case I would say it was poorly chosen but we should keep it.
https://codereview.chromium.org/1256803002/diff/1170001/components/password_m... File components/password_manager/core/common/password_manager_pref_names.h (right): https://codereview.chromium.org/1256803002/diff/1170001/components/password_m... components/password_manager/core/common/password_manager_pref_names.h:31: extern const char kCredentialsEnableService[]; On 2015/09/15 07:32:56, battre wrote: > On 2015/09/15 07:23:17, melandory wrote: > > On 2015/09/14 19:41:26, battre wrote: > > > Should this be named kCredentialsServiceEnabled ? > > > > I would keep current name, because it matches pref name > > "credentials_enable_service". > > Oh, is that the preference that exists already? In that case I would say it was > poorly chosen but we should keep it. Yes: https://cs.corp.google.com/#piper///depot/google3/java/com/google/security/ls...
This is as far as I got today, I'll look at the Sync tests tomorrow. https://codereview.chromium.org/1256803002/diff/1280001/chrome/browser/passwo... File chrome/browser/password_manager/password_manager_setting_migrator_service.cc (right): https://codereview.chromium.org/1256803002/diff/1280001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service.cc:23: void ChangeOnePrefBecauseAnotherPrefHasChanged( Nit: Do we really need this? The functionality of this seems to be subsumed by UpdatePreferencesValuesIfNeeded apart from the metrics and checking if a value exists. https://codereview.chromium.org/1256803002/diff/1280001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service.cc:27: bool changed_pref = prefs->GetBoolean(changed_pref_name.c_str()); You will need to use GetUserPrefValue() and GetDefaultPrefValue() so that if an enterprise policy is controlling this preference, we will still work with the user pref value (that the sync logic itself also uses). In a few places in this file. https://codereview.chromium.org/1256803002/diff/1280001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service.cc:44: void InitCurrentPrefState(PrefService* prefs, nit: I'd suggest renaming this to SaveCurrentPrefValues() to better indicate that this is a read-only operation. https://codereview.chromium.org/1256803002/diff/1280001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service.cc:141: return sync_service_ && sync_service_->CanSyncStart(); Do we need to check here if the 'preferences' data type is actually synced? https://codereview.chromium.org/1256803002/diff/1280001/chrome/browser/passwo... File chrome/browser/password_manager/password_manager_setting_migrator_service.h (right): https://codereview.chromium.org/1256803002/diff/1280001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service.h:77: // depends on PrefServiceSyncable http://crbug.com/522536. nit: ". See: https:// ..." https://codereview.chromium.org/1256803002/diff/1280001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service.h:115: virtual bool CanSyncStart(); nit: Can this be made protected? https://codereview.chromium.org/1256803002/diff/1280001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service.h:143: PrefChangeRegistrar pref_change_registrar_; It's usually best practice to move all registrar definitions to the very end, so they are destroyed first when the instance is destroyed, and there is absolutely no chance of getting notifications to a half-destroyed instance. https://codereview.chromium.org/1256803002/diff/1280001/chrome/browser/passwo... File chrome/browser/password_manager/password_manager_setting_migrator_service_unittests.cc (right): https://codereview.chromium.org/1256803002/diff/1280001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service_unittests.cc:31: #include "sync/protocol/sync.pb.h" nit: I think this includes the above two already. https://codereview.chromium.org/1256803002/diff/1280001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service_unittests.cc:38: const char kEnabledGroupName[] = "PasswordManagerSettingsMigration.Enable"; Just use "Enable", What I meant earlier is that, semantically speaking, the group name is implicitly understood to be scoped within the field trial so no further qualification is necessary. Might need to change it elsewhere too. https://codereview.chromium.org/1256803002/diff/1280001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service_unittests.cc:40: enum PrefState { naming nit: BooleanPrefState to be more descriptive nit: Seems like a nice candidate for a scoped enumeration (enum class). https://codereview.chromium.org/1256803002/diff/1280001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service_unittests.cc:43: EMPTY, // datatype bucket is empty nit: For simplicity, I'd suggest removing this fourth state unless it is really needed, see my comment below. https://codereview.chromium.org/1256803002/diff/1280001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service_unittests.cc:67: void SetPrefValueIfNeeded(PrefService* prefs, naming nit: SetupLocalPrefState() to be more descriptive nit: I'd suggest to make this a member of the testing fixture, as it is very simple and that way it could figure out the |prefs| itself. https://codereview.chromium.org/1256803002/diff/1280001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service_unittests.cc:69: PrefState value) { nit: s/value/state/ https://codereview.chromium.org/1256803002/diff/1280001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service_unittests.cc:74: } nit: To make sure we are actually testing with unset values else if (value == UNSET) ASSERT_TRUE(prefs->FindPreference(name)->IsDefaultValue()); or else if (value == UNSET) prefs->ClearPref(name); Depending on whether the ASSERT is always satisfied. https://codereview.chromium.org/1256803002/diff/1280001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service_unittests.cc:80: void StartSyncing(PrefServiceSyncable* prefs, Based on whether we can optimize out one on UNSET/EMPTY, I'd suggest merging this method into StartSyncingPrefWithValue() for simplicity. https://codereview.chromium.org/1256803002/diff/1280001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service_unittests.cc:84: if (name == password_manager::prefs::kPasswordManagerSavingEnabled) nit: Lines 84--87 could go into StartSyncingPrefWithValue(), and this could take |type| directly. (Ignore if we are merging the two methods.) https://codereview.chromium.org/1256803002/diff/1280001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service_unittests.cc:96: void StartSyncingPrefWithValue(PrefServiceSyncable* prefs, nit: For simplicity, how about just naming this StartSyncingPref() if we can merge the above method into this? StartSyncing(..., syncer::SyncDataList()) is a sub-case of this with value == EMPTY. https://codereview.chromium.org/1256803002/diff/1280001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service_unittests.cc:98: PrefState value) { nit: s/value/pref_state_in_sync/ to be more descriptive https://codereview.chromium.org/1256803002/diff/1280001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service_unittests.cc:99: if (value == UNSET) Could you please remind me under which circumstances does this happen in reality? https://codereview.chromium.org/1256803002/diff/1280001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service_unittests.cc:101: if (value == EMPTY) { This only happens if the user has just started syncing, or if this is the first time the new preference is introduced to Sync, right? https://codereview.chromium.org/1256803002/diff/1280001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service_unittests.cc:115: public: I think most, if not all, public members of this class can become protected. https://codereview.chromium.org/1256803002/diff/1280001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service_unittests.cc:126: EXPECT_CALL(*profile_sync_service(), CanSyncStart()) nit: Either change this ON_CALL (so that the mock service is used as a stub, i.e., to return a canned value and we don't care if it's called), or add expectations as to how many times it needs to be called (so that it is a real mock). See: https://code.google.com/p/googlemock/wiki/ForDummies#Cardinalities:_How_Many_... https://code.google.com/p/googlemock/wiki/CookBook#Knowing_When_to_Expect https://codereview.chromium.org/1256803002/diff/1280001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service_unittests.cc:128: base::FieldTrialList::CreateFieldTrial(kFieldTrialName, kEnabledGroupName); Haven't looked at the sync test yet, but we should have a test that makes sure nothing happens if the user is not in the enabled experiment group. https://codereview.chromium.org/1256803002/diff/1280001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service_unittests.cc:131: void AssertPrefValues(bool new_pref_value, bool old_pref_value) { nit: ExpectBothPrefValuesAreEqualTo(bool) to be more descriptive -- it is using the EXPECT macros and is always called with both arguments being the same. The terminology is that ASSERT is "EXPECT + also abort the test if it fails", and usually used to: (1) verify (and document) that certain preconditions for a test case are (must be) satisfied, (2) in cases when the test would crash after an EXPECT failure (e.g., checking array size, then values at given indices). https://codereview.chromium.org/1256803002/diff/1280001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service_unittests.cc:151: NotifyProfileAdded(); We set both preferences to !value here to make sure the we have good testing coverage. https://codereview.chromium.org/1256803002/diff/1280001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service_unittests.cc:218: struct { nit: const https://codereview.chromium.org/1256803002/diff/1280001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service_unittests.cc:224: } testing_table[] = { nit: kTestingTable https://codereview.chromium.org/1256803002/diff/1280001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service_unittests.cc:242: << test_case.old_pref_local_value << " " nit: In other places, the order of these two prefs are always (new, old), let's be consistent with that. https://codereview.chromium.org/1256803002/diff/1280001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service_unittests.cc:263: EXPECT_CALL(*profile_sync_service(), CanSyncStart()) nit: Same here, please see my rant above about ON_CALL. https://codereview.chromium.org/1256803002/diff/1280001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service_unittests.cc:273: EXPECT_EQ(prefs->GetBoolean(prefs::kCredentialsEnableService), true); nit: ASSERT_EQ, as this is a precondition for the test, see my rant above. :) https://codereview.chromium.org/1256803002/diff/1280001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service_unittests.cc:274: EXPECT_CALL(*profile_sync_service(), CanSyncStart()) nit: Same here.
Patchset #38 (id:1300001) has been deleted
Patchset #38 (id:1320001) has been deleted
Patchset #38 (id:1340001) has been deleted
Patchset #38 (id:1360001) has been deleted
Patchset #38 (id:1380001) has been deleted
Patchset #38 (id:1400001) has been deleted
https://codereview.chromium.org/1256803002/diff/1280001/chrome/browser/passwo... File chrome/browser/password_manager/password_manager_setting_migrator_service.cc (right): https://codereview.chromium.org/1256803002/diff/1280001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service.cc:23: void ChangeOnePrefBecauseAnotherPrefHasChanged( On 2015/09/15 19:37:20, engedy wrote: > Nit: Do we really need this? The functionality of this seems to be subsumed by > UpdatePreferencesValuesIfNeeded apart from the metrics and checking if a value > exists. I don't like to skip check here, so I'd rather leave it as it is. https://codereview.chromium.org/1256803002/diff/1280001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service.cc:27: bool changed_pref = prefs->GetBoolean(changed_pref_name.c_str()); On 2015/09/15 19:37:20, engedy wrote: > You will need to use GetUserPrefValue() and GetDefaultPrefValue() so that if an > enterprise policy is controlling this preference, we will still work with the > user pref value (that the sync logic itself also uses). In a few places in this > file. I see, so you're proposing that we perform migration on the "User value level", so if policy removed, then user falls back to migrated values. I have a concern about this approach, but I prefer to discuss offline. https://codereview.chromium.org/1256803002/diff/1280001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service.cc:44: void InitCurrentPrefState(PrefService* prefs, On 2015/09/15 19:37:20, engedy wrote: > nit: I'd suggest renaming this to SaveCurrentPrefValues() to better indicate > that this is a read-only operation. Done. https://codereview.chromium.org/1256803002/diff/1280001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service.cc:141: return sync_service_ && sync_service_->CanSyncStart(); On 2015/09/15 19:37:20, engedy wrote: > Do we need to check here if the 'preferences' data type is actually synced? Yep, it should be checked. https://codereview.chromium.org/1256803002/diff/1280001/chrome/browser/passwo... File chrome/browser/password_manager/password_manager_setting_migrator_service.h (right): https://codereview.chromium.org/1256803002/diff/1280001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service.h:77: // depends on PrefServiceSyncable http://crbug.com/522536. On 2015/09/15 19:37:20, engedy wrote: > nit: ". See: https:// ..." Done. https://codereview.chromium.org/1256803002/diff/1280001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service.h:115: virtual bool CanSyncStart(); On 2015/09/15 19:37:20, engedy wrote: > nit: Can this be made protected? Done. https://codereview.chromium.org/1256803002/diff/1280001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service.h:143: PrefChangeRegistrar pref_change_registrar_; On 2015/09/15 19:37:20, engedy wrote: > It's usually best practice to move all registrar definitions to the very end, so > they are destroyed first when the instance is destroyed, and there is absolutely > no chance of getting notifications to a half-destroyed instance. Done. https://codereview.chromium.org/1256803002/diff/1280001/chrome/browser/passwo... File chrome/browser/password_manager/password_manager_setting_migrator_service_unittests.cc (right): https://codereview.chromium.org/1256803002/diff/1280001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service_unittests.cc:31: #include "sync/protocol/sync.pb.h" On 2015/09/15 19:37:21, engedy wrote: > nit: I think this includes the above two already. Done. https://codereview.chromium.org/1256803002/diff/1280001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service_unittests.cc:38: const char kEnabledGroupName[] = "PasswordManagerSettingsMigration.Enable"; On 2015/09/15 19:37:21, engedy wrote: > Just use "Enable", What I meant earlier is that, semantically speaking, the > group name is implicitly understood to be scoped within the field trial so no > further qualification is necessary. > > Might need to change it elsewhere too. Done. https://codereview.chromium.org/1256803002/diff/1280001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service_unittests.cc:43: EMPTY, // datatype bucket is empty On 2015/09/15 19:37:21, engedy wrote: > nit: For simplicity, I'd suggest removing this fourth state unless it is really > needed, see my comment below. Done. https://codereview.chromium.org/1256803002/diff/1280001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service_unittests.cc:67: void SetPrefValueIfNeeded(PrefService* prefs, On 2015/09/15 19:37:21, engedy wrote: > naming nit: SetupLocalPrefState() to be more descriptive > nit: I'd suggest to make this a member of the testing fixture, as it is very > simple and that way it could figure out the |prefs| itself. Done. https://codereview.chromium.org/1256803002/diff/1280001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service_unittests.cc:69: PrefState value) { On 2015/09/15 19:37:21, engedy wrote: > nit: s/value/state/ Done. https://codereview.chromium.org/1256803002/diff/1280001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service_unittests.cc:74: } On 2015/09/15 19:37:20, engedy wrote: > nit: To make sure we are actually testing with unset values > > else if (value == UNSET) > ASSERT_TRUE(prefs->FindPreference(name)->IsDefaultValue()); > > or > > else if (value == UNSET) > prefs->ClearPref(name); > > Depending on whether the ASSERT is always satisfied. > Done. https://codereview.chromium.org/1256803002/diff/1280001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service_unittests.cc:80: void StartSyncing(PrefServiceSyncable* prefs, On 2015/09/15 19:37:21, engedy wrote: > Based on whether we can optimize out one on UNSET/EMPTY, I'd suggest merging > this method into StartSyncingPrefWithValue() for simplicity. Done. https://codereview.chromium.org/1256803002/diff/1280001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service_unittests.cc:84: if (name == password_manager::prefs::kPasswordManagerSavingEnabled) On 2015/09/15 19:37:21, engedy wrote: > nit: Lines 84--87 could go into StartSyncingPrefWithValue(), and this could take > |type| directly. (Ignore if we are merging the two methods.) Done. https://codereview.chromium.org/1256803002/diff/1280001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service_unittests.cc:96: void StartSyncingPrefWithValue(PrefServiceSyncable* prefs, On 2015/09/15 19:37:21, engedy wrote: > nit: For simplicity, how about just naming this StartSyncingPref() if we can > merge the above method into this? StartSyncing(..., syncer::SyncDataList()) is a > sub-case of this with value == EMPTY. Done. https://codereview.chromium.org/1256803002/diff/1280001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service_unittests.cc:98: PrefState value) { On 2015/09/15 19:37:20, engedy wrote: > nit: s/value/pref_state_in_sync/ to be more descriptive Done. https://codereview.chromium.org/1256803002/diff/1280001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service_unittests.cc:99: if (value == UNSET) On 2015/09/15 19:37:21, engedy wrote: > Could you please remind me under which circumstances does this happen in > reality? It shouldn't happen for the MergeDataAndStartSyncing step in reality, so I remove this one as you suggested. https://codereview.chromium.org/1256803002/diff/1280001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service_unittests.cc:101: if (value == EMPTY) { On 2015/09/15 19:37:21, engedy wrote: > This only happens if the user has just started syncing, or if this is the first > time the new preference is introduced to Sync, right? If there was no change to the pref value, then it doesn't arrive from sync. https://codereview.chromium.org/1256803002/diff/1280001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service_unittests.cc:115: public: On 2015/09/15 19:37:21, engedy wrote: > I think most, if not all, public members of this class can become protected. Done. https://codereview.chromium.org/1256803002/diff/1280001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service_unittests.cc:126: EXPECT_CALL(*profile_sync_service(), CanSyncStart()) On 2015/09/15 19:37:21, engedy wrote: > nit: Either change this ON_CALL (so that the mock service is used as a stub, > i.e., to return a canned value and we don't care if it's called), or add > expectations as to how many times it needs to be called (so that it is a real > mock). > > See: > https://code.google.com/p/googlemock/wiki/ForDummies#Cardinalities:_How_Many_... > https://code.google.com/p/googlemock/wiki/CookBook#Knowing_When_to_Expect Done. https://codereview.chromium.org/1256803002/diff/1280001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service_unittests.cc:128: base::FieldTrialList::CreateFieldTrial(kFieldTrialName, kEnabledGroupName); On 2015/09/15 19:37:21, engedy wrote: > Haven't looked at the sync test yet, but we should have a test that makes sure > nothing happens if the user is not in the enabled experiment group. I want to have it as separate CL (I actually already have implemented it), actually not only this change, but also some modifications we need to do for mobile (because current algorithm and test needs some tweaking in order to be able to work correctly there) plus metrics as two separate CLs. https://codereview.chromium.org/1256803002/diff/1280001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service_unittests.cc:131: void AssertPrefValues(bool new_pref_value, bool old_pref_value) { On 2015/09/15 19:37:21, engedy wrote: > nit: ExpectBothPrefValuesAreEqualTo(bool) to be more descriptive -- it is using > the EXPECT macros and is always called with both arguments being the same. > > The terminology is that ASSERT is "EXPECT + also abort the test if it fails", > and usually used to: > (1) verify (and document) that certain preconditions for a test case are (must > be) satisfied, > (2) in cases when the test would crash after an EXPECT failure (e.g., checking > array size, then values at given indices). I'll keep two parameters, because tests for case when user is not migration require call to this function with two distinct arguments. https://codereview.chromium.org/1256803002/diff/1280001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service_unittests.cc:151: NotifyProfileAdded(); On 2015/09/15 19:37:21, engedy wrote: > We set both preferences to !value here to make sure the we have good testing > coverage. Done. https://codereview.chromium.org/1256803002/diff/1280001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service_unittests.cc:218: struct { On 2015/09/15 19:37:20, engedy wrote: > nit: const Done. https://codereview.chromium.org/1256803002/diff/1280001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service_unittests.cc:224: } testing_table[] = { On 2015/09/15 19:37:21, engedy wrote: > nit: kTestingTable Done. https://codereview.chromium.org/1256803002/diff/1280001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service_unittests.cc:242: << test_case.old_pref_local_value << " " On 2015/09/15 19:37:21, engedy wrote: > nit: In other places, the order of these two prefs are always (new, old), let's > be consistent with that. Done. https://codereview.chromium.org/1256803002/diff/1280001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service_unittests.cc:263: EXPECT_CALL(*profile_sync_service(), CanSyncStart()) On 2015/09/15 19:37:21, engedy wrote: > nit: Same here, please see my rant above about ON_CALL. Done. https://codereview.chromium.org/1256803002/diff/1280001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service_unittests.cc:273: EXPECT_EQ(prefs->GetBoolean(prefs::kCredentialsEnableService), true); On 2015/09/15 19:37:20, engedy wrote: > nit: ASSERT_EQ, as this is a precondition for the test, see my rant above. :) Done. https://codereview.chromium.org/1256803002/diff/1280001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service_unittests.cc:274: EXPECT_CALL(*profile_sync_service(), CanSyncStart()) On 2015/09/15 19:37:20, engedy wrote: > nit: Same here. Done.
Patchset #41 (id:1480001) has been deleted
Looking good! I am doing a final pass on tests to make sure all interesting cases are caught. https://codereview.chromium.org/1256803002/diff/1280001/chrome/browser/passwo... File chrome/browser/password_manager/password_manager_setting_migrator_service.cc (right): https://codereview.chromium.org/1256803002/diff/1280001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service.cc:23: void ChangeOnePrefBecauseAnotherPrefHasChanged( On 2015/09/16 21:39:44, melandory wrote: > On 2015/09/15 19:37:20, engedy wrote: > > Nit: Do we really need this? The functionality of this seems to be subsumed by > > UpdatePreferencesValuesIfNeeded apart from the metrics and checking if a value > > exists. > > I don't like to skip check here, so I'd rather leave it as it is. Acknowledged. https://codereview.chromium.org/1256803002/diff/1280001/chrome/browser/passwo... File chrome/browser/password_manager/password_manager_setting_migrator_service.h (right): https://codereview.chromium.org/1256803002/diff/1280001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service.h:77: // depends on PrefServiceSyncable http://crbug.com/522536. On 2015/09/16 21:39:45, melandory wrote: > On 2015/09/15 19:37:20, engedy wrote: > > nit: ". See: https:// ..." > > Done. Please also insert ". See: ". https://codereview.chromium.org/1256803002/diff/1280001/chrome/browser/passwo... File chrome/browser/password_manager/password_manager_setting_migrator_service_unittests.cc (right): https://codereview.chromium.org/1256803002/diff/1280001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service_unittests.cc:99: if (value == UNSET) On 2015/09/16 21:39:46, melandory wrote: > On 2015/09/15 19:37:21, engedy wrote: > > Could you please remind me under which circumstances does this happen in > > reality? > > It shouldn't happen for the MergeDataAndStartSyncing step in reality, so I > remove this one as you suggested. It can happen on mobile, though, where we are not syncing normal prefs, right? Is this is what you meant when you were talking about mobile changes in a follow-up CL? https://codereview.chromium.org/1256803002/diff/1280001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service_unittests.cc:101: if (value == EMPTY) { On 2015/09/16 21:39:45, melandory wrote: > On 2015/09/15 19:37:21, engedy wrote: > > This only happens if the user has just started syncing, or if this is the > first > > time the new preference is introduced to Sync, right? > > If there was no change to the pref value, then it doesn't arrive from sync. But in that case there would be no pref change notifications fired anyway, right? https://codereview.chromium.org/1256803002/diff/1280001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service_unittests.cc:128: base::FieldTrialList::CreateFieldTrial(kFieldTrialName, kEnabledGroupName); On 2015/09/16 21:39:46, melandory wrote: > On 2015/09/15 19:37:21, engedy wrote: > > Haven't looked at the sync test yet, but we should have a test that makes sure > > nothing happens if the user is not in the enabled experiment group. > > I want to have it as separate CL (I actually already have implemented it), > actually not only this change, but also some modifications we need to do for > mobile (because current algorithm and test needs some tweaking in order to be > able to work correctly there) plus metrics as two separate CLs. Acknowledged. https://codereview.chromium.org/1256803002/diff/1280001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service_unittests.cc:131: void AssertPrefValues(bool new_pref_value, bool old_pref_value) { On 2015/09/16 21:39:45, melandory wrote: > On 2015/09/15 19:37:21, engedy wrote: > > nit: ExpectBothPrefValuesAreEqualTo(bool) to be more descriptive -- it is > using > > the EXPECT macros and is always called with both arguments being the same. > > > > The terminology is that ASSERT is "EXPECT + also abort the test if it fails", > > and usually used to: > > (1) verify (and document) that certain preconditions for a test case are > (must > > be) satisfied, > > (2) in cases when the test would crash after an EXPECT failure (e.g., > checking > > array size, then values at given indices). > > I'll keep two parameters, because tests for case when user is not migration > require call to this function with two distinct arguments. Acknowledged. https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/passwo... File chrome/browser/password_manager/password_manager_setting_migrator_service.cc (right): https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service.cc:17: #include "components/sync_driver/sync_service.h" nit: Is this needed? https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service.cc:106: registrar_.Add(this, chrome::NOTIFICATION_PROFILE_ADDED, Add comment: // If there will be a ProfileSyncService, the rest of the initialization should take place after that service is created. The ProfileSyncService is normally created before NOTIFICATION_PROFILE_ADDED, so defer the rest of the initialization until after that. https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service.cc:127: if (!CanSyncStart()) { nit: No braces. https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service.cc:217: bool sync_legacy_pref_value = sync_data_[sync_data_.size() - 1]; nit: sync_data_.back(); https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/passwo... File chrome/browser/password_manager/password_manager_setting_migrator_service.h (right): https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service.h:31: // calling to SyncService:CanSyncStart()) are migrated on a startup of the nit: on startup https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service.h:35: // 3. N = 1, L = 0 I think the fourth state is now also possible given that we start syncing for everyone but hold back the migration logic using an experiment. https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service.h:39: // The service also observes changes to both preferences, so if one of the nit: Merge this with the third-to-last paragraph. https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service.h:50: // first time and has default value which is also 1, the sync data snapshot nit: This example here suggests that having a different pair of settings in the Sync DB than in the PrefStore on MergeDataAndStartSyncing is a common case. Should we use the example N=0 L=1 instead? (Note that this can happen now.) https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service.h:65: // 01 x x x x x x x x # impossible state I think this is no longer impossible given the experiment. https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service.h:72: // After the service is created it waits until Profile and profile related nit: On second thought, this should go to the CC file above the registration. I think only the PROFILE_ADDED notification requires clarification. https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service.h:143: // The vector minimum size is 0 and the vector maximum size is 3: nit: 4 https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service.h:150: // from sync, e.g. local state is 10 and 11 came from sync. nit: Shouldn't this be 10 and 01? Or is this referring to the case when the new preference still had the default value? https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service.h:153: // to 3, otherwise the vector contains the value only for one preference. nit: 4 https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service.h:157: bool initial_new_pref_value_; These values should be initialized in the constructor. You may initialize them inline here as well if you prefer. https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/passwo... File chrome/browser/password_manager/password_manager_setting_migrator_service_unittests.cc (right): https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service_unittests.cc:178: TEST_F(PasswordManagerSettingMigratorServiceTest, nit: Is it possible to express this test method as a test case below? https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/sync/t... File chrome/browser/sync/test/integration/multiple_client_password_manager_setting_migrator_service_sync_test.cc (right): https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/sync/t... chrome/browser/sync/test/integration/multiple_client_password_manager_setting_migrator_service_sync_test.cc:5: #include "base/metrics/field_trial.h" nit: I think the first 4 includes are not needed. https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/sync/t... chrome/browser/sync/test/integration/multiple_client_password_manager_setting_migrator_service_sync_test.cc:14: #include "chrome/common/pref_names.h" nit: This includes the one below. Do we need anything other than password manager prefs? https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/sync/t... chrome/browser/sync/test/integration/multiple_client_password_manager_setting_migrator_service_sync_test.cc:16: #include "components/syncable_prefs/pref_service_syncable.h" nit: I think these 5 includes are no longer needed here. https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/sync/t... chrome/browser/sync/test/integration/multiple_client_password_manager_setting_migrator_service_sync_test.cc:24: using preferences_helper::BooleanPrefMatches; nit: Not used. https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/sync/t... chrome/browser/sync/test/integration/multiple_client_password_manager_setting_migrator_service_sync_test.cc:26: using preferences_helper::GetPrefs; nit: Not used. https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/sync/t... chrome/browser/sync/test/integration/multiple_client_password_manager_setting_migrator_service_sync_test.cc:43: trial_ = password_manager_setting_migrater_helper::GetMigrationFieldTrial(); Given this is specific to one client, this should go to SetupClients() or to SetUp(). https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/sync/t... chrome/browser/sync/test/integration/multiple_client_password_manager_setting_migrator_service_sync_test.cc:62: EnsureMigrationStartsForClient(0); This test intentionally executes MergeDataAndStartSyncing before the migration logic is activated, right? https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/sync/t... chrome/browser/sync/test/integration/multiple_client_password_manager_setting_migrator_service_sync_test.cc:68: ASSERT_TRUE(AwaitBooleanPrefMatches( Is this intentionally repeated? If so, could you please add a comment why this is needed? https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/sync/t... File chrome/browser/sync/test/integration/password_manager_setting_migrator_helper.cc (right): https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/sync/t... chrome/browser/sync/test/integration/password_manager_setting_migrator_helper.cc:7: #include "base/metrics/field_trial.h" Need to include field_trial_list.h, and I think field_trial.h itself is not needed. https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/sync/t... chrome/browser/sync/test/integration/password_manager_setting_migrator_helper.cc:12: #include "chrome/browser/sync/test/integration/sync_test.h" nit: I think this include is not needed. https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/sync/t... chrome/browser/sync/test/integration/password_manager_setting_migrator_helper.cc:29: base::FieldTrial* GetMigrationFieldTrial() { nit: SetupFieldTrial(). nit2: You can just ignore the return value (you might want to check that it is non-null), as it is registered into the global singleton FieldTrialList which will hold a reference to it anyway. https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/sync/t... chrome/browser/sync/test/integration/password_manager_setting_migrator_helper.cc:45: ASSERT_EQ(new_pref_value, nit: ASSERT_EQ has a shortcoming that it only aborts the function it is in when there is a failure (it basically has a "return;" in the macro definition), but not enclosing functions (i.e., not the entire test if it is not in the uppermost test method). You could either replace it with EXPECT and rename the method to ExpectPrefValuesOnClient() if it is acceptable that the test continues, or repeatedly enclose each call to this function into an ASSERT_NO_FATAL_FAILURE(...) macro, until you bubble up to the main test method. https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/sync/t... File chrome/browser/sync/test/integration/password_manager_setting_migrator_helper.h (right): https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/sync/t... chrome/browser/sync/test/integration/password_manager_setting_migrator_helper.h:15: base::FieldTrial* GetMigrationFieldTrial(); phrasing nit: // Enables the password manager setting migration field trial. https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/sync/t... chrome/browser/sync/test/integration/password_manager_setting_migrator_helper.h:18: // PasswordManagerSettingMigrater observes this notification and registers nit: s/and registers/to register/ https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/sync/t... chrome/browser/sync/test/integration/password_manager_setting_migrator_helper.h:23: // kCredentialsEnableService equals to |new_pref_value| and the value for nit: either "equals" or "is equal to" https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/sync/t... File chrome/browser/sync/test/integration/single_client_password_manager_setting_migrator_service_sync_test.cc (right): https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/sync/t... chrome/browser/sync/test/integration/single_client_password_manager_setting_migrator_service_sync_test.cc:5: #include "base/guid.h" nit: Is this needed? https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/sync/t... chrome/browser/sync/test/integration/single_client_password_manager_setting_migrator_service_sync_test.cc:7: #include "base/metrics/field_trial.h" nit: No longer needed. https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/sync/t... chrome/browser/sync/test/integration/single_client_password_manager_setting_migrator_service_sync_test.cc:9: #include "base/test/mock_entropy_provider.h" nit: I don't think this is needed. https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/sync/t... chrome/browser/sync/test/integration/single_client_password_manager_setting_migrator_service_sync_test.cc:10: #include "chrome/browser/chrome_notification_types.h" nit: I don't think this is needed. https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/sync/t... chrome/browser/sync/test/integration/single_client_password_manager_setting_migrator_service_sync_test.cc:11: #include "chrome/browser/password_manager/password_manager_setting_migrator_service.h" nit: I don't think this is needed. https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/sync/t... chrome/browser/sync/test/integration/single_client_password_manager_setting_migrator_service_sync_test.cc:15: #include "chrome/browser/sync/test/integration/profile_sync_service_harness.h" nit: Is this needed? https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/sync/t... chrome/browser/sync/test/integration/single_client_password_manager_setting_migrator_service_sync_test.cc:18: #include "chrome/common/pref_names.h" nit: I don't think this is needed. https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/sync/t... chrome/browser/sync/test/integration/single_client_password_manager_setting_migrator_service_sync_test.cc:21: #include "content/public/browser/notification_details.h" nit: These 4 are not needed. https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/sync/t... chrome/browser/sync/test/integration/single_client_password_manager_setting_migrator_service_sync_test.cc:24: #include "content/public/test/test_browser_thread_bundle.h" nit: I think this is not needed. https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/sync/t... chrome/browser/sync/test/integration/single_client_password_manager_setting_migrator_service_sync_test.cc:25: #include "sync/internal_api/public/base/model_type.h" nit: I think this is not needed. https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/sync/t... chrome/browser/sync/test/integration/single_client_password_manager_setting_migrator_service_sync_test.cc:28: #include "sync/protocol/sync.pb.h" nit: I think this includes the above two. https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/sync/t... chrome/browser/sync/test/integration/single_client_password_manager_setting_migrator_service_sync_test.cc:32: using preferences_helper::AwaitBooleanPrefMatches; nit: This and the below 2 are not used. https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/sync/t... chrome/browser/sync/test/integration/single_client_password_manager_setting_migrator_service_sync_test.cc:35: using preferences_helper::GetPrefs; nit: This is only used in one place. As a rule of thumb, avoid using declarations unless they are saving vertical lines. https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/sync/t... chrome/browser/sync/test/integration/single_client_password_manager_setting_migrator_service_sync_test.cc:39: void InjectDataToFakeServer(fake_server::FakeServer* fake_server, nit: InjectPreferenceValueToFakeServer https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/sync/t... chrome/browser/sync/test/integration/single_client_password_manager_setting_migrator_service_sync_test.cc:43: JSONStringValueSerializer json(&serialized); nit: You could simplify this a tiny bit by using JSONWriter::Write(). Not really sure why we have both utilities, though. https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/sync/t... chrome/browser/sync/test/integration/single_client_password_manager_setting_migrator_service_sync_test.cc:109: AssertPrefValues(new_pref_local_value, old_pref_local_value); I think this is the only usage of AssertPrefValues() where we want to abort after failure. https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/sync/t... chrome/browser/sync/test/integration/single_client_password_manager_setting_migrator_service_sync_test.cc:113: using namespace password_manager::prefs; nit: Note that the using-directive is banned even inside functions by the style guide: http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Namespaces You could use the using-declaration to create an alias to the namespace within the function, though. https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/sync/t... chrome/browser/sync/test/integration/single_client_password_manager_setting_migrator_service_sync_test.cc:121: trial_ = password_manager_setting_migrater_helper::GetMigrationFieldTrial(); Same here with the field trial: no need to hold on to the return value. https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/sync/t... chrome/browser/sync/test/integration/single_client_password_manager_setting_migrator_service_sync_test.cc:152: ASSERT_TRUE(SetupClients()); nit: (although this is the default, to make this more readable) SetLocalPrefValues(true /* kCredentialsEnableService */, true /* kPasswordManagerSavingEnabled */) https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/sync/t... chrome/browser/sync/test/integration/single_client_password_manager_setting_migrator_service_sync_test.cc:243: IN_PROC_BROWSER_TEST_F( I think it would be useful to add: * false-true, false-true (this will be the case for all users for whom we'll enable the migration later on through the field trial mechanism and who has opted out of the new settting). * false-false, false-false (to check that values are preserved in steady state) * true-true, true-true (to check that values are preserved in steady state) https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/sync/t... File chrome/browser/sync/test/integration/two_client_password_manager_setting_migrator_service_sync_test.cc (right): https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/sync/t... chrome/browser/sync/test/integration/two_client_password_manager_setting_migrator_service_sync_test.cc:5: #include "base/metrics/field_trial.h" nit: Please remove unused includes and using declarations in this file too. https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/sync/t... chrome/browser/sync/test/integration/two_client_password_manager_setting_migrator_service_sync_test.cc:50: // Check that changed pref is equals on both clients nit: s/is equals/has the same value/ nit: Given that we are checking both explicitly, do we need the first check? https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/sync/t... chrome/browser/sync/test/integration/two_client_password_manager_setting_migrator_service_sync_test.cc:56: ~TwoClientsPasswordManagerSettingMigratorServiceSyncTest() override {} nit: Move DTOR to the after the CTOR. https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/sync/t... chrome/browser/sync/test/integration/two_client_password_manager_setting_migrator_service_sync_test.cc:59: trial_ = password_manager_setting_migrater_helper::GetMigrationFieldTrial(); nit: Same thing with field trial here. https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/sync/t... chrome/browser/sync/test/integration/two_client_password_manager_setting_migrator_service_sync_test.cc:64: void ExpectValueOnBothClientsForBothPreference(bool value) { nit: Make Expect/Assert terminology usage consistent. https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/sync/t... chrome/browser/sync/test/integration/two_client_password_manager_setting_migrator_service_sync_test.cc:71: bool TestUsesSelfNotifications() override { return false; } Would this be something that we might want to set in other tests too? https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/sync/t... chrome/browser/sync/test/integration/two_client_password_manager_setting_migrator_service_sync_test.cc:90: ASSERT_TRUE(SetupClients()); nit: SetupSync and remove that call later? https://codereview.chromium.org/1256803002/diff/1500001/components/password_m... File components/password_manager/core/browser/password_manager_settings_migration_experiment.cc (left): https://codereview.chromium.org/1256803002/diff/1500001/components/password_m... components/password_manager/core/browser/password_manager_settings_migration_experiment.cc:14: return base::FieldTrialList::FindFullName(kFieldTrialName) == I think we should change this to prefix matching, so that it also accepts Enable2, Enable3, etc. In the past, it proved useful to have multiple groups with the same behavior. I don't have anything specific in mind, though. base::StartsWith(base::FieldTrialList::FindFullName(kFieldTrialName), "Enable", base::CompareCase::INSENSITIVE_ASCII);
LGTM % open comments. Thank you for your heroic effort on driving this CL to fruition! https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/passwo... File chrome/browser/password_manager/password_manager_setting_migrator_service_unittests.cc (right): https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service_unittests.cc:204: bool result_value; We should add the no-longer-impossible scenarios here as well. https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service_unittests.cc:251: TEST_F(PasswordManagerSettingMigratorServiceTest, Should we have a version of this with just default values for both prefs? That is probably the case for most users not syncing. https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/sync/t... File chrome/browser/sync/test/integration/multiple_client_password_manager_setting_migrator_service_sync_test.cc (right): https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/sync/t... chrome/browser/sync/test/integration/multiple_client_password_manager_setting_migrator_service_sync_test.cc:88: ChangeBooleanPref(0, password_manager::prefs::kPasswordManagerSavingEnabled); Not sure I understand this test. Shouldn't at least one (or not both) of the 0s as first arguments be 2s instead? https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/sync/t... File chrome/browser/sync/test/integration/single_client_password_manager_setting_migrator_service_sync_test.cc (right): https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/sync/t... chrome/browser/sync/test/integration/single_client_password_manager_setting_migrator_service_sync_test.cc:77: prefs->RemoveObserver(this); I'd rather unsubscribe in the DTOR (after checking there is a non-null profile and service) to make sure there isn't a use-after-free in case this gets destroyed early on due to a test failure. https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/sync/t... chrome/browser/sync/test/integration/single_client_password_manager_setting_migrator_service_sync_test.cc:137: SetLocalPrefValues(true /* kCredentialsEnableService */, To clarify, does that data being injected through InjectDataToFakeServer() get sent down to the client in MergeDataAndStartSyncing, or in subsequent ProcessSyncChanges? If the former, then that local and sync values are different can only happen if the user has just set up Sync, right? If so, should we reorder these test cases and let CanSyncStart return false first, and then call SetupSync to simulate a real world situation better? What I'd like to somehow verify here is that changes made during the MergeDataAndStartSycing step will end up in Sync (so it would get synced to legacy clients)? (Happy to discuss offline.) https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/sync/t... chrome/browser/sync/test/integration/single_client_password_manager_setting_migrator_service_sync_test.cc:152: ASSERT_TRUE(SetupClients()); On 2015/09/17 15:15:57, engedy wrote: > nit: (although this is the default, to make this more readable) > > SetLocalPrefValues(true /* kCredentialsEnableService */, > true /* kPasswordManagerSavingEnabled */) On the other hand, having tests with just default pref values might have merit too. https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/sync/t... File chrome/browser/sync/test/integration/two_client_password_manager_setting_migrator_service_sync_test.cc (right): https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/sync/t... chrome/browser/sync/test/integration/two_client_password_manager_setting_migrator_service_sync_test.cc:140: } Could we also add a sanity check test here with the special case we've talked about: 1 migrating and 1 legacy client, initial state NL=11, then the legacy client would do (without doing the waiting methods): 1.) N=0 2.) L=0 3.) N=1 Basically, I'd like to ensure that if two conflicting changes arrive through ProcessSyncChanges in the same batch, we still do the right thing, and this gets Synced to the legacy client.
Patchset #42 (id:1520001) has been deleted
The CQ bit was checked by melandory@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mlerman@chromium.org, zea@chromium.org, pvalenzuela@chromium.org, vabr@chromium.org, engedy@chromium.org Link to the patchset: https://codereview.chromium.org/1256803002/#ps1390017 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1256803002/1390017 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1256803002/1390017
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Patchset #42 (id:1390017) has been deleted
Patchset #42 (id:1550001) has been deleted
Patchset #42 (id:1570001) has been deleted
The CQ bit was checked by melandory@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pvalenzuela@chromium.org, mlerman@chromium.org, engedy@chromium.org, vabr@chromium.org, zea@chromium.org Link to the patchset: https://codereview.chromium.org/1256803002/#ps1590001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1256803002/1590001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1256803002/1590001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
Patchset #43 (id:1610001) has been deleted
Patchset #42 (id:1590001) has been deleted
The CQ bit was checked by melandory@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pvalenzuela@chromium.org, mlerman@chromium.org, engedy@chromium.org, vabr@chromium.org, zea@chromium.org Link to the patchset: https://codereview.chromium.org/1256803002/#ps1630001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1256803002/1630001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1256803002/1630001
Message was sent while issue was closed.
Committed patchset #42 (id:1630001)
Message was sent while issue was closed.
Patchset 42 (id:??) landed as https://crrev.com/fac58ec68f43cb21f1dc6a18e8878e03a63797fc Cr-Commit-Position: refs/heads/master@{#350162}
Message was sent while issue was closed.
https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/passwo... File chrome/browser/password_manager/password_manager_setting_migrator_service.cc (right): https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service.cc:17: #include "components/sync_driver/sync_service.h" On 2015/09/17 15:15:55, engedy wrote: > nit: Is this needed? Done. https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service.cc:106: registrar_.Add(this, chrome::NOTIFICATION_PROFILE_ADDED, On 2015/09/17 15:15:55, engedy wrote: > Add comment: > > // If there will be a ProfileSyncService, the rest of the initialization should > take place after that service is created. The ProfileSyncService is normally > created before NOTIFICATION_PROFILE_ADDED, so defer the rest of the > initialization until after that. Done. https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service.cc:127: if (!CanSyncStart()) { On 2015/09/17 15:15:55, engedy wrote: > nit: No braces. Done. https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service.cc:217: bool sync_legacy_pref_value = sync_data_[sync_data_.size() - 1]; On 2015/09/17 15:15:55, engedy wrote: > nit: sync_data_.back(); Done. https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/passwo... File chrome/browser/password_manager/password_manager_setting_migrator_service.h (right): https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service.h:31: // calling to SyncService:CanSyncStart()) are migrated on a startup of the On 2015/09/17 15:15:55, engedy wrote: > nit: on startup Done. https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service.h:35: // 3. N = 1, L = 0 On 2015/09/17 15:15:55, engedy wrote: > I think the fourth state is now also possible given that we start syncing for > everyone but hold back the migration logic using an experiment. Done. https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service.h:39: // The service also observes changes to both preferences, so if one of the On 2015/09/17 15:15:55, engedy wrote: > nit: Merge this with the third-to-last paragraph. Done. https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service.h:65: // 01 x x x x x x x x # impossible state On 2015/09/17 15:15:55, engedy wrote: > I think this is no longer impossible given the experiment. Done. https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service.h:72: // After the service is created it waits until Profile and profile related On 2015/09/17 15:15:55, engedy wrote: > nit: On second thought, this should go to the CC file above the registration. I > think only the PROFILE_ADDED notification requires clarification. Done. https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service.h:143: // The vector minimum size is 0 and the vector maximum size is 3: On 2015/09/17 15:15:55, engedy wrote: > nit: 4 Done. https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service.h:150: // from sync, e.g. local state is 10 and 11 came from sync. On 2015/09/17 15:15:55, engedy wrote: > nit: Shouldn't this be 10 and 01? Or is this referring to the case when the new > preference still had the default value? I meant 11 and 01 with default value for N. when N=O arrives from sync, we will add 0 and 0 to sync_data, then for L = 1 we will add 1 and 1 to sync data. https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service.h:153: // to 3, otherwise the vector contains the value only for one preference. On 2015/09/17 15:15:55, engedy wrote: > nit: 4 Done. https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service.h:157: bool initial_new_pref_value_; On 2015/09/17 15:15:55, engedy wrote: > These values should be initialized in the constructor. You may initialize them > inline here as well if you prefer. I'm initializing them in SaveCurrentPrefState in constructor. https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/passwo... File chrome/browser/password_manager/password_manager_setting_migrator_service_unittests.cc (right): https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service_unittests.cc:178: TEST_F(PasswordManagerSettingMigratorServiceTest, On 2015/09/17 15:15:55, engedy wrote: > nit: Is it possible to express this test method as a test case below? Done. https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service_unittests.cc:204: bool result_value; On 2015/09/17 17:41:21, engedy wrote: > We should add the no-longer-impossible scenarios here as well. Done. https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/passwo... chrome/browser/password_manager/password_manager_setting_migrator_service_unittests.cc:251: TEST_F(PasswordManagerSettingMigratorServiceTest, On 2015/09/17 17:41:21, engedy wrote: > Should we have a version of this with just default values for both prefs? That > is probably the case for most users not syncing. Done. https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/sync/t... File chrome/browser/sync/test/integration/multiple_client_password_manager_setting_migrator_service_sync_test.cc (right): https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/sync/t... chrome/browser/sync/test/integration/multiple_client_password_manager_setting_migrator_service_sync_test.cc:5: #include "base/metrics/field_trial.h" On 2015/09/17 15:15:56, engedy wrote: > nit: I think the first 4 includes are not needed. Done. https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/sync/t... chrome/browser/sync/test/integration/multiple_client_password_manager_setting_migrator_service_sync_test.cc:14: #include "chrome/common/pref_names.h" On 2015/09/17 15:15:55, engedy wrote: > nit: This includes the one below. Do we need anything other than password > manager prefs? Done. https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/sync/t... chrome/browser/sync/test/integration/multiple_client_password_manager_setting_migrator_service_sync_test.cc:16: #include "components/syncable_prefs/pref_service_syncable.h" On 2015/09/17 15:15:56, engedy wrote: > nit: I think these 5 includes are no longer needed here. Done. https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/sync/t... chrome/browser/sync/test/integration/multiple_client_password_manager_setting_migrator_service_sync_test.cc:24: using preferences_helper::BooleanPrefMatches; On 2015/09/17 15:15:56, engedy wrote: > nit: Not used. Done. https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/sync/t... chrome/browser/sync/test/integration/multiple_client_password_manager_setting_migrator_service_sync_test.cc:26: using preferences_helper::GetPrefs; On 2015/09/17 15:15:55, engedy wrote: > nit: Not used. Done. https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/sync/t... chrome/browser/sync/test/integration/multiple_client_password_manager_setting_migrator_service_sync_test.cc:43: trial_ = password_manager_setting_migrater_helper::GetMigrationFieldTrial(); On 2015/09/17 15:15:56, engedy wrote: > Given this is specific to one client, this should go to SetupClients() or to > SetUp(). Done. https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/sync/t... chrome/browser/sync/test/integration/multiple_client_password_manager_setting_migrator_service_sync_test.cc:62: EnsureMigrationStartsForClient(0); On 2015/09/17 15:15:56, engedy wrote: > This test intentionally executes MergeDataAndStartSyncing before the migration > logic is activated, right? Yes, single client tests are used for MergeDataAndStartSyncing testing. https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/sync/t... chrome/browser/sync/test/integration/multiple_client_password_manager_setting_migrator_service_sync_test.cc:68: ASSERT_TRUE(AwaitBooleanPrefMatches( On 2015/09/17 15:15:55, engedy wrote: > Is this intentionally repeated? If so, could you please add a comment why this > is needed? Nope, not intentional. https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/sync/t... chrome/browser/sync/test/integration/multiple_client_password_manager_setting_migrator_service_sync_test.cc:88: ChangeBooleanPref(0, password_manager::prefs::kPasswordManagerSavingEnabled); On 2015/09/17 17:41:21, engedy wrote: > Not sure I understand this test. Shouldn't at least one (or not both) of the 0s > as first arguments be 2s instead? Done. https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/sync/t... File chrome/browser/sync/test/integration/password_manager_setting_migrator_helper.cc (right): https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/sync/t... chrome/browser/sync/test/integration/password_manager_setting_migrator_helper.cc:7: #include "base/metrics/field_trial.h" On 2015/09/17 15:15:56, engedy wrote: > Need to include field_trial_list.h, and I think field_trial.h itself is not > needed. field_trial_list.h exists only for android, base::FieldTrialList::CreateFieldTrial declared in field_trial.h https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/sync/t... chrome/browser/sync/test/integration/password_manager_setting_migrator_helper.cc:12: #include "chrome/browser/sync/test/integration/sync_test.h" On 2015/09/17 15:15:56, engedy wrote: > nit: I think this include is not needed. Done. https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/sync/t... chrome/browser/sync/test/integration/password_manager_setting_migrator_helper.cc:29: base::FieldTrial* GetMigrationFieldTrial() { On 2015/09/17 15:15:56, engedy wrote: > nit: SetupFieldTrial(). > nit2: You can just ignore the return value (you might want to check that it is > non-null), as it is registered into the global singleton FieldTrialList which > will hold a reference to it anyway. Done. https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/sync/t... chrome/browser/sync/test/integration/password_manager_setting_migrator_helper.cc:45: ASSERT_EQ(new_pref_value, On 2015/09/17 15:15:56, engedy wrote: > nit: ASSERT_EQ has a shortcoming that it only aborts the function it is in when > there is a failure (it basically has a "return;" in the macro definition), but > not enclosing functions (i.e., not the entire test if it is not in the uppermost > test method). > > You could either replace it with EXPECT and rename the method to > ExpectPrefValuesOnClient() if it is acceptable that the test continues, or > repeatedly enclose each call to this function into an > ASSERT_NO_FATAL_FAILURE(...) macro, until you bubble up to the main test method. Done. https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/sync/t... File chrome/browser/sync/test/integration/password_manager_setting_migrator_helper.h (right): https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/sync/t... chrome/browser/sync/test/integration/password_manager_setting_migrator_helper.h:15: base::FieldTrial* GetMigrationFieldTrial(); On 2015/09/17 15:15:56, engedy wrote: > phrasing nit: > > // Enables the password manager setting migration field trial. Done. https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/sync/t... chrome/browser/sync/test/integration/password_manager_setting_migrator_helper.h:18: // PasswordManagerSettingMigrater observes this notification and registers On 2015/09/17 15:15:56, engedy wrote: > nit: s/and registers/to register/ Done. https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/sync/t... chrome/browser/sync/test/integration/password_manager_setting_migrator_helper.h:23: // kCredentialsEnableService equals to |new_pref_value| and the value for On 2015/09/17 15:15:56, engedy wrote: > nit: either "equals" or "is equal to" Done. https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/sync/t... File chrome/browser/sync/test/integration/single_client_password_manager_setting_migrator_service_sync_test.cc (right): https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/sync/t... chrome/browser/sync/test/integration/single_client_password_manager_setting_migrator_service_sync_test.cc:5: #include "base/guid.h" On 2015/09/17 15:15:56, engedy wrote: > nit: Is this needed? Done. https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/sync/t... chrome/browser/sync/test/integration/single_client_password_manager_setting_migrator_service_sync_test.cc:7: #include "base/metrics/field_trial.h" On 2015/09/17 15:15:56, engedy wrote: > nit: No longer needed. Needed, I hold pointer to base::FieldTrial as class member. https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/sync/t... chrome/browser/sync/test/integration/single_client_password_manager_setting_migrator_service_sync_test.cc:9: #include "base/test/mock_entropy_provider.h" On 2015/09/17 15:15:56, engedy wrote: > nit: I don't think this is needed. Done. https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/sync/t... chrome/browser/sync/test/integration/single_client_password_manager_setting_migrator_service_sync_test.cc:10: #include "chrome/browser/chrome_notification_types.h" On 2015/09/17 15:15:57, engedy wrote: > nit: I don't think this is needed. Done. https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/sync/t... chrome/browser/sync/test/integration/single_client_password_manager_setting_migrator_service_sync_test.cc:11: #include "chrome/browser/password_manager/password_manager_setting_migrator_service.h" On 2015/09/17 15:15:56, engedy wrote: > nit: I don't think this is needed. Done. https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/sync/t... chrome/browser/sync/test/integration/single_client_password_manager_setting_migrator_service_sync_test.cc:15: #include "chrome/browser/sync/test/integration/profile_sync_service_harness.h" On 2015/09/17 15:15:57, engedy wrote: > nit: Is this needed? Done. https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/sync/t... chrome/browser/sync/test/integration/single_client_password_manager_setting_migrator_service_sync_test.cc:18: #include "chrome/common/pref_names.h" On 2015/09/17 15:15:57, engedy wrote: > nit: I don't think this is needed. Done. https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/sync/t... chrome/browser/sync/test/integration/single_client_password_manager_setting_migrator_service_sync_test.cc:21: #include "content/public/browser/notification_details.h" On 2015/09/17 15:15:57, engedy wrote: > nit: These 4 are not needed. Done. https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/sync/t... chrome/browser/sync/test/integration/single_client_password_manager_setting_migrator_service_sync_test.cc:24: #include "content/public/test/test_browser_thread_bundle.h" On 2015/09/17 15:15:56, engedy wrote: > nit: I think this is not needed. Done. https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/sync/t... chrome/browser/sync/test/integration/single_client_password_manager_setting_migrator_service_sync_test.cc:25: #include "sync/internal_api/public/base/model_type.h" On 2015/09/17 15:15:56, engedy wrote: > nit: I think this is not needed. Done. https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/sync/t... chrome/browser/sync/test/integration/single_client_password_manager_setting_migrator_service_sync_test.cc:28: #include "sync/protocol/sync.pb.h" On 2015/09/17 15:15:56, engedy wrote: > nit: I think this includes the above two. Done. https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/sync/t... chrome/browser/sync/test/integration/single_client_password_manager_setting_migrator_service_sync_test.cc:32: using preferences_helper::AwaitBooleanPrefMatches; On 2015/09/17 15:15:56, engedy wrote: > nit: This and the below 2 are not used. Done. https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/sync/t... chrome/browser/sync/test/integration/single_client_password_manager_setting_migrator_service_sync_test.cc:35: using preferences_helper::GetPrefs; On 2015/09/17 15:15:57, engedy wrote: > nit: This is only used in one place. As a rule of thumb, avoid using > declarations unless they are saving vertical lines. Done. https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/sync/t... chrome/browser/sync/test/integration/single_client_password_manager_setting_migrator_service_sync_test.cc:39: void InjectDataToFakeServer(fake_server::FakeServer* fake_server, On 2015/09/17 15:15:57, engedy wrote: > nit: InjectPreferenceValueToFakeServer Done. https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/sync/t... chrome/browser/sync/test/integration/single_client_password_manager_setting_migrator_service_sync_test.cc:43: JSONStringValueSerializer json(&serialized); On 2015/09/17 15:15:56, engedy wrote: > nit: You could simplify this a tiny bit by using JSONWriter::Write(). Not really > sure why we have both utilities, though. Done. https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/sync/t... chrome/browser/sync/test/integration/single_client_password_manager_setting_migrator_service_sync_test.cc:77: prefs->RemoveObserver(this); On 2015/09/17 17:41:21, engedy wrote: > I'd rather unsubscribe in the DTOR (after checking there is a non-null profile > and service) to make sure there isn't a use-after-free in case this gets > destroyed early on due to a test failure. Added TODO, given that !prefs->IsSyncing() && !prefs->IsPrioritySyncing() happens at least twice, this change is requires more logic, than just moving RemoveObserver to DTOR. https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/sync/t... chrome/browser/sync/test/integration/single_client_password_manager_setting_migrator_service_sync_test.cc:109: AssertPrefValues(new_pref_local_value, old_pref_local_value); On 2015/09/17 15:15:57, engedy wrote: > I think this is the only usage of AssertPrefValues() where we want to abort > after failure. But it also shoudn't harm if we don't, we'll just get more unfulfilled conditions. https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/sync/t... chrome/browser/sync/test/integration/single_client_password_manager_setting_migrator_service_sync_test.cc:113: using namespace password_manager::prefs; On 2015/09/17 15:15:56, engedy wrote: > nit: Note that the using-directive is banned even inside functions by the style > guide: > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Namespaces > > You could use the using-declaration to create an alias to the namespace within > the function, though. Done. https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/sync/t... chrome/browser/sync/test/integration/single_client_password_manager_setting_migrator_service_sync_test.cc:121: trial_ = password_manager_setting_migrater_helper::GetMigrationFieldTrial(); On 2015/09/17 15:15:56, engedy wrote: > Same here with the field trial: no need to hold on to the return value. Done. https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/sync/t... chrome/browser/sync/test/integration/single_client_password_manager_setting_migrator_service_sync_test.cc:137: SetLocalPrefValues(true /* kCredentialsEnableService */, On 2015/09/17 17:41:21, engedy wrote: > To clarify, does that data being injected through InjectDataToFakeServer() get > sent down to the client in MergeDataAndStartSyncing, or in subsequent > ProcessSyncChanges? > > If the former, then that local and sync values are different can only happen if > the user has just set up Sync, right? If so, should we reorder these test cases > and let CanSyncStart return false first, and then call SetupSync to simulate a > real world situation better? > > What I'd like to somehow verify here is that changes made during the > MergeDataAndStartSycing step will end up in Sync (so it would get synced to > legacy clients)? > > (Happy to discuss offline.) Let's discuss offline and if I got your idea right, it will be significant change to the tests, so I propose to implement it as separate CL. https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/sync/t... chrome/browser/sync/test/integration/single_client_password_manager_setting_migrator_service_sync_test.cc:243: IN_PROC_BROWSER_TEST_F( On 2015/09/17 15:15:56, engedy wrote: > I think it would be useful to add: > > * false-true, false-true (this will be the case for all users for whom we'll > enable the migration later on through the field trial mechanism and who has > opted out of the new settting). > * false-false, false-false (to check that values are preserved in steady > state) > * true-true, true-true (to check that values are preserved in steady state) Given that sync tests are not the fastest one I wouldn't add false-false, false-false and true-true, true-true because we already have unit tests for such cases. But on the other hand two tests doesn;t make a lot of difference. https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/sync/t... File chrome/browser/sync/test/integration/two_client_password_manager_setting_migrator_service_sync_test.cc (right): https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/sync/t... chrome/browser/sync/test/integration/two_client_password_manager_setting_migrator_service_sync_test.cc:5: #include "base/metrics/field_trial.h" On 2015/09/17 15:15:57, engedy wrote: > nit: Please remove unused includes and using declarations in this file too. Done. https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/sync/t... chrome/browser/sync/test/integration/two_client_password_manager_setting_migrator_service_sync_test.cc:50: // Check that changed pref is equals on both clients On 2015/09/17 15:15:57, engedy wrote: > nit: s/is equals/has the same value/ > nit: Given that we are checking both explicitly, do we need the first check? Done. https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/sync/t... chrome/browser/sync/test/integration/two_client_password_manager_setting_migrator_service_sync_test.cc:56: ~TwoClientsPasswordManagerSettingMigratorServiceSyncTest() override {} On 2015/09/17 15:15:57, engedy wrote: > nit: Move DTOR to the after the CTOR. Done. https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/sync/t... chrome/browser/sync/test/integration/two_client_password_manager_setting_migrator_service_sync_test.cc:59: trial_ = password_manager_setting_migrater_helper::GetMigrationFieldTrial(); On 2015/09/17 15:15:57, engedy wrote: > nit: Same thing with field trial here. Done. https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/sync/t... chrome/browser/sync/test/integration/two_client_password_manager_setting_migrator_service_sync_test.cc:64: void ExpectValueOnBothClientsForBothPreference(bool value) { On 2015/09/17 15:15:57, engedy wrote: > nit: Make Expect/Assert terminology usage consistent. Done. https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/sync/t... chrome/browser/sync/test/integration/two_client_password_manager_setting_migrator_service_sync_test.cc:71: bool TestUsesSelfNotifications() override { return false; } On 2015/09/17 15:15:57, engedy wrote: > Would this be something that we might want to set in other tests too? Not in single client. https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/sync/t... chrome/browser/sync/test/integration/two_client_password_manager_setting_migrator_service_sync_test.cc:90: ASSERT_TRUE(SetupClients()); On 2015/09/17 15:15:57, engedy wrote: > nit: SetupSync and remove that call later? I prefer that by the time sync starts we have initialized migration service. Although it shouldn't make a difference. https://codereview.chromium.org/1256803002/diff/1500001/chrome/browser/sync/t... chrome/browser/sync/test/integration/two_client_password_manager_setting_migrator_service_sync_test.cc:140: } On 2015/09/17 17:41:21, engedy wrote: > Could we also add a sanity check test here with the special case we've talked > about: 1 migrating and 1 legacy client, initial state NL=11, then the legacy > client would do (without doing the waiting methods): > > 1.) N=0 > 2.) L=0 > 3.) N=1 > > Basically, I'd like to ensure that if two conflicting changes arrive through > ProcessSyncChanges in the same batch, we still do the right thing, and this gets > Synced to the legacy client. As per offline discussion: think how to do it in another CL. https://codereview.chromium.org/1256803002/diff/1500001/components/password_m... File components/password_manager/core/browser/password_manager_settings_migration_experiment.cc (left): https://codereview.chromium.org/1256803002/diff/1500001/components/password_m... components/password_manager/core/browser/password_manager_settings_migration_experiment.cc:14: return base::FieldTrialList::FindFullName(kFieldTrialName) == On 2015/09/17 15:15:57, engedy wrote: > I think we should change this to prefix matching, so that it also accepts > Enable2, Enable3, etc. In the past, it proved useful to have multiple groups > with the same behavior. I don't have anything specific in mind, though. > > base::StartsWith(base::FieldTrialList::FindFullName(kFieldTrialName), "Enable", > base::CompareCase::INSENSITIVE_ASCII); Done.
Message was sent while issue was closed.
Summary of left overs, which should be adressed in separate CLs: chrome/browser/password_manager/password_manager_setting_migrator_service.х:50 nit: This example here suggests that having a different pair of settings in the Sync DB than in the PrefStore on MergeDataAndStartSyncing is a common case. Should we use the example N=0 L=1 instead? (Note that this can happen now. chrome/browser/sync/test/integration/single_client_password_manager_setting_migrator_service_sync_test.cc:109 > I think this is the only usage of AssertPrefValues() where we want to abort > after failure. But it also shoudn't harm if we don't, we'll just get more unfulfilled conditions. chrome/browser/sync/test/integration/single_client_password_manager_setting_migrator_service_sync_test.cc:139 To clarify, does that data being injected through InjectDataToFakeServer() get sent down to the client in MergeDataAndStartSyncing, or in subsequent ProcessSyncChanges? If the former, then that local and sync values are different can only happen if the user has just set up Sync, right? If so, should we reorder these test cases and let CanSyncStart return false first, and then call SetupSync to simulate a real world situation better? What I'd like to somehow verify here is that changes made during the MergeDataAndStartSycing step will end up in Sync (so it would get synced to legacy clients)? chrome/browser/sync/test/integration/two_client_password_manager_setting_migrator_service_sync_test.cc:140 > Could we also add a sanity check test here with the special case we've talked > about: 1 migrating and 1 legacy client, initial state NL=11, then the legacy > client would do (without doing the waiting methods): > > 1.) N=0 > 2.) L=0 > 3.) N=1 > > Basically, I'd like to ensure that if two conflicting changes arrive through > ProcessSyncChanges in the same batch, we still do the right thing, and this gets > Synced to the legacy client. As per offline discussion: think how to do it in another CL. |
