917942 - Create a JS API for Sync accounts

Status: RESOLVED FIXED

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

Description

[:Margaret Leibovic]

I want to re-implement the sync promo banner using the new home banner API, and to do that, I want to know in JS-land whether or not sync is set up.

Comment 1

[Richard Newman [:rnewman]]

Attachment: Proposed patch. v1

Here, give this a shot...

Comment 2

[:Margaret Leibovic]

Comment on attachment 806811 [details] [diff] [review]
Proposed patch. v1

Is there a reason you made this into a helper script as opposed to a jsm? I feel like this would be better as /mobile/android/modules/AccountsHelper.jsm because then it could be loaded as a standalone thing. Most of the reason we have things in lazy-loaded scripts is because they depend on other things in browser.js, so we can't actually run them independently.

This looks like it's on the right track, but it didn't work because it expects JNI to already be defined, and when I imported JNI.jsm in my add-on, I ran into this error:
TypeError: JNI.GetForThread is not a function (chrome://browser/content/AccountsHelper.js:47)" {file: "chrome://browser/content/AccountsHelper.js" line: 47}]

Here's the add-on I'm using to test, in case you're curious:
https://github.com/leibovic/promo-banner/blob/master/bootstrap.js

Comment 3

[Richard Newman [:rnewman]]

Attachment: Extend JNI.jsm. v1

Untested.

Comment 4

[Richard Newman [:rnewman]]

Attachment: Proposed patch. v2

Shifted to a module. Still untested :P

Comment 5

[:Margaret Leibovic]

Re-noming, we'll want this when the new sync ships, so we can only show the sync promo banner if sync isn't set up.

Comment 6

[:Margaret Leibovic]

This isn't just for add-ons. This would give us a less fragile way to do this in JS (encapsulating the JNI stuff behind an API).

Comment 7

[Nick Alexander :nalexander [he/him]]

This is more interesting, now that we have multiple Android Account types.  We now can match

(Legacy Sync)?|(Firefox Account)*

on the system.  I'm going to assert that we won't ever allow both account types at the same time (but the code here should be robust enough to handle that case) and it's unlikely that we'll allow (Firefox Account){2,} anytime soon.

That suggests returning a list of Accounts, each an object with a type.  Most callers will just query if the list is non-empty, for which we might provide a helper function.

Comment 8

[Nick Alexander :nalexander [he/him]]

As part of Bug 958891, we'll want this to grow the ability to open the Sync|FxA Getting Started activity.

Comment 9

[Richard Newman [:rnewman]]

Attachment: Create a JS interface to Sync configuration.

Comment 10

[Richard Newman [:rnewman]]

Comment on attachment 8377381 [details] [diff] [review]
Create a JS interface to Sync configuration.

Tested on my device.

I spent a bunch of time on the JNI approach to no avail.

N.B., this uses promises. Spinning the event loop to wait for the result (exactly as does SharedPreferences.jsm) caused stalls up to 30 seconds long on my phone, and is a terrible idea anyway, so rock on with your bad async self.

Comment 11

[Richard Newman [:rnewman]]

N.B., this implementation depends on Bug 946344's GeckoEventResponder changes. It'll need adjustment to land on 29, or that bug will need to be uplifted.

Comment 12

[:Margaret Leibovic]

Comment on attachment 8377381 [details] [diff] [review]
Create a JS interface to Sync configuration.

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

::: mobile/android/base/BrowserApp.java
@@ +1268,5 @@
> +                    EventDispatcher.sendResponse(message, response);
> +                } else {
> +                    response.put("error", "Unknown kind");
> +                    EventDispatcher.sendError(message, response);
> +                }

Sigh, more stuff in BrowserApp. Part of me thinks we should make a new class for this to live in, but I don't know if that should just be punted to part of a greater cleanup effort.

::: mobile/android/modules/Accounts.jsm
@@ +18,5 @@
> + *
> + * Usage:
> + *
> + *   Cu.import("resource://gre/modules/Accounts.jsm");
> + *   Accounts.anySyncAccountsExist(

This should be Accounts.anySyncAccountsExist().then(

Comment 13

[:Margaret Leibovic]

Also, can we write a test for this?

Comment 14

[Richard Newman [:rnewman]]

With tests, no less!

https://hg.mozilla.org/integration/fx-team/rev/f51db1b6c9a6

needinfo on me for uplift.

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Had to back this out of fx-team in https://hg.mozilla.org/integration/fx-team/rev/36e0669fa1dd for breaking rc4 on Android: https://tbpl.mozilla.org/php/getParsedLog.php?id=34887717&tree=Fx-Team

Comment 16

[Richard Newman [:rnewman]]

Pah. Tests passed locally but not remotely because the setup activity was just taking too long to launch.

(Yes, I'm one of the tiny set of people who actually runs the Robocop tests before landing.)

Commented out the failing part of the test and relanded:

https://hg.mozilla.org/integration/fx-team/rev/2af0db58021b

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2af0db58021b

Comment 18

[Richard Newman [:rnewman]]

Comment on attachment 8377381 [details] [diff] [review]
Create a JS interface to Sync configuration.

Requesting uplift. Needed for Sync promo on 29. No strings. Happily baked on m-c for a while.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ac1f14b91331

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Backed out for bustage (depends on bug 946344 IIUC). Please post a branch-specific patch for uplift here or nominate that for uplift as well.
https://hg.mozilla.org/releases/mozilla-aurora/rev/a4d31bafa9a6

https://tbpl.mozilla.org/php/getParsedLog.php?id=35149110&tree=Mozilla-Aurora

Comment 21

[Richard Newman [:rnewman]]

Attachment: Patch for uplift.

Comment 22

[Richard Newman [:rnewman]]

Comment on attachment 8380795 [details] [diff] [review]
Patch for uplift.

Flagging margaret to double-check that I Auroraified this correctly. Tests pass.

Comment 23

[:Margaret Leibovic]

Comment on attachment 8380795 [details] [diff] [review]
Patch for uplift.

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

::: mobile/android/modules/Accounts.jsm
@@ +52,5 @@
> +      kind: kind,
> +    }, (data, error) => {
> +      if (error) {
> +        deferred.reject(error);
> +        return;

I don't think there was an error param in the old world, so I'm not sure that this will ever get called. But I suppose that's okay, since you're handling the !data case below.

Comment 24

[Richard Newman [:rnewman]]

Thanks, Margaret!

Comment 25

[Richard Newman [:rnewman]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8e7a694ef27b