Remove year+ old migration support of "session.urls_to_restore_on_startup".

Remove year+ old migration support of "session.urls_to_restore_on_startup".

Remove code supporting the migration of "session.urls_to_restore_on_startup"
to "session.startup_urls", the corresponding constants, histograms and unit
tests. This migration was there to support pre M-32 profiles.

This is a preparatory step before componentization of //chrome/browser/prefs
as keeping the migration code in the component is not worth the complexity.

Mark histograms as obsolete as of 2015/12 due to the removal of the migration
code they were mesuring.

BUG=525079
Committed: https://crrev.com/39dc3a1ffc119ce551abb056057c7076b8017401
Cr-Commit-Position: refs/heads/master@{#362981}

[sdefresne, 2015-08-21 12:58:23]

sdefresne@chromium.org changed reviewers:
+ gab@chromium.org

[sdefresne, 2015-08-21 12:58:23]

Please take a look.

[sdefresne, 2015-08-24 13:49:22]

ping?

[gab, 2015-08-24 21:26:40]

On 2015/08/24 13:49:22, sdefresne wrote:
> ping?

Looking but first, curious why you say M19? From my research this is more M32.

i.e., migration was introduced in 990ebf179de71b951e063042797218aeb243c667 which
from "git log 990ebf179de71b951e063042797218aeb243c667 -- chrome/VERSION" is
branch 1676 which from
https://en.wikipedia.org/wiki/Google_Chrome_release_history is M32 (branched off
of trunk at 1700).

Not saying we shouldn't remove it, but we should document the removal with the
right info.

[gab, 2015-08-24 21:29:51]

+robert/mad, FYI, some old migration code of yours being cleaned up :-)

[MAD, 2015-08-24 21:33:48]

Cool! thanks... :-)

Le lun. 24 août 2015 à 17:29, <gab@chromium.org> a écrit :

> +robert/mad, FYI, some old migration code of yours being cleaned up :-)
>
> https://codereview.chromium.org/1302303002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[gab, 2015-08-24 21:48:45]

gab@chromium.org changed reviewers:
+ zea@chromium.org

[gab, 2015-08-24 21:48:46]

+zea, see question below

Please cross-reference with https://codereview.chromium.org/24930003 which
introduced this code to see if there isn't more dead code to remove.

Thanks,
Gab

https://codereview.chromium.org/1302303002/diff/1/chrome/browser/prefs/pref_m...
File chrome/browser/prefs/pref_model_associator.cc (right):

https://codereview.chromium.org/1302303002/diff/1/chrome/browser/prefs/pref_m...
chrome/browser/prefs/pref_model_associator.cc:63: { NULL, NULL },
+zea: Think we should keep this around or simply get rid of it? I know it was
tricky to add so perhaps it is indeed worth keeping around an an empty list for
now?

https://codereview.chromium.org/1302303002/diff/1/chrome/browser/prefs/pref_s...
File chrome/browser/prefs/pref_service_syncable_unittest.cc (left):

