Last Comment Bug 772645 - Support concurrent Fennecs accessing different Android Account types
: Support concurrent Fennecs accessing different Android Account types
Status: RESOLVED FIXED
[qa+]
:
Product: Android Background Services
Classification: Client Software
Component: Android Sync (show other bugs)
: unspecified
: All Android
P1 normal
: mozilla17
Assigned To: Nick Alexander :nalexander
: android-sync
:
Mentors:
: 768467 (view as bug list)
Depends on: 764867
Blocks: 761206 777800 755437 755638 757121
  Show dependency treegraph
 
Reported: 2012-07-10 14:07 PDT by Nick Alexander :nalexander
Modified: 2014-12-09 07:38 PST (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
-


Attachments
Patch against m-i (3.12 KB, patch)
2012-07-20 16:15 PDT, Nick Alexander :nalexander
rnewman: review+
blassey.bugs: feedback+
Details | Diff | Splinter Review
patch against m-i (3.52 KB, patch)
2012-07-24 21:30 PDT, Nick Alexander :nalexander
no flags Details | Diff | Splinter Review

Description User image Nick Alexander :nalexander 2012-07-10 14:07:13 PDT

    
Comment 1 User image Nick Alexander :nalexander 2012-07-20 16:15:29 PDT
Created attachment 644518 [details] [diff] [review]
Patch against m-i

Part 1/2: make Fennec set Android shared account type depending on package.
Comment 2 User image Nick Alexander :nalexander 2012-07-20 16:17:37 PDT
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 User image Nick Alexander :nalexander 2012-07-20 16:18:02 PDT
> shared user IDs and singing types.  This is a step towards Bug 761206.

signing keys.
Comment 4 User image Nick Alexander :nalexander 2012-07-20 16:42:09 PDT
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 User image Nick Alexander :nalexander 2012-07-20 16:42:27 PDT
Adding ibarlow for UX input.
Comment 6 User image Richard Newman [:rnewman] 2012-07-20 18:15:10 PDT
(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 User image Richard Newman [:rnewman] 2012-07-20 18:23:02 PDT
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 User image Brad Lassey [:blassey] (use needinfo?) 2012-07-22 20:33:22 PDT
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 User image Nick Alexander :nalexander 2012-07-23 11:16:42 PDT
Adding Sean Martell for input on branding.
Comment 10 User image Nick Alexander :nalexander 2012-07-24 21:30:19 PDT
Created attachment 645644 [details] [diff] [review]
patch against m-i

Updated to incorporate blassey's feedback.
Comment 11 User image Nick Alexander :nalexander 2012-07-25 15:38:55 PDT
Depends on Bug 764867 for branding, so that you can distinguish the various Accounts and Send Tab providers.
Comment 13 User image Nick Alexander :nalexander 2012-07-26 09:46:52 PDT
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 nalexander+test0724@mozilla.com 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: nalexander+test0724@mozilla.com

16. verify that you can share to both "Firefox Sync" options, and the pages get delivered to the correct Sync accounts.
Comment 15 User image Robert Kaiser 2012-08-02 08:01:21 PDT
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 User image Richard Newman [:rnewman] 2012-08-02 09:46:09 PDT
(In reply to Robert Kaiser (:kairo@mozilla.com) 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 User image Lukas Blakk [:lsblakk] use ?needinfo 2012-08-20 19:24:25 PDT
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.
Comment 18 User image Richard Newman [:rnewman] 2014-12-09 07:38:55 PST
*** Bug 768467 has been marked as a duplicate of this bug. ***

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