Closed Bug 225740 Opened 17 years ago Closed 10 years ago

utilityOverlay.js needs cleaning

Categories

(SeaMonkey :: UI Design, defect, trivial)

defect
Not set
trivial

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: rjkeller, Assigned: sgautherie)

References

(Depends on 1 open bug)

Details

(Whiteboard: [Leave opened])

Attachments

(4 files, 2 obsolete files)

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.
Status: NEW → ASSIGNED
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.
Attachment #137878 - Attachment is obsolete: true
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.
Attachment #138769 - Attachment is obsolete: true
Attachment #150655 - Flags: review?(danm-moz)
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.
Attachment #150655 - Flags: review?(danm.moz) → review-
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.
Assignee: rlk → jag
Status: ASSIGNED → NEW
Product: Core → Mozilla Application Suite
Severity: normal → trivial
OS: Windows XP → All
QA Contact: pawyskoczka
Hardware: PC → All
Per comment 6.
Assignee: jag → sgautherie.bz
Status: NEW → ASSIGNED
Attachment #328363 - Flags: review?(mano)
Older </xpfe/> patch, ported to </suite/>,
removals only.
Attachment #328364 - Flags: superreview?(jag)
Attachment #328364 - Flags: review?(jag)
Attachment #328364 - Flags: superreview?(jag)
Attachment #328364 - Flags: superreview+
Attachment #328364 - Flags: review?(jag)
Attachment #328364 - Flags: review+
Keywords: checkin-needed
Whiteboard: [c-n: Cv1-SM // Leave opened]
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
Keywords: checkin-needed
Whiteboard: [c-n: Cv1-SM // Leave opened]
Attachment #328364 - Attachment description: (Cv1-SM) </suite/> 3 removals → (Cv1-SM) </suite/> 3 removals [Checkin: Comment 14]
Comment on attachment 328363 [details] [diff] [review]
(Bv1-FF) </browser/> 1 removal (Checkin: Comment 16)

r=mano
Attachment #328363 - Flags: review?(mano) → review+
Keywords: checkin-needed
Whiteboard: [c-n: Bv1-FF // Leave opened]
Depends on: 446336
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
Attachment #328363 - Attachment description: (Bv1-FF) </browser/> 1 removal → (Bv1-FF) </browser/> 1 removal (Checked in)
Keywords: checkin-needed
Whiteboard: [c-n: Bv1-FF // Leave opened] → [Leave opened]
Attachment #328363 - Attachment description: (Bv1-FF) </browser/> 1 removal (Checked in) → (Bv1-FF) </browser/> 1 removal (Checkin: see comment 16)
Attachment #328363 - Attachment description: (Bv1-FF) </browser/> 1 removal (Checkin: see comment 16) → (Bv1-FF) </browser/> 1 removal (Checkin: Comment 16)
No activity since 2008, some of this was done; any more please do in followup bugs
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
QA Contact: ui-design
You need to log in before you can comment on or make changes to this bug.