Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
24930003:
Migrate startup URLs pref. (Closed)
|
Created:
7 years, 2 months ago by MAD Modified:
7 years, 2 months ago Reviewers:
Bernhard Bauer, James Hawkins, Alexei Svitkine (slow), vabr (Chromium), robertshield, battre, Nicolas Zea, grt (UTC plus 2) CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionMigrate startup URLs pref.
TBRing battre@ for tiny profile_resetter change, other OWNERs appear to be out as well.
TBR=battre
BUG=299290
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=229536
Patch Set 1 #
Total comments: 10
Patch Set 2 : Now with sync support. #
Total comments: 12
Patch Set 3 : CR Comments - 1. #
Total comments: 3
Patch Set 4 : Address Nicolas' nit, Greg's feedback. #
Total comments: 19
Patch Set 5 : Dear Greg, #
Total comments: 2
Patch Set 6 : Updating all references to deprecated pref name. #Patch Set 7 : rebase #Patch Set 8 : Rebase again after dcommitting line ending change. #Patch Set 9 : Rebase again, this time picking up the EOL fix. #
Total comments: 4
Patch Set 10 : Add rather than update previously unseen migrated prefs. #
Total comments: 8
Patch Set 11 : Move to using a map for sync data entries. Add synced old pref value names to synced_preferences_. #
Total comments: 2
Patch Set 12 : Nicolas' feedback. Add unit tests. #
Total comments: 12
Patch Set 13 : Nicolas' feedback, fix ModelAssociationCloudHasOldMigratedData test. #Patch Set 14 : #
Total comments: 22
Patch Set 15 : MAD and Alexei feedback. #
Total comments: 2
Patch Set 16 : Update histograms strings. #
Total comments: 18
Patch Set 17 : Bernhard's feedback. #Patch Set 18 : Rebase #Patch Set 19 : Rename remaining old pref value. #Messages
Total messages: 201 (0 generated)
will any changes be required on the sync side of the universe? https://codereview.chromium.org/24930003/diff/1/chrome/browser/prefs/pref_met... File chrome/browser/prefs/pref_metrics_service.cc (right): https://codereview.chromium.org/24930003/diff/1/chrome/browser/prefs/pref_met... chrome/browser/prefs/pref_metrics_service.cc:167: LogStartupURLsPrefChange("StartupURLsResetTime", url_list); to match when Log..() is called by the ChangeRegistrar, use "Settings.StartupURLsResetTime.PushedToSync". alternatively, call OnPrefChanged to simulate a true pref change. https://codereview.chromium.org/24930003/diff/1/chrome/browser/prefs/pref_met... chrome/browser/prefs/pref_metrics_service.cc:260: const std::string& histogram_name, why not use this on line 273 (it should be "Settings.StartupURLsResetTime.PushedToSync" when invoked by the OnPrefChanged)? https://codereview.chromium.org/24930003/diff/1/chrome/browser/prefs/pref_met... chrome/browser/prefs/pref_metrics_service.cc:266: // Empty migration time delta will identify initial migration. do you want to log initial migration?
Thanks... I'm not sure about the sync aspect of renaming a pref, I will investigate... https://codereview.chromium.org/24930003/diff/1/chrome/browser/prefs/pref_met... File chrome/browser/prefs/pref_metrics_service.cc (right): https://codereview.chromium.org/24930003/diff/1/chrome/browser/prefs/pref_met... chrome/browser/prefs/pref_metrics_service.cc:167: LogStartupURLsPrefChange("StartupURLsResetTime", url_list); I don't think I need that since this is not a sync'able pref. https://codereview.chromium.org/24930003/diff/1/chrome/browser/prefs/pref_met... chrome/browser/prefs/pref_metrics_service.cc:260: const std::string& histogram_name, Because this is not a sync'able pref. https://codereview.chromium.org/24930003/diff/1/chrome/browser/prefs/pref_met... chrome/browser/prefs/pref_metrics_service.cc:266: // Empty migration time delta will identify initial migration. It is logged with a value of 0.0.
https://codereview.chromium.org/24930003/diff/1/chrome/browser/prefs/pref_met... File chrome/browser/prefs/pref_metrics_service.cc (right): https://codereview.chromium.org/24930003/diff/1/chrome/browser/prefs/pref_met... chrome/browser/prefs/pref_metrics_service.cc:260: const std::string& histogram_name, On 2013/09/27 14:54:20, MAD wrote: > Because this is not a sync'able pref. This code is confusing as-is. There are two ways to get into this function: from PrefMetricsService::OnPrefChanged by way of the SyncedPrefChangeRegistrar, and from the direct call on line 167. In the former case, histogram_name is "Settings.StartupURLsResetTime.PushedToSync". In the latter case, histogram_name is "StartupURLsResetTime". In both cases it is ignored. Please change something here. I think things are more obvious if the histogram name is consistent with the other histograms, and if argument values aren't different and ignored. https://codereview.chromium.org/24930003/diff/1/chrome/browser/prefs/pref_met... chrome/browser/prefs/pref_metrics_service.cc:266: // Empty migration time delta will identify initial migration. On 2013/09/27 14:54:20, MAD wrote: > It is logged with a value of 0.0. If I understand the use of histograms here, there are only 50 buckets for a range from 60 to 604800 seconds, so the 0.0 value may be in the same bucket as a fairly large reset-delta range. Will we be able to extract meaning from this first bucket?
Go it... Working on an updated patch... Thanks! BYE MAD https://codereview.chromium.org/24930003/diff/1/chrome/browser/prefs/pref_met... File chrome/browser/prefs/pref_metrics_service.cc (right): https://codereview.chromium.org/24930003/diff/1/chrome/browser/prefs/pref_met... chrome/browser/prefs/pref_metrics_service.cc:260: const std::string& histogram_name, OK, you convinced me, I'll work something up... https://codereview.chromium.org/24930003/diff/1/chrome/browser/prefs/pref_met... chrome/browser/prefs/pref_metrics_service.cc:266: // Empty migration time delta will identify initial migration. D'Ho! Of course... I didn't realize that because all my tests needed enough time to reach the next bucket... Thanks!
Here's a new version that works well while sync'ing with older version of Chrome... Let me know what you think... Thanks! BYE MAD
looking really good! some comments: https://codereview.chromium.org/24930003/diff/14001/chrome/browser/prefs/sess... File chrome/browser/prefs/session_startup_pref.cc (right): https://codereview.chromium.org/24930003/diff/14001/chrome/browser/prefs/sess... chrome/browser/prefs/session_startup_pref.cc:187: now - last_migration_time, in theory, this could be negative. What's the effect on the uma metric when that happens? https://codereview.chromium.org/24930003/diff/14001/chrome/common/pref_names.cc File chrome/common/pref_names.cc (right): https://codereview.chromium.org/24930003/diff/14001/chrome/common/pref_names.... chrome/common/pref_names.cc:87: // Old startup url pref and migration time delta as a double. Mention in the comment the units of time. https://codereview.chromium.org/24930003/diff/14001/chrome/common/pref_names.... chrome/common/pref_names.cc:89: const char kRestoreStartupURLsMigrated[] = "session.startup_urls_migrated"; s/Migrated/MigrationTime
https://codereview.chromium.org/24930003/diff/14001/chrome/browser/prefs/pref... File chrome/browser/prefs/pref_model_associator.cc (right): https://codereview.chromium.org/24930003/diff/14001/chrome/browser/prefs/pref... chrome/browser/prefs/pref_model_associator.cc:116: if (!CreatePrefSyncData(pref_name, *new_value, &sync_data)) { not that this is only going to update the new synced value, not the old. You probably need to add another case here to update the old value with the result of the merge. https://codereview.chromium.org/24930003/diff/14001/chrome/browser/prefs/pref... chrome/browser/prefs/pref_model_associator.cc:191: sync_pref_name = prefs::kURLsToRestoreOnStartup; Maybe mention that we'll be merging any difference between the new and old values and syncing them back?
I think it would be nice to do this without special casing particular preferences in the code here and there. Is it possible to generalize it a little bit? For example, could you make the code handle the general case of changing the name of a syncable pref, and then have a static mapping that contains the actual pref names? https://codereview.chromium.org/24930003/diff/14001/chrome/browser/prefs/pref... File chrome/browser/prefs/pref_model_associator.cc (right): https://codereview.chromium.org/24930003/diff/14001/chrome/browser/prefs/pref... chrome/browser/prefs/pref_model_associator.cc:195: // reconstruct all sync data for it's datatype (therefore having it's -> its
Done / Answered... Thanks! BYE MAD https://codereview.chromium.org/24930003/diff/14001/chrome/browser/prefs/pref... File chrome/browser/prefs/pref_model_associator.cc (right): https://codereview.chromium.org/24930003/diff/14001/chrome/browser/prefs/pref... chrome/browser/prefs/pref_model_associator.cc:116: if (!CreatePrefSyncData(pref_name, *new_value, &sync_data)) { On 2013/10/04 17:45:01, Nicolas Zea wrote: > not that this is only going to update the new synced value, not the old. You > probably need to add another case here to update the old value with the result > of the merge. Done. https://codereview.chromium.org/24930003/diff/14001/chrome/browser/prefs/pref... chrome/browser/prefs/pref_model_associator.cc:191: sync_pref_name = prefs::kURLsToRestoreOnStartup; On 2013/10/04 17:45:01, Nicolas Zea wrote: > Maybe mention that we'll be merging any difference between the new and old > values and syncing them back? Done. https://codereview.chromium.org/24930003/diff/14001/chrome/browser/prefs/pref... chrome/browser/prefs/pref_model_associator.cc:195: // reconstruct all sync data for it's datatype (therefore having On 2013/10/04 18:24:07, grt wrote: > it's -> its Done. https://codereview.chromium.org/24930003/diff/14001/chrome/browser/prefs/sess... File chrome/browser/prefs/session_startup_pref.cc (right): https://codereview.chromium.org/24930003/diff/14001/chrome/browser/prefs/sess... chrome/browser/prefs/session_startup_pref.cc:187: now - last_migration_time, On 2013/10/04 15:16:15, robertshield wrote: > in theory, this could be negative. What's the effect on the uma metric when that > happens? I just tried it and it would go in the 0 bucket (if the UMA server reacts like chrome://histograms). If you prefer we could also make it explicit, e.g., now > last_migration_time ? now - last_migration_time : 0 https://codereview.chromium.org/24930003/diff/14001/chrome/common/pref_names.cc File chrome/common/pref_names.cc (right): https://codereview.chromium.org/24930003/diff/14001/chrome/common/pref_names.... chrome/common/pref_names.cc:87: // Old startup url pref and migration time delta as a double. On 2013/10/04 15:16:15, robertshield wrote: > Mention in the comment the units of time. Done. https://codereview.chromium.org/24930003/diff/14001/chrome/common/pref_names.... chrome/common/pref_names.cc:89: const char kRestoreStartupURLsMigrated[] = "session.startup_urls_migrated"; On 2013/10/04 15:16:15, robertshield wrote: > s/Migrated/MigrationTime Actually, that's on purpose...I wanted to use the same terminology as above.
sync changes LGTM https://codereview.chromium.org/24930003/diff/24001/chrome/browser/prefs/pref... File chrome/browser/prefs/pref_model_associator.cc (right): https://codereview.chromium.org/24930003/diff/24001/chrome/browser/prefs/pref... chrome/browser/prefs/pref_model_associator.cc:127: syncer::SyncData sync_data; nit: CreatePrefSyncData will overwrite a SyncData object anyways, so no need to instantiate a new one.
@Nicolas: one quick question about the sync changes. Also, I'll be taking over finishing off this patch for MAD, hopefully I can do so with the same rietveld issue. https://codereview.chromium.org/24930003/diff/24001/chrome/browser/prefs/pref... File chrome/browser/prefs/pref_model_associator.cc (right): https://codereview.chromium.org/24930003/diff/24001/chrome/browser/prefs/pref... chrome/browser/prefs/pref_model_associator.cc:432: NotifySyncedPrefObservers(name, true /*from_sync*/); @Nicolas: it looks like this uses the old "name" rather than the potentially updated "pref_name". Same on line 436 below. Is this right?
https://codereview.chromium.org/24930003/diff/24001/chrome/browser/prefs/pref... File chrome/browser/prefs/pref_model_associator.cc (right): https://codereview.chromium.org/24930003/diff/24001/chrome/browser/prefs/pref... chrome/browser/prefs/pref_model_associator.cc:432: NotifySyncedPrefObservers(name, true /*from_sync*/); On 2013/10/07 16:42:57, robertshield wrote: > @Nicolas: it looks like this uses the old "name" rather than the potentially > updated "pref_name". Same on line 436 below. Is this right? I missed this, but as it happens it's okay to leave. NotifySyncedPrefObservers is only important for local listeners to the preference, and all local listeners should be listening to the new preference. |synced_preferences_| is only used when listening to local pref changes, which for the old pref doesn't matter.
@grt: please could you take a look and see if this resembles what you had in mind.
pretty much lgtm with the tweaks below. please wait for Nicolas to review the details of the preference handling, since i have zero knowledge in that domain. thanks. https://codereview.chromium.org/24930003/diff/34001/chrome/browser/prefs/pref... File chrome/browser/prefs/pref_model_associator.cc (right): https://codereview.chromium.org/24930003/diff/34001/chrome/browser/prefs/pref... chrome/browser/prefs/pref_model_associator.cc:52: // the old and new preference names here will do the right thing by the sync what is "the right thing"? maybe spell out that it causes values under the old name that come down from sync to warp to the new name (assuming that's what it means). https://codereview.chromium.org/24930003/diff/34001/chrome/browser/prefs/pref... chrome/browser/prefs/pref_model_associator.cc:55: struct MigratedPreferences { const struct mega nit: this type doesn't need a name, but maybe it's happier with one. https://codereview.chromium.org/24930003/diff/34001/chrome/browser/prefs/pref... chrome/browser/prefs/pref_model_associator.cc:56: const char* old_name; const char* const https://codereview.chromium.org/24930003/diff/34001/chrome/browser/prefs/pref... chrome/browser/prefs/pref_model_associator.cc:63: for (int i = 0; i < arraysize(kMigratedPreferences); ++i) { int -> size_t since you're comparing with arraysize, which returns size_t (here and line 71). https://codereview.chromium.org/24930003/diff/34001/chrome/browser/prefs/pref... chrome/browser/prefs/pref_model_associator.cc:67: return ""; "" -> std::string() here and line 75 https://codereview.chromium.org/24930003/diff/34001/chrome/browser/prefs/pref... chrome/browser/prefs/pref_model_associator.cc:460: } else if (!IsPrefRegistered(pref_name)) { in the block above, it's safe to assume that new_name.c_str() is always registered? https://codereview.chromium.org/24930003/diff/34001/chrome/browser/prefs/sess... File chrome/browser/prefs/session_startup_pref.cc (right): https://codereview.chromium.org/24930003/diff/34001/chrome/browser/prefs/sess... chrome/browser/prefs/session_startup_pref.cc:185: DCHECK(now > last_migration_time); DCHECK_GT for safety, i suggest adding: if (now < last_migration_time) last_migration_time = now; i don't know what exactly happens around DST, leap seconds, flaky BIOS clocks, unpredictable rounding, IEEE doubles, etc, but may as well play it safe. https://codereview.chromium.org/24930003/diff/34001/chrome/common/pref_names.cc File chrome/common/pref_names.cc (right): https://codereview.chromium.org/24930003/diff/34001/chrome/common/pref_names.... chrome/common/pref_names.cc:87: // Old startup url pref and migration time delta as a double in miliseconds. kRestoreStartupURLsMigrated is not a delta, nor is it in milliseconds. it is the time at which the last migration took place, measured in the (fractional) number number of seconds since the UNIX epoch. given that this is an internal value used for our own computations, why not use an int64 to hold it, and use Time::{From,To}InternalValue to store/load it? this seems more efficient and more accurate. https://codereview.chromium.org/24930003/diff/34001/chrome/common/pref_names.h File chrome/common/pref_names.h (right): https://codereview.chromium.org/24930003/diff/34001/chrome/common/pref_names.... chrome/common/pref_names.h:37: extern const char kURLsToRestoreOnStartupOld[]; is there an echo in here?
Thanks Greg, PTAL. @Nicolas: would you mind taking one more look over the changes in pref_model_associator.cc to make sure they still look alright? Notably there is a small behaviour change at line 464 here https://codereview.chromium.org/24930003/diff2/34001:45001/chrome/browser/pre... that would be great if you could sign off on. https://codereview.chromium.org/24930003/diff/34001/chrome/browser/prefs/pref... File chrome/browser/prefs/pref_model_associator.cc (right): https://codereview.chromium.org/24930003/diff/34001/chrome/browser/prefs/pref... chrome/browser/prefs/pref_model_associator.cc:52: // the old and new preference names here will do the right thing by the sync On 2013/10/07 20:15:16, grt wrote: > what is "the right thing"? maybe spell out that it causes values under the old > name that come down from sync to warp to the new name (assuming that's what it > means). Done. https://codereview.chromium.org/24930003/diff/34001/chrome/browser/prefs/pref... chrome/browser/prefs/pref_model_associator.cc:55: struct MigratedPreferences { On 2013/10/07 20:15:16, grt wrote: > const struct Done. > mega nit: this type doesn't need a name, but maybe it's happier with one. I prefer useful symbol names and dislike anonymous types. https://codereview.chromium.org/24930003/diff/34001/chrome/browser/prefs/pref... chrome/browser/prefs/pref_model_associator.cc:56: const char* old_name; On 2013/10/07 20:15:16, grt wrote: > const char* const Done. https://codereview.chromium.org/24930003/diff/34001/chrome/browser/prefs/pref... chrome/browser/prefs/pref_model_associator.cc:63: for (int i = 0; i < arraysize(kMigratedPreferences); ++i) { On 2013/10/07 20:15:16, grt wrote: > int -> size_t since you're comparing with arraysize, which returns size_t (here > and line 71). Done. https://codereview.chromium.org/24930003/diff/34001/chrome/browser/prefs/pref... chrome/browser/prefs/pref_model_associator.cc:67: return ""; On 2013/10/07 20:15:16, grt wrote: > "" -> std::string() here and line 75 Done. https://codereview.chromium.org/24930003/diff/34001/chrome/browser/prefs/pref... chrome/browser/prefs/pref_model_associator.cc:460: } else if (!IsPrefRegistered(pref_name)) { On 2013/10/07 20:15:16, grt wrote: > in the block above, it's safe to assume that new_name.c_str() is always > registered? That's interesting. MAD's patch was doing so, but looking at line 464 I am not so sure. I see no reason to bypass the IsPrefRegistered check here just because of the migration, so I'll drop the 'else'. @Nicolas, please holler if you object. https://codereview.chromium.org/24930003/diff/34001/chrome/browser/prefs/sess... File chrome/browser/prefs/session_startup_pref.cc (right): https://codereview.chromium.org/24930003/diff/34001/chrome/browser/prefs/sess... chrome/browser/prefs/session_startup_pref.cc:185: DCHECK(now > last_migration_time); On 2013/10/07 20:15:16, grt wrote: > DCHECK_GT Doesn't seem to work with base::Time objects. > for safety, i suggest adding: > if (now < last_migration_time) > last_migration_time = now; > i don't know what exactly happens around DST, leap seconds, flaky BIOS clocks, > unpredictable rounding, IEEE doubles, etc, but may as well play it safe. I asked MAD about this here: https://codereview.chromium.org/24930003/diff/14001/chrome/browser/prefs/sess... and the behaviour is apparently that 0 will be recorded anyway for negative values. I'll clamp it to zero anyway to make it explicit. https://codereview.chromium.org/24930003/diff/34001/chrome/common/pref_names.cc File chrome/common/pref_names.cc (right): https://codereview.chromium.org/24930003/diff/34001/chrome/common/pref_names.... chrome/common/pref_names.cc:87: // Old startup url pref and migration time delta as a double in miliseconds. On 2013/10/07 20:15:16, grt wrote: > kRestoreStartupURLsMigrated is not a delta, nor is it in milliseconds. This is true, I'll update the comment. it is the > time at which the last migration took place, measured in the (fractional) number > number of seconds since the UNIX epoch. given that this is an internal value > used for our own computations, why not use an int64 to hold it, and use > Time::{From,To}InternalValue to store/load it? this seems more efficient and > more accurate. Probably not more efficient as json doesn't have a big int type, while it does have a double type. The prefs system will convert int64s to strings and back under the hood. Likely more accurate, though not sure if meaningfully so. I'll change it anyway since this value appears to only be used for serialization and base::Time explicitly recommends using int64s (via ToInternalValue) for serialization. https://codereview.chromium.org/24930003/diff/34001/chrome/common/pref_names.h File chrome/common/pref_names.h (right): https://codereview.chromium.org/24930003/diff/34001/chrome/common/pref_names.... chrome/common/pref_names.h:37: extern const char kURLsToRestoreOnStartupOld[]; On 2013/10/07 20:15:16, grt wrote: > is there an echo in here? here here here... ugh, merge splatter, thanks for catching.
lgtm https://codereview.chromium.org/24930003/diff/34001/chrome/browser/prefs/sess... File chrome/browser/prefs/session_startup_pref.cc (right): https://codereview.chromium.org/24930003/diff/34001/chrome/browser/prefs/sess... chrome/browser/prefs/session_startup_pref.cc:185: DCHECK(now > last_migration_time); On 2013/10/08 01:52:13, robertshield wrote: > On 2013/10/07 20:15:16, grt wrote: > > DCHECK_GT > > Doesn't seem to work with base::Time objects. pity. > > for safety, i suggest adding: > > if (now < last_migration_time) > > last_migration_time = now; > > i don't know what exactly happens around DST, leap seconds, flaky BIOS clocks, > > unpredictable rounding, IEEE doubles, etc, but may as well play it safe. > > I asked MAD about this here: > https://codereview.chromium.org/24930003/diff/14001/chrome/browser/prefs/sess... > and the behaviour is apparently that 0 will be recorded anyway for negative > values. oh, sorry for repeating. https://codereview.chromium.org/24930003/diff/45001/chrome/browser/prefs/sess... File chrome/browser/prefs/session_startup_pref.cc (right): https://codereview.chromium.org/24930003/diff/45001/chrome/browser/prefs/sess... chrome/browser/prefs/session_startup_pref.cc:177: base::Time::Now().ToInternalValue()); nit: indentation
new sync logic LGTM
https://codereview.chromium.org/24930003/diff/80001/chrome/browser/prefs/pref... File chrome/browser/prefs/pref_model_associator.cc (right): https://codereview.chromium.org/24930003/diff/80001/chrome/browser/prefs/pref... chrome/browser/prefs/pref_model_associator.cc:172: syncer::SyncChange::ACTION_UPDATE, It occurs to me this isn't always correct (and this is likely the reason the tests are failing). We should only UPDATE if the old pref exists, else we should ADD. It's a bit tricky to know if a synced version of a preference exists at this point, because we're still iterating over the set of synced preferences during the MergeDataAndStartSyncing call. It might be better to wait to push these changes until the end of MergeDataAndStartSyncing, when you know whether a synced version of the old pref exists or not. https://codereview.chromium.org/24930003/diff/80001/chrome/browser/prefs/pref... chrome/browser/prefs/pref_model_associator.cc:594: syncer::SyncChange::ACTION_UPDATE, here too. We can check synced_preferences_.count(old_name) to see if it exists or not
Thanks Nicolas, PTAL https://codereview.chromium.org/24930003/diff/45001/chrome/browser/prefs/sess... File chrome/browser/prefs/session_startup_pref.cc (right): https://codereview.chromium.org/24930003/diff/45001/chrome/browser/prefs/sess... chrome/browser/prefs/session_startup_pref.cc:177: base::Time::Now().ToInternalValue()); On 2013/10/08 13:20:11, grt wrote: > nit: indentation Done. https://codereview.chromium.org/24930003/diff/80001/chrome/browser/prefs/pref... File chrome/browser/prefs/pref_model_associator.cc (right): https://codereview.chromium.org/24930003/diff/80001/chrome/browser/prefs/pref... chrome/browser/prefs/pref_model_associator.cc:172: syncer::SyncChange::ACTION_UPDATE, On 2013/10/09 17:40:52, Nicolas Zea wrote: > It occurs to me this isn't always correct (and this is likely the reason the > tests are failing). We should only UPDATE if the old pref exists, else we should > ADD. > > It's a bit tricky to know if a synced version of a preference exists at this > point, because we're still iterating over the set of synced preferences during > the MergeDataAndStartSyncing call. It might be better to wait to push these > changes until the end of MergeDataAndStartSyncing, when you know whether a > synced version of the old pref exists or not. Done. https://codereview.chromium.org/24930003/diff/80001/chrome/browser/prefs/pref... chrome/browser/prefs/pref_model_associator.cc:594: syncer::SyncChange::ACTION_UPDATE, On 2013/10/09 17:40:52, Nicolas Zea wrote: > here too. We can check synced_preferences_.count(old_name) to see if it exists > or not Done.
Some comments. Additionally, I think this patch has reached sufficient complexity that some pref_model_associator_unittests are meritted. https://codereview.chromium.org/24930003/diff/89001/chrome/browser/prefs/pref... File chrome/browser/prefs/pref_model_associator.cc (right): https://codereview.chromium.org/24930003/diff/89001/chrome/browser/prefs/pref... chrome/browser/prefs/pref_model_associator.cc:206: synced_preferences_.insert(pref_name); I don't think this will ever add the old pref name to the synced_preferences_ list, which means that the below check in synced_preferences_ for the name won't work properly. It probably makes sense to have this insert preference.name() now. https://codereview.chromium.org/24930003/diff/89001/chrome/browser/prefs/pref... File chrome/browser/prefs/pref_model_associator.h (right): https://codereview.chromium.org/24930003/diff/89001/chrome/browser/prefs/pref... chrome/browser/prefs/pref_model_associator.h:114: typedef std::vector<std::pair<std::string, syncer::SyncData> > why switch to a vector? I think a map is still correct. Consider, for example, the case where you have both a synced version of the new preference and one of the old preference, and they don't match with the local version of the new preference. As you're iterating, you'll encounter the synced version of one with value A, merge it with the local version with value L, and wind up with AL as the new synced value you need to push back to the old pref. You'll then continue, see the second synced pref with value B, merge it with the local value AL, and wind up with ABL, which is _now_ the value you should be syncing back to _both. I think it would make more sense to use a map, so that the second time you generate the value, you'll overwrite the first value (although in practice passing two changes to the same value is the same as just passing the second change).
Thanks Nicolas, new patch uploaded. Please check the comment about inserting into synced_preferences_ to see whether I understood you correctly. Figuring out tests now, figure some combinations of merging local / remote migrated values are in order, do have any other recommendations for tests? https://codereview.chromium.org/24930003/diff/89001/chrome/browser/prefs/pref... File chrome/browser/prefs/pref_model_associator.cc (right): https://codereview.chromium.org/24930003/diff/89001/chrome/browser/prefs/pref... chrome/browser/prefs/pref_model_associator.cc:206: synced_preferences_.insert(pref_name); On 2013/10/10 17:44:14, Nicolas Zea wrote: > I don't think this will ever add the old pref name to the synced_preferences_ > list, which means that the below check in synced_preferences_ for the name won't > work properly. Ah true, the pref will still get synced but subsequent updates will again run as ADD actions which I guess won't work as desired. It probably makes sense to have this insert preference.name() > now. I'm not 100% sure I understand this last bit. Do you mean rather that I should insert GetOldMigratedPreferenceName(pref_name) into synced_preferences_ (presumably ~line 173), or do you mean something else? https://codereview.chromium.org/24930003/diff/89001/chrome/browser/prefs/pref... File chrome/browser/prefs/pref_model_associator.h (right): https://codereview.chromium.org/24930003/diff/89001/chrome/browser/prefs/pref... chrome/browser/prefs/pref_model_associator.h:114: typedef std::vector<std::pair<std::string, syncer::SyncData> > On 2013/10/10 17:44:14, Nicolas Zea wrote: > why switch to a vector? I think a map is still correct. > > Consider, for example, the case where you have both a synced version of the new > preference and one of the old preference, and they don't match with the local > version of the new preference. > > As you're iterating, you'll encounter the synced version of one with value A, > merge it with the local version with value L, and wind up with AL as the new > synced value you need to push back to the old pref. You'll then continue, see > the second synced pref with value B, merge it with the local value AL, and wind > up with ABL, which is _now_ the value you should be syncing back to _both. > > I think it would make more sense to use a map, so that the second time you > generate the value, you'll overwrite the first value (although in practice > passing two changes to the same value is the same as just passing the second > change). Ok, I hadn't understood that MergeDataAndStartSyncing could iterate over the same preference name multiple times. I've switched it back to a map.
https://codereview.chromium.org/24930003/diff/89001/chrome/browser/prefs/pref... File chrome/browser/prefs/pref_model_associator.cc (right): https://codereview.chromium.org/24930003/diff/89001/chrome/browser/prefs/pref... chrome/browser/prefs/pref_model_associator.cc:206: synced_preferences_.insert(pref_name); On 2013/10/10 20:06:21, robertshield wrote: > On 2013/10/10 17:44:14, Nicolas Zea wrote: > > I don't think this will ever add the old pref name to the synced_preferences_ > > list, which means that the below check in synced_preferences_ for the name > won't > > work properly. > > Ah true, the pref will still get synced but subsequent updates will again run as > ADD actions which I guess won't work as desired. > > It probably makes sense to have this insert preference.name() > > now. > > I'm not 100% sure I understand this last bit. Do you mean rather that I should > insert GetOldMigratedPreferenceName(pref_name) into synced_preferences_ > (presumably ~line 173), or do you mean something else? > Basically, if the preference sync is passing to the model associator is the old preference, we need to somehow know that sync has an entity for the old preference already. preference.name() matches whatever sync has (pref_name is the one that would be the migrated pref), so I think it makes sense to insert preference.name() here. https://codereview.chromium.org/24930003/diff/89001/chrome/browser/prefs/pref... File chrome/browser/prefs/pref_model_associator.h (right): https://codereview.chromium.org/24930003/diff/89001/chrome/browser/prefs/pref... chrome/browser/prefs/pref_model_associator.h:114: typedef std::vector<std::pair<std::string, syncer::SyncData> > On 2013/10/10 20:06:21, robertshield wrote: > On 2013/10/10 17:44:14, Nicolas Zea wrote: > > why switch to a vector? I think a map is still correct. > > > > Consider, for example, the case where you have both a synced version of the > new > > preference and one of the old preference, and they don't match with the local > > version of the new preference. > > > > As you're iterating, you'll encounter the synced version of one with value A, > > merge it with the local version with value L, and wind up with AL as the new > > synced value you need to push back to the old pref. You'll then continue, see > > the second synced pref with value B, merge it with the local value AL, and > wind > > up with ABL, which is _now_ the value you should be syncing back to _both. > > > > I think it would make more sense to use a map, so that the second time you > > generate the value, you'll overwrite the first value (although in practice > > passing two changes to the same value is the same as just passing the second > > change). > > Ok, I hadn't understood that MergeDataAndStartSyncing could iterate over the > same preference name multiple times. I've switched it back to a map. Well, it's not that MergeDataAndStartSyncing will iterate over a preference multiple times (the SyncDataList has only one of each synced preference), it's that we may wind up with multiple writes to the same migrated preference. https://codereview.chromium.org/24930003/diff/101001/chrome/browser/prefs/pre... File chrome/browser/prefs/pref_model_associator.cc (right): https://codereview.chromium.org/24930003/diff/101001/chrome/browser/prefs/pre... chrome/browser/prefs/pref_model_associator.cc:159: syncer::SyncChange::ACTION_UPDATE, thinking about it some more, ACTION_UPDATE here might also be prone to errors if the migrated preference hasn't been synced yet. For example, if the old preference is synced, but the new one isn't yet, this will attempt to update the new preference, which is an error. I think you probably need to put this value into the map if pref_name is the new migrated name and preference.name() is the old migrated name.
Thanks Nicolas, made the changes you suggested and added some tests. PTAL and let me know if there are any other tests I could add. Thanks! https://codereview.chromium.org/24930003/diff/89001/chrome/browser/prefs/pref... File chrome/browser/prefs/pref_model_associator.cc (right): https://codereview.chromium.org/24930003/diff/89001/chrome/browser/prefs/pref... chrome/browser/prefs/pref_model_associator.cc:206: synced_preferences_.insert(pref_name); On 2013/10/10 20:35:09, Nicolas Zea wrote: > On 2013/10/10 20:06:21, robertshield wrote: > > On 2013/10/10 17:44:14, Nicolas Zea wrote: > > > I don't think this will ever add the old pref name to the > synced_preferences_ > > > list, which means that the below check in synced_preferences_ for the name > > won't > > > work properly. > > > > Ah true, the pref will still get synced but subsequent updates will again run > as > > ADD actions which I guess won't work as desired. > > > > It probably makes sense to have this insert preference.name() > > > now. > > > > I'm not 100% sure I understand this last bit. Do you mean rather that I should > > insert GetOldMigratedPreferenceName(pref_name) into synced_preferences_ > > (presumably ~line 173), or do you mean something else? > > > > Basically, if the preference sync is passing to the model associator is the old > preference, we need to somehow know that sync has an entity for the old > preference already. preference.name() matches whatever sync has (pref_name is > the one that would be the migrated pref), so I think it makes sense to insert > preference.name() here. Done. https://codereview.chromium.org/24930003/diff/89001/chrome/browser/prefs/pref... File chrome/browser/prefs/pref_model_associator.h (right): https://codereview.chromium.org/24930003/diff/89001/chrome/browser/prefs/pref... chrome/browser/prefs/pref_model_associator.h:114: typedef std::vector<std::pair<std::string, syncer::SyncData> > On 2013/10/10 20:35:09, Nicolas Zea wrote: > On 2013/10/10 20:06:21, robertshield wrote: > > On 2013/10/10 17:44:14, Nicolas Zea wrote: > > > why switch to a vector? I think a map is still correct. > > > > > > Consider, for example, the case where you have both a synced version of the > > new > > > preference and one of the old preference, and they don't match with the > local > > > version of the new preference. > > > > > > As you're iterating, you'll encounter the synced version of one with value > A, > > > merge it with the local version with value L, and wind up with AL as the new > > > synced value you need to push back to the old pref. You'll then continue, > see > > > the second synced pref with value B, merge it with the local value AL, and > > wind > > > up with ABL, which is _now_ the value you should be syncing back to _both. > > > > > > I think it would make more sense to use a map, so that the second time you > > > generate the value, you'll overwrite the first value (although in practice > > > passing two changes to the same value is the same as just passing the second > > > change). > > > > Ok, I hadn't understood that MergeDataAndStartSyncing could iterate over the > > same preference name multiple times. I've switched it back to a map. > > Well, it's not that MergeDataAndStartSyncing will iterate over a preference > multiple times (the SyncDataList has only one of each synced preference), it's > that we may wind up with multiple writes to the same migrated preference. Done. https://codereview.chromium.org/24930003/diff/101001/chrome/browser/prefs/pre... File chrome/browser/prefs/pref_model_associator.cc (right): https://codereview.chromium.org/24930003/diff/101001/chrome/browser/prefs/pre... chrome/browser/prefs/pref_model_associator.cc:159: syncer::SyncChange::ACTION_UPDATE, On 2013/10/10 20:35:09, Nicolas Zea wrote: > thinking about it some more, ACTION_UPDATE here might also be prone to errors if > the migrated preference hasn't been synced yet. For example, if the old > preference is synced, but the new one isn't yet, this will attempt to update the > new preference, which is an error. > > I think you probably need to put this value into the map if pref_name is the new > migrated name and preference.name() is the old migrated name. Done.
https://codereview.chromium.org/24930003/diff/111001/chrome/browser/prefs/pre... File chrome/browser/prefs/pref_model_associator.cc (right): https://codereview.chromium.org/24930003/diff/111001/chrome/browser/prefs/pre... chrome/browser/prefs/pref_model_associator.cc:156: if (preference.name() == old_pref_name && migrated_preference_list) { I think it's invalid to call InitPrefAndAssociate with an old preference and not have the migrated_preference_list. In that case, I'd leave the first condition, and have DCHECK(migrated_preference_list) within the body. https://codereview.chromium.org/24930003/diff/111001/chrome/browser/prefs/pre... chrome/browser/prefs/pref_model_associator.cc:181: synced_preferences_.insert(preference.name()); I think rather than having this here, you need to change the one at line 217 to use preference.name(). For example, if the migrated synced pref doesn't exist, pref_name will be for the migrated pref, and adding it into synced_preferences_ is invalid. If you do, at the later migrated_preferences phase you'll assume the preference exists, and do an ACTION_UPDATE, when it should be an ACTION_ADD. https://codereview.chromium.org/24930003/diff/111001/chrome/browser/prefs/pre... chrome/browser/prefs/pref_model_associator.cc:638: (synced_preferences_.count(name) == 0) ? this should be old_pref_name right? https://codereview.chromium.org/24930003/diff/111001/chrome/browser/sync/prof... File chrome/browser/sync/profile_sync_service_preference_unittest.cc (right): https://codereview.chromium.org/24930003/diff/111001/chrome/browser/sync/prof... chrome/browser/sync/profile_sync_service_preference_unittest.cc:413: ListPrefUpdate update(prefs_, prefs::kURLsToRestoreOnStartupOld); Is this used for anything? https://codereview.chromium.org/24930003/diff/111001/chrome/browser/sync/prof... chrome/browser/sync/profile_sync_service_preference_unittest.cc:424: cloud_data[prefs::kURLsToRestoreOnStartup] = urls_to_restore; If this test is for old migrated data in the cloud, shouldn't this be kURLsToRestoreOnStartupOld? https://codereview.chromium.org/24930003/diff/111001/chrome/browser/sync/prof... chrome/browser/sync/profile_sync_service_preference_unittest.cc:437: // Expect that finish comment?
Thanks Nicolas, PTAL https://codereview.chromium.org/24930003/diff/111001/chrome/browser/prefs/pre... File chrome/browser/prefs/pref_model_associator.cc (right): https://codereview.chromium.org/24930003/diff/111001/chrome/browser/prefs/pre... chrome/browser/prefs/pref_model_associator.cc:156: if (preference.name() == old_pref_name && migrated_preference_list) { On 2013/10/11 19:58:54, Nicolas Zea wrote: > I think it's invalid to call InitPrefAndAssociate with an old preference and not > have the migrated_preference_list. In that case, I'd leave the first condition, > and have DCHECK(migrated_preference_list) within the body. Afaict it should never happen and I'll add the DCHECK. I'm leery of leaving the pointer dereference there in release mode without checking it because I'm not extremely confident in my assessment that it will never happen. Let me know if you think I'm being overly paranoid and I'll drop the migrated_preference_list check too. https://codereview.chromium.org/24930003/diff/111001/chrome/browser/prefs/pre... chrome/browser/prefs/pref_model_associator.cc:181: synced_preferences_.insert(preference.name()); On 2013/10/11 19:58:54, Nicolas Zea wrote: > I think rather than having this here, you need to change the one at line 217 to > use preference.name(). > > For example, if the migrated synced pref doesn't exist, pref_name will be for > the migrated pref, and adding it into synced_preferences_ is invalid. If you do, > at the later migrated_preferences phase you'll assume the preference exists, and > do an ACTION_UPDATE, when it should be an ACTION_ADD. preference.name() is only defined if sync_pref.IsValid() is true. In cases where sync_pref is not valid (there are two such callsites) I think we should still add pref_name and rather just make sure to not add pref_name if preference.name() has already been added. Did that, let me know what you think. https://codereview.chromium.org/24930003/diff/111001/chrome/browser/prefs/pre... chrome/browser/prefs/pref_model_associator.cc:638: (synced_preferences_.count(name) == 0) ? On 2013/10/11 19:58:54, Nicolas Zea wrote: > this should be old_pref_name right? oops, yes. good catch. https://codereview.chromium.org/24930003/diff/111001/chrome/browser/sync/prof... File chrome/browser/sync/profile_sync_service_preference_unittest.cc (right): https://codereview.chromium.org/24930003/diff/111001/chrome/browser/sync/prof... chrome/browser/sync/profile_sync_service_preference_unittest.cc:413: ListPrefUpdate update(prefs_, prefs::kURLsToRestoreOnStartupOld); On 2013/10/11 19:58:54, Nicolas Zea wrote: > Is this used for anything? This should have been (and now is) prefs::kURLsToRestoreOnStartup. https://codereview.chromium.org/24930003/diff/111001/chrome/browser/sync/prof... chrome/browser/sync/profile_sync_service_preference_unittest.cc:424: cloud_data[prefs::kURLsToRestoreOnStartup] = urls_to_restore; On 2013/10/11 19:58:54, Nicolas Zea wrote: > If this test is for old migrated data in the cloud, shouldn't this be > kURLsToRestoreOnStartupOld? Yes, sorry, this test was borked. Revised. https://codereview.chromium.org/24930003/diff/111001/chrome/browser/sync/prof... chrome/browser/sync/profile_sync_service_preference_unittest.cc:437: // Expect that On 2013/10/11 19:58:54, Nicolas Zea wrote: > finish comment? Done.
LGTM!
OWNERS reviews please! battre@ -> chrome\browser\prefs, chrome\browser\profile_resetter jhawkins@ -> chrome\browser\resources\options asvitkine@ -> histograms Thanks! Robert
There are two changes that shouldn't be there. And sorry for not noticing that there were places where the pref names were used verbatim as opposed to using the goobal kVariable[]... :-( https://codereview.chromium.org/24930003/diff/127001/chrome/browser/resources... File chrome/browser/resources/options/browser_options.js (left): https://codereview.chromium.org/24930003/diff/127001/chrome/browser/resources... chrome/browser/resources/options/browser_options.js:106: Preferences.getInstance().addEventListener('session.restore_on_startup', This is not the same pref as urls_to_restore_on_startup, you should not change that! https://codereview.chromium.org/24930003/diff/127001/chrome/browser/resources... File chrome/browser/resources/options/startup_section.html (left): https://codereview.chromium.org/24930003/diff/127001/chrome/browser/resources... chrome/browser/resources/options/startup_section.html:7: pref="session.restore_on_startup" Again, this is not the same pref as urls_to_restore_on_startup, you should not change that!
https://codereview.chromium.org/24930003/diff/127001/chrome/browser/prefs/ses... File chrome/browser/prefs/session_startup_pref.cc (right): https://codereview.chromium.org/24930003/diff/127001/chrome/browser/prefs/ses... chrome/browser/prefs/session_startup_pref.cc:27: STARTUP_URLS_MIGRATION_METRICS_STARTED, I'm not sure _STARTED is accurate here. Perhaps _PERFORMED would be a better choice? https://codereview.chromium.org/24930003/diff/127001/chrome/browser/prefs/ses... chrome/browser/prefs/session_startup_pref.cc:161: if (old_startup_urls && old_startup_urls->GetSize() > 0) { Nit: use !old_startup_urls->empty() https://codereview.chromium.org/24930003/diff/127001/chrome/browser/prefs/ses... chrome/browser/prefs/session_startup_pref.cc:167: STARTUP_URLS_MIGRATION_METRICS_STARTED, Nit: Can you move the UMA macro below outside the if and make a local var that's the enum value that you'll log. This way, you won't need to repeat the macro two times. Alternatively, make a small function in the anon namespace that takes the enum value as a param and logs the histogram. Then, you can also call it for the STARTUP_URLS_MIGRATION_METRICS_RESET case below. Context: Each histogram macro expands to a large-ish block of code, so it's better to avoid repeating them (i.e. better for code size, etc). https://codereview.chromium.org/24930003/diff/127001/chrome/browser/prefs/ses... chrome/browser/prefs/session_startup_pref.cc:170: // We migrated but the migration data got deleted somehow. Wouldn't this case be quite normal if there were simply no startup urls in prefs but the migration code hadn't run yet? If so, perhaps update the comment / enum name (e.g. to _NO_URLS_TO_MIGRATE) for example. https://codereview.chromium.org/24930003/diff/127001/chrome/browser/prefs/ses... chrome/browser/prefs/session_startup_pref.cc:178: } else if (old_startup_urls && old_startup_urls->GetSize() > 0) { Nit: old_startup_urls->empty() https://codereview.chromium.org/24930003/diff/127001/chrome/browser/prefs/ses... chrome/browser/prefs/session_startup_pref.cc:185: DCHECK(now > last_migration_time); Can DCHECK_GT() work here? https://codereview.chromium.org/24930003/diff/127001/chrome/common/pref_names.cc File chrome/common/pref_names.cc (right): https://codereview.chromium.org/24930003/diff/127001/chrome/common/pref_names... chrome/common/pref_names.cc:88: // base::Time::ToInternalValue for details). Nit: I'd have one comment above each constant below (so it's clear which is which), rather than commenting them together. https://codereview.chromium.org/24930003/diff/127001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/24930003/diff/127001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:15407: + The time elapsed in miliseconds in between startup URLs pref migration. Nit: milliseconds (two l's) Mention that a value of 0 indicates that the last migration time was in the future (e.g. due to wrong system time). https://codereview.chromium.org/24930003/diff/127001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:27043: + <int value="0" label="Started"/> Nit: "Started migration" - same for the others below.
Thanks! PTAL https://codereview.chromium.org/24930003/diff/127001/chrome/browser/prefs/ses... File chrome/browser/prefs/session_startup_pref.cc (right): https://codereview.chromium.org/24930003/diff/127001/chrome/browser/prefs/ses... chrome/browser/prefs/session_startup_pref.cc:27: STARTUP_URLS_MIGRATION_METRICS_STARTED, On 2013/10/15 14:40:55, Alexei Svitkine wrote: > I'm not sure _STARTED is accurate here. Perhaps _PERFORMED would be a better > choice? Done. https://codereview.chromium.org/24930003/diff/127001/chrome/browser/prefs/ses... chrome/browser/prefs/session_startup_pref.cc:161: if (old_startup_urls && old_startup_urls->GetSize() > 0) { On 2013/10/15 14:40:55, Alexei Svitkine wrote: > Nit: use !old_startup_urls->empty() Done. https://codereview.chromium.org/24930003/diff/127001/chrome/browser/prefs/ses... chrome/browser/prefs/session_startup_pref.cc:167: STARTUP_URLS_MIGRATION_METRICS_STARTED, On 2013/10/15 14:40:55, Alexei Svitkine wrote: > Nit: Can you move the UMA macro below outside the if and make a local var that's > the enum value that you'll log. This way, you won't need to repeat the macro two > times. > > Alternatively, make a small function in the anon namespace that takes the enum > value as a param and logs the histogram. Then, you can also call it for the > STARTUP_URLS_MIGRATION_METRICS_RESET case below. > > Context: Each histogram macro expands to a large-ish block of code, so it's > better to avoid repeating them (i.e. better for code size, etc). Done. https://codereview.chromium.org/24930003/diff/127001/chrome/browser/prefs/ses... chrome/browser/prefs/session_startup_pref.cc:170: // We migrated but the migration data got deleted somehow. On 2013/10/15 14:40:55, Alexei Svitkine wrote: > Wouldn't this case be quite normal if there were simply no startup urls in prefs > but the migration code hadn't run yet? If so, perhaps update the comment / enum > name (e.g. to _NO_URLS_TO_MIGRATE) for example. I believe you are correct. Updated the name to STARTUP_URLS_MIGRATION_METRICS_NOT_PRESENT and updated the value in histograms.xml as well. https://codereview.chromium.org/24930003/diff/127001/chrome/browser/prefs/ses... chrome/browser/prefs/session_startup_pref.cc:178: } else if (old_startup_urls && old_startup_urls->GetSize() > 0) { On 2013/10/15 14:40:55, Alexei Svitkine wrote: > Nit: old_startup_urls->empty() Done. https://codereview.chromium.org/24930003/diff/127001/chrome/browser/prefs/ses... chrome/browser/prefs/session_startup_pref.cc:185: DCHECK(now > last_migration_time); On 2013/10/15 14:40:55, Alexei Svitkine wrote: > Can DCHECK_GT() work here? Sadly, no. I tried and it doesn't work with base::Time due to a lack of an operator<< implementation. https://codereview.chromium.org/24930003/diff/127001/chrome/browser/resources... File chrome/browser/resources/options/browser_options.js (left): https://codereview.chromium.org/24930003/diff/127001/chrome/browser/resources... chrome/browser/resources/options/browser_options.js:106: Preferences.getInstance().addEventListener('session.restore_on_startup', On 2013/10/15 14:31:16, MAD wrote: > This is not the same pref as urls_to_restore_on_startup, you should not change > that! Thanks, good catch. https://codereview.chromium.org/24930003/diff/127001/chrome/browser/resources... File chrome/browser/resources/options/startup_section.html (left): https://codereview.chromium.org/24930003/diff/127001/chrome/browser/resources... chrome/browser/resources/options/startup_section.html:7: pref="session.restore_on_startup" On 2013/10/15 14:31:16, MAD wrote: > Again, this is not the same pref as urls_to_restore_on_startup, you should not > change that! Done. https://codereview.chromium.org/24930003/diff/127001/chrome/common/pref_names.cc File chrome/common/pref_names.cc (right): https://codereview.chromium.org/24930003/diff/127001/chrome/common/pref_names... chrome/common/pref_names.cc:88: // base::Time::ToInternalValue for details). On 2013/10/15 14:40:55, Alexei Svitkine wrote: > Nit: I'd have one comment above each constant below (so it's clear which is > which), rather than commenting them together. Done. https://codereview.chromium.org/24930003/diff/127001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/24930003/diff/127001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:15407: + The time elapsed in miliseconds in between startup URLs pref migration. On 2013/10/15 14:40:55, Alexei Svitkine wrote: > Nit: milliseconds (two l's) > > Mention that a value of 0 indicates that the last migration time was in the > future (e.g. due to wrong system time). Done. https://codereview.chromium.org/24930003/diff/127001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:27043: + <int value="0" label="Started"/> On 2013/10/15 14:40:55, Alexei Svitkine wrote: > Nit: "Started migration" - same for the others below. Done.
LGTM with a comment https://codereview.chromium.org/24930003/diff/148001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/24930003/diff/148001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:27046: + <int value="1" label="Deleted migration"/> Nit: Update these labels per the new enum names.
Thanks Alexei. Moar OWNERS reviews please! bauerb@ -> chrome\browser\prefs vabr@-> chrome\browser\profile_resetter jhawkins@ -> chrome\browser\resources\options https://codereview.chromium.org/24930003/diff/148001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/24930003/diff/148001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:27046: + <int value="1" label="Deleted migration"/> On 2013/10/15 18:05:30, Alexei Svitkine wrote: > Nit: Update these labels per the new enum names. Sorry, missed this file last upload. Done.
https://codereview.chromium.org/24930003/diff/131001/chrome/browser/prefs/pre... File chrome/browser/prefs/pref_model_associator.cc (right): https://codereview.chromium.org/24930003/diff/131001/chrome/browser/prefs/pre... chrome/browser/prefs/pref_model_associator.cc:153: if (IsMigratedPreference(pref_name.c_str())) { This logic is fairly tricky. We should think about some way to make sure that other pref migration code added in the future will also be handled here. https://codereview.chromium.org/24930003/diff/131001/chrome/browser/prefs/ses... File chrome/browser/prefs/session_startup_pref.cc (right): https://codereview.chromium.org/24930003/diff/131001/chrome/browser/prefs/ses... chrome/browser/prefs/session_startup_pref.cc:166: if (old_startup_urls && !old_startup_urls->empty()) { |old_startup_urls| is guaranteed not to be NULL. https://codereview.chromium.org/24930003/diff/131001/chrome/browser/prefs/ses... chrome/browser/prefs/session_startup_pref.cc:168: prefs->Set(prefs::kURLsToRestoreOnStartupOld, base::ListValue()); You can use prefs->ClearPref(). https://codereview.chromium.org/24930003/diff/131001/chrome/browser/prefs/ses... chrome/browser/prefs/session_startup_pref.cc:182: prefs->Set(prefs::kURLsToRestoreOnStartupOld, base::ListValue()); It seems like you are doing a lot of similar things in the two branches here (clear the old value, set the new migration time). Would it simplify things if you merged the two branches? https://codereview.chromium.org/24930003/diff/131001/chrome/browser/prefs/ses... chrome/browser/prefs/session_startup_pref.cc:187: DCHECK(now > last_migration_time); Nit: DCHECK_GT(now, last_migration_time) gives nicer messages in case of a check failure. https://codereview.chromium.org/24930003/diff/131001/chrome/browser/prefs/ses... chrome/browser/prefs/session_startup_pref.cc:188: if (now < last_migration_time) This looks like you're handling a DCHECK failure? You're not supposed to do that, see http://dev.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREACHED-. https://codereview.chromium.org/24930003/diff/131001/chrome/common/pref_names.cc File chrome/common/pref_names.cc (right): https://codereview.chromium.org/24930003/diff/131001/chrome/common/pref_names... chrome/common/pref_names.cc:92: const char kRestoreStartupURLsMigrated[] = "session.startup_urls_migrated"; You could rename this to make it clearer that it's a migration timestamp rather than the migrated value of |kRestoreStartupURLs|.
Thanks Bernhard, PTAL https://codereview.chromium.org/24930003/diff/131001/chrome/browser/prefs/pre... File chrome/browser/prefs/pref_model_associator.cc (right): https://codereview.chromium.org/24930003/diff/131001/chrome/browser/prefs/pre... chrome/browser/prefs/pref_model_associator.cc:153: if (IsMigratedPreference(pref_name.c_str())) { On 2013/10/15 22:03:52, Bernhard Bauer wrote: > This logic is fairly tricky. We should think about some way to make sure that > other pref migration code added in the future will also be handled here. It is more complex than I had hoped, but it is as future proof as I think it is worth making it. Two types of future migration come to mind: 1) Migrating additional prefs using the same sync and merge strategy. As per the comment on line 56, migrating new prefs using the same strategy will be quite straightforward. 2) Migrating prefs using different sync and merging rules. We don't currently have plans to do this and given the complexity of this relatively simple operation, it's unlikely that we'll pursue this further. I'd prefer not to over-generalise this code until and unless such plans come about. https://codereview.chromium.org/24930003/diff/131001/chrome/browser/prefs/ses... File chrome/browser/prefs/session_startup_pref.cc (right): https://codereview.chromium.org/24930003/diff/131001/chrome/browser/prefs/ses... chrome/browser/prefs/session_startup_pref.cc:166: if (old_startup_urls && !old_startup_urls->empty()) { On 2013/10/15 22:03:52, Bernhard Bauer wrote: > |old_startup_urls| is guaranteed not to be NULL. That does not line up with my reading of PrefService::GetList in pref_service.cc. Am I misreading? https://codereview.chromium.org/24930003/diff/131001/chrome/browser/prefs/ses... chrome/browser/prefs/session_startup_pref.cc:168: prefs->Set(prefs::kURLsToRestoreOnStartupOld, base::ListValue()); On 2013/10/15 22:03:52, Bernhard Bauer wrote: > You can use prefs->ClearPref(). Done. https://codereview.chromium.org/24930003/diff/131001/chrome/browser/prefs/ses... chrome/browser/prefs/session_startup_pref.cc:182: prefs->Set(prefs::kURLsToRestoreOnStartupOld, base::ListValue()); On 2013/10/15 22:03:52, Bernhard Bauer wrote: > It seems like you are doing a lot of similar things in the two branches here > (clear the old value, set the new migration time). Would it simplify things if > you merged the two branches? Played around with the logic a bit. I extracted the histogram logic (which helps with code size, as Alexei pointed out UMA macros expand to mucho code) but wasn't able to make this any shorter without making it more convoluted. Let me know if you have a better factorization in mind. https://codereview.chromium.org/24930003/diff/131001/chrome/browser/prefs/ses... chrome/browser/prefs/session_startup_pref.cc:187: DCHECK(now > last_migration_time); On 2013/10/15 22:03:52, Bernhard Bauer wrote: > Nit: DCHECK_GT(now, last_migration_time) gives nicer messages in case of a check > failure. Sadly, DCHECK_GT doesn't work with base::Time instances. Seems there's a missing operator<< or some such. This said, I'll remove the DCHECK based on your next comment. https://codereview.chromium.org/24930003/diff/131001/chrome/browser/prefs/ses... chrome/browser/prefs/session_startup_pref.cc:188: if (now < last_migration_time) On 2013/10/15 22:03:52, Bernhard Bauer wrote: > This looks like you're handling a DCHECK failure? You're not supposed to do > that, see > http://dev.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREACHED-. Removing the DCHECK, there's no reason for it to be there. https://codereview.chromium.org/24930003/diff/131001/chrome/common/pref_names.cc File chrome/common/pref_names.cc (right): https://codereview.chromium.org/24930003/diff/131001/chrome/common/pref_names... chrome/common/pref_names.cc:92: const char kRestoreStartupURLsMigrated[] = "session.startup_urls_migrated"; On 2013/10/15 22:03:52, Bernhard Bauer wrote: > You could rename this to make it clearer that it's a migration timestamp rather > than the migrated value of |kRestoreStartupURLs|. Done.
https://codereview.chromium.org/24930003/diff/131001/chrome/browser/prefs/pre... File chrome/browser/prefs/pref_model_associator.cc (right): https://codereview.chromium.org/24930003/diff/131001/chrome/browser/prefs/pre... chrome/browser/prefs/pref_model_associator.cc:153: if (IsMigratedPreference(pref_name.c_str())) { On 2013/10/16 01:23:27, robertshield wrote: > On 2013/10/15 22:03:52, Bernhard Bauer wrote: > > This logic is fairly tricky. We should think about some way to make sure that > > other pref migration code added in the future will also be handled here. > > It is more complex than I had hoped, but it is as future proof as I think it is > worth making it. Two types of future migration come to mind: > > 1) Migrating additional prefs using the same sync and merge strategy. As per the > comment on line 56, migrating new prefs using the same strategy will be quite > straightforward. > > 2) Migrating prefs using different sync and merging rules. We don't currently > have plans to do this and given the complexity of this relatively simple > operation, it's unlikely that we'll pursue this further. I'd prefer not to > over-generalise this code until and unless such plans come about. Sorry, I wasn't suggesting to change this code, but I just wanted to make sure we also use this for future migrations. More of a note-to-self :) https://codereview.chromium.org/24930003/diff/131001/chrome/browser/prefs/ses... File chrome/browser/prefs/session_startup_pref.cc (right): https://codereview.chromium.org/24930003/diff/131001/chrome/browser/prefs/ses... chrome/browser/prefs/session_startup_pref.cc:166: if (old_startup_urls && !old_startup_urls->empty()) { On 2013/10/16 01:23:27, robertshield wrote: > On 2013/10/15 22:03:52, Bernhard Bauer wrote: > > |old_startup_urls| is guaranteed not to be NULL. > > That does not line up with my reading of PrefService::GetList in > pref_service.cc. Am I misreading? Well, it's guaranteed not to be NULL in the absence of bugs (as in, running into a NOTREACHED() means a bug). Admittedly, the code in pref_service.cc that does `NOTREACHED(); return NULL;` should not do that. If we would change it to a pure DCHECK, it would make the contract clearer.
https://codereview.chromium.org/24930003/diff/131001/chrome/browser/prefs/pre... File chrome/browser/prefs/pref_model_associator.cc (right): https://codereview.chromium.org/24930003/diff/131001/chrome/browser/prefs/pre... chrome/browser/prefs/pref_model_associator.cc:153: if (IsMigratedPreference(pref_name.c_str())) { On 2013/10/16 15:13:33, Bernhard Bauer wrote: > On 2013/10/16 01:23:27, robertshield wrote: > > On 2013/10/15 22:03:52, Bernhard Bauer wrote: > > > This logic is fairly tricky. We should think about some way to make sure > that > > > other pref migration code added in the future will also be handled here. > > > > It is more complex than I had hoped, but it is as future proof as I think it > is > > worth making it. Two types of future migration come to mind: > > > > 1) Migrating additional prefs using the same sync and merge strategy. As per > the > > comment on line 56, migrating new prefs using the same strategy will be quite > > straightforward. > > > > 2) Migrating prefs using different sync and merging rules. We don't currently > > have plans to do this and given the complexity of this relatively simple > > operation, it's unlikely that we'll pursue this further. I'd prefer not to > > over-generalise this code until and unless such plans come about. > > Sorry, I wasn't suggesting to change this code, but I just wanted to make sure > we also use this for future migrations. More of a note-to-self :) No worries, I got one less UMA macro expansion out of it, so it's all good :) https://codereview.chromium.org/24930003/diff/131001/chrome/browser/prefs/ses... File chrome/browser/prefs/session_startup_pref.cc (right): https://codereview.chromium.org/24930003/diff/131001/chrome/browser/prefs/ses... chrome/browser/prefs/session_startup_pref.cc:166: if (old_startup_urls && !old_startup_urls->empty()) { On 2013/10/16 15:13:33, Bernhard Bauer wrote: > On 2013/10/16 01:23:27, robertshield wrote: > > On 2013/10/15 22:03:52, Bernhard Bauer wrote: > > > |old_startup_urls| is guaranteed not to be NULL. > > > > That does not line up with my reading of PrefService::GetList in > > pref_service.cc. Am I misreading? > > Well, it's guaranteed not to be NULL in the absence of bugs (as in, running into > a NOTREACHED() means a bug). On line 308 (https://code.google.com/p/chromium/codesearch#chromium/src/base/prefs/pref_se...), GetList returns NULL if the value type is not of type list. The value type is (afaict) ultimately derived from what is in the preferences file, which is user-controlled data and should not be trusted to be intact. Having a corrupted preferences file looks like it would cause NULL to be returned and we should not crash in release builds if this happens. > Admittedly, the code in pref_service.cc that does > `NOTREACHED(); return NULL;` should not do that. If we would change it to a pure > DCHECK, it would make the contract clearer.
LGTM as discussed off-review.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
Retried try job too often on linux_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
Retried try job too often on linux_aura for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/168001
Step "update" is always a major failure. Look at the try server FAQ for more details. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/24930003/1075001
Message was sent while issue was closed.
Change committed as 229536
Message was sent while issue was closed.
chrome\browser\profile_resetter LGTM. (Sorry for the delay, I was OOO.) Cheers, Vaclav |
