Closed Bug 1139657 Opened 5 years ago Closed 5 years ago

Expose a method on FxAccounts for retrieving profile information

Categories

(Firefox :: Firefox Accounts, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 39
Tracking Status
firefox39 --- fixed

People

(Reporter: zaach, Assigned: zaach)

References

Details

Attachments

(1 file, 5 obsolete files)

The FxAccounts module should expose a way for consumers to retrieve profile information for the currently signed in account. Currently the only useful data is the profile image but this should expand to include a display name soon.
Attachment #8572906 - Attachment is obsolete: true
Attachment #8572906 - Flags: review?(mhammond)
Attachment #8572907 - Flags: review?(mhammond)
This should use the OAuth token caching (currently being discussed on IRC) once that's ready. Right now it  requests a new one per-browser session, per-signed-in account.
Blocks: 1139677
Blocks: 1139698
Comment on attachment 8572907 [details] [diff] [review]
Expose a method on FxAccounts for retrieving profile information

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

We discussed this a little on IRC, but I think some of this is too focused on the profileImage rather than the entire profile.  Eg, we will eventually want to get stuff like "display name" from the profile, and we'll probably want a cache-and-validate strategy for the profile in it's entirety - and the profileImage URL should fall naturally from that.  Something like:

* cache the entire profile.
* as soon as the profile is requested use the cached data, but also...
* ... unconditionally re-fetch the profile and unconditionally issue a profile-changed notification. Later, we'd change this to "issue a conditional request for new profile info using an etag and only issue the profile-changed notification if it actually is modified.
* expand the profile change notification to simply be "something in the profile changed" and not that just the image changed (ie, if the user changed their display name and avatar, both should obviously update)

This means at startup there is a risk of stale info being used for a few seconds, but this is also going to happen when Fx is already running when the profile changes - there'll be some period when the stale data is in the UI, but eventually the profile-change notification will correct it.

The end result is that the patch should probably just treat the entire profile object as a JSON blob with no special casing of the image URL.

Zaach, does that sound like what we discussed and sound OK to you?

::: services/fxaccounts/FxAccounts.jsm
@@ +221,5 @@
> +      .then(() => this.profile.getProfileImageUrl());
> +  },
> +
> +  // Instantiate a FxAccountsProfile with a fresh OAuth token if needed
> +  initProfile: function (options) {

it looks like this should be done like whenKeysReadyDeferred so that we don't end up with 2 in-flight promises when the profile is requested.

@@ +233,5 @@
> +      scope: "profile:avatar"
> +    };
> +
> +    if (this.profile) {
> +      return this.resolve(this.profile);

Similarly, I wonder if exposing |options| here makes sense - what happens if this is called twice with different options?  It looks like the second call would ignore the options and just reuse the profile fetched in the previous call with different options.  (Or if it just for testing, maybe add a comment saying that - I haven't got to the tests yet ;)

::: services/fxaccounts/FxAccountsProfile.jsm
@@ +1,4 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +

I should have remembered to say this before (but I always forget for new modules I create too), but "use strict;" should be added.

@@ +51,5 @@
> +FxAccountsProfile.prototype = {
> +  _Request: RESTRequest,
> +
> +  tearDown: function () {
> +    this.stopListeningForProfileChanges();

I wonder if we should just make this listener a "singleton" to avoid the teardown complexity and the risk of multiple channels if a teardown is missed?  IIUC, this listener just sends an observer notification and the code observing will still need to do the "get new profile info" dance, so I don't think it's that big of a deal if the profile listener stays alive even after the user logs out - listeners for the observer will either have stopped listening, or abort early due to the lack of a signedInUser.

@@ +57,5 @@
> +    this.client = null;
> +  },
> +
> +  // Make a HEAD request to see if the image is available
> +  isImageAvailable: function (url) {

it would probably be helpful to use a leading underscore on "internal" methods, to help make it clearer what are implementation details and what is the actual interface.

@@ +102,5 @@
> +
> +  // Initialize a profile channel to listen for account changes.
> +  listenForProfileChanges: function () {
> +    let contentUri = this.contentUrl;
> +    debugger;

kill this
Attachment #8572907 - Flags: review?(mhammond) → feedback+
Attachment #8572907 - Attachment is obsolete: true
Attachment #8574005 - Flags: review?(mhammond)
Attachment #8574005 - Attachment is obsolete: true
Attachment #8574005 - Flags: review?(mhammond)
Attachment #8574067 - Flags: review?(mhammond)
Fixed a failing test.
Attachment #8574067 - Attachment is obsolete: true
Attachment #8574067 - Flags: review?(mhammond)
Attachment #8574140 - Flags: review?(mhammond)
Attachment #8574140 - Attachment is obsolete: true
Attachment #8574140 - Flags: review?(mhammond)
Attachment #8574395 - Flags: review?(mhammond)
Comment on attachment 8574395 [details] [diff] [review]
Expose a method on FxAccounts for retrieving profile information

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

I think this looks good with the requested changes.

::: services/fxaccounts/FxAccounts.jsm
@@ +240,5 @@
> +      return this.resolve(this.profile);
> +    }
> +
> +    if (!this.initProfileDeferred) {
> +      this.initProfileDeferred = this.fxaInternal.getOAuthToken(oAuthOptions)

I think initProfileDeferred is a promise rather than a deferred, so this.initProfileDeferred.reject() above will fail.  It sucks this module is using deprecated "deferred" objects, but it should be possible to use PromiseUtils.jsm to create and use a deferred object here which chains to the real promise.

But thinking some more about this, I don't think we we need this.initProfileDeferred at all (and yeah, I know I asked for it :)  whenVerifiedDeferred etc are used outside the AccountState but this isn't - so I actually think we could just use a promise and the tweak below would make it safe.

(ie, just have this.initProfilePromise be a regular promise (which it already is in your patch) and skip the attempt to reject it in abort()), and use accountState.resolve below.)

@@ +1090,5 @@
> +          let profile = JSON.parse(JSON.stringify(profileData));
> +          // profileData doesn't include "verified", but it must be true
> +          // if we've gotten this far.
> +          profile.verified = true;
> +          return profile;

I think simply doing return accountState.resolve(profile) here will protect all we need (ie, cause rejection if the signed in user changed before this completed) and allow us to avoid needing a deferred object above.

@@ +1098,5 @@
> +
> +          return this.getSignedInUser().then(data => {
> +            // If we fail to fetch the profile and have no profile cached
> +            // we resort to using the account data for basic profile data.
> +            return {

and here too (although I wonder how useful this fallback is - the caller must already have access to the signed in user data, so I'd think it better they fallback if it makes sense in their context) - but I'm not too bothered if you think this is better.

::: services/fxaccounts/FxAccountsProfile.jsm
@@ +109,5 @@
> +  // Cache fetched data if it is different from what's in the cache.
> +  // Send out a notification if it has changed so that UI can update.
> +  _cacheProfile: function (profileData) {
> +    let currentState = this.currentAccountState;
> +    return currentState.getUserAccountData()

It seems there may be a race when the user has logged out while the profile data is being fetched - ie, that currentState will be null here.  Simply early-returning here should be fine, and the other requested change in FxAccounts.jsm (ie, to use accountState.resolve()) would mean this is safe.

::: services/fxaccounts/tests/xpcshell/test_accounts.js
@@ +899,5 @@
> +    accountState.profile = mockProfile;
> +
> +    accountState.getProfile()
> +      .then(result => {
> +         do_check_true(!! result);

Please remove the space after the operators
Attachment #8574395 - Flags: review?(mhammond) → review+
Blocks: 1147047
Backed out in:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf60b84ee258
for xpcshell failures.  (See commit message of backout.)
Flags: needinfo?(zack.carter)
Hm. That error didn't show up in my try build from Monday: https://treeherder.mozilla.org/#/jobs?repo=try&revision=78255b2ea4b2

I've started new one here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ae148cd0901c
Flags: needinfo?(zack.carter)
markh helped me spot the failure. Fixed in the latest patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9aa76ca13b17
https://hg.mozilla.org/mozilla-central/rev/d2ac6c4c13bd
Assignee: nobody → zack.carter
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Depends on: 1153708
Product: Core → Firefox
Target Milestone: mozilla39 → Firefox 39
You need to log in before you can comment on or make changes to this bug.