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

RESOLVED FIXED in Thunderbird 16.0

Status

MailNews Core
Backend
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: standard8, Assigned: jcranmer)

Tracking

Trunk
Thunderbird 16.0
Dependency tree / graph

Thunderbird Tracking Flags

(thunderbird16 fixed, thunderbird17 fixed, thunderbird18 unaffected)

Details

Attachments

(6 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
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 jar.mn 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 jar.mn file.

Its probably worth recording which jar.mn 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. :)
(Assignee)

Comment 3

5 years ago
Created attachment 628823 [details]
Bash script to find useless preprocessing

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 jar.mn 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/brand.properties has no preprocessing
mail/branding/aurora/content/aboutDialog.css could lose its preprocessing:
mail/branding/aurora/locales/en-US/brand.properties 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/brand.properties 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

5 years ago
I wonder where I got CCed on this bug :)

Comment 5

5 years ago
Comment on attachment 628823 [details]
Bash script to find useless preprocessing

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

Comment 6

5 years ago
(In reply to neil@parkwaycc.co.uk 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."

Comment 7

5 years ago
Created attachment 629199 [details] [diff] [review]
Remove the asterisk from non-preprocessed files from suite/
Attachment #629199 - Flags: review?(iann_bugzilla)

Updated

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

Comment 8

5 years ago
Created attachment 629537 [details]
Bash script to find and fix useless preprocessing

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 jar.mn 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
(Assignee)

Comment 9

5 years ago
Created attachment 629661 [details] [diff] [review]
Stop preprocessing several mailnews/ files
Assignee: nobody → Pidgeot18
Status: NEW → ASSIGNED
Attachment #629661 - Flags: review?(mbanner)
(Assignee)

Comment 10

5 years ago
Created attachment 629662 [details] [diff] [review]
Stop preprocessing in calendar/
Attachment #629662 - Flags: review?(philipp)
(Assignee)

Comment 11

5 years ago
Created attachment 629663 [details] [diff] [review]
Stop preprocessing in mail/

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

Results of this patch queue on try is here: <https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=10f9d750e41b>.
Attachment #629663 - Flags: review?(mbanner)

Updated

5 years ago
Attachment #629199 - Attachment description: Remove the asterix from non-preprocessed files from suite/ → Remove the asterisk from non-preprocessed files from suite/
(Reporter)

Updated

5 years ago
Attachment #629661 - Flags: review?(mbanner) → review+
(Reporter)

Updated

5 years ago
Component: General → Backend
Product: Thunderbird → MailNews Core
QA Contact: general → backend
Whiteboard: [mentor=Standard8][lang=js|xul]
(Reporter)

Comment 12

5 years ago
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+

Updated

5 years ago
Blocks: 762393

Comment 14

5 years ago
When is this expected to land?
And why is the version set to 13?

Updated

5 years ago
Version: 13 → Trunk
Comment on attachment 629662 [details] [diff] [review]
Stop preprocessing in calendar/

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

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

Comment 16

5 years ago
Pushed:
<https://hg.mozilla.org/comm-central/pushloghtml?changeset=1157389e8920> (Suite part)
<https://hg.mozilla.org/comm-central/pushloghtml?changeset=49e357c499d2> (everything else)
Status: ASSIGNED → RESOLVED
Last Resolved: 5 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 @@
> -#   //    > atwola.com
> -#   //
> -#   //    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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Created attachment 665097 [details] [diff] [review]
Restore line lost in the mail/ patch
Attachment #665097 - Flags: review?(Pidgeot18)
(Assignee)

Updated

5 years ago
Attachment #665097 - Flags: review?(Pidgeot18) → review+
https://hg.mozilla.org/comm-central/rev/59d56f2f7e80
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED

Comment 22

5 years ago
:irving, :jcranmer, how can I find what branch did the commit https://hg.mozilla.org/comm-central/rev/59d56f2f7e80 land in?

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

Updated

5 years ago
Blocks: 804598
It landed on trunk (that's hg.mozilla.org/comm-central), 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

5 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.
(Reporter)

Updated

5 years ago
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+
(Reporter)

Comment 26

5 years ago
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-
(Reporter)

Comment 27

5 years ago
https://hg.mozilla.org/releases/comm-beta/rev/4a95ecbb75f2
https://hg.mozilla.org/releases/comm-release/rev/533f194299e9
status-thunderbird16: --- → fixed
status-thunderbird17: --- → fixed
status-thunderbird18: --- → unaffected
You need to log in before you can comment on or make changes to this bug.