55798 - javascript strict warnings in navigator.js

Status: VERIFIED FIXED

(SeaMonkey :: UI Design, defect, P3)

Description

[Mike Shaver (:shaver emeritus)]

Some strict warnings, including my all-time favourite about HTTPIndex, and some
general switching from dump() to debug() to cut down on the console clutter.

Patch soon.

Comment 1

[Mike Shaver (:shaver emeritus)]

Attachment: All the JS improvement that the law allows.

Comment 2

[Doron Rosenberg (IBM)]

keyword magic.

shaver - do you have a reviewer?

Comment 3

[Ben Goodger (use ben at mozilla dot org for email)]

r=ben

Comment 4

[Mike Shaver (:shaver emeritus)]

I have _Ben_!

Can I have brendan too?

Comment 5

[Mike Shaver (:shaver emeritus)]

I don't think this is a perf issue, so I'm anally adjusting the keywords.

Comment 6

[Doron Rosenberg (IBM)]

to my knowledge, perf peams optimizations as well, which I would consider this
to be.  *shrug*

Comment 7

[Brendan Eich [:brendan]]

+    if ( ('arguments' in window) && window.arguments.length > 1 ) {

in is a relational operator, so the parens are as unnecessary in the first
clause as in the second.


-    dump("This is before the search " + window._content.location.href + "\n");
-       dump("This is before the search " + searchStr + "\n");
+    debug("This is before the search " + window._content.location.href + "\n");
+       debug("This is before the search " + searchStr + "\n");

What, you aren't taking cvsblame and fixing tabs and other indentation probs?

+      var tagName;
+      if ('tagName' in target) tagName = target.tagName.toLowerCase();
+      var type;
+      if ('type' in target) type = target.type.toLowerCase();

don't you prefer

+      var tagName = 'tagName' in target && target.tagName.toLowerCase();
+      var type = 'type' in target && target.type.toLowerCase();

better?  I do.

/be

Comment 8

[Mike Shaver (:shaver emeritus)]

brendan: good advice, I'll adjust my patch -- and snag some other strict evil
that has come to my attention -- and repost.

doron: ``perf'' is certainly more specific than ``optimization'', especially if
you take the latter in its Latin sense.  ``perf''ormance enhancements are almost
always improvements in the speed-and-size characteristics of the system, for our
purposes.

Comment 9

[Mike Shaver (:shaver emeritus)]

So.  I've been cleaning up navigator.js (consistent brace placement and
indentation, code improvements, etc.), and it's proving to be quite the task.

I'm going to attach the diff -w so that people can read it before I'm quite
complete -- it's a big'un, so you'll want the headstart.

Generally, navigator.js seems to need a more complete architectural treatment
than this, but I don't have the time right now.

Comment 10

[Mike Shaver (:shaver emeritus)]

Attachment: diff -wu

Comment 11

[Mike Shaver (:shaver emeritus)]

Review, I crave review.

Comment 12

[Simon Lucy]

Will general applause help?
:-)

Comment 13

[Henrik Gemal]

*** Bug 57142 has been marked as a duplicate of this bug. ***

Comment 14

[Daniel Bratell]

Now I've read through the whole patch and it seems good to me FWIW (and that's 
not much). Only one thing:

-    if ( !appCore ) {
-        alert( "Error creating browser instance\n" );
-    }
+
+  if ( !appCore )
+    dump( "Error creating browser instance\n" );
   }

Isn't this error serious enough to deserve an alert dialog?

Comment 15

[Alec Flett]

I might just do some review...adding myself to the cc

Comment 16

[Alec Flett]

[s]r=alecf
man my DND cleanup is gonna conflict with this so bad, but get this in first

as for the appCore stuff, I say we simple shouldn't touch it right now because 
we're going to take a stick to it.

Comment 17

[Brendan Eich [:brendan]]

+  } catch( exception : exception == Components.results.NS_ERROR_ABORT) { 

Eek!  Shaver, you restored 'if' as the punctuator here, not ':', right?  Yup, so
says http://lxr.mozilla.org/mozilla/source/js/src/jsparse.c#1259.

/be

/be

Comment 18

[Blake Ross]

The patch for bug 12056 contains some navigator.js smacking.  I'm checking it 
in, and will work tonight to create an updated patch for this which applies 
cleanly after my changes, then will attach it.

Comment 19

[Mike Shaver (:shaver emeritus)]

alecf, blake: land your (important, functional) changes first, and clean up JS
style and correctness as you have the inclination, in areas that you touch.

My language-nits can wait until you're done (and until, as brendan points out, I
learn the freaking language). alecf, if you can make your DnD bug block this
one, I'd love ya for it.

Comment 20

[Henrik Gemal]

*** Bug 53836 has been marked as a duplicate of this bug. ***

Comment 21

[Henrik Gemal]

*** Bug 53692 has been marked as a duplicate of this bug. ***

Comment 22

[Henrik Gemal]

*** Bug 53161 has been marked as a duplicate of this bug. ***

Comment 23

[Henrik Gemal]

*** Bug 53691 has been marked as a duplicate of this bug. ***

Comment 24

[Henrik Gemal]

Sorry for the spam. Doing alot of bugzilla cleanup and dups...

Comment 25

[Peter "jag" Annema]

I'll do this rewrite as part of 46200, marking this depend on it to keep track.

Comment 26

[Henrik Gemal]

today the console goes amok:. I'm getting tons of:

JavaScript strict warning:
chrome://navigator/content/navigator.js line 1155: reference to undefined
property _content.setCursor

JavaScript error:
chrome://navigator/content/navigator.js line 1155: _content.setCursor is not a
function

