Extract abstract base to API directory for ProfileSyncService.

Extract abstract base to API directory for ProfileSyncService.

Move ProfileSyncServiceObserver to API directory.

Use these to break Autofill's concrete dependency on ProfileSyncService.

Remove last usages of Profile in Autofill code (modulo usage in one file that I expect will be moved to chrome/browser/configuration).

TBR=ben@chromium.org,mechanicalowners@chromium.org
BUG=140037
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=157563

[Jói, 2012-09-04 00:58:02]

erikwright: Review everything, please.

tim: Please review the extracted interface and other refactorings to sync. Right
now this is just based on what Autofill is using, not on any wider-reaching
analysis of what bits of ProfileSyncService feature code actually needs to use. 
We can develop the interface over time, or we can try to add bits you know will
be needed now.

Cheers,
Jói

[erikwright (departed), 2012-09-04 19:22:11]

Why ProfileSyncServiceBase?

Seems that the client doesn't care that it's associated with a profile. So it
should just be SyncServiceBase. In which case, can we just name it SyncService?

http://codereview.chromium.org/10910071/diff/2001/chrome/browser/api/sync/pro...
File chrome/browser/api/sync/profile_sync_service_base.h (right):

http://codereview.chromium.org/10910071/diff/2001/chrome/browser/api/sync/pro...
chrome/browser/api/sync/profile_sync_service_base.h:38: // Adds/removes an
observer. ProfileSyncService does not take ownership of
ProfileSyncServiceBase

[tim (not reviewing), 2012-09-04 19:31:10]

On 2012/09/04 19:22:11, erikwright wrote:
> Why ProfileSyncServiceBase?
> 
> Seems that the client doesn't care that it's associated with a profile. So it
> should just be SyncServiceBase. In which case, can we just name it
SyncService?
> 
>
http://codereview.chromium.org/10910071/diff/2001/chrome/browser/api/sync/pro...
> File chrome/browser/api/sync/profile_sync_service_base.h (right):
> 
>
http://codereview.chromium.org/10910071/diff/2001/chrome/browser/api/sync/pro...
> chrome/browser/api/sync/profile_sync_service_base.h:38: // Adds/removes an
> observer. ProfileSyncService does not take ownership of
> ProfileSyncServiceBase

Can you provide more details on the basis for this change?  We are are moving
the core sync engine into a dedicated component, see src/sync. The code in
chrome/browser/sync is intended to be "what the rest of Chrome connects to for
syncing purposes".  Making PSS mostly an interface seems reasonable though, we'd
like to head in that direction to facilitate testing as well.   I may be able to
make better suggestions for naming and/or what methods should be split-off where
if I understand better -- I'd like to avoid splitting sync into multiple
components unless the goals are explicitly different :) I'd also like to avoid
confusion between existing interfaces like "SyncFrontend".

re: naming, the feature is effectively known as "Profile Sync", so that makes
sense to me.  Also, "SyncService" is overly vague -- it won't help the slew of
bugs that already get erroneously tagged as "Feature-Sync" because of the
presence of (e.g.) "Sync" as in "synchronous IPC" in callstacks or comments.

Thanks!

[Jói, 2012-09-04 19:58:53]

Hi Tim,

Thanks for the feedback.  This is part of the Browser Components work,
see http://www.chromium.org/developers/design-documents/browser-components
for background.

The work entails modularizing various features of Chrome (e.g.
Autofill, History, Bookmarks, and so on) and breaking their concrete
dependencies on the rest of Chrome, so that they can more easily be
reused in a different context (e.g. on Android) and more easily be
tested in a focused test harness that provides mock implementations of
dependencies.  We are also rationalizing the DEPS rules for these
components to better enforce the intended architecture.

One of the ways we break concrete dependencies is by wrapping services
provided by the browser in more abstract, API-like interfaces.
ProfileSyncServiceBase is an example of one of these.  A side benefit
of creating these APIs is that it becomes easier to understand how to
use various parts of Chrome, since you can start by just looking at
the API.

Therefore, the goal would be that ProfileSyncServiceBase should have
the functionality commonly used by "normal feature code" in Chrome.
There will always be "glue" code that configures and initializes
things and can access the concrete implementation of
ProfileSyncService., so the API doesn't need to have everything you
need to set up and configure the service, just the bits that are
commonly used.

Hope that makes sense.  There is no intent to split the service up at
all, just to extract an abstract API.

Cheers,
Jói



