Create and remove sync nodes whenever preferences flip their managed flag

Create and remove sync nodes whenever preferences flip their managed flag

BUG=none
TEST=none

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

[Mattias Nissler, 2010-05-25 16:49:10]

Albert,

here is a first incarnation of sync node maintenance upon managed pref state
switches. I've found it to work well locally, but I'm not at all familiar with
the sync stuff, so I thought I'd ask for comments early. Thanks!

Mattias

[ncarter, 2010-05-25 17:52:54]

You must add unit tests that exercise this code.  It would be valuable to have a
sync_integration_test, too; rsimha might be willing to write one.

http://codereview.chromium.org/2182001/diff/2001/3001
File chrome/browser/sync/glue/preference_change_processor.cc (right):

http://codereview.chromium.org/2182001/diff/2001/3001#newcode77
chrome/browser/sync/glue/preference_change_processor.cc:77:
model_associator_->InitPreferenceAssociation(&trans, &root, preference);
If this fails, should we log and handle the failure?

http://codereview.chromium.org/2182001/diff/2001/3001#newcode80
chrome/browser/sync/glue/preference_change_processor.cc:80: return;
You remove the OnUnrecoverableError case here, which was a valuable invariant
check.  I think that's a mistake.

http://codereview.chromium.org/2182001/diff/2001/3001#newcode148
chrome/browser/sync/glue/preference_change_processor.cc:148: if
(pref->IsManaged())
If managed preferences are not ever supposed to be in the
model_associator_->synced_preferences() set, then we should add a NOTREACHED()
here before the continue.

http://codereview.chromium.org/2182001/diff/2001/3002
File chrome/browser/sync/glue/preference_model_associator.cc (right):

