1536935 - [de-xbl] remove the forked <notification> binding from thunderbird

Status: RESOLVED FIXED

(Thunderbird :: General, task)

Description

[Magnus Melin [:mkmelin]]

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

Comment 1

[Alessandro Castellani [:aleca]]

Attachment: de-xbl-notification.patch

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

Comment 2

[Magnus Melin [:mkmelin]]

Comment on attachment 9053808 [details] [diff] [review]
de-xbl-notification.patch

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

Some driveby comments:
* please use let instead of var now where you're changing things

Pleas note there is the toolkit notificationbox already https://searchfox.org/comm-central/source/mozilla/toolkit/content/widgets/notificationbox.js, so it's probably something you would just use, or extend if really needed.

::: mail/base/content/bindings.css
@@ +3,4 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  @import url("chrome://messenger/skin/textbox.css");
> +@import url("chrome://messenger/skin/notification.css");

This should not be here.
Probably the Thunderbird specific icon rules could just be in some general file like messenger.css

::: mail/base/content/notificationbox.js
@@ +37,5 @@
> +    });
> +    this.appendChild(this.stack);
> +  }
> +
> +  get _allowAnimation() {

perhaps just inline this method

@@ +42,5 @@
> +    return Services.prefs.getBoolPref("toolkit.cosmeticAnimations.enabled");
> +  }
> +
> +  get notificationsHidden() {
> +    return this.getAttribute("notificationshidden") === true;

Does this work? The attribute would be a string

@@ +62,5 @@
> +    return Array.filter(notifications, n => n != closedNotification);
> +  }
> +
> +  getNotificationWithValue(aValue) {
> +    var notifications = this.allNotifications;

perhaps just do this.allNotifications.find((n) => n.getAttribute("value") == aValue)

@@ +117,5 @@
> +    if (aPriority < this.PRIORITY_INFO_LOW ||
> +        aPriority > this.PRIORITY_CRITICAL_BLOCK)
> +      throw new Error("Invalid notification priority " + aPriority);
> +
> +    // check for where the notification should be inserted according to

Nit: capitalize Check

@@ +151,5 @@
> +      // added to the first button (unless that button has isDefault
> +      // set to false). There cannot be multiple default buttons.
> +      var defaultElem;
> +
> +      for (var b = 0; b < aButtons.length; b++) {

please use a standard i instead of b

@@ +186,5 @@
> +      }
> +    }
> +
> +    newItem.priority = aPriority;
> +    if (aPriority >= this.PRIORITY_CRITICAL_LOW)

please use braces for all if elses

@@ +347,5 @@
> +        var context = canvas.getContext("2d");
> +        context.globalAlpha = 0.5;
> +        context.drawWindow(win, win.scrollX, win.scrollY,
> +                            width, height, bgcolor);
> +      } catch (ex) { }

I wonder what this try-catch is about. Doesn't look like it would be needed

Comment 3

[Alessandro Castellani [:aleca]]

Attachment: de-xbl-notification.patch

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.

Comment 4

[Magnus Melin [:mkmelin]]

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.

Comment 5

[Alessandro Castellani [:aleca]]

Attachment: 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?

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.

Comment 6

[Magnus Melin [:mkmelin]]

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

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

Looks good, but more to go

::: mail/base/content/messenger.css
@@ +608,5 @@
> +
> +notification.animated {
> +  transition: margin-top 300ms var(--animation-easing-function), opacity 300ms var(--animation-easing-function);
> +}
> +

Most (all?) of this file is from toolkit notification.inc.css right?
You shouldn't duplicate that, but include it straight off (if it isn't already)

::: mail/base/content/messenger.xul
@@ +669,4 @@
>                        <!-- The msgNotificationBar appears on top of the message
>                             and displays information like: junk, contains remote
>                             images, or is a suspected phishing URL. -->
> +                      <hbox id="mail-notification-top">

perhaps change it to msg-.... instead of mail-.... we have newsgroups too etc.
or perhaps compose-

