OTR notification box: Only shown for one contact.
Categories
(Chat Core :: Security: OTR, defect)
Tracking
(thunderbird68 fixed, thunderbird69 fixed)
People
(Reporter: KaiE, Assigned: aleca)
References
Details
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
aleca
:
review+
aleca
:
feedback+
jorgk-bmo
:
approval-comm-beta+
|
Details | Diff | Splinter Review |
The first part of this bug is a cleanup question/request.
In https://phabricator.services.mozilla.com/D17691 regarding this code:
const gNotification = {};
XPCOMUtils.defineLazyGetter(gNotification, "notificationbox", () => {
return new MozElements.NotificationBox(element => {
element.setAttribute("flex", "1");
document.getElementById("otr-notification-box").append(element);
});
});
Florian said:
This lazy getter is strange. Why can't it be inlined at the first use?
Alex, can you please answer and potentially clean this up? (After the initial landing of bug 1518172).
Second, I found a bug related to the notification box.
- have two chat contacts
- no verifications have been done yet
(you can reset the state up by deleting file otr.fingerprints
in your profile directory) - start an OTR chat with the first contact,
and you'll get the yellow notification box - start an OTR chat with the second contact
Actual result:
- no notification box for the second contact is shown
- when switching between the contacts,
only a notification for the first contact is shown,
despite for both chat sessions, the OTR button has state "unverified"
Expected result:
- notifications for both accounts should be shown
and please also verify that:
- if the notification for one gets closed,
the notification for the second contact remains.
Please let me know if you disagree about the expected behavior.
Assignee | ||
Comment 1•5 years ago
|
||
(In reply to Kai Engert (:kaie:) from comment #0)
Florian said:
This lazy getter is strange. Why can't it be inlined at the first use?
Alex, can you please answer and potentially clean this up? (After the initial landing of bug 1518172).
This approach was first recommended by Fallen when we de-xbl'ed the notificationbox.
Mozilla is using the XPCOMUtils.defineLazyGetter
and we decided to follow it as it saves up a lot of lines of code.
Every notification in our code base is using this method, so we should keep the consistency.
Second, I found a bug related to the notification box.
Expected result:
- notifications for both accounts should be shown
Yup, that's a bug, my bad. I'll take a look at it tomorrow and submit a patch.
and please also verify that:
- if the notification for one gets closed,
the notification for the second contact remains.
Yes, that should be the expected behaviour.
I implemented the notification box to be independent and per-conversation based, but apparently some bugs slipped away.
Reporter | ||
Comment 2•5 years ago
|
||
(In reply to Alessandro Castellani (:aleca) from comment #1)
This approach was first recommended by Fallen when we de-xbl'ed the notificationbox.
Thanks, I forwarded the explanation to the code review.
I'll take a look at it tomorrow and submit a patch.
Please wait until after we landed bug 1518172.
Yes, that should be the expected behaviour.
Thanks for confirming.
Reporter | ||
Comment 4•5 years ago
|
||
Assignee | ||
Comment 5•5 years ago
|
||
Asking Feedback to Magnus for this before check-in
Comment 6•5 years ago
|
||
Updated•5 years ago
|
Assignee | ||
Comment 7•5 years ago
|
||
(In reply to Magnus Melin [:mkmelin] from comment #6)
something looks wrong here.
Why is key being put into status? In some callers it's the verified key and
in some the value is some text
That's a string key used to style the notification box based on the current status of the encryption, as shown in the otr.css
file.
#otr-notification-box notification[type="warning"][status="otr:auth-waiting"]
The different values used for that attribute are:
otr:auth-error
otr:auth-success
otr:auth-successThem
otr:auth-fail
otr:auth-waiting
I can't find where that value is the verified key, where does it happen?
Comment 8•5 years ago
|
||
Comment 9•5 years ago
|
||
Constants should really not be named like "authVerify"... Uppercased would be much more clear.
Assignee | ||
Comment 10•5 years ago
|
||
Kai, this is ready to be merged.
Do you want to wait until your patch gets reviewed and approved before mark it as checkin-needed?
Or maybe you want to make this land sooner in order to fix the notification issue?
Reporter | ||
Comment 11•5 years ago
|
||
Please go ahead and land, and please also uplift to comm-beta.
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Updated•5 years ago
|
Comment 12•5 years ago
|
||
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/2249b3ca338c
Show OTR notification box per contact. r=kaie
Updated•5 years ago
|
Updated•5 years ago
|
Comment 13•5 years ago
|
||
Updated•5 years ago
|
Description
•