Last Comment Bug 647699 - Make ::addMessage return an nsIMsgDBHdr on successful add.
: Make ::addMessage return an nsIMsgDBHdr on successful add.
Status: RESOLVED FIXED
:
Product: MailNews Core
Classification: Components
Component: Database (show other bugs)
: unspecified
: All All
: -- normal (vote)
: Thunderbird 13.0
Assigned To: Magnus Melin
:
:
Mentors:
Depends on:
Blocks: 522645
  Show dependency treegraph
 
Reported: 2011-04-04 09:46 PDT by alta88
Modified: 2012-03-14 23:15 PDT (History)
3 users (show)
mkmelin+mozilla: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
proposed fix (17.98 KB, patch)
2012-02-06 11:03 PST, Magnus Melin
no flags Details | Diff | Splinter Review
fix retention test failing (19.02 KB, patch)
2012-02-14 16:07 PST, David :Bienvenu
mozilla: review+
neil: superreview+
Details | Diff | Splinter Review
proposed fix v2 (for checkin) (11.34 KB, patch)
2012-02-20 11:46 PST, Magnus Melin
no flags Details | Diff | Splinter Review

Comment 1 Magnus Melin 2012-02-06 11:03:12 PST
Created attachment 594754 [details] [diff] [review]
proposed fix
Comment 2 David :Bienvenu 2012-02-14 14:35:32 PST
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.
Comment 3 David :Bienvenu 2012-02-14 15:02:40 PST
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 David :Bienvenu 2012-02-14 16:07:23 PST
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 :-))
Comment 5 neil@parkwaycc.co.uk 2012-02-14 16:31:40 PST
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?
Comment 6 Magnus Melin 2012-02-20 11:44:16 PST
(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
Comment 7 Magnus Melin 2012-02-20 11:46:02 PST
Created attachment 598931 [details] [diff] [review]
proposed fix v2 (for checkin)
Comment 8 Magnus Melin 2012-02-20 11:48:35 PST
http://hg.mozilla.org/comm-central/rev/77c812a9a820

->FIXED
Comment 9 Jens Hatlak (:InvisibleSmiley) 2012-03-14 16:06:36 PDT
(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? :-/
Comment 10 Magnus Melin 2012-03-14 23:15:12 PDT
Fixed, thx!

Note You need to log in before you can comment on or make changes to this bug.