235879 - In <messageWindow.js>, "2" warnings; and fixing related code flow, in <mail3PaneWindowCommands.js> too.

Status: ASSIGNED

(SeaMonkey :: MailNews: Message Display, defect)

Description

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.7a) Gecko/20040219] (W98SE)

Part A:
{{
Warning: redeclaration of var loadedFolder
Source File: chrome://messenger/content/messageWindow.js
Line: 773, Column: 12
Source Code:
        var loadedFolder = GetLoadedMsgFolder();

Warning: redeclaration of var loadedFolder
Source File: chrome://messenger/content/messageWindow.js
Line: 843, Column: 12
Source Code:
        var loadedFolder = GetLoadedMsgFolder();
}}

Part B:
{{
Warning: anonymous function does not always return a value
Source File: chrome://messenger/content/messageWindow.js
Line: 857, Column: 1
Source Code:
	},
}}

I don't know what triggered this.

Comment 1

[Serge Gautherie (:sgautherie)]

Attachment: (Av1) <messageWindow.js>

Part A fix,
plus a few "cleanup".

Comment 2

[Serge Gautherie (:sgautherie)]

(In reply to comment #0)

> I don't know what triggered this.

Testcase:
1. From MailNews (3-pane), double-click a message to open it in a new window !

> Part B:
> {{
> Warning: anonymous function does not always return a value
> Source File: chrome://messenger/content/messageWindow.js
> Line: 857, Column: 1
> Source Code:
> 	},
> }}

