If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Incorrect RDF Data Source function prototypes following nsISupportsArray patch in bug 435290 (IsCommandEnabled, DoCommand)

RESOLVED FIXED in mozilla1.9

Status

MailNews Core
Backend
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: standard8, Assigned: sid0)

Tracking

(Blocks: 1 bug, {regression})

Trunk
mozilla1.9
regression
Dependency tree / graph
Bug Flags:
blocking-thunderbird3.0a2 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

9 years ago
Compiling with the latest trunk I'm seeing the following errors when compiling nsMailModule.cpp:

/Users/moztest/mozilla/hg/mozilla/mailnews/build/../base/src/nsMsgRDFDataSource.h:68: warning: ‘virtual nsresult nsMsgRDFDataSource::IsCommandEnabled(nsISupportsArray*, nsIRDFResource*, nsISupportsArray*, PRBool*)’ was hidden
/Users/moztest/mozilla/hg/mozilla/mailnews/build/../base/src/nsMsgFolderDataSource.h:117: warning:   by ‘virtual nsresult nsMsgFolderDataSource::IsCommandEnabled(nsIArray*, nsIRDFResource*, nsIArray*, PRBool*)’
/Users/moztest/mozilla/hg/mozilla/mailnews/build/../base/src/nsMsgRDFDataSource.h:68: warning: ‘virtual nsresult nsMsgRDFDataSource::DoCommand(nsISupportsArray*, nsIRDFResource*, nsISupportsArray*)’ was hidden
/Users/moztest/mozilla/hg/mozilla/mailnews/build/../base/src/nsMsgFolderDataSource.h:121: warning:   by ‘virtual nsresult nsMsgFolderDataSource::DoCommand(nsIArray*, nsIRDFResource*, nsIMutableArray*)’

nsMsgFolderDataSource inherits via nsMsgRDFDataSource from nsIRDFDataSource.idl

I'm not sure if we really make use of DoCommand and IsCommandEnabled, but its not a good idea to change those function prototypes.

Seeing as we're not going to able to change nsIRDFDataSource (core code), it looks as if we're going to have to back these bits out from bug 435290?
Flags: blocking-thunderbird3.0a2?
(Assignee)

Comment 1

9 years ago
Unfortunately everything down the stack from there uses the same arrays, so pretty much the whole bug will have to be backed out.

I'm pretty sure nothing in JS uses the RDF commands anymore. I'm looking this up.

Comment 2

9 years ago
Should fix this for a2.
Flags: blocking-thunderbird3.0a2? → blocking-thunderbird3.0a2+
(Assignee)

Comment 3

9 years ago
Created attachment 323308 [details] [diff] [review]
removed DoCommand, IsCommandEnabled and related functions

They almost certainly aren't used any more.
Assignee: nobody → sid1337
Status: NEW → ASSIGNED
(Assignee)

Comment 4

9 years ago
To elaborate, local and mxr greps on the entire mail/ and mailnews/ folders produced no results which were relevant. There is a separate DoCommand in nsMsgDBView.cpp, http://mxr.mozilla.org/mozilla/source/mailnews/base/src/nsMsgDBView.cpp#2231 , with a different function signature, and this is the only one that's used.

grep: http://mxr.mozilla.org/mozilla/search?string=DoCommand&find=%2Fmail&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla
(Assignee)

Updated

9 years ago
Attachment #323308 - Flags: review?(bugzilla)

Comment 5

9 years ago
Did you actually *check* that they aren't used anymore?
The whole command stuff is core XUL code and so mxring only mailnews and mail does sound a bit frightening...
(Assignee)

Comment 6

9 years ago
Created attachment 323315 [details] [diff] [review]
Conservative patch

This is a conservative patch that reverts to nsISupportsArray, but copies data to nsIMutableArrays wherever required (well, just one place it seems). Either this patch or the other should be checked in.

Karsten: I'm slightly confused. Do you think that this code might be accessed in other places through the nsIRDFDataSource interface?
Attachment #323308 - Attachment is obsolete: true
Attachment #323315 - Flags: review?(bugzilla)
Attachment #323308 - Flags: review?(bugzilla)
(Assignee)

Updated

9 years ago
Attachment #323308 - Attachment is obsolete: false
Attachment #323308 - Flags: review?(bugzilla)
(Assignee)

Updated

9 years ago
Blocks: 430614
(Reporter)

Comment 7

9 years ago
Comment on attachment 323308 [details] [diff] [review]
removed DoCommand, IsCommandEnabled and related functions

I think given that we're working on removing rdf, and the uncertainty over being sure this isn't required, we should go for the conservative patch.
Attachment #323308 - Flags: review?(bugzilla)
(Reporter)

Comment 8

9 years ago
Comment on attachment 323315 [details] [diff] [review]
Conservative patch

+    // Create an nsIMutableArray from the nsISupportsArray
+    nsCOMPtr<nsIMutableArray> folderArray(do_CreateInstance(NS_ARRAY_CONTRACTID));
+    for (PRUint32 i = 0; i < itemCount; i++)
+    {
+      folderArray->AppendElement(arguments->ElementAt(i), PR_FALSE);
+    }

You don't need the braces around this.

r=me with those removed.
Attachment #323315 - Flags: review?(bugzilla) → review+
(Assignee)

Comment 9

9 years ago
Created attachment 323683 [details] [diff] [review]
Update to conservative patch, addressing comment

(carrying forward r=Mark)
Attachment #323315 - Attachment is obsolete: true
Attachment #323683 - Flags: review+
(Assignee)

Updated

9 years ago
Attachment #323683 - Flags: superreview?(bienvenu)

Comment 10

9 years ago
Comment on attachment 323683 [details] [diff] [review]
Update to conservative patch, addressing comment

yes, I agree.
Attachment #323683 - Flags: superreview?(bienvenu) → superreview+
(Assignee)

Updated

9 years ago
Keywords: checkin-needed
(Reporter)

Comment 11

9 years ago
Checking in mailnews/base/src/nsMsgFolderDataSource.cpp;
/cvsroot/mozilla/mailnews/base/src/nsMsgFolderDataSource.cpp,v  <--  nsMsgFolderDataSource.cpp
new revision: 1.223; previous revision: 1.222
done
Checking in mailnews/base/src/nsMsgFolderDataSource.h;
/cvsroot/mozilla/mailnews/base/src/nsMsgFolderDataSource.h,v  <--  nsMsgFolderDataSource.h
new revision: 1.85; previous revision: 1.84
done
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.