and in the end:
************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "[JavaScript Error: "_content.setCursor is not a function" {file:
"chrome://navigator/content/navigator.js" line: 1151}] [nsIXULBrowserWindow::onS
tateChange]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)
"  location: "JS frame :: chrome://navigator/content/navigator.js :: loadURI ::
line 832"  data: yes]
************************************************************

Comment 27

[Henrik Gemal]

more to navigator:
JavaScript strict warning:
chrome://navigator/content/navigator.js line 617: reference to undefined propert
y node.getAttribute

JavaScript error:
chrome://navigator/content/navigator.js line 617: node.getAttribute is not a fun
ction

Comment 28

[Mike Shaver (:shaver emeritus)]

Not critical for 0.9.

Comment 29

[Henrik Gemal]

now also this:

JavaScript strict warning:
chrome://navigator/content/navigator.js line 938: reference to undefined 
property Components.classes['@mozilla.org/xpcom/leakdetector;1']

Comment 30

[Henrik Gemal]

[e-mail]: you check in some stuff in navigator that causes a whole
lof of strict warnings. I'm seeing like 40+ of

Warning: reference to undefined property webNav.postData
Source File: chrome://navigator/content/navigator.js
Line: 1426

You can/should turn on javascript strict warnings in Edit -> Prefs -> Debug!

Comment 31

[Blake Ross]

Well, no, it's because the necessary backend hasn't yet been brought over to the
trunk (this was all for the branch, now it's going onto the trunk).  Patience,
my friend, warnings won't kill you, just severely hurt you...

Comment 32

[Henrik Gemal]

In todays build 20011012 I saw:
Warning: assignment to undeclared variable kNC_Name
Source File: chrome://navigator/content/navigator.js
Line: 644

Warning: assignment to undeclared variable defaultEngine
Source File: chrome://navigator/content/navigator.js
Line: 646

Comment 33

[Alex Vincent [:WeirdAl]]

Henrik, how do you reproduce these strict warnings?

Also, as of Mozilla/5.0 (Windows; U; Win98; en-US; rv:0.9.7+) Gecko/20020105,
they appear to be lines 657 and 659.

I'll create a quick patch, but I would like to know if you can reproduce these
warnings in a current build, and how.

Comment 34

[Alex Vincent [:WeirdAl]]

Attachment: patch for Henrik's suggested strict warning

Comment 35

[Alex Vincent [:WeirdAl]]

Adding patch keyword as I cannot check in the patch I have just filed.

Comment 36

[Boris Zbarsky [:bzbarsky]]

Please put the "var defaultEngine;" on a separate line.  Shouldn't kNC_Name be a 
const instead of a var?

Comment 37

[Christian :Biesinger (don't email me, ping me on IRC)]

sending to XPApps

Comment 38

[timeless]

i'm sure someone will gladly check this in for you when it has r+sr[+a]

Comment 39

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 63705 [details] [diff] [review]
patch for Henrik's suggested strict warning

comment #36

Comment 40

[Alex Vincent [:WeirdAl]]

I'd very much like someone to tell me if they can reproduce those strict
warnings and if so, how.  Failing that, I'm going to close this bug as INVALID
in a few days.

Comment 41

[Boris Zbarsky [:bzbarsky]]

I can, no problem.  Steps:

1) Enable search button
2) Type "Mozilla crap" in the url bar
3) Bonk the search button

Comment 42

[Alex Vincent [:WeirdAl]]

Attachment: patch adjusted to put defaultEngine on new line

About time I did this. :)

Comment 43

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 75326 [details] [diff] [review]
patch adjusted to put defaultEngine on new line

r=bzbarsky

Comment 44

[Alec Flett]

Comment on attachment 75326 [details] [diff] [review]
patch adjusted to put defaultEngine on new line

sr=alecf

Comment 45

[Henrik Gemal]

please expand the patch to fix these ones too:

Warning: function OpenBookmarkGroup does not always return a value
Source File: chrome://navigator/content/navigator.js
Line: 632, Column: 65
Source Code:
  return OpenBookmarkGroupFromResource(resource, datasource, rdf);

Warning: variable resource hides argument
Source File: chrome://navigator/content/navigator.js
Line: 644, Column: 8
Source Code:
    var resource =
containerChildren.getNext().QueryInterface(Components.interfaces.nsIRDFResource);

seen in 20020326

Comment 46

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 75326 [details] [diff] [review]
patch adjusted to put defaultEngine on new line

checked in

leaving bug open for the warnings gemal mentioned

Comment 47

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch to fix new warnings

fixes the two warnings mentioned in comment 45

Comment 48

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: correct patch

Comment 49

[Fabian Guisset]

Comment on attachment 80275 [details] [diff] [review]
correct patch

r=fabian after discussion with biesi

Comment 50

[Christian :Biesinger (don't email me, ping me on IRC)]

alecf, can you super-review? thanks!

Comment 51

[Alec Flett]

I'll review warnings patches only with groups of 5 or more files (or 5 or more
warnings-bugs at a time) - so find 4 more and I'll review.

Comment 52

[Morten Nilsen]

Attachment: two more warnings

This patch gets rid of two more warnings from navigator.js

Comment 53

[Morten Nilsen]

hmm.. sorry about that :[ appears my patch was unneeded

Comment 54

[Alec Flett]

Comment on attachment 80275 [details] [diff] [review]
correct patch

sr=alecf

Comment 55

[Christian :Biesinger (don't email me, ping me on IRC)]

checked in

Comment 56

[Fuzzy Gorilla]

Is this fix going to be added to the 1.0 branch or just remain on the trunk?

Comment 57

[Alec Flett]

warnings fixes do not go on the branch, it is a waste of time and risk.

Comment 58

[sairuh (rarely reading bugmail)]

vrfy'd fixed using 2002.06.17.08 comm trunk bits (linux rh7.2).