Clear a preference when sync tries to delete a preference

Clear a preference when sync tries to delete a preference

Preference can be deleted during sync rollback if the preference was not set before
first signin. Clear the preference if that happens.

BUG=362679
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=289048

[gab, 2014-08-08 12:55:43]

lg

+Nicolas as I don't think I should be the sole reviewer on this since I'm not
familiar with the guts of this code.

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

https://codereview.chromium.org/444763005/diff/1/chrome/browser/prefs/pref_mo...
chrome/browser/prefs/pref_model_associator.cc:515:
*pref_service_->GetDefaultPrefValue(pref_name));
You should just do pref_service_->ClearPref(pref_name); this will make it such
that there is no user-selected value and the default will thus kick in; as-is
otherwise this will explicitly make the default value the user-selected value
(i.e. subtle difference which matters when the default itself is changed in the
future).

[Nicolas Zea, 2014-08-08 17:38:33]

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

https://codereview.chromium.org/444763005/diff/1/chrome/browser/prefs/pref_mo...
chrome/browser/prefs/pref_model_associator.cc:481: if (iter->change_type() !=
syncer::SyncChange::ACTION_DELETE &&
does the ACTION_DELETE condition still need to be there?

https://codereview.chromium.org/444763005/diff/1/chrome/browser/prefs/pref_mo...
chrome/browser/prefs/pref_model_associator.cc:515:
*pref_service_->GetDefaultPrefValue(pref_name));
On 2014/08/08 12:55:43, gab wrote:
> You should just do pref_service_->ClearPref(pref_name); this will make it such
> that there is no user-selected value and the default will thus kick in; as-is
> otherwise this will explicitly make the default value the user-selected value
> (i.e. subtle difference which matters when the default itself is changed in
the
> future).

Agreed. This will actually sync the default value around since it's being stored
in the user store, even though different platforms sometimes have different
default values.

[haitaol1, 2014-08-08 21:28:03]

On 2014/08/08 17:38:33, Nicolas Zea wrote:
>
https://codereview.chromium.org/444763005/diff/1/chrome/browser/prefs/pref_mo...
> File chrome/browser/prefs/pref_model_associator.cc (right):
> 
>
https://codereview.chromium.org/444763005/diff/1/chrome/browser/prefs/pref_mo...
> chrome/browser/prefs/pref_model_associator.cc:481: if (iter->change_type() !=
> syncer::SyncChange::ACTION_DELETE &&
> does the ACTION_DELETE condition still need to be there?
> 
>
https://codereview.chromium.org/444763005/diff/1/chrome/browser/prefs/pref_mo...
> chrome/browser/prefs/pref_model_associator.cc:515:
> *pref_service_->GetDefaultPrefValue(pref_name));
> On 2014/08/08 12:55:43, gab wrote:
> > You should just do pref_service_->ClearPref(pref_name); this will make it
such
> > that there is no user-selected value and the default will thus kick in;
as-is
> > otherwise this will explicitly make the default value the user-selected
value
> > (i.e. subtle difference which matters when the default itself is changed in
> the
> > future).
> 
> Agreed. This will actually sync the default value around since it's being
stored
> in the user store, even though different platforms sometimes have different
> default values.

done

[haitaol1, 2014-08-08 22:50:02]

Rearranged name/value handling to simplify for deletion. Also added test. PTAL

[Nicolas Zea, 2014-08-08 23:19:07]

lgtm

[haitaol1, 2014-08-10 16:10:58]

The CQ bit was checked by haitaol@chromium.org

[commit-bot: I haz the power, 2014-08-10 16:12:08]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/haitaol@chromium.org/444763005/40001

[commit-bot: I haz the power, 2014-08-10 17:37:25]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[commit-bot: I haz the power, 2014-08-10 17:39:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-10 17:39:46]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[haitaol1, 2014-08-11 16:26:18]

gab@: can you LGTM?

[haitaol1, 2014-08-12 16:55:50]

On 2014/08/11 16:26:18, haitaol1 wrote:
> gab@: can you LGTM?

ping

[gab, 2014-08-12 17:00:44]

lgtm (sorry for the delay, I was OOO sick at the end of last week and was
attending now sick wife/child yesterday -- I thought I'd reviewed this yesterday
but looks like I forgot to actually send the mail in the midst of all this..)

Thanks,
Gab

[haitaol1, 2014-08-12 19:20:05]

On 2014/08/12 17:00:44, gab wrote:
> lgtm (sorry for the delay, I was OOO sick at the end of last week and was
> attending now sick wife/child yesterday -- I thought I'd reviewed this
yesterday
> but looks like I forgot to actually send the mail in the midst of all this..)
> 
> Thanks,
> Gab

No problem. Thanks for reviewing and best wishes to your family.

[haitaol1, 2014-08-12 19:20:11]

The CQ bit was checked by haitaol@chromium.org

[commit-bot: I haz the power, 2014-08-12 19:22:03]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/haitaol@chromium.org/444763005/40001

[commit-bot: I haz the power, 2014-08-12 19:28:32]

Change committed as 289048