Closed Bug 55798 Opened 24 years ago Closed 22 years ago

javascript strict warnings in navigator.js

Categories

(SeaMonkey :: UI Design, defect, P3)

Product:
SeaMonkey
Component:
UI Design
Type:
defect

Tracking

(Not tracked)

Status:
VERIFIED FIXED
Milestone:
mozilla1.0

People

(Reporter: shaver, Assigned: WeirdAl)

References

Details

Attachments

(3 files, 4 obsolete files)

All the JS improvement that the law allows.
7.60 KB, patch
Details | Diff | Splinter Review
diff -wu
57.41 KB, patch
Details | Diff | Splinter Review
correct patch
1.30 KB, patch
fabian
: review+
alecf
: superreview+
Details | Diff | Splinter Review 
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?
Keywords: patch, perf
I have _Ben_!

Can I have brendan too?

I don't think this is a perf issue, so I'm anally adjusting the keywords.
Keywords: perf
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.
Assignee: ben → shaver
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.
Status: NEW → ASSIGNED
Review, I crave review.

Will general applause help?
:-)

*** Bug 57142 has been marked as a duplicate of this bug. ***
OS: Linux → All
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.
Target Milestone: --- → mozilla0.9
Summary: Strictly speaking, navigator.js needs work → Lots of strict warnings in navigator.js
*** 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...
Summary: Lots of strict warnings in navigator.js → javascript strict warnings in navigator.js
I'll do this rewrite as part of 46200, marking this depend on it to keep track.
Depends on: 46200
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.
Target Milestone: mozilla0.9 → mozilla1.0
now also this:

JavaScript strict warning:
chrome://navigator/content/navigator.js line 938: reference to undefined 
property Components.classes['@mozilla.org/xpcom/leakdetector;1']
Keywords: patch
Hardware: PC → All
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.
Keywords: patch
Please put the "var defaultEngine;" on a separate line.  Shouldn't kNC_Name be a 
const instead of a var?
sending to XPApps
Assignee: shaver → trudelle
Status: ASSIGNED → NEW
Component: Browser-General → XP Apps
QA Contact: doronr → sairuh
i'm sure someone will gladly check this in for you when it has r+sr[+a]
Assignee: trudelle → jscript
Comment on attachment 63705 [details] [diff] [review]
patch for Henrik's suggested strict warning

comment #36
Attachment #63705 - Flags: needs-work+
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. :)
Attachment #63705 - Attachment is obsolete: true
Comment on attachment 75326 [details] [diff] [review]
patch adjusted to put defaultEngine on new line

r=bzbarsky
Attachment #75326 - Flags: review+
Comment on attachment 75326 [details] [diff] [review]
patch adjusted to put defaultEngine on new line

sr=alecf
Attachment #75326 - Flags: superreview+
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
Attachment #75326 - Attachment is obsolete: true
Attached patch patch to fix new warnings (obsolete) — — Splinter Review 
fixes the two warnings mentioned in comment 45
Comment on attachment 80275 [details] [diff] [review]
correct patch

r=fabian after discussion with biesi
Attachment #80275 - Flags: review+
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.
Attached patch two more warnings (obsolete) — — Splinter 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
Attachment #80275 - Flags: superreview+
checked in
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
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).
Status: RESOLVED → VERIFIED
Product: Core → Mozilla Application Suite
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: