Closed Bug 436792 Opened 16 years ago Closed 16 years ago

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

Categories

(MailNews Core :: Backend, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla1.9

People

(Reporter: standard8, Assigned: rain1)

References

Details

(Keywords: regression)

Attachments

(2 files, 1 obsolete file)

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?
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.
Should fix this for a2.
Flags: blocking-thunderbird3.0a2? → blocking-thunderbird3.0a2+
They almost certainly aren't used any more.
Assignee: nobody → sid1337
Status: NEW → ASSIGNED
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
Attachment #323308 - Flags: review?(bugzilla)
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...
Attached patch Conservative patch (obsolete) — Splinter Review
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)
Attachment #323308 - Attachment is obsolete: false
Attachment #323308 - Flags: review?(bugzilla)
Blocks: 430614
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)
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+
(carrying forward r=Mark)
Attachment #323315 - Attachment is obsolete: true
Attachment #323683 - Flags: review+
Attachment #323683 - Flags: superreview?(bienvenu)
Comment on attachment 323683 [details] [diff] [review]
Update to conservative patch, addressing comment

yes, I agree.
Attachment #323683 - Flags: superreview?(bienvenu) → superreview+
Keywords: checkin-needed
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
Closed: 16 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.

Attachment

General

Created:
Updated:
Size: