The default bug view has changed. See this FAQ.

Fennec "Enable Sync" checkbox is wrong

VERIFIED FIXED in Firefox 9

Status

Fennec Graveyard
General
P1
normal
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: rnewman, Assigned: lucasr)

Tracking

Trunk
Firefox 9
All
Android
Dependency tree / graph
Bug Flags:
in-litmus +

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

6 years ago
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.
(Reporter)

Comment 1

6 years ago
CCing Fennec General in the hopes of getting an opinion.
(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.
Component: Firefox Sync: UI → General
Product: Mozilla Services → Fennec
QA Contact: sync-ui → general
Version: unspecified → Trunk

Updated

6 years ago
Priority: -- → P1

Updated

6 years ago
Blocks: 669516
Duplicate of this bug: 669516
For the record: The checkbox reenables itself after unchecking.
(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.
tracking-fennec: --- → ?
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?
Keywords: uiwanted
Let's just make sure Sync is enabled by default and remove this checkbox altogether.
tracking-fennec: ? → 9+
Assignee: nobody → mbrubeck
Assignee: mbrubeck → lucasr.at.mozilla
(Assignee)

Comment 8

6 years ago
Created attachment 561744 [details] [diff] [review]
Remove checkbox to enable/disable sync from preferences UI
Attachment #561744 - Flags: review?(mark.finkle)
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.
Attachment #561744 - Flags: review?(mark.finkle) → review+
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.
Attachment #561744 - Flags: review+ → review?(mark.finkle)
Attachment #561744 - Flags: review?(mark.finkle)
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)
(Assignee)

Comment 12

6 years ago
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.
Attachment #561744 - Attachment is obsolete: true
Attachment #562130 - Flags: review?(mbrubeck)
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
Attachment #562130 - Flags: feedback-
(Assignee)

Comment 14

6 years ago
(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 on attachment 562130 [details] [diff] [review]
Remove ability to enable/disable sync from preferences UI

Nice!
Attachment #562130 - Flags: review?(mbrubeck) → review+
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.)
Attachment #562130 - Flags: feedback-
https://hg.mozilla.org/integration/mozilla-inbound/rev/81d4441e31a9
Status: NEW → ASSIGNED
Keywords: uiwanted
Whiteboard: [inbound]
https://hg.mozilla.org/mozilla-central/rev/81d4441e31a9
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → Firefox 9

Updated

6 years ago
Flags: in-litmus?(fennec)
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
Status: RESOLVED → VERIFIED
status-firefox9: --- → fixed

Comment 20

6 years ago
Test case added: https://litmus.mozilla.org/show_test.cgi?id=33517
Flags: in-litmus?(fennec) → in-litmus+
Depends on: 694311
You need to log in before you can comment on or make changes to this bug.