Cleanup & tests for the mac startup pref migration.

Cleanup & tests for the mac startup pref migration.


BUG=142958
TEST=SessionStartupPref.MacDefaultStartupPrefMigration


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=145325

[marja, 2012-06-29 15:05:22]

Hi battre & ivankr,

This adds a test for the Mac default startup pref migration, and I also noticed
that ProtectedPrefsWatcher::EnsurePrefsMigration calls
SessionStartupPref::MigrateIfNecessary, so it should probably call
MigrateMacDefaultPrefIfNecessary, too.

Ptal.

[battre, 2012-06-29 15:32:32]

lgtm

[Ivan Korotkov, 2012-06-29 16:33:56]

This one's actually not so simple.

You'd have to increment the current version in ProtectedPrefsWatcher to 5 and
add a migration step that looks same as 2->3 migration (i.e., set
kBackupRestoreOnStartup from kRestoreOnStartup). Also, please, add a pyauto test
similar to the existing startup pref migration tests in protector.py.

http://codereview.chromium.org/10702040/diff/8001/chrome/browser/prefs/sessio...
File chrome/browser/prefs/session_startup_pref.cc (right):

http://codereview.chromium.org/10702040/diff/8001/chrome/browser/prefs/sessio...
chrome/browser/prefs/session_startup_pref.cc:124:
MigrateMacDefaultPrefIfNecessary(prefs);
Why not simply move this call to MigrateIfNecessary?

[marja, 2012-07-03 13:36:49]

I added protector.SessionStartupPref.testMacSessionStartupDefaultPrefMigration.
However, it will only pass on mac lion which has the "restore windows" system
preference set, and I don't know how to configure the test so that it runs only
in that case. I could exclude non-mac platforms, but how would I make it run
only when the system preference is set? Afaics there is no way for pyauto to
access this restore_utils::IsWindowRestoreEnabled.

Also, after doing the protector changes,
protector.ProtectorSessionStartupTest.testSessionStartupPrefNoMigrationOnHomepageChange
fails. It seems that before the control goes to the test, a Profile is created,
its version is 1.0.0.0, and the migration code path is hit. I'd need to prevent
this somehow, but I'm not sure how. Or I could just make the test not run on mac
at all.

ilyakr, do you have more advice on how to make this work? Or is it worth it
doing the protector migration at all?

http://codereview.chromium.org/10702040/diff/8001/chrome/browser/prefs/sessio...
File chrome/browser/prefs/session_startup_pref.cc (right):

http://codereview.chromium.org/10702040/diff/8001/chrome/browser/prefs/sessio...
chrome/browser/prefs/session_startup_pref.cc:124:
MigrateMacDefaultPrefIfNecessary(prefs);
On 2012/06/29 16:33:56, Ivan Korotkov wrote:
> Why not simply move this call to MigrateIfNecessary?

I'd like to keep the 2 migrations separate, e.g., they are not necessarily
removed at the same time.

[Ivan Korotkov, 2012-07-03 15:02:11]

It probably makes sense not to do this migration at all:
(1) if kRestoreOnStartup is later cleared, it's migration would happen again,
triggering the protector
(2) this adds even more complexity to the protector code.

[marja, 2012-07-03 16:17:25]

Ok, the current version doesn't do the migration in the protector, and doesn't
add protector tests, as discussed.

Adding the SessionStartupPref test still makes sense.

As batter LGTMd that part, I'll commit this...

[commit-bot: I haz the power, 2012-07-03 16:17:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/marja@chromium.org/10702040/3005

[commit-bot: I haz the power, 2012-07-03 17:26:12]

Change committed as 145325