Closed Bug 1224708 Opened 6 years ago Closed 6 years ago

Retrieve FxAccount in background thread.

Categories

(Firefox for Android Graveyard :: General, defect)

45 Branch
defect
Not set
normal

Tracking

(firefox45 affected, firefox46 fixed)

RESOLVED FIXED
Firefox 46
Tracking Status
firefox45 --- affected
firefox46 --- fixed

People

(Reporter: vivek, Assigned: vivek, Mentored)

References

Details

Attachments

(2 files, 1 obsolete file)

This is a followup to bug 1189356 to address the review comments https://bugzilla.mozilla.org/show_bug.cgi?id=1189356#c14
Bug 1224708 : FxAccount check done in background thread. r?nalexander
Attachment #8687400 - Flags: review?(nalexander)
Comment on attachment 8687400 [details]
MozReview Request: Bug 1224708 :  Update SyncPreference asynchronously using Loaders r?nalexander

vivek and I talked today, and we're going to explore using AccountLoader for this rather than managing Task instances locally.
Attachment #8687400 - Flags: review?(nalexander)
Blocks: 1189356
Attachment #8687400 - Attachment description: MozReview Request: Bug 1224708 : FxAccount check done in background thread. r?nalexander → MozReview Request: Bug 1224708 : Update SyncPreference asynchronously using Loaders r?nalexander
Attachment #8687400 - Flags: review?(nalexander)
Comment on attachment 8687400 [details]
MozReview Request: Bug 1224708 :  Update SyncPreference asynchronously using Loaders r?nalexander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25197/diff/1-2/
Blocks: 1225099
Attached patch 1224708.patch (obsolete) — Splinter Review
@Nick/Margaret:
This is an alternate solution to my previous patch and the preference UI updated through SharedPreferences callback.
This solution tries to avoid any dependency between GeckoPrefefenceActivity/Fragment and SyncPreference i.e
SyncPreference reference is not cached or searched for in the above activity/fragment

This patch could potentialy leak the listener in onPrepareForRemoval() which may never be called.

I'm not sure what is the right approach here, please advice.
Attachment #8691528 - Flags: review?(nalexander)
Attachment #8691528 - Flags: review?(margaret.leibovic)
Attached patch 1224708.patchSplinter Review
Sorry for spamming.Uploading the correct patch
Attachment #8691528 - Attachment is obsolete: true
Attachment #8691528 - Flags: review?(nalexander)
Attachment #8691528 - Flags: review?(margaret.leibovic)
Attachment #8691535 - Flags: review?(nalexander)
Attachment #8691535 - Flags: review?(margaret.leibovic)
Comment on attachment 8687400 [details]
MozReview Request: Bug 1224708 :  Update SyncPreference asynchronously using Loaders r?nalexander

https://reviewboard.mozilla.org/r/25197/#review23743

nits, and a question (not a blocker).  I *much* prefer this approach with the coupling to the alternate with `SharedPreferences`.  Nice patch!

::: mobile/android/base/preferences/GeckoPreferenceFragment.java:187
(Diff revision 2)
> +        // Forcing reload as the account may have been deleted while the app was in background.

nit: "Force a reload..." or "Force reload...".

Is this necessary?  I feel like the `Loader` should "see" this if it exists and already do the right thing.  Is the issue that the loader sees the event, but it's not really attached when the activity is in the background, so the update doesn't propagate?

Teach me about loaders!

::: mobile/android/base/preferences/GeckoPreferenceFragment.java:260
(Diff revision 2)
> +        return (SyncPreference) getPreferenceManager().findPreference(GeckoPreferences.PREFS_SYNC);

Let's move the null check here and throw `IllegalStateException` if the pref can't be found, so that we can drop the null checks elswewhere.

::: mobile/android/base/preferences/GeckoPreferenceFragment.java:281
(Diff revision 2)
> +            syncPreference.update(new AndroidFxAccount(getActivity(), account));

nit: drop newline.
Attachment #8687400 - Flags: review?(nalexander) → review+
Comment on attachment 8691535 [details] [diff] [review]
1224708.patch

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

Sorry this took so long to get attention.  I much prefer the alternate patch, so let's polish that up, test it, and land it!
Attachment #8691535 - Flags: review?(nalexander)
Attachment #8691535 - Flags: review?(margaret.leibovic)
Attachment #8691535 - Flags: feedback-
Comment on attachment 8687400 [details]
MozReview Request: Bug 1224708 :  Update SyncPreference asynchronously using Loaders r?nalexander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25197/diff/2-3/
Comment on attachment 8687400 [details]
MozReview Request: Bug 1224708 :  Update SyncPreference asynchronously using Loaders r?nalexander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25197/diff/3-4/
https://reviewboard.mozilla.org/r/25197/#review24187

My browser state may be confused;  this says I have a pending review.  Posting it, and vivek and I talked on IRC and I think this is fine.

::: mobile/android/base/preferences/GeckoPreferenceFragment.java:272
(Diff revision 2)
> +            if (getSyncPreference() == null) {

This should be the saved `syncPreference`.
https://hg.mozilla.org/integration/fx-team/rev/d5193b7ae7c54aea4deaa8bfbc27a07aa45783de
Bug 1224708 :  Update SyncPreference asynchronously using Loaders r=nalexander
https://hg.mozilla.org/mozilla-central/rev/d5193b7ae7c5
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
Duplicate of this bug: 1225099
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.