772645 - Support concurrent Fennecs accessing different Android Account types

Status: RESOLVED FIXED

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

Comment 1

[Nick Alexander :nalexander [he/him]]

Attachment: Patch against m-i

Part 1/2: make Fennec set Android shared account type depending on package.

Comment 2

[Nick Alexander :nalexander [he/him]]

Comment on attachment 644518 [details] [diff] [review]
Patch against m-i

Including mfinkle and blassey for optional feedback, just in case there's an unseen issue with making the account type match the (released in the wild) shared user IDs and singing types.  This is a step towards Bug 761206.

Comment 3

[Nick Alexander :nalexander [he/him]]

> shared user IDs and singing types.  This is a step towards Bug 761206.

signing keys.

Comment 4

[Nick Alexander :nalexander [he/him]]

There are a few places this change will be user facing, and I'd like to broaden the discussion of what this might look like to the user.

1. we will need to differentiate Account types in Accounts & Settings.

I suggest the following labels:

"Firefox Sync" for (official and beta)
"Firefox Sync for Aurora and Nightly" (for Aurora and Nightly)
"Firefox Sync for Developers" (for developer Fennec builds)

We might want Sync iconography that includes the Fx branding too.

The first one is what we have now.  The second one is probably too long.  The third one should never be visible to the public.

2. we will need to provide some consistency between Send Tab activities.