This looks like a "false" warning from the JavaScript "engine"... (which would
need it's own bug):
brendan: you tell me !

The involved code is:
(I added a comment on the 3 places which trigger this warning)
{{
      case "cmd_createFilterFromPopup":
      case "cmd_createFilterFromMenu":
        loadedFolder = GetLoadedMsgFolder();
        if (!(loadedFolder && loadedFolder.server.canHaveFilters))
          return false;
        // *** Place number 1 ***
			case "cmd_delete":
        UpdateDeleteCommand();
        // fall through
        // *** Place number 2 ***
			case "button_delete":
			case "cmd_shiftDelete":
        loadedFolder = GetLoadedMsgFolder();
        return gCurrentMessageUri && loadedFolder &&
(loadedFolder.canDeleteMessages || isNewsURI(gCurrentFolderUri));
      case "button_junk":
        UpdateJunkToolbarButton();
        // fall through
        // *** Place number 3 ***
      case "cmd_markAsJunk":
			case "cmd_markAsNotJunk":
      case "cmd_recalculateJunkScore":
        // can't do junk on news yet
        return (!isNewsURI(gCurrentFolderUri));
}}
The JS engine fails to take into account the |return| which is after the next
|case "xxx" :| :-(

Part C:
Nevertheless, at "Place number 1",
there is no |// fall through|,
and I beleive there is a "logical" |return true| missing !!?
neil: you tell me !

Comment 3

[Brendan Eich [:brendan]]

Yes, the strict warning stuff for return vs. falling off the end of a function
in jsparse.c is cheesy.  Please file a bug on me.  Thanks,

/be

Comment 4

[Serge Gautherie (:sgautherie)]

(In reply to comment #3)
> Please file a bug on me.  Thanks,

Part B is now moved to bug 235930 :-)

(Part A and C remain here.)

Comment 5

[Serge Gautherie (:sgautherie)]

Attachment: (Av1b+Cv1) <messageWindow.js> [Checked in: Comment 18]

Av1 simplified, plus Cv1,
plus an additional "cleanup".

Comment 6

[Serge Gautherie (:sgautherie)]

Updating:
*(S) minor -> normal, as Part C is an actual code (flow) bug.

Comment 7

[[e-mail]]

Comment on attachment 142524 [details] [diff] [review]
(Av1b+Cv1) <messageWindow.js>
[Checked in: Comment 18]

That fall through certainly looks wrong... was that the only place you found?
Try also looking in the similar mail3PaneWindowCommands.js

Comment 8

[Serge Gautherie (:sgautherie)]

(In reply to comment #7)
> (From update of attachment 142524 [details] [diff] [review])
> That fall through certainly looks wrong... was that the only place you found?

Yes, there is no much doubt:
*this one was actually "found" by (testing) bug 235930 (ex-Part B) ;->
*I have "triple" checked by now :-/

> Try also looking in the similar mail3PaneWindowCommands.js

Worse there: it is now Part D :-<

Comment 9

[Serge Gautherie (:sgautherie)]

Attachment: (Dv1) <mail3PaneWindowCommands.js> (for review only)

Related fix and cleanup, per comment 7 request.

Comment 10

[Serge Gautherie (:sgautherie)]

Comment on attachment 142593 [details] [diff] [review]
(Dv1) <mail3PaneWindowCommands.js> (for review only)


'r=?':
Look for a few '// XXX' questions !

Comment 11

[Serge Gautherie (:sgautherie)]

Comment on attachment 142593 [details] [diff] [review]
(Dv1) <mail3PaneWindowCommands.js> (for review only)


'r=?':
And there are 2 |// fall through // I guess it's wanted that way !!?| to check
too !

Comment 12

[Serge Gautherie (:sgautherie)]

Attachment: (Dv1b) <mail3PaneWindowCommands> (for review only)

Dv1, plus removal of an unreachable |return false;|.

Comment 13

[[e-mail]]

Comment on attachment 142607 [details] [diff] [review]
(Dv1b) <mail3PaneWindowCommands> (for review only)

>+    var enabled = new Object();
>+    enabled.value = false;
FYI you can write this as
var enabled = { value: false };

>       case "cmd_createFilterFromPopup":
>-        var loadedFolder = GetLoadedMsgFolder();
>-        if (!(loadedFolder && loadedFolder.server.canHaveFilters))
>-          return false;
>+        loadedFolder = GetLoadedMsgFolder();
>+        return (loadedFolder && loadedFolder.server.canHaveFilters);
>       case "cmd_createFilterFromMenu":
>         loadedFolder = GetLoadedMsgFolder();
>-        if (!(loadedFolder && loadedFolder.server.canHaveFilters) || !(IsMessageDisplayedInMessagePane()))
>-          return false;
>+        return (loadedFolder && loadedFolder.server.canHaveFilters &&
>+                IsMessageDisplayedInMessagePane()); // XXX Is this needed !??
Currently it is, because that command extracts some information from the
message pane, which requires there to be a message displayed in it. I think
someone has filed a bug to make it extract the information from the message
header instead, which would make that test unnecessary. FYI,
cmd_createFilterFromPopup is only available from the message pane which is
probably why the check isn't made in that case.

>       // these are enabled on when we are in threaded mode
>       case "cmd_selectThread":
>-        if (GetNumSelectedMessages() <= 0) return false;
>+        if (GetNumSelectedMessages() == 0) // XXX It can't actually be < 0, can it ??
>+          return false;
>+        // fall through // XXX I guess it's wanted that way !!?
That's correct, all three commands are covered by the comment above.
>       case "cmd_expandAllThreads":
>       case "cmd_collapseAllThreads":
>-        if (!gDBView || !gDBView.supportsThreading) 
>-          return false;
>-        return (gDBView.viewFlags & nsMsgViewFlagsType.kThreadedDisplay);
>-        break;
>+        return (gDBView && gDBView.supportsThreading &&
>+                (gDBView.viewFlags & nsMsgViewFlagsType.kThreadedDisplay));

Comment 14

[[e-mail]]

Comment on attachment 142524 [details] [diff] [review]
(Av1b+Cv1) <messageWindow.js>
[Checked in: Comment 18]

Clearing request because patch has bitrotted.

Comment 15

[[e-mail]]

Comment on attachment 142524 [details] [diff] [review]
(Av1b+Cv1) <messageWindow.js>
[Checked in: Comment 18]

My fault, I was looking at the wrong patch.

Comment 16

[Serge Gautherie (:sgautherie)]

Comment on attachment 142524 [details] [diff] [review]
(Av1b+Cv1) <messageWindow.js>
[Checked in: Comment 18]


approval1.7b=?:
Simple cleanup, no risk;
_fixes_
{{
-	 if (!(loadedFolder && loadedFolder.server.canHaveFilters))
-	   return false;
+	 return (loadedFolder && loadedFolder.server.canHaveFilters);
}}

Comment 17

[chris hofmann]

Comment on attachment 142524 [details] [diff] [review]
(Av1b+Cv1) <messageWindow.js>
[Checked in: Comment 18]

a=chofmann for 1.7b

Comment 18

[Serge Gautherie (:sgautherie)]

Comment on attachment 142524 [details] [diff] [review]
(Av1b+Cv1) <messageWindow.js>
[Checked in: Comment 18]


Check in: { 03/15/2004 08:58	neil%parkwaycc.co.uk	mozilla/ mailnews/
base/ resources/ content/ messageWindow.js   1.95 }