Closed
Bug 806605
Opened 12 years ago
Closed 12 years ago
Replace loggedInEmail parameter with loggedInUser
Categories
(Core Graveyard :: Identity, enhancement)
Core Graveyard
Identity
Tracking
(blocking-basecamp:+, firefox18 fixed, firefox19 fixed)
People
(Reporter: jedp, Assigned: jedp)
References
Details
(Whiteboard: [qa-])
Attachments
(2 files, 4 obsolete files)
17.16 KB,
patch
|
Details | Diff | Splinter Review | |
15.72 KB,
patch
|
akeybl
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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
Comment 1•12 years ago
|
||
Here's how the shim handles it: https://github.com/mozilla/browserid/blob/dev/resources/static/include_js/include.js#L1092
Assignee | ||
Comment 2•12 years ago
|
||
Zach, would you mind giving this a quick look?
Attachment #676375 -
Flags: feedback?(zack.carter)
Assignee | ||
Comment 3•12 years ago
|
||
(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.
Assignee | ||
Comment 4•12 years ago
|
||
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 5•12 years ago
|
||
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.
Assignee | ||
Comment 6•12 years ago
|
||
(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 ...
Assignee | ||
Comment 7•12 years ago
|
||
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)
Assignee | ||
Comment 8•12 years ago
|
||
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 9•12 years ago
|
||
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+
Assignee | ||
Comment 10•12 years ago
|
||
Addresses Comment #9. r=benadida
Attachment #676405 -
Attachment is obsolete: true
Assignee | ||
Comment 11•12 years ago
|
||
whoops! sending inbound as a patch, apparently. Happy Halloween!
Keywords: checkin-needed
Assignee | ||
Updated•12 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 12•12 years ago
|
||
Addresses Comment #9, r=benadida
Attachment #677165 -
Attachment is obsolete: true
Assignee | ||
Updated•12 years ago
|
Keywords: checkin-needed
Comment 13•12 years ago
|
||
Pushed to Try since I don't see any results posted here. https://tbpl.mozilla.org/?tree=Try&rev=079dc1ebac54
Comment 14•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/724a3ec3ed48
Flags: in-testsuite+
Keywords: checkin-needed
Comment 15•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/724a3ec3ed48
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Updated•12 years ago
|
Whiteboard: [qa-]
Assignee | ||
Comment 16•12 years ago
|
||
(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.
Assignee | ||
Comment 17•11 years ago
|
||
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.
Assignee | ||
Updated•11 years ago
|
Blocks: basecamp-id
Assignee | ||
Comment 18•11 years ago
|
||
Even though this is closed, I need someone to mark it bb+ for the beta patch.
blocking-basecamp: --- → ?
Comment 19•11 years ago
|
||
(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.
Comment 20•11 years ago
|
||
(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.
Assignee | ||
Comment 21•11 years ago
|
||
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?
Assignee | ||
Updated•11 years ago
|
blocking-basecamp: ? → ---
Updated•11 years ago
|
blocking-basecamp: --- → +
Updated•11 years ago
|
Attachment #689296 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 22•11 years ago
|
||
https://hg.mozilla.org/releases/mozilla-beta/rev/1cb531b32040
status-firefox18:
--- → fixed
status-firefox19:
--- → fixed
Updated•5 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•