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

RESOLVED FIXED

Status

--
trivial
RESOLVED FIXED
13 years ago
13 years ago

People

(Reporter: ssitter, Assigned: mostafah)

Tracking

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

13 years ago
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:
(Reporter)

Comment 1

13 years ago
Created attachment 200279 [details] [diff] [review]
Remove unnecessary function call

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

Comment 2

13 years ago
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 mvl@exedo.nl or myself.  For
lightning, dmose@mozilla.org 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.
Attachment #200279 - Flags: first-review-

Comment 3

13 years ago
Created attachment 200346 [details] [diff] [review]
Patch v1

Looks like this fixes the problem for me, not sure this is the way to go though
:)
Attachment #200346 - Flags: first-review?(jminta)

Comment 4

13 years ago
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().
Attachment #200346 - Flags: first-review?(jminta) → first-review-
(Reporter)

Comment 5

13 years ago
Created attachment 200352 [details] [diff] [review]
Using Components.interfaces.nsIPrefBranch

2nd guess: Using Components.interfaces.nsIPrefBranch. Not sure if the case for
"calendar.alarms.showmissed" is obsolete or not.
Attachment #200279 - Attachment is obsolete: true
Attachment #200352 - Flags: first-review?(jminta)

Comment 6

13 years ago
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.)
Attachment #200352 - Flags: first-review?(jminta) → first-review+
(Reporter)

Comment 7

13 years ago
Created attachment 200376 [details] [diff] [review]
Using Components.interfaces.nsIPrefBranch2, v2

Incorporate review comments from comment #6. Rename pbi to pb2, get rid of
rootPrefNode, reuse subject.
Attachment #200352 - Attachment is obsolete: true
Attachment #200376 - Flags: first-review?(jminta)

Comment 8

13 years ago
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.
Attachment #200376 - Flags: first-review?(jminta) → first-review+

Comment 9

13 years ago
Patch checked in.
Status: UNCONFIRMED → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.