745065 - Remove apps sync engine

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

[Anant Narayanan [:anant]]

When the new AITC client (bug 744257) lands, the apps sync engine in the tree may removed as it duplicates functionality provided by AITC.

Comment 1

[Richard Newman [:rnewman]]

Is there any need to wait?

Comment 2

[Anant Narayanan [:anant]]

No reason to wait, in fact Fabrice also suggests we remove it now so we can check in the changes we need to DOMApplicationRegistry.

I'll attach a patch shortly.

Comment 3

[Anant Narayanan [:anant]]

Attachment: Remove App Sync Engine - v1

This patch removes the app sync engine, which will be replaced by the new AITC client.

Comment 4

[Gregory Szorc [:gps]]

There are a bunch of removed-files.in files scattered in the tree. Do we need to update those as well? (I'm really not sure what they are used for.)

Comment 5

[Philipp von Weitershausen [:philikon]]

(In reply to Gregory Szorc [:gps] from comment #4)
> There are a bunch of removed-files.in files scattered in the tree. Do we
> need to update those as well? (I'm really not sure what they are used for.)

# Removed-files.in is processed at build time to create a list of files that
# should be removed during an application update.

So yes, files that were in a release at some point but should be removed in an update because they're no longer shipped, must be added to removed-files.in.

Comment 6

[Anant Narayanan [:anant]]

Ah, you're right; they are used to remove files on application updates, so we'll have to update that to make sure we don't leave apps.js lying around in modules/services-sync/engines/ on a Firefox update. I'll attach a patch to update removed-files.in.

Comment 7

[Anant Narayanan [:anant]]

Looking at removed-files.in, all of the sync engine files are already listed (line 1047). The blame for that line leads to bug 556644, which seems to indicate that since we switched to Omnijar it is unnecessary to add lines to removed-files.in.

Are there platforms where we still don't use Omnijar? If so we'd need to add an entry, but only if apps.js is also present on those platforms.

Comment 8

[Anant Narayanan [:anant]]

Attachment: Remove App Sync Engine - v1.1

Add apps.js to removed-files.in. I've put it in two places (inside an #ifdef MOZ_OMNIJAR and outside) to be safe and remove the file from all platforms, whether or not we had Omnijar support on them.

Comment 9

[Richard Newman [:rnewman]]

Comment on attachment 614686 [details] [diff] [review]
Remove App Sync Engine - v1.1

Review of attachment 614686 [details] [diff] [review]:
-----------------------------------------------------------------

This looks fine. Please make sure that TPS and xpcshell tests pass.

You perhaps also want to revert some of the changes to webapps.jsm:

https://bug706545.bugzilla.mozilla.org/attachment.cgi?id=579890

Comment 10

[Anant Narayanan [:anant]]

Thanks! The changes to Webapps.jsm will be done in bug 745069. Pushing to try to check tests.

Comment 11

[Mozilla RelEng Bot]

Autoland Patchset:
	Patches: 614686
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/pushloghtml?changeset=b5099b5b4a39
Try run started, revision b5099b5b4a39. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=b5099b5b4a39

Comment 12

[Mozilla RelEng Bot]

Try run for b5099b5b4a39 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=b5099b5b4a39
Results (out of 15 total builds):
    success: 15
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-b5099b5b4a39

Comment 13

[Anant Narayanan [:anant]]

Pushed: https://hg.mozilla.org/services/services-central/rev/e24ad9cee741

Forgot the r= in the commit message! Clearly, it's been a while since I did a hg push...

Comment 14

[Gregory Szorc [:gps]]

We add this whiteboard for s-c pushes to help track the tree.

Comment 15

[Anant Narayanan [:anant]]

https://tbpl.mozilla.org/?tree=Services-Central&rev=e24ad9cee741 shows a few oranges for this check-in. What is the general procedure when this occurs and how do we investigate which failures are real and which are intermittent?

Comment 16

[Richard Newman [:rnewman]]

(In reply to Anant Narayanan [:anant] from comment #15)
> https://tbpl.mozilla.org/?tree=Services-Central&rev=e24ad9cee741 shows a few
> oranges for this check-in. What is the general procedure when this occurs
> and how do we investigate which failures are real and which are intermittent?

You star them! Take a look at tbpl now, as I do so.

Comment 17

[Richard Newman [:rnewman]]

In general, if we break something like this it'll be in xpcshell-tests (the "X" in tbpl), and it'll usually be on all platforms. You're just seeing intermittent orange here.

Comment 18

[Anant Narayanan [:anant]]

Nice, thanks!

TPS tests are also run for check-ins to services-central, right? Where can I find out whether they succeeded?

Comment 19

[Richard Newman [:rnewman]]

The last TPS report I received was on the 10th, so it doesn't appear to have run for your push.

Will ping jgriffin.

You can run them yourself:

https://developer.mozilla.org/en/TPS

or you can ask jgriffin to add you to the mailing list if you want to keep an eye on them over time.

Comment 20

[Anant Narayanan [:anant]]

We're holding off on removing the sync engine as the decision to use AITC exclusively on all Gecko platforms is being revisited. In the meantime, the plan is to have both the classic sync engine for apps as well as AITC, behind prefs so only one is active at any given time.

Once clear consensus has been reached, one of these will likely go away.

https://hg.mozilla.org/services/services-central/rev/c019b4bbe9ff

Comment 21

[Richard Newman [:rnewman]]

(In reply to Anant Narayanan [:anant] from comment #20)
> We're holding off on removing the sync engine as the decision to use AITC
> exclusively on all Gecko platforms is being revisited.

Does that mean that Bug 705910 needs to be reopened?

Comment 22

[Anant Narayanan [:anant]]

The code in bug 705910 did not end up landing in mozilla-central, while the code from bug 706545 did. As for a re-review of the latter, it may or may not be required, but I suggest that we wait for consensus on which app sync solution is the right one going forward before spending any time on it.

Comment 23

[Tracy Walker [:tracy]]

services train in QA today.  gps called out this bug, but it's actually a no action item, correct?

Comment 24

[Gregory Szorc [:gps]]

https://hg.mozilla.org/mozilla-central/rev/e24ad9cee741
https://hg.mozilla.org/mozilla-central/rev/c019b4bbe9ff