Closed Bug 397888 Opened 12 years ago Closed 10 years ago

get rid of anonymous functions

Categories

(Calendar :: Internal Components, defect, minor)

defect
Not set
minor

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: ssitter, Assigned: ssitter)

References

Details

Attachments

(1 file, 2 obsolete files)

We should get rid of anonymous functions as much as possible to improve the readability of debug information or error messages (e.g. as in Bug 387559) or profiler information (e.g. as in <http://groups.google.com/group/mozilla.dev.apps.calendar/msg/9354b4b61128db5a>).
This is the just first patch but there will be more files to fix.
Assignee: nobody → ssitter
Status: NEW → ASSIGNED
Attachment #282677 - Flags: review?(daniel.boelzle)
Comment on attachment 282677 [details] [diff] [review]
fix for calItemBase.js, calEvent.js, calTodo.js and calRecurrenceInfo.js

>+    QueryInterface: function cIB_QueryInterface(aIID) {
I'd prefer more verbose names, e.g. calItemBase_QueryInterface, so I don't need to decrypt stack traces.

>-        calcomp.prodid = "-//Mozilla Calendar//NONSGML Sunbird//EN";
>+        calcomp.prodid = "-//Mozilla.org/NONSGML Mozilla Calendar V1.1//EN";

>-        calcomp.prodid = "-//Mozilla Calendar//NONSGML Sunbird//EN";
>+        calcomp.prodid = "-//Mozilla.org/NONSGML Mozilla Calendar V1.1//EN";
leftovers

Stefan, please postpone any further work until we have checked the [checkin-needed after 0.7] pile of patches.
Attachment #282677 - Flags: review?(daniel.boelzle) → review-
I realized I was one of the big advocates of this, but see also bug 389368.
I'd actually prefer the short names, to save line length, a doubleclick on the line in the stack trace should give you the filename and therefore the context. I know this is an extra step that makes things more complicated, but the short names at least give some hints and I think that we will get used to the abbreviations.
(In reply to comment #2)
In most cases e.g. error messages, profiler traces, (stack traces?) the information about the file name is already present. Only the information about the function name is missing. Therefore I prefer the shorter function names.

(In reply to comment #3)
So the question is: How big would be the performance lost compared to the potential benefit we get from naming the functions? Bug 389368 won't help us much until it's fixed and we switch to Trunk.
This is just the up to date version of the previous patch. Waiting with review request until the open issues from Comment #5 are clarified.
Attachment #282677 - Attachment is obsolete: true
The recent performance measurement posts in mozilla.dev.apps.calendar with it's listing of "anonymous" functions makes me think we should fix this bug to improve this situation. 

Anyone wants to answer the open questions from Comment #5?
Attachment #290097 - Attachment is obsolete: true
Assignee: ssitter → nobody
Status: ASSIGNED → NEW
Depends on: 472314
The stack trace in Bug 501402 and its "anonymous" entries reminded me of this bug again. Several functions in calStorageCalendar.js are already named "stor_*", this patch takes care of the remaining ones.
Attachment #386050 - Flags: review?(philipp)
Assignee: nobody → ssitter
Status: NEW → ASSIGNED
Comment on attachment 386050 [details] [diff] [review]
[checked in] fix for calStorageCalendar.js

Using cSC_ everywhere would fit better with the scheme used in some other files, but I guess I'm ok with stor_ too.

r=philipp
Attachment #386050 - Flags: review?(philipp) → review+
Comment on attachment 386050 [details] [diff] [review]
[checked in] fix for calStorageCalendar.js

Changed 'stor_' to 'cSC_' and checked in to comm-central: <https://hg.mozilla.org/comm-central/rev/78a7e4a8a2b9>
Attachment #386050 - Attachment description: fix for calStorageCalendar.js → [checked in] fix for calStorageCalendar.js
Assignee: ssitter → nobody
Status: ASSIGNED → NEW
We have about 530 anonymous functions left, do we really want to fix them all in this bug, or should we just proceed doing so during big changes?
I just count 344, but that's also enough. ;) It's okay with me to close this bug, and do the fixing during other work or on new dedicated bugs.
"FIXED" for the checkin in comment #10 :-)
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.0
Assignee: nobody → ssitter
These bugs are likely targeted at Lightning 1.0b1, not Lightning 1.0. If this change was done in error, please adjust the target milestone to its correct value. To filter on this bugspam, you can use "lightning-10-target-move".
Target Milestone: 1.0 → 1.0b1
You need to log in before you can comment on or make changes to this bug.