1290324 - Remove notifications.xul, notifications.dtd, newmailalert.* and alerts.totalOpenTime preference from OS X builds

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Description

[Javi Rueda :javirid]

On OS X we are using Notification Center for alert the user when new mails arrive.

However, we are still including newmailalert.* on the bundle when we release, although they are no referenced from any place for that platform.

The preference alerts.totalOpenTime is also in the same problem. The only reference is in newmailalert.* https://dxr.mozilla.org/comm-central/search?q=totalOpenTime&redirect=false

The same about notifications.*. They are not used in OS X, as the dialog box loads from the General pane Preferences and the buttin is ifded'd for not-OS X.

Test files will be affected as those files will no be found when removed, so the patch should also make something about it.

Comment 1

[Javi Rueda :javirid]

Attachment: Mozmill Tested patch

Hi :Paenglab

This patch removes these files, which are useless in the OS X package as they are not used in the platform.

notifications.* are used on the other platform from the General settings pane to setup which information should be shown on the notifications. But not on OS X.

newmailalert.* is not used in OS X as we are using the native Notification Center API to tshow the alerts. Theming CSS file for Mac doesn't exist.

alerts.totalOpenTime is used also on the other platforms for the same reason as newmailalert ones.

I ran mozmill tests (the notifications/ one in mail/test/mozmill) and there where no failed ones and 16 skipped due to "platform exclusiion".

Asking tentatively to you, but maybe :aleth is a better guess.

Comment 2

[Richard Marti (:Paenglab)]

Comment on attachment 8798247 [details] [diff] [review]
Mozmill Tested patch

The mailnews change affects also SM. Adding Ratty to be sure we don't break SM on OS X.

Comment 3

[Richard Marti (:Paenglab)]

Comment on attachment 8798247 [details] [diff] [review]
Mozmill Tested patch

LGTM

Comment 4

[aleth [:aleth]]

Comment on attachment 8798247 [details] [diff] [review]
Mozmill Tested patch

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

Not sure if TB has platform-specific default prefs. I assume the idea of this patch is to not ship some effectively dead code on OS X.

Comment 5

[Javi Rueda :javirid]

All four files have been modified on the patch by IFNDEFing for OS X, so other platforms aren't affected.

This patch aims to avoid shipping to the final user files and preferences that will not be used on OS X.

Comment 6

[Magnus Melin [:mkmelin]]

Comment on attachment 8798247 [details] [diff] [review]
Mozmill Tested patch

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

LGTM! r=mkmelin

Comment 7

[Javi Rueda :javirid]

Ping Philip Chee (:Ratty?)

Comment 8

[Philip Chee]

Comment on attachment 8798247 [details] [diff] [review]
Mozmill Tested patch

looks reasonable. Make it so.

Comment 9

[Javi Rueda :javirid]

There have been three reviews of the patch. However, it is never a bad practice to ask one more. Anything to say before I ask for check-in, :aleth?

Comment 10

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/e768dd761076c3ba33b7c277c430160507c35eaf
Bug 1290324 - Remove notifications.xul, notifications.dtd, newmailalert.* and alerts.totalOpenTime preference on OS X. r=paenglab,mkmelin,philip.chee a=philip.chee

Comment 11

[aleth [:aleth]]

That SM review took so long the patch bitrotted ;) Fixed on checkin.

Comment 12

[Jorg K (CEST = GMT+2)]

Aleth, Javi, have you noticed that ever since this landed here
https://treeherder.mozilla.org/#/jobs?repo=comm-central&revision=e768dd761076c3ba33b7c277c430160507c35eaf
there are heaps of Mozmill failures.

The tests fail like this:
18:49:33     INFO -  SUMMARY-UNEXPECTED-FAIL | test-notification.js | test-notification.js::setupTest
18:49:33     INFO -    EXCEPTION: Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIPrefBranch.getIntPref]
18:49:33     INFO -      at: nonesuch line 151
18:49:33     INFO -         setupTest test-notification.js:151 20

You removed pref("alerts.totalOpenTime", 10000); on Mac, so
  Services.prefs.setIntPref("alerts.totalOpenTime", gTotalOpenTime);
and
  gTotalOpenTime = Services.prefs.getIntPref("alerts.totalOpenTime");
  Services.prefs.setIntPref("alerts.totalOpenTime", 3000);
obviously now fail.

Please fix the test in the next few days or I'll back out this bug :-(

Comment 13

[Javi Rueda :javirid]

Working on it right now

Comment 14

[Jorg K (CEST = GMT+2)]

Thanks, I only meant to threaten a tiny bit since we need to keep our tree green and branch day is coming soon. On M-C this would have been backed out immediately with no mercy at all. Actually, it's good practice to check the tree after a landing to see whether anything broke.

Comment 15

[Javi Rueda :javirid]

Attachment: Fixing mozmill bust

It was my fault. I tested on Mozmill, but before fixing the totalOpenTime part, so it didn't failed then.

My new patch now passes all of them, excepting the ones which should be skipped.

It just doesn't do anything with the now removed preference if Mozmill is running in OS X (Darwin). It is using Services.jsm, as the rest of the file it modifies.

Could you take a look into this fix, Magnus? Thank you.

Comment 16

[Jorg K (CEST = GMT+2)]

Comment on attachment 8808797 [details] [diff] [review]
Fixing mozmill bust

Very nice. Thanks, exactly what I was going to suggest.

Let me land this with a bug of mine in the next hour.

Thanks for the quick reply.

Comment 17

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/e4ab5c338d81d9c1e457364141e9aeac5fcfe422