Closed Bug 1030227 Opened 8 years ago Closed 8 years ago

FxA is not firing onlogin for RPs such as FMD which have called watch()

Categories

(Firefox OS Graveyard :: FxA, defect)

Product:
Firefox OS Graveyard
Component:
FxA
Platform:
ARM
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(blocking-b2g:2.0+, firefox31 wontfix, firefox32 fixed, firefox33 fixed, b2g-v2.0 fixed, b2g-v2.1 fixed)

Status:
RESOLVED FIXED
Milestone:
2.0 S5 (4july)
Project Flags:
blocking-b2g 2.0+
Tracking Flags:
Tracking Status
firefox31 --- wontfix
firefox32 --- fixed
firefox33 --- fixed
b2g-v2.0 --- fixed
b2g-v2.1 --- fixed

People

(Reporter: spenrose, Assigned: spenrose)

References

Details

Attachments

(1 file, 1 obsolete file)

1030227-fire-onlogin.patch
3.12 KB, patch
Details | Diff | Splinter Review 
No description provided.
blocking-b2g: --- → 2.0?
Attached patch 1030227-fire-onlogin.patch (obsolete) — — Splinter Review 
As a follow-up, I think the getAssertion().then(rp.doLogin()) bit should be broken out into a common routine, but ONLOGIN -> request() seems safe to me for now. Thanks Jed!
Attachment #8446008 - Flags: review?(jparsons)
Target Milestone: --- → 2.0 S5 (4july)
blocking-b2g: 2.0? → 2.0+
Comment on attachment 8446008 [details] [diff] [review]
1030227-fire-onlogin.patch

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

I don't think we have tests for any of fxaccounts:onlogin, :onlogout, or :onverified, do we?  We ought to ensure that they result in onlogin() etc. being fired for an RP.  And we should ensure that these sequences are on the checklist of manual tests to perform.

I agree that some refactoring could be done with getAssertion().then(rp.doLogin()).

One little fix, apart from which, this looks good to me!  Thanks, Sam.

::: toolkit/identity/FirefoxAccounts.jsm
@@ +90,5 @@
>          for (let [rpId,] of this._rpFlows) {
>            this.doLogout(rpId);
>          }
>          break;
>        case "quit-application-granted":

After the test for ONVERIFIED_NOTIFICATION, can you remove the observers for ONLOGIN and also ONLOGOUT, which seems to have been missed in some prior patch?
Attachment #8446008 - Flags: review?(jparsons) → review+
Thanks Jed, especially for catching that leak. Agreed on all points.
Attachment #8446008 - Attachment is obsolete: true
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.