We have a forked notification binding, since Firefox removed it in bug 1496827. This needs to be reworked

WIP patch!
Notifications have been de-xbl'd but more testing and a thorough clean up is necessary.

Another WIP patch with some code clean up.

I was trying to use the MozElements.NotificationBox class from the toolkit but I can't figure out the proper way of doing it.

It seems that class is not generating any XUL element and it relies on the <notification> XUL generated by the MozElements.Notification subclass.
In TB, we use the <notificationbox> element which seems to work with the custom element.
I'll keep trying to figure out how it's implemented in FF and what to do, but if you have any hint or direction, taht'd be great.

Also, is there a preference or setting to trigger the various notification throughout the entire application? So far, I've been visually testing only the "attach" notification in the compose message.

Well, for the notificationbox is seems toolkit did bug 1471403, so you will likely end up doing that first.

I would say, the approach in the WIP patch probably is not worth perusing. You should be able to simply use the toolkit widgets directly. I'd suggest to look at what they did in bug 1471403, then remove the forked notification related bindings and work your way back to a working state.

For testing, the attachment reminder is an easy one to test with. Another one is looking at an email with remote content.

I removed the code duplication and I'm implementing the built-in toolkit class to generate the notification box.

Some thoughts and questions before I continue:

1. In the MsgComposeCommand.js file I'm defining the this.notificationbox as an Object property since it's used multiple times throughout multiple methods.

I'm also creating a new MozElements.NotificationBox element inside the get notificationBar() method of the gComposeNotificationBar object, since the ID attribute is different.

I'm thinking maybe I should define a single method to create a new MozElements.NotificationBox() to be used for both cases and dynamically apply different IDs based on which object is calling that method.
Or should I define an object variable to allow the gComposeNotificationBar object to access the this.notificationbox in case is already defined, and simply remove the ID attribute?

What do you think?

2. I noticed that if the setIdentityWarning() method is triggered with the proper notification warning, the attachment reminder notification is not triggered unless I create a new message.

Is this a wanted/expected behavior or a bug? It happens also in TB 60.6.1

3. There are a lot of <notificationbox> XBL elements around the code base in the following folders:

• calendar/
• mail/base/
• mail/components
• suite/components
• suite/mailnews

Which should I replace with the new Custom Element?

Cheers.

