Rename native navigator.id to navigator.mozId

RESOLVED FIXED in Firefox 21

Status

()

--
major
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: jedp, Assigned: jedp)

Tracking

unspecified
mozilla21
Points:
---

Firefox Tracking Flags

(blocking-basecamp:+, firefox19 wontfix, firefox20 wontfix, firefox21 fixed, b2g18 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

In order to smoothly and nimbly transition from having marketplace as the sole user of the native persona implementation, with other RPs using the shim, to a shining future where all RPs access the native implementation's functions, we propose renaming navigator.id to navigator.mozId in native code.  

The marketplace include.js will be modified to attach to navigator.mozId, and in the future, the global person.org include.js can follow as well.

Marketplace will temporarily have to decide which include.js to load by checking the user agent string or (as may be necessary on desktop builds) checking for a pref.  Example:

if (userAgentIsB2G) {
    document.write('<script async defer src="https://login.native-persona.org/include.js"></script>');
} else {
    document.write('<script async defer src="https://login.persona.org/include.js"></script>');
}

This check will be temporary until a single include file can be used.  Nevertheless, marketplace will always be able to tell whether a native implementation (on b2g) or a shimmed version (on desktop) has been used by checking to see if navigator.id._shimmed is defined.

Apart from the temporary UserAgent check, the change will not impact our users in any way.  They will still call navigator.id functions.
Per email - this one I think is a late breaking change to the DOM. I think a DOM module owner needs to analyze this situation and make a call on what to do for v1 given this situation presented.

Johnny - Can you give some insight here?
Flags: needinfo?(jst)
(In reply to Jason Smith [:jsmith] from comment #1)
> Per email - this one I think is a late breaking change to the DOM. I think a
> DOM module owner needs to analyze this situation and make a call on what to
> do for v1 given this situation presented.

I sympathise wholeheartedly with your concern about late-breaking changes, so let me give more context on the scope of the proposed changes within gecko/DOM.  

What I think sets this DOM update apart is that the *only* caller of navigator.id functions so far is a hosted javascript file that detects the presence of the native navigator.id.  If it's there, it uses it; if not, it provides the functions itself.  So we don't run the risk of disrupting any callers expecting the native navigator.id API; they are all using the hosted include.js file.

I'm pretty sure that the gecko changes required for this patch are simply to substitute 'mozId' for 'id' in the Identity.manifest file, and probably (Johnny, please correct me if this is unnecessary) to change the uuids for that property.  The uuid change has to be reflected in nsDOMIdentity.js.  So altogether, I think it's at most a five-line change in gecko.

The hosted include.js file will be updated so that all clients will still call the navigator.id.* functions.  To relying parties, the API will be the same, and the change will be invisible.  We will simply be wrapping a native navigator.mozId object instead of a native navigator.id.

Johnny, does that seem reasonable?
I'll add that, if I know Johnny and Jonas at all, they will say that we should have been doing it this way from the start. navigator.id is not standardized yet, we should support only the prefixed version, just like we do for nav.mozPay.
I think renaming it to mozId would make it harder to standardize it and remigrating back to .id later would risk breakage. I think it would be much better to ship without prefix per https://groups.google.com/d/topic/mozilla.dev.platform/34JfwyEh5e4/discussion
Adding Jonas as either Johnny or Jonas can address the points here.
Flags: needinfo?(jonas)
I don't really have a strong opinion on this.

I'm not sure I fully understand the proposal here though. There seems to be three "navigator.id"s involved here.

* The native implementation exposed by B2G
* The shim added by https://login.native-persona.org/include.js
* The shim added by https://login.persona.org/include.js

It sounds like you are proposing to rename navigator.id in the first two, is that correct? And potentially rename the third one at some later point?

Either way, I don't have a very strong opinion on if we should prefix or not. Both have advantages. If you think that we'll want to modify the API in the future it might be a good idea to prefix.

Though it generally seems like a bad idea if we don't use a consistent name. Then it might be easy to end up with the worst of both worlds.
Flags: needinfo?(jst)
Flags: needinfo?(jonas)
Jonas: to be clear, we want to rename the *native* implementation exposed by B2G only, and let the shims bridge nav.id to nav.mozId when it makes sense (and continue to shim when it doesn't.)

This is the only way that we can properly support Persona login in apps other than Marketplace, and since there is still a chance that the API will change (plus it's not standardized), it seems best to me to have the native implementation use the prefixed version.
blocking-basecamp: --- → +
(In reply to Henri Sivonen (:hsivonen) from comment #4)
> I think renaming it to mozId would make it harder to standardize it and
> remigrating back to .id later would risk breakage.

No, actually it's the opposite: breakage is happening now because of the unprefixed native version that has some FXOS-specifics because of the tight deadline.

The most forward-compatible route is prefixed. If there are no DOM technical objections, I'd like to move forward on this ASAP.

-Ben
Created attachment 699426 [details] [diff] [review]
rename navigator.id to navigator.mozId

Provisional patch to change navigator.id to navigator.mozId.

tbpl: https://tbpl.mozilla.org/?tree=Try&rev=3c8eed612e96
Attachment #699426 - Flags: review-
The necessary browserid PR has been done and pushed to our test server.

https://github.com/mozilla/browserid/pull/2917

It will work both for native builds that provide navigator.id and those that provide navigator.mozId.  It's in production now, so the next step, landing the patch, should provide a seamless upgrade for all concerned.

Try server is looking pretty good, so I'm going to venture to ask for r=benadida now.
Attachment #699426 - Flags: review?(benadida)
Comment on attachment 699426 [details] [diff] [review]
rename navigator.id to navigator.mozId

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

I don't care so much but, I'm curious, any reason why you regenerated the UUIDs?

Also, we don't have tests for navigator.id that need to be updated for navigator.mozId?

Ass
Attachment #699426 - Flags: review?(benadida)
Well, that was a weird truncated word up there that looks like I signed that message with some profanity. Whoops! I think I was typing "Assuming" and then thought better of it.
(In reply to Ben Adida [:benadida] from comment #11)
> Comment on attachment 699426 [details] [diff] [review]
> rename navigator.id to navigator.mozId
> 
> Review of attachment 699426 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't care so much but, I'm curious, any reason why you regenerated the
> UUIDs?

Perhaps it's not necessary.  I don't have a good understanding of how these are actually used deep inside gecko, and in my ignorance, I thought I'd play it safe and ensure that we had all new symbols for the new interface name.  (If anyone can give me a pointer to good documentation on this, I would be grateful :) 

> Also, we don't have tests for navigator.id that need to be updated for
> navigator.mozId?

No; I was pleasantly surprised to discover that we do not.  We have xpcshell tests, which all operate on the IDService object itself; We have marionette tests, which all operate on navigator.id once it's been attached to navigator.mozId; We used to have mochitests, for which this would have mattered, but those were removed because mochitests don't work with b2g.
Attachment #699426 - Flags: review?(benadida)
Comment on attachment 699426 [details] [diff] [review]
rename navigator.id to navigator.mozId

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

ok, let's make sure we add tests in a followup bug, but in any case this is good to go.
Attachment #699426 - Flags: review?(benadida) → review+
Created attachment 699913 [details] [diff] [review]
rename navigator.id to navigator.mozId; r=benadida

updated patch header
Attachment #699426 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/2c9c8df5520b

https://hg.mozilla.org/releases/mozilla-b2g18/rev/bacaca866ca3
status-b2g18: --- → fixed
status-firefox19: --- → wontfix
status-firefox20: --- → wontfix
Closing per inbound + b2g18 rules (confirmed with jst that we should still be doing this).
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Jason, I'm not familiar with the inbound+b2g18 rules - I hope this will still be landing in m-c?
(In reply to Jed Parsons [:jparsons] from comment #19)
> Jason, I'm not familiar with the inbound+b2g18 rules - I hope this will
> still be landing in m-c?

Yeah it'll land on m-c. I ran this by jst to make sure I got the process right here.

Updated

6 years ago
Keywords: checkin-needed → verifyme

Comment 21

6 years ago
Guys, this is a DOM change and requires review from a DOM peer. Please don't short-circuit the review process and module ownership (that having said, I think its a good patch, but jonas should + this).

Updated

6 years ago
Attachment #699913 - Flags: review?(jonas)
(In reply to Andreas Gal :gal from comment #21)
> Guys, this is a DOM change and requires review from a DOM peer.

Jonas already said, above, that he didn't have a strong opinion on this. Also, a prefixed API is what we had agreed to with DOM peers earlier. I don't think short-circuiting is going on.
I think Andreas was looking for an r? or sr? directed at jst or jonas, to be clear about DOM peer review.

Henri's point is deep and we've seen it in the real world with -webkit- prefixes: if the prefixed API becomes popular, you will regret prefixing a de-facto standard which is therefore hard to unprefix. If the prefixed API does not catch on widely, you can change it more easily -- but you still break your clients (however few) and you signal that the API is not ready for standardization.

Ben is right also in that if the overriding likelihood is that the API will change, then prefixing can win to reduce risk of the de-facto standard being set under the unprefixed name with a poor API. B2G has done this for Web Telephony and Web SMS APIs.

I'm generally against prefixing, I side with Henri. But that is because I think we should try to hit the target the first time, with "minimum viable" APIs that we can stick with. This is hard to do in all cases.

/be
To be clear: DOM Peers and I agreed to .mozId months back. The fact that .id snuck in there when we started the B2G code was an unintended mistake. All we're doing here is reverting to agreed upon behavior!

I'm happy to have the extended discussion for future updates to B2G codebase, but for now it's pretty critical that we move forward to *correct* the situation back to what we had agreed upon before. Let's not make a mistake the reason for delaying an agreed-upon solution.
I don't think setting r?:sicking or whatever slows anything down appreciably, vs. getting a comment here to same effect.

Also, please -- no cargo cult regeneration of UUIDs. These need to be regened if any interface changed in a binary-incompatible way. If none did so change, then no need for UUID regen.

/be
But yes, no uuid change is needed here. My review applies either way though.
Thank you everyone for your comments and your help.  Jonas, thank you for your review.
Created attachment 700163 [details]
rename navigator.id to navigator.mozId; r=sicking,benadida

Updated patch headers
Attachment #699913 - Attachment is obsolete: true
Keywords: checkin-needed
(In reply to Ben Adida [:benadida] from comment #15)
> Comment on attachment 699426 [details] [diff] [review]
> rename navigator.id to navigator.mozId
> 
> Review of attachment 699426 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ok, let's make sure we add tests in a followup bug, but in any case this is
> good to go.

I've created Bug 829110, which exercises navigator.mozId directly now
status-firefox21: --- → fixed
Target Milestone: --- → mozilla21

Updated

6 years ago
QA Contact: jsmith

Updated

6 years ago
Keywords: verifyme
QA Contact: jsmith
You need to log in before you can comment on or make changes to this bug.