313198 - Changing "Tools > Options > Alarms > Show missed alarms" gives JavaScript error

Status: RESOLVED FIXED

(Calendar :: Sunbird Only, defect)

Description

[Stefan Sitter [:ssitter]]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b5) Gecko/20051020 Firefox/1.5
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20051020 Mozilla Sunbird/0.2+

Changing "Tools > Options > Alarms > Show missed alarms" gives JavaScript error
when leaving dialog with Ok:

  Error: subject.function::getBoolPref is not a function
  Source File: chrome://calendar/content/pref/rootCalendarPref.js
  Line: 99

Reproducible: Always

Steps to Reproduce:

Comment 1

[Stefan Sitter [:ssitter]]

Attachment: Remove unnecessary function call

This fixes things for me. As this is my first patch I'm open-minded towards
feedback.

Comment 2

[Joey Minta]

Comment on attachment 200279 [details] [diff] [review]
Remove unnecessary function call

Thanks for the patch!  In the future, you should ask for review from someone
when submitting a patch.  Patches are only checked in once they're properly
reviewed.  For Sunbird, good reviewers are [e-mail] or myself.  For
lightning, [e-mail] is a good one.

Now, on to the patch...
It's true that the current call is useless.  However, this bug is a symptom of
a larger problem in rootCalendarPref.js, namely all of the calls involving
subject can fail.  For instance, I can produce a similar error by changing the
number of weeks in the multiweek view.	This time the error is

Error: subject.function::getIntPref is not a function
Source File: chrome://calendar/content/pref/rootCalendarPref.js
Line: 77

A better solution to this bug would fix all of these problems.

Hint: You'll need to learn the very basics of the QueryInterface function.

Comment 3

[Robin Edrenius]

Attachment: Patch v1

Looks like this fixes the problem for me, not sure this is the way to go though
:)

Comment 4

[Joey Minta]

Comment on attachment 200346 [details] [diff] [review]
Patch v1

Nope, this isn't it either.  This just makes the case be missed by the
switch().

Comment 5

[Stefan Sitter [:ssitter]]

Attachment: Using Components.interfaces.nsIPrefBranch

2nd guess: Using Components.interfaces.nsIPrefBranch. Not sure if the case for
"calendar.alarms.showmissed" is obsolete or not.

Comment 6

[Joey Minta]

Comment on attachment 200352 [details] [diff] [review]
Using Components.interfaces.nsIPrefBranch

Now we're getting somewhere! :-)

This is good, but it can be tightened up a bit.

+	 var pbi =
rootPrefNode.QueryInterface(Components.interfaces.nsIPrefBranch2);
+
rootPrefNode is a bad thing in itself, and I'm actually going to trash it soon,
(probably bug 306079).	|subject| will actually QI to nsIPrefBranch2 as well. 
I'd prefer if you used that instead.

-		   newWeeks.setAttribute("value", subject.getIntPref( prefName
) );
+		   newWeeks.setAttribute("value", pbi.getIntPref( prefName ) );
No need to create another  variable, either.  Just reuse subject as the result
of the QI.  That's more of a personal style thing, though.  If you really want
another variable I won't argue, but call it pb2 instead of pbi
(nsIPrefBranchInternal isn't really used anymore)

r=jminta with those changes.  (Attach another patch with them fixed and I'll
check it in.)

Comment 7

[Stefan Sitter [:ssitter]]

Attachment: Using Components.interfaces.nsIPrefBranch2, v2

Incorporate review comments from comment #6. Rename pbi to pb2, get rid of
rootPrefNode, reuse subject.

Comment 8

[Joey Minta]

Comment on attachment 200376 [details] [diff] [review]
Using Components.interfaces.nsIPrefBranch2, v2

Thanks for the patch! r=jminta

-		 if( subject.getBoolPref( prefName ) ) {
+		 if (subject.getBoolPref( prefName )) {

		 gDefaultTimezone = subject.getCharPref( prefName );
-
		 if (this.CalendarPreferences.calendarWindow.currentView !=
null) {

In the future, please be careful of random, or pseudo-random whitespace
changes.  They make cvs blame much more confusing for others to read.

Comment 9

[Joey Minta]

Patch checked in.