Move multiprofile pref migration preference.

Move multiprofile pref migration preference.
    
Move the registration of kMultipleProfilePrefMigration to be in the same
file where it is used. Also, remove redundant DCHECKs (the preference
service will already hit NOTREACHED in this code if the prefs in
question do not exist).


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

[Torne, 2011-08-18 10:50:07]

Hi Miranda, would you mind taking a quick look at this? I'm trying to move
various non-UI-related prefs out of browser/ui.

[Miranda Callahan, 2011-08-18 15:14:26]

On 2011/08/18 10:50:07, Torne wrote:
> Hi Miranda, would you mind taking a quick look at this? I'm trying to move
> various non-UI-related prefs out of browser/ui.

Hi, Torne -- thanks for this.  I think this pref is so old that it can actually
be deleted.  LGTM for your change, and I'll file a bug against myself to get rid
of this.

[torne_google.com, 2011-08-18 15:18:22]

There's already http://crbug.com/69995 and we are past six months from
that.. shall I just remove this code instead? I don't mind :)

On 18 August 2011 16:14,  <mirandac@chromium.org> wrote:
> On 2011/08/18 10:50:07, Torne wrote:
>>
>> Hi Miranda, would you mind taking a quick look at this? I'm trying to move
>> various non-UI-related prefs out of browser/ui.
>
> Hi, Torne -- thanks for this.  I think this pref is so old that it can
> actually
> be deleted.  LGTM for your change, and I'll file a bug against myself to get
> rid
> of this.
>
> http://codereview.chromium.org/7670063/
>



-- 
Torne (Richard Coles)
torne@google.com

[Miranda Callahan, 2011-08-18 15:21:52]

Ha -- yes, that would be brilliant.  Thanks! :-D

On 2011/08/18 15:18:22, Torne (Richard Coles) wrote:
> There's already http://crbug.com/69995 and we are past six months from
> that.. shall I just remove this code instead? I don't mind :)
> 
> On 18 August 2011 16:14,  <mailto:mirandac@chromium.org> wrote:
> > On 2011/08/18 10:50:07, Torne wrote:
> >>
> >> Hi Miranda, would you mind taking a quick look at this? I'm trying to move
> >> various non-UI-related prefs out of browser/ui.
> >
> > Hi, Torne -- thanks for this. &nbsp;I think this pref is so old that it can
> > actually
> > be deleted. &nbsp;LGTM for your change, and I'll file a bug against myself
to get
> > rid
> > of this.
> >
> > http://codereview.chromium.org/7670063/
> >
> 
> 
> 
> -- 
> Torne (Richard Coles)
> mailto:torne@google.com

[Torne, 2011-08-18 16:11:42]

OK, uploaded a new patch that just removes this pref and the migration code.

On 2011/08/18 15:21:52, Miranda Callahan wrote:
> Ha -- yes, that would be brilliant.  Thanks! :-D
> 
> On 2011/08/18 15:18:22, Torne (Richard Coles) wrote:
> > There's already http://crbug.com/69995 and we are past six months from
> > that.. shall I just remove this code instead? I don't mind :)
> > 
> > On 18 August 2011 16:14,  <mailto:mirandac@chromium.org> wrote:
> > > On 2011/08/18 10:50:07, Torne wrote:
> > >>
> > >> Hi Miranda, would you mind taking a quick look at this? I'm trying to
move
> > >> various non-UI-related prefs out of browser/ui.
> > >
> > > Hi, Torne -- thanks for this. &nbsp;I think this pref is so old that it
can
> > > actually
> > > be deleted. &nbsp;LGTM for your change, and I'll file a bug against myself
> to get
> > > rid
> > > of this.
> > >
> > > http://codereview.chromium.org/7670063/
> > >
> > 
> > 
> > 
> > -- 
> > Torne (Richard Coles)
> > mailto:torne@google.com

[Torne, 2011-08-19 12:41:04]

Aand, actually made it pass trybots (modulo some flakes) by fixing the test
also.

Miranda, can you just check this looks sane again, since it's a completely
different patch now really :)

