Closed Bug 1313045 Opened 3 years ago Closed 3 years ago

remove toolkit/identity/

Categories

(Toolkit :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: aryx, Assigned: stefanh)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 5 obsolete files)

Implemented in bug 762993, looks like only used on B2G. And B2G gets removed from mozilla-central in bug 1306391 .
Attached patch Remove it (obsolete) — Splinter Review
Builds fine locally. Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=be19a6fe5a134a3738280348bdb6b2c65529860d
Assignee: nobody → stefanh
Status: NEW → ASSIGNED
Attachment #8811868 - Flags: review?(dolske)
Comment on attachment 8811868 [details] [diff] [review]
Remove it

Whoops, I never ran the installer...
Attachment #8811868 - Flags: review?(dolske)
Attached patch Correct version (obsolete) — Splinter Review
I forgot to remove identity.xpt from installer/package-manifest.in... New try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c8c394bb17fb53dc5c63ee2f18849c235dc9e3e5
Attachment #8811868 - Attachment is obsolete: true
Attachment #8811897 - Flags: review?(dolske)
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 :).
Attachment #8811868 - Attachment is obsolete: false
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.
Attachment #8811868 - Flags: review-
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.)
Attachment #8811897 - Flags: review?(dolske)
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.
Status: ASSIGNED → NEW
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.
Flags: needinfo?(markh)
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.
Flags: needinfo?(markh)
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).
Attachment #8811868 - Attachment is obsolete: true
Attachment #8811897 - Attachment is obsolete: true
Attachment #8815485 - Attachment is obsolete: true
Attachment #8817648 - Flags: review?(dolske)
Here's part2. nsIIdentityCryptoService is now in services-crypto-component, so I updated the CID.
Attachment #8817667 - Flags: review?(dolske)
Patches doesn't apply anymore. But if this is still interesting, I can update them.
Status: NEW → ASSIGNED
Flags: needinfo?(dolske)
(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.
Flags: needinfo?(dolske)
Attachment #8817648 - Flags: review?(dolske)
Attachment #8817667 - Flags: review?(dolske)
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
Attachment #8817648 - Attachment is obsolete: true
Attachment #8817667 - Attachment is obsolete: true
Attachment #8829271 - Flags: review?(MattN+bmo)
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
Attachment #8829272 - Flags: review?(MattN+bmo)
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
Attachment #8829271 - Flags: review?(MattN+bmo) → review+
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.
Attachment #8829272 - Flags: review?(MattN+bmo) → review+
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.
Blocks: 1333485
(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
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Depends on: 1333692
No longer blocks: 1369194
You need to log in before you can comment on or make changes to this bug.