Closed Bug 1599031 Opened 3 months ago Closed 3 months ago

Add ability to query build time option MOZ_OPENPGP from JS

Categories

(MailNews Core :: Security: OpenPGP, enhancement)

enhancement
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 72.0

People

(Reporter: KaiE, Assigned: KaiE)

Details

Attachments

(1 file, 2 obsolete files)

While the openpgp integration is disabled by default, I need the ability to query from JS if MOZ_OPENPGP was enabled.

This is required for global messenger code, to enable/disable UI behavior.

(As a first step, I'll need that for the popup menu on the attachment list. Need that to test import of attached OpenPGP keys from received emails.)

I'd like to implement something similar to FF AppConstants.

I see we already have mail/base/modules/MailConsts.jsm
but that doesn't use the "Object.freeze" wrapping that AppConstants uses for the build time flags.

Magnus, please let me know your preference:
(a) use the attached patch, to add an additional MailConstants module that uses Object.freeze
(b) add the new flag to the existing MailConsts.jsm and change that to also use Object.freeze

Attached patch 1599031-v1.patch (obsolete) — Splinter Review
Attachment #9111169 - Flags: review?(mkmelin+mozilla)

Hmm, I just notice that I copied the style that AppConstants used:

this.EXPORTED_SYMBOLS
...
this.MailConstants = Object.freeze({

I guess I should rather use the following?

var EXPORTED_SYMBOLS
...
var MailConstants = Object.freeze({

Attached patch 1599031-v2.patch (obsolete) — Splinter Review
Attachment #9111169 - Attachment is obsolete: true
Attachment #9111169 - Flags: review?(mkmelin+mozilla)
Attachment #9111171 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9111171 [details] [diff] [review]
1599031-v2.patch

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

Hmm, preferably these kind of build-time options should go into AppConstants.jsm, but we can't really touch that...

The way it's used, for now it might be easiest to just define a global variable in bond.js?

(In reply to Magnus Melin [:mkmelin] from comment #5)

The way it's used, for now it might be easiest to just define a global
variable in bond.js?

If MOZ_OPENPGP is undefined, then file bond.js isn't loaded. Your suggestion would require to always load the bond.js file, which I tried to avoid.

We need a property in a JS file that's always loaded, even if OpenPGP is disabled.

Flags: needinfo?(mkmelin+mozilla)
Comment on attachment 9111171 [details] [diff] [review]
1599031-v2.patch

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

Alright, let's go with this. r=mkmelin
Attachment #9111171 - Flags: review?(mkmelin+mozilla) → review+
Status: NEW → ASSIGNED
Flags: needinfo?(mkmelin+mozilla)

Thanks Magnus. I'm removing the checkin-needed-tb keyword. I'll land the OpenPGP related patches while we're in the initial development phase. I might depend on landing patches in a certain order.

Pushed by kaie@kuix.de:
https://hg.mozilla.org/comm-central/rev/5629275db993
Protect keyring using an automatic password that's protected with the master password. r=patrick DONTBUILD
https://hg.mozilla.org/comm-central/rev/10abecc1ae80
Add ability to query build time option MOZ_OPENPGP from JS. r=mkmelin DONTBUILD

Status: ASSIGNED → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED

(In reply to Pulsebot from comment #9)

https://hg.mozilla.org/comm-central/rev/5629275db993
Protect keyring using an automatic password that's protected with the master password. r=patrick DONTBUILD

This had an incorrect bug number in the commit message, it was for bug 1599233.

Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Please add a bug number to backout commit messages. hg qbackout does that automatically, not sure about hg backout.

And was there any particular reason to land the backout without DONTBUILD as was agreed :-(

Jörg, sorry for the missing bug number.

I think building was useful, because the build was busted, and needed to be fixed, and triggering a build to get a confirmation seemed appropriate.

The code that was backed out wasn't in the openpgp subdirectory. It was global integration code, which caused eslint to fail, and we weren't sure if it could have caused any other problems. Not sure why you're unhappy about an additional build.

It was unnecessary. Despite the linting error, the Daily build ran through OK, so a backout wouldn't have changed the result. Since after
https://treeherder.mozilla.org/#/jobs?repo=comm-central&revision=b8e5b419b13691d345f66b04ab3a936b9a0c4bea
the tree was pretty orange, there wasn't really a reason to repeat that. I don't know who cancelled the remaining jobs, but that was the correct action.

I found that all JS files that contain "#filter substitution" are listed in .eslintignore, even toolkit's AppConstants.jsm, so I'm adding our new MailConstants.jsm, too.

I've added
/* global Enigmail */
to mail/base/content/msgHdrView.js to fix the ESLint error.

Attachment #9111171 - Attachment is obsolete: true
Attachment #9112379 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9112379 [details] [diff] [review]
1599031-v4b.patch

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

This isn't a proper hg patch (missing commit message and such), but looks good otherwise
Attachment #9112379 - Flags: review?(mkmelin+mozilla) → review+

Pushed by kaie@kuix.de:
https://hg.mozilla.org/comm-central/rev/c35cc8809fea
Add ability to query build time option MOZ_OPENPGP from JS. r=mkmelin DONTBUILD

Status: REOPENED → RESOLVED
Closed: 3 months ago3 months ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 72.0
You need to log in before you can comment on or make changes to this bug.