The default bug view has changed. See this FAQ.

mimeservice is supposed to be treated as a service

RESOLVED FIXED in Thunderbird 14.0

Status

MailNews Core
MIME
RESOLVED FIXED
14 years ago
4 years ago

People

(Reporter: timeless, Assigned: aceman)

Tracking

Trunk
Thunderbird 14.0

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

10.78 KB, patch
aceman
: review+
Details | Diff | Splinter Review
(Reporter)

Description

14 years ago
 
(Reporter)

Comment 1

14 years ago
Created attachment 126638 [details] [diff] [review]
comptr/do_GetService, use nsresult...
(Reporter)

Updated

14 years ago
Attachment #126638 - Flags: superreview?(bzbarsky)
Attachment #126638 - Flags: review?(neil.parkwaycc.co.uk)
(Reporter)

Comment 2

14 years ago
Comment on attachment 126638 [details] [diff] [review]
comptr/do_GetService, use nsresult...

For some unknown reason this patch triggers:
###!!! ASSERTION: This function was called on two disconnected nodes!: 'Error',
file
mozilla/content/base/src/nsContentUtils.cpp, line 1110
Break: at file mozilla/content/base/src/nsContentUtils.cpp, line 1110

I need to figure out why :(
Attachment #126638 - Attachment description: comptr/do_GetService, correctly use nsresult... → failed attempt #1 @ comptr/do_GetService, use nsresult...
Attachment #126638 - Flags: superreview?(bzbarsky)
Attachment #126638 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #126638 - Flags: review-

Comment 3

14 years ago
Comment on attachment 126638 [details] [diff] [review]
comptr/do_GetService, use nsresult...

I made a silly mistake and gave timeless bad information (the assertion occurs
regardless of his patch). Undoing his changes per his request.
Attachment #126638 - Attachment description: failed attempt #1 @ comptr/do_GetService, use nsresult... → comptr/do_GetService, use nsresult...
Attachment #126638 - Flags: superreview?(bzbarsky)
Attachment #126638 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #126638 - Flags: review-
Comment on attachment 126638 [details] [diff] [review]
comptr/do_GetService, use nsresult...

This needs review from someone who actually knows this code.. in particular,
nsMimeConverter is NOT the MIME service; I don't know whether it's supposed to
be a service at all... and neither does Neil, I'm sure.

Please look up cvsblame for this code and ask for appropriate review.

Comment 5

14 years ago
Comment on attachment 126638 [details] [diff] [review]
comptr/do_GetService, use nsresult...

>+  nsCOMPtr<nsIMimeConverter> converter = do_GetService(NS_MIME_CONVERTER_CONTRACTID);
>+  if (!converter)
>+    return nsnull;
Except nsnull isn't an nsresult.
> 
>-  return NS_SUCCEEDED(res) ? 0 : -1;
>+  nsresult res = converter->EncoderDestroy(data, abort_p);
>+  return res;
Just return converter->EncoderDestroy(data, abort_p);
You aren't consistent, sometimes you do get these right.
Attachment #126638 - Flags: review?(neil.parkwaycc.co.uk) → review-
(Reporter)

Comment 6

14 years ago
Created attachment 126792 [details] [diff] [review]
thanks neil
Attachment #126638 - Attachment is obsolete: true
(Reporter)

Updated

14 years ago
Attachment #126792 - Flags: superreview?(dmose)
Attachment #126792 - Flags: review?(neil.parkwaycc.co.uk)

Comment 7

14 years ago
Comment on attachment 126792 [details] [diff] [review]
thanks neil

Looks good to me.
Attachment #126792 - Flags: review?(neil.parkwaycc.co.uk) → review+

Comment 8

14 years ago
Comment on attachment 126792 [details] [diff] [review]
thanks neil

Spoke too soon :-(
No rv in MIME_EncoderWrite
Attachment #126792 - Flags: review+ → review-
Attachment #126638 - Flags: superreview?(bzbarsky)
Product: MailNews → Core

Comment 9

12 years ago
Comment on attachment 126792 [details] [diff] [review]
thanks neil

Need new patch re Neil's comment before sr-ing.
Attachment #126792 - Flags: superreview?(dmose)
QA Contact: stephend → mime
Product: Core → MailNews Core
(Assignee)

Comment 10

5 years ago
Is this still relevant? Is timeless working on it?
(In reply to :aceman from comment #10)
> Is this still relevant? Is timeless working on it?

No he ain't.
(Assignee)

Comment 12

5 years ago
It seems that today the line(in several functions):
nsIMimeConverter *converter;
nsresult res = nsComponentManager::CreateInstance(NS_MIME_CONVERTER_CONTRACTID, nsnull, 
                                   NS_GET_IID(nsIMimeConverter), (void **)&converter);

is converted to:

nsIMimeConverter *converter;
nsresult res = CallCreateInstance(NS_MIME_CONVERTER_CONTRACTID, &converter);

The patch wants to convert the original line to this:
nsCOMPtr<nsIMimeConverter> converter = do_GetService(NS_MIME_CONVERTER_CONTRACTID);

Neil, should this still be done today?
Assignee: timeless → nobody
(In reply to aceman from comment #12)
> nsIMimeConverter *converter;
> nsresult res =
> nsComponentManager::CreateInstance(NS_MIME_CONVERTER_CONTRACTID, nsnull, 
>                                    NS_GET_IID(nsIMimeConverter), (void **)&converter);
I don't actually recognise this way of creating instances, but I'm so used to using CallCreateInstance etc. that I don't actually know what predated them.

CallGetService is the service version of CallCreateInstance, thus
> nsresult res = CallCreateInstance(NS_MIME_CONVERTER_CONTRACTID, &converter);
becomes
> nsresult res = CallGetService(NS_MIME_CONVERTER_CONTRACTID, &converter);

do_CreateInstance is the nsCOMPtr version of CallCreateInstance, thus
> nsIMimeConverter *converter;
> nsresult res = CallCreateInstance(NS_MIME_CONVERTER_CONTRACTID, &converter);
becomes
> nsresult res;
> nsCOMPtr<nsIMimeConverter> converter = do_CreateInstance(NS_MIME_CONVERTER_CONTRACTID, &res);

do_GetService is just making both changes at once, i.e.
> nsresult res;
> nsCOMPtr<nsIMimeConverter> converter = do_GetService(NS_MIME_CONVERTER_CONTRACTID, &res);

Note that the &res is optional, the equivalent is calling CallCreateInstance without saving the result code.
(Assignee)

Comment 14

5 years ago
Thanks, according to that answer it looks like the change the patch wants to do is wanted.

I'll try to refresh it.
Assignee: nobody → acelists
(Assignee)

Comment 15

5 years ago
Created attachment 598939 [details] [diff] [review]
updated patch

Updated patch. I skipped this change, because I think it changes the logic (status will not be set):

@@ -758,9 +760,9 @@ nsMsgSendPart::Write()
   
   if (m_encoder_data) 
   {
-    status = MIME_EncoderDestroy(m_encoder_data, PR_FALSE);
+    nsresult rv = MIME_EncoderDestroy(m_encoder_data, PR_FALSE);
     m_encoder_data = nsnull;
-    if (status < 0)
+    if (NS_FAILED(rv))
       goto FAIL;
   }

And the parts in MimeEmitters seem to be already applied.
Attachment #126792 - Attachment is obsolete: true
Attachment #598939 - Flags: review?(neil)
(Assignee)

Updated

5 years ago
Status: NEW → ASSIGNED
Hardware: x86 → All
Comment on attachment 598939 [details] [diff] [review]
updated patch

Sorry for taking so long to get back to you on this.

> #include "nsComponentManagerUtils.h"
> 
> extern "C" MimeEncoderData *
>+MIME_B64EncoderInit(nsresult (*output_fn) (const char *buf, PRInt32 size, void *closure), void *closure)
> {
>   MimeEncoderData *returnEncoderData = nsnull;
>+  nsCOMPtr<nsIMimeConverter> converter = do_GetService(NS_MIME_CONVERTER_CONTRACTID);
Unfortunately do_GetService is in nsServiceManagerUtils.h, which means that the build will break under certain configurations. (nsComponentManagerUtils.h is however no longer necessary with these changes.)

>+    nsresult rv = MIME_EncoderWrite(m_encoder_data, encoded_data, length);
>+    if (NS_FAILED(rv))
>+      status = -1;
[See below.]

>-    if (!buffer) return NS_ERROR_OUT_OF_MEMORY;
>+    NS_ENSURE_TRUE(buffer, NS_ERROR_OUT_OF_MEMORY);
[Bah, this method returns an int, not an nsresult...]

>     status = MIME_EncoderDestroy(m_encoder_data, false);
>     m_encoder_data = nsnull;
>     needToWriteCRLFAfterEncodedBody = !m_parent;
>     if (status < 0)
>       goto FAIL;
It would be nice to have some way of treating MIME_EncoderDestroy as an nsresult, so as to be consistent with SMIME which does it correctly. I notice you worked around it for MIME_EncoderWrite above, maybe you could copy that?
Attachment #598939 - Flags: review?(neil) → review-
(Assignee)

Comment 17

5 years ago
Created attachment 606862 [details] [diff] [review]
patch v3

There are more spots in nsMsgSendPart.cpp where a function is declared int but sometimes returns some status and sometimes NS_ERROR_*. Can I do something about that in a new bug? Convert it all to nsresult? Would it be hard?
Attachment #598939 - Attachment is obsolete: true
Attachment #606862 - Flags: review?(neil)
(In reply to aceman from comment #17)
> There are more spots in nsMsgSendPart.cpp where a function is declared int
> but sometimes returns some status and sometimes NS_ERROR_*. Can I do something
> about that in a new bug? Convert it all to nsresult? Would it be hard?
I imagine a new bug to convert to nsresult would be the best option, but I don't really know how hard it would be, maybe bienvenu could weigh in?
Comment on attachment 606862 [details] [diff] [review]
patch v3

>-#include "nsComponentManagerUtils.h"
Looks good but I meant for you to replace this with nsServiceManagerUtils.h
r=me with that fixed.
Attachment #606862 - Flags: review?(neil) → review+
(Assignee)

Comment 20

5 years ago
And why when it still compiles?
(In reply to comment #16)
> the build will break under certain configurations.
                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^

(In reply to aceman from comment #20)
> And why when it still compiles?
The old code would have compiled in the configuration you were using without that line too, but that was no reason to remove it.
(Assignee)

Comment 22

5 years ago
Created attachment 606877 [details] [diff] [review]
patch v4

Ok.
Attachment #606862 - Attachment is obsolete: true
Attachment #606877 - Flags: review+

Comment 23

5 years ago
(In reply to neil@parkwaycc.co.uk from comment #18)
> (In reply to aceman from comment #17)
> > There are more spots in nsMsgSendPart.cpp where a function is declared int
> > but sometimes returns some status and sometimes NS_ERROR_*. Can I do something
> > about that in a new bug? Convert it all to nsresult? Would it be hard?
> I imagine a new bug to convert to nsresult would be the best option, but I
> don't really know how hard it would be, maybe bienvenu could weigh in?

Probably fairly easy.
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
(Assignee)

Updated

5 years ago
Blocks: 736766
http://hg.mozilla.org/comm-central/rev/cd3cd74613f8
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite?
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 14.0
Flags: in-testsuite?
You need to log in before you can comment on or make changes to this bug.