Remove knowledge of Chrome-specific SyncableService classes from WebDataService.

Remove knowledge of Chrome-specific SyncableService classes from WebDataService.

TBR=tim@chromium.org
BUG=181277
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=189474

[Jói, 2013-03-18 17:46:40]

caitkp: Main reviewer, for an early look, this is not quite ready.

rsleevi: Because this reverses the change you made in r144130, titled
"RefCounted types should not have public destructors, base/ edition". This is
not actually a refcounted class, and I think it can be convenient to be able to
use it stand-alone rather than inheriting from it. Let me know what you think.

Depends on https://codereview.chromium.org/12780012/

Cheers,
Jói

[Ryan Sleevi, 2013-03-18 18:41:48]

Joi:

The context was that there were (are? I'd need to check) a number of classes
that were ref-counted and derived from SupportsUserData, even though
SupportsUserData was not itself ref-counted.

The goal of making the dtor protected-virtual was to ensure that no one deleted
a class via a SupportsUserData* interface, but through the base type pointer
appropriate for that class (which may be a RefCounted type)

If it's supposed to be possible to delete a SupportsUserData*, then the
counter-design would be that no refcounted classes can publicly implemented
SupportsUserData.

The goal of either design is to make it a hard compile failure, rather than
allowing the subtle bugs to creep in (of which there were a number of, in the
past, of precariously balanced deleters that made a variety of assumptions they
shouldn't have)

[Jói, 2013-03-18 19:18:54]

OK, makes sense for this and other classes that are designed to be
inherited.  In this case I'll probably just locally subclass it to
expose the destructor, since I want to aggregate it.

Cheers,
Jói


On Mon, Mar 18, 2013 at 6:41 PM,  <rsleevi@chromium.org> wrote:
> Joi:
>
> The context was that there were (are? I'd need to check) a number of classes
> that were ref-counted and derived from SupportsUserData, even though
> SupportsUserData was not itself ref-counted.
>
> The goal of making the dtor protected-virtual was to ensure that no one
> deleted
> a class via a SupportsUserData* interface, but through the base type pointer
> appropriate for that class (which may be a RefCounted type)
>
> If it's supposed to be possible to delete a SupportsUserData*, then the
> counter-design would be that no refcounted classes can publicly implemented
> SupportsUserData.
>
> The goal of either design is to make it a hard compile failure, rather than
> allowing the subtle bugs to creep in (of which there were a number of, in
> the
> past, of precariously balanced deleters that made a variety of assumptions
> they
> shouldn't have)
>
> https://codereview.chromium.org/12494020/

[Ilya Sherman, 2013-03-18 21:24:47]

https://codereview.chromium.org/12494020/diff/1/chrome/browser/webdata/autoco...
File chrome/browser/webdata/autocomplete_syncable_service.h (right):

https://codereview.chromium.org/12494020/diff/1/chrome/browser/webdata/autoco...
chrome/browser/webdata/autocomplete_syncable_service.h:55: WebDataServiceBase*
web_data);
Why not follow the pattern used elsewhere in the Autofill code, of inheriting
from base::SupportsUserData::Data, exposing CreateForWebDataService() instead of
AttachToWebData(), and hiding the constructor and destructor?

[Jói, 2013-03-18 22:11:34]

https://codereview.chromium.org/12494020/diff/1/chrome/browser/webdata/autoco...
File chrome/browser/webdata/autocomplete_syncable_service.h (right):

https://codereview.chromium.org/12494020/diff/1/chrome/browser/webdata/autoco...
chrome/browser/webdata/autocomplete_syncable_service.h:55: WebDataServiceBase*
web_data);
On 2013/03/18 21:24:48, Ilya Sherman wrote:
> Why not follow the pattern used elsewhere in the Autofill code, of inheriting
> from base::SupportsUserData::Data, exposing CreateForWebDataService() instead
of
> AttachToWebData(), and hiding the constructor and destructor?

I was aware of the pattern but didn't think to apply it this time; I think I had
pigeonholed it as a pattern for things hanging off of WebContents. Makes perfect
sense, will switch to that.

[Jói, 2013-03-18 23:46:19]

I've addressed rsleevi@ and isherman@'s suggestions. Adding reviewers:

dhollowa: OWNERS approval for chrome/browser/webdata.

tim: OWNERS approval for chrome/browser/sync; the changes to production code are
trivial, the changes to unit tests slightly less so.

Cheers,
Jói

[Ilya Sherman, 2013-03-18 23:54:24]

Erm, meant to say "thanks", and remove myself from the list of reviewers (my
comment was just a drive-by nit).

[Cait (Slow), 2013-03-19 19:49:46]

https://codereview.chromium.org/12494020/diff/9001/chrome/browser/webdata/aut...
File chrome/browser/webdata/autofill_profile_syncable_service.cc (right):

https://codereview.chromium.org/12494020/diff/9001/chrome/browser/webdata/aut...
chrome/browser/webdata/autofill_profile_syncable_service.cc:62: WebDataService*
web_data) {
It seems like the syncable services should be hung off of AutofillWebData or
WebDataServiceBase rather than WebDataService. Their purpose is to listen for
Autofill changes and sync them, and presumably, once we split the webdatas,
notifications of these changes will come from AutofillWebDatas rather than
WebDataService objects... wdyt?

[Cait (Slow), 2013-03-19 20:24:24]

https://codereview.chromium.org/12494020/diff/9001/chrome/browser/webdata/aut...
File chrome/browser/webdata/autofill_profile_syncable_service.cc (right):

https://codereview.chromium.org/12494020/diff/9001/chrome/browser/webdata/aut...
chrome/browser/webdata/autofill_profile_syncable_service.cc:62: WebDataService*
web_data) {
On 2013/03/19 19:49:46, caitkp wrote:
> It seems like the syncable services should be hung off of AutofillWebData or
> WebDataServiceBase rather than WebDataService. Their purpose is to listen for
> Autofill changes and sync them, and presumably, once we split the webdatas,
> notifications of these changes will come from AutofillWebDatas rather than
> WebDataService objects... wdyt?

Just saw your TODOs on this -- I'll work on converting these to be keyed off
AutofillWebData now.

[Jói, 2013-03-19 20:27:15]

SGTM, thanks.

On Tue, Mar 19, 2013 at 8:24 PM,  <caitkp@chromium.org> wrote:
>
>
https://codereview.chromium.org/12494020/diff/9001/chrome/browser/webdata/aut...
> File chrome/browser/webdata/autofill_profile_syncable_service.cc
> (right):
>
>
https://codereview.chromium.org/12494020/diff/9001/chrome/browser/webdata/aut...
> chrome/browser/webdata/autofill_profile_syncable_service.cc:62:
> WebDataService* web_data) {
> On 2013/03/19 19:49:46, caitkp wrote:
>>
>> It seems like the syncable services should be hung off of
>
> AutofillWebData or
>>
>> WebDataServiceBase rather than WebDataService. Their purpose is to
>
> listen for
>>
>> Autofill changes and sync them, and presumably, once we split the
>
> webdatas,
>>
>> notifications of these changes will come from AutofillWebDatas rather
>
> than
>>
>> WebDataService objects... wdyt?
>
>
> Just saw your TODOs on this -- I'll work on converting these to be keyed
> off AutofillWebData now.
>
> https://codereview.chromium.org/12494020/

[dhollowa, 2013-03-20 21:47:37]

LGTM w/ nits.  chrome/browser/webdata/...

https://codereview.chromium.org/12494020/diff/20001/chrome/browser/webdata/au...
File chrome/browser/webdata/autocomplete_syncable_service.cc (right):

https://codereview.chromium.org/12494020/diff/20001/chrome/browser/webdata/au...
chrome/browser/webdata/autocomplete_syncable_service.cc:83: int user_data_key =
0;
nit: this could be pulled into the body of UserDataKey() as a static.  Principle
of narrowest scope...

https://codereview.chromium.org/12494020/diff/20001/chrome/browser/webdata/au...
File chrome/browser/webdata/autofill_profile_syncable_service.cc (right):

https://codereview.chromium.org/12494020/diff/20001/chrome/browser/webdata/au...
chrome/browser/webdata/autofill_profile_syncable_service.cc:37: int
user_data_key = 0;
nit: Pull inside UserDataKey() as static.

https://codereview.chromium.org/12494020/diff/20001/chrome/browser/webdata/we...
File chrome/browser/webdata/web_data_service.cc (right):

https://codereview.chromium.org/12494020/diff/20001/chrome/browser/webdata/we...
chrome/browser/webdata/web_data_service.cc:7: #include "base/stl_util.h"
Why was this added?

[Jói, 2013-03-20 22:03:15]

https://codereview.chromium.org/12494020/diff/20001/chrome/browser/webdata/au...
File chrome/browser/webdata/autocomplete_syncable_service.cc (right):

https://codereview.chromium.org/12494020/diff/20001/chrome/browser/webdata/au...
chrome/browser/webdata/autocomplete_syncable_service.cc:83: int user_data_key =
0;
On 2013/03/20 21:47:37, dhollowa wrote:
> nit: this could be pulled into the body of UserDataKey() as a static. 
Principle
> of narrowest scope...

Done. I've also added it to my TODO list to update a few instances of the same
pattern in the codebase.

https://codereview.chromium.org/12494020/diff/20001/chrome/browser/webdata/au...
File chrome/browser/webdata/autofill_profile_syncable_service.cc (right):

https://codereview.chromium.org/12494020/diff/20001/chrome/browser/webdata/au...
chrome/browser/webdata/autofill_profile_syncable_service.cc:37: int
user_data_key = 0;
On 2013/03/20 21:47:37, dhollowa wrote:
> nit: Pull inside UserDataKey() as static.

Done.

https://codereview.chromium.org/12494020/diff/20001/chrome/browser/webdata/we...
File chrome/browser/webdata/web_data_service.cc (right):

https://codereview.chromium.org/12494020/diff/20001/chrome/browser/webdata/we...
chrome/browser/webdata/web_data_service.cc:7: #include "base/stl_util.h"
On 2013/03/20 21:47:37, dhollowa wrote:
> Why was this added?

It became necessary when the two syncable service headers were removed.  Without
it we get this error in a couple of places:

error: 'STLDeleteElements' was not declared in this scope

[Jói, 2013-03-20 23:46:12]

TBR=tim@chromium.org for chrome/browser/sync changes. I thought I had added you
as a reviewer yesterday, but it turns out I didn't. Will TBR and address any
comments you might have in a follow-up change. The two files of production code
have trivial usage updates, whereas the two unit test files have slightly bigger
but still straight-forward updates, again to reflect usage changes.

The whole change was reviewed by caitkp and has passed tryjobs at
https://codereview.chromium.org/12805007/ (working around [ git try ] issues by
using [ git cl try ] on a patch that rolls up this patch and parent patches,
both of which have now landed).

Cheers,
Jói

[Jói, 2013-03-20 23:51:14]

Committed patchset #8 manually as r189474 (presubmit successful).

[tim (not reviewing), 2013-03-21 23:39:01]

sync/ LGTM