[Sync] Rename classes in sync/ that start with Chrome

[Sync] Rename classes in sync/ that start with Chrome

There should be as few references as possible to Chrome from sync/.

BUG=


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

[akalin, 2012-09-05 00:29:30]

+rlarocque for review

[rlarocque, 2012-09-05 00:58:31]

I'm not sure about some of the names.

ChromeInvalidationClient -> SyncInvalidationClient makes sense if we're planning
to make all receivers of notifications have their own implementation of
InvalidationClient.  That's covered as part of the current plan to expose
invalidations to non-sync chrome components, right?

SystemResouces looks like it's an interface that abstracts away
platform-specific tasks.  I'd argue that the current implementation is built on
top of the Chrome "platform", not sync, so ChromeSystemResources would be a
better name.

https://chromiumcodereview.appspot.com/10907070/diff/2001/sync/notifier/sync_...
File sync/notifier/sync_system_resources_unittest.cc (left):

https://chromiumcodereview.appspot.com/10907070/diff/2001/sync/notifier/sync_...
sync/notifier/sync_system_resources_unittest.cc:52: : chrome_system_resources_(
Missed one?

https://chromiumcodereview.appspot.com/10907070/diff/2001/tools/valgrind/memc...
File tools/valgrind/memcheck/suppressions.txt (right):

https://chromiumcodereview.appspot.com/10907070/diff/2001/tools/valgrind/memc...
tools/valgrind/memcheck/suppressions.txt:5290:
fun:_ZN13sync_notifier24SyncInvalidationClient5StartERKSsS2_S2_RKSt3mapIN8syncable9ModelTypeElSt4lessIS5_ESaISt4pairIKS5_lEEERKN12browser_sync10WeakHandleINS_24InvalidationStateTrackerEEEPNS0_8ListenerEPNS_11StateWriterE
Good catch.  I would not have thought to look here.

Unfortunately, this might not be enough.  I can successfully decode the old name
using c++filt, but not the new one.  That suggests your change has produced an
invalid mangled name.

Did this name come from the binary itself?  If not, you should check it against
the symbols in your binary, just to be sure.

[akalin, 2012-09-05 20:23:11]

On 2012/09/05 00:58:31, rlarocque wrote:
> I'm not sure about some of the names.
> 
> ChromeInvalidationClient -> SyncInvalidationClient makes sense if we're
planning
> to make all receivers of notifications have their own implementation of
> InvalidationClient.  That's covered as part of the current plan to expose
> invalidations to non-sync chrome components, right?

I'm not sure what you mean by all notification receivers having their own
implementation of InvalidationClient.  ChromeInvalidationClient is meant to be
an internal class.

Anyway, I agree that SyncInvalidationClient is a bad name.  Changed it to
SyncInvalidationListener, to match with invalidation::InvalidationListener,
which it implements.

I also changed its inner Listener class to Delegate, since that's more accurate
anyway.

> SystemResouces looks like it's an interface that abstracts away
> platform-specific tasks.  I'd argue that the current implementation is built
on
> top of the Chrome "platform", not sync, so ChromeSystemResources would be a
> better name.

Well, it's built on top of stuff in 'sync', as well as its dependencies (base,
net, etc.) so saying it's based on the Chrome platform is too general.  I think
we try to avoid layering violations even in names (e.g., net/ stuff doesn't
refer to Chrome, etc.).  Theoretically, one could pull out sync/ and use that
independently from Chrome.

