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

[sync] Componentize sync: Part 2: Add 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 annotations to classes / methods within src/sync.

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

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

[Raghu Simha, 2012-12-10 23:07:52]

Fred, please review. Thanks.

[akalin, 2012-12-11 00:02:52]

few questions/comments

https://codereview.chromium.org/11515009/diff/4002/sync/internal_api/public/c...
File sync/internal_api/public/change_record.h (right):

https://codereview.chromium.org/11515009/diff/4002/sync/internal_api/public/c...
sync/internal_api/public/change_record.h:35: SYNC_EXPORT const
sync_pb::PasswordSpecificsData& unencrypted() const;
isn't this an error?  Shouldn't it be the class that has this annotation?

https://codereview.chromium.org/11515009/diff/4002/sync/internal_api/public/s...
File sync/internal_api/public/sync_encryption_handler.h (right):

https://codereview.chromium.org/11515009/diff/4002/sync/internal_api/public/s...
sync/internal_api/public/sync_encryption_handler.h:58: class SYNC_EXPORT
Observer {
this is a bit weird, but I guess it's okay

https://codereview.chromium.org/11515009/diff/4002/sync/internal_api/public/u...
File sync/internal_api/public/util/syncer_error.h (right):

https://codereview.chromium.org/11515009/diff/4002/sync/internal_api/public/u...
sync/internal_api/public/util/syncer_error.h:46: SYNC_EXPORT const char *
GetSyncerErrorString(SyncerError);
remove the space before * since you're here

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

https://codereview.chromium.org/11515009/diff/4002/sync/internal_api/public/u...
sync/internal_api/public/util/weak_handle.h:103: class SYNC_EXPORT
WeakHandleCoreBase {
hmm.  Can you see if you can avoid exporting this and instead have
WeakHandleCore annotate its inheritance of WeakHandleCoreBase with
NON_EXPORTED_BASE()?

https://codereview.chromium.org/11515009/diff/4002/sync/syncable/directory.h
File sync/syncable/directory.h (right):

https://codereview.chromium.org/11515009/diff/4002/sync/syncable/directory.h#...
sync/syncable/directory.h:289: SYNC_EXPORT std::string cache_guid() const;
yeah, i guess I'm not sure if annotating member functions is a good thing.  So
something outside of sync can have a Cryptographer*, but if it calls the wrong
function it leads to a linker error?  Hmm.  I haven't seen this in other uses of
*_EXPORT but maybe it's uncommon.  Perhaps consult with one of the export gurus
(rsleevi?) about this?

[Raghu Simha, 2012-12-11 01:27:51]

Fred, changes made. PTAL.

https://codereview.chromium.org/11515009/diff/4002/sync/internal_api/public/c...
File sync/internal_api/public/change_record.h (right):

https://codereview.chromium.org/11515009/diff/4002/sync/internal_api/public/c...
sync/internal_api/public/change_record.h:35: SYNC_EXPORT const
sync_pb::PasswordSpecificsData& unencrypted() const;
On 2012/12/11 00:02:52, akalin wrote:
> isn't this an error?  Shouldn't it be the class that has this annotation?

Interesting. I was initially trying to export the bare minimum required to get a
component build going. I've changed all method-only exports to class exports.

Fixed.

https://codereview.chromium.org/11515009/diff/4002/sync/internal_api/public/s...
File sync/internal_api/public/sync_encryption_handler.h (right):

https://codereview.chromium.org/11515009/diff/4002/sync/internal_api/public/s...
sync/internal_api/public/sync_encryption_handler.h:58: class SYNC_EXPORT
Observer {
On 2012/12/11 00:02:52, akalin wrote:
> this is a bit weird, but I guess it's okay

Weird because SyncEncryptionHandler isn't being exported? I suppose I should
export it too, instead of just exporting SensitiveTypes(). Fixed.

https://codereview.chromium.org/11515009/diff/4002/sync/internal_api/public/u...
File sync/internal_api/public/util/syncer_error.h (right):

https://codereview.chromium.org/11515009/diff/4002/sync/internal_api/public/u...
sync/internal_api/public/util/syncer_error.h:46: SYNC_EXPORT const char *
GetSyncerErrorString(SyncerError);
On 2012/12/11 00:02:52, akalin wrote:
> remove the space before * since you're here

Done.

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

https://codereview.chromium.org/11515009/diff/4002/sync/internal_api/public/u...
sync/internal_api/public/util/weak_handle.h:103: class SYNC_EXPORT
WeakHandleCoreBase {
On 2012/12/11 00:02:52, akalin wrote:
> hmm.  Can you see if you can avoid exporting this and instead have
> WeakHandleCore annotate its inheritance of WeakHandleCoreBase with
> NON_EXPORTED_BASE()?

Done.

https://codereview.chromium.org/11515009/diff/4002/sync/syncable/directory.h
File sync/syncable/directory.h (right):

https://codereview.chromium.org/11515009/diff/4002/sync/syncable/directory.h#...
sync/syncable/directory.h:289: SYNC_EXPORT std::string cache_guid() const;
On 2012/12/11 00:02:52, akalin wrote:
> yeah, i guess I'm not sure if annotating member functions is a good thing.  So
> something outside of sync can have a Cryptographer*, but if it calls the wrong
> function it leads to a linker error?  Hmm.  I haven't seen this in other uses
of
> *_EXPORT but maybe it's uncommon.  Perhaps consult with one of the export
gurus
> (rsleevi?) about this?

I did a bunch of grepping of the source code for other FOO_EXPORT usage, and
you're right -- I couldn't find precedent for method-only exports. I've gone and
fixed all instances in this CL.

[Raghu Simha, 2012-12-11 21:28:18]

Ping.

[akalin, 2012-12-11 21:35:10]

LGTM!  yay!

[commit-bot: I haz the power, 2012-12-11 21:38:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rsimha@chromium.org/11515009/1061

[commit-bot: I haz the power, 2012-12-11 23:47:44]

Change committed as 172458