Open Bug 235879 Opened 20 years ago Updated 15 years ago

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

Categories

(SeaMonkey :: MailNews: Message Display, defect)

defect
Not set
normal

Tracking

(Not tracked)

ASSIGNED

People

(Reporter: sgautherie, Assigned: sgautherie)

Details

Attachments

(1 file, 3 obsolete files)

[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.
Attached patch (Av1) <messageWindow.js> (obsolete) — Splinter Review
Part A fix,
plus a few "cleanup".
Attachment #142478 - Flags: review?(neil.parkwaycc.co.uk)
(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 !
Assignee: sspitzer → gautheri
OS: Windows 98 → All
Hardware: PC → All
Status: NEW → ASSIGNED
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
(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.)
Attachment #142478 - Attachment is obsolete: true
Attachment #142478 - Flags: review?(neil.parkwaycc.co.uk)
Av1 simplified, plus Cv1,
plus an additional "cleanup".
Attachment #142524 - Flags: review?(neil.parkwaycc.co.uk)
Updating:
*(S) minor -> normal, as Part C is an actual code (flow) bug.
Severity: minor → normal
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
(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 :-<
Summary: In <messageWindow.js>, "2" warnings → In <messageWindow.js>, "2" warnings; and fixing related code flow, in <mail3PaneWindowCommands.js> too.
Related fix and cleanup, per comment 7 request.
Comment on attachment 142593 [details] [diff] [review]
(Dv1) <mail3PaneWindowCommands.js> (for review only)


'r=?':
Look for a few '// XXX' questions !
Attachment #142593 - Attachment description: (Dv1) <messageWindow.js> → (Dv1) <messageWindow.js> (for review only)
Attachment #142593 - Flags: review?(neil.parkwaycc.co.uk)
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 !
Attachment #142593 - Attachment is obsolete: true
Attachment #142593 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #142593 - Attachment description: (Dv1) <messageWindow.js> (for review only) → (Dv1) <mail3PaneWindowCommands.js> (for review only)
Dv1, plus removal of an unreachable |return false;|.
Attachment #142607 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #142607 - Attachment description: (Dv1) <mail3PaneWindowCommands> (for review only) → (Dv1b) <mail3PaneWindowCommands> (for review only)
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));
Attachment #142607 - Flags: review?(neil.parkwaycc.co.uk)
Comment on attachment 142524 [details] [diff] [review]
(Av1b+Cv1) <messageWindow.js>
[Checked in: Comment 18]

Clearing request because patch has bitrotted.
Attachment #142524 - Flags: review?(neil.parkwaycc.co.uk)
Comment on attachment 142524 [details] [diff] [review]
(Av1b+Cv1) <messageWindow.js>
[Checked in: Comment 18]

My fault, I was looking at the wrong patch.
Attachment #142524 - Flags: review+
Attachment #142524 - Flags: superreview?(mscott)
Attachment #142524 - Flags: superreview?(mscott) → superreview+
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);
}}
Attachment #142524 - Flags: approval1.7b?
Comment on attachment 142524 [details] [diff] [review]
(Av1b+Cv1) <messageWindow.js>
[Checked in: Comment 18]

a=chofmann for 1.7b
Attachment #142524 - Flags: approval1.7b? → approval1.7b+
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 }
Attachment #142524 - Attachment description: (Av1b+Cv1) <messageWindow.js> → (Av1b+Cv1) <messageWindow.js> [Checked in: Comment 18]
Attachment #142524 - Attachment is obsolete: true
Product: Browser → Seamonkey
QA Contact: esther → search
QA Contact: search → message-display
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: