Reduce amount of preprocessing of content files - don't pre-process out license headers

RESOLVED FIXED in Thunderbird 16.0


7 years ago
7 years ago


(Reporter: standard8, Assigned: jcranmer)


Thunderbird 16.0
Dependency tree / graph

Thunderbird Tracking Flags

(thunderbird16 fixed, thunderbird17 fixed, thunderbird18 unaffected)



(6 attachments, 1 obsolete attachment)

Back around the time toolkit was started, there was a bit of an effort to change the license headers in content files so that they could be preprocessed out and save the size of the eventual build/download package.

Now that our headers have been changed from around 38 lines/1700ish chars to 3 lines/198 chars, I think that the benefit here is much reduced, especially when we consider that preprocessing makes it more difficult for development (need to re-generate files, slower for building, difficult to line up error console error numbers with actual code).

It looks like the Linux builds lost approx 400k when we did the relicense in comm-central, so I suspect we can afford to add a little bit more back again.

Hence I think we should remove the pre-processing where we don't need to preprocess the file for other reasons (i.e. we'll still need to preprocess for platform specific ifdefs).

Here's the process:

- Look through mail/, mailnews/ (and maybe other-licenses) for files where the start of the line contains *.
- Look through the file, if there's an ifdef or #expand, ignore it.
- If there's nothing that requires preprocessing,
-- change the license header to the native format for the file, e.g. for a js file change "# This Source" to "// This Source" etc.
-- Remove the start at the star of the line in the file.

Its probably worth recording which files have been looked at as the list is explored to save duplicating work.
This sounds like an excellent opportunity for some enterprising young contributor to flex their utility scripting skills...
And I was using "young" loosely to make the phrasing work. Any age will do. :)

Comment 3

7 years ago
Useless preprocessing is defined as the following:
1. The use of #literal (which isn't needed if it's not preprocessed)
2. A line that begins with # that doesn't match one of the keywords defined later.

