60662 - javascript strict warnings in overrideHandler.js

Status: RESOLVED FIXED

(SeaMonkey :: Preferences, defect, P3)

Description

[Henrik Gemal]

JavaScript strict warning:
chrome://communicator/content/pref/overrideHandler.js line 250: trailing comma 
is not legal in ECMA-262 object initializers

JavaScript strict warning:
chrome://communicator/content/pref/overrideHandler.js line 292: function 
getLiteral does not always return a value

JavaScript strict warning:
chrome://communicator/content/pref/overrideHandler.js line 305: function 
getHandlerInfoForType does not always return a value

Comment 1

[Mark Olson]

Attachment: patch - fix strict warnings

Comment 2

[Mark Olson]

Ok, this patch makes getLiteral and getHandlerInfoForType return null when no
matching value is found in the RDF.  Removed an extraneous tab while i was in there.

Comment 3

[Peter "jag" Annema]

Nit (since you're touching that):

  6 var     gRDF =
Components.classes["@mozilla.org/rdf/rdf-service;1"].getService();
  7 if (gRDF)
  8   gRDF = gRDF.QueryInterface(Components.interfaces.nsIRDFService);

There's no need for that if (gRDF)

But I'm wondering if returning null is the right thing to do. If those two
methods should never reach those points (and that's kinda what the current code
suggests, since then no value is returned), I'd rather throw an exception to
silence strict js warnings without accidentily hiding any problems.

Comment 4

[Mark Olson]

Attachment: patch - throw NS_ERROR_FAILURE instead of return null

Comment 5

[Peter "jag" Annema]

+var gRDF = Components.classes["@mozilla.org/rdf/rdf-service;1"].getService();
+gRDF = gRDF.QueryInterface(Components.interfaces.nsIRDFService);

D'oh... That's what you get for not being clear...

+var gRDF = Components.classes["@mozilla.org/rdf/rdf-service;1"]
+                     .getService(Components.interfaces.nsIRDFService);

r=jag if you make that change.

I don't think we could ever hit the throw in a valid case, but cc'ing Ben for
input on that. If it does need to be |return null;| after all, then just make
that change (and the above) and you'll have r=jag on that.

Comment 6

[timeless]

should it be |throw something;| ?

Comment 7

[Peter "jag" Annema]

timeless: ??

mao: oops... you forgot the ; after NS_ERROR_FAILURE...

Comment 8

[Mark Olson]

Copy paste error on my part, my dist dir has the correct change.  Correct patch
momentarily.

Comment 9

[Mark Olson]

Attachment: patch - back to return ""; from throw

Comment 10

[Mark Olson]

Turns out throwing an exception wasn't really what we need here.  Since these
codepaths are hit in situations where we don't have a handler or extension,
returning an empty string makes the most sense.  (Otherwise, |null| shows up as
the handler or extension).

Comment 11

[Peter "jag" Annema]

r=jag

Comment 12

[Mark Olson]

cc'ing alecf for sr

Comment 13

[Alec Flett]

sr=alecf

Comment 14

[timeless]

.

Comment 15

[timeless]

fix checked in, thanks

Comment 16

[Henrik Gemal]

Warning: reference to undefined property urns[i][2]
Source File: chrome://communicator/content/pref/overrideHandler.js
Line: 399

Comment 17

[Alex Vincent [:WeirdAl]]

Ok, the line number hasn't changed on
http://lxr.mozilla.org/seamonkey/source/xpfe/components/prefwindow/resources/content/overrideHandler.js
.

I look at the code from lines 373-377, especially 377, and I notice there isn't
a third sub element to the APP_URI line, which would cause this strict warning.

Due to a glitch in today's build (2002010508), I can't test this bug to see the
strict warning.

Anyone care to advise on this one about correct behavior?

Comment 18

[Serge Gautherie (:sgautherie)]

(Filter "spam" on 'prefs-nobody-20080612'.)

Comment 19

[Philip Chee]

> Warning: reference to undefined property urns[i][2]
> Source File: chrome://communicator/content/pref/overrideHandler.js
> Line: 399

Fixed in Bug 86640