On Tue, Sep 4, 2012 at 12:31 PM,  <tim@chromium.org> wrote:
> On 2012/09/04 19:22:11, erikwright wrote:
>>
>> Why ProfileSyncServiceBase?
>
>
>> Seems that the client doesn't care that it's associated with a profile. So
>> it
>> should just be SyncServiceBase. In which case, can we just name it
>
> SyncService?
>
>
>
http://codereview.chromium.org/10910071/diff/2001/chrome/browser/api/sync/pro...
>>
>> File chrome/browser/api/sync/profile_sync_service_base.h (right):
>
>
>
>
http://codereview.chromium.org/10910071/diff/2001/chrome/browser/api/sync/pro...
>>
>> chrome/browser/api/sync/profile_sync_service_base.h:38: // Adds/removes an
>> observer. ProfileSyncService does not take ownership of
>> ProfileSyncServiceBase
>
>
> Can you provide more details on the basis for this change?  We are are
> moving
> the core sync engine into a dedicated component, see src/sync. The code in
> chrome/browser/sync is intended to be "what the rest of Chrome connects to
> for
> syncing purposes".  Making PSS mostly an interface seems reasonable though,
> we'd
> like to head in that direction to facilitate testing as well.   I may be
> able to
> make better suggestions for naming and/or what methods should be split-off
> where
> if I understand better -- I'd like to avoid splitting sync into multiple
> components unless the goals are explicitly different :) I'd also like to
> avoid
> confusion between existing interfaces like "SyncFrontend".
>
> re: naming, the feature is effectively known as "Profile Sync", so that
> makes
> sense to me.  Also, "SyncService" is overly vague -- it won't help the slew
> of
> bugs that already get erroneously tagged as "Feature-Sync" because of the
> presence of (e.g.) "Sync" as in "synchronous IPC" in callstacks or comments.
>
> Thanks!
>
>
> http://codereview.chromium.org/10910071/

[tim (not reviewing), 2012-09-06 17:00:05]

http://codereview.chromium.org/10910071/diff/8001/chrome/browser/api/DEPS
File chrome/browser/api/DEPS (right):

