Closed Bug 1220906 Opened 4 years ago Closed 4 years ago

Delete Old Sync Android Account type

Categories

(Firefox for Android :: Android Sync, defect)

Firefox 46
All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 46
Tracking Status
firefox46 --- fixed
relnote-firefox --- 46+
fennec 46+ ---

People

(Reporter: nalexander, Assigned: nalexander, Mentored)

References

Details

(Whiteboard: [lang=java][good next bug])

Attachments

(9 files)

58 bytes, text/x-review-board-request
nalexander
: review+
Details
58 bytes, text/x-review-board-request
nalexander
: review+
rnewman
: review+
Details
58 bytes, text/x-review-board-request
rnewman
: review+
Details
58 bytes, text/x-review-board-request
rnewman
: review+
Details
58 bytes, text/x-review-board-request
rnewman
: review+
Details
58 bytes, text/x-review-board-request
rnewman
: review+
Details
58 bytes, text/x-review-board-request
rnewman
: review+
Details
58 bytes, text/x-review-board-request
rnewman
: review+
Details
58 bytes, text/x-review-board-request
rnewman
: review+
Details
Messaging in Bug 1220904, actual removal of the Android Account type here.
This should track 46.
tracking-fennec: --- → ?
Mentor: nalexander
tracking-fennec: ? → 46+
Mentor: rnewman
OS: Unspecified → Android
Hardware: Unspecified → All
Whiteboard: [lang=java][good next bug]
Version: unspecified → Firefox 46
This won't make 46 if we wait for a contributor...
Will those of us who are still using Sync 1.1 (Weave) with our own custom servers be able to turn off Firefox updates so we don't lose the Sync 1.1 client service?
Comment on attachment 8709105 [details]
MozReview Request: Bug 1220906 : Delete deprecated Sync Android Account type r?nalexander

https://reviewboard.mozilla.org/r/31259/#review28045

This looks good, modulo my one substantive issue.  Please check that we don't change the APK permissions (using `aapt dump badging`, etc).

I'm not comfortable landing this without ripping out all the code consumers.  Can you make this part 1, and get to that?  Start with removing Fennec consumers of `SyncAccounts` and the field https://dxr.mozilla.org/mozilla-central/source/mobile/android/services/src/main/java/org/mozilla/gecko/sync/SyncConstants.java#21.  Then there's lots of Old Sync account and setup code that can go, including the Sync-specific pickler and update code.  I can help with questions here.

Thanks for jumping on this!

::: mobile/android/base/AndroidManifest.xml.in
(Diff revision 1)
> -#include ../services/manifests/SyncAndroidManifest_permissions.xml.in

There's a single permission that needs to be kept from here:

https://dxr.mozilla.org/mozilla-central/source/mobile/android/services/manifests/SyncAndroidManifest_permissions.xml.in#24

Move it into the main manifest (rather than into a services manifest, for example).

