582885 - services/sync/modules/service.js should export Sync

Status: NEW

(Core :: General, defect)

Description

[Dão Gottwald [:dao]]

Attachment: patch

Comment 1

[Dão Gottwald [:dao]]

Comment on attachment 461152 [details] [diff] [review]
patch

Ugh, apparently services/sync/modules/ext/Sync.js exports Sync.

Comment 2

[Dão Gottwald [:dao]]

Attachment: patch

Comment 3

[Philipp von Weitershausen [:philikon]]

Development for services/sync is happening in http://hg.mozilla.org/services/fx-sync/ and is periodically (3 times so far) merged to m-c. That's because development on the add-on is expected to continue past the Firefox 4.0 release, in which case the add-on replaces resource://services-sync with its newer version and amends Firefox's UI as needed. API compatibility is crucial for this to work, so we need to make sure m-c and fx-sync don't get out of sync.

Thus this bug should probably be moved to the Weave->Sync component and land in fx-sync first (r?mconnor).

Comment 4

[Dão Gottwald [:dao]]

How long is it expected to stay like this? It seems like the primary copy should be in mozilla-central at some point.

Comment 5

[Mike Beltzner [:beltzner, not reading bugmail]]

Does this hard block the UI port of Sync? If so, can we get someone like Vlad to review it and land it in both places at once?

Comment 6

[Dão Gottwald [:dao]]

(In reply to comment #5)
> Does this hard block the UI port of Sync?

No, just cleanup that we should get done.

Comment 7

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 461161 [details] [diff] [review]
patch

I'm not really in a position to review changes to this code, since as far as I know it's not part of the toolkit/firefox module. I don't really have a good idea of what the compatibility constraints are either.

I would like to get rid of Sync.withCb entirely, but that's a separate issue.

Comment 8

[Ed Lee :Mardak]

Curious, why the removal of the Sync function export? It's used to wrap functions where the caller doesn't need an explicit callback.

Comment 9

[Dão Gottwald [:dao]]

(In reply to comment #8)
> Curious, why the removal of the Sync function export? It's used to wrap
> functions where the caller doesn't need an explicit callback.

You mean it's actually used? I couldn't find code using it in mozilla-central.

Comment 10

[Mike Connor [:mconnor]]

Comment on attachment 461161 [details] [diff] [review]
patch

Clearing this for now, though bug 587027 will obsolete this patch anyway.  I don't see a clear rationale here, other than the stumble over Sync being something different.  If we don't want "Weave" as the main object, let's do that, but not halfway like this patch does.

Comment 11

[Dão Gottwald [:dao]]

(In reply to comment #10)
> I don't see a clear rationale here, other than the stumble over Sync being
> something different.

The rationale is that swinging back and forth between "Weave" and "Sync" isn't useful. service.js should export Sync. Since some scripts import both service.js and Sync.js, the much less important Sync.js should stop exporting Sync.

> If we don't want "Weave" as the main object, let's do
> that, but not halfway like this patch does.

There have been concerns (which I didn't share) about breaking compatibility with add-ons currently interacting with the Sync add-on, but at first glance bug 587027 doesn't seem to care about this...