Closed Bug 806605 Opened 7 years ago Closed 7 years ago

Replace loggedInEmail parameter with loggedInUser

Categories

(Core Graveyard :: Identity, enhancement)

enhancement
Not set

Tracking

(blocking-basecamp:+, firefox18 fixed, firefox19 fixed)

RESOLVED FIXED
mozilla19
blocking-basecamp +
Tracking Status
firefox18 --- fixed
firefox19 --- fixed

People

(Reporter: jedp, Assigned: jedp)

References

Details

(Whiteboard: [qa-])

Attachments

(2 files, 4 obsolete files)

MDN docs indicate that loggedInEmail should be renamed to loggedInUser (https://developer.mozilla.org/en-US/docs/DOM/navigator.id.watch#Parameters)

identity code should probably be updated to use loggedInUser and, if that is not defined, fall back to loggedInEmail
Zach, would you mind giving this a quick look?
Attachment #676375 - Flags: feedback?(zack.carter)
(In reply to Zachary Carter [:zaach] from comment #1)
> Here's how the shim handles it:
> https://github.com/mozilla/browserid/blob/dev/resources/static/include_js/
> include.js#L1092

Ah - yes, a checkRenamed function looks like a good safety.  I'll add that.
Attached patch check for renamed parameters (obsolete) — Splinter Review
Now has checkRenamed function like the shim
Attachment #676375 - Attachment is obsolete: true
Attachment #676375 - Flags: feedback?(zack.carter)
Attachment #676395 - Flags: feedback?(zack.carter)
Comment on attachment 676395 [details] [diff] [review]
check for renamed parameters

We should be able to call checkRenamed in nsDOMIdentity.js and remove all other references to loggedInEmail. I don't think loggedInEmail can occur anywhere besides that initial call to watch, where it would be removed by the deprecation check. That would get rid of some duplicate logic.
(In reply to Zachary Carter [:zaach] from comment #5)
> Comment on attachment 676395 [details] [diff] [review]
> check for renamed parameters
> 
> We should be able to call checkRenamed in nsDOMIdentity.js and remove all
> other references to loggedInEmail. I don't think loggedInEmail can occur
> anywhere besides that initial call to watch, where it would be removed by
> the deprecation check. That would get rid of some duplicate logic.

excellent point!  fix coming up ...
Attached patch check for renamed parameters (obsolete) — Splinter Review
Zach points out that we only need to check loggedInEmail vs User in the nsDOM identity entry point
Attachment #676395 - Attachment is obsolete: true
Attachment #676395 - Flags: feedback?(zack.carter)
Comment on attachment 676405 [details] [diff] [review]
check for renamed parameters

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

This patch addresses the change of name from loggedInEmail to loggedInUser as described here: https://developer.mozilla.org/en-US/docs/DOM/navigator.id.watch#Parameters
Attachment #676405 - Flags: review?(benadida)
Comment on attachment 676405 [details] [diff] [review]
check for renamed parameters

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

r+ with small nit below

::: b2g/components/SignInToWebsite.jsm
@@ +142,5 @@
>     * communicate - launch a gaia window with certain options and
>     * provide a callback for handling messages.
>     *
>     * @param aRpOptions        options describing the Relying Party's
> +   *        (dicitonary)      call, such as origin and loggedInUser.

might as well fix spelling of dictionary
Attachment #676405 - Flags: review?(benadida) → review+
Addresses Comment #9.  r=benadida
Attachment #676405 - Attachment is obsolete: true
whoops!  sending inbound as a patch, apparently.  Happy Halloween!
Keywords: checkin-needed
Addresses Comment #9, r=benadida
Attachment #677165 - Attachment is obsolete: true
Pushed to Try since I don't see any results posted here.
https://tbpl.mozilla.org/?tree=Try&rev=079dc1ebac54
https://hg.mozilla.org/mozilla-central/rev/724a3ec3ed48
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Whiteboard: [qa-]
(In reply to Ryan VanderMeulen from comment #13)
> Pushed to Try since I don't see any results posted here.
> https://tbpl.mozilla.org/?tree=Try&rev=079dc1ebac54

Ryan, thank you for jumping in and pushing to try and starring the oranges there.  I greatly appreciate your help!  cheers, j.
A patch for this bug that can be cleanly applied to beta.  The patch has been refreshed to not create the file toolkit/identity/IdentityUtils.js, which already exists in beta.  Apart from that, there is no change.
Even though this is closed, I need someone to mark it bb+ for the beta patch.
blocking-basecamp: --- → ?
(In reply to Jed Parsons [:jparsons] from comment #18)
> Even though this is closed, I need someone to mark it bb+ for the beta patch.

If it isn't basecamp+ btw, you can also nominate this for approval instead to get the uplift if it's +ed.
(In reply to Jason Smith [:jsmith] from comment #19)
> (In reply to Jed Parsons [:jparsons] from comment #18)
> > Even though this is closed, I need someone to mark it bb+ for the beta patch.
> 
> If it isn't basecamp+ btw, you can also nominate this for approval instead
> to get the uplift if it's +ed.

Meant to say - if it's not plused, you nominate for approval.
Comment on attachment 689296 [details] [diff] [review]
refreshed patch can be applied to beta

[Approval Request Comment]
Bug caused by (feature/regressing bug #): feature
User impact if declined: identity won't work
Testing completed (on m-c, etc.): yes
Risk to taking this patch (and alternatives if risky): low; isolated component is preffed off
String or UUID changes made by this patch: none
Attachment #689296 - Flags: approval-mozilla-beta?
blocking-basecamp: ? → ---
blocking-basecamp: --- → +
Attachment #689296 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.