This script needs to be run in the root of comm-central, and it looks through directories recursively to find the files. For all of comm-central, the affected files are:
mail/components/im/content/imContextMenu.js could lose its preprocessing:
mail/components/migration/content/migration.js could lose its preprocessing:
mail/components/search/content/searchOverlay.js has no preprocessing
mail/components/compose/content/addressingWidgetOverlay.js could lose its preprocessing:
mail/components/compose/content/editorOverlay.xul has no preprocessing
mail/components/activity/content/activity.xml has no preprocessing
mail/components/addrbook/content/abCardViewOverlay.js could lose its preprocessing:
mail/components/addrbook/content/addressbook.js could lose its preprocessing:
mail/components/addrbook/content/abCommon.js could lose its preprocessing:
mail/components/addrbook/content/abCardOverlay.js could lose its preprocessing:
mail/components/addrbook/content/abEditListDialog.xul could lose its preprocessing:
mail/components/addrbook/content/abMailListDialog.xul could lose its preprocessing:
mail/components/addrbook/content/abContactsPanel.xul could lose its preprocessing:
mail/components/addrbook/content/abContactsPanel.js has no preprocessing
mail/components/preferences/general.js could lose its preprocessing:
mail/components/preferences/display.xul has no preprocessing
mail/components/preferences/chat.xul could lose its preprocessing:
mail/components/preferences/compose.xul could lose its preprocessing:
mail/components/preferences/compose.js could lose its preprocessing:
mail/components/preferences/sendoptions.js could lose its preprocessing:
mail/components/preferences/security.xul could lose its preprocessing:
mail/components/preferences/security.js could lose its preprocessing:
mail/components/preferences/junkLog.xul could lose its preprocessing:
mail/components/preferences/junkLog.js could lose its preprocessing:
mail/components/preferences/connection.js could lose its preprocessing:
mail/components/preferences/connection.xul could lose its preprocessing:
mail/components/preferences/attachmentReminder.js could lose its preprocessing:
mail/components/preferences/attachmentReminder.xul could lose its preprocessing:
mail/components/preferences/applicationManager.xul could lose its preprocessing:
mail/components/preferences/applicationManager.js could lose its preprocessing:
mail/components/preferences/handlers.xml has no preprocessing
mail/components/preferences/handlers.css has no preprocessing
mail/components/preferences/actionsshared.js could lose its preprocessing:
mail/components/preferences/notifications.xul could lose its preprocessing:
mail/components/preferences/offline.xul could lose its preprocessing:
mail/components/preferences/cookies.js could lose its preprocessing:
mail/components/preferences/permissions.js could lose its preprocessing:
mail/components/preferences/permissionsutils.js could lose its preprocessing:
mail/base/content/hiddenWindow.js could lose its preprocessing:
mail/base/content/mailCommands.js could lose its preprocessing:
mail/base/content/customizeToolbarOverlay.xul has no preprocessing
mail/base/content/ABSearchDialog.xul could lose its preprocessing:
mail/base/content/ABSearchDialog.js could lose its preprocessing:
mail/base/content/FilterListDialog.xul could lose its preprocessing:
mail/base/content/FilterListDialog.js has no preprocessing
mail/base/content/subscribe.xul could lose its preprocessing:
mail/base/content/systemIntegrationDialog.js could lose its preprocessing:
mail/base/content/msgSelectOffline.xul could lose its preprocessing:
mail/base/content/msgPrintEngine.xul could lose its preprocessing:
mail/base/content/mail-offline.js could lose its preprocessing:
mail/base/content/newmailalert.xul could lose its preprocessing:
mail/base/content/configEditorOverlay.xul could lose its preprocessing:
mail/base/content/contentAreaClick.js has no preprocessing
mail/extensions/smime/content/msgReadSMIMEOverlay.xul could lose its preprocessing:
mail/extensions/smime/content/msgCompSMIMEOverlay.xul could lose its preprocessing:
mail/extensions/smime/content/msgCompSMIMEOverlay.js has no preprocessing
mail/extensions/smime/content/msgHdrViewSMIMEOverlay.js could lose its preprocessing:
mail/branding/nightly/content/aboutDialog.css could lose its preprocessing:
mail/branding/nightly/locales/en-US/ has no preprocessing
mail/branding/aurora/content/aboutDialog.css could lose its preprocessing:
mail/branding/aurora/locales/en-US/ has no preprocessing
mail/themes/gnomestripe/mail/chat.css could lose its preprocessing:
mail/themes/gnomestripe/../../components/im/themes/imAccounts.css could lose its preprocessing:
mail/themes/gnomestripe/../../components/im/themes/imAccountWizard.css could lose its preprocessing:
mail/themes/gnomestripe/../../components/im/themes/imRichlistbox.css has no preprocessing
mail/themes/pinstripe/mail/chat.css could lose its preprocessing:
mail/themes/pinstripe/../../components/im/themes/imAccounts.css could lose its preprocessing:
mail/themes/pinstripe/../../components/im/themes/imAccountWizard.css could lose its preprocessing:
mail/themes/pinstripe/../../components/im/themes/imRichlistbox.css has no preprocessing
mail/themes/qute/mail/chat.css could lose its preprocessing:
mail/themes/qute/../../components/im/themes/imAccounts.css could lose its preprocessing:
mail/themes/qute/../../components/im/themes/imAccountWizard.css could lose its preprocessing:
mail/themes/qute/../../components/im/themes/imRichlistbox.css has no preprocessing
mail/themes/qute/mail/primaryToolbar-aero.css could lose its preprocessing:
mail/themes/qute/mail/chat-aero.css could lose its preprocessing:
mail/themes/qute/../../components/im/themes/imAccounts.css could lose its preprocessing:
mail/themes/qute/../../components/im/themes/imAccountWizard.css could lose its preprocessing:
mail/themes/qute/../../components/im/themes/imRichlistbox.css has no preprocessing
mail/themes/qute/mail/browserRequest-aero.css could lose its preprocessing:
mail/themes/qute/mail/messenger-aero.css could lose its preprocessing:
mail/themes/qute/mail/mailWindow1-aero.css could lose its preprocessing:
mail/themes/qute/mail/tagColors-aero.css has no preprocessing
mail/themes/qute/mail/folderPane-aero.css could lose its preprocessing:
mail/themes/qute/mail/searchDialog-aero.css could lose its preprocessing:
mail/themes/qute/mail/activity/activity-aero.css has no preprocessing
mail/themes/qute/mail/addrbook/abContactsPanel-aero.css could lose its preprocessing:
mail/themes/qute/mail/cloudfile/addAccountDialog-aero.css has no preprocessing
mail/themes/qute/mail/preferences/preferences-aero.css could lose its preprocessing:
mailnews/base/search/content/searchWidgets.xml has no preprocessing
mailnews/extensions/newsblog/content/newsblogOverlay.js could lose its preprocessing:
mailnews/extensions/newsblog/content/Feed.js could lose its preprocessing:
mailnews/extensions/newsblog/content/FeedItem.js could lose its preprocessing:
mailnews/extensions/newsblog/content/feed-parser.js has no preprocessing
mailnews/extensions/newsblog/content/utils.js could lose its preprocessing:
mailnews/extensions/newsblog/content/feed-subscriptions.js could lose its preprocessing:
mailnews/extensions/newsblog/content/feed-subscriptions.xul could lose its preprocessing:
mailnews/extensions/newsblog/content/am-newsblog.xul could lose its preprocessing:
mailnews/extensions/newsblog/content/am-newsblog.js could lose its preprocessing:
calendar/sunbird/base/content/aboutDialog.css has no preprocessing
calendar/sunbird/branding/nightly/locales/en-US/ could lose its preprocessing:
calendar/lightning/content/messenger-overlay-sidebar.js has no preprocessing
sources/content/datetimepickers/datetimepickers.xml has no preprocessing
calendar/resources/content/datetimepickers/datetimepickers.xml has no preprocessing
other-licenses/branding/thunderbird/content/aboutDialog.css could lose its preprocessing:
suite/common/about.xhtml has no preprocessing
suite/common/pref/pref-applicationManager.js has no preprocessing

