686579 - Kick off a sync immediately after setting up Sync on Fennec

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Philipp von Weitershausen [:philikon]]

OMG I just found out that WeaveGlue only calls Weave.Service.login(), but it doesn't kick off a sync. It really should. With Instant Sync, we may even not kick off a Sync at all until our idle status changes.

Also, we shouldn't use Service.login() at all. "Logging in" is no longer a UI-facing feature and we want to deprecate this API. We should just set username + password + Sync Key on the Service object and then kick off a sync.

Comment 1

[Philipp von Weitershausen [:philikon]]

Attachment: v1

Not tested yet, but this is pretty much what we should be doing (and what the desktop already does).

Comment 2

[Philipp von Weitershausen [:philikon]]

With the current situation we would never sync immediately, but I think with Instant Sync (new in Firefox 7) we may actually not start syncing until something triggers a sync interval change. Marking Firefox 7 and 8 as affected accordingly.

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 560054 [details] [diff] [review]
v1

>-    Weave.Service.login(Weave.Service.username, this.setupData.password, this.setupData.synckey);
>+    Weave.Service.password = this.setupData.password;
>+    Weave.Service.passphrase = this.setupData.synckey;
>     Weave.Service.persistLogin();
>+    setTimeout(function () { Weave.Serivce.sync(); }, 0);

Weave.Service.sync()

r- for this fix and I want a mobile dev to take this off your hands and do some testing. Thanks for filing and getting the patch started.

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

Philipp - re-assigning to Wes so he can make the tweak and do some testing. Feel free to grab it back if you have time to wrap it up.

Comment 5

[Philipp von Weitershausen [:philikon]]

Wes, any progress on this? I'll be happy to fix the patch and do the testing, too. This is a pretty annoying bug exacerbated by Instant Sync. We should fix it asap.

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Philipp von Weitershausen [:philikon] from comment #5)
> Wes, any progress on this? I'll be happy to fix the patch and do the
> testing, too. This is a pretty annoying bug exacerbated by Instant Sync. We
> should fix it asap.

Wes got loaded with some other stuff, so if you want to wrap it up, go ahead.

Comment 7

[Philipp von Weitershausen [:philikon]]

Gotcha. Thanks.

Comment 8

[Philipp von Weitershausen [:philikon]]

Attachment: v2

Fixed typo in v1 and actually tested this patch on a local Fennec build. It works!

Also send the 'weave:service:setup-complete' notification. Weave.Service.login() sends that notification (we did that to retrofit the Fennec UI's behaviour), but since we're no longer using Weave.Service.login() thanks to this patch, we should send it ourselves.

There are some deeper running issues in this code. I really don't understand the Enable Sync checkbox (bug 669199). It still used the no-longer used loggedIn flag (bug 688574). And we probably also want to audit its use of error messages and notifications. I will follow up with more bugs.

Comment 9

[Philipp von Weitershausen [:philikon]]

Also marking Firefox 9 as affected since it's like that by the time this lands, Firefox 9 will probably be in Aurora.

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

I also want to get bug 669199 in fx9 too. Lucas is working on a patch there. CC'ing Lucas so he is aware of this patch.

Comment 11

[Philipp von Weitershausen [:philikon]]

https://hg.mozilla.org/services/services-central/rev/95bee1fc8117

The next Services QA train will probably not make the Aurora cut-off, but I'm
hopefull we can convince release drivers to allow this to go into Aurora (9)
and Beta (8).

Comment 12

[Philipp von Weitershausen [:philikon]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9561bb68864b

Comment 13

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/9561bb68864b

Comment 14

[Aaron Train [:aaronmt]]

Mozilla/5.0 (Android; Linux armv7l; rv:9.0a1) Gecko/20110925 Firefox/9.0a1 Fennec/9.0a1

Comment 15

[Philipp von Weitershausen [:philikon]]

Comment on attachment 561838 [details] [diff] [review]
v2

Asking for Aurora and Beta approval as this severely undermines the first setup UX on mobile right now. As a bonus it also fixes an error in the error console. Patch was verified by QA.

Comment 16

[Tracy Walker [:tracy]]

verified on fennec s-c

Comment 17

[Philipp von Weitershausen [:philikon]]

Patch was landed before the Aurora cut-off, so not landing in Aurora (Firefox 9). Marking Firefox 7 as wontfix since it's released. Landed for Beta (Firefox 8):

https://hg.mozilla.org/releases/mozilla-beta/rev/be7de1a5bbf1

Comment 18

[Kevin Brosnan [Ex-Mozilla]]

Verified

Mozilla/5.0 (Android; Linux armv7l; rv:8.0) Gecko/20111006 Firefox/8.0 Fennec/8.0 ID:20111006003041

Mozilla/5.0 (Android; Linux armv7l; rv:9.0a2) Gecko/20111010 Firefox/9.0a2 Fennec/9.0a2 ID:20111010042022