120970 - Break up and clean charsetOverlay JS, XUL for Editor, Mail/News, Browser

Status: RESOLVED INVALID

(Core :: Internationalization, defect)

Description

[Alex Vincent [:WeirdAl]]

Spun off of bug 120275.  Our friend timeless required I separate
charsetOverlay.js and charsetOverlay.xul for 0.9.9, as a part of clean-up efforts.

Comment 1

[Alex Vincent [:WeirdAl]]

This is mine.  Whether I want it or not, a promise is a promise...

Comment 2

[Alex Vincent [:WeirdAl]]

Really reassigning.

Comment 3

[Alex Vincent [:WeirdAl]]

Attachment: partial fix for this bug

What this patch does is it breaks up the reference to the global
charsetOverlay.xul into four separate charsetOverlay.xul files.  Each
charsetOverlay.xul file will have its own charsetOverlay.js file in the same
directory.

Opinions requested on whether the files should retain the same filenames.

Comment 4

[Alex Vincent [:WeirdAl]]

Attachment: second part of patch (new files)

I'm not sure what will be required to check these files into the CVS tree.

In the XUL files, I separated the four XUL portions into four different XUL
files, removing from each instance the XUL code from the other three.  I also
broke up the XUL code such that the rows did not have too many characters.

In the JS files, I did several things:

(1) I preserved functions the corresponding XUL file permits.
(2) I preserved functions which those functions might call, and those which
might be called later in the chain, etc., etc.
(3) I removed any functions not in the chain.
(4) I moved functions which other functions call before the functions that
would call them.
(5) I moved top-level commands to the end of each script.

This is all I can do at this point.  I do not have checkin access, nor do I
have the equipment to test these changes in the various platforms.  Would
someone please create the code fragment needed to check in these files and
remove the global charsetOverlay JS and XUL files?

Comment 5

[Alex Vincent [:WeirdAl]]

Based on bz's advice, here's where the files should go imho:

chrome\content\editor\charsetOverlay.xul
chrome\content\editor\charsetOverlay.js

to:
http://lxr.mozilla.org/seamonkey/source/editor/ui/composer/content/
chrome://editor/content/

chrome\content\messenger\charsetOverlay.xul
chrome\content\messenger\charsetOverlay.js

to:
http://lxr.mozilla.org/seamonkey/source/mailnews/base/resources/content/
chrome://messenger/content/

chrome\content\messenger\messengercompose\charsetOverlay.xul
chrome\content\messenger\messengercompose\charsetOverlay.js

to:
http://lxr.mozilla.org/seamonkey/source/mailnews/compose/resources/content/
chrome://messenger/content/messengercompose/

chrome\content\navigator\charsetOverlay.xul
chrome\content\navigator\charsetOverlay.js

to:
http://lxr.mozilla.org/seamonkey/source/xpfe/browser/resources/content/
chrome://navigator/content/

Comment 6

[Roy Yokoyama]

cc'ing ftang and nhotta

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Combined patch

This has both old and new files.  The new ones have line-ending issues that
should be addressed before they get checked in.

Comment 8

[Alex Vincent [:WeirdAl]]

I've also been warned about keeping up with changes to the original
charsetOverlay.js and charsetOverlay.xul.  Far be it for me to be the one
responsible for bugs marked with the regression keyword.

http://bonsai.mozilla.org/cvslog.cgi?file=mozilla/xpfe/global/resources/content/charsetOverlay.js

The XUL file hasn't had any changes from the file I built my approach on.  

Comment 9

[nhottanscp]

Could someone explain why this is needed? It is not clear from the original report.

Comment 10

[Alex Vincent [:WeirdAl]]

timeless has briefly explained to me this would be desirable in case the user
wants to install or execute only a few of the various components.  For instance,
if the user wants to install only the browser, he should not be saddled with the
editor or mail/news code for the charset overlays.

Hopefully, timeless will speak up to clarify, if I am wrong.

Comment 11

[timeless]

the cost to load this large overlay has to be at least a bit larger than 
loading just the bit for navigator, so this change should improve navigator 
slightly.  And as mentioned in comment 10 i noted that users who don't install 
certain components (mailnews, or ideally composer) shouldn't be penalized for 
their features.

Comment 12

[nhottanscp]

Thanks for the explanations. So overlay loading performance improvement, has
that been done for other places?
Please make sure to test the menu in browser, composer and mail. Please contact
[e-mail] and [e-mail] how to test the menu.

Comment 13

[Alex Vincent [:WeirdAl]]

I understand a new testcase for this bug shall be available soon.  However, I 
shall not.

I've had a recent regression in my Internet connection.  As in dial-up, limited 
time.  Needless to say, this makes testing very difficult.

Will someone please send me an e-mail when the checkin has landed and the 
revised testcase is available?  Also, the checkin still has not received r= or 
sr=.

Comment 14

[Alex Vincent [:WeirdAl]]

I am a dunce.  The r= and sr= were waiting on my ability to test.

I'm not able, either now or in the foreseeable future, to act on this bug.  
Would someone else be kind enough to take it off my hands and see it gets 
proper love?

Resetting milestone; no way can this make it in for 0.9.9.

Comment 15

[nhottanscp]

You can reassign to me. But I do not plan to do this for moz1.0. 

Comment 16

[Alex Vincent [:WeirdAl]]

I hope the patch hasn't bit-rotted.

Any word whatsoever on a new testcase for this bug?  Give me a testcase, and
I'll tell you if it passes.

Comment 17

[Rui Xu]

Code issue, QA -> yokoyama for now.

Comment 18

[Phil Ringnalda (:philor)]

(In reply to comment #16)
> I hope the patch hasn't bit-rotted.

Just ever so slightly in the intervening seven years, since the overlay's in toolkit instead of xpfe now.