http://codereview.chromium.org/2182001/diff/2001/3002#newcode46
chrome/browser/sync/glue/preference_model_associator.cc:46: bool
PreferenceModelAssociator::InitPreferenceAssociation(
This choice of name does not adequately distinguish this operation from other
methods of this class (e.g. what's the difference between
PreferenceModelAssociator::InitPreferenceAssociation and
PreferenceModelAssociator::Associate?) As a name, maybe PropagateAndAssociate,
or something that indicates you're also adding/removing sync nodes and
preferences, not only updating the node association map?  Albertb might have a
better idea.

http://codereview.chromium.org/2182001/diff/2001/3002#newcode128
chrome/browser/sync/glue/preference_model_associator.cc:128: if
(sync_api::kInvalidId == InitPreferenceAssociation(&trans, &root, pref))
This function returns a bool, not an ID.

[Mattias Nissler, 2010-05-26 09:52:36]

Thanks for your input! I also see the need for unit tests, and I'll go and write
some. My reason for publishing this code early was to get some guidance and to
find out whether I'm heading in the right direction.

http://codereview.chromium.org/2182001/diff/2001/3001
File chrome/browser/sync/glue/preference_change_processor.cc (right):

http://codereview.chromium.org/2182001/diff/2001/3001#newcode77
chrome/browser/sync/glue/preference_change_processor.cc:77:
model_associator_->InitPreferenceAssociation(&trans, &root, preference);
On 2010/05/25 17:52:55, nick wrote:
> If this fails, should we log and handle the failure?

Yes, of course.
Done.

http://codereview.chromium.org/2182001/diff/2001/3001#newcode80
chrome/browser/sync/glue/preference_change_processor.cc:80: return;
On 2010/05/25 17:52:55, nick wrote:
> You remove the OnUnrecoverableError case here, which was a valuable invariant
> check.  I think that's a mistake.

You're correct, I added it back in at the appropriate places.
Done.

http://codereview.chromium.org/2182001/diff/2001/3001#newcode148
chrome/browser/sync/glue/preference_change_processor.cc:148: if
(pref->IsManaged())
On 2010/05/25 17:52:55, nick wrote:
> If managed preferences are not ever supposed to be in the
> model_associator_->synced_preferences() set, then we should add a NOTREACHED()
> here before the continue.

Oh, they are in synced_preferences(). The whole point of this CL is to enable
the sync code to cope with preferences flipping their managed bit at runtime.

Note that it may actually happen that we get a sync model change triggered by a
browser instance that's not running under configuration management, so it's
expected for configuration-managed instances to hit this conditional.

http://codereview.chromium.org/2182001/diff/2001/3002
File chrome/browser/sync/glue/preference_model_associator.cc (right):

http://codereview.chromium.org/2182001/diff/2001/3002#newcode46
chrome/browser/sync/glue/preference_model_associator.cc:46: bool
PreferenceModelAssociator::InitPreferenceAssociation(
On 2010/05/25 17:52:55, nick wrote:
> This choice of name does not adequately distinguish this operation from other
> methods of this class (e.g. what's the difference between
> PreferenceModelAssociator::InitPreferenceAssociation and
> PreferenceModelAssociator::Associate?) As a name, maybe PropagateAndAssociate,
> or something that indicates you're also adding/removing sync nodes and
> preferences, not only updating the node association map?  Albertb might have a
> better idea.

Done.

http://codereview.chromium.org/2182001/diff/2001/3002#newcode128
chrome/browser/sync/glue/preference_model_associator.cc:128: if
(sync_api::kInvalidId == InitPreferenceAssociation(&trans, &root, pref))
On 2010/05/25 17:52:55, nick wrote:
> This function returns a bool, not an ID.

Done.

[ncarter, 2010-06-01 21:16:10]

On 2010/05/26 09:52:36, mnissler wrote:
> Thanks for your input! I also see the need for unit tests, and I'll go and
write
> some. My reason for publishing this code early was to get some guidance and to
> find out whether I'm heading in the right direction.
> 
> http://codereview.chromium.org/2182001/diff/2001/3001
> File chrome/browser/sync/glue/preference_change_processor.cc (right):
> 
> http://codereview.chromium.org/2182001/diff/2001/3001#newcode77
> chrome/browser/sync/glue/preference_change_processor.cc:77:
> model_associator_->InitPreferenceAssociation(&trans, &root, preference);
> On 2010/05/25 17:52:55, nick wrote:
> > If this fails, should we log and handle the failure?
> 
> Yes, of course.
> Done.
> 
> http://codereview.chromium.org/2182001/diff/2001/3001#newcode80
> chrome/browser/sync/glue/preference_change_processor.cc:80: return;
> On 2010/05/25 17:52:55, nick wrote:
> > You remove the OnUnrecoverableError case here, which was a valuable
invariant
> > check.  I think that's a mistake.
> 
> You're correct, I added it back in at the appropriate places.
> Done.
> 
> http://codereview.chromium.org/2182001/diff/2001/3001#newcode148
> chrome/browser/sync/glue/preference_change_processor.cc:148: if
> (pref->IsManaged())
> On 2010/05/25 17:52:55, nick wrote:
> > If managed preferences are not ever supposed to be in the
> > model_associator_->synced_preferences() set, then we should add a
NOTREACHED()
> > here before the continue.
> 
> Oh, they are in synced_preferences(). The whole point of this CL is to enable
> the sync code to cope with preferences flipping their managed bit at runtime.
> 
> Note that it may actually happen that we get a sync model change triggered by
a
> browser instance that's not running under configuration management, so it's
> expected for configuration-managed instances to hit this conditional.
> 
> http://codereview.chromium.org/2182001/diff/2001/3002
> File chrome/browser/sync/glue/preference_model_associator.cc (right):
> 
> http://codereview.chromium.org/2182001/diff/2001/3002#newcode46
> chrome/browser/sync/glue/preference_model_associator.cc:46: bool
> PreferenceModelAssociator::InitPreferenceAssociation(
> On 2010/05/25 17:52:55, nick wrote:
> > This choice of name does not adequately distinguish this operation from
other
> > methods of this class (e.g. what's the difference between
> > PreferenceModelAssociator::InitPreferenceAssociation and
> > PreferenceModelAssociator::Associate?) As a name, maybe
PropagateAndAssociate,
> > or something that indicates you're also adding/removing sync nodes and
> > preferences, not only updating the node association map?  Albertb might have
a
> > better idea.
> 
> Done.
> 
> http://codereview.chromium.org/2182001/diff/2001/3002#newcode128
> chrome/browser/sync/glue/preference_model_associator.cc:128: if
> (sync_api::kInvalidId == InitPreferenceAssociation(&trans, &root, pref))
> On 2010/05/25 17:52:55, nick wrote:
> > This function returns a bool, not an ID.
> 
> Done.

What's the status of this patch set?  Waiting on unit tests?

[Mattias Nissler, 2010-06-02 07:08:39]

On 2010/06/01 21:16:10, nick wrote:
> On 2010/05/26 09:52:36, mnissler wrote:
> > Thanks for your input! I also see the need for unit tests, and I'll go and
> write
> > some. My reason for publishing this code early was to get some guidance and
to
> > find out whether I'm heading in the right direction.
> > 
> > http://codereview.chromium.org/2182001/diff/2001/3001
> > File chrome/browser/sync/glue/preference_change_processor.cc (right):
> > 
> > http://codereview.chromium.org/2182001/diff/2001/3001#newcode77
> > chrome/browser/sync/glue/preference_change_processor.cc:77:
> > model_associator_->InitPreferenceAssociation(&trans, &root, preference);
> > On 2010/05/25 17:52:55, nick wrote:
> > > If this fails, should we log and handle the failure?
> > 
> > Yes, of course.
> > Done.
> > 
> > http://codereview.chromium.org/2182001/diff/2001/3001#newcode80
> > chrome/browser/sync/glue/preference_change_processor.cc:80: return;
> > On 2010/05/25 17:52:55, nick wrote:
> > > You remove the OnUnrecoverableError case here, which was a valuable
> invariant
> > > check.  I think that's a mistake.
> > 
> > You're correct, I added it back in at the appropriate places.
> > Done.
> > 
> > http://codereview.chromium.org/2182001/diff/2001/3001#newcode148
> > chrome/browser/sync/glue/preference_change_processor.cc:148: if
> > (pref->IsManaged())
> > On 2010/05/25 17:52:55, nick wrote:
> > > If managed preferences are not ever supposed to be in the
> > > model_associator_->synced_preferences() set, then we should add a
> NOTREACHED()
> > > here before the continue.
> > 
> > Oh, they are in synced_preferences(). The whole point of this CL is to
enable
> > the sync code to cope with preferences flipping their managed bit at
runtime.
> > 
> > Note that it may actually happen that we get a sync model change triggered
by
> a
> > browser instance that's not running under configuration management, so it's
> > expected for configuration-managed instances to hit this conditional.
> > 
> > http://codereview.chromium.org/2182001/diff/2001/3002
> > File chrome/browser/sync/glue/preference_model_associator.cc (right):
> > 
> > http://codereview.chromium.org/2182001/diff/2001/3002#newcode46
> > chrome/browser/sync/glue/preference_model_associator.cc:46: bool
> > PreferenceModelAssociator::InitPreferenceAssociation(
> > On 2010/05/25 17:52:55, nick wrote:
> > > This choice of name does not adequately distinguish this operation from
> other
> > > methods of this class (e.g. what's the difference between
> > > PreferenceModelAssociator::InitPreferenceAssociation and
> > > PreferenceModelAssociator::Associate?) As a name, maybe
> PropagateAndAssociate,
> > > or something that indicates you're also adding/removing sync nodes and
> > > preferences, not only updating the node association map?  Albertb might
have
> a
> > > better idea.
> > 
> > Done.
> > 
> > http://codereview.chromium.org/2182001/diff/2001/3002#newcode128
> > chrome/browser/sync/glue/preference_model_associator.cc:128: if
> > (sync_api::kInvalidId == InitPreferenceAssociation(&trans, &root, pref))
> > On 2010/05/25 17:52:55, nick wrote:
> > > This function returns a bool, not an ID.
> > 
> > Done.
> 
> What's the status of this patch set?  Waiting on unit tests?

I will write them, but I'm waiting for the CL that enables PrefService to handle
managed prefs first since it's needed to write unit tests.

[Mattias Nissler, 2010-08-02 18:14:44]

Nick, can you please take another look? The reloading of policy is now actively
being worked on, so I'd like to get this CL in.

[Mattias Nissler, 2010-08-04 15:53:37]

+skrul

No reply? Anyone in sync land who could review this? ;) Thanks.

[skrul, 2010-08-09 19:03:18]

http://codereview.chromium.org/2182001/diff/18001/19001
File chrome/browser/sync/glue/preference_change_processor.cc (right):

http://codereview.chromium.org/2182001/diff/18001/19001#newcode57
chrome/browser/sync/glue/preference_change_processor.cc:57: bool user_modifiable
= preference->IsUserModifiable();
would it simplify the "if" nesting below if you moved the !user_modifiable case
right under here?  e.g.
if (! modifiable) {
  // disassociate & return
}

http://codereview.chromium.org/2182001/diff/18001/19004
File chrome/browser/sync/glue/preference_model_associator.h (right):

http://codereview.chromium.org/2182001/diff/18001/19004#newcode27
chrome/browser/sync/glue/preference_model_associator.h:27: class
WriteTransaction;
you can combine this forward declare with the previous one since it is the same
namespace

http://codereview.chromium.org/2182001/diff/18001/19004#newcode56
chrome/browser/sync/glue/preference_model_associator.h:56: sync_api::BaseNode&
root,
style guide disallows non-const references afaik

[albertb, 2010-08-09 19:08:55]

http://codereview.chromium.org/2182001/diff/18001/19003
File chrome/browser/sync/glue/preference_model_associator.cc (right):

http://codereview.chromium.org/2182001/diff/18001/19003#newcode89
chrome/browser/sync/glue/preference_model_associator.cc:89: } else if
(pref->IsUserControlled()) {
I'm a little confused by this. What's the difference between UserControlled and
UserModifiable? Can a pref ever be UserControlled but not UserModifiable?

http://codereview.chromium.org/2182001/diff/18001/19005
File chrome/browser/sync/profile_sync_service_preference_unittest.cc (right):

http://codereview.chromium.org/2182001/diff/18001/19005#newcode146
chrome/browser/sync/profile_sync_service_preference_unittest.cc:146: if
(node.InitByIdLookup(node_id))
You could move the if up and have an else if instead.

[Mattias Nissler, 2010-08-10 12:33:22]

Thanks for your comments! I just uploaded an updated version.

http://codereview.chromium.org/2182001/diff/18001/19001
File chrome/browser/sync/glue/preference_change_processor.cc (right):

http://codereview.chromium.org/2182001/diff/18001/19001#newcode57
chrome/browser/sync/glue/preference_change_processor.cc:57: bool user_modifiable
= preference->IsUserModifiable();
On 2010/08/09 19:03:18, skrul wrote:
> would it simplify the "if" nesting below if you moved the !user_modifiable
case
> right under here?  e.g.
> if (! modifiable) {
>   // disassociate & return
> }
> 

Yes, good idea. Thanks!

http://codereview.chromium.org/2182001/diff/18001/19003
File chrome/browser/sync/glue/preference_model_associator.cc (right):

http://codereview.chromium.org/2182001/diff/18001/19003#newcode89
chrome/browser/sync/glue/preference_model_associator.cc:89: } else if
(pref->IsUserControlled()) {
On 2010/08/09 19:08:55, albertb wrote:
> I'm a little confused by this. What's the difference between UserControlled
and
> UserModifiable? Can a pref ever be UserControlled but not UserModifiable?

No. UserControlled implies UserModifiable, but also means that there is a
non-default value configured by the user (otherwise we shouldn't sync the
value).

http://codereview.chromium.org/2182001/diff/18001/19004
File chrome/browser/sync/glue/preference_model_associator.h (right):

http://codereview.chromium.org/2182001/diff/18001/19004#newcode27
chrome/browser/sync/glue/preference_model_associator.h:27: class
WriteTransaction;
On 2010/08/09 19:03:18, skrul wrote:
> you can combine this forward declare with the previous one since it is the
same
> namespace

Done.

http://codereview.chromium.org/2182001/diff/18001/19004#newcode56
chrome/browser/sync/glue/preference_model_associator.h:56: sync_api::BaseNode&
root,
On 2010/08/09 19:03:18, skrul wrote:
> style guide disallows non-const references afaik

Done.

http://codereview.chromium.org/2182001/diff/18001/19005
File chrome/browser/sync/profile_sync_service_preference_unittest.cc (right):

http://codereview.chromium.org/2182001/diff/18001/19005#newcode146
chrome/browser/sync/profile_sync_service_preference_unittest.cc:146: if
(node.InitByIdLookup(node_id))
On 2010/08/09 19:08:55, albertb wrote:
> You could move the if up and have an else if instead.

Done.

[skrul, 2010-08-10 21:19:56]

Great, LGTM!