Comment 4

7 years ago
I wonder where I got CCed on this bug :)
Comment on attachment 628823 [details]
Bash script to find useless preprocessing

I wonder how this script compares to my attachment 626938 [details]...

Comment 6

7 years ago
(In reply to from comment #5)
> Comment on attachment 628823 [details]
> Bash script to find useless preprocessing
> I wonder how this script compares to my attachment 626938 [details]...

I don't handle css files correctly (turns out c-c doesn't need them), nor do I do l10n at all (there was only one case IIRC). I also look at the file contents to see if the preprocessing is actually useful, whereas you appear to look for files which have none. In my list, there's a distinction between "could lose its preprocessing" and "has no preprocessing."


7 years ago
Attachment #629199 - Flags: review?(iann_bugzilla) → review+

Comment 8

7 years ago
Heck, I need the sed practice. This only works for .css, .js, .jsm, and XML/HTML-ish files. There's a few wacky edge cases that it doesn't handle (*+ in files is one of them, apparently), but it's easy enough to handle everything else automatically. Patches to be posted once I make sure the output is sane.
Attachment #628823 - Attachment is obsolete: true

Comment 9

7 years ago
Assignee: nobody → Pidgeot18
Attachment #629661 - Flags: review?(mbanner)

Comment 10

7 years ago
Attachment #629662 - Flags: review?(philipp)

Comment 11

7 years ago
I've also gone ahead and fixed modelines I'm already touching in here.

Results of this patch queue on try is here: <>.
Attachment #629663 - Flags: review?(mbanner)
Attachment #629199 - Attachment description: Remove the asterix from non-preprocessed files from suite/ → Remove the asterisk from non-preprocessed files from suite/
Attachment #629661 - Flags: review?(mbanner) → review+
Component: General → Backend
Product: Thunderbird → MailNews Core
QA Contact: general → backend
Whiteboard: [mentor=Standard8][lang=js|xul]
Comment on attachment 629663 [details] [diff] [review]
Stop preprocessing in mail/

Mike's agreed to look at this for me.
Attachment #629663 - Flags: review?(mbanner) → review?(mconley)
Comment on attachment 629663 [details] [diff] [review]
Stop preprocessing in mail/