https://codereview.chromium.org/1302303002/diff/1/chrome/browser/prefs/pref_s...
chrome/browser/prefs/pref_service_syncable_unittest.cc:297:
TEST_F(PrefServiceSyncableTest, ModelAssociationMigrateOldData) {
If we do keep the code around (see comment in .cc) though, we should keep the
tests around -- which I guess means dummy pref names in impl :-( or better mocks
for tests?

https://codereview.chromium.org/1302303002/diff/1/chrome/browser/prefs/sessio...
File chrome/browser/prefs/session_startup_pref.cc (left):

https://codereview.chromium.org/1302303002/diff/1/chrome/browser/prefs/sessio...
chrome/browser/prefs/session_startup_pref.cc:21: enum
StartupURLsMigrationMetrics {
rm this enum

https://codereview.chromium.org/1302303002/diff/1/chrome/browser/prefs/sessio...
chrome/browser/prefs/session_startup_pref.cc:169: base::Time now =
base::Time::Now();
Cross-referencing with https://codereview.chromium.org/24930003 which introduced
this code, looks like time.h and histograms.h were added then and that this was
still there only use in this file to this day so they can be removed.

https://codereview.chromium.org/1302303002/diff/1/chrome/installer/util/maste...
File chrome/installer/util/master_preferences.cc (left):

https://codereview.chromium.org/1302303002/diff/1/chrome/installer/util/maste...
chrome/installer/util/master_preferences.cc:264:
master_dictionary_->Set(prefs::kURLsToRestoreOnStartup,
This is problematic because the "latest" master_preferences documentation still
references the old value :-( :
https://www.chromium.org/administrators/configuring-other-preferences

Please loop in enterprise folks to see what to do about this.

https://codereview.chromium.org/1302303002/diff/1/tools/metrics/histograms/hi...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/1302303002/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:39976: - 
<owner>mad@chromium.org</owner>
Mark <osbolete> rather than deleting.

[sdefresne, 2015-08-25 07:37:03]

sdefresne@chromium.org changed reviewers:
+ atwilson@chromium.org

[sdefresne, 2015-08-25 07:37:24]

On 2015/08/24 at 21:48:46, gab wrote:
> +zea, see question below
> 
> Please cross-reference with https://codereview.chromium.org/24930003 which
introduced this code to see if there isn't more dead code to remove.
> 
> Thanks,
> Gab
> 
>
https://codereview.chromium.org/1302303002/diff/1/chrome/browser/prefs/pref_m...
> File chrome/browser/prefs/pref_model_associator.cc (right):
> 
>
https://codereview.chromium.org/1302303002/diff/1/chrome/browser/prefs/pref_m...
> chrome/browser/prefs/pref_model_associator.cc:63: { NULL, NULL },
> +zea: Think we should keep this around or simply get rid of it? I know it was
tricky to add so perhaps it is indeed worth keeping around an an empty list for
now?
> 
>
https://codereview.chromium.org/1302303002/diff/1/chrome/browser/prefs/pref_s...
> File chrome/browser/prefs/pref_service_syncable_unittest.cc (left):
> 
>
https://codereview.chromium.org/1302303002/diff/1/chrome/browser/prefs/pref_s...
> chrome/browser/prefs/pref_service_syncable_unittest.cc:297:
TEST_F(PrefServiceSyncableTest, ModelAssociationMigrateOldData) {
> If we do keep the code around (see comment in .cc) though, we should keep the
tests around -- which I guess means dummy pref names in impl :-( or better mocks
for tests?
> 
>
https://codereview.chromium.org/1302303002/diff/1/chrome/browser/prefs/sessio...
> File chrome/browser/prefs/session_startup_pref.cc (left):
> 
>
https://codereview.chromium.org/1302303002/diff/1/chrome/browser/prefs/sessio...
> chrome/browser/prefs/session_startup_pref.cc:21: enum
StartupURLsMigrationMetrics {
> rm this enum
> 
>
https://codereview.chromium.org/1302303002/diff/1/chrome/browser/prefs/sessio...
> chrome/browser/prefs/session_startup_pref.cc:169: base::Time now =
base::Time::Now();
> Cross-referencing with https://codereview.chromium.org/24930003 which
introduced this code, looks like time.h and histograms.h were added then and
that this was still there only use in this file to this day so they can be
removed.
> 
>
https://codereview.chromium.org/1302303002/diff/1/chrome/installer/util/maste...
> File chrome/installer/util/master_preferences.cc (left):
> 
>
https://codereview.chromium.org/1302303002/diff/1/chrome/installer/util/maste...
> chrome/installer/util/master_preferences.cc:264:
master_dictionary_->Set(prefs::kURLsToRestoreOnStartup,
> This is problematic because the "latest" master_preferences documentation
still references the old value :-( :
https://www.chromium.org/administrators/configuring-other-preferences
> 
> Please loop in enterprise folks to see what to do about this.

atwilson: can we safely remove this?

>
https://codereview.chromium.org/1302303002/diff/1/tools/metrics/histograms/hi...
> File tools/metrics/histograms/histograms.xml (left):
> 
>
https://codereview.chromium.org/1302303002/diff/1/tools/metrics/histograms/hi...
> tools/metrics/histograms/histograms.xml:39976: - 
<owner>mad@chromium.org</owner>
> Mark <osbolete> rather than deleting.

[sdefresne, 2015-08-25 08:32:08]

On 2015/08/24 at 21:26:40, gab wrote:
> On 2015/08/24 13:49:22, sdefresne wrote:
> > ping?
> 
> Looking but first, curious why you say M19? From my research this is more M32.
> 
> i.e., migration was introduced in 990ebf179de71b951e063042797218aeb243c667
which from "git log 990ebf179de71b951e063042797218aeb243c667 -- chrome/VERSION"
is branch 1676 which from
https://en.wikipedia.org/wiki/Google_Chrome_release_history is M32 (branched off
of trunk at 1700).
> 
> Not saying we shouldn't remove it, but we should document the removal with the
right info.

Gotcha (I though it was introduced at the same point as the other migration for
kRestoreOnStartupMigrated. Corrected the description.

[sdefresne, 2015-08-25 08:36:11]

https://codereview.chromium.org/1302303002/diff/1/chrome/browser/prefs/sessio...
File chrome/browser/prefs/session_startup_pref.cc (left):

https://codereview.chromium.org/1302303002/diff/1/chrome/browser/prefs/sessio...
chrome/browser/prefs/session_startup_pref.cc:21: enum
StartupURLsMigrationMetrics {
On 2015/08/24 at 21:48:45, gab wrote:
> rm this enum

Done.

https://codereview.chromium.org/1302303002/diff/1/chrome/browser/prefs/sessio...
chrome/browser/prefs/session_startup_pref.cc:169: base::Time now =
base::Time::Now();
On 2015/08/24 at 21:48:45, gab wrote:
> Cross-referencing with https://codereview.chromium.org/24930003 which
introduced this code, looks like time.h and histograms.h were added then and
that this was still there only use in this file to this day so they can be
removed.

Done.

https://codereview.chromium.org/1302303002/diff/1/chrome/installer/util/maste...
File chrome/installer/util/master_preferences.cc (left):

https://codereview.chromium.org/1302303002/diff/1/chrome/installer/util/maste...
chrome/installer/util/master_preferences.cc:264:
master_dictionary_->Set(prefs::kURLsToRestoreOnStartup,
On 2015/08/24 at 21:48:45, gab wrote:
> This is problematic because the "latest" master_preferences documentation
still references the old value :-( :
https://www.chromium.org/administrators/configuring-other-preferences
> 
> Please loop in enterprise folks to see what to do about this.

+atwilson: WDYT? Can we remove this? Should I update the documentation?

https://codereview.chromium.org/1302303002/diff/1/tools/metrics/histograms/hi...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/1302303002/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:39976: - 
<owner>mad@chromium.org</owner>
On 2015/08/24 at 21:48:46, gab wrote:
> Mark <osbolete> rather than deleting.

Done.

[Nicolas Zea, 2015-08-25 17:10:30]

LG at a high level

https://codereview.chromium.org/1302303002/diff/1/chrome/browser/prefs/pref_m...
File chrome/browser/prefs/pref_model_associator.cc (right):

https://codereview.chromium.org/1302303002/diff/1/chrome/browser/prefs/pref_m...
chrome/browser/prefs/pref_model_associator.cc:63: { NULL, NULL },
On 2015/08/24 21:48:45, gab wrote:
> +zea: Think we should keep this around or simply get rid of it? I know it was
> tricky to add so perhaps it is indeed worth keeping around an an empty list
for
> now?

I'd rather get rid of it personally. If someone does want to bring it back,
it'll be preserved in the git history (and we could always leave a comment
explaining that if a migration does happen, where it should be performed and
how).

[sdefresne, 2015-08-26 16:00:56]

Please take another look.

https://codereview.chromium.org/1302303002/diff/1/chrome/browser/prefs/pref_m...
File chrome/browser/prefs/pref_model_associator.cc (right):

https://codereview.chromium.org/1302303002/diff/1/chrome/browser/prefs/pref_m...
chrome/browser/prefs/pref_model_associator.cc:63: { NULL, NULL },
On 2015/08/25 at 17:10:30, Nicolas Zea wrote:
> On 2015/08/24 21:48:45, gab wrote:
> > +zea: Think we should keep this around or simply get rid of it? I know it
was
> > tricky to add so perhaps it is indeed worth keeping around an an empty list
for
> > now?
> 
> I'd rather get rid of it personally. If someone does want to bring it back,
it'll be preserved in the git history (and we could always leave a comment
explaining that if a migration does happen, where it should be performed and
how).

Done.

https://codereview.chromium.org/1302303002/diff/1/chrome/browser/prefs/pref_s...
File chrome/browser/prefs/pref_service_syncable_unittest.cc (left):

https://codereview.chromium.org/1302303002/diff/1/chrome/browser/prefs/pref_s...
chrome/browser/prefs/pref_service_syncable_unittest.cc:297:
TEST_F(PrefServiceSyncableTest, ModelAssociationMigrateOldData) {
On 2015/08/24 at 21:48:45, gab wrote:
> If we do keep the code around (see comment in .cc) though, we should keep the
tests around -- which I guess means dummy pref names in impl :-( or better mocks
for tests?

Implementation has been removed as recommended by zea@.

https://codereview.chromium.org/1302303002/diff/1/chrome/installer/util/maste...
File chrome/installer/util/master_preferences.cc (left):

https://codereview.chromium.org/1302303002/diff/1/chrome/installer/util/maste...
chrome/installer/util/master_preferences.cc:264:
master_dictionary_->Set(prefs::kURLsToRestoreOnStartup,
On 2015/08/25 at 08:36:10, sdefresne wrote:
> On 2015/08/24 at 21:48:45, gab wrote:
> > This is problematic because the "latest" master_preferences documentation
still references the old value :-( :
https://www.chromium.org/administrators/configuring-other-preferences
> > 
> > Please loop in enterprise folks to see what to do about this.
> 
> +atwilson: WDYT? Can we remove this? Should I update the documentation?

According to offline discussion, we can remove this and the documentation has
been updated.

[sdefresne, 2015-08-26 16:17:30]

sdefresne@chromium.org changed reviewers:
+ bartfab@chromium.org

[Nicolas Zea, 2015-08-26 16:57:37]

pref_model_associator changes LGTM

[gab, 2015-08-27 18:01:50]

lgtm, thanks for this cleanup :-)

https://codereview.chromium.org/1302303002/diff/40001/chrome/browser/prefs/pr...
File chrome/browser/prefs/pref_model_associator.h (right):

https://codereview.chromium.org/1302303002/diff/40001/chrome/browser/prefs/pr...
chrome/browser/prefs/pref_model_associator.h:15: #include
"base/gtest_prod_util.h"
rm this include (looks like it was added in
https://codereview.chromium.org/24930003 and its need as already been removed,
but let's clean it up in this CL)

[sdefresne, 2015-08-31 09:46:00]

sdefresne@chromium.org changed reviewers:
+ asvitkine@chromium.org
- bartfab@chromium.org

[sdefresne, 2015-08-31 09:46:01]

asvitkine: can you review
  tools/metrics/histograms/histograms.xml

[Alexei Svitkine (slow), 2015-08-31 15:08:12]

lgtm

https://codereview.chromium.org/1302303002/diff/60001/chrome/browser/prefs/pr...
File chrome/browser/prefs/pref_model_associator.cc (right):

https://codereview.chromium.org/1302303002/diff/60001/chrome/browser/prefs/pr...
chrome/browser/prefs/pref_model_associator.cc:184: } else {
Nit: No need for } else { if you have a continue above.

[bartfab (slow), 2015-08-31 15:49:18]

bartfab@chromium.org changed reviewers:
+ bartfab@chromium.org

[bartfab (slow), 2015-08-31 15:49:19]

I am not sure we are ready to remove this code. UMA shows that all three code
paths (migration timestamp missing / migration never happened / old pref value
changed after migration) were executed millions of times in the past month.

[gab, 2015-08-31 16:57:27]

On 2015/08/31 15:49:19, bartfab wrote:
> I am not sure we are ready to remove this code. UMA shows that all three code
> paths (migration timestamp missing / migration never happened / old pref value
> changed after migration) were executed millions of times in the past month.

See email thread.

[sdefresne, 2015-09-02 11:23:37]

Thank you for the review.

bartfab@: do you think this should land or not now you have the extra
information from gab@?

https://codereview.chromium.org/1302303002/diff/60001/chrome/browser/prefs/pr...
File chrome/browser/prefs/pref_model_associator.cc (right):

https://codereview.chromium.org/1302303002/diff/60001/chrome/browser/prefs/pr...
chrome/browser/prefs/pref_model_associator.cc:184: } else {
On 2015/08/31 at 15:08:11, Alexei Svitkine wrote:
> Nit: No need for } else { if you have a continue above.

Done.

[bartfab (slow), 2015-09-02 11:41:33]

On 2015/09/02 11:23:37, sdefresne wrote:
> Thank you for the review.
> 
> bartfab@: do you think this should land or not now you have the extra
> information from gab@?
> 
>
https://codereview.chromium.org/1302303002/diff/60001/chrome/browser/prefs/pr...
> File chrome/browser/prefs/pref_model_associator.cc (right):
> 
>
https://codereview.chromium.org/1302303002/diff/60001/chrome/browser/prefs/pr...
> chrome/browser/prefs/pref_model_associator.cc:184: } else {
> On 2015/08/31 at 15:08:11, Alexei Svitkine wrote:
> > Nit: No need for } else { if you have a continue above.
> 
> Done.

The thread is still ongoing. My concern is that we should not break the
migration for master_preferences without an unambiguous OK by PM. If you modify
your CL so that it kills the main migration path but leaves the one in
master_preferences.cc in place, I think this can land straight away.

[gab, 2015-11-12 13:43:56]

On 2015/09/02 11:41:33, bartfab (slow) wrote:
> On 2015/09/02 11:23:37, sdefresne wrote:
> > Thank you for the review.
> > 
> > bartfab@: do you think this should land or not now you have the extra
> > information from gab@?
> > 
> >
>
https://codereview.chromium.org/1302303002/diff/60001/chrome/browser/prefs/pr...
> > File chrome/browser/prefs/pref_model_associator.cc (right):
> > 
> >
>
https://codereview.chromium.org/1302303002/diff/60001/chrome/browser/prefs/pr...
> > chrome/browser/prefs/pref_model_associator.cc:184: } else {
> > On 2015/08/31 at 15:08:11, Alexei Svitkine wrote:
> > > Nit: No need for } else { if you have a continue above.
> > 
> > Done.
> 
> The thread is still ongoing. My concern is that we should not break the
> migration for master_preferences without an unambiguous OK by PM. If you
modify
> your CL so that it kills the main migration path but leaves the one in
> master_preferences.cc in place, I think this can land straight away.

Anything left to decide here? Can we land this?

[sdefresne, 2015-11-12 13:59:20]

On 2015/11/12 at 13:43:56, gab wrote:
> On 2015/09/02 11:41:33, bartfab (slow) wrote:
> > On 2015/09/02 11:23:37, sdefresne wrote:
> > > Thank you for the review.
> > > 
> > > bartfab@: do you think this should land or not now you have the extra
> > > information from gab@?
> > > 
> > >
> >
https://codereview.chromium.org/1302303002/diff/60001/chrome/browser/prefs/pr...
> > > File chrome/browser/prefs/pref_model_associator.cc (right):
> > > 
> > >
> >
https://codereview.chromium.org/1302303002/diff/60001/chrome/browser/prefs/pr...
> > > chrome/browser/prefs/pref_model_associator.cc:184: } else {
> > > On 2015/08/31 at 15:08:11, Alexei Svitkine wrote:
> > > > Nit: No need for } else { if you have a continue above.
> > > 
> > > Done.
> > 
> > The thread is still ongoing. My concern is that we should not break the
> > migration for master_preferences without an unambiguous OK by PM. If you
modify
> > your CL so that it kills the main migration path but leaves the one in
> > master_preferences.cc in place, I think this can land straight away.
> 
> Anything left to decide here? Can we land this?

No decision was taken in the thread AFAICT.

[bartfab (slow), 2015-11-12 14:05:42]

On 2015/11/12 13:59:20, sdefresne wrote:
> On 2015/11/12 at 13:43:56, gab wrote:
> > On 2015/09/02 11:41:33, bartfab (slow) wrote:
> > > On 2015/09/02 11:23:37, sdefresne wrote:
> > > > Thank you for the review.
> > > > 
> > > > bartfab@: do you think this should land or not now you have the extra
> > > > information from gab@?
> > > > 
> > > >
> > >
>
https://codereview.chromium.org/1302303002/diff/60001/chrome/browser/prefs/pr...
> > > > File chrome/browser/prefs/pref_model_associator.cc (right):
> > > > 
> > > >
> > >
>
https://codereview.chromium.org/1302303002/diff/60001/chrome/browser/prefs/pr...
> > > > chrome/browser/prefs/pref_model_associator.cc:184: } else {
> > > > On 2015/08/31 at 15:08:11, Alexei Svitkine wrote:
> > > > > Nit: No need for } else { if you have a continue above.
> > > > 
> > > > Done.
> > > 
> > > The thread is still ongoing. My concern is that we should not break the
> > > migration for master_preferences without an unambiguous OK by PM. If you
> modify
> > > your CL so that it kills the main migration path but leaves the one in
> > > master_preferences.cc in place, I think this can land straight away.
> > 
> > Anything left to decide here? Can we land this?
> 
> No decision was taken in the thread AFAICT.

Correct. The thread seems to have stalled but nothing was decided. Feel free to
ping the thread.

[sdefresne, 2015-12-02 11:58:51]

Description was changed from

==========
Remove year+ old migration support of "session.urls_to_restore_on_startup".

Remove code supporting the migration of "session.urls_to_restore_on_startup"
to "session.startup_urls", the corresponding constants, histograms and unit
tests. This migration was there to support pre M-32 profiles.

This is a preparatory step before componentization of //chrome/browser/prefs
as keeping the migration code in the component is not worth the complexity.

Mark histograms as obsolete as of 2015/09 due to the removal of the migration
code they were mesuring.

BUG=522534,525079
==========

to

==========
Remove year+ old migration support of "session.urls_to_restore_on_startup".

Remove code supporting the migration of "session.urls_to_restore_on_startup"
to "session.startup_urls", the corresponding constants, histograms and unit
tests. This migration was there to support pre M-32 profiles.

This is a preparatory step before componentization of //chrome/browser/prefs
as keeping the migration code in the component is not worth the complexity.

Mark histograms as obsolete as of 2015/09 due to the removal of the migration
code they were mesuring.

BUG=525079
==========

[sdefresne, 2015-12-02 12:12:47]

gab: Please take another look!

[gab, 2015-12-02 21:06:52]

I'm amazed by how much code was supporting this lone migration..! So glad this
is being cleaned up, thanks a lot :-)!

lgtm w/ comments

Please update the CL description to be relevant to today's status.

Cheers,
Gab

https://codereview.chromium.org/1302303002/diff/180001/chrome/browser/prefs/b...
File chrome/browser/prefs/browser_prefs.cc (right):

https://codereview.chromium.org/1302303002/diff/180001/chrome/browser/prefs/b...
chrome/browser/prefs/browser_prefs.cc:239: // by
"session.startup_urls_migration_time". Both are obsolete now (2015/09) and
2015/12

https://codereview.chromium.org/1302303002/diff/180001/components/syncable_pr...
File components/syncable_prefs/pref_model_associator.cc (right):

https://codereview.chromium.org/1302303002/diff/180001/components/syncable_pr...
components/syncable_prefs/pref_model_associator.cc:194:
remaining_preferences.erase(sync_pref_name);
If I'm reading the old logic correctly, this should in the else block of the
above if?

https://codereview.chromium.org/1302303002/diff/180001/components/syncable_pr...
File components/syncable_prefs/pref_service_syncable_unittest.cc (right):

https://codereview.chromium.org/1302303002/diff/180001/components/syncable_pr...
components/syncable_prefs/pref_service_syncable_unittest.cc:38: const char
kListPrefName[] = "new_list_pref_name";
s/new_list_pref_name/list_pref_name/

https://codereview.chromium.org/1302303002/diff/180001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1302303002/diff/180001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:43684: +    Deprecated 12/2015 when
migration code was removed, crrev/1302303002.
"Deprecated 12/2015."

Is sufficient IMO. People can git blame and find this CL if they care to know
why.

[sdefresne, 2015-12-03 09:54:34]

Description was changed from

==========
Remove year+ old migration support of "session.urls_to_restore_on_startup".

Remove code supporting the migration of "session.urls_to_restore_on_startup"
to "session.startup_urls", the corresponding constants, histograms and unit
tests. This migration was there to support pre M-32 profiles.

This is a preparatory step before componentization of //chrome/browser/prefs
as keeping the migration code in the component is not worth the complexity.

Mark histograms as obsolete as of 2015/09 due to the removal of the migration
code they were mesuring.

BUG=525079
==========

to

==========
Remove year+ old migration support of "session.urls_to_restore_on_startup".

Remove code supporting the migration of "session.urls_to_restore_on_startup"
to "session.startup_urls", the corresponding constants, histograms and unit
tests. This migration was there to support pre M-32 profiles.

This is a preparatory step before componentization of //chrome/browser/prefs
as keeping the migration code in the component is not worth the complexity.

Mark histograms as obsolete as of 2015/12 due to the removal of the migration
code they were mesuring.

BUG=525079
==========

[sdefresne, 2015-12-03 10:02:30]

Thank you for the review.

https://codereview.chromium.org/1302303002/diff/180001/chrome/browser/prefs/b...
File chrome/browser/prefs/browser_prefs.cc (right):

https://codereview.chromium.org/1302303002/diff/180001/chrome/browser/prefs/b...
chrome/browser/prefs/browser_prefs.cc:239: // by
"session.startup_urls_migration_time". Both are obsolete now (2015/09) and
On 2015/12/02 at 21:06:51, gab wrote:
> 2015/12

Done.

https://codereview.chromium.org/1302303002/diff/180001/components/syncable_pr...
File components/syncable_prefs/pref_model_associator.cc (right):

https://codereview.chromium.org/1302303002/diff/180001/components/syncable_pr...
components/syncable_prefs/pref_model_associator.cc:194:
remaining_preferences.erase(sync_pref_name);
On 2015/12/02 at 21:06:51, gab wrote:
> If I'm reading the old logic correctly, this should in the else block of the
above if?

There is no else block now as I changed the code from the following :

  if (condition) {
    if (false) {
    } else {
      continue;
    }
  } else {
    dox();
  }

to

  if (condition) {
    continue;
  }
  dox();

And thus removed the "else" block as per style-guide (i.e. no "else" if the "if"
block contains return/continue).

https://codereview.chromium.org/1302303002/diff/180001/components/syncable_pr...
File components/syncable_prefs/pref_service_syncable_unittest.cc (right):

https://codereview.chromium.org/1302303002/diff/180001/components/syncable_pr...
components/syncable_prefs/pref_service_syncable_unittest.cc:38: const char
kListPrefName[] = "new_list_pref_name";
On 2015/12/02 at 21:06:52, gab wrote:
> s/new_list_pref_name/list_pref_name/

Done.

[sdefresne, 2015-12-03 13:50:14]

The CQ bit was checked by sdefresne@chromium.org

[sdefresne, 2015-12-03 13:50:15]

The patchset sent to the CQ was uploaded after l-g-t-m from zea@chromium.org,
asvitkine@chromium.org, gab@chromium.org
Link to the patchset: https://codereview.chromium.org/1302303002/#ps200001
(title: "Address comments")

[commit-bot: I haz the power, 2015-12-03 13:50:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1302303002/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1302303002/200001

[sdefresne, 2015-12-03 13:52:07]

I'm sending the CL to the CQ as I've addressed all comments and replied to one
that I believed to be incorrect ("else" clause).

[commit-bot: I haz the power, 2015-12-03 15:06:27]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2015-12-03 15:07:13]

Description was changed from

==========
Remove year+ old migration support of "session.urls_to_restore_on_startup".

Remove code supporting the migration of "session.urls_to_restore_on_startup"
to "session.startup_urls", the corresponding constants, histograms and unit
tests. This migration was there to support pre M-32 profiles.

This is a preparatory step before componentization of //chrome/browser/prefs
as keeping the migration code in the component is not worth the complexity.

Mark histograms as obsolete as of 2015/12 due to the removal of the migration
code they were mesuring.

BUG=525079
==========

to

==========
Remove year+ old migration support of "session.urls_to_restore_on_startup".

Remove code supporting the migration of "session.urls_to_restore_on_startup"
to "session.startup_urls", the corresponding constants, histograms and unit
tests. This migration was there to support pre M-32 profiles.

This is a preparatory step before componentization of //chrome/browser/prefs
as keeping the migration code in the component is not worth the complexity.

Mark histograms as obsolete as of 2015/12 due to the removal of the migration
code they were mesuring.

BUG=525079

Committed: https://crrev.com/39dc3a1ffc119ce551abb056057c7076b8017401
Cr-Commit-Position: refs/heads/master@{#362981}
==========

[commit-bot: I haz the power, 2015-12-03 15:07:14]

Patchset 11 (id:??) landed as
https://crrev.com/39dc3a1ffc119ce551abb056057c7076b8017401
Cr-Commit-Position: refs/heads/master@{#362981}

[gab, 2015-12-03 15:25:37]

lgtm

https://codereview.chromium.org/1302303002/diff/180001/components/syncable_pr...
File components/syncable_prefs/pref_model_associator.cc (right):

https://codereview.chromium.org/1302303002/diff/180001/components/syncable_pr...
components/syncable_prefs/pref_model_associator.cc:194:
remaining_preferences.erase(sync_pref_name);
On 2015/12/03 10:02:29, sdefresne wrote:
> On 2015/12/02 at 21:06:51, gab wrote:
> > If I'm reading the old logic correctly, this should in the else block of the
> above if?
> 
> There is no else block now as I changed the code from the following :
> 
>   if (condition) {
>     if (false) {
>     } else {
>       continue;
>     }
>   } else {
>     dox();
>   }
> 
> to
> 
>   if (condition) {
>     continue;
>   }
>   dox();
> 
> And thus removed the "else" block as per style-guide (i.e. no "else" if the
"if"
> block contains return/continue).

Ah, indeed, had missed that.