On 2011/08/18 16:11:42, Torne wrote:
> OK, uploaded a new patch that just removes this pref and the migration code.
> 
> On 2011/08/18 15:21:52, Miranda Callahan wrote:
> > Ha -- yes, that would be brilliant.  Thanks! :-D
> > 
> > On 2011/08/18 15:18:22, Torne (Richard Coles) wrote:
> > > There's already http://crbug.com/69995 and we are past six months from
> > > that.. shall I just remove this code instead? I don't mind :)
> > > 
> > > On 18 August 2011 16:14,  <mailto:mirandac@chromium.org> wrote:
> > > > On 2011/08/18 10:50:07, Torne wrote:
> > > >>
> > > >> Hi Miranda, would you mind taking a quick look at this? I'm trying to
> move
> > > >> various non-UI-related prefs out of browser/ui.
> > > >
> > > > Hi, Torne -- thanks for this. &nbsp;I think this pref is so old that it
> can
> > > > actually
> > > > be deleted. &nbsp;LGTM for your change, and I'll file a bug against
myself
> > to get
> > > > rid
> > > > of this.
> > > >
> > > > http://codereview.chromium.org/7670063/
> > > >
> > > 
> > > 
> > > 
> > > -- 
> > > Torne (Richard Coles)
> > > mailto:torne@google.com

[Miranda Callahan, 2011-08-19 12:43:35]

LGTM -- very much appreciated. :-)

On 2011/08/19 12:41:04, Torne wrote:
> Aand, actually made it pass trybots (modulo some flakes) by fixing the test
> also.
> 
> Miranda, can you just check this looks sane again, since it's a completely
> different patch now really :)
> 
> On 2011/08/18 16:11:42, Torne wrote:
> > OK, uploaded a new patch that just removes this pref and the migration code.
> > 
> > On 2011/08/18 15:21:52, Miranda Callahan wrote:
> > > Ha -- yes, that would be brilliant.  Thanks! :-D
> > > 
> > > On 2011/08/18 15:18:22, Torne (Richard Coles) wrote:
> > > > There's already http://crbug.com/69995 and we are past six months from
> > > > that.. shall I just remove this code instead? I don't mind :)
> > > > 
> > > > On 18 August 2011 16:14,  <mailto:mirandac@chromium.org> wrote:
> > > > > On 2011/08/18 10:50:07, Torne wrote:
> > > > >>
> > > > >> Hi Miranda, would you mind taking a quick look at this? I'm trying to
> > move
> > > > >> various non-UI-related prefs out of browser/ui.
> > > > >
> > > > > Hi, Torne -- thanks for this. &nbsp;I think this pref is so old that
it
> > can
> > > > > actually
> > > > > be deleted. &nbsp;LGTM for your change, and I'll file a bug against
> myself
> > > to get
> > > > > rid
> > > > > of this.
> > > > >
> > > > > http://codereview.chromium.org/7670063/
> > > > >
> > > > 
> > > > 
> > > > 
> > > > -- 
> > > > Torne (Richard Coles)
> > > > mailto:torne@google.com

[commit-bot: I haz the power, 2011-08-19 12:59:34]

Presubmit check for 7670063-7001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
If this change requires manual test instructions to QA team, add
TEST=[instructions].

** Presubmit ERRORS **
Missing LGTM from an OWNER for: chrome/browser/net/predictor_api.cc

Presubmit checks took 1.8s to calculate.

[Torne, 2011-08-19 13:26:18]

Hi Jim,

Would you be able to take a quick look at this, as an OWNER for
chrome/browser/net ?

Thanks!

[jar (doing other things), 2011-08-19 16:55:38]

What is the consequence for users that have not migrated yet?  

What happens to them when they upgrade?

[Miranda Callahan, 2011-08-19 17:08:45]

On 2011/08/19 16:55:38, jar wrote:
> What is the consequence for users that have not migrated yet?  
> 
> What happens to them when they upgrade?

These users will lose their settings and have them reset to the defaults.
However, this migration code has been in the build since January, so this should
only happen to someone who hasn't used Chrome since last year -- hopefully long
enough to have forgotten about (or not care about) any of these settings.

[Torne, 2011-08-23 14:13:35]

Okay, I've thought about this more and it seems like a better way to clean this
code up is to keep the migration code (at least for now, maybe indefinitely?)
but remove the *preference* recording whether the migration has happened. It
just asks the pref service if the old prefs to be migrated/removed actually
exist (with HasPrefPath, which works even if the pref is not registered) first;
as long as the migration ultimately removes the old pref this is idempotent and
should be safe to leave there. This seems nicer than keeping a bitmask of which
migrations have been run.

