Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
944433002:
Revamp the MigrateBrowserPrefs and MigrateUserPrefs code. (Closed)
DescriptionRevamp the MigrateBrowserPrefs and MigrateUserPrefs code.
Restructure it so it is organized by date-added, and delete all migrations that are sufficiently old.
BUG=69995, 165672
Committed: https://crrev.com/f216cafc43a6f43937ecc2e178f16217bd736c4b
Cr-Commit-Position: refs/heads/master@{#318780}
Patch Set 1 #Patch Set 2 : readd method #
Total comments: 2
Patch Set 3 : gab comments #Patch Set 4 : gab comments #
Total comments: 22
Patch Set 5 : gab comments #
Total comments: 6
Patch Set 6 : extra removal #
Total comments: 28
Patch Set 7 : more gab comments and rebase #Patch Set 8 : more fixes #Patch Set 9 : revert argument for profileprefs to original state #Patch Set 10 : 1 line whitespace change #Patch Set 11 : second half of gab comments #Patch Set 12 : readd kbrowserwindowplacement to ui_prefs.cc #
Total comments: 4
Patch Set 13 : more comments. removing default_pinned_apps-field_trial #Patch Set 14 : Remove include/call to default_pinned_apps #Patch Set 15 : delete migration test #
Total comments: 2
Patch Set 16 : clean up other pref_service test #Patch Set 17 : additional cleanup #
Total comments: 12
Patch Set 18 : gab comments #Patch Set 19 : move comments #Patch Set 20 : change asserts to expect in setupuserdatadirectory #Messages
Total messages: 66 (19 generated)
rkaplow@chromium.org changed reviewers: + gab@chromium.org, noms@chromium.org
noms@chromium.org: Please review changes in profile gab@chromium.org: Please review changes in prefs
As mentioned on the bug, these calls have since been reused to cleanup more recently deprecated prefs, we should clean this up for sure but not by directly getting rid of it. An easy drop site for deprecated pref removal is often required.
As mentioned on the bug, these calls have since been reused to cleanup more recently deprecated prefs, we should clean this up for sure but not by directly getting rid of it. An easy drop site for deprecated pref removal is often required.
Ok, sounds good. I left the method, but removed the current usage. Added a note in the method comment.
profiles lgtm
https://codereview.chromium.org/944433002/diff/20001/chrome/browser/profiles/... File chrome/browser/profiles/profile_impl.cc (left): https://codereview.chromium.org/944433002/diff/20001/chrome/browser/profiles/... chrome/browser/profiles/profile_impl.cc:861: // TODO(ivankr): remove cleanup code eventually (crbug.com/165672). No, we shouldn't get rid of this hook. If anything we should leave both of these hooks here (removing the TODOs). And instead cleanup the old pref clearance (git blame to see how old) in those methods themselves (even if that means leaving them empty). (also changing the API to make it clear that those methods should be used to clear old prefs and that their content should be cycled over time)
https://codereview.chromium.org/944433002/diff/20001/chrome/browser/profiles/... File chrome/browser/profiles/profile_impl.cc (left): https://codereview.chromium.org/944433002/diff/20001/chrome/browser/profiles/... chrome/browser/profiles/profile_impl.cc:861: // TODO(ivankr): remove cleanup code eventually (crbug.com/165672). On 2015/02/20 19:04:03, gab wrote: > No, we shouldn't get rid of this hook. > > If anything we should leave both of these hooks here (removing the TODOs). > > And instead cleanup the old pref clearance (git blame to see how old) in those > methods themselves (even if that means leaving them empty). > > (also changing the API to make it clear that those methods should be used to > clear old prefs and that their content should be cycled over time) Hm, ok. How long should pref migration live for? I.e. what should the cutoff be?
On 2015/02/20 19:08:53, rkaplow wrote: > https://codereview.chromium.org/944433002/diff/20001/chrome/browser/profiles/... > File chrome/browser/profiles/profile_impl.cc (left): > > https://codereview.chromium.org/944433002/diff/20001/chrome/browser/profiles/... > chrome/browser/profiles/profile_impl.cc:861: // TODO(ivankr): remove cleanup > code eventually (crbug.com/165672). > On 2015/02/20 19:04:03, gab wrote: > > No, we shouldn't get rid of this hook. > > > > If anything we should leave both of these hooks here (removing the TODOs). > > > > And instead cleanup the old pref clearance (git blame to see how old) in those > > methods themselves (even if that means leaving them empty). > > > > (also changing the API to make it clear that those methods should be used to > > clear old prefs and that their content should be cycled over time) > > Hm, ok. > > How long should pref migration live for? I.e. what should the cutoff be? Based on a comment I saw - I'm going to clean up everything a year+ old. I'll cut off everything in 2013 and earlier. I will add comments for the remaining lines for which day they were added. Does that sound like what you had in mind?
New patchsets have been uploaded after l-g-t-m from noms@chromium.org
gab: this more what you had in mind?
Thanks for the cleanup, yes this is more along the lines of what I had in mind. More comments below. Please also update the CL description. Thanks! Gab https://codereview.chromium.org/944433002/diff/60001/chrome/browser/prefs/bro... File chrome/browser/prefs/browser_prefs.cc (left): https://codereview.chromium.org/944433002/diff/60001/chrome/browser/prefs/bro... chrome/browser/prefs/browser_prefs.cc:561: prefs->ClearPref(kBackupPref); Remove kBackupPref's definition higher up in this file. https://codereview.chromium.org/944433002/diff/60001/chrome/browser/prefs/bro... chrome/browser/prefs/browser_prefs.cc:583: local_state->GetInteger(prefs::kMultipleProfilePrefMigration); Also remove all constants cleaned up below from the codebase (all of them should only have been referred to here). https://codereview.chromium.org/944433002/diff/60001/chrome/browser/prefs/bro... File chrome/browser/prefs/browser_prefs.cc (right): https://codereview.chromium.org/944433002/diff/60001/chrome/browser/prefs/bro... chrome/browser/prefs/browser_prefs.cc:565: #if !defined(OS_ANDROID) I liked having the comment inside the ifdef, please switch it back. (same below) https://codereview.chromium.org/944433002/diff/60001/chrome/browser/prefs/bro... chrome/browser/prefs/browser_prefs.cc:581: local_state->ClearPref(kLegacyProfileResetPromptMemento); For prefs that are cleared inline (like this one), please inline the constant in this method as a static const char[] -- e.g., there is no point leaving globally defined constants around just to clean them up here. https://codereview.chromium.org/944433002/diff/60001/chrome/browser/prefs/bro... File chrome/browser/prefs/browser_prefs.h (right): https://codereview.chromium.org/944433002/diff/60001/chrome/browser/prefs/bro... chrome/browser/prefs/browser_prefs.h:34: // This should always be executed, and will only migrate the newest changes. Change this entire comment to something like: // Migrate/cleanup deprecated prefs in |local_state|. Over-time, long deprecated prefs should be removed as new ones are added, but this call should never go away (even if it becomes an empty call for some time) as it should remain *the* place to drop deprecated browser prefs at. https://codereview.chromium.org/944433002/diff/60001/chrome/browser/prefs/bro... chrome/browser/prefs/browser_prefs.h:35: void MigrateBrowserPrefs(Profile* profile, PrefService* local_state); Remove |profile| parameter. https://codereview.chromium.org/944433002/diff/60001/chrome/browser/prefs/bro... chrome/browser/prefs/browser_prefs.h:38: // This should always be executed, and will only migrate the newest changes. Change this entire comment to something like: // Migrate/cleanup deprecated prefs in |profile_prefs|. Over-time, long deprecated prefs should be removed as new ones are added, but this call should never go away (even if it becomes an empty call for some time) as it should remain *the* place to drop deprecated profile prefs at. https://codereview.chromium.org/944433002/diff/60001/chrome/browser/prefs/bro... chrome/browser/prefs/browser_prefs.h:39: void MigrateUserPrefs(Profile* profile); Change the argument from a |Profile* profile| to a |PrefService* profile_prefs| -- the caller (a Profile) already has the prefs object as a member it can hand over directly. https://codereview.chromium.org/944433002/diff/60001/chrome/browser/profiles/... File chrome/browser/profiles/profile_impl.cc (right): https://codereview.chromium.org/944433002/diff/60001/chrome/browser/profiles/... chrome/browser/profiles/profile_impl.cc:858: // Migrate all obsolete prefs. rm "all" (it's implicit IMO) PS: I like "obsolete" perhaps add that to the migration method's names? e.g. MigrateObsoleteBrowserPrefs() and MigrateObsoleteProfilePrefs() -- oh yea, also, I prefer ProfilePrefs to UserPrefs as it's more explicit (i.e. a user can have multiple profiles).
rkaplow@chromium.org changed reviewers: + msw@chromium.org
msw@chromium.org: Please review changes in browser/ui Apologizes reviewers, I did a rebase locally and didn't realize it wouldn't get sycned on codereview.chromium, so there is diffs from other CLs mixed in. It only affects the pref part. Gab - it should be reasonably clear. https://codereview.chromium.org/944433002/diff/60001/chrome/browser/prefs/bro... File chrome/browser/prefs/browser_prefs.cc (left): https://codereview.chromium.org/944433002/diff/60001/chrome/browser/prefs/bro... chrome/browser/prefs/browser_prefs.cc:561: prefs->ClearPref(kBackupPref); On 2015/02/20 20:33:57, gab wrote: > Remove kBackupPref's definition higher up in this file. Am I good to remove this initialization of the pref? // Preferences registered only for migration (clearing or moving to a new key) // go here. registry->RegisterDictionaryPref( kBackupPref, new base::DictionaryValue(), user_prefs::PrefRegistrySyncable::UNSYNCABLE_PREF); } I'm guessing this is fine to remove but want to double check https://codereview.chromium.org/944433002/diff/60001/chrome/browser/prefs/bro... File chrome/browser/prefs/browser_prefs.cc (right): https://codereview.chromium.org/944433002/diff/60001/chrome/browser/prefs/bro... chrome/browser/prefs/browser_prefs.cc:565: #if !defined(OS_ANDROID) On 2015/02/20 20:33:57, gab wrote: > I liked having the comment inside the ifdef, please switch it back. > > (same below) Done. I rearranged everything to be ordered by date so here is an obvious addition/removal flow https://codereview.chromium.org/944433002/diff/60001/chrome/browser/prefs/bro... chrome/browser/prefs/browser_prefs.cc:581: local_state->ClearPref(kLegacyProfileResetPromptMemento); On 2015/02/20 20:33:57, gab wrote: > For prefs that are cleared inline (like this one), please inline the constant in > this method as a static const char[] -- e.g., there is no point leaving globally > defined constants around just to clean them up here. This gets used elsewhere, https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pre... I think I need to remove it there as well. Can you confirm? https://codereview.chromium.org/944433002/diff/60001/chrome/browser/prefs/bro... File chrome/browser/prefs/browser_prefs.h (right): https://codereview.chromium.org/944433002/diff/60001/chrome/browser/prefs/bro... chrome/browser/prefs/browser_prefs.h:34: // This should always be executed, and will only migrate the newest changes. On 2015/02/20 20:33:57, gab wrote: > Change this entire comment to something like: > > // Migrate/cleanup deprecated prefs in |local_state|. Over-time, long deprecated > prefs should be removed as new ones are added, but this call should never go > away (even if it becomes an empty call for some time) as it should remain *the* > place to drop deprecated browser prefs at. Done. Your suggestion SGTM https://codereview.chromium.org/944433002/diff/60001/chrome/browser/prefs/bro... chrome/browser/prefs/browser_prefs.h:35: void MigrateBrowserPrefs(Profile* profile, PrefService* local_state); On 2015/02/20 20:33:57, gab wrote: > Remove |profile| parameter. Done. https://codereview.chromium.org/944433002/diff/60001/chrome/browser/prefs/bro... chrome/browser/prefs/browser_prefs.h:38: // This should always be executed, and will only migrate the newest changes. On 2015/02/20 20:33:57, gab wrote: > Change this entire comment to something like: > > // Migrate/cleanup deprecated prefs in |profile_prefs|. Over-time, long > deprecated prefs should be removed as new ones are added, but this call should > never go away (even if it becomes an empty call for some time) as it should > remain *the* place to drop deprecated profile prefs at. Done. https://codereview.chromium.org/944433002/diff/60001/chrome/browser/prefs/bro... chrome/browser/prefs/browser_prefs.h:39: void MigrateUserPrefs(Profile* profile); On 2015/02/20 20:33:57, gab wrote: > Change the argument from a |Profile* profile| to a |PrefService* profile_prefs| > -- the caller (a Profile) already has the prefs object as a member it can hand > over directly. Done. https://codereview.chromium.org/944433002/diff/60001/chrome/browser/profiles/... File chrome/browser/profiles/profile_impl.cc (right): https://codereview.chromium.org/944433002/diff/60001/chrome/browser/profiles/... chrome/browser/profiles/profile_impl.cc:858: // Migrate all obsolete prefs. On 2015/02/20 20:33:57, gab wrote: > rm "all" (it's implicit IMO) > > PS: I like "obsolete" perhaps add that to the migration method's names? e.g. > MigrateObsoleteBrowserPrefs() and MigrateObsoleteProfilePrefs() -- oh yea, also, > I prefer ProfilePrefs to UserPrefs as it's more explicit (i.e. a user can have > multiple profiles). Seems fine. Note that some of the work is done by names that mirror the current name, eg. autofill::AutofillManager::MigrateUserPrefs In the long run those will go away so it seems fine for a temporary discrepancy. https://codereview.chromium.org/944433002/diff/80001/chrome/browser/ui/browse... File chrome/browser/ui/browser_window_state.cc (right): https://codereview.chromium.org/944433002/diff/80001/chrome/browser/ui/browse... chrome/browser/ui/browser_window_state.cc:73: return prefs::kBrowserWindowPlacementPopup; I'm not too sure about this. Can someone with context let me know if this is correct now? https://codereview.chromium.org/944433002/diff/80001/chrome/common/pref_names.h File chrome/common/pref_names.h (right): https://codereview.chromium.org/944433002/diff/80001/chrome/common/pref_names... chrome/common/pref_names.h:168: extern const char kMultipleProfilePrefMigration[]; // OBSOLETE some are marked OBSOLETE. Should I be removing this at this point or just mark as OBSOLETE?
Woot, this cleanup is epic! Please update CL title and definition according to what it now does. https://codereview.chromium.org/944433002/diff/60001/chrome/browser/prefs/bro... File chrome/browser/prefs/browser_prefs.cc (left): https://codereview.chromium.org/944433002/diff/60001/chrome/browser/prefs/bro... chrome/browser/prefs/browser_prefs.cc:561: prefs->ClearPref(kBackupPref); On 2015/02/20 21:58:08, rkaplow wrote: > On 2015/02/20 20:33:57, gab wrote: > > Remove kBackupPref's definition higher up in this file. > > Am I good to remove this initialization of the pref? > > > // Preferences registered only for migration (clearing or moving to a new key) > // go here. > registry->RegisterDictionaryPref( > kBackupPref, > new base::DictionaryValue(), > user_prefs::PrefRegistrySyncable::UNSYNCABLE_PREF); > } > > I'm guessing this is fine to remove but want to double check Yes, prefs that are still cleared will need to be registered (and in fact now that I recall, this is why they had to be declared at least at file scope rather than cleanup method scope, sadly). https://codereview.chromium.org/944433002/diff/60001/chrome/browser/prefs/bro... File chrome/browser/prefs/browser_prefs.cc (right): https://codereview.chromium.org/944433002/diff/60001/chrome/browser/prefs/bro... chrome/browser/prefs/browser_prefs.cc:581: local_state->ClearPref(kLegacyProfileResetPromptMemento); On 2015/02/20 21:58:08, rkaplow wrote: > On 2015/02/20 20:33:57, gab wrote: > > For prefs that are cleared inline (like this one), please inline the constant > in > > this method as a static const char[] -- e.g., there is no point leaving > globally > > defined constants around just to clean them up here. > > This gets used elsewhere, > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pre... > > I think I need to remove it there as well. Can you confirm? Oops as mentioned above we'll need keep the file scope definition to be able to register it, it would make sense however to have a local RegisterDeprecatedPrefs() method call from RegisterUserPrefs() for all of those registrations to be grouped together (with similar dates + removal comments). https://codereview.chromium.org/944433002/diff/80001/chrome/common/pref_names.h File chrome/common/pref_names.h (right): https://codereview.chromium.org/944433002/diff/80001/chrome/common/pref_names... chrome/common/pref_names.h:168: extern const char kMultipleProfilePrefMigration[]; // OBSOLETE On 2015/02/20 21:58:08, rkaplow wrote: > some are marked OBSOLETE. Should I be removing this at this point or just mark > as OBSOLETE? The other ones FWICT are declared globally but only used locally for cleanup. I'd prefer that the ones only needed in browser_prefsl.cc be removed from this global declaration file and moved to a local file scope declaration in browser_prefs.cc so that despite still needing them for a while, we keep the cleanup code as local as possible. (consider doing this for the 3 existing OBSOLETE prefs as part of this CL as well) https://codereview.chromium.org/944433002/diff/100001/chrome/browser/prefs/br... File chrome/browser/prefs/browser_prefs.cc (left): https://codereview.chromium.org/944433002/diff/100001/chrome/browser/prefs/br... chrome/browser/prefs/browser_prefs.cc:586: local_state->DeprecatedGetPrefRegistry()); Woot, one less call to DeprecatedGetPrefRegistry() :-) https://codereview.chromium.org/944433002/diff/100001/chrome/browser/prefs/br... chrome/browser/prefs/browser_prefs.cc:588: if (!(current_version & DNS_PREFS)) { Looks like you can now also get rid of the enum defining DNS_PREFS and WINDOW_PREFS :-) https://codereview.chromium.org/944433002/diff/100001/chrome/browser/prefs/br... chrome/browser/prefs/browser_prefs.cc:622: chromeos::default_pinned_apps_field_trial::MigratePrefs(local_state); Also clean up this no longer call method (and all of its file it looks like!): https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr... https://codereview.chromium.org/944433002/diff/100001/chrome/browser/prefs/br... chrome/browser/prefs/browser_prefs.cc:626: MigrateBrowserTabStripPrefs(local_state); Also remove call (and associated registrations) @ https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... https://codereview.chromium.org/944433002/diff/100001/chrome/browser/prefs/br... File chrome/browser/prefs/browser_prefs.cc (right): https://codereview.chromium.org/944433002/diff/100001/chrome/browser/prefs/br... chrome/browser/prefs/browser_prefs.cc:234: const char kBackupPref[] = "backup"; Remove it completely. https://codereview.chromium.org/944433002/diff/100001/chrome/browser/prefs/br... chrome/browser/prefs/browser_prefs.cc:557: // This method should prune away migrations year+ old. s/should prune away/should be pruned from/ (i.e. the method itself doesn't do the pruning) https://codereview.chromium.org/944433002/diff/100001/chrome/browser/prefs/br... chrome/browser/prefs/browser_prefs.cc:557: // This method should prune away migrations year+ old. s/migrations year+ old/year+ old migrations/ https://codereview.chromium.org/944433002/diff/100001/chrome/browser/prefs/br... chrome/browser/prefs/browser_prefs.cc:576: } Looks like you'll need to sync/merge again for the new addition of MigrateGoogleNowPrefs(): https://chromium.googlesource.com/chromium/src/+blame/master/chrome/browser/p... Also looks like this brings back the dependency on having a Profile* as an argument... for the sake of simplicity, let's add it back and not try to be fancier for now. https://codereview.chromium.org/944433002/diff/100001/chrome/browser/prefs/br... chrome/browser/prefs/browser_prefs.cc:579: // This method should prune away migrations year+ old. Same comments as above. https://codereview.chromium.org/944433002/diff/100001/chrome/browser/ui/brows... File chrome/browser/ui/browser_window_state.cc (left): https://codereview.chromium.org/944433002/diff/100001/chrome/browser/ui/brows... chrome/browser/ui/browser_window_state.cc:74: prefs::kBrowserWindowPlacementPopup : prefs::kBrowserWindowPlacement; Do not change this code. The migration code you removed moved the pref from Local State (browser prefs) to per-profile state (user prefs), it doesn't get rid of the pref per se. https://codereview.chromium.org/944433002/diff/100001/chrome/browser/ui/prefs... File chrome/browser/ui/prefs/prefs_tab_helper.cc (left): https://codereview.chromium.org/944433002/diff/100001/chrome/browser/ui/prefs... chrome/browser/ui/prefs/prefs_tab_helper.cc:510: pref_store->RegisterOverlayPref(prefs::kBrowserWindowPlacement); Do not remove this. https://codereview.chromium.org/944433002/diff/100001/chrome/common/pref_name... File chrome/common/pref_names.cc (left): https://codereview.chromium.org/944433002/diff/100001/chrome/common/pref_name... chrome/common/pref_names.cc:381: const char kDnsStartupPrefetchList[] = "StartupDNSPrefetchList"; Also remove declaration in header for all prefs removed from this file. https://codereview.chromium.org/944433002/diff/100001/chrome/common/pref_name... chrome/common/pref_names.cc:1429: const char kBrowserWindowPlacement[] = "browser.window_placement"; Do not remove this. https://codereview.chromium.org/944433002/diff/100001/chrome/common/pref_names.h File chrome/common/pref_names.h (left): https://codereview.chromium.org/944433002/diff/100001/chrome/common/pref_name... chrome/common/pref_names.h:494: extern const char kBrowserWindowPlacement[]; Do not remove this.
+Bernhard FYI, we're removing all pref migration/cleanup code that is a year+ old, let us know if you don't think this is right.
bauerb@chromium.org changed reviewers: + bauerb@chromium.org
W00t, LGTM!
Sorry I made a bit of a hash of the uploads and the rebasing. Let me know if anything is unclear. I changed the arguments to the older ones to let the googlenow one still work. https://codereview.chromium.org/944433002/diff/60001/chrome/browser/prefs/bro... File chrome/browser/prefs/browser_prefs.cc (right): https://codereview.chromium.org/944433002/diff/60001/chrome/browser/prefs/bro... chrome/browser/prefs/browser_prefs.cc:581: local_state->ClearPref(kLegacyProfileResetPromptMemento); On 2015/02/23 16:09:37, gab wrote: > On 2015/02/20 21:58:08, rkaplow wrote: > > On 2015/02/20 20:33:57, gab wrote: > > > For prefs that are cleared inline (like this one), please inline the > constant > > in > > > this method as a static const char[] -- e.g., there is no point leaving > > globally > > > defined constants around just to clean them up here. > > > > This gets used elsewhere, > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pre... > > > > I think I need to remove it there as well. Can you confirm? > > Oops as mentioned above we'll need keep the file scope definition to be able to > register it, it would make sense however to have a local > RegisterDeprecatedPrefs() method call from RegisterUserPrefs() for all of those > registrations to be grouped together (with similar dates + removal comments). There's actually a comment for kLegacyProfileResetPromptMemento at least. For now I think I will leave to try to cut this CL at once place. We can do what you're suggesting in a follow up https://codereview.chromium.org/944433002/diff/100001/chrome/browser/prefs/br... File chrome/browser/prefs/browser_prefs.cc (right): https://codereview.chromium.org/944433002/diff/100001/chrome/browser/prefs/br... chrome/browser/prefs/browser_prefs.cc:234: const char kBackupPref[] = "backup"; On 2015/02/23 16:09:38, gab wrote: > Remove it completely. Done. https://codereview.chromium.org/944433002/diff/100001/chrome/browser/prefs/br... chrome/browser/prefs/browser_prefs.cc:557: // This method should prune away migrations year+ old. On 2015/02/23 16:09:38, gab wrote: > s/migrations year+ old/year+ old migrations/ Done. https://codereview.chromium.org/944433002/diff/100001/chrome/browser/prefs/br... chrome/browser/prefs/browser_prefs.cc:557: // This method should prune away migrations year+ old. On 2015/02/23 16:09:38, gab wrote: > s/should prune away/should be pruned from/ > > (i.e. the method itself doesn't do the pruning) true, wording was odd ;) https://codereview.chromium.org/944433002/diff/100001/chrome/browser/prefs/br... chrome/browser/prefs/browser_prefs.cc:576: } On 2015/02/23 16:09:38, gab wrote: > Looks like you'll need to sync/merge again for the new addition of > MigrateGoogleNowPrefs(): > https://chromium.googlesource.com/chromium/src/+blame/master/chrome/browser/p... > > Also looks like this brings back the dependency on having a Profile* as an > argument... for the sake of simplicity, let's add it back and not try to be > fancier for now. Done. https://codereview.chromium.org/944433002/diff/100001/chrome/browser/prefs/br... chrome/browser/prefs/browser_prefs.cc:579: // This method should prune away migrations year+ old. On 2015/02/23 16:09:37, gab wrote: > Same comments as above. Done.
On 2015/02/23 22:55:04, rkaplow wrote: > Sorry I made a bit of a hash of the uploads and the rebasing. Let me know if > anything is unclear. I changed the arguments to the older ones to let the > googlenow one still work. > > https://codereview.chromium.org/944433002/diff/60001/chrome/browser/prefs/bro... > File chrome/browser/prefs/browser_prefs.cc (right): > > https://codereview.chromium.org/944433002/diff/60001/chrome/browser/prefs/bro... > chrome/browser/prefs/browser_prefs.cc:581: > local_state->ClearPref(kLegacyProfileResetPromptMemento); > On 2015/02/23 16:09:37, gab wrote: > > On 2015/02/20 21:58:08, rkaplow wrote: > > > On 2015/02/20 20:33:57, gab wrote: > > > > For prefs that are cleared inline (like this one), please inline the > > constant > > > in > > > > this method as a static const char[] -- e.g., there is no point leaving > > > globally > > > > defined constants around just to clean them up here. > > > > > > This gets used elsewhere, > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pre... > > > > > > I think I need to remove it there as well. Can you confirm? > > > > Oops as mentioned above we'll need keep the file scope definition to be able > to > > register it, it would make sense however to have a local > > RegisterDeprecatedPrefs() method call from RegisterUserPrefs() for all of > those > > registrations to be grouped together (with similar dates + removal comments). > > There's actually a comment for kLegacyProfileResetPromptMemento at least. > For now I think I will leave to try to cut this CL at once place. We can do what > you're suggesting in a follow up > > https://codereview.chromium.org/944433002/diff/100001/chrome/browser/prefs/br... > File chrome/browser/prefs/browser_prefs.cc (right): > > https://codereview.chromium.org/944433002/diff/100001/chrome/browser/prefs/br... > chrome/browser/prefs/browser_prefs.cc:234: const char kBackupPref[] = "backup"; > On 2015/02/23 16:09:38, gab wrote: > > Remove it completely. > > Done. > > https://codereview.chromium.org/944433002/diff/100001/chrome/browser/prefs/br... > chrome/browser/prefs/browser_prefs.cc:557: // This method should prune away > migrations year+ old. > On 2015/02/23 16:09:38, gab wrote: > > s/migrations year+ old/year+ old migrations/ > > Done. > > https://codereview.chromium.org/944433002/diff/100001/chrome/browser/prefs/br... > chrome/browser/prefs/browser_prefs.cc:557: // This method should prune away > migrations year+ old. > On 2015/02/23 16:09:38, gab wrote: > > s/should prune away/should be pruned from/ > > > > (i.e. the method itself doesn't do the pruning) > > true, wording was odd ;) > > https://codereview.chromium.org/944433002/diff/100001/chrome/browser/prefs/br... > chrome/browser/prefs/browser_prefs.cc:576: } > On 2015/02/23 16:09:38, gab wrote: > > Looks like you'll need to sync/merge again for the new addition of > > MigrateGoogleNowPrefs(): > > > https://chromium.googlesource.com/chromium/src/+blame/master/chrome/browser/p... > > > > Also looks like this brings back the dependency on having a Profile* as an > > argument... for the sake of simplicity, let's add it back and not try to be > > fancier for now. > > Done. > > https://codereview.chromium.org/944433002/diff/100001/chrome/browser/prefs/br... > chrome/browser/prefs/browser_prefs.cc:579: // This method should prune away > migrations year+ old. > On 2015/02/23 16:09:37, gab wrote: > > Same comments as above. > > Done. I think I've missed some of the comments now that I am re-looking so hold off on the review.
https://codereview.chromium.org/944433002/diff/60001/chrome/browser/prefs/bro... File chrome/browser/prefs/browser_prefs.cc (left): https://codereview.chromium.org/944433002/diff/60001/chrome/browser/prefs/bro... chrome/browser/prefs/browser_prefs.cc:561: prefs->ClearPref(kBackupPref); On 2015/02/23 16:09:37, gab wrote: > On 2015/02/20 21:58:08, rkaplow wrote: > > On 2015/02/20 20:33:57, gab wrote: > > > Remove kBackupPref's definition higher up in this file. > > > > Am I good to remove this initialization of the pref? > > > > > > // Preferences registered only for migration (clearing or moving to a new > key) > > // go here. > > registry->RegisterDictionaryPref( > > kBackupPref, > > new base::DictionaryValue(), > > user_prefs::PrefRegistrySyncable::UNSYNCABLE_PREF); > > } > > > > I'm guessing this is fine to remove but want to double check > > Yes, prefs that are still cleared will need to be registered (and in fact now > that I recall, this is why they had to be declared at least at file scope rather > than cleanup method scope, sadly). Done. https://codereview.chromium.org/944433002/diff/60001/chrome/browser/prefs/bro... chrome/browser/prefs/browser_prefs.cc:583: local_state->GetInteger(prefs::kMultipleProfilePrefMigration); On 2015/02/20 20:33:57, gab wrote: > Also remove all constants cleaned up below from the codebase (all of them should > only have been referred to here). should be done. https://codereview.chromium.org/944433002/diff/80001/chrome/common/pref_names.h File chrome/common/pref_names.h (right): https://codereview.chromium.org/944433002/diff/80001/chrome/common/pref_names... chrome/common/pref_names.h:168: extern const char kMultipleProfilePrefMigration[]; // OBSOLETE On 2015/02/23 16:09:37, gab wrote: > On 2015/02/20 21:58:08, rkaplow wrote: > > some are marked OBSOLETE. Should I be removing this at this point or just mark > > as OBSOLETE? > > The other ones FWICT are declared globally but only used locally for cleanup. > > I'd prefer that the ones only needed in browser_prefsl.cc be removed from this > global declaration file and moved to a local file scope declaration in > browser_prefs.cc so that despite still needing them for a while, we keep the > cleanup code as local as possible. > > (consider doing this for the 3 existing OBSOLETE prefs as part of this CL as > well) already having a bit of trouble keeping track of everything so will not do that as part of this CL. Added TODO https://codereview.chromium.org/944433002/diff/100001/chrome/browser/prefs/br... File chrome/browser/prefs/browser_prefs.cc (left): https://codereview.chromium.org/944433002/diff/100001/chrome/browser/prefs/br... chrome/browser/prefs/browser_prefs.cc:586: local_state->DeprecatedGetPrefRegistry()); On 2015/02/23 16:09:38, gab wrote: > Woot, one less call to DeprecatedGetPrefRegistry() :-) :) https://codereview.chromium.org/944433002/diff/100001/chrome/browser/prefs/br... chrome/browser/prefs/browser_prefs.cc:588: if (!(current_version & DNS_PREFS)) { On 2015/02/23 16:09:37, gab wrote: > Looks like you can now also get rid of the enum defining DNS_PREFS and > WINDOW_PREFS :-) Done. https://codereview.chromium.org/944433002/diff/100001/chrome/browser/prefs/br... chrome/browser/prefs/browser_prefs.cc:622: chromeos::default_pinned_apps_field_trial::MigratePrefs(local_state); On 2015/02/23 16:09:38, gab wrote: > Also clean up this no longer call method (and all of its file it looks like!): > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr... Done. https://codereview.chromium.org/944433002/diff/100001/chrome/browser/prefs/br... chrome/browser/prefs/browser_prefs.cc:626: MigrateBrowserTabStripPrefs(local_state); On 2015/02/23 16:09:38, gab wrote: > Also remove call (and associated registrations) @ > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... Done. https://codereview.chromium.org/944433002/diff/100001/chrome/browser/ui/brows... File chrome/browser/ui/browser_window_state.cc (left): https://codereview.chromium.org/944433002/diff/100001/chrome/browser/ui/brows... chrome/browser/ui/browser_window_state.cc:74: prefs::kBrowserWindowPlacementPopup : prefs::kBrowserWindowPlacement; On 2015/02/23 16:09:38, gab wrote: > Do not change this code. > > The migration code you removed moved the pref from Local State (browser prefs) > to per-profile state (user prefs), it doesn't get rid of the pref per se. Done. https://codereview.chromium.org/944433002/diff/100001/chrome/browser/ui/prefs... File chrome/browser/ui/prefs/prefs_tab_helper.cc (left): https://codereview.chromium.org/944433002/diff/100001/chrome/browser/ui/prefs... chrome/browser/ui/prefs/prefs_tab_helper.cc:510: pref_store->RegisterOverlayPref(prefs::kBrowserWindowPlacement); On 2015/02/23 16:09:38, gab wrote: > Do not remove this. Done. https://codereview.chromium.org/944433002/diff/100001/chrome/common/pref_name... File chrome/common/pref_names.cc (left): https://codereview.chromium.org/944433002/diff/100001/chrome/common/pref_name... chrome/common/pref_names.cc:381: const char kDnsStartupPrefetchList[] = "StartupDNSPrefetchList"; On 2015/02/23 16:09:38, gab wrote: > Also remove declaration in header for all prefs removed from this file. Done. https://codereview.chromium.org/944433002/diff/100001/chrome/common/pref_name... chrome/common/pref_names.cc:1429: const char kBrowserWindowPlacement[] = "browser.window_placement"; On 2015/02/23 16:09:38, gab wrote: > Do not remove this. Done. https://codereview.chromium.org/944433002/diff/100001/chrome/common/pref_names.h File chrome/common/pref_names.h (left): https://codereview.chromium.org/944433002/diff/100001/chrome/common/pref_name... chrome/common/pref_names.h:494: extern const char kBrowserWindowPlacement[]; On 2015/02/23 16:09:38, gab wrote: > Do not remove this. Done.
lg, a few more things can be removed and then we're good to go I think :-)! https://codereview.chromium.org/944433002/diff/80001/chrome/common/pref_names.h File chrome/common/pref_names.h (right): https://codereview.chromium.org/944433002/diff/80001/chrome/common/pref_names... chrome/common/pref_names.h:168: extern const char kMultipleProfilePrefMigration[]; // OBSOLETE On 2015/02/23 23:27:52, rkaplow wrote: > On 2015/02/23 16:09:37, gab wrote: > > On 2015/02/20 21:58:08, rkaplow wrote: > > > some are marked OBSOLETE. Should I be removing this at this point or just > mark > > > as OBSOLETE? > > > > The other ones FWICT are declared globally but only used locally for cleanup. > > > > I'd prefer that the ones only needed in browser_prefsl.cc be removed from this > > global declaration file and moved to a local file scope declaration in > > browser_prefs.cc so that despite still needing them for a while, we keep the > > cleanup code as local as possible. > > > > (consider doing this for the 3 existing OBSOLETE prefs as part of this CL as > > well) > > > already having a bit of trouble keeping track of everything so will not do that > as part of this CL. Added TODO Ok NP, though I think kCookieBehavior is the only remaining such OBSOLETE pref after you removed 2/3 of them in this CL (and it's not referred to by any code, so it looks like a 2-line strip with no side-effects which may just be easier to do in this CL)... I rarely advocate for more in the same CL (in fact this CL could probably be split up but it's not that hard to follow as-is so I didn't suggest doing so), but in this case feels simple enough to include in this CL. https://codereview.chromium.org/944433002/diff/220001/chrome/browser/chromeos... File chrome/browser/chromeos/login/default_pinned_apps_field_trial.cc (left): https://codereview.chromium.org/944433002/diff/220001/chrome/browser/chromeos... chrome/browser/chromeos/login/default_pinned_apps_field_trial.cc:26: void MigratePrefs(PrefService* local_state) { Delete the rest of this file and its header in fact, it's now only registering an unused pref :-). https://codereview.chromium.org/944433002/diff/220001/chrome/browser/prefs/br... File chrome/browser/prefs/browser_prefs.cc (right): https://codereview.chromium.org/944433002/diff/220001/chrome/browser/prefs/br... chrome/browser/prefs/browser_prefs.cc:520: // go here. Remove this comment I guess now that it comments nothing (or add a generic note that future ones should go here, but I'm worried people would miss that and put them somewhere else anyways so might as well remove it).
https://codereview.chromium.org/944433002/diff/80001/chrome/common/pref_names.h File chrome/common/pref_names.h (right): https://codereview.chromium.org/944433002/diff/80001/chrome/common/pref_names... chrome/common/pref_names.h:168: extern const char kMultipleProfilePrefMigration[]; // OBSOLETE On 2015/02/24 20:55:19, gab wrote: > On 2015/02/23 23:27:52, rkaplow wrote: > > On 2015/02/23 16:09:37, gab wrote: > > > On 2015/02/20 21:58:08, rkaplow wrote: > > > > some are marked OBSOLETE. Should I be removing this at this point or just > > mark > > > > as OBSOLETE? > > > > > > The other ones FWICT are declared globally but only used locally for > cleanup. > > > > > > I'd prefer that the ones only needed in browser_prefsl.cc be removed from > this > > > global declaration file and moved to a local file scope declaration in > > > browser_prefs.cc so that despite still needing them for a while, we keep the > > > cleanup code as local as possible. > > > > > > (consider doing this for the 3 existing OBSOLETE prefs as part of this CL as > > > well) > > > > > > already having a bit of trouble keeping track of everything so will not do > that > > as part of this CL. Added TODO > > Ok NP, though I think kCookieBehavior is the only remaining such OBSOLETE pref > after you removed 2/3 of them in this CL (and it's not referred to by any code, > so it looks like a 2-line strip with no side-effects which may just be easier to > do in this CL)... I rarely advocate for more in the same CL (in fact this CL > could probably be split up but it's not that hard to follow as-is so I didn't > suggest doing so), but in this case feels simple enough to include in this CL. You're correct. Done https://codereview.chromium.org/944433002/diff/220001/chrome/browser/chromeos... File chrome/browser/chromeos/login/default_pinned_apps_field_trial.cc (left): https://codereview.chromium.org/944433002/diff/220001/chrome/browser/chromeos... chrome/browser/chromeos/login/default_pinned_apps_field_trial.cc:26: void MigratePrefs(PrefService* local_state) { On 2015/02/24 20:55:20, gab wrote: > Delete the rest of this file and its header in fact, it's now only registering > an unused pref :-). ah, missed that. Done https://codereview.chromium.org/944433002/diff/220001/chrome/browser/prefs/br... File chrome/browser/prefs/browser_prefs.cc (right): https://codereview.chromium.org/944433002/diff/220001/chrome/browser/prefs/br... chrome/browser/prefs/browser_prefs.cc:520: // go here. On 2015/02/24 20:55:20, gab wrote: > Remove this comment I guess now that it comments nothing (or add a generic note > that future ones should go here, but I'm worried people would miss that and put > them somewhere else anyways so might as well remove it). make sense. We actually have this comment on line 362 now, so they can be deposited there anyway.
rkaplow@chromium.org changed reviewers: + piman@chromium.org
piman@chromium.org: Please review changes in browser/chromeos/login
lgtm, thanks! Please modify title / first line of CL description with: s/MigrateUserPref/MigrateBrowserPrefs. Also, add a line break after the title in the CL description (and another empty line before continuing the description -- i.e., description should start with "Title\n\nDescription". Cheers, Gab
rkaplow@chromium.org changed reviewers: + nkostylev@chromium.org - piman@chromium.org
Add nkostylev for browser/chromeos/login
piman@chromium.org changed reviewers: + piman@chromium.org
lgtm
The CQ bit was checked by rkaplow@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org, noms@chromium.org Link to the patchset: https://codereview.chromium.org/944433002/#ps240001 (title: "more comments. removing default_pinned_apps-field_trial")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/944433002/240001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2015/02/26 14:43:01, I haz the power (commit-bot) wrote: > Try jobs failed on following builders: > linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) Had to make one last small change - removal of 2 lines in browser_prefs related to the newly removed default_pinned_apps_field_trial.h
The CQ bit was checked by rkaplow@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org, noms@chromium.org, gab@chromium.org, piman@chromium.org Link to the patchset: https://codereview.chromium.org/944433002/#ps260001 (title: "Remove include/call to default_pinned_apps")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/944433002/260001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by rkaplow@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/944433002/260001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
lgtm
gab: hopefully last change. I deleted the test PreservedWindowPlacementIsMigrated. I believe we should no longer need it. Do a quick check?
On 2015/03/02 15:44:16, rkaplow wrote: > gab: hopefully last change. I deleted the test > PreservedWindowPlacementIsMigrated. > I believe we should no longer need it. > Do a quick check? I ran trybots and the failing jobs are not passing.
On 2015/03/02 16:45:35, rkaplow wrote: > On 2015/03/02 15:44:16, rkaplow wrote: > > gab: hopefully last change. I deleted the test > > PreservedWindowPlacementIsMigrated. > > I believe we should no longer need it. > > Do a quick check? > > I ran trybots and the failing jobs are not passing. er, *now*, not not.
Woot, more code removal :-) -- I think that should be the end of the removal exploration though ;-)! Cheers, Gab https://codereview.chromium.org/944433002/diff/280001/chrome/browser/prefs/pr... File chrome/browser/prefs/pref_service_browsertest.cc (right): https://codereview.chromium.org/944433002/diff/280001/chrome/browser/prefs/pr... chrome/browser/prefs/pref_service_browsertest.cc:55: explicit PreferenceServiceTest(bool new_profile) : new_profile_(new_profile) { Remove bool param (and member) here (now never used in false mode) and simplify the rest of this test fixture accordingly.
https://codereview.chromium.org/944433002/diff/280001/chrome/browser/prefs/pr... File chrome/browser/prefs/pref_service_browsertest.cc (right): https://codereview.chromium.org/944433002/diff/280001/chrome/browser/prefs/pr... chrome/browser/prefs/pref_service_browsertest.cc:55: explicit PreferenceServiceTest(bool new_profile) : new_profile_(new_profile) { On 2015/03/02 17:21:18, gab wrote: > Remove bool param (and member) here (now never used in false mode) and simplify > the rest of this test fixture accordingly. Done. Sorry I'm being a bit myopic with changes!
Last couple nits :-) https://codereview.chromium.org/944433002/diff/320001/chrome/browser/prefs/br... File chrome/browser/prefs/browser_prefs.cc (right): https://codereview.chromium.org/944433002/diff/320001/chrome/browser/prefs/br... chrome/browser/prefs/browser_prefs.cc:539: // This method should be periodically pruned of year+ old migrations. Indent. https://codereview.chromium.org/944433002/diff/320001/chrome/browser/prefs/br... chrome/browser/prefs/browser_prefs.cc:541: // Added 05/2014. Comment inside ifdef (and thus indented). https://codereview.chromium.org/944433002/diff/320001/chrome/browser/prefs/br... chrome/browser/prefs/browser_prefs.cc:546: // Added 08/2014. Comment inside ifdef (and thus indented). https://codereview.chromium.org/944433002/diff/320001/chrome/browser/prefs/pr... File chrome/browser/prefs/pref_service_browsertest.cc (right): https://codereview.chromium.org/944433002/diff/320001/chrome/browser/prefs/pr... chrome/browser/prefs/pref_service_browsertest.cc:67: CHECK(base::CreateDirectory(tmp_pref_file_)); cleanup nit: s/CHECK/ASSERT_TRUE (asserts are used over CHECKs in tests, might as well clean it up while we're here)
https://codereview.chromium.org/944433002/diff/320001/chrome/browser/prefs/br... File chrome/browser/prefs/browser_prefs.cc (right): https://codereview.chromium.org/944433002/diff/320001/chrome/browser/prefs/br... chrome/browser/prefs/browser_prefs.cc:539: // This method should be periodically pruned of year+ old migrations. On 2015/03/02 19:28:09, gab wrote: > Indent. I agree, but what happened here was git cl format actually puts it here. I suspect it is getting confused by the #ifdef below. I would prefer to put it like this, as otherwise the git cl format will be attempting to switch it back. I'd suggest filing a bug to have it handle this case correctly. https://codereview.chromium.org/944433002/diff/320001/chrome/browser/prefs/br... chrome/browser/prefs/browser_prefs.cc:541: // Added 05/2014. On 2015/03/02 19:28:09, gab wrote: > Comment inside ifdef (and thus indented). Done. https://codereview.chromium.org/944433002/diff/320001/chrome/browser/prefs/br... chrome/browser/prefs/browser_prefs.cc:546: // Added 08/2014. On 2015/03/02 19:28:09, gab wrote: > Comment inside ifdef (and thus indented). Done. https://codereview.chromium.org/944433002/diff/320001/chrome/browser/prefs/pr... File chrome/browser/prefs/pref_service_browsertest.cc (right): https://codereview.chromium.org/944433002/diff/320001/chrome/browser/prefs/pr... chrome/browser/prefs/pref_service_browsertest.cc:67: CHECK(base::CreateDirectory(tmp_pref_file_)); On 2015/03/02 19:28:09, gab wrote: > cleanup nit: s/CHECK/ASSERT_TRUE > > (asserts are used over CHECKs in tests, might as well clean it up while we're > here) Done.
https://codereview.chromium.org/944433002/diff/320001/chrome/browser/prefs/br... File chrome/browser/prefs/browser_prefs.cc (right): https://codereview.chromium.org/944433002/diff/320001/chrome/browser/prefs/br... chrome/browser/prefs/browser_prefs.cc:539: // This method should be periodically pruned of year+ old migrations. On 2015/03/02 19:36:35, rkaplow wrote: > On 2015/03/02 19:28:09, gab wrote: > > Indent. > > I agree, but what happened here was git cl format actually puts it here. > I suspect it is getting confused by the #ifdef below. > > I would prefer to put it like this, as otherwise the git cl format will be > attempting to switch it back. I'd suggest filing a bug to have it handle this > case correctly. Actually I think this highlights that the comment should really be outside the method (which hints about implementation details for the entire method) rather than inside (which would hint that it's a comment about the next statement). I debated that in my initial review but figured it was okay, if the format tool insists, it hints further that it doesn't belong there IMO. If you want to leave it there, please indent it despite the tool's desire and file a bug with the clang-format team for Chromium (there is a chromium-dev thread somewhere on how to do that). https://codereview.chromium.org/944433002/diff/320001/chrome/browser/prefs/br... chrome/browser/prefs/browser_prefs.cc:554: PrefService* profile_prefs = profile->GetPrefs(); Actually these two lines particularly highlight what I'm trying to say above, as-is it looks like the comment on 553 is about the statement on 554 which it's not. I'm strongly in favor of moving the comment out of the method (but keeping it in the impl (not the header) as it's really an implementation detail).
entirely agree. https://codereview.chromium.org/944433002/diff/320001/chrome/browser/prefs/br... File chrome/browser/prefs/browser_prefs.cc (right): https://codereview.chromium.org/944433002/diff/320001/chrome/browser/prefs/br... chrome/browser/prefs/browser_prefs.cc:539: // This method should be periodically pruned of year+ old migrations. On 2015/03/02 20:05:08, gab wrote: > On 2015/03/02 19:36:35, rkaplow wrote: > > On 2015/03/02 19:28:09, gab wrote: > > > Indent. > > > > I agree, but what happened here was git cl format actually puts it here. > > I suspect it is getting confused by the #ifdef below. > > > > I would prefer to put it like this, as otherwise the git cl format will be > > attempting to switch it back. I'd suggest filing a bug to have it handle this > > case correctly. > > Actually I think this highlights that the comment should really be outside the > method (which hints about implementation details for the entire method) rather > than inside (which would hint that it's a comment about the next statement). > > I debated that in my initial review but figured it was okay, if the format tool > insists, it hints further that it doesn't belong there IMO. > > If you want to leave it there, please indent it despite the tool's desire and > file a bug with the clang-format team for Chromium (there is a chromium-dev > thread somewhere on how to do that). Done. https://codereview.chromium.org/944433002/diff/320001/chrome/browser/prefs/br... chrome/browser/prefs/browser_prefs.cc:554: PrefService* profile_prefs = profile->GetPrefs(); On 2015/03/02 20:05:08, gab wrote: > Actually these two lines particularly highlight what I'm trying to say above, > as-is it looks like the comment on 553 is about the statement on 554 which it's > not. > > I'm strongly in favor of moving the comment out of the method (but keeping it in > the impl (not the header) as it's really an implementation detail). Done.
lgtm, thanks a lot for this 200+ lines cleanup :-)!
The CQ bit was checked by rkaplow@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org, noms@chromium.org, nkostylev@chromium.org, piman@chromium.org Link to the patchset: https://codereview.chromium.org/944433002/#ps360001 (title: "move comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/944433002/360001
One last change, I think. changed ASSERT_TRUE to EXPECT_TRUE in bool SetUpUserDataDirectory()
On 2015/03/02 20:59:05, rkaplow wrote: > One last change, I think. changed ASSERT_TRUE to EXPECT_TRUE in bool > SetUpUserDataDirectory() lgtm
The CQ bit was checked by rkaplow@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org, noms@chromium.org, nkostylev@chromium.org, piman@chromium.org Link to the patchset: https://codereview.chromium.org/944433002/#ps370010 (title: "change asserts to expect in setupuserdatadirectory")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/944433002/370010
Message was sent while issue was closed.
Committed patchset #20 (id:370010)
Message was sent while issue was closed.
Patchset 20 (id:??) landed as https://crrev.com/f216cafc43a6f43937ecc2e178f16217bd736c4b Cr-Commit-Position: refs/heads/master@{#318780} |
