Last Comment Bug 686579 - Kick off a sync immediately after setting up Sync on Fennec
: Kick off a sync immediately after setting up Sync on Fennec
Status: VERIFIED FIXED
[verified in services]
: verified-aurora, verified-beta
Product: Fennec Graveyard
Classification: Graveyard
Component: General (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 9
Assigned To: Philipp von Weitershausen [:philikon]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-09-13 16:00 PDT by Philipp von Weitershausen [:philikon]
Modified: 2011-10-10 08:16 PDT (History)
9 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
v1 (1.22 KB, patch)
2011-09-13 16:04 PDT, Philipp von Weitershausen [:philikon]
mark.finkle: review-
Details | Diff | Review
v2 (1.96 KB, patch)
2011-09-22 12:43 PDT, Philipp von Weitershausen [:philikon]
mark.finkle: review+
mark.finkle: approval‑mozilla‑aurora+
mark.finkle: approval‑mozilla‑beta+
Details | Diff | Review

Description Philipp von Weitershausen [:philikon] 2011-09-13 16:00:08 PDT
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] 2011-09-13 16:04:54 PDT
Created attachment 560054 [details] [diff] [review]
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] 2011-09-13 16:08:05 PDT
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?) 2011-09-13 22:49:33 PDT
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?) 2011-09-13 22:50:57 PDT
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] 2011-09-21 12:53:59 PDT
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?) 2011-09-21 18:31:50 PDT
(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] 2011-09-21 18:35:22 PDT
Gotcha. Thanks.
Comment 8 Philipp von Weitershausen [:philikon] 2011-09-22 12:43:07 PDT
Created attachment 561838 [details] [diff] [review]
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] 2011-09-22 12:44:23 PDT
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?) 2011-09-22 12:57:19 PDT
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] 2011-09-22 13:15:51 PDT
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] 2011-09-22 14:11:13 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/9561bb68864b
Comment 13 Ed Morley [:emorley] 2011-09-23 04:53:27 PDT
https://hg.mozilla.org/mozilla-central/rev/9561bb68864b
Comment 14 Aaron Train [:aaronmt] 2011-09-25 06:47:21 PDT
Mozilla/5.0 (Android; Linux armv7l; rv:9.0a1) Gecko/20110925 Firefox/9.0a1 Fennec/9.0a1
Comment 15 Philipp von Weitershausen [:philikon] 2011-09-28 15:24:57 PDT
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] 2011-09-28 16:22:48 PDT
verified on fennec s-c
Comment 17 Philipp von Weitershausen [:philikon] 2011-09-29 15:22:59 PDT
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 [:kbrosnan] 2011-10-10 08:16:20 PDT
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

Note You need to log in before you can comment on or make changes to this bug.