Review of attachment 629663 [details] [diff] [review]:

Whew! That was a tough review.

Glad to see it green on try.

Just 3 nits - with those fixed, r=me.

::: mail/base/content/hiddenWindow.js
@@ +1,1 @@
> +/* -*- Mode: Javascript; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

Nit - line one's comment block should not end, and we should then not have to re-open it on line 2.

::: mail/components/addrbook/content/abCardOverlay.js
@@ +1,1 @@
> +/* -*- Mode: Javascript; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

Same nit as earlier - the comment block shouldn't be closed on line 1, and a new one should not be opened on line 2.

::: mail/components/preferences/attachmentReminder.js
@@ +1,1 @@
> +/* -*- Mode: Javascript; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

This comment block should not end, and a new one should not need to be opened on line 2.
Attachment #629663 - Flags: review?(mconley) → review+


7 years ago
Blocks: 762393

Comment 14

7 years ago
When is this expected to land?
And why is the version set to 13?
Version: 13 → Trunk
Comment on attachment 629662 [details] [diff] [review]
Stop preprocessing in calendar/

Review of attachment 629662 [details] [diff] [review]:

Attachment #629662 - Flags: review?(philipp) → review+

Comment 16

7 years ago
<> (Suite part)
<> (everything else)
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 16.0
(In reply to Joshua Cranmer from comment #3)
> Useless preprocessing is defined as the following:
> 1. The use of #literal (which isn't needed if it's not preprocessed)
Fortunately we don't seem to use #literal in content.
Comment on attachment 629663 [details] [diff] [review]
Stop preprocessing in mail/

Review of attachment 629663 [details] [diff] [review]:

::: mail/components/preferences/cookies.js
@@ -634,5 @@
> -#   //    >
> -#   //
> -#   //    Before SelectedIndex: 1   Before RowCount: 4
> -#   //    After  SelectedIndex: 1   After  RowCount: 3
> -    var seln = this._view.selection;

Did you really intend to delete this line? The variable is used below (noticed this while reading JS changes to produce the extension compatibility report)
Reopened to fix lost line
Resolution: FIXED → ---


7 years ago
Attachment #665097 - Flags: review?(Pidgeot18) → review+
Last Resolved: 7 years ago7 years ago
Resolution: --- → FIXED

Comment 22

7 years ago
:irving, :jcranmer, how can I find what branch did the commit land in?

We have report of this in bug 804598. If the fix is not in TB16, we need to nominate this.


7 years ago
Blocks: 804598
It landed on trunk (that's, the question then is when, so that we know which version it landed in. The "target version" field on this bug is 16, so unless Joshua slipped filling out the bug, that's when it landed.

Good catch though, we should have nominated the fix-up patch for landing on the branches.
Comment on attachment 665097 [details] [diff] [review]
Restore line lost in the mail/ patch

[Approval Request Comment]
Regression caused by (bug #): 760126
User impact if declined: Bug 804598
Testing completed (on c-c, etc.): Several weeks on trunk
Risk to taking this patch (and alternatives if risky): Low
Attachment #665097 - Flags: approval-comm-release?
Attachment #665097 - Flags: approval-comm-beta?
Attachment #665097 - Flags: approval-comm-aurora?

Comment 25

7 years ago
The "Thunderbird 16.0" milestone was set when the original patch  (the breakage) landed on 2012-06-07. You landed the fix on 2012-09-28 so I don't think 16.0 applies to the fix, that's why I ask and need nominating to 16 branch.
Attachment #665097 - Flags: approval-comm-release?
Attachment #665097 - Flags: approval-comm-release+
Attachment #665097 - Flags: approval-comm-beta?
Attachment #665097 - Flags: approval-comm-beta+
Attachment #665097 - Flags: approval-comm-aurora?
Attachment #665097 - Flags: approval-comm-aurora+
Comment on attachment 665097 [details] [diff] [review]
Restore line lost in the mail/ patch

Actually, this is already on aurora, so we just need it in beta & release.
Attachment #665097 - Flags: approval-comm-aurora+ → approval-comm-aurora-
You need to log in before you can comment on or make changes to this bug.