Enable Firefox Accounts Profile Image in sync pref view and don't cache image

RESOLVED FIXED in Firefox 41

Status

()

defect
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: edwong, Assigned: markh)

Tracking

Trunk
Firefox 41
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox41 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Logging this pending approval to enable profile images in nightly. Disable to read from local cache (bug 1150344) for now in order to test feature UX in Nightly
Ryan how do you feel about flipping this on in nightly fetching the image from remote each time so we get early eyes on the feature?
Flags: needinfo?(rfkelly)
Blocks: 1127540
Seems sane enough as long as it's only in nightly; will the fetch happen once at browser startup and be cached in memory, or happen every time someone opens prefs view?
Flags: needinfo?(rfkelly)
(In reply to Ryan Kelly [:rfkelly] from comment #2)
> Seems sane enough as long as it's only in nightly; will the fetch happen
> once at browser startup and be cached in memory, or happen every time
> someone opens prefs view?

TBD - ideally the former.

Zach, this is a very early WIP, but I'm putting it up so you can see what I'm thinking. This is mainly concerned with the token fetching and expiry.

I've moved the profile code out of the currentAccountState - IMO it should be removed anyway (along with getCertificate etc), but the main reason is that the token caching code is on the FxA object and should remain there. So instead of passing the accountState around we pass FxA.

The caching of the data is directly on the FxAccountsProfile object, so this means that calling getSignedInUser() will *not* return the profile data - callers will need to explicitly call getProfile().  We can fix that when we fix the storage - and given the short-term plan to fix storage and re-cache the value to disk, I've left chunks of the tests commented out (referencing the storage bug) rather than removing - it should help to "undo" some of the test hacks later.

Like I said, an early WIP I haven't even self-reviewed yet :) It also needs new tests for the token changes, but existing ones all pass, but all feedback still welcome.
Assignee: nobody → markh
Status: NEW → ASSIGNED
Attachment #8615214 - Flags: feedback?(zack.carter)
Almost all tests work, and includes flipping the pref to true. And it actually works in the Sync prefs pane :)
Attachment #8615214 - Attachment is obsolete: true
Attachment #8615214 - Flags: feedback?(zack.carter)
Attachment #8615870 - Flags: feedback?(zack.carter)
FxA train is being tested now and should go live on 6/16. Let's target 6/16 for landing this. It's not critical they are in sync but would be nice for testing end to end.
Blocks: 1139698
:zaach any updates on the review so we can push this by 6/16?
Flags: needinfo?(zack.carter)
Comment on attachment 8615870 [details] [diff] [review]
0001-Bug-1171253-enable-FxA-profile-image-WIP.patch

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

This looks good Mark!

::: services/fxaccounts/FxAccountsProfile.jsm
@@ +71,5 @@
>  }
>  
> +this.FxAccountsProfile = function (fxa, options = {}) {
> +  this._cachedProfile = null;
> +  this.fxa = fxa; // pass in options?????

Using options would make the API a little more flexible.

::: services/fxaccounts/FxAccountsProfileClient.jsm
@@ +149,5 @@
>          } else {
> +          return reject(new FxAccountsProfileClientError({
> +            error: body.error || ERROR_UNKNOWN,
> +            errno: body.errno || ERRNO_UNKNOWN_ERROR,
> +            code: request.response.status, // this overrides body.code - is that ok?????

Yeah, body.code, when present, should be the same.
Attachment #8615870 - Flags: feedback?(zack.carter) → feedback+
Flags: needinfo?(zack.carter)
Attachment #8615870 - Attachment is obsolete: true
Attachment #8621459 - Flags: review?(zack.carter)
Comment on attachment 8621459 [details] [diff] [review]
0001-Bug-1171253-enable-FxA-profile-image-in-Sync-prefere.patch

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

This looks good overall; just a few nits.

::: browser/app/profile/firefox.js
@@ +1830,5 @@
>  // The remote URL of the FxA OAuth Server
>  pref("identity.fxaccounts.remote.oauth.uri", "https://oauth.accounts.firefox.com/v1");
>  
> +// Whether we display profile images in the UI or not.
> +pref("identity.fxaccounts.profile_image.enabled", true);

Yay!

::: services/fxaccounts/tests/xpcshell/test_accounts.js
@@ +963,1 @@
>  add_test(function test_accountState_getProfile() {

Might want to change this test name to reference getSignedInUserProfile rather than accountState_getProfile.

::: services/fxaccounts/tests/xpcshell/test_profile.js
@@ +53,5 @@
>  };
>  
> +let mockClient = function (fxa) {
> +  let options = {
> +    serverURL: "http://127.0.0.1:1111/v1",

This option is not actually used-- profileServerUrl is used to initialize the profileClient.

@@ +87,5 @@
> +    fxa = mockFxa();
> +  }
> +  let options = {
> +    fxa: fxa,
> +    serverURL: "http://127.0.0.1:1111/v1",

Same here; can remove serverURL.

@@ +235,5 @@
>    accountData.setUserAccountData = function (data) {
>      do_check_null(data.profile.avatar);
>      run_next_test();
>      return Promise.resolve();
>    };

We can probably get rid of this whole block since you have the check in the observer now.
Attachment #8621459 - Flags: review?(zack.carter) → review+
:rfkelly and I agree we're all clear to push to nightly upon r+
https://hg.mozilla.org/mozilla-central/rev/7e03a233b801
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Product: Core → Firefox
Target Milestone: mozilla41 → Firefox 41
You need to log in before you can comment on or make changes to this bug.