official and beta will share an Account (good) but both will publish a Send Tab Share intent (bad, since there's now duplication in the Share list).  rnewman thinks that users won't differentiate between Send Tab providers, so these should have the same name and icon.  But if we add features to Send Tab in a future version, there will be no way for the user to distinguish the providers.

Comment 5

[Nick Alexander :nalexander [he/him]]

Adding ibarlow for UX input.

Comment 6

[Richard Newman [:rnewman]]

(In reply to Nick Alexander :nalexander from comment #4)

> The first one is what we have now.  The second one is probably too long. 

Perhaps "Firefox Sync (pre-Beta)"? Or just "Firefox Sync" with a different icon — Aurora, or with a "beta" banner across it?

> official and beta will share an Account (good) but both will publish a Send
> Tab Share intent (bad, since there's now duplication in the Share list). 
> rnewman thinks that users won't differentiate between Send Tab providers, so
> these should have the same name and icon.

Kinda, and unless there are two of them.

The main issue is that if we customize the name for that activity to avoid confusion, it'll *also* apply when there's only one app involved, which looks janky.

So I would suggest:

* Per-app icon, matching the app
* Same name
* Attempt to filter for the current running Firefox activity, if we're sharing from inside Firefox.

Comment 7

[Richard Newman [:rnewman]]

Comment on attachment 644518 [details] [diff] [review]
Patch against m-i

Review of attachment 644518 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/Makefile.in
@@ +232,5 @@
>  ifeq ($(MOZ_APP_NAME),fennec)
>  ICON_PATH = $(topsrcdir)/$(MOZ_BRANDING_DIRECTORY)/content/fennec_48x48.png
>  ICON_PATH_HDPI = $(topsrcdir)/$(MOZ_BRANDING_DIRECTORY)/content/fennec_72x72.png
> +DEFINES += -DMOZ_ANDROID_SHARED_ID="$(ANDROID_PACKAGE_NAME).sharedID"
> +DEFINES += -DMOZ_ANDROID_SHARED_ACCOUNT_TYPE="$(ANDROID_PACKAGE_NAME)_sync"

This will add MOZ_ANDROID_SHARED_ACCOUNT_TYPE to DEFINES twice: once if the app name is "fennec", and once in the clause below. It just so happens that they're the same computed value, but I'd rather be neater about this, because copypasta is evil.

Comment 8

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 644518 [details] [diff] [review]
Patch against m-i

Review of attachment 644518 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/Makefile.in
@@ +232,5 @@
>  ifeq ($(MOZ_APP_NAME),fennec)
>  ICON_PATH = $(topsrcdir)/$(MOZ_BRANDING_DIRECTORY)/content/fennec_48x48.png
>  ICON_PATH_HDPI = $(topsrcdir)/$(MOZ_BRANDING_DIRECTORY)/content/fennec_72x72.png
> +DEFINES += -DMOZ_ANDROID_SHARED_ID="$(ANDROID_PACKAGE_NAME).sharedID"
> +DEFINES += -DMOZ_ANDROID_SHARED_ACCOUNT_TYPE="$(ANDROID_PACKAGE_NAME)_sync"

one way to fix this is to set a MOZ_ANDROID_SHARED_ACCOUNT_TYPE variable in this logic and then below it do:

ifdef MOZ_ANDROID_SHARED_ACCOUNT_TYPE
DEFINES += -DMOZ_ANDROID_SHARED_ACCOUNT_TYPE="$(MOZ_ANDROID_SHARED_ACCOUNT_TYPE)"
endif

Comment 9

[Nick Alexander :nalexander [he/him]]

Adding Sean Martell for input on branding.

Comment 10

[Nick Alexander :nalexander [he/him]]

Attachment: patch against m-i

Updated to incorporate blassey's feedback.

Comment 11

[Nick Alexander :nalexander [he/him]]

Depends on Bug 764867 for branding, so that you can distinguish the various Accounts and Send Tab providers.

Comment 12

[Nick Alexander :nalexander [he/him]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/28106e179603
https://hg.mozilla.org/integration/mozilla-inbound/rev/6010529ba8fb

Comment 13

[Nick Alexander :nalexander [he/him]]

To verify:

This ticket lets you concurrently use Sync with one Firefox from each of the following sets:

{official, beta}
{aurora, nightly}
{developer}

1. install official or beta (from Google Play)
2. install Nightly
3. navigate to Android > Settings > Accounts & sync > Add account
4. verify that there are two "Firefox Sync" accounts with *different* icons: one should be the old "Gray Rotating Arrows", one should be the Nightly logo.
5. select the Nightly logo one.
6. pair Nightly with existing Sync account.
7. verify selecting the Nightly button (instead of Settings) takes you to Nightly.
8. verify sync worked in Nightly (history, bookmarks, etc).
9. verify logs like

I FxSync(13050)               fennec :: SyncAdapter :: Syncing account named [e-mail] for client named 'Fennec ncalexan on GT-I9100' with client guid mPRy-KJGydpS (sync account has 0 clients).

10. start Nightly again, and verify "Set up Sync" button IS NOT shown.
11. start Official, and verify "Set up Sync" button IS shown.
12. select "Set up Sync".
13. pair Official with existing Sync account (different than above to test there's no accidental overlap, if possible, but the same account should work fine).
14. verify sync worked in Official (history, etc).
15. verify logs like

I SyncAdapter(13318)          Got onPerformSync. Extras bundle is Bundle[{}]
I SyncAdapter(13318)          Account name: [e-mail]

16. verify that you can share to both "Firefox Sync" options, and the pages get delivered to the correct Sync accounts.

Comment 14

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/28106e179603
https://hg.mozilla.org/mozilla-central/rev/6010529ba8fb

Comment 15

[Robert Kaiser]

Bug 755638 says that it fixed the crashes there, and we're still seeing quite a number of those on Aurora 16, so I think we should track and land this for that train as well.

Comment 16

[Richard Newman [:rnewman]]

(In reply to Robert Kaiser (:[e-mail]) from comment #15)
> Bug 755638 says that it fixed the crashes there, and we're still seeing
> quite a number of those on Aurora 16, so I think we should track and land
> this for that train as well.

Apparently all of our crash-reporting users had a Nightly or developer build installed.

This change as landed -- that is, on Nightly but nowhere else -- puts Nightly and Aurora in different sets. If we were to land it on Aurora, Nightly and Aurora would end up back in the same set (see Comment 13), and the crash would reoccur.

The actual fix for the crash is Bug 777800, which is part of the full multi-version support in Bug 761206.

I would like to see this code in Aurora, but not rushed or incomplete.

Comment 17

[Lukas Blakk [:lsblakk] use ?needinfo]

With comment 16 I'm untracking this for 16 then.  We're about a week away from merge and 16 will become Beta, so when the time comes nominate for Aurora as needed but it may not be 16.