(In reply to Alessandro Castellani (:aleca) from comment #5)

Created attachment 9054989 [details] [diff] [review]
de-xbl-notification.patch

I removed the code duplication and I'm implementing the built-in toolkit
class to generate the notification box.

Some thoughts and questions before I continue:

1. In the MsgComposeCommand.js file I'm defining the
   this.notificationbox as an Object property since it's used multiple times
   throughout multiple methods.

I'm also creating a new MozElements.NotificationBox element inside the
get notificationBar() method of the gComposeNotificationBar object,
since the ID attribute is different.

I'm thinking maybe I should define a single method to create a new
MozElements.NotificationBox() to be used for both cases and dynamically
apply different IDs based on which object is calling that method.
Or should I define an object variable to allow the gComposeNotificationBar
object to access the this.notificationbox in case is already defined, and
simply remove the ID attribute?

What do you think?

What you have now seems ok to me. It's true the id may not be necessary

2. I noticed that if the setIdentityWarning() method is triggered with the
   proper notification warning, the attachment reminder notification is not
   triggered unless I create a new message.

Is this a wanted/expected behavior or a bug?

Seems like a bug to me.

3. There are a lot of <notificationbox> XBL elements around the code base
   in the following folders:

• calendar/
• mail/base/
• mail/components
• suite/components
• suite/mailnews

The ones in calendar and unser mail/**. The ones in suite/ are for Seamonkey and that can be ignored.

Thanks for the review.
Here's another WIP patch for a quick review.

I fixed the issue with the missing attachment notification after the identity alert by using a single object that handles all the notifications for the compose message window.

I also removed the duplicated CSS classes we inherited from the old xbl-notification.css file.

I think the implementation for this section feels solid and clean enough. I'll move forward to the mail/ and calendar/ folders and update the tests accordingly.

I hope to have a complete patch for Monday end-of-day.

The background colour defined in the toolkit is drastically different from the one we're currently using.
Is it OK to adopt this?

I'll leave it up to you if you want that or not.

I converted the special tabs for telemetry and "know-your-rights".

I'm currently converting the notification in the imAccounts.js file, but it seems like this section doesn't actually work in the current version of TB.

A screenshot of my current implementation will follow this message.
Would you be able to trigger that notification in the current version of TB, or let me know how to trigger it, in order for me to compare it and be sure everything works and looks as expected?

Work is progressing, only a few old boxes are left in the calendar section.
All the converted boxes have been visually tested by triggering the notification via code in the console.

I can't seem to figure out if these 3 <notificationbox> are actually used somewhere:

• https://searchfox.org/comm-central/source/mail/base/content/messenger.xul#717
• https://searchfox.org/comm-central/source/mail/base/content/messenger.xul#730
• https://searchfox.org/comm-central/source/mail/base/content/messenger.xul#770

These don't seem to be triggered anywhere in the code and I wonder if those containers are used only to inherit the style.

Another more generic question.
A lot of files in the lightning calendar section have a tab with 4 spaces indentation. Should I reset the indentation when I stumble upon those or better not touch the sections I'm not modifying to not mess up the history?

And as a last question, I noticed that sometimes the messenger.css is not used in some calendar sections or dialogs. Should I include it to inherit the style changes to the notification element, or do you think it would be better to create a dedicated notification.css to import accordingly when needed?

Getting closer on finally shipping this, just another day of completing the calendar and updating the tests, and then it should be ready for a full review.

Cheers,

(In reply to Alessandro Castellani (:aleca) from comment #15)

I can't seem to figure out if these 3 <notificationbox> are actually used
somewhere:
*

I'm not sure if those actually are used for something. Seem maybe not. Geoff, can you confirm?

Another more generic question.
A lot of files in the lightning calendar section have a tab with 4 spaces
indentation.

Yes, calendar uses 4 space indent unfortunately (bug 1530221) so you should keep that, and also for new code.

And as a last question, I noticed that sometimes the messenger.css is not
used in some calendar sections or dialogs. Should I include it to inherit
the style changes to the notification element, or do you think it would be
better to create a dedicated notification.css to import accordingly when
needed?

Perhaps it's better to have a separate file for them.

The patch has a bunch of trailing whitespace (shows up in red through the review link). Put this under [hooks] in .hgrc

post-qrefresh.whitespace = hg qdiff | (! egrep --color -C 2 -n '^+.*\s$')

That will give you a warning if there's something to fix regarding whitespace.

(In reply to Alessandro Castellani (:aleca) from comment #15)

I can't seem to figure out if these 3 <notificationbox> are actually used somewhere:

• https://searchfox.org/comm-central/source/mail/base/content/messenger.xul#717
• https://searchfox.org/comm-central/source/mail/base/content/messenger.xul#730
• https://searchfox.org/comm-central/source/mail/base/content/messenger.xul#770

These don't seem to be triggered anywhere in the code and I wonder if those containers are used only to inherit the style.

I suspect they are not used. One did get used for add-on installation notifications, but those were moved to a popup.

There it is, the complete de-xbl'ing of the notification box.
Only one test covered a section with a notificationbox and that test hasn't been affected since I'm using the same ID.

I slightly updated some styles to increase the readability to AAA between background and foreground colours of the warning state, and created a .notification-inline class to properly render the box when inside a container that doesn't grow to the edge of the window.

(In reply to Philipp Kewisch [:Fallen] [:📆] from comment #22)

I've reviewed the calendar files, assuming you have r+ from Magnus for mail.
I've noticed a few more places you changed indent or wrapping in the mail
code. For line wrapping. Are you aiming at 80 or 100? We've been aiming for
100 in calendar.

Yes, the patch has been r+ by Magnus.
I'm aiming at 80 char limit as recommended by Magnus, should I stick to 100 for the calendar?

I think you might be able to use XPCOMUtils.defineLazyGetter() here.

I've never used it, but I can read the docs and implement it if you think it's necessary.

Since this code is repeated pretty often, do you think it makes sense to add
a helper somewhere?

We could, but I'd suggest to do it in a dedicated bug. Right now, during creation of a new MozElements.NotificationBox, different attributes and selectors are used each time. A helper method would be useful indeed, but would be better to handle it on a dedicated patch since it needs to be modular and usable throughout the entire app.
What do you think?

I fixed the comments and indentations, sorry about that.

I had to do a small update in the calendarCreation.js and define a const object in order for the notification to work after pulling the latest updates.

I fixed the indentation and applied the XPCOMUtils.defineLazyGetter() as a test. It works like a charm.
Do you want me to use this method for all the calendar related notifications?

(In reply to Alessandro Castellani (:aleca) from comment #23)

I'm aiming at 80 char limit as recommended by Magnus, should I stick to 100 for the calendar?

Magnus, is this something you want to hold on to for mail? I think 80 is pretty limiting, and the standard terminal is a bit wider nowadays. Let's aim for 100 in calendar though.

We could, but I'd suggest to do it in a dedicated bug. Right now, during creation of a new MozElements.NotificationBox, different attributes and selectors are used each time. A helper method would be useful indeed, but would be better to handle it on a dedicated patch since it needs to be modular and usable throughout the entire app.
What do you think?

The way we've usually been doing this is creating the helper for the purpose it is used for, and if others need to use it then adapt. The uses in this patch are already enough that I'd consider a helper.

(In reply to Alessandro Castellani (:aleca) from comment #24)

I fixed the indentation and applied the XPCOMUtils.defineLazyGetter() as a test. It works like a charm.
Do you want me to use this method for all the calendar related notifications?
Yes, I think it would simplify things :)

Yes, I think we still want to have the (soft) 80ch limit. It's quite useful especially for reviewing that lines are not too long.

Re XPCOMUtils.defineLazyGetter, it doesn't look like that buys you much in these cases. But, then I looked at what firefox is doing and notice that they simplified these things a bit in bug 1513508 - so perhaps not all wrong. I would suggest to do that for us too, but in a followup. https://searchfox.org/mozilla-central/rev/1b2636e8517aa48422ed516affe4d28cb7fa220a/browser/base/content/browser.js#296

Regarding helpers - personally I think it's not worth it. Especially after that followup, it's ~4 lines of code, so trying to parametrize that will not be super useful, especially as loading the function with helpers will also have it's runtime overhead.

Thanks both for your insightful feedback.
So, to quickly recap:

• Create a follow up bug to replace Object.defineProperty() with XPCOMUtils.defineLazyGetter().
• Don't create the helper for now, but explore the possibility of parametrize it during the follow up bug.
• Keep the limit to a soft 80ch and hard 100ch.

If that's correct, and the patch is r+, are we ready for a check-in?

I'm happy to defer the decision to Magnus, but I if we don't do the XPCOMUtils change now I think it is never going to happen. The followup bug would never be interesting enough to take and unless it breaks nobody will work on it. Sounds good to me otherwise.

(In reply to Philipp Kewisch [:Fallen] [:📆] from comment #29)

The followup bug would never be interesting enough to take and unless it breaks nobody will work on it.

I'd be happy to take care of the follow up bug and provide a patch in a few hours. :D
Since I've been working on this notification task for weeks, I know exactly where to go to quickly update things.

Sounds good to me too. (Not totally convinced about the helper)

Can you please do a try. I don't land stuff that hasn't been on try.

Rebased from https://hg.mozilla.org/comm-central/rev/e920876482870bfc239ab3bc3ecd1514d9958382

Try run coming?

A bit of an update on this.
I fixed all the linting issues and since the eslintrc settings in the calendar don't allow the assign on return, I decided to go ahead and implement the XPCOMUtils.defineLazyGetter, making bug 1545173 not necessary anymore.

I'm sorry to pull you into this patch once again, but after running all the tests I found out I missed a big portion of the bigFileObserver.js, and all the related tests were failing.

Since the patch went trough some substantial updates, I think would be better to get another review.

Cheers,

Here's the push to try I did: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=12d537aa4c10a5be59af15e5030975ff9ff8958c

All the mozmill tests related to the notificationbox are fixed.
Those failed tests reported are not happening when I run them on my computer.
Any suggestions?

Hard to tell right now. The X1 is a known failure, I can't see the Z2 and Z4 on the tree, but there are other new failure which cloud the situation. Let me fix those first.

Actually, those Z2 and Z4 failures were present on you previous try run as well, so I'm afraid your patch has caused them.

I updated the few things you reported, thanks.

The tests don't fail locally, everything seems good.
I did another try-push and waiting for the results.

#reminder-notifications {
padding: 0 4px 4px;
}
-> messengercompose.css?

No, this attribute is used in a calendar notification, which I moved into the proper CSS file.

Some tests are failing, not the same as the previous try push tho, and those that are failing are all teardownTest() or setupModules() method.

As usual, those tests are not failing when I run them locally.

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=72fffc9772fbf1e71de719ca33fee8f0c09562c8

Yes, but only Z2 testEventDialogModificationPrompt.js is a known issue (bug 1546130). All the other failing Z tests are not failing on the "regular" tree. I'm sorry, we can't accept the patch like this.

testLocalICS.js and testBasicFunctionality.js are failing with "EXCEPTION: menuitem is null".

Absolute, I totally agree that the patch is not acceptable.
I'll try to investigate and try to understand what's going on, but of course if anyone else wants to give it a try locally, it would be super helpful.

I ran the entire "folder-display" directory with mozmake mozmill-one SOLO_TEST=folder-display/ and it all passed, hmm.

mozmake -C comm/calendar/test/mozmill SOLO_TEST=testBasicFunctionality.js mozmill-one and mozmake -C comm/calendar/test/mozmill SOLO_TEST=testLocalICS.js mozmill-one both fail on my local debug build with:
Assertion failure: false (do not use eval with system privileges), at c:/mozilla-source/comm-central/dom/security/nsContentSecurityManager.cpp:205

If I set pref("security.allow_eval_with_system_principal", true); they both fail, both with "EXCEPTION: menuitem is null", the latter with also hung on an unanswered prompt. So these need to be fixed.

The "folder-display" stuff is more of a mystery. Here I can only suggest to turn off testEventDialogModificationPrompt.js, add testEventDialogModificationPrompt.__force_skip__ = true; right before the function. That way we make sure its failure doesn't pollute later tests.

Rebased for bug 1535265 and one hunk also for bug 1535725 (follow-up).

Here's a try with testEventDialogModificationPrompt switched off. That's already switched off on Mac, so no failures there. If my theory is correct, you have two failing tests to worry about which do fail locally.

Note: I'm mostly running this to try my patch for bug 1535725.

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=4d79e198e81d342d2315ce3a5d3365c623a53ba0

OK, Z4 broken as expected, Z2 looking better but still not perfect, I retriggered it.

Crikey, that retrigger is just terrible, the entire directory failed this time. So there's something highly unstable in there. I'll retrigger it again.

Rebased and fixed the 2 calendar failures.

So, apparently, using XPCOMUtils.defineLazyGetter() in a utils file which exports its methods, causes issues as it delays something on creation and those methods are not available.

Triggering another try-push soon.

So, I've been running local tests and a couple of try pushes to test some theories.
It seems like the issue is related to the height of the notification containers.
Here's my rational.

The errors are all related to the resize_to() method of the test-window-helpers.js file.
The issue seems to happen with the <hbox id="messenger-notification-footer"> in the messenger.xul file.

Running folder-display/ tests locally
If I don't touch anything, I get random errors of wrong visible messages, or wrong selected rows, or wrong window height sizing by a pixel or 2.
If I set a flex="1" attribute to the messenger-notification-footer, all tests pass.

Setting flex="1" is not a viable solution as it shows the notification container as an empty grey box.

Running folder-display/ tests on try-comm-central
The usual Z2 errors with the following message:

EXCEPTION: Timeout waiting for resize (current screen size: 1600X1200),
Requested width 1024 but got 1125, Request height 768 but got 768

I have no clue where those extra 101 pixels are coming from and why this is not happening locally.
I'll keep testing and upload the patch with the updated warning of the resize_to method.

I found the culprit of the extra width.
I needed to set an attribute flex="1" to the generated notification node.
That takes care of text longer than the container.

And we got a winner! Z2 tests are finally passing: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=7653c2caa2510de054457f587512f5a660434018

I'm requesting another review since I updated a bunch of stuff since the last r+

Yay!! \o/

Thanks for the review.
I updated the CSS and the condition in the newsblogOverlay.js with:

if (gMessageNotificationBar.isShowingRemoteContentNotification()) {

Since that condition is to determine if the remote content warning is shown, there's a method to check it directly instead of checking if the element exists in the window.
What do you think?

Sounds good to me, we don't need that check.
Ready for a check-in?

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/9a139533ba44
[de-xbl] remove the forked notification binding. r=mkmelin

May this have caused the failure of test-attachment-reminder.js::test_disabling_attachment_reminder on all platforms? It can be seen that the attachment notification has a button with its label duplicated: "Remind me Later Remind me Later".

Or maybe it's because of bug 1538983.

(In reply to :aceman from comment #59)

May this have caused the failure of test-attachment-reminder.js::test_disabling_attachment_reminder
on all platforms? It can be seen that the attachment notification has a button with its
label duplicated: "Remind me Later Remind me Later".

Well, the pus with this change
https://treeherder.mozilla.org/#/jobs?repo=comm-central&revision=87de40d001ca77e92342b9f1e6b5fc1f98fc4c45
was green.

The one after:
https://treeherder.mozilla.org/#/jobs?repo=comm-central&revision=dc7943c0ecc96b6565b45878a80150944941ac1a
wasn't.

So unless something in this push broke it, which I doubt, it must be something else.

A duplicated notification should be fixed here.

Filed bug 1547238.

(In reply to Richard Marti (:Paenglab) from comment #60)

Or maybe it's because of bug 1538983.

I also thought so, but it seems the tests worked with that change, but failed immediately after his one.

Aceman, the test last passed with M-C at 7e40e33da3da and this change here. Please read bug 1547238 comment #0.