Closed Bug 506126 Opened 15 years ago Closed 15 years ago

globalOverlay.js is full of caller-less functions

Categories

(Firefox :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 3.6a1

People

(Reporter: dietrich, Assigned: ehsan.akhgari)

Details

(Keywords: perf, Whiteboard: [ts])

Attachments

(1 file, 1 obsolete file)

mxr showed no callers for isValidLeftClick, goOnEvent, goSetAccessKey, goSetMenuValue.

FillInTooltip's only caller is http://mxr.mozilla.org/mozilla-central/source/toolkit/obsolete/content/globalOverlay.xul, which from it's name, seems obsolete.

we should remove the dead funcs, see what's left, and evaluate if whatever's left can be put inside globalOverlay.xul, or wherever makes sense given the callers.
Keywords: perf
Whiteboard: [ts]
Attached patch Clean up globalOverlay.js (obsolete) — Splinter Review
Assignee: nobody → ehsan.akhgari
Status: NEW → ASSIGNED
Attachment #390744 - Flags: review?
Attachment #390744 - Flags: review? → review?(enndeakin)
Several of these methods are being used for Thunderbird.
(In reply to comment #2)
> Several of these methods are being used for Thunderbird.

Yes, I checked them all and the following functions were used in comm-central:

goSetMenuValue
goSetAccessKey
goOnEvent

Do we want to move them into comm-central?
How does this bug relate to perf? Surely it is more footprint than perf? No?

(In reply to comment #3)
> (In reply to comment #2)
> > Several of these methods are being used for Thunderbird.
> 
> Yes, I checked them all and the following functions were used in comm-central:
> 
> goSetMenuValue

Used by Thunderbird, SeaMonkey and Calendar

> goSetAccessKey

Used by Thunderbird & SeaMonkey

> goOnEvent

Used by Thunderbird & SeaMonkey in one place for each app.

> Do we want to move them into comm-central?

goOnEvent and goSetAccessKey could potentially be moved into mailnews/ - goOnEvent is the bigger candidate here as it is just used once.

I don't know where we'd move goSetMenuValue to as Sunbird needs that as well and we have no common place across all three apps at the moment. We'd either have to put it in mailnews/ and have Sunbird duplicate it, or find some other place.

Whilst these functions are "bloat" to Firefox they are used by at least 2/3 applications in comm-central (and possibly extensions/xulrunner apps). They are easy to implement/replicate but that's not encouraging reuse of code. So it is possible to move them, but I'm not sure it is the right thing to do.
(In reply to comment #4)
> Whilst these functions are "bloat" to Firefox they are used by at least 2/3
> applications in comm-central (and possibly extensions/xulrunner apps). They are
> easy to implement/replicate but that's not encouraging reuse of code. So it is
> possible to move them, but I'm not sure it is the right thing to do.

Yes, I agree.  Maybe we can preprocess them out in Firefox and Fennec?  I don't know what the Fennec counterpart to MOZ_PHOENIX is though.
(In reply to comment #5)
> Yes, I agree.  Maybe we can preprocess them out in Firefox and Fennec?  I don't
> know what the Fennec counterpart to MOZ_PHOENIX is though.

Preprocessing isn't an option. For one it would mean Firefox on xulrunner would be different to Firefox in its current build type. This is toolkit so there should be no app-specific ifdefs in there (I know comm-central has various, but we're slowly working towards removing them).

Is there really a huge perf impact with these functions?
(In reply to comment #6)
> (In reply to comment #5)
> > Yes, I agree.  Maybe we can preprocess them out in Firefox and Fennec?  I don't
> > know what the Fennec counterpart to MOZ_PHOENIX is though.
> 
> Preprocessing isn't an option. For one it would mean Firefox on xulrunner would
> be different to Firefox in its current build type. This is toolkit so there
> should be no app-specific ifdefs in there (I know comm-central has various, but
> we're slowly working towards removing them).

You're right.

> Is there really a huge perf impact with these functions?

No, I don't think so.  Let's keep those three functions for now unless someone has a better idea.
Attachment #390744 - Attachment is obsolete: true
Attachment #391047 - Flags: review?(enndeakin)
Attachment #390744 - Flags: review?(enndeakin)
Attachment #391047 - Flags: review?(enndeakin) → review+
(In reply to comment #7)
> > Is there really a huge perf impact with these functions?
> 
> No, I don't think so.  Let's keep those three functions for now unless someone
> has a better idea.

Yeah, I know this seems like such a small amount of code. However, on WinCE and Maemo devices for example, i/o is the #1 cause for slowness. Reducing code definitely helps, as there's less to read off disk and less to interpret. See bug 506128 for automated-dead-code-finder work.
Sorry, I meant to add that leaving these 3 functions is fine. I was commenting more on the general goodness of less code :)
http://hg.mozilla.org/mozilla-central/rev/9b321a6922e7
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.6a1
Dietrich: does mobile need this to land on 1.9.1?  If not, I don't think it's necessary to take it on 1.9.1.
Mobile isn't shipping off 1.9.1, we shouldn't take this there.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: