Implemented in bug 762993, looks like only used on B2G. And B2G gets removed from mozilla-central in bug 1306391 .

Builds fine locally. Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=be19a6fe5a134a3738280348bdb6b2c65529860d

Comment on attachment 8811868 [details] [diff] [review] Remove it Whoops, I never ran the installer...

I forgot to remove identity.xpt from installer/package-manifest.in... New try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c8c394bb17fb53dc5c63ee2f18849c235dc9e3e5

Comment on attachment 8811868 [details] [diff] [review] Remove it (I assume you didn't mean to obsolete this part, which is doing the bulk of the removal :).

Comment on attachment 8811868 [details] [diff] [review] Remove it Review of attachment 8811868 [details] [diff] [review]: ----------------------------------------------------------------- Hmm, this seems to be missing a few files. I don't see a deletion for: IdentityUtils.jsm jwcrypto.jsm (but this is still used in services/fxaccounts, so it and associated tests should probably be moved) RelyingParty.jsm r- for a few nits and to catch those missing files, but otherwise on the right track! Thanks for doing this. ::: toolkit/identity/FirefoxAccounts.jsm @@ -1,1 @@ > -/* This Source Code Form is subject to the terms of the Mozilla Public There's a reference to this file in ~/.eslintignore that can be removed too ::: toolkit/identity/LogUtils.jsm @@ -1,1 @@ > -/* -*- js-indent-level: 2; indent-tabs-mode: nil -*- */ tools/lint/eslint/modules.json contains a reference to LogUtils.jsm that can be removed. ::: toolkit/identity/MinimalIdentity.jsm @@ -1,1 @@ > -/* -*- js-indent-level: 2; indent-tabs-mode: nil -*- */ tools/lint/eslint/modules.json contains a reference to MinimalIdentity.jsm that can be removed. ::: toolkit/identity/tests/unit/test_jwcrypto.js @@ -1,1 @@ > -/* Any copyright is dedicated to the Public Domain. jwcrypto.jsm is still used a wee bit in /services -- http://searchfox.org/mozilla-central/rev/957458d8fa2328c2a760dbb30e7f1f1efa55b4d0/services/fxaccounts/FxAccounts.jsm#28-29 So it and its tests should stay alive. As noted in the overview, probably best to just move this over to /services.

Comment on attachment 8811897 [details] [diff] [review] Correct version Looks fine. Can you just roll this into the previous patch with the deletions? Although I'd suggest having a separate patch, taking care of moving jwcrypto.jsm + tests, so it's clear in Hg history that there's one changeset moving a couple files, and another changeset nuking everything else. (I'm assuming the jwcrypto move is trivial, if it's not I could be convinced to leave it in place for now.)

Hmm, I don't know this code at all, but I spent a few hours looking at this and there are more dependencies than I first noticed: http://searchfox.org/mozilla-central/rev/d98418da69edeb1f2f8e6f3840157fae1512f89b/toolkit/identity/jwcrypto.jsm#17 for example. And looking at test_jwcrypto.js, it needs more from toolkit/identity: http://searchfox.org/mozilla-central/rev/d98418da69edeb1f2f8e6f3840157fae1512f89b/toolkit/identity/tests/unit/test_jwcrypto.js#8 And then looking at /toolkit/identity/Identity.jsm (needed by test_jwcrypto.js), it looks like it uses IdentityStore.jsm, RelyingParty.jsm and IdentityProvider.jsm.

Is the jwcrypto.js code in FxAccounts.jsm still used?

Oh, derp, I didn't look to see if jwcrypto depended on stuff. I assumed it was independent. And the try failures from the comment 3 link further indicate there's a dependency. :) So I think this means, in addition to keeping the jwcrypto bits: 1) Keep IdentityCryptoService stuff 2) Keep LogUtils.jsm (or nuke it and change its usage to something else) 3) Keep test_crypto_service.js and any needed bits from head_identity.js I didn't look at the tests in detail, but for at least the one example linked in comment 7 that Cu.import is actually unused. Mark, any thoughts here? I assume FxAccounts.jsm still needs the jwcrypto bits. Does moving jwcrypto + IdentityCryptoService over to /services/fxaccounts/ make sense to you too? Have any cycles to help move the code? Stefan, if you're not able to move the code yourself, I'd still take a patch that nukes almost everything and leaves needed things in place. Although if untangling some of the dependencies is still an issue, might be easier to see if Mark can just help more the still-needed code first. Then /toolkit/identity would just be a simple deletion.

Oops, meant to actually NI Mark. :) See last comment.

Let's wait for Mark ;-) But it looks like getAssertionFromCert and getKeypairAndCertificate in FxAccounts.jsm needs jwcrypto.js. And from what I can see getKeypairAndCertificate is used in services/fxaccounts/tests/xpcshell/test_accounts.js

(In reply to Justin Dolske [:Dolske] from comment #9) > Mark, any thoughts here? I assume FxAccounts.jsm still needs the jwcrypto > bits. Does moving jwcrypto + IdentityCryptoService over to > /services/fxaccounts/ make sense to you too? Yep - or into the existing services/crypto dir. > Have any cycles to help move the code? Not before Hawaii :( But I think that should be relatively easy - best I can tell, there's exactly 1 import of jwcrypto in all of sync/fxa.

Just for reference, here's a complete removal of toolkit/identity where also the dependencies of jwcrypto.js in FxAccounts.jsm are removed. I haven't disabled services/fxaccounts/tests/xpcshell/test_accounts.js, though.

(In reply to Stefan [:stefanh] from comment #13) > Just for reference, here's a complete removal of toolkit/identity where also > the dependencies of jwcrypto.js in FxAccounts.jsm are removed. I'm a little confused. What's the purpose of this? > I haven't > disabled services/fxaccounts/tests/xpcshell/test_accounts.js, though. We can't disable that test, nor can we remove jwcrypto from FxAccounts.jsm.

(In reply to Mark Hammond [:markh] from comment #14) > (In reply to Stefan [:stefanh] from comment #13) > > Just for reference, here's a complete removal of toolkit/identity where also > > the dependencies of jwcrypto.js in FxAccounts.jsm are removed. > > I'm a little confused. What's the purpose of this? I just wanted to put it somewhere and use bits from it. I'll take a look at the move, will hopefully have something that works next week.

I took a slightly different route when it comes to splitting this in 2 parts, mainly because I've been struggling a bit with the move. This part will remove files/bits not needed, but keep the needed files in toolkit/identity. Everything should work fine :-) Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ce66066220a12f5fe56b43def78c88bf6159c714. The above push is nearly identical to an older push (where I forgot to remove the const PREF_DEBUG = "toolkit.identity.debug" in LogUtils.jsm) which looked good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=907278ec8599803a7cddf7fe8e90142f0e39dd21 Some comments: 1) I removed the toolkit.identity.debug pref and edited LogUtils.jsm a bit. I don't think a pref for this is needed, since the LogUtils.jsm logging is only used in tests. 2) I kept head_identity.js for now, but it will go in the next part. 3) Turns out that the xpcshell tests have been b2g-only, but the 2 tests works fine on almost all other platforms (they fail on Android, so I turned them off for Android).

Here's part2. nsIIdentityCryptoService is now in services-crypto-component, so I updated the CID.

Try push (Part1 + Part2): https://treeherder.mozilla.org/#/jobs?repo=try&revision=288c74777c58be4b1620a1397d5019b4e579d410

Patches doesn't apply anymore. But if this is still interesting, I can update them.

(In reply to Stefan [:stefanh] from comment #19) > Patches doesn't apply anymore. But if this is still interesting, I can > update them. Yes please. Even just removing low-hanging fruit would be great.

(In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from comment #20) > (In reply to Stefan [:stefanh] from comment #19) > > Patches doesn't apply anymore. But if this is still interesting, I can > > update them. > > Yes please. Even just removing low-hanging fruit would be great. OK, I'll try to have something up for review in a few days.

This is the removal part, next part will move the needed files to services/crypto (and remove the remaining files in toolkit/identity). See also comment #16. Results from try push is here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fb5f783b4cb3fc3e8448435a92f607d855f54791

This is part2 which will move stuff to services/cryptoand nuke the remaining files in toolkit/identity. See also comment #17. Try push (Part1 + Part2): Try push, part1 + part 2: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9871cdcc6a1468195292b762caf19fb56b356c72

Comment on attachment 8829271 [details] [diff] [review] Part1: Remove unneeded files/bits Review of attachment 8829271 [details] [diff] [review]: ----------------------------------------------------------------- Thanks Stefan! ::: toolkit/identity/LogUtils.jsm @@ -18,5 @@ > Cu.import("resource://gre/modules/Services.jsm"); > > -function IdentityLogger() { > - Services.prefs.addObserver(PREF_DEBUG, this, false); > - this._debug = Services.prefs.getBoolPref(PREF_DEBUG); This logging needs to stay behind a pref as we don't want to spew non-error messages in regular builds. Can you revert the changes to this file but switch the pref to services.sync.log.cryptoDebug for now? Then file a follow-up in the services product to remove this file and replace the logging in jwcrypto.jsm the Log.jsm approach used in the rest of the related services code? @@ -68,5 @@ > > /** > * log() - utility function to print a list of arbitrary things > * > - * Enable with about:config pref toolkit.identity.debug Don't forget to update this comment with services.sync.log.cryptoDebug

Comment on attachment 8829272 [details] [diff] [review] Part2: Move stuff to services/crypto and remove remaining files in toolkit/identity Review of attachment 8829272 [details] [diff] [review]: ----------------------------------------------------------------- r=me if we leave the CID alone. Thanks Perhaps you want to take bug 1244950 or removing LogUtils.jsm as a next bug? ::: services/crypto/component/IdentityCryptoService.cpp @@ +543,5 @@ > { nullptr } > }; > > const mozilla::Module::ContractIDEntry kContracts[] = { > + { "@mozilla.org/services-crypto/identity;1", &kNS_IDENTITYCRYPTOSERVICE_CID }, "@mozilla.org/services-crypto/identity;1" doesn't seem like a good name to me. Usually services end with "service" and the suffix usually describes what it's about. In this case I think "identity" is confusing. Since the file is still IdentityCryptoService.cpp I think the old contract name is fine. I also wonder if you would need to request a CLOBBER if you change this? I think it's easiest to just leave this alone throughout the patch.

Thanks, I'll make all your suggested/required changes and push this in the next day or so. > Perhaps you want to take bug 1244950 or removing LogUtils.jsm as a next bug? Let me see what I can do, I'll start with filing a bug for the LogUtils.jsm removal ;-)

Pushed by stefanh@inbox.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/7f4f047cfc54 Remove toolkit/identity, part1: Remove unneeded files/bits. r=MattN. https://hg.mozilla.org/integration/mozilla-inbound/rev/1bab9319950e Remove toolkit/identity, part2: Move used files/bits to services/crypto and remove remaining files in toolkit/identity. r=MattN.

(In reply to Stefan [:stefanh] from comment #26) > Let me see what I can do, I'll start with filing a bug for the LogUtils.jsm > removal ;-) Filed bug 1333485.

https://hg.mozilla.org/mozilla-central/rev/7f4f047cfc54 https://hg.mozilla.org/mozilla-central/rev/1bab9319950e