669199 - Fennec "Enable Sync" checkbox is wrong

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P1)

Description

[Richard Newman [:rnewman]]

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]]

CCing Fennec General in the hopes of getting an opinion.

Comment 2

[Philipp von Weitershausen [:philikon]]

(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 4

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

For the record: The checkbox reenables itself after unchecking.

Comment 5

[Matt Brubeck (:mbrubeck)]

(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)]

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?)]

Let's just make sure Sync is enabled by default and remove this checkbox altogether.

Comment 8

[Lucas Rocha (:lucasr)]

Attachment: Remove checkbox to enable/disable sync from preferences UI

Comment 9

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

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)]

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?)]

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)]

Attachment: 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]]

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)]

(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)]

Comment on attachment 562130 [details] [diff] [review]
Remove ability to enable/disable sync from preferences UI

Nice!

Comment 16

[Philipp von Weitershausen [:philikon]]

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 17

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/81d4441e31a9

Comment 18

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/81d4441e31a9

Comment 19

[Cristian Nicolae (:xti)]

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]

Test case added: https://litmus.mozilla.org/show_test.cgi?id=33517