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.

keyword magic. shaver - do you have a reviewer?

r=ben

I have _Ben_! Can I have brendan too?

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

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

+ 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

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.

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.

Review, I crave review.

Will general applause help? :-)

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

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?

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

[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.

+ } 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

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.

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.

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

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

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

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

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

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

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] ************************************************************

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

Not critical for 0.9.

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

blakeross@telocity.com: 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!

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...

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

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.

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

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

sending to XPApps

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

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

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.

I can, no problem. Steps: 1) Enable search button 2) Type "Mozilla crap" in the url bar 3) Bonk the search button

About time I did this. :)

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

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

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 on attachment 75326 [details] [diff] [review] patch adjusted to put defaultEngine on new line checked in leaving bug open for the warnings gemal mentioned

fixes the two warnings mentioned in comment 45

Comment on attachment 80275 [details] [diff] [review] correct patch r=fabian after discussion with biesi

alecf, can you super-review? thanks!

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.

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

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

Comment on attachment 80275 [details] [diff] [review] correct patch sr=alecf

checked in

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

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

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