utilityOverlay.js needs to be cleaned and split into multiple files. There are lots of functions in that file that need to be split into other JS files. I'll fix this bug around the next alpha cycle.

Clarification: we're not thinking of making it explode, we're thinking of restoring its focus. Originally it was intended to service utilityOverlay.xul but over time it accumulated other functionality as well, probably because of its name and the lack of a true pure utility.js file. utilityOverlay.js serves as the catch-all js function shopping bag. It's rather unsuited to this because it contains several functions that are specific to the .xul file. Among these is a startup function that attaches a load listener to the window. We wanted to make a new global utility function available to Mozilla. We tried to put it in this file and then convert similar code already in place to use the function but were quickly caught in a web of users of inclusions and overlays that made a seemingly simple task troublesome. (See bug 216426.) We fancy that providing a true utility file separate from utilityOverlay.xul will make somewhat cleaner this one corner of mozilla's dodecacronic hexecontahedron. It's possible I miscounted vertices. Specifically I think the plan is to split out the seven (soon to be eight, I think) functions that have nothing to do with the .xul file, and then re-align Mozilla to match.

I'm thinking of having a general JS file (general.js?) that will hold general functions, and the utiltyOverlay.js will hold functions used by utilityOverlay.xul. Since there aren't a lot of functions that should be remove (not as many as I thought), I don't think it'd be appropriate to move all of these functions into different files (even though they're in different categories). That's why I'm thining of a general file. The functions that should be moved to general file: goPreferences(containerID, paneURL, itemID) goClickThrobber( urlPref ) goHelpMenu( url ) gatherTextUnder ( root ) GenerateValidFilename(filename, extension) Functions that are never called and should be removed: goHelpMenu( url ) extractFileNameFromUrl(urlstr) Tell me if you think that other functions should be moved/removed/stay or any other feedback.

basic patch, lots to do still. This is the file split up, and I'll correct all the XUL files later. I wasn't sure what to do with the licensing, so I just copied and pasted it with me as a contributor. Sounded right :). I don't know much about licensing. Tell me if I missed a function that should be moved or vice versa.

That's the right license. Go ahead and update the copyright notice to 1998,2004 (assuming you check it in that year). This is apparently somewhat confusing even to lawyers, but last time I brought this up with Mitchell, she thought that was the right thing. I think what needs to be done is what you did (comment 2) plus these things: delete (not used anywhere): var goPrefWindow function goHelpMenu (your patch does this, but comment 2 missed it) move to utility.js: function goAboutDialog function getBrowserURL function openTopWin function getTopWin function validateFileName getBrowserURL, openTopWin and getTopWin are an interdependent set validateFileName, GenerateValidFilename are an interdependent set goAboutDialog, goPreferences, goClickThrobber go together (and use openTopWin) Yes, all of these except validateFileName are used by utilityOverlay.xul but in my opinion they go logically with the sets I outlined above, they have nothing to do with the unique logic of utilityOverlay.xul, and except for goAboutDialog they are also used in other files, some of which would I believe no longer need to include utilityOverlay.xul with this change. Also please rename getTopWin and openTopWin. Both functions deal with the topmost navigator:browser window, but their names imply something more generic. getTopBrowserWin and openTopBrowserWin seem nice (and aren't currently being used by anyone in the codebase, according to lxr). Both functions are used in other files, but you're going to be updating them as part of this patch, anyway. It'd be nice to change GenerateValidFilename to use a lower case first character; the JavaScript standard. And please mention in your checkin comment where the contents of this new file came from, to make it easier for some unlucky future coder trying to track down the change history.

This patch contains the file split and updates the XUL files that need utility.js. This patch also includes all of danm's suggestions.

Looks good so far. Couple of questions: what about openTopWin()? I really think it should be in the same file as getTopWin(). Also goPrefWindow; never used according to lxr (though it is declared, and again not used, in Firebird's version of this file.) lxr really needs to cover Firebird. Also, is that the complete list of files that need updating? Have you teased out the interdependencies in extensions/venkman, for example? There is the interesting case of venkman-utils.js (included by, among others, venkman-bpprops.xul). It uses getBrowserURL, presumably from utilityOverlay. And venkman-bpprops.xul at least doesn't seem to include utilityOverlay.anything. I don't quite see how that works. Nor do I see where else it could be getting getBrowserURL from. More interesting, venkman-utils.js also implements its own version of openTopWin, and this function appears to be functionally equivalent. That version uses another local function, getWindowByType, which is equivalent to utilityOverlay.js getTopWin. To replace what seem to be duplicate functions from venkman with their global equivalents, or not? Regardless, venkman xul files need updating to include the new utility.js so they'll have access to getBrowserURL, don't they? Are there no others?

Patch with danm's comments. Sorry for the long wait! I got busy :). This is the patch. I kept venkman as it is because I'm sure there is a logical reason as to why they made a copy of the functions in a separate file. It could create a dependency using utilityOverlay.js that they may not want. Until I talk to the venkman developers (who I don't know who that is :)), I'm going to keep venkman as it is.

Comment on attachment 150655 [details] [diff] [review] Patch w/ danm's comments I think we're on the same page about what needs to move into utility.js. I'm concerned about repercussions. grep tells me there are 44 .xul files in the codebase that currently mention utilityOverlay and also mention at least one of the 32 .js files that themselves mention one of the functions being moved to utility.js. Those would seem to be the files that might possibly want to include utility.js and also need to be looked at to see if utilityOverlay.js can be no longer included. This is all about cleanup, after all. The very first file in my list of 44, browser/base/content/openLocation.xul, includes openLocation.js, which uses getBrowserURL. It's missing from the patch and I'm pretty sure it needs to be included. At this point I'm wondering whether we really want to do this. It's bound to break something.

danm: I'm sure it will break something. But what is more important, good code organization and cleanup or breaking something that could easily be fixed? Out of the files you listed, 16 use the Firefox utilityOverlay.js, so they can't be counted (unless they recently switched over and I hadn't heard about it).

I wish you hadn't mentioned Firefox's utilityOverlay.js. Might as well give it the same treatment, you know. Though that could be saved for dessert. Cleanup is nice, but I don't look forward to a month of bug reports about people continuously finding more and more remote things that used to work. There's a lot of tedious due diligence that could be done to prevent unnecessary breakage. In fact it seems the right thing is to check all these files for problems, and then to run exclusively with these changes on your machine for as long as it takes to flush out all the JavaScript errors. I mean I think I'd be kinda mad if I pulled a new build to discover someone had checked in something like this expecting everyone else to help debug it.

Per comment 6.

Older </xpfe/> patch, ported to </suite/>, removals only.

Cv1-SM: Checking in suite/common/utilityOverlay.js; /cvsroot/mozilla/suite/common/utilityOverlay.js,v <-- utilityOverlay.js new revision: 1.113; previous revision: 1.112 done

Comment on attachment 328363 [details] [diff] [review] (Bv1-FF) </browser/> 1 removal (Checkin: Comment 16) r=mano

Comment on attachment 328363 [details] [diff] [review] (Bv1-FF) </browser/> 1 removal (Checkin: Comment 16) http://hg.mozilla.org/mozilla-central/index.cgi/rev/81021975270c

No activity since 2008, some of this was done; any more please do in followup bugs