Is this okay with you now, Jim? It keeps the desired behaviour for upgrading but
is less complicated.

[jar (doing other things), 2011-08-23 16:28:11]

SGTM!

Thanks!

On Tue, Aug 23, 2011 at 7:13 AM, <torne@chromium.org> wrote:

> Okay, I've thought about this more and it seems like a better way to clean
> this
> code up is to keep the migration code (at least for now, maybe
> indefinitely?)
> but remove the *preference* recording whether the migration has happened.
> It
> just asks the pref service if the old prefs to be migrated/removed actually
> exist (with HasPrefPath, which works even if the pref is not registered)
> first;
> as long as the migration ultimately removes the old pref this is idempotent
> and
> should be safe to leave there. This seems nicer than keeping a bitmask of
> which
> migrations have been run.
>
> Is this okay with you now, Jim? It keeps the desired behaviour for
> upgrading but
> is less complicated.
>
>
>
http://codereview.chromium.**org/7670063/<http://codereview.chromium.org/7670...
>

[torne_google.com, 2011-08-23 16:31:01]

The minor complication being that this patch doesn't actually seem to
work, but I shall poke it until it does :p

On 23 August 2011 17:28, Jim Roskind <jar@chromium.org> wrote:
> SGTM!
> Thanks!
>
> On Tue, Aug 23, 2011 at 7:13 AM, <torne@chromium.org> wrote:
>>
>> Okay, I've thought about this more and it seems like a better way to clean
>> this
>> code up is to keep the migration code (at least for now, maybe
>> indefinitely?)
>> but remove the *preference* recording whether the migration has happened.
>> It
>> just asks the pref service if the old prefs to be migrated/removed
>> actually
>> exist (with HasPrefPath, which works even if the pref is not registered)
>> first;
>> as long as the migration ultimately removes the old pref this is
>> idempotent and
>> should be safe to leave there. This seems nicer than keeping a bitmask of
>> which
>> migrations have been run.
>>
>> Is this okay with you now, Jim? It keeps the desired behaviour for
>> upgrading but
>> is less complicated.
>>
>> http://codereview.chromium.org/7670063/
>
>



-- 
Torne (Richard Coles)
torne@google.com

[Torne, 2011-08-26 13:43:47]

Okay; sorry Miranda and Jim, it's turned out to be kinda unreasonable to
actually remove this preference; it's the simplest way to ensure the migration
happens when it should.

I've reverted back to the original patch which just moves the preference to the
same file where it's used and removes the redundant DCHECKs, which is sufficient
to fix the original issues I was having. It's up to you what you want to about
potentially removing this migration code in future (and whether we keep that
issue open).

Once this has passed a try job I'll commit it since Miranda already approved the
first time around :)

On 2011/08/23 16:31:01, Torne (Richard Coles) wrote:
> The minor complication being that this patch doesn't actually seem to
> work, but I shall poke it until it does :p
> 
> On 23 August 2011 17:28, Jim Roskind <mailto:jar@chromium.org> wrote:
> > SGTM!
> > Thanks!
> >
> > On Tue, Aug 23, 2011 at 7:13 AM, <mailto:torne@chromium.org> wrote:
> >>
> >> Okay, I've thought about this more and it seems like a better way to clean
> >> this
> >> code up is to keep the migration code (at least for now, maybe
> >> indefinitely?)
> >> but remove the *preference* recording whether the migration has happened.
> >> It
> >> just asks the pref service if the old prefs to be migrated/removed
> >> actually
> >> exist (with HasPrefPath, which works even if the pref is not registered)
> >> first;
> >> as long as the migration ultimately removes the old pref this is
> >> idempotent and
> >> should be safe to leave there. This seems nicer than keeping a bitmask of
> >> which
> >> migrations have been run.
> >>
> >> Is this okay with you now, Jim? It keeps the desired behaviour for
> >> upgrading but
> >> is less complicated.
> >>
> >> http://codereview.chromium.org/7670063/
> >
> >
> 
> 
> 
> -- 
> Torne (Richard Coles)
> mailto:torne@google.com

[commit-bot: I haz the power, 2011-08-30 13:54:13]

Change committed as 98809