I checked the services and activities, and they look clean.  Be careful, though...
Attachment #8709105 - Flags: review?(nalexander) → review+
(In reply to Craig Arno from comment #3)
> Will those of us who are still using Sync 1.1 (Weave) with our own custom
> servers be able to turn off Firefox updates so we don't lose the Sync 1.1
> client service?

You can disable automatic updates in Google Play, and you can turn off Firefox's own updates for Nightly and Aurora in Settings.

It's your decision whether you'd rather have performance, stability, and security fixes with Mozilla's Sync setup, do the work to host your own Sync 1.5 service, or stick with Firefox 45 and any bugs and vulnerabilities it has.
The two stages are EnsureClusterURLStage (the FxA + Sync cluster URL
is determined directly from the token provided by the token server),
and MigrationSentinelSyncStage.  This allowed to merge the two
{Base}GlobalSessionCallback classes together, as well as removing the
specializing FxAcccountGlobalSession.

Review commit: https://reviewboard.mozilla.org/r/31345/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/31345/
Attachment #8709229 - Flags: review?(rnewman)
Blocks: 1240919
Comment on attachment 8709222 [details]
MozReview Request: Bug 1220906 - Part 1: Delete deprecated Sync Android Account type. r=nalexander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31331/diff/1-2/
Comment on attachment 8709223 [details]
MozReview Request: Bug 1220906 - Part 2: Deprecate Accounts.syncAccountsExist. r?rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31333/diff/1-2/
Comment on attachment 8709224 [details]
MozReview Request: Bug 1220906 - Part 3: Remove references to SyncAccounts and SyncConstants.ACCOUNTTYPE_SYNC outside of *.sync.*. r?rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31335/diff/1-2/
Comment on attachment 8709225 [details]
MozReview Request: Bug 1220906 - Part 4: Remove tests referencing SyncAccounts. r?rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31337/diff/1-2/
Comment on attachment 8709226 [details]
MozReview Request: Bug 1220906 - Part 5: Remove Old Sync setup and JPAKE code. r?rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31339/diff/1-2/
Comment on attachment 8709227 [details]
MozReview Request: Bug 1220906 - Part 6: Remove Old Sync configuration code. r?rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31341/diff/1-2/
Comment on attachment 8709228 [details]
MozReview Request: Bug 1220906 - Part 7: Miscellaneous purging. r?rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31343/diff/1-2/
Comment on attachment 8709229 [details]
MozReview Request: Bug 1220906 - Part 8: Get rid of Old Sync-specific stages. r?rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31345/diff/1-2/
Comment on attachment 8709222 [details]
MozReview Request: Bug 1220906 - Part 1: Delete deprecated Sync Android Account type. r=nalexander

https://reviewboard.mozilla.org/r/31331/#review28239
Attachment #8709222 - Flags: review+
Comment on attachment 8709223 [details]
MozReview Request: Bug 1220906 - Part 2: Deprecate Accounts.syncAccountsExist. r?rnewman

https://reviewboard.mozilla.org/r/31333/#review28241
Attachment #8709223 - Flags: review?(rnewman) → review+
Comment on attachment 8709224 [details]
MozReview Request: Bug 1220906 - Part 3: Remove references to SyncAccounts and SyncConstants.ACCOUNTTYPE_SYNC outside of *.sync.*. r?rnewman

https://reviewboard.mozilla.org/r/31335/#review28243

::: mobile/android/base/java/org/mozilla/gecko/preferences/SyncPreference.java:103
(Diff revision 2)
>          // Otherwise, launch the FxA "Get started" activity, which will

Fix comment.

::: mobile/android/base/java/org/mozilla/gecko/widget/ActivityChooserModel.java:1313
(Diff revision 2)
> -        if (!FirefoxAccounts.firefoxAccountsExist(mContext) &&
> +        if (!FirefoxAccounts.firefoxAccountsExist(mContext))  {

Nit: double-spaced.

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/AccountLoader.java:47
(Diff revision 2)
> -    Account foundAccount = FirefoxAccounts.getFirefoxAccount(context);
> +    return FirefoxAccounts.getFirefoxAccount(context);

Inline `context`.
Attachment #8709224 - Flags: review?(rnewman) → review+
Attachment #8709225 - Flags: review?(rnewman) → review+
Comment on attachment 8709225 [details]
MozReview Request: Bug 1220906 - Part 4: Remove tests referencing SyncAccounts. r?rnewman

https://reviewboard.mozilla.org/r/31337/#review28245
Comment on attachment 8709226 [details]
MozReview Request: Bug 1220906 - Part 5: Remove Old Sync setup and JPAKE code. r?rnewman

https://reviewboard.mozilla.org/r/31339/#review28247
Attachment #8709226 - Flags: review?(rnewman) → review+
Comment on attachment 8709227 [details]
MozReview Request: Bug 1220906 - Part 6: Remove Old Sync configuration code. r?rnewman

https://reviewboard.mozilla.org/r/31341/#review28249
Attachment #8709227 - Flags: review?(rnewman) → review+
Comment on attachment 8709228 [details]
MozReview Request: Bug 1220906 - Part 7: Miscellaneous purging. r?rnewman

https://reviewboard.mozilla.org/r/31343/#review28251
Attachment #8709228 - Flags: review?(rnewman) → review+
Comment on attachment 8709229 [details]
MozReview Request: Bug 1220906 - Part 8: Get rid of Old Sync-specific stages. r?rnewman

https://reviewboard.mozilla.org/r/31345/#review28253
Attachment #8709229 - Flags: review?(rnewman) → review+
Assignee: nobody → nalexander
Status: NEW → ASSIGNED
Comment on attachment 8709222 [details]
MozReview Request: Bug 1220906 - Part 1: Delete deprecated Sync Android Account type. r=nalexander

This is a lightly modified version of vivek's patch, which already had my r+.
Attachment #8709222 - Flags: review?(nalexander) → review+
Blocks: 1241846
Release Note Request (optional, but appreciated)

[Why is this notable]: removes a large chunk of functionality, which Mozilla has long since deprecated, but which self hosters may still be using.  The number of such self hosters is unknown but very low -- probably less than 10k users.

[Suggested wording]: Removed support for Old Sync accounts based on Firefox Sync 1.1.  Use the new Firefox Account based Sync instead.

[Links (documentation, blog post, etc)]: https://blog.mozilla.org/services/2015/07/31/shutting-down-the-legacy-sync-service/
relnote-firefox: --- → ?
Blocks: 1257901
Product: Android Background Services → Firefox for Android
You need to log in before you can comment on or make changes to this bug.