337885 - Many xbl <setter>s don't return val

Status: RESOLVED FIXED

(Calendar :: Internal Components, defect)

Description

[Joey Minta]

When defining a <setter> in xbl, val should always be returned, to allow for some fancier bits of javascript.  There are a ton of places in the code (mostly written by me) that don't do this.

Comment 1

[Joey Minta]

All the setters in Calendar: http://landfill.mozilla.org/mxr-test/mozilla/search?string=%3Csetter%3E&find=calendar&filter=

Comment 2

[Reed Loden [:reed]]

The bugspam monkeys have struck again. They are currently chewing on default assignees for Calendar. Be afraid for your sanity!

Comment 3

[(no longer active)]

Attachment: Setter return val patch

Added "return val;"s to setter method.  Pay special attention to <http://landfill.mozilla.org/mxr-test/mozilla/source/calendar/base/content/calendar-month-view.xml#601> (that is marked with a comment in the patch.)  I wasn't sure what to do about it, so I just left it as it is now (returning this.mIndex).

Comment 4

[Joey Minta]

Comment on attachment 235864 [details] [diff] [review]
Setter return val patch

matt, can you take a first pass at this?

Comment 5

[Matthew (lilmatt) Willis]

Comment on attachment 235864 [details] [diff] [review]
Setter return val patch

>Index: calendar/base/content/calendar-month-view.xml
>===================================================================
>@@ -594,16 +596,17 @@
>         <setter><![CDATA[
>           this.mIndex = val % 7;
> 
>           var label = document.getAnonymousElementByAttribute(this, "anonid", "label");
>           var dayName = calGetString("dateFormat", "day." + (this.mIndex+1) + ".name");
>           label.setAttribute("value", dayName);
> 
>           return this.mIndex;
>+          /* should this return this.mIndex directly? */
>         ]]></setter>

I think this is fine to leave alone. It's returning val % 7, and since we're already returning a value, we may be using it elsewhere.

Otherwise, r1=lilmatt.

Nice job.

Comment 6

[Joey Minta]

Comment on attachment 235864 [details] [diff] [review]
Setter return val patch

This looks great.  I agree that returning this.mIndex is fine, since it's just a number. r2=jminta

Thanks for the patch!

Comment 7

[Matthew (lilmatt) Willis]

Patch checked in on MOZILLA_1_8_BRANCH and trunk.

-> FIXED

Thanks!