Invoke the StartSyncFlare for prefs when performing an automatic reset.

Invoke the StartSyncFlare for prefs when performing an automatic reset.

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

[gab, 2014-07-16 20:04:58]

Nicolas PTAL, as discussed in sync/protector syncup before break.

Erik PTAL at PrefHashFilter changes

CC rlarocque as FYI (one more DEPS on sync/internal_api/public/base/model_type.h
-- shouldn't make it harder to move/fix all other places depending on this
header already)

[erikwright (departed), 2014-07-16 20:23:43]

https://codereview.chromium.org/398893002/diff/20001/chrome/browser/prefs/pro...
File chrome/browser/prefs/profile_pref_store_manager.cc (right):

https://codereview.chromium.org/398893002/diff/20001/chrome/browser/prefs/pro...
chrome/browser/prefs/profile_pref_store_manager.cc:120: const base::Closure
start_sync_flare_for_prefs =
This should be a dependency passed in by chrome_pref_service_factory, not a
static access in this class. That will be much more testable.

[gab, 2014-07-16 20:30:40]

https://codereview.chromium.org/398893002/diff/20001/chrome/browser/prefs/pro...
File chrome/browser/prefs/profile_pref_store_manager.cc (right):

https://codereview.chromium.org/398893002/diff/20001/chrome/browser/prefs/pro...
chrome/browser/prefs/profile_pref_store_manager.cc:120: const base::Closure
start_sync_flare_for_prefs =
On 2014/07/16 20:23:43, erikwright wrote:
> This should be a dependency passed in by chrome_pref_service_factory, not a
> static access in this class. That will be much more testable.

How is it more testable? I thought about it, but I'm not sure that another layer
of piping gives us anything here... The |on_reset| callback is already tested in
pref_hash_filter_unittest.cc; testing that the flare itself is called would
likely require a huge test (involving services et al.) and I'm not convinced
that's truly required for such a simple hook.

[erikwright (departed), 2014-07-16 20:51:39]

Right, you don't test that the flare is called. If you choose to, you may
test that ProfilePrefStoreManager does correctly wire up a callback and
that it is invoked in the right circumstances.

Either way, you eliminate a dependency on a profile-keyed service from code
that is otherwise completely unit-testable with no static dependencies.
Your current patchset
requires sync_start_util::GetFlareForSyncableService() to be safe to invoke
and for the returned closure to have no problematic side-effects.