::: mail/components/compose/content/MsgComposeCommands.js
@@ +153,5 @@
>    msgWindow = null;
>  }
>  
> +// Notification box shown at the bottom of the window.
> +Object.defineProperty(this, "notificationbox", {

we usually denote globals with a starting g. so make this something like "gBottomNotificationBox" (we want to use it for other things than the attachment reminder too)

@@ +162,5 @@
> +
> +    let newNotificationBox = new MozElements.NotificationBox(element => {
> +      element.setAttribute("flex", "1");
> +      element.setAttribute("notificationside", "bottom");
> +      element.setAttribute("id", "attachmentNotificationBox");

... and then adjust the id too

::: mail/components/compose/content/messengercompose.xul
@@ +2078,4 @@
>              hidden="true"/>
>    </panel>
>  
> +  <hbox id="mail-bottom-notification">

same for this name

Comment 7

[Magnus Melin [:mkmelin]]

(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.

Comment 8

[Alessandro Castellani [:aleca]]

Attachment: de-xbl-notification.patch

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.

Comment 9

[Magnus Melin [:mkmelin]]

Comment on attachment 9056274 [details] [diff] [review]
de-xbl-notification.patch

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

(you can flag for feedback when the patch is not intended to be final/complete)

::: mail/base/content/mailWindowOverlay.js
@@ +2920,5 @@
> +                              remoteContentMsg,
> +                              "remoteContent",
> +                              "chrome://messenger/skin/icons/remote-blocked.svg",
> +                              this.msgNotificationBar.PRIORITY_WARNING_MEDIUM,
> +                              (aCanOverride ? buttons : []));

nit: slightly odd indention, though it's not a clear cut how to indent for cases like this if you want one thing per line. should probably put it like

let notification = .....
  remoteContentMsg,
  "remoteContent"

::: mail/base/content/messenger.css
@@ +592,5 @@
> +/* Notification box custom colors */
> +
> +notification[type="warning"] {
> +  background: #ffc;
> +  color: #0c0c0d;

not quite duplicated, but we probably want to update to what they updated to and can leave this rule out

@@ +598,5 @@
> +
> +notification[type="critical"] {
> +  background: #d70022;
> +  color: #fff;
> +}

this is still duplicated from toolkit

::: mail/components/compose/content/MsgComposeCommands.js
@@ +2159,5 @@
>      }
>    }
>  
> +  let notification = gBottomNotificationBox.notification.
> +                          getNotificationWithValue("attachmentReminder");

nit: would indent this too with under let (with two space intention)

Comment 10

[Alessandro Castellani [:aleca]]

Attachment: notification-color.png

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

Comment 11

[Magnus Melin [:mkmelin]]

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

Comment 12

[Alessandro Castellani [:aleca]]

Attachment: de-xbl-notification.patch

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?

Comment 13

[Alessandro Castellani [:aleca]]

Attachment: Screenshot from 2019-04-08 17-23-09.png

Comment 14

[Magnus Melin [:mkmelin]]

Comment on attachment 9056765 [details] [diff] [review]
de-xbl-notification.patch

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

For the im notification you may have to trigger it manually (in code)

I didn't test the telemetry now (apparently not enabled in my build) but code looks like it should work

::: mail/base/content/mailWindowOverlay.js
@@ +2952,5 @@
> +      let notification = this.msgNotificationBar.appendNotification(
> +                              phishingMsgNote, "maybeScam",
> +                              "chrome://messenger/skin/icons/phishing.png",
> +                              this.msgNotificationBar.PRIORITY_CRITICAL_MEDIUM,
> +                              buttons);

please fix this indention too

::: mail/components/compose/content/MsgComposeCommands.js
@@ +2235,5 @@
>    };
>  
> +  notification = gBottomNotificationBox.notification
> +                     .appendNotification("", "attachmentReminder",
> +                                         /* fake out the image so we can do it in CSS */

not sure what this comment was about. should be safe to remove. 
and fix the indention which is odd

::: mail/components/im/content/imAccounts.xul
@@ +148,5 @@
> +                      onselect="gAccountManager.onAccountSelect();"
> +                      ondragstart="nsDragAndDrop.startDrag(event, gAMDragAndDrop);"
> +                      ondragover="nsDragAndDrop.dragOver(event, gAMDragAndDrop);"
> +                      ondragend="gAMDragAndDrop.cleanBorders(true);"
> +                      ondrop="nsDragAndDrop.drop(event, gAMDragAndDrop);"/>

misaligned attributes (should be under id)

Comment 15

[Alessandro Castellani [:aleca]]

Attachment: de-xbl-notification.patch

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,

Comment 16

[Magnus Melin [:mkmelin]]

Comment on attachment 9057139 [details] [diff] [review]
de-xbl-notification.patch

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

::: mail/components/compose/content/MsgComposeCommands.js
@@ +155,5 @@
>  
> +// Notification box shown at the bottom of the window.
> +const gBottomNotificationBox = {}
> +
> +Object.defineProperty(gBottomNotificationBox, "notification", {

"notification" is not very distinct for an id.

Comment 17

[Magnus Melin [:mkmelin]]

(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.

Comment 18

[Geoff Lankow (:darktrojan)]

(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.

Comment 19

[Alessandro Castellani [:aleca]]

Attachment: de-xbl-notification.patch

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.

Comment 20

[Magnus Melin [:mkmelin]]

Comment on attachment 9057411 [details] [diff] [review]
de-xbl-notification.patch

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

Looks good to me, except for some minor nits below. Please have Fallen review for the calendar/ bits

::: calendar/resources/content/calendarCreation.js
@@ +146,2 @@
>          if (canAdvance && document.getElementById("calendar-uri").value &&
> +        curPage.pageid == "locationPage") {

this indention is off

@@ +213,4 @@
>      gCalendar.setProperty("color", cal_color);
>      if (!gCalendar.getProperty("cache.always")) {
>          gCalendar.setProperty("cache.enabled", gCalendar.getProperty("cache.supported") === false
> +        ? false : document.getElementById("cache").checked);

and this indention should be left alone

::: mail/base/content/notification.css
@@ +44,5 @@
> +  font-weight: bold;
> +  margin-inline-start: 0;
> +  text-decoration: underline;
> +  cursor: pointer;
> +}

#attachmentReminderText and #attachmentKeywords should stay in messengercompose.css

Comment 21

[Alessandro Castellani [:aleca]]

Attachment: de-xbl-notification.patch

Comment 22

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 9057588 [details] [diff] [review]
de-xbl-notification.patch

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

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.

::: calendar/base/content/dialogs/calendar-alarm-dialog.js
@@ +18,4 @@
>      });
>  });
>  
> +Object.defineProperty(this, "notificationbox", {

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

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

::: calendar/resources/content/calendarCreation.js
@@ +172,4 @@
>  }
>  
>  /**
> +* Create the calendar, so that the customize page can already check for

Something happened with the indent here, the first stars should be aligned. Here and in other places.

@@ +244,5 @@
>  /**
> +* Parses the given uri value to check if it is valid and there is not already
> +* a calendar with this uri.
> +*
> +* @param aUri              The string to parse as an uri.

We've lately been going with @param {Type} paramName, can you adapt?

Comment 23

[Alessandro Castellani [:aleca]]

(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.

Comment 24

[Alessandro Castellani [:aleca]]

Attachment: de-xbl-notification.patch

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?

Comment 25

[Philipp Kewisch [:Fallen][📱📆]]

(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.

Comment 26

[Philipp Kewisch [:Fallen][📱📆]]

(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 :)

Comment 27

[Magnus Melin [:mkmelin]]

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.

Comment 28

[Alessandro Castellani [:aleca]]

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?

Comment 29

[Philipp Kewisch [:Fallen][📱📆]]

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.

Comment 30

[Alessandro Castellani [:aleca]]

(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.

Comment 31

[Magnus Melin [:mkmelin]]

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

Comment 32

[Jorg K (CEST = GMT+2)]

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

Comment 33

[Alessandro Castellani [:aleca]]

Attachment: de-xbl-notification.patch

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

Comment 34

[Jorg K (CEST = GMT+2)]

Try run coming?

Comment 35

[Alessandro Castellani [:aleca]]

Attachment: de-xbl-notification.patch

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.

Comment 36

[Alessandro Castellani [:aleca]]

Attachment: de-xbl-notification.patch

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,

Comment 37

[Alessandro Castellani [:aleca]]

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?

Comment 38

[Jorg K (CEST = GMT+2)]

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.

Comment 39

[Jorg K (CEST = GMT+2)]

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

Comment 40

[Magnus Melin [:mkmelin]]

Comment on attachment 9059417 [details] [diff] [review]
de-xbl-notification.patch

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

Looks ok to me. Do the failing tests fail locally too?

::: calendar/base/content/dialogs/calendar-event-dialog-reminder.js
@@ +168,4 @@
>  function setupMaxReminders() {
>      let args = window.arguments[0];
>      let listbox = document.getElementById("reminder-listbox");
> +    // let notificationbox = document.getElementById("reminder-notifications");

should be removed?

::: mail/base/content/notification.css
@@ +32,5 @@
> +}
> +
> +notification#attachmentNotificationBox > hbox > .messageImage {
> +  background-image: url("chrome://messenger/skin/icons/attach.svg");
> +}

I guess this too should be in messengercompose.css

@@ +44,5 @@
> +}
> +
> +#reminder-notifications {
> +  padding: 0 4px 4px;
> +}
\ No newline at end of file

-> messengercompose.css?

Comment 41

[Alessandro Castellani [:aleca]]

Attachment: de-xbl-notification.patch

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.

Comment 42

[Alessandro Castellani [:aleca]]

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

Comment 43

[Jorg K (CEST = GMT+2)]

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".

Comment 44

[Alessandro Castellani [:aleca]]

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.

Comment 45

[Jorg K (CEST = GMT+2)]

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.

Comment 46

[Jorg K (CEST = GMT+2)]

Attachment: 3b0e247af0bca283ccf73a54e3830d16ff55809a - rebased

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

Comment 47

[Jorg K (CEST = GMT+2)]

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

Comment 48

[Jorg K (CEST = GMT+2)]

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.

Comment 49

[Alessandro Castellani [:aleca]]

Attachment: de-xbl-notification.patch

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.

Comment 50

[Alessandro Castellani [:aleca]]

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.

Comment 51

[Alessandro Castellani [:aleca]]

Attachment: de-xbl-notification.patch

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+

Comment 52

[Jorg K (CEST = GMT+2)]

Yay!! \o/

Comment 53

[Magnus Melin [:mkmelin]]

Comment on attachment 9060633 [details] [diff] [review]
de-xbl-notification.patch

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

Yay, looks good to me appart from the below.

::: mail/themes/shared/mail/messengercompose.css
@@ +394,5 @@
>    text-decoration: underline;
>    cursor: pointer;
>  }
> +
> +notification#attachmentNotificationBox > hbox > .messageImage {

for things with ids you shouldn't specify the type (slower)

::: mailnews/extensions/newsblog/content/newsblogOverlay.js
@@ +259,4 @@
>        }
>        // If in a non rss folder, hide possible remote content bar on a web
>        // page load, as it doesn't apply.
> +      if ("mail-notification-top" in window) {

this change is wrong I think

Comment 54

[Alessandro Castellani [:aleca]]

Attachment: de-xbl-notification.patch

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?

Comment 55

[Magnus Melin [:mkmelin]]

Comment on attachment 9060753 [details] [diff] [review]
de-xbl-notification.patch

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

::: mailnews/extensions/newsblog/content/newsblogOverlay.js
@@ +259,4 @@
>        }
>        // If in a non rss folder, hide possible remote content bar on a web
>        // page load, as it doesn't apply.
> +      if (gMessageNotificationBar.isShowingRemoteContentNotification()) {

Well, it could be showing some other notification too, like junk or scam. So this is not correct. 
But looks like this is actually wrong to start with too. "msgNotifncationBar" would not be in window but in gMessageNotificationBar. 

Maybe we don't need the check at all...

Comment 56

[Alessandro Castellani [:aleca]]

Attachment: de-xbl-notification.patch

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

Comment 57

[Magnus Melin [:mkmelin]]

Comment on attachment 9060831 [details] [diff] [review]
de-xbl-notification.patch

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

Yeah let's land!

Comment 58

[Pulsebot]

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

Comment 59

[:aceman]

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".

Comment 60

[Richard Marti (:Paenglab)]

Or maybe it's because of bug 1538983.

Comment 61

[Jorg K (CEST = GMT+2)]

(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.

Comment 62

[Jorg K (CEST = GMT+2)]

Filed bug 1547238.

Comment 63

[:aceman]

(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.

Comment 64

[Jorg K (CEST = GMT+2)]

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