Make ::addMessage return an nsIMsgDBHdr on successful add.

RESOLVED FIXED in Thunderbird 13.0

Status

MailNews Core
Database
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: alta88, Assigned: Magnus Melin)

Tracking

unspecified
Thunderbird 13.0
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

6 years ago
from https://bugzilla.mozilla.org/show_bug.cgi?id=522645#c22
(Reporter)

Updated

6 years ago
Blocks: 522645
(Assignee)

Comment 1

5 years ago
Created attachment 594754 [details] [diff] [review]
proposed fix
Assignee: nobody → mkmelin+mozilla
Status: NEW → ASSIGNED
Attachment #594754 - Flags: superreview?(dbienvenu)
Attachment #594754 - Flags: review?(dbienvenu)

Comment 2

5 years ago
Comment on attachment 594754 [details] [diff] [review]
proposed fix

Thx for the patch! A single reviewer can't do both r and sr anymore, so I'll ask Neil for sr on the interface change. The patch looks good to me; just running through the tests now.
Attachment #594754 - Flags: superreview?(dbienvenu) → superreview?(neil)

Comment 3

5 years ago
Comment on attachment 594754 [details] [diff] [review]
proposed fix

this seems to break mailnews/base/test/unit/test_retention.js, perhaps because it causes a db to get held open. I'll try to see if I can fix that.

Comment 4

5 years ago
Created attachment 597231 [details] [diff] [review]
fix retention test failing

OK, I tweaked test_retention.js so that it should pass, by adding a forceGC. Magnus, please make sure that you include that change before landing (after Neil's sr, of course :-))
Attachment #594754 - Attachment is obsolete: true
Attachment #594754 - Flags: superreview?(neil)
Attachment #594754 - Flags: review?(dbienvenu)
Attachment #597231 - Flags: superreview?(neil)
Attachment #597231 - Flags: review+

Comment 5

5 years ago
Comment on attachment 597231 [details] [diff] [review]
fix retention test failing

>   const char *aMessages[] = {aMessage};
>-  return AddMessageBatch(1, aMessages);
>+  nsresult rv = AddMessageBatch(1, aMessages, getter_AddRefs(hdrs));
[Can you not use &aMessage instead?]

>+  nsCOMPtr<nsIMsgDBHdr> hdr(do_QueryElementAt(hdrs, 0, &rv));
[It's a pity there's no CallQueryElementAt(hdrs, 0, aHdr)]

>+  NS_IF_ADDREF(*aHdr = hdr);
hdr.forget(aHdr);

>-[test_localSubFolders.js]
>+[test_nsIMsgLocalMailFolder.js]
This needs to be an hg rename as well, no?
Attachment #597231 - Flags: superreview?(neil) → superreview+
(Assignee)

Comment 6

5 years ago
(In reply to neil@parkwaycc.co.uk from comment #5)
> Comment on attachment 597231 [details] [diff] [review]
> fix retention test failing
> 
> >   const char *aMessages[] = {aMessage};
> >-  return AddMessageBatch(1, aMessages);
> >+  nsresult rv = AddMessageBatch(1, aMessages, getter_AddRefs(hdrs));
> [Can you not use &aMessage instead?]

I didn't get this to work.

> >-[test_localSubFolders.js]
> >+[test_nsIMsgLocalMailFolder.js]
> This needs to be an hg rename as well, no?

Yes, my .hgrc was missing git=1
(Assignee)

Comment 7

5 years ago
Created attachment 598931 [details] [diff] [review]
proposed fix v2 (for checkin)
Attachment #597231 - Attachment is obsolete: true
(Assignee)

Comment 8

5 years ago
http://hg.mozilla.org/comm-central/rev/77c812a9a820

->FIXED
Flags: in-testsuite+
Target Milestone: --- → Thunderbird 13.0
(In reply to Magnus Melin from comment #8)
> http://hg.mozilla.org/comm-central/rev/77c812a9a820
> 
> ->FIXED

Hmm, but the Status field still says ASSIGNED, so what is it? :-/
(Assignee)

Comment 10

5 years ago
Fixed, thx!
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.