Replace loggedInEmail parameter with loggedInUser

RESOLVED FIXED in Firefox 18

Status

()

Core
Identity
--
enhancement
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jedp, Assigned: jedp)

Tracking

unspecified
mozilla19
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

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

Details

(Whiteboard: [qa-])

Attachments

(2 attachments, 4 obsolete attachments)

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
Created attachment 676375 [details] [diff] [review]
rename loggedInEmail to loggedInUser; fallback 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.
Created attachment 676395 [details] [diff] [review]
check for renamed parameters

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 ...
Created attachment 676405 [details] [diff] [review]
check for renamed parameters

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+
Created attachment 677165 [details] [diff] [review]
pass rp caller params from nsDOM through and up to browserid

Addresses Comment #9.  r=benadida
Attachment #676405 - Attachment is obsolete: true
whoops!  sending inbound as a patch, apparently.  Happy Halloween!
Keywords: checkin-needed
Keywords: checkin-needed
Created attachment 677200 [details] [diff] [review]
check for renamed parameters

Addresses Comment #9, r=benadida
Attachment #677165 - Attachment is obsolete: true
Keywords: checkin-needed
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
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19

Updated

5 years ago
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.
Created attachment 689296 [details] [diff] [review]
refreshed patch can be applied to beta

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: ? → ---

Updated

5 years ago
blocking-basecamp: --- → +

Updated

5 years ago
Attachment #689296 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
https://hg.mozilla.org/releases/mozilla-beta/rev/1cb531b32040
status-firefox18: --- → fixed
status-firefox19: --- → fixed
You need to log in before you can comment on or make changes to this bug.