Closed Bug 120970 Opened 20 years ago Closed 9 months ago

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

Categories

(Core :: Internationalization, defect)

defect
Not set
normal

Tracking

()

RESOLVED INVALID

People

(Reporter: WeirdAl, Assigned: smontagu)

Details

(Keywords: intl, Whiteboard: [patchlove])

Attachments

(1 file, 2 obsolete files)

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.
This is mine.  Whether I want it or not, a promise is a promise...
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla0.9.9
Really reassigning.
Assignee: yokoyama → jscript
Status: ASSIGNED → NEW
Attached patch partial fix for this bug (obsolete) — Splinter Review
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.
Attached file second part of patch (new files) (obsolete) —
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?
Attachment #65813 - Attachment mime type: application/octet-stream → application/zip
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/
cc'ing ftang and nhotta
Keywords: intl
Attached patch Combined patchSplinter Review
This has both old and new files.  The new ones have line-ending issues that
should be addressed before they get checked in.
Attachment #65801 - Attachment is obsolete: true
Attachment #65813 - Attachment is obsolete: true
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.  
Could someone explain why this is needed? It is not clear from the original report.
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.
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.
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
ji@netscape.com and teruko@netscape.com how to test the menu.
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=.
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.
Target Milestone: mozilla0.9.9 → mozilla1.0
You can reassign to me. But I do not plan to do this for moz1.0. 
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.
Assignee: jscript → nhotta
Target Milestone: mozilla1.0 → mozilla1.1alpha
Status: NEW → ASSIGNED
Target Milestone: mozilla1.1alpha → mozilla1.2alpha
Code issue, QA -> yokoyama for now.
QA Contact: ruixu → yokoyama
Target Milestone: mozilla1.2alpha → ---
(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.
Assignee: nhottanscp → smontagu
Status: ASSIGNED → NEW
QA Contact: tetsuroy → i18n
Flags: in-testsuite?
OS: Windows 98 → All
Hardware: x86 → All
Whiteboard: [patchlove]
Status: NEW → RESOLVED
Closed: 9 months ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.