>
https://chromiumcodereview.appspot.com/10907070/diff/2001/sync/notifier/sync_...
> File sync/notifier/sync_system_resources_unittest.cc (left):
> 
>
https://chromiumcodereview.appspot.com/10907070/diff/2001/sync/notifier/sync_...
> sync/notifier/sync_system_resources_unittest.cc:52: :
chrome_system_resources_(
> Missed one?

Fixed.
> 
>
https://chromiumcodereview.appspot.com/10907070/diff/2001/tools/valgrind/memc...
> File tools/valgrind/memcheck/suppressions.txt (right):
> 
>
https://chromiumcodereview.appspot.com/10907070/diff/2001/tools/valgrind/memc...
> tools/valgrind/memcheck/suppressions.txt:5290:
>
fun:_ZN13sync_notifier24SyncInvalidationClient5StartERKSsS2_S2_RKSt3mapIN8syncable9ModelTypeElSt4lessIS5_ESaISt4pairIKS5_lEEERKN12browser_sync10WeakHandleINS_24InvalidationStateTrackerEEEPNS0_8ListenerEPNS_11StateWriterE
> Good catch.  I would not have thought to look here.
> 
> Unfortunately, this might not be enough.  I can successfully decode the old
name
> using c++filt, but not the new one.  That suggests your change has produced an
> invalid mangled name.
> 
> Did this name come from the binary itself?  If not, you should check it
against
> the symbols in your binary, just to be sure.

I think I had to change the prefix lengths, too.  However, coincidentally,
SyncInvalidationListener is also the correct length.

I also corrected the erroneous namespace (sync_notifier instead of syncer).  It
looks like the bug is still open so I didn't remove the suppression entirely.

PTAL.

[rlarocque, 2012-09-06 00:11:32]

On 2012/09/05 20:23:11, akalin wrote:
> On 2012/09/05 00:58:31, rlarocque wrote:
> > I'm not sure about some of the names.
> > 
> > ChromeInvalidationClient -> SyncInvalidationClient makes sense if we're
> planning
> > to make all receivers of notifications have their own implementation of
> > InvalidationClient.  That's covered as part of the current plan to expose
> > invalidations to non-sync chrome components, right?
> 
> I'm not sure what you mean by all notification receivers having their own
> implementation of InvalidationClient.  ChromeInvalidationClient is meant to be
> an internal class.

Sounds good.  I had assumed that the InvalidationClient was going to be part of
the API that allows other components besides sync to receive notifications.

> 
> Anyway, I agree that SyncInvalidationClient is a bad name.  Changed it to
> SyncInvalidationListener, to match with invalidation::InvalidationListener,
> which it implements.
> 
> I also changed its inner Listener class to Delegate, since that's more
accurate
> anyway.
> 
> > SystemResouces looks like it's an interface that abstracts away
> > platform-specific tasks.  I'd argue that the current implementation is built
> on
> > top of the Chrome "platform", not sync, so ChromeSystemResources would be a
> > better name.
> 
> Well, it's built on top of stuff in 'sync', as well as its dependencies (base,
> net, etc.) so saying it's based on the Chrome platform is too general.  I
think
> we try to avoid layering violations even in names (e.g., net/ stuff doesn't
> refer to Chrome, etc.).  Theoretically, one could pull out sync/ and use that
> independently from Chrome.
> 

I see that now.  I was looking at the task-posting and logging implementations,
which were built with components from base/.  The storage implementation is
sync-specific, though.  Your original name makes more sense now.

> >
>
https://chromiumcodereview.appspot.com/10907070/diff/2001/sync/notifier/sync_...
> > File sync/notifier/sync_system_resources_unittest.cc (left):
> > 
> >
>
https://chromiumcodereview.appspot.com/10907070/diff/2001/sync/notifier/sync_...
> > sync/notifier/sync_system_resources_unittest.cc:52: :
> chrome_system_resources_(
> > Missed one?
> 
> Fixed.
> > 
> >
>
https://chromiumcodereview.appspot.com/10907070/diff/2001/tools/valgrind/memc...
> > File tools/valgrind/memcheck/suppressions.txt (right):
> > 
> >
>
https://chromiumcodereview.appspot.com/10907070/diff/2001/tools/valgrind/memc...
> > tools/valgrind/memcheck/suppressions.txt:5290:
> >
>
fun:_ZN13sync_notifier24SyncInvalidationClient5StartERKSsS2_S2_RKSt3mapIN8syncable9ModelTypeElSt4lessIS5_ESaISt4pairIKS5_lEEERKN12browser_sync10WeakHandleINS_24InvalidationStateTrackerEEEPNS0_8ListenerEPNS_11StateWriterE
> > Good catch.  I would not have thought to look here.
> > 
> > Unfortunately, this might not be enough.  I can successfully decode the old
> name
> > using c++filt, but not the new one.  That suggests your change has produced
an
> > invalid mangled name.
> > 
> > Did this name come from the binary itself?  If not, you should check it
> against
> > the symbols in your binary, just to be sure.
> 
> I think I had to change the prefix lengths, too.  However, coincidentally,
> SyncInvalidationListener is also the correct length.
> 
> I also corrected the erroneous namespace (sync_notifier instead of syncer). 
It
> looks like the bug is still open so I didn't remove the suppression entirely.
> 
> PTAL.

[rlarocque, 2012-09-06 00:13:04]

LGTM, except for the extra diffs in the suppressions file.

https://chromiumcodereview.appspot.com/10907070/diff/11001/tools/valgrind/mem...
File tools/valgrind/memcheck/suppressions.txt (right):

https://chromiumcodereview.appspot.com/10907070/diff/11001/tools/valgrind/mem...
tools/valgrind/memcheck/suppressions.txt:5717: bug_145244
There seem to be a lot of changes in this file between patch sets 2 and 3.  Are
these actually in your diff, or is codereview just confused?

[akalin, 2012-09-06 00:34:18]

committing via trybots

https://chromiumcodereview.appspot.com/10907070/diff/11001/tools/valgrind/mem...
File tools/valgrind/memcheck/suppressions.txt (right):

https://chromiumcodereview.appspot.com/10907070/diff/11001/tools/valgrind/mem...
tools/valgrind/memcheck/suppressions.txt:5717: bug_145244
On 2012/09/06 00:13:04, rlarocque wrote:
> There seem to be a lot of changes in this file between patch sets 2 and 3. 
Are
> these actually in your diff, or is codereview just confused?

That's probably just because I rebased and this file changes frequently.

[commit-bot: I haz the power, 2012-09-06 00:34:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/akalin@chromium.org/10907070/11001

[commit-bot: I haz the power, 2012-09-06 03:06:15]

Change committed as 155110