Last Comment Bug 760126 - Reduce amount of preprocessing of content files - don't pre-process out license headers
: Reduce amount of preprocessing of content files - don't pre-process out licen...
Status: RESOLVED FIXED
:
Product: MailNews Core
Classification: Components
Component: Backend (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Thunderbird 16.0
Assigned To: Joshua Cranmer [:jcranmer]
:
:
Mentors:
Depends on:
Blocks: 762393 804598
  Show dependency treegraph
 
Reported: 2012-05-31 08:03 PDT by Mark Banner (:standard8, limited time in Dec)
Modified: 2012-10-25 07:22 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
fixed
fixed
unaffected


Attachments
Bash script to find useless preprocessing (1.14 KB, text/plain)
2012-05-31 11:12 PDT, Joshua Cranmer [:jcranmer]
no flags Details
Remove the asterisk from non-preprocessed files from suite/ (1.55 KB, patch)
2012-06-01 08:20 PDT, Edmund Wong (:ewong)
iann_bugzilla: review+
Details | Diff | Splinter Review
Bash script to find and fix useless preprocessing (2.10 KB, text/plain)
2012-06-02 18:27 PDT, Joshua Cranmer [:jcranmer]
no flags Details
Stop preprocessing several mailnews/ files (13.75 KB, patch)
2012-06-03 15:04 PDT, Joshua Cranmer [:jcranmer]
standard8: review+
Details | Diff | Splinter Review
Stop preprocessing in calendar/ (5.16 KB, patch)
2012-06-03 15:06 PDT, Joshua Cranmer [:jcranmer]
philipp: review+
Details | Diff | Splinter Review
Stop preprocessing in mail/ (76.68 KB, patch)
2012-06-03 15:09 PDT, Joshua Cranmer [:jcranmer]
mconley: review+
Details | Diff | Splinter Review
Restore line lost in the mail/ patch (945 bytes, patch)
2012-09-26 13:12 PDT, :Irving Reid (No longer working on Firefox)
Pidgeot18: review+
standard8: approval‑comm‑aurora-
standard8: approval‑comm‑beta+
standard8: approval‑comm‑release+
Details | Diff | Splinter Review

Description Mark Banner (:standard8, limited time in Dec) 2012-05-31 08:03:21 PDT
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.
Comment 1 Mike Conley (:mconley) 2012-05-31 08:39:33 PDT
This sounds like an excellent opportunity for some enterprising young contributor to flex their utility scripting skills...
Comment 2 Mike Conley (:mconley) 2012-05-31 08:40:24 PDT
And I was using "young" loosely to make the phrasing work. Any age will do. :)
Comment 3 Joshua Cranmer [:jcranmer] 2012-05-31 11:12:49 PDT
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 :aceman 2012-05-31 11:22:48 PDT
I wonder where I got CCed on this bug :)
Comment 5 neil@parkwaycc.co.uk 2012-06-01 07:36:32 PDT
Comment on attachment 628823 [details]
Bash script to find useless preprocessing

I wonder how this script compares to my attachment 626938 [details]...
Comment 6 Joshua Cranmer [:jcranmer] 2012-06-01 07:49:24 PDT
(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 Edmund Wong (:ewong) 2012-06-01 08:20:55 PDT
Created attachment 629199 [details] [diff] [review]
Remove the asterisk from non-preprocessed files from suite/
Comment 8 Joshua Cranmer [:jcranmer] 2012-06-02 18:27:38 PDT
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.
Comment 9 Joshua Cranmer [:jcranmer] 2012-06-03 15:04:17 PDT
Created attachment 629661 [details] [diff] [review]
Stop preprocessing several mailnews/ files
Comment 10 Joshua Cranmer [:jcranmer] 2012-06-03 15:06:56 PDT
Created attachment 629662 [details] [diff] [review]
Stop preprocessing in calendar/
Comment 11 Joshua Cranmer [:jcranmer] 2012-06-03 15:09:44 PDT
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>.
Comment 12 Mark Banner (:standard8, limited time in Dec) 2012-06-05 08:26:28 PDT
Comment on attachment 629663 [details] [diff] [review]
Stop preprocessing in mail/

Mike's agreed to look at this for me.
Comment 13 Mike Conley (:mconley) 2012-06-06 10:49:14 PDT
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.
Comment 14 :aceman 2012-06-07 00:22:03 PDT
When is this expected to land?
And why is the version set to 13?
Comment 15 Philipp Kewisch [:Fallen] 2012-06-07 04:53:51 PDT
Comment on attachment 629662 [details] [diff] [review]
Stop preprocessing in calendar/

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

r=philipp
Comment 17 neil@parkwaycc.co.uk 2012-06-23 03:54:12 PDT
(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 18 :Irving Reid (No longer working on Firefox) 2012-09-25 13:09:04 PDT
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)
Comment 19 :Irving Reid (No longer working on Firefox) 2012-09-25 13:09:29 PDT
Reopened to fix lost line
Comment 20 :Irving Reid (No longer working on Firefox) 2012-09-26 13:12:50 PDT
Created attachment 665097 [details] [diff] [review]
Restore line lost in the mail/ patch
Comment 21 :Irving Reid (No longer working on Firefox) 2012-09-28 10:57:20 PDT
https://hg.mozilla.org/comm-central/rev/59d56f2f7e80
Comment 22 :aceman 2012-10-24 07:49:09 PDT
: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.
Comment 23 :Irving Reid (No longer working on Firefox) 2012-10-24 08:01:47 PDT
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 24 :Irving Reid (No longer working on Firefox) 2012-10-24 08:03:25 PDT
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
Comment 25 :aceman 2012-10-24 08:05:28 PDT
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.
Comment 26 Mark Banner (:standard8, limited time in Dec) 2012-10-25 07:15:34 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.