As it happens, the returned callback requires a UI thread to exist and will
call StartSyncOnUIThread
<https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/syn...>,
which does all kinds of things that may or may not crash without proper
initialization and may or may not spawn follow-up tasks that could cause
unknown problems if they execute (or if they don't).

By making this an external dependency instead of a static one, you can
ignore those problems in your unit test, and test _only_ the purpose of
ProfilePrefStoreManager, which is to wire up all of the stuff related to
the pref stores. That can include wiring up a callback in the right place,
but save yourself the trouble of testing what that callback actually does;
presumably GetFlareForSyncableService is already tested elsewhere.


On Wed, Jul 16, 2014 at 4:30 PM, <gab@chromium.org> wrote:

>
> https://codereview.chromium.org/398893002/diff/20001/
> chrome/browser/prefs/profile_pref_store_manager.cc
> File chrome/browser/prefs/profile_pref_store_manager.cc (right):
>
> https://codereview.chromium.org/398893002/diff/20001/
> chrome/browser/prefs/profile_pref_store_manager.cc#newcode120
> chrome/browser/prefs/profile_pref_store_manager.cc:120: const
> base::Closure start_sync_flare_for_prefs =
> On 2014/07/16 20:23:43, erikwright wrote:
>
>> This should be a dependency passed in by chrome_pref_service_factory,
>>
> not a
>
>> static access in this class. That will be much more testable.
>>
>
> How is it more testable? I thought about it, but I'm not sure that
> another layer of piping gives us anything here... The |on_reset|
> callback is already tested in pref_hash_filter_unittest.cc; testing that
> the flare itself is called would likely require a huge test (involving
> services et al.) and I'm not convinced that's truly required for such a
> simple hook.
>
> https://codereview.chromium.org/398893002/
>

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

[gab, 2014-07-18 21:02:36]

Done, I'm not sure I prefer this as it now makes another layer explicitly aware
of resets (previously it was aware of "protected preferences", but not really of
the protection mechanism... plus it's just more piping...

But I agree that it's nice to keep ProfilePrefStoreManager pure of external
dependencies...

PTAL.

Thanks!
Gab

[Nicolas Zea, 2014-07-18 21:11:16]

LGTM with some comments/nits

https://codereview.chromium.org/398893002/diff/60001/chrome/browser/prefs/DEPS
File chrome/browser/prefs/DEPS (right):

https://codereview.chromium.org/398893002/diff/60001/chrome/browser/prefs/DEP...
chrome/browser/prefs/DEPS:2: #TODO(rlarocque): Move this dependency into the
invalidation component.
This comment doesn't make sense here. I think it's fine to allow depending on
model type.

https://codereview.chromium.org/398893002/diff/60001/chrome/browser/prefs/DEP...
chrome/browser/prefs/DEPS:4: "+sync/internal_api/public/base",
if you're just depending on model_type.h, may as well explicitly do so. I.e.:
"+sync/internal_api/public/base/model_type.h"

https://codereview.chromium.org/398893002/diff/60001/chrome/browser/prefs/pre...
File chrome/browser/prefs/pref_hash_filter.h (right):

https://codereview.chromium.org/398893002/diff/60001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_filter.h:64: // If |on_reset| is provided, it
will be invoked if a reset occurs in
nit: might be better to call it something like reset_on_load_callback or
something, to make it clear it's only invoked on load as part of the name.

[gab, 2014-07-21 16:22:04]

Thanks, Bernhard PTAL at the prefs changes.

Cheers,
Gab

https://codereview.chromium.org/398893002/diff/60001/chrome/browser/prefs/DEPS
File chrome/browser/prefs/DEPS (right):

https://codereview.chromium.org/398893002/diff/60001/chrome/browser/prefs/DEP...
chrome/browser/prefs/DEPS:2: #TODO(rlarocque): Move this dependency into the
invalidation component.
On 2014/07/18 21:11:16, Nicolas Zea wrote:
> This comment doesn't make sense here. I think it's fine to allow depending on
> model type.

Done.

https://codereview.chromium.org/398893002/diff/60001/chrome/browser/prefs/DEP...
chrome/browser/prefs/DEPS:4: "+sync/internal_api/public/base",
On 2014/07/18 21:11:16, Nicolas Zea wrote:
> if you're just depending on model_type.h, may as well explicitly do so. I.e.:
> "+sync/internal_api/public/base/model_type.h"

Done.

https://codereview.chromium.org/398893002/diff/60001/chrome/browser/prefs/pre...
File chrome/browser/prefs/pref_hash_filter.h (right):

https://codereview.chromium.org/398893002/diff/60001/chrome/browser/prefs/pre...
chrome/browser/prefs/pref_hash_filter.h:64: // If |on_reset| is provided, it
will be invoked if a reset occurs in
On 2014/07/18 21:11:16, Nicolas Zea wrote:
> nit: might be better to call it something like reset_on_load_callback or
> something, to make it clear it's only invoked on load as part of the name.

Renamed to |on_reset_on_load|

[Bernhard Bauer, 2014-07-21 17:22:27]

LGTM

[gab, 2014-07-21 17:25:03]

The CQ bit was checked by gab@chromium.org

[commit-bot: I haz the power, 2014-07-21 17:26:30]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/gab@chromium.org/398893002/80001

[commit-bot: I haz the power, 2014-07-21 21:47:24]

Change committed as 284507