Jed Parsons (use needinfo, please) [:jedp, :jparsons] Assignee
	Description • 12 years ago

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.

	Jason Smith [:jsmith]
	Comment 1 • 12 years ago

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?

	Jed Parsons (use needinfo, please) [:jedp, :jparsons] Assignee
	Comment 2 • 12 years ago

(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?

	Ben Adida [:benadida]
	Comment 3 • 12 years ago

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.

	Henri Sivonen (:hsivonen) (away from Bugzilla until 2023-09-11)
	Comment 4 • 12 years ago

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

	Jason Smith [:jsmith]
	Comment 5 • 12 years ago

Adding Jonas as either Johnny or Jonas can address the points here.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 6 • 12 years ago

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.

	Ben Adida [:benadida]
	Comment 7 • 12 years ago

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.

	Ben Adida [:benadida]
	Comment 8 • 12 years ago

(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

	Jed Parsons (use needinfo, please) [:jedp, :jparsons] Assignee
	Comment 9 • 12 years ago

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

tbpl: https://tbpl.mozilla.org/?tree=Try&rev=3c8eed612e96

	Jed Parsons (use needinfo, please) [:jedp, :jparsons] Assignee
	Comment 10 • 12 years ago

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.

	Ben Adida [:benadida]
	Comment 11 • 12 years ago

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

	Ben Adida [:benadida]
	Comment 12 • 12 years ago

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.

	Jed Parsons (use needinfo, please) [:jedp, :jparsons] Assignee
	Comment 13 • 12 years ago

(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.

	Jed Parsons (use needinfo, please) [:jedp, :jparsons] Assignee
	Comment 14 • 12 years ago

try server is done and green: https://tbpl.mozilla.org/?tree=Try&rev=3c8eed612e96

	Ben Adida [:benadida]
	Comment 15 • 12 years ago

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.

	Jed Parsons (use needinfo, please) [:jedp, :jparsons] Assignee
	Comment 16 • 12 years ago

updated patch header

	Andrea Marchesini [:baku]
	Comment 17 • 12 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/2c9c8df5520b

https://hg.mozilla.org/releases/mozilla-b2g18/rev/bacaca866ca3

	Jason Smith [:jsmith]
	Comment 18 • 12 years ago

Closing per inbound + b2g18 rules (confirmed with jst that we should still be doing this).

	Jed Parsons (use needinfo, please) [:jedp, :jparsons] Assignee
	Comment 19 • 12 years ago

Jason, I'm not familiar with the inbound+b2g18 rules - I hope this will still be landing in m-c?

	Jason Smith [:jsmith]
	Comment 20 • 12 years ago

(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.

	Andreas Gal :gal
	Comment 21 • 12 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).

	Ben Adida [:benadida]
	Comment 22 • 12 years ago

(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.

	Brendan Eich [:brendan]
	Comment 23 • 12 years ago

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

	Ben Adida [:benadida]
	Comment 24 • 12 years ago

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.

	Brendan Eich [:brendan]
	Comment 25 • 12 years ago

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

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 26 • 12 years ago

But yes, no uuid change is needed here. My review applies either way though.

	Jed Parsons (use needinfo, please) [:jedp, :jparsons] Assignee
	Comment 27 • 12 years ago

Thank you everyone for your comments and your help.  Jonas, thank you for your review.

	Jed Parsons (use needinfo, please) [:jedp, :jparsons] Assignee
	Comment 28 • 12 years ago

Updated patch headers

	Ed Morley [:emorley]
	Comment 29 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/2c9c8df5520b

	Jed Parsons (use needinfo, please) [:jedp, :jparsons] Assignee
	Comment 30 • 12 years ago

(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