sync: add InternalComponentsFactory::Switches to simplify passing switches to internal components.

sync: add InternalComponentsFactory::Switches to simplify passing switches to internal components.

Cleans up backoff retry override code to use InternalComponentsFactory::Switches rather than
global bool hack.

Also puts keystore encryption flag atop new mechanism.

(TBR sky for new chrome_switch).

TBR=sky@chromium.org
BUG=142029, 139839
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=151664

[tim (not reviewing), 2012-08-14 17:56:06]

rlarocque: please review everything

zea: fyi / optional review on InternalComponentsFactory changes, (what we
discussed yesterday re: easier configurability of SyncManager components).

[tim (not reviewing), 2012-08-14 18:15:39]

note - uploaded patchset that passes the switches configured by SyncBackendHost
along to the test factory in TestProfileSyncService.

[rlarocque, 2012-08-14 19:54:07]

http://codereview.chromium.org/10837231/diff/8031/chrome/browser/sync/glue/sy...
File chrome/browser/sync/glue/sync_backend_host.cc (right):

http://codereview.chromium.org/10837231/diff/8031/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/sync_backend_host.cc:424: if
(cl->HasSwitch(switches::kSyncKeystoreEncryption)) {
nit: I think this would be more readable if this was an if/else branch.  That
would make it more clear that the InternalComponentsFactory flag depends
entirely on the command line flag.

http://codereview.chromium.org/10837231/diff/8031/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/sync_backend_host.cc:428: if
(cl->HasSwitch(switches::kSyncShortInitialRetryOverride)) {
same nit here.

http://codereview.chromium.org/10837231/diff/8031/sync/internal_api/internal_...
File sync/internal_api/internal_components_factory_impl.cc (right):

http://codereview.chromium.org/10837231/diff/8031/sync/internal_api/internal_...
sync/internal_api/internal_components_factory_impl.cc:28: if
(switches_.backoff_override == BACKOFF_SHORT_INITIAL_RETRY_OVERRIDE)
nit: Again, I think this would be nicer as an if/else (or even a switch).

http://codereview.chromium.org/10837231/diff/8031/sync/internal_api/public/in...
File sync/internal_api/public/internal_components_factory.h (right):

http://codereview.chromium.org/10837231/diff/8031/sync/internal_api/public/in...
sync/internal_api/public/internal_components_factory.h:59: struct Switches {
I'd prefer to see this struct's members embedded directly in the factory.  It
would be simpler to pass these flags directly to the factory's constructor,
rather than constructing this object then passing it to the factory's
constructor.

[tim (not reviewing), 2012-08-14 20:20:51]

http://codereview.chromium.org/10837231/diff/8031/chrome/browser/sync/glue/sy...
File chrome/browser/sync/glue/sync_backend_host.cc (right):

http://codereview.chromium.org/10837231/diff/8031/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/sync_backend_host.cc:424: if
(cl->HasSwitch(switches::kSyncKeystoreEncryption)) {
On 2012/08/14 19:54:07, rlarocque wrote:
> nit: I think this would be more readable if this was an if/else branch.  That
> would make it more clear that the InternalComponentsFactory flag depends
> entirely on the command line flag.

The reason I did it this way is because it is most obvious what the default
expected behavior is, and we ensure that we always start from the same default
regardless of how many times the "extraordinary circumstances" below change or
the code gets re-arranged.

Maybe it'd be nicer to pull the CommandLine changes into their own routine, to
make this more clear?  Something like 

Switches factory_switches = {ENCRYPTION_LEGACY, BACKOFF_NORMAL};
OverrideSwitchesIfNecessary(&switches, CommandLine::ForCurrentProcess());

http://codereview.chromium.org/10837231/diff/8031/sync/internal_api/public/in...
File sync/internal_api/public/internal_components_factory.h (right):

http://codereview.chromium.org/10837231/diff/8031/sync/internal_api/public/in...
sync/internal_api/public/internal_components_factory.h:59: struct Switches {
On 2012/08/14 19:54:07, rlarocque wrote:
> I'd prefer to see this struct's members embedded directly in the factory.  It
> would be simpler to pass these flags directly to the factory's constructor,
> rather than constructing this object then passing it to the factory's
> constructor. 

That's exactly what I wanted to avoid.  In the past we've had to change
constructors in interfaces, in real impls, in fake impls, etc for config or
transient flags, and this has proven to be a pain and considerably more prone to
branch merge conflict (which is one reason why, for example, I hacked a smaller
patch together for the backoff retry last week).  In cases where we've done
something like this struct instead, it has made it very easy to only change code
in the places that actually care about the value (similar to command line
flags).  I'm speaking based on not only my own feelings but feedback I received
from other folks when they had to e.g. add a new counter to SessionState (now
ModelNeutralState).

An alternative is the java-y builder pattern, and while it might make for pretty
c++ I think it's overkill / not as elegant here as java (particularly when
non-const& return values are not permitted by style guide).

[rlarocque, 2012-08-14 21:13:04]

LGTM

http://codereview.chromium.org/10837231/diff/8031/chrome/browser/sync/glue/sy...
File chrome/browser/sync/glue/sync_backend_host.cc (right):

http://codereview.chromium.org/10837231/diff/8031/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/sync_backend_host.cc:424: if
(cl->HasSwitch(switches::kSyncKeystoreEncryption)) {
On 2012/08/14 20:20:51, timsteele wrote:
> On 2012/08/14 19:54:07, rlarocque wrote:
> > nit: I think this would be more readable if this was an if/else branch. 
That
> > would make it more clear that the InternalComponentsFactory flag depends
> > entirely on the command line flag.
> 
> The reason I did it this way is because it is most obvious what the default
> expected behavior is, and we ensure that we always start from the same default
> regardless of how many times the "extraordinary circumstances" below change or
> the code gets re-arranged.
> 
> Maybe it'd be nicer to pull the CommandLine changes into their own routine, to
> make this more clear?  Something like 
> 
> Switches factory_switches = {ENCRYPTION_LEGACY, BACKOFF_NORMAL};
> OverrideSwitchesIfNecessary(&switches, CommandLine::ForCurrentProcess());

I'd rather not hide the processing inside a function.  

I don't feel that strongly about my recommendation.  It's good enough as is.

http://codereview.chromium.org/10837231/diff/8031/sync/internal_api/public/in...
File sync/internal_api/public/internal_components_factory.h (right):

http://codereview.chromium.org/10837231/diff/8031/sync/internal_api/public/in...
sync/internal_api/public/internal_components_factory.h:59: struct Switches {
On 2012/08/14 20:20:51, timsteele wrote:
> On 2012/08/14 19:54:07, rlarocque wrote:
> > I'd prefer to see this struct's members embedded directly in the factory. 
It
> > would be simpler to pass these flags directly to the factory's constructor,
> > rather than constructing this object then passing it to the factory's
> > constructor. 
> 
> That's exactly what I wanted to avoid.  In the past we've had to change
> constructors in interfaces, in real impls, in fake impls, etc for config or
> transient flags, and this has proven to be a pain and considerably more prone
to
> branch merge conflict (which is one reason why, for example, I hacked a
smaller
> patch together for the backoff retry last week).  In cases where we've done
> something like this struct instead, it has made it very easy to only change
code
> in the places that actually care about the value (similar to command line
> flags).  I'm speaking based on not only my own feelings but feedback I
received
> from other folks when they had to e.g. add a new counter to SessionState (now
> ModelNeutralState).
> 
> An alternative is the java-y builder pattern, and while it might make for
pretty
> c++ I think it's overkill / not as elegant here as java (particularly when
> non-const& return values are not permitted by style guide).

I could write a long rant about why this is wrong.  I dislike sacrificing code
cleanliness in favor of less typing.  

However, you're probably right that this will be more maintainable.  If you're
going to evaluate the code according to that criteria, then this is the right
decision.

[tim (not reviewing), 2012-08-15 00:53:20]

On 2012/08/14 21:13:04, rlarocque wrote:
> LGTM
> 
>
http://codereview.chromium.org/10837231/diff/8031/chrome/browser/sync/glue/sy...
> File chrome/browser/sync/glue/sync_backend_host.cc (right):
> 
>
http://codereview.chromium.org/10837231/diff/8031/chrome/browser/sync/glue/sy...
> chrome/browser/sync/glue/sync_backend_host.cc:424: if
> (cl->HasSwitch(switches::kSyncKeystoreEncryption)) {
> On 2012/08/14 20:20:51, timsteele wrote:
> > On 2012/08/14 19:54:07, rlarocque wrote:
> > > nit: I think this would be more readable if this was an if/else branch. 
> That
> > > would make it more clear that the InternalComponentsFactory flag depends
> > > entirely on the command line flag.
> > 
> > The reason I did it this way is because it is most obvious what the default
> > expected behavior is, and we ensure that we always start from the same
default
> > regardless of how many times the "extraordinary circumstances" below change
or
> > the code gets re-arranged.
> > 
> > Maybe it'd be nicer to pull the CommandLine changes into their own routine,
to
> > make this more clear?  Something like 
> > 
> > Switches factory_switches = {ENCRYPTION_LEGACY, BACKOFF_NORMAL};
> > OverrideSwitchesIfNecessary(&switches, CommandLine::ForCurrentProcess());
> 
> I'd rather not hide the processing inside a function.  
> 
> I don't feel that strongly about my recommendation.  It's good enough as is.
> 
>
http://codereview.chromium.org/10837231/diff/8031/sync/internal_api/public/in...
> File sync/internal_api/public/internal_components_factory.h (right):
> 
>
http://codereview.chromium.org/10837231/diff/8031/sync/internal_api/public/in...
> sync/internal_api/public/internal_components_factory.h:59: struct Switches {
> On 2012/08/14 20:20:51, timsteele wrote:
> > On 2012/08/14 19:54:07, rlarocque wrote:
> > > I'd prefer to see this struct's members embedded directly in the factory. 
> It
> > > would be simpler to pass these flags directly to the factory's
constructor,
> > > rather than constructing this object then passing it to the factory's
> > > constructor. 
> > 
> > That's exactly what I wanted to avoid.  In the past we've had to change
> > constructors in interfaces, in real impls, in fake impls, etc for config or
> > transient flags, and this has proven to be a pain and considerably more
prone
> to
> > branch merge conflict (which is one reason why, for example, I hacked a
> smaller
> > patch together for the backoff retry last week).  In cases where we've done
> > something like this struct instead, it has made it very easy to only change
> code
> > in the places that actually care about the value (similar to command line
> > flags).  I'm speaking based on not only my own feelings but feedback I
> received
> > from other folks when they had to e.g. add a new counter to SessionState
(now
> > ModelNeutralState).
> > 
> > An alternative is the java-y builder pattern, and while it might make for
> pretty
> > c++ I think it's overkill / not as elegant here as java (particularly when
> > non-const& return values are not permitted by style guide).
> 
> I could write a long rant about why this is wrong.  I dislike sacrificing code
> cleanliness in favor of less typing.  
> 
> However, you're probably right that this will be more maintainable.  If you're
> going to evaluate the code according to that criteria, then this is the right
> decision.

OK.  I'm going to land this for now, but lets chat tomorrow maybe over lunch,
I'm definitely interested in hearing an abridged version of the rant!

[commit-bot: I haz the power, 2012-08-15 00:53:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tim@chromium.org/10837231/8031

[commit-bot: I haz the power, 2012-08-15 00:54:00]

Presubmit check for 10837231-8031 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome

Presubmit checks took 2.2s to calculate.

[commit-bot: I haz the power, 2012-08-15 01:03:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tim@chromium.org/10837231/8031

[commit-bot: I haz the power, 2012-08-15 06:18:38]

Change committed as 151664