Last Comment Bug 669199 - Fennec "Enable Sync" checkbox is wrong
: Fennec "Enable Sync" checkbox is wrong
Status: VERIFIED FIXED
:
Product: Fennec Graveyard
Classification: Graveyard
Component: General (show other bugs)
: Trunk
: All Android
: P1 normal (vote)
: Firefox 9
Assigned To: Lucas Rocha (:lucasr)
:
:
Mentors:
: 669516 (view as bug list)
Depends on: 694311
Blocks: 669516
  Show dependency treegraph
 
Reported: 2011-07-04 11:59 PDT by Richard Newman [:rnewman]
Modified: 2011-10-13 17:02 PDT (History)
12 users (show)
andreea.pod: in‑litmus+
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Remove checkbox to enable/disable sync from preferences UI (2.80 KB, patch)
2011-09-22 08:23 PDT, Lucas Rocha (:lucasr)
no flags Details | Diff | Splinter Review
Remove ability to enable/disable sync from preferences UI (13.20 KB, patch)
2011-09-23 12:27 PDT, Lucas Rocha (:lucasr)
mbrubeck: review+
Details | Diff | Splinter Review

Description Richard Newman [:rnewman] 2011-07-04 11:59:43 PDT
All it does right now is log in and log out.

http://mxr.mozilla.org/mozilla-central/source/mobile/chrome/content/sync.js#255

That's not a useful operation: on the next scheduled sync, Sync will just log in again, and then the checkbox (and Fennec's browser.sync.enabled pref!) will be misleading. Logging out won't (IIRC) stop the next sync.

If enabling or disabling Sync is a requirement, then we need to bake this into the Service interface properly. Right now this checkbox should be ripped out, because it's plain wrong.
Comment 1 Richard Newman [:rnewman] 2011-07-04 12:01:58 PDT
CCing Fennec General in the hopes of getting an opinion.
Comment 2 Philipp von Weitershausen [:philikon] 2011-07-04 19:31:11 PDT
(In reply to comment #1)
> CCing Fennec General in the hopes of getting an opinion.

Sync's Fennec UI is owned by the Mobile UI team, so moving this bug to the Fennec :: General component.
Comment 3 Mark Finkle (:mfinkle) (use needinfo?) 2011-07-11 12:08:04 PDT
*** Bug 669516 has been marked as a duplicate of this bug. ***
Comment 4 Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout) 2011-09-19 01:50:29 PDT
For the record: The checkbox reenables itself after unchecking.
Comment 5 Matt Brubeck (:mbrubeck) 2011-09-19 09:33:08 PDT
(In reply to Archaeopteryx [:aryx] from comment #4)
> For the record: The checkbox reenables itself after unchecking.

If you're seeing this in recent nightly builds, it is probably bug 687298.
Comment 6 Matt Brubeck (:mbrubeck) 2011-09-19 09:52:31 PDT
Madhava, I seem to recall that we were planning on removing the "enable sync" checkbox anyway as part of an upcoming UI change.  Is that still planned, and is there a bug for it?
Comment 7 Mark Finkle (:mfinkle) (use needinfo?) 2011-09-21 14:17:39 PDT
Let's just make sure Sync is enabled by default and remove this checkbox altogether.
Comment 8 Lucas Rocha (:lucasr) 2011-09-22 08:23:12 PDT
Created attachment 561744 [details] [diff] [review]
Remove checkbox to enable/disable sync from preferences UI
Comment 9 Mark Finkle (:mfinkle) (use needinfo?) 2011-09-22 08:51:01 PDT
Comment on attachment 561744 [details] [diff] [review]
Remove checkbox to enable/disable sync from preferences UI

r+, but let's also remove all occurrences of "browser.sync.enabled" since we should be assuming it's always true now. "browser.sync.enabled" was a Fennec only, made up pref. Not part of the real Sync engine.
Comment 10 Matt Brubeck (:mbrubeck) 2011-09-22 08:53:16 PDT
Comment on attachment 561744 [details] [diff] [review]
Remove checkbox to enable/disable sync from preferences UI

There's some more code that will need to be removed, including the references to the autosync checkbox in mobile/chrome/content/sync.js.  Make sure we are not regressing bug 624552.
Comment 11 Mark Finkle (:mfinkle) (use needinfo?) 2011-09-23 11:35:36 PDT
Lucas - Matt is referring to:
http://mxr.mozilla.org/mozilla-central/source/mobile/chrome/content/sync.js#383

where we do a document.getElementById("sync" + foo) 
and foo will be "autosync"

More locations:
http://mxr.mozilla.org/mozilla-central/source/mobile/chrome/content/sync.js#404 (and any use of autosync variable)
http://mxr.mozilla.org/mozilla-central/source/mobile/chrome/content/sync.js#260 (all of toggleSyncEnabled)
Comment 12 Lucas Rocha (:lucasr) 2011-09-23 12:27:28 PDT
Created attachment 562130 [details] [diff] [review]
Remove ability to enable/disable sync from preferences UI

Here's a more complete patch removing everything related to enabling/disabling sync in Fennec. I've also ensured that we're not regressing on bug 624552.
Comment 13 Philipp von Weitershausen [:philikon] 2011-09-23 12:32:04 PDT
Comment on attachment 562130 [details] [diff] [review]
Remove ability to enable/disable sync from preferences UI

There's also the 'syncEnabled' variable in the 'observe' method which should go away: https://mxr.mozilla.org/mozilla-central/source/mobile/chrome/content/sync.js#410
Comment 14 Lucas Rocha (:lucasr) 2011-09-23 12:40:58 PDT
(In reply to Philipp von Weitershausen [:philikon] from comment #13)
> Comment on attachment 562130 [details] [diff] [review] [diff] [details] [review]
> Remove ability to enable/disable sync from preferences UI
> 
> There's also the 'syncEnabled' variable in the 'observe' method which should
> go away:
> https://mxr.mozilla.org/mozilla-central/source/mobile/chrome/content/sync.
> js#410

My new patch removes it.
Comment 15 Matt Brubeck (:mbrubeck) 2011-09-23 13:13:05 PDT
Comment on attachment 562130 [details] [diff] [review]
Remove ability to enable/disable sync from preferences UI

Nice!
Comment 16 Philipp von Weitershausen [:philikon] 2011-09-23 13:18:42 PDT
Comment on attachment 562130 [details] [diff] [review]
Remove ability to enable/disable sync from preferences UI

Sorry, I must obviously have been blind. Great patch! (I wish diff -p would actually recognize JS methods and not print "var BrowserUI = {" like in this case.)
Comment 18 Ed Morley [:emorley] 2011-09-23 20:47:37 PDT
https://hg.mozilla.org/mozilla-central/rev/81d4441e31a9
Comment 19 Cristian Nicolae (:xti) 2011-09-27 06:25:15 PDT
Verified fixed on:
Mozilla/5.0 (Android;Linux armv7l;rv:9.0a1)Gecko/20110927
Firefox/9.0a1 Fennec/9.0a1
Device: Acer ICONIA A500
OS: Android 3.1
Comment 20 Andreea Pod 2011-10-13 07:10:10 PDT
Test case added: https://litmus.mozilla.org/show_test.cgi?id=33517

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