1309030 - Remove DOM/identity and related code

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Comment 1

[Michelangelo De Simone [Offline] [:mds]]

Attachment: Bug 1309030 - Remove DOM/identity and related code.

Review commit: https://reviewboard.mozilla.org/r/84674/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/84674/

Comment 2

[Nils Ohlmeier [:drno]]

I could not find a link between this and PeerIdentity, but to be safe: Martin this code removal is not going to affect PeerIdentity, right?

Comment 3

[Martin Thomson [:mt:]]

If the tests continue to pass, we're definitely OK to remove this.  A really early version of the peerIdentity stuff did depend on this code, but it wasn't fit for purpose.

Comment 4

[(no longer active)]

Comment on attachment 8799527 [details]
Bug 1309030 - Remove DOM/identity and related code.

https://reviewboard.mozilla.org/r/84674/#review83446

This looks good, but please eliminate all uses of the dom.identity.enabled pref and mozId.  There are a few ones left that your patch doesn't remove:

 ehsan  Ehsans-MacBook-Pro  ~  moz  src  work  $ git grep -w mozId
b2g/app/b2g.js:960:// Enable sync and mozId with Firefox Accounts.
dom/identity/tests/mochitest/file_browserid_rp_noOnlogin.html:33:      navigator.mozId.watch({ });
dom/identity/tests/mochitest/file_browserid_rp_ok.html:32:      navigator.mozId.watch({
dom/identity/tests/mochitest/file_declareAudience.html:27:      navigator.mozId.watch({
dom/identity/tests/mochitest/file_declareAudience.html:32:            navigator.mozId.request();
dom/identity/tests/mochitest/file_fxa_rp_noOnlogin.html:33:      navigator.mozId.watch({
dom/identity/tests/mochitest/file_fxa_rp_noOnlogout.html:33:      navigator.mozId.watch({
dom/identity/tests/mochitest/file_fxa_rp_noOnready.html:33:      navigator.mozId.watch({
dom/identity/tests/mochitest/file_fxa_rp_ok.html:32:      navigator.mozId.watch({
dom/identity/tests/mochitest/file_syntheticEvents.html:9:  Certified and privileged apps can call mozId outside an event handler
dom/identity/tests/mochitest/file_syntheticEvents.html:27:    navigator.mozId.watch({
dom/identity/tests/mochitest/file_syntheticEvents.html:31:          navigator.mozId.request();
dom/identity/tests/mochitest/test_rpHasValidCallbacks.html:35:// will invoke navigator.mozId.watch().  If they do not provide the
dom/identity/tests/mochitest/test_rpHasValidCallbacks.html:83:    ['dom.identity.enabled', true],               // navigator.mozId
dom/webidl/Identity.webidl:57: NavigatorProperty="mozId",
services/fxaccounts/FxAccountsManager.jsm:529:   * the heart of the response to navigator.mozId.request() on device.
services/fxaccounts/tests/mochitest/test_invalidEmailCase.html:119:    ["dom.identity.enabled", true],                // navigator.mozId
 ehsan  Ehsans-MacBook-Pro  ~  moz  src  work  $ git grep dom.identity.enabled
b2g/app/b2g.js:795:pref("dom.identity.enabled", true);
dom/identity/nsDOMIdentity.js:10:const PREF_ENABLED = "dom.identity.enabled";
dom/identity/tests/mochitest/test_declareAudience.html:273:    ["dom.identity.enabled", true],
dom/identity/tests/mochitest/test_rpHasValidCallbacks.html:83:    ['dom.identity.enabled', true],               // navigator.mozId
dom/identity/tests/mochitest/test_syntheticEvents.html:195:    ["dom.identity.enabled", true],
dom/webidl/Identity.webidl:58: Pref="dom.identity.enabled"]
services/fxaccounts/tests/mochitest/test_invalidEmailCase.html:119:    ["dom.identity.enabled", true],                // navigator.mozId

Comment 5

[Michelangelo De Simone [Offline] [:mds]]

Comment on attachment 8799527 [details]
Bug 1309030 - Remove DOM/identity and related code.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/84674/diff/1-2/

Comment 6

[Michelangelo De Simone [Offline] [:mds]]

(In reply to :Ehsan Akhgari from comment #4)

> This looks good, but please eliminate all uses of the dom.identity.enabled
> pref and mozId.  There are a few ones left that your patch doesn't remove:

Thanks.

As the patch removes dom/identity altogether, there was only one occurrance of "dom.identity.enabled" left in services/fxaccounts/tests/mochitest/test_invalidEmailCase.html and one in b2g/app/b2g.js.
I've removed them both, even though the one in b2g/ is inconsequential as b2g/ is probably gonna be nuked anyway.

The only mention of "mozId" left is in services/fxaccounts/FxAccountsManager.jsm:529 as part of the comment to getAssertion(). I'm not familiar with that code to evaluate whether that function is, infact, being used or not.

If aren't either, probably we need to ni? some folks in the Sync team before going ahead. Your call.

Comment 7

[(no longer active)]

(In reply to Michelangelo De Simone [:Michelangelo] from comment #6)
> The only mention of "mozId" left is in
> services/fxaccounts/FxAccountsManager.jsm:529 as part of the comment to
> getAssertion(). I'm not familiar with that code to evaluate whether that
> function is, infact, being used or not.

This function was added in bug 1004319, but I'm not sure how it's related to mozId.  Perhaps Sam or Fernando can clarify?  At any rate, we can fix the comment post-landing per their recommendation, not a big deal.

Comment 8

[(no longer active)]

Comment on attachment 8799527 [details]
Bug 1309030 - Remove DOM/identity and related code.

https://reviewboard.mozilla.org/r/84674/#review83606

Comment 9

[Michelangelo De Simone [Offline] [:mds]]

(In reply to :Ehsan Akhgari from comment #7)

> At any rate, we can fix the comment post-landing per their recommendation, not a big deal.

Thanks!

Comment 10

[Sam Penrose]

The comment is obsolete. Given that B2G is defunct and FxAccounts is now OAuth-only (right?), I would suspect the entire file is obsolete. However, I have been out of this code for over a year. I would recommend working through the Sync team to determine how much of services/fxaccounts/*jsm is cruft.

Comment 11

[Sam Penrose]

PS I believe you will find cruft in toolkit/identity also.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/feb1c52ebe9e
Remove DOM/identity and related code. r=Ehsan

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/feb1c52ebe9e

Comment 14

[(no longer active)]

Michelangelo, can you please file a follow-up as per comment 10 to remove more unneeded code?  Thanks!

Comment 15

[Michelangelo De Simone [Offline] [:mds]]

(In reply to :Ehsan Akhgari from comment #14)

> Michelangelo, can you please file a follow-up as per comment 10 to remove
> more unneeded code?  Thanks!

As I said, I'm not familiar enough with that part of code. I've opened bug 1309632 to track it, though.

Comment 16

[Chris Mills [:cmills]]

Adding ddn, just to check the docs for this.

Comment 17

[Chris Mills [:cmills]]

I've looked this over - we have an explainer article that has already been archived, and we never wrote ref docs for the Identity interface.

I've added a note to the Fx52 rel notes:

https://developer.mozilla.org/en-US/Firefox/Releases/52#Changes_and_removals_3

Let me know if that looks OK. Thanks!