Closed Bug 337885 Opened 14 years ago Closed 14 years ago

Many xbl <setter>s don't return val

Categories

(Calendar :: Internal Components, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: jminta, Assigned: ehsan)

Details

(Whiteboard: [good first bug])

Attachments

(1 file)

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.
Whiteboard: [good first bug]
The bugspam monkeys have struck again. They are currently chewing on default assignees for Calendar. Be afraid for your sanity!
Assignee: base → nobody
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).
Assignee: nobody → ehsan.akhgari
Status: NEW → ASSIGNED
Attachment #235864 - Flags: first-review?(jminta)
Comment on attachment 235864 [details] [diff] [review]
Setter return val patch

matt, can you take a first pass at this?
Attachment #235864 - Flags: second-review?(jminta)
Attachment #235864 - Flags: first-review?(mattwillis)
Attachment #235864 - Flags: first-review?(jminta)
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.
Attachment #235864 - Flags: first-review?(mattwillis) → first-review+
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!
Attachment #235864 - Flags: second-review?(jminta) → second-review+
Patch checked in on MOZILLA_1_8_BRANCH and trunk.

-> FIXED

Thanks!
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Keywords: qawanted
Keywords: qawanted
Keywords: verifyme
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.