http://codereview.chromium.org/10910071/diff/8001/chrome/browser/api/DEPS#new...
chrome/browser/api/DEPS:31: "+sync/internal_api/public/base/model_type.h",
Hm, so previously, the only thing that could depend on sync/internal_api/public
was stuff in chrome/browser/sync.  Can we only allow this for the sync/ subdir? 
And if so, make it sync/internal_api/public/*?

http://codereview.chromium.org/10910071/diff/8001/chrome/browser/api/sync/pro...
File chrome/browser/api/sync/profile_sync_service_base.h (right):

http://codereview.chromium.org/10910071/diff/8001/chrome/browser/api/sync/pro...
chrome/browser/api/sync/profile_sync_service_base.h:15: // Abstract base for
ProfileSyncService.
This is intended to remain a pure interface definition, right?  And not actually
be abstract / contain code?

http://codereview.chromium.org/10910071/diff/8001/chrome/browser/api/sync/pro...
chrome/browser/api/sync/profile_sync_service_base.h:16: class
ProfileSyncServiceBase {
What's the plan with OWNERS for this file?  As long as only pure virtual methods
are added, I guess it's not a big deal since we'll need to approve the
corresponding chrome/browser/sync change.

http://codereview.chromium.org/10910071/diff/8001/chrome/browser/api/sync/pro...
chrome/browser/api/sync/profile_sync_service_base.h:34: // the user).  See class
comment for more on what it means for a datatype
class comment on ProfileSyncService...

http://codereview.chromium.org/10910071/diff/8001/chrome/browser/sync/profile...
File chrome/browser/sync/profile_sync_service.h (right):

http://codereview.chromium.org/10910071/diff/8001/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service.h:231: virtual bool
HasObserver(Observer* observer) const OVERRIDE;
If we describe ProfileSyncServiceBase as "what normal feature code" should use,
there are more pieces we need to include in that API.  On one hand I feel it's
better to only add as necessary, but on the other hand I can't reason about what
PSSBase actually _is_ without seeing the full picture of what it claims to be in
the methods it exposes.  But it seems like it's a work in progress, so for now
it's likely better to not add extra pieces.

http://codereview.chromium.org/10910071/diff/8001/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service.h:266: virtual void
SetSyncSetupCompleted();
It's weird that this is so asymmetric now. Just double checking, but "browser
UI" doesn't qualify as "common feature code" as per your earlier comment on this
review, right?  If the goal was to have browser UI depend on browser/api
(instead of things like browser/sync/*) we'd really need all of
ProfileSyncService.

[Jói, 2012-09-06 20:08:34]

Thanks Erik and Tim.  I uploaded a new snapshot that addresses your review
comments.

Cheers,
Jói

http://codereview.chromium.org/10910071/diff/2001/chrome/browser/api/sync/pro...
File chrome/browser/api/sync/profile_sync_service_base.h (right):

http://codereview.chromium.org/10910071/diff/2001/chrome/browser/api/sync/pro...
chrome/browser/api/sync/profile_sync_service_base.h:38: // Adds/removes an
observer. ProfileSyncService does not take ownership of
On 2012/09/04 19:22:11, erikwright wrote:
> ProfileSyncServiceBase

Done.

http://codereview.chromium.org/10910071/diff/8001/chrome/browser/api/DEPS
File chrome/browser/api/DEPS (right):

http://codereview.chromium.org/10910071/diff/8001/chrome/browser/api/DEPS#new...
chrome/browser/api/DEPS:31: "+sync/internal_api/public/base/model_type.h",
On 2012/09/06 17:00:06, timsteele wrote:
> Hm, so previously, the only thing that could depend on
sync/internal_api/public
> was stuff in chrome/browser/sync.  Can we only allow this for the sync/
subdir? 
> And if so, make it sync/internal_api/public/*?

Done.

http://codereview.chromium.org/10910071/diff/8001/chrome/browser/api/sync/pro...
File chrome/browser/api/sync/profile_sync_service_base.h (right):

http://codereview.chromium.org/10910071/diff/8001/chrome/browser/api/sync/pro...
chrome/browser/api/sync/profile_sync_service_base.h:15: // Abstract base for
ProfileSyncService.
On 2012/09/06 17:00:06, timsteele wrote:
> This is intended to remain a pure interface definition, right?  And not
actually
> be abstract / contain code?

Correct.  Changed the comment to just "API for ProfileSyncService."

http://codereview.chromium.org/10910071/diff/8001/chrome/browser/api/sync/pro...
chrome/browser/api/sync/profile_sync_service_base.h:16: class
ProfileSyncServiceBase {
On 2012/09/06 17:00:06, timsteele wrote:
> What's the plan with OWNERS for this file?  As long as only pure virtual
methods
> are added, I guess it's not a big deal since we'll need to approve the
> corresponding chrome/browser/sync change.

Whoops, copied the OWNERS file from chrome/browser/sync, that is what we
generally do for subdirs of api.

http://codereview.chromium.org/10910071/diff/8001/chrome/browser/api/sync/pro...
chrome/browser/api/sync/profile_sync_service_base.h:34: // the user).  See class
comment for more on what it means for a datatype
On 2012/09/06 17:00:06, timsteele wrote:
> class comment on ProfileSyncService...

Done.

http://codereview.chromium.org/10910071/diff/8001/chrome/browser/sync/profile...
File chrome/browser/sync/profile_sync_service.h (right):

http://codereview.chromium.org/10910071/diff/8001/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service.h:231: virtual bool
HasObserver(Observer* observer) const OVERRIDE;
On 2012/09/06 17:00:06, timsteele wrote:
> If we describe ProfileSyncServiceBase as "what normal feature code" should
use,
> there are more pieces we need to include in that API.  On one hand I feel it's
> better to only add as necessary, but on the other hand I can't reason about
what
> PSSBase actually _is_ without seeing the full picture of what it claims to be
in
> the methods it exposes.  But it seems like it's a work in progress, so for now
> it's likely better to not add extra pieces.

OK.  I agree it's probably best to evolve it as we start using it for other
(non-UI) components.

http://codereview.chromium.org/10910071/diff/8001/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service.h:266: virtual void
SetSyncSetupCompleted();
On 2012/09/06 17:00:06, timsteele wrote:
> It's weird that this is so asymmetric now. Just double checking, but "browser
> UI" doesn't qualify as "common feature code" as per your earlier comment on
this
> review, right?  If the goal was to have browser UI depend on browser/api
> (instead of things like browser/sync/*) we'd really need all of
> ProfileSyncService.

Correct, the APIs are intended for "business logic" type components, whereas UI
in general is allowed to depend concretely on everything.  This may of course
evolve over time but for now we should consider the APIs as what's needed for
the business logic of feature code.  Instantiation, set-up and configuration,
that require a larger interface surface, would either happen under
chrome/browser/configuration (which is a directory for that kind of glue code
that we are establishing) or if UI-specific, under chrome/browser/ui somewhere,
and both of those could use the full concrete implementation directly.

I moved SetSyncSetupCompleted to be right before HasSyncSetupCompleted, i.e.
right after the ProfileSyncServiceBase methods, so it's easier to see that the
concrete class has both the setter and getter.

[tim (not reviewing), 2012-09-06 20:48:26]

sync bits LGTM, thanks!

[Jói, 2012-09-11 19:13:54]

Erik: LGTY?

[erikwright (departed), 2012-09-11 19:29:52]

LGTM.

[commit-bot: I haz the power, 2012-09-14 10:57:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joi@chromium.org/10910071/12003

[commit-bot: I haz the power, 2012-09-14 11:03:04]

Retried try job too often for step(s) crypto_unittests, unit_tests,
cacheinvalidation_unittests, remoting_unittests, jingle_unittests,
nacl_integration, ipc_tests, interactive_ui_tests, browser_tests, net_unittests,
content_unittests, gpu_unittests, base_unittests, sync_integration_tests,
media_unittests, safe_browsing_tests, content_browsertests, sql_unittests,
printing_unittests, sync_unit_tests, check_deps

[commit-bot: I haz the power, 2012-09-18 11:40:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joi@chromium.org/10910071/16001

[commit-bot: I haz the power, 2012-09-18 12:33:33]

Retried try job too often for step(s) unit_tests

[commit-bot: I haz the power, 2012-09-19 16:58:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joi@chromium.org/10910071/16016

[commit-bot: I haz the power, 2012-09-19 17:46:12]

Change committed as 157563