61059 - javascript strict warnings in directory.js

Status: RESOLVED FIXED

(SeaMonkey :: Download & File Handling, defect)

Description

[Henrik Gemal]

JavaScript strict warning:
chrome://communicator/content/directory/directory.js line 261: function OnClick
does not always return a value

JavaScript strict warning:
chrome://communicator/content/directory/directory.js line 193: reference to 
undefined property window._content.defaultCharacterset

JavaScript strict warning:
chrome://communicator/content/directory/directory.js line 201: reference to 
undefined property window._content.parentWindow

Comment 1

[Peter "jag" Annema]

I may have fixed these, let me check...

Comment 2

[Henrik Gemal]

2 warnings still here in 20010109...:

JavaScript strict warning:
chrome://communicator/content/directory/directory.js line 191: reference to 
undefined property window._content.defaultCharacterset

JavaScript strict warning:
chrome://communicator/content/directory/directory.js line 199: reference to 
undefined property window._content.parentWindow

Comment 3

[Peter "jag" Annema]

Mass move to [e-mail]

Comment 4

[Peter Trudelle]

Jag sez: Hey Mao, how 'bout taking this one too?

Comment 5

[benc]

please confirm component: is this an ftp bug?

Comment 6

[Henrik Gemal]

now also this:

Warning: function OnClick does not always return a value
Source File: chrome://communicator/content/directory/directory.js
Line: 269
Source Code:
}

Comment 7

[benc]

(big FTP bug cleanup...)

Can someone confirm this belongs in FTP? If not, I'm going to send it back to
browser-general for bug routing in the next milestone review (probably when I 
functional test 1.0)

Comment 8

[Henrik Gemal]

both FTP and local file handling...

Comment 9

[Alex Vincent [:WeirdAl]]

Attachment: patch, losing the strict warnings

Comment 10

[Alex Vincent [:WeirdAl]]

Comment on attachment 138793 [details] [diff] [review]
patch, losing the strict warnings

I took a quick look at directory.xul.  Adding return true does no damage; the
calls to OnClick() are not evaluated.

The only question mark I have is on window._content.defaultCharacterset.
Should that possibly be defaultCharacterSet?

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 138793 [details] [diff] [review]
patch, losing the strict warnings

>Index: xpfe/components/directory/directory.js
>-	  // Lets also enable the loggin window.
>+	  // Lets also enable the login window.

As long as you're here, "Let's".

>+      if (typeof window._content.defaultCharacterset != "undefined")

typeof(foo) is more readable to me; dunno if it's equivalent...

There's nothing I can find that exposes a "defaultCharacterset" property. 
There is a defaultCharacterSet property on nsIMarkupDocumentViewer, but
window._content is not such a beast... The code as written will always skip the
body of the if.

I'd rather this code were fixed to actually work than to quitly not work.

>@@ -213,16 +213,17 @@ function OnClick(event)

>+  return true;

Isn't that a behavior change?  Isn't a null return equivalent to return false? 
Or does it return whatever the last value we evaluated was or something?

Comment 12

[Alex Vincent [:WeirdAl]]

> typeof(foo) is more readable to me; dunno if it's equivalent... 
 
If you mean typeof(foo) != undefined, sure. JS doesn't care if you include extra parentheses 
around something... 
 
> I'd rather this code were fixed to actually work than to quitly not work. 
 
Good point; not sure what goes on in directory.xul and directory.js entirely anyway.  This 
was my "best guess". 
 
>  
> >@@ -213,16 +213,17 @@ function OnClick(event) 
>  
> >+  return true; 
>  
> Isn't that a behavior change?  Isn't a null return equivalent to return false?  
> Or does it return whatever the last value we evaluated was or something? 
>  
 
It doesn't result in an actual behavior change; the strict warning was for no returned value 
sometimes.  We could just as easily return false or null.  I thought returning true would be 
appropriate, considering the earlier return false was a bailout.  However, I wish to note again 
that the value this function returns is not evaluated under any circumstances, per comment 10 
and directory.xul, which calls that function. 

Comment 13

[:aceman]

There seems to be no file directory.js and no call to defaultCharacterset in m-c.

This file seems to still exist in Seamonkey, per the URL field. The call to defaultCharacterset seems to be valid there as it is initialized at http://mxr.mozilla.org/comm-central/source/suite/browser/navigator.js#1937 .

So I'll try to use the remaining fixes from the patch where appropriate.

Comment 14

[:aceman]

Attachment: patch v2

As it is for SM, I didn't test it in any way. But I used the changes from the original patch that still applied and seemed useful.

Comment 15

[[e-mail]]

Comment on attachment 8428889 [details] [diff] [review]
patch v2

>-	  // Lets also enable the loggin window.
>+    // Let's also enable the login window.
logging, actually.

>+    var node = document.getElementById("main-splitter");
>+    node.setAttribute("hidden", false);
I would normally recommend removeAttribute or just .hidden =

>-    var genDataURL = 
>-      Components.classes[WSTRING_CONTRACTID].createInstance(nsISupportsString);
> 
>     transferable.init(null);
>     transferable.addDataFlavor("text/x-moz-url");
>     transferable.addDataFlavor("text/html");
>     transferable.addDataFlavor("text/unicode");
>     
>     genDataURL.data = url + "\n" + desc;
This looks wrong.

Comment 16

[:aceman]

(In reply to [e-mail] from comment #15)
> >+    var node = document.getElementById("main-splitter");
> >+    node.setAttribute("hidden", false);
> I would normally recommend removeAttribute or just .hidden =
I just reindent this code, but I can change it as you say.

> >-    var genDataURL = 
> >-      Components.classes[WSTRING_CONTRACTID].createInstance(nsISupportsString);
> > 
> >     transferable.init(null);
> >     transferable.addDataFlavor("text/x-moz-url");
> >     transferable.addDataFlavor("text/html");
> >     transferable.addDataFlavor("text/unicode");
> >     
> >     genDataURL.data = url + "\n" + desc;
> This looks wrong.

There is a duplicate declaration of genDataURL higher in the patch so it appeared this one can be removed. Is it wrong?

Comment 17

[[e-mail]]

Comment on attachment 8428889 [details] [diff] [review]
patch v2

Sorry, I hadn't noticed that.

As for the reindenting, it's apparently possible to annotate without whitespace changes, so you don't need to bother changing .hidden at the same time.

Comment 18

[:aceman]

Attachment: patch v2.1 r=Neil [check-in comment 19]

Thanks.

Comment 19

[Philip Chee]

Checked in to comm-central:
http://hg.mozilla.org/comm-central/rev/5dd3c0e27d09