397888 - get rid of anonymous functions

Status: RESOLVED FIXED

(Calendar :: Internal Components, defect)

Description

[Stefan Sitter [:ssitter]]

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>).

Comment 1

[Stefan Sitter [:ssitter]]

Attachment: fix for calItemBase.js, calEvent.js, calTodo.js and calRecurrenceInfo.js

This is the just first patch but there will be more files to fix.

Comment 2

[Daniel Boelzle [:dbo]]

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.

Comment 3

[Joey Minta]

I realized I was one of the big advocates of this, but see also bug 389368.

Comment 4

[Philipp Kewisch [:Fallen][☁️📆]]

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.

Comment 5

[Stefan Sitter [:ssitter]]

(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.

Comment 6

[Stefan Sitter [:ssitter]]

Attachment: fix for calItemBase.js, calEvent.js, calTodo.js and calRecurrenceInfo.js, v2

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.

Comment 7

[Stefan Sitter [:ssitter]]

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?

Comment 8

[Stefan Sitter [:ssitter]]

Attachment: [checked in] fix for calStorageCalendar.js

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.

Comment 9

[Philipp Kewisch [:Fallen][☁️📆]]

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

Comment 10

[Stefan Sitter [:ssitter]]

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>

Comment 11

[Philipp Kewisch [:Fallen][☁️📆]]

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?

Comment 12

[Martin Schröder [:mschroeder]]

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.

Comment 13

[Philipp Kewisch [:Fallen][☁️📆]]

"FIXED" for the checkin in comment #10 :-)

Comment 14

[Philipp Kewisch [:Fallen][☁️📆]]

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".