[sync] Componentize sync: Part 6: Add more SYNC_EXPORTs to files in src/sync/

[sync] Componentize sync: Part 6: Add more SYNC_EXPORTs to files in src/sync/

One of the long term goals of the sync team is to pull sync code out of
chrome.dll and into its own component. As of today, several chrome targets
depend on various sync targets as defined in sync.gyp. We'd like to move
to a world where all chrome targets outside sync.gyp simply depend on the
target sync.gyp:sync, which is built into its own component.

This patch sets the stage for full componentization by adding
SYNC_EXPORT_PRIVATE (and some SYNC_EXPORT) annotations to classes / methods
within src/sync so that various chrome targets can reference them.

It also makes a few assorted fixes like adding missing destructors, etc. These
errors get flagged when component builds are enabled after componentizing
sync.

The final step of breaking off sync into its own component will be done
in https://codereview.chromium.org/11412211.

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

[Raghu Simha, 2012-12-22 01:25:52]

Richard, please review. Thanks!

[rlarocque, 2012-12-22 02:00:49]

https://codereview.chromium.org/11624037/diff/9001/sync/internal_api/public/u...
File sync/internal_api/public/util/weak_handle.h (left):

https://codereview.chromium.org/11624037/diff/9001/sync/internal_api/public/u...
sync/internal_api/public/util/weak_handle.h:112: ~WeakHandleCoreBase();
Come to think of it, why is this not virtual?

https://codereview.chromium.org/11624037/diff/9001/sync/internal_api/public/u...
sync/internal_api/public/util/weak_handle.h:128: : public
NON_EXPORTED_BASE(WeakHandleCoreBase),
It seems like this change goes against the original author's intent.  Why won't
this work as written?

https://codereview.chromium.org/11624037/diff/9001/sync/syncable/directory_ba...
File sync/syncable/directory_backing_store.cc (right):

https://codereview.chromium.org/11624037/diff/9001/sync/syncable/directory_ba...
sync/syncable/directory_backing_store.cc:43: SYNC_EXPORT_PRIVATE extern const
int32 kCurrentDBVersion;
There should be no need for extern declarations in .cc files.  Could we just
remove this line?

https://codereview.chromium.org/11624037/diff/9001/sync/syncable/directory_ba...
File sync/syncable/directory_backing_store.h (left):

https://codereview.chromium.org/11624037/diff/9001/sync/syncable/directory_ba...
sync/syncable/directory_backing_store.h:174:
DISALLOW_COPY_AND_ASSIGN(DirectoryBackingStore);
Why?

[Raghu Simha, 2012-12-22 02:25:02]

Richard, feedback addressed. PTAL.

+akalin as backup reviewer, in case Richard isn't able to get to this due to his
upcoming vacation.

https://codereview.chromium.org/11624037/diff/9001/sync/internal_api/public/u...
File sync/internal_api/public/util/weak_handle.h (left):

https://codereview.chromium.org/11624037/diff/9001/sync/internal_api/public/u...
sync/internal_api/public/util/weak_handle.h:112: ~WeakHandleCoreBase();
On 2012/12/22 02:00:49, rlarocque wrote:
> Come to think of it, why is this not virtual?

Good catch. Fixed.

https://codereview.chromium.org/11624037/diff/9001/sync/internal_api/public/u...
sync/internal_api/public/util/weak_handle.h:128: : public
NON_EXPORTED_BASE(WeakHandleCoreBase),
On 2012/12/22 02:00:49, rlarocque wrote:
> It seems like this change goes against the original author's intent.  Why
won't
> this work as written?

I agree. Perhaps the error I saw had to do with the non-virtual destructor. I've
restored this like to what it was.

https://codereview.chromium.org/11624037/diff/9001/sync/syncable/directory_ba...
File sync/syncable/directory_backing_store.cc (right):

https://codereview.chromium.org/11624037/diff/9001/sync/syncable/directory_ba...
sync/syncable/directory_backing_store.cc:43: SYNC_EXPORT_PRIVATE extern const
int32 kCurrentDBVersion;
On 2012/12/22 02:00:49, rlarocque wrote:
> There should be no need for extern declarations in .cc files.  Could we just
> remove this line?

Done.

https://codereview.chromium.org/11624037/diff/9001/sync/syncable/directory_ba...
File sync/syncable/directory_backing_store.h (left):

https://codereview.chromium.org/11624037/diff/9001/sync/syncable/directory_ba...
sync/syncable/directory_backing_store.h:174:
DISALLOW_COPY_AND_ASSIGN(DirectoryBackingStore);
On 2012/12/22 02:00:49, rlarocque wrote:
> Why?

Probably happened while I was debugging. Restored.

[Raghu Simha, 2012-12-26 08:02:38]

Fred, roping you in as main reviewer since Richard is on vacation this week. 

I was able to get a green component build with this patch (after applying
https://codereview.chromium.org/11412211 to my local branch).

Let me know what you think.

https://codereview.chromium.org/11624037/diff/2004/sync/internal_api/public/u...
File sync/internal_api/public/util/weak_handle.h (right):

https://codereview.chromium.org/11624037/diff/2004/sync/internal_api/public/u...
sync/internal_api/public/util/weak_handle.h:128: : public
NON_EXPORTED_BASE(WeakHandleCoreBase),
Note: I had added this NON_EXPORTED_BASE annotation in a previous CL thinking it
made sense. It doesn't, since the classes that inherit from WeakHandleCoreBase
are all template classes that are entirely defined in the header file, and
therefore, do not need to be exported.

https://codereview.chromium.org/11624037/diff/1100/sync/internal_api/public/u...
File sync/internal_api/public/util/weak_handle.h (right):

https://codereview.chromium.org/11624037/diff/1100/sync/internal_api/public/u...
sync/internal_api/public/util/weak_handle.h:103: class SYNC_EXPORT
WeakHandleCoreBase {
Unfortunately, WeakHandleCoreBase must be exported in order for MakeWeakHandle
to work from components outside sync.

[akalin, 2012-12-26 09:39:57]

https://codereview.chromium.org/11624037/diff/9001/sync/internal_api/public/u...
File sync/internal_api/public/util/weak_handle.h (left):

https://codereview.chromium.org/11624037/diff/9001/sync/internal_api/public/u...
sync/internal_api/public/util/weak_handle.h:112: ~WeakHandleCoreBase();
On 2012/12/22 02:25:02, rsimha wrote:
> On 2012/12/22 02:00:49, rlarocque wrote:
> > Come to think of it, why is this not virtual?
> 
> Good catch. Fixed.

This non-virtual on purpose.  The inheritance is just to reduce template bloat,
and WeakHandles are never destroyed through a superclass pointer, so there's no
need for virtual destructors.

https://codereview.chromium.org/11624037/diff/9001/sync/internal_api/public/u...
sync/internal_api/public/util/weak_handle.h:128: : public
NON_EXPORTED_BASE(WeakHandleCoreBase),
On 2012/12/22 02:25:02, rsimha wrote:
> On 2012/12/22 02:00:49, rlarocque wrote:
> > It seems like this change goes against the original author's intent.  Why
> won't
> > this work as written?
> 
> I agree. Perhaps the error I saw had to do with the non-virtual destructor.
I've
> restored this like to what it was.

Hmm, I still think this is correct.  Template classes don't need export
declarations since they're templated, but all these inherit from a single
internal class, which shouldn't need to be exported, and NON_EXPORTED_BASE is to
suppress the warning this produces in MSVC.

[Raghu Simha, 2012-12-27 01:07:35]

Fred, PTAL. Thanks.

On 2012/12/26 09:39:57, akalin wrote:
> This non-virtual on purpose.  The inheritance is just to reduce template
bloat,
> and WeakHandles are never destroyed through a superclass pointer, so there's
no
> need for virtual destructors.

SG. I've undone all the "virtual" changes to this file.

> Hmm, I still think this is correct.  Template classes don't need export
> declarations since they're templated, but all these inherit from a single
> internal class, which shouldn't need to be exported, and NON_EXPORTED_BASE is
to
> suppress the warning this produces in MSVC.

I wasn't exporting WeakHandleCoreBase merely due to linker warnings. The problem
that arises  when sync is moved into its own component is when a class in
chrome/browser tries to instantiate a WeakPtr<T>. The symbols IsOnOwnerThread()
and Get() aren't exported, as a result of which there are linker errors. See
lines 307-311, and my earlier comment on weak_handle.h in patch set 5.

In any case, I don't believe this one issue should block this patch, so I've
undone all changes to weak_handle.h for now. I'll revisit this (yet again :) )
when I finally componentize sync, and see if there's any way of not exporting
WeakHandleCoreBase.

Let me know what you think.

[Raghu Simha, 2012-12-27 01:50:18]

On 2012/12/27 01:07:35, rsimha wrote:
> I wasn't exporting WeakHandleCoreBase merely due to linker warnings. The
problem
> that arises  when sync is moved into its own component is when a class in
> chrome/browser tries to instantiate a WeakPtr<T>. The symbols
IsOnOwnerThread()
> and Get() aren't exported, as a result of which there are linker errors. See
> lines 307-311, and my earlier comment on weak_handle.h in patch set 5.

Follow up: I just tried building sync as a component without exporting
WeakHandleCoreBase, and here is a sample linker error I'm seeing:

browser_ui.sync_internals_ui.obj : error LNK2019: unresolved external symbol
"public: bool __thiscall
syncer::internal::WeakHandleCoreBase::IsOnOwnerThread(void)const "
(?IsOnOwnerThread@WeakHandleCoreBase@internal@syncer@@QBE_NXZ) referenced in
function "public: class base::WeakPtr<class SyncInternalsUI> __thiscall
syncer::internal::WeakHandleCore<class SyncInternalsUI>::Get(void)const "
(?Get@?$WeakHandleCore@VSyncInternalsUI@@@internal@syncer@@QBE?AV?$WeakPtr@VSyncInternalsUI@@@base@@XZ)

I'll check to see if there's something else I'm missing, but it's looking like
exporting WeakHandleCoreBase might be the only fix. In any case, since it's just
the one change, I'm fine with not making it in this patch.

[Raghu Simha, 2012-12-27 02:25:01]

On 2012/12/27 01:50:18, rsimha wrote:
> I'll check to see if there's something else I'm missing, but it's looking like
> exporting WeakHandleCoreBase might be the only fix. In any case, since it's
just
> the one change, I'm fine with not making it in this patch.

Fred: after more testing, it looks like exporting WeakHandleCoreBase is the only
fix for the linker errors I was seeing with component builds (with sync pulled
out as a component). I've gone ahead and uploaded patch set 7, where the only
change from patch set 6 is that WeakHandleCoreBase is now exported. Assuming
there are no other issues you have with this CL, you can either approve patch
set 6 or patch set 7.

(Sorry about the flurry of messages, but I figured this might help eliminate a
round trip, since I'm 12 hrs ahead of PST this week :) )

[akalin, 2012-12-27 07:14:42]

I'm okay with exporting WeakHandleCoreBase.  I'd have to think about it a bit
more, but I think it does have to be exported.

I'll take another pass in a bit.

[akalin, 2012-12-27 19:48:52]

If there's no good reason to make ~AckHandle() and ~Invalidation() virtual and
stuff works when removing them, then LGTM that.  Otherwise, I'd like to know why
it's necessary first.

https://codereview.chromium.org/11624037/diff/24001/sync/engine/apply_updates...
File sync/engine/apply_updates_and_resolve_conflicts_command.h (right):

https://codereview.chromium.org/11624037/diff/24001/sync/engine/apply_updates...
sync/engine/apply_updates_and_resolve_conflicts_command.h:17: friend class
ApplyUpdatesAndResolveConflictsCommandTest;
out of curiosity, why did this end up being needed?  Did it have to do with
SYNC_EXPORT_PRIVATE?

https://codereview.chromium.org/11624037/diff/24001/sync/internal_api/public/...
File sync/internal_api/public/base/invalidation.h (right):

https://codereview.chromium.org/11624037/diff/24001/sync/internal_api/public/...
sync/internal_api/public/base/invalidation.h:35: virtual ~AckHandle();
hunh, why is this necessary?  Nothing inherits from AckHandle, right?

https://codereview.chromium.org/11624037/diff/24001/sync/internal_api/public/...
sync/internal_api/public/base/invalidation.h:52: virtual ~Invalidation();
here, too

https://codereview.chromium.org/11624037/diff/24001/sync/internal_api/public/...
File sync/internal_api/public/http_bridge.h (right):

https://codereview.chromium.org/11624037/diff/24001/sync/internal_api/public/...
sync/internal_api/public/http_bridge.h:125: net::URLRequestContextGetter*
GetRequestContextGetter() const;
Append ForTest()

[Raghu Simha, 2013-01-02 06:49:24]

https://codereview.chromium.org/11624037/diff/24001/sync/engine/apply_updates...
File sync/engine/apply_updates_and_resolve_conflicts_command.h (right):

https://codereview.chromium.org/11624037/diff/24001/sync/engine/apply_updates...
sync/engine/apply_updates_and_resolve_conflicts_command.h:17: friend class
ApplyUpdatesAndResolveConflictsCommandTest;
On 2012/12/27 19:48:52, akalin wrote:
> out of curiosity, why did this end up being needed?  Did it have to do with
> SYNC_EXPORT_PRIVATE?

Good question. Turns out it isn't necessary after all. I must've added it while
trying to fix some errors and not removed it after that. Removed now.

https://codereview.chromium.org/11624037/diff/24001/sync/internal_api/public/...
File sync/internal_api/public/base/invalidation.h (right):

https://codereview.chromium.org/11624037/diff/24001/sync/internal_api/public/...
sync/internal_api/public/base/invalidation.h:35: virtual ~AckHandle();
On 2012/12/27 19:48:52, akalin wrote:
> hunh, why is this necessary?  Nothing inherits from AckHandle, right?

You're right that the destructors don't have to be virtual. However, not adding
them at all results in a LNK2005 (multiply defined symbol) error because the
compiler automatically defines the destructors in the components that include
this header. I've made the destructor non-virtual.

https://codereview.chromium.org/11624037/diff/24001/sync/internal_api/public/...
sync/internal_api/public/base/invalidation.h:52: virtual ~Invalidation();
On 2012/12/27 19:48:52, akalin wrote:
> here, too

Made this non-virtual too.

https://codereview.chromium.org/11624037/diff/24001/sync/internal_api/public/...
File sync/internal_api/public/http_bridge.h (right):

https://codereview.chromium.org/11624037/diff/24001/sync/internal_api/public/...
sync/internal_api/public/http_bridge.h:125: net::URLRequestContextGetter*
GetRequestContextGetter() const;
On 2012/12/27 19:48:52, akalin wrote:
> Append ForTest()

Done.

[akalin, 2013-01-02 07:07:52]

LGTM!

On 2013/01/02 06:49:24, rsimha wrote:
>
https://codereview.chromium.org/11624037/diff/24001/sync/engine/apply_updates...
> File sync/engine/apply_updates_and_resolve_conflicts_command.h (right):
> 
>
https://codereview.chromium.org/11624037/diff/24001/sync/engine/apply_updates...
> sync/engine/apply_updates_and_resolve_conflicts_command.h:17: friend class
> ApplyUpdatesAndResolveConflictsCommandTest;
> On 2012/12/27 19:48:52, akalin wrote:
> > out of curiosity, why did this end up being needed?  Did it have to do with
> > SYNC_EXPORT_PRIVATE?
> 
> Good question. Turns out it isn't necessary after all. I must've added it
while
> trying to fix some errors and not removed it after that. Removed now.
> 
>
https://codereview.chromium.org/11624037/diff/24001/sync/internal_api/public/...
> File sync/internal_api/public/base/invalidation.h (right):
> 
>
https://codereview.chromium.org/11624037/diff/24001/sync/internal_api/public/...
> sync/internal_api/public/base/invalidation.h:35: virtual ~AckHandle();
> On 2012/12/27 19:48:52, akalin wrote:
> > hunh, why is this necessary?  Nothing inherits from AckHandle, right?
> 
> You're right that the destructors don't have to be virtual. However, not
adding
> them at all results in a LNK2005 (multiply defined symbol) error because the
> compiler automatically defines the destructors in the components that include
> this header. I've made the destructor non-virtual.
> 
>
https://codereview.chromium.org/11624037/diff/24001/sync/internal_api/public/...
> sync/internal_api/public/base/invalidation.h:52: virtual ~Invalidation();
> On 2012/12/27 19:48:52, akalin wrote:
> > here, too
> 
> Made this non-virtual too.
> 
>
https://codereview.chromium.org/11624037/diff/24001/sync/internal_api/public/...
> File sync/internal_api/public/http_bridge.h (right):
> 
>
https://codereview.chromium.org/11624037/diff/24001/sync/internal_api/public/...
> sync/internal_api/public/http_bridge.h:125: net::URLRequestContextGetter*
> GetRequestContextGetter() const;
> On 2012/12/27 19:48:52, akalin wrote:
> > Append ForTest()
> 
> Done.