Closed Bug 834757 Opened 11 years ago Closed 11 years ago

Remove [noscript] methods from nsIMimeConverter

Categories

(MailNews Core :: MIME, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 22.0

People

(Reporter: jcranmer, Assigned: jcranmer)

References

Details

Attachments

(2 files, 3 obsolete files)

As part of the process of moving to JS implementation of MIME, I need to remove [noscript] methods on interfaces so libmime can implement them.
I replaced the [noscript] converter methods on nsIMimeConverter with a pure-C++ API, since
a) I'm not planning on rewriting MIME assembly in pure JS in the near future
b) All of the consumers are already in C++
c) This actually makes the API a lot easier to use.

Also, mailnews slowly gets to learn what C++ namespaces are :-)
Attachment #706431 - Flags: superreview?(neil)
Attachment #706431 - Flags: review?(irving)
Comment on attachment 706431 [details] [diff] [review]
Part 1: Make a pure-C++ API for qp/base64 encoding

Review of attachment 706431 [details] [diff] [review]:
-----------------------------------------------------------------

Random nits:

::: mailnews/compose/src/nsMsgAttachmentHandler.h
@@ +54,5 @@
> +namespace mailnews {
> +class MimeEncoder;
> +}
> +}
> +using mozilla::mailnews::MimeEncoder;

No using statements in the global scope, please. Looks like you can just put it in the class.

::: mailnews/compose/src/nsMsgSend.h
@@ +160,5 @@
> +namespace mailnews {
> +class MimeEncoder;
> +}
> +}
> +using mozilla::mailnews::MimeEncoder;

Same here

::: mailnews/compose/src/nsMsgSendPart.h
@@ +10,5 @@
>  #include "prprf.h" /* should be defined into msgCore.h? */
>  #include "nsMsgSend.h"
>  
> +#include "mozilla/mailnews/MimeEncoder.h"
> +using mozilla::mailnews::MimeEncoder;

And here

@@ +77,5 @@
>    char                *m_type;
>    char                *m_other;
>    char                m_charset_name[64+1];        // charset name associated with this part
>  	bool                m_strip_sensitive_headers;
> +	MimeEncoder         *m_encoder;

Oh.. Tabs.

::: mailnews/extensions/smime/src/nsMsgComposeSecure.h
@@ +24,5 @@
> +namespace mailnews {
> +class MimeEncoder;
> +}
> +}
> +using mozilla::mailnews::MimeEncoder;

Same here

::: mailnews/mime/public/MimeEncoder.h
@@ +20,5 @@
> +  virtual nsresult Write(const char *buffer, int32_t size) = 0;
> +  virtual nsresult Destroy(bool abort) { return NS_OK; }
> +
> +  static MimeEncoder *GetBase64Encoder(OutputCallback callback, void *closure);
> +  static MimeEncoder *GetQPEncoder(OutputCallback callback, void *closure);

For whatever that is worth, Gecko drops the "Get" from functions that can't return null. (Might be worth document the non-null-ness anyway.)
(In reply to :Ms2ger from comment #2)
> Comment on attachment 706431 [details] [diff] [review]
> Part 1: Make a pure-C++ API for qp/base64 encoding
> 
> Review of attachment 706431 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Random nits:
> 
> ::: mailnews/compose/src/nsMsgAttachmentHandler.h
> @@ +54,5 @@
> > +namespace mailnews {
> > +class MimeEncoder;
> > +}
> > +}
> > +using mozilla::mailnews::MimeEncoder;
> 
> No using statements in the global scope, please. Looks like you can just put
> it in the class.

Only if I want the following error message:
error: using-declaration for non-member at class-scope
Comment on attachment 706431 [details] [diff] [review]
Part 1: Make a pure-C++ API for qp/base64 encoding

Just some style stuff:

>-  MimeEncoderData       *m_encoder_data;  /* Opaque state for base64/qp encoder. */
>+  MimeEncoder           *m_encoder;
The various MimeEncoder* pointers get deleted a lot, and sometimes even set to null afterwards outside of destructors. It looks as if it would be superior to use nsAutoPtr<MimeEncoder> throughout instead.

>+    m_attachment1_encoder->Destroy(true);
This doesn't really explain what happens.
I suggest making the (virtual) destructor do the equivalent of Destroy(true).
Then rename Destroy(false) to Close() or Flush() or some such.

>+#include "mozilla/mailnews/MimeEncoder.h"
>+using mozilla::mailnews::MimeEncoder;
When do you #include and when do you forward declare?
Attachment #706431 - Flags: superreview?(neil)
Attachment #706431 - Flags: review?(irving)
(In reply to Joshua Cranmer [:jcranmer] from comment #3)
> (In reply to :Ms2ger from comment #2)
> > Comment on attachment 706431 [details] [diff] [review]
> > Part 1: Make a pure-C++ API for qp/base64 encoding
> > 
> > Review of attachment 706431 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Random nits:
> > 
> > ::: mailnews/compose/src/nsMsgAttachmentHandler.h
> > @@ +54,5 @@
> > > +namespace mailnews {
> > > +class MimeEncoder;
> > > +}
> > > +}
> > > +using mozilla::mailnews::MimeEncoder;
> > 
> > No using statements in the global scope, please. Looks like you can just put
> > it in the class.
> 
> Only if I want the following error message:
> error: using-declaration for non-member at class-scope

Sorry; I meant typedef mozilla::mailnews::MimeEncoder MimeEncoder; in the class. Would be nice if using worked, but hey, C++...
(In reply to neil@parkwaycc.co.uk from comment #4)
> Comment on attachment 706431 [details] [diff] [review]
> Part 1: Make a pure-C++ API for qp/base64 encoding
> 
> Just some style stuff:
> 
> >-  MimeEncoderData       *m_encoder_data;  /* Opaque state for base64/qp encoder. */
> >+  MimeEncoder           *m_encoder;
> The various MimeEncoder* pointers get deleted a lot, and sometimes even set
> to null afterwards outside of destructors. It looks as if it would be
> superior to use nsAutoPtr<MimeEncoder> throughout instead.

I keep forgetting about nsAutoPtr. Thank you for reminding me.

> >+    m_attachment1_encoder->Destroy(true);
> This doesn't really explain what happens.
> I suggest making the (virtual) destructor do the equivalent of Destroy(true).
> Then rename Destroy(false) to Close() or Flush() or some such.

I was originally going for minimum disruption of API (although introducing AUTF8String to the other methods has a rather impressive follow-on effect of CString-ification).

> >+#include "mozilla/mailnews/MimeEncoder.h"
> >+using mozilla::mailnews::MimeEncoder;
> When do you #include and when do you forward declare?

I meant to forward-declare in all headers, although I may have missed one place.
One ancillary change I want to make while doing this is to remove nsIMimeConverter::encodeMimePartIIStr. It is completely pointless, since the UTF8 version is more useful considering how the string is generated, it's much easier to use the UTF8 version, we have a single caller in our codebase, and only one up-to-date addon actually uses it (when they should use the _UTF8 variant instead). And it's much more annoying to implement in JS than in C++.
(In reply to Joshua Cranmer [:jcranmer] from comment #7)
> One ancillary change I want to make while doing this is to remove
> nsIMimeConverter::encodeMimePartIIStr.

I'm a fan of reducing the API surface and standardizing on UTF8 where possible, so as long as we warn the addon author about the upcoming change I like this.
Attachment #706431 - Attachment is obsolete: true
Attachment #712308 - Flags: superreview?(neil)
Attachment #712308 - Flags: review?(irving)
This is the rest of the changes needed to make nsIMimeConverter implementable by JS.

I could have made more string parameters into ACString/AUTF8String, but I think this amount of changes is suitable for the time being.
Attachment #712309 - Flags: superreview?(neil)
Attachment #712309 - Flags: review?(irving)
When I originally filed the bug, I suspected that I would be fixing both nsIMsgHeaderParser and nsIMimeConverter as interfaces here. It turns out, after looking in more detail, that nsIMsgHeaderParser is a far more insane interface than I had thought and that it needs something more powerful than some cosmetic changes applied to it.
Summary: Remove [noscript] methods from MIME interfaces → Remove [noscript] methods from nsIMimeConverter
Attachment #712308 - Flags: superreview?(neil) → superreview+
Comment on attachment 712309 [details] [diff] [review]
Part 2: Tweak the other methods on nsIMimeConverter

>-    boolean matchRfc2047String(in string aString, in string charset, in boolean charsetOverride);
>+    boolean matchRfc2047String(in AUTF8String aString, in string charset, in boolean charsetOverride);
Surely aString uses charset, not UTF8, so you need to ue ACString here?
(In reply to neil@parkwaycc.co.uk from comment #12)
> Comment on attachment 712309 [details] [diff] [review]
> Part 2: Tweak the other methods on nsIMimeConverter
> 
> >-    boolean matchRfc2047String(in string aString, in string charset, in boolean charsetOverride);
> >+    boolean matchRfc2047String(in AUTF8String aString, in string charset, in boolean charsetOverride);
> Surely aString uses charset, not UTF8, so you need to ue ACString here?

Yeah, I realized that after I posted the patch, so I changed it to ACString locally (and added a brief documentation comment).
Comment on attachment 712308 [details] [diff] [review]
Part 1: Make a pure-C++ API for qp/base64 encoding

Review of attachment 712308 [details] [diff] [review]:
-----------------------------------------------------------------

Please fix the patch comment before committing. r=irving with plausible answers to the questions below.

::: mailnews/compose/src/nsMsgAttachmentHandler.cpp
@@ -380,2 @@
>        mime_delivery_state);
> -    if (!m_encoder_data) return NS_ERROR_OUT_OF_MEMORY;

You've removed the error case when creating the encoder fails - I'm guessing because we'll get an exception if new fails? How cleanly will we fail in that case?

::: mailnews/compose/src/nsMsgSend.cpp
@@ -734,5 @@
> -        {
> -          status = NS_ERROR_OUT_OF_MEMORY;
> -          goto FAIL;
> -        }
> -        plainpart->SetEncoderData(plaintext_enc);

Again, error handling removed in a few places in this file?

::: mailnews/extensions/smime/src/nsMsgComposeSecure.cpp
@@ -557,4 @@
>                            this);
> -  if (!mCryptoEncoderData) {
> -    return NS_ERROR_OUT_OF_MEMORY;
> -  }

Again, removed error handling

::: mailnews/mime/src/mimeenc.cpp
@@ +966,5 @@
> +    else if (k < 62)  *out++ = k - 52 + '0';
> +    else if (k == 62) *out++ = '+';
> +    else if (k == 63) *out++ = '/';
> +    else MOZ_NOT_REACHED("6 bits should only be between 0 and 64");
> +  }

Is there a straightforward way to not duplicate the 3 bytes to 4 logic here?

@@ +993,2 @@
>    char out_buffer[80];
> +  RangedPtr<char> out(out_buffer);

Why do you use RangedPtr here, but not in the base 64 encoder?
Attachment #712308 - Flags: review?(irving) → review+
Comment on attachment 712309 [details] [diff] [review]
Part 2: Tweak the other methods on nsIMimeConverter

Review of attachment 712309 [details] [diff] [review]:
-----------------------------------------------------------------

This time I'd like to see answers and then do a quick re-review.

Please update the patch comment before commit.

I grow increasingly convinced that the existence of separate 8 and 16 bit string types was devised by Cthulhu to drive us inexorably toward madness.

::: mailnews/base/search/src/nsMsgSearchTerm.cpp
@@ +1066,5 @@
>  
>  // *pResult is false when strings don't match, true if they do.
> +nsresult nsMsgSearchTerm::MatchString(const nsACString &stringToMatch,
> +                                      const char *charset,
> +                                      bool *pResult)

Is there a string type we can choose here that doesn't leave us converting back and forth between UTF-8 and UTF-16 at the scripted interface? https://developer.mozilla.org/en-US/docs/XPIDL makes me think that IDL's DOMString (c++ nsAString) might be what we want.

I suppose that might leave us converting 8 -> 16 in more places in the C++ code, so this isn't a blocker.

::: mailnews/compose/src/nsMsgCompose.cpp
@@ +2340,5 @@
>              // if the input is not MIME encoded ASCII.
>              if (mMimeConverter)
> +              mMimeConverter->DecodeMimeHeaderToUTF8(authorName, charset,
> +                                                     charetOverride, true,
> +                                                     decodedAuthor);

The misspelling was pre-existing, but s/charetOverride/charsetOverride/

::: mailnews/compose/src/nsSmtpUrl.cpp
@@ +197,5 @@
> +        variable = decodedString; \
> +    } \
> +  } while (false)
> +
> +  nsAutoCString decodedString;

Declare this inside the do {} while block, or make CONVERT a function and let the compiler inline it for you if it wants.

::: mailnews/mime/src/comi18n.cpp
@@ +682,5 @@
>  
>  
> +void MIME_DecodeMimeHeader(const char *header, const char *default_charset,
> +                           bool override_charset, bool eatContinuations,
> +                           nsACString &result)

Is it OK to have "nsACstring &result" parameter in an extern "C" declaration?

::: mailnews/mime/src/mimedrft.cpp
@@ +398,5 @@
>    }
>  
>    if (references) {
> +    MIME_DecodeMimeHeader(references, charset, false, true, val);
> +    cFields->SetReferences(!val.IsEmpty() ? val.get() : references);

SetReferences, SetPriority, SetNewsPost aren't UTF16 but everything else is?

::: mailnews/mime/src/mimehdrs.cpp
@@ +30,5 @@
>  int32_t MimeHeaders_build_heads_list(MimeHeaders *hdrs);
>  
>  void
>  MimeHeaders_convert_header_value(MimeDisplayOptions *opt, nsCString &value,
>                                   bool convert_charset_only)

Existing code, but should the "value" parameter be nsACString?

::: mailnews/mime/src/nsMimeConverter.cpp
@@ +57,5 @@
>  {
>    NS_ENSURE_ARG_POINTER(header);
>  
>    // apply MIME decode.
> +  nsCString decodedCString;

nsAutoCString?
Attachment #712309 - Flags: review?(irving) → review-
(In reply to Irving Reid (:irving) from comment #15)
> I grow increasingly convinced that the existence of separate 8 and 16 bit
> string types was devised by Cthulhu to drive us inexorably toward madness.

The world would be much better off if everyone moved to UTF-8.

The root of the problem here (for header values, at least) is that we delay RFC 2047 decoding until as late as possible. Which means that the database stores MIME-encoded headers of unknown charset, so every single use requires decoding. At least in the narrow hole I've been hiding myself in recently, we use UTF16 pretty reliably post-2047 decoding, so it's just a matter of where do we pay the conversion cost.

> Is there a string type we can choose here that doesn't leave us converting
> back and forth between UTF-8 and UTF-16 at the scripted interface?
> https://developer.mozilla.org/en-US/docs/XPIDL makes me think that IDL's
> DOMString (c++ nsAString) might be what we want.

We want to use AString, not DOMString (that is designed specifically for compatibility with DOM specs) for UTF-16 strings.
(In reply to Irving Reid (:irving) from comment #14)
> ::: mailnews/compose/src/nsMsgAttachmentHandler.cpp
> @@ -380,2 @@
> >        mime_delivery_state);
> > -    if (!m_encoder_data) return NS_ERROR_OUT_OF_MEMORY;
> 
> You've removed the error case when creating the encoder fails - I'm guessing
> because we'll get an exception if new fails? How cleanly will we fail in
> that case?

::operator new (in mozilla) will abort if it can't allocate memory. That means that most of our attempts to handle out-of-memory are actually dead code.

> ::: mailnews/mime/src/mimeenc.cpp
> @@ +966,5 @@
> > +    else if (k < 62)  *out++ = k - 52 + '0';
> > +    else if (k == 62) *out++ = '+';
> > +    else if (k == 63) *out++ = '/';
> > +    else MOZ_NOT_REACHED("6 bits should only be between 0 and 64");
> > +  }
> 
> Is there a straightforward way to not duplicate the 3 bytes to 4 logic here?

I originally started looking into reusing one of the half-dozen base64 encoding functions we have here, but that makes logic more difficult and we might see measurable slow-downs due to excessive copying.

Actually, I can probably pull out the logic of that inner for-loop into a static method (Base64EncodeBits).

> @@ +993,2 @@
> >    char out_buffer[80];
> > +  RangedPtr<char> out(out_buffer);
> 
> Why do you use RangedPtr here, but not in the base 64 encoder?

QP's encoder was scarier to me when I first read it (specifically, the out-- it does in one place), so it was less clear that it was safe. I can add it to the b64 encoder, though.
(In reply to Irving Reid (:irving) from comment #15)
> ::: mailnews/mime/src/comi18n.cpp
> @@ +682,5 @@
> >  
> >  
> > +void MIME_DecodeMimeHeader(const char *header, const char *default_charset,
> > +                           bool override_charset, bool eatContinuations,
> > +                           nsACString &result)
> 
> Is it OK to have "nsACstring &result" parameter in an extern "C" declaration?

extern "C" means "don't mangle the function name" (which makes it easier to link against).

> ::: mailnews/mime/src/mimedrft.cpp
> @@ +398,5 @@
> >    }
> >  
> >    if (references) {
> > +    MIME_DecodeMimeHeader(references, charset, false, true, val);
> > +    cFields->SetReferences(!val.IsEmpty() ? val.get() : references);
> 
> SetReferences, SetPriority, SetNewsPost aren't UTF16 but everything else is?

nsIMsgCompFields is a bit of an annoying interface. Oh, and we convert everything (even the AString ones) back to UTF-8 in nsMsgCompFields to store them. Lovely, ain't it?

> ::: mailnews/mime/src/mimehdrs.cpp
> @@ +30,5 @@
> >  int32_t MimeHeaders_build_heads_list(MimeHeaders *hdrs);
> >  
> >  void
> >  MimeHeaders_convert_header_value(MimeDisplayOptions *opt, nsCString &value,
> >                                   bool convert_charset_only)
> 
> Existing code, but should the "value" parameter be nsACString?

Probably.
Comment on attachment 712309 [details] [diff] [review]
Part 2: Tweak the other methods on nsIMimeConverter

>+nsresult nsMsgSearchTerm::MatchString(const nsACString &stringToMatch,
>+                                      const char *charset,
>+                                      bool *pResult)
Seems that only one caller passes a charset, any plans to move the charset code out of MatchString?

>+      ConvertToUnicode(charset, nsCString(stringToMatch), utf16StrToMatch);
Nit: nsDependentCString

>   if (!escapedToPart.IsEmpty())
>   {
>     MsgUnescapeString(escapedToPart, 0, m_toPart);
>-    if (mimeConverter)
>-    {
>-      if (NS_SUCCEEDED(mimeConverter->DecodeMimeHeaderToCharPtr(
>-                           m_toPart.get(), "UTF-8", false, true,
>-                           &decodedString)) && decodedString)
>-        m_toPart.Adopt(decodedString);
>-    }
>+    CONVERT(m_toPart);
>   }
Why not CONVERT(escapedToPart, m_toPart); etc.?

> extern "C" {
[Does anyone still need this?]
Attachment #712309 - Flags: superreview?(neil) → superreview+
(In reply to neil@parkwaycc.co.uk from comment #19)
> Comment on attachment 712309 [details] [diff] [review]
> Part 2: Tweak the other methods on nsIMimeConverter
> 
> >+nsresult nsMsgSearchTerm::MatchString(const nsACString &stringToMatch,
> >+                                      const char *charset,
> >+                                      bool *pResult)
> Seems that only one caller passes a charset, any plans to move the charset
> code out of MatchString?

Looking more closely, it looks like this really ought to just be nsAString and make the callers do charset conversion. Except our wonderful mork database decides that GetProperty (the AString variant) should do an ASCII-to-UTF-16 conversion instead of message-dependent charset decoding as happens for the mime2Decoded* stuff.

> 
> > extern "C" {
> [Does anyone still need this?]

I don't think it's necessary to be extern "C" any more, but this patch is already starting to get long with follow-on changes.

(In reply to Irving Reid (:irving) from comment #15)
> ::: mailnews/mime/src/nsMimeConverter.cpp
> @@ +57,5 @@
> >  {
> >    NS_ENSURE_ARG_POINTER(header);
> >  
> >    // apply MIME decode.
> > +  nsCString decodedCString;
> 
> nsAutoCString?

Apparently, this causes the ternary operator to blow up in my face if I do this.
Rebased version of the first part
Attachment #712308 - Attachment is obsolete: true
Attachment #723105 - Flags: superreview+
Attachment #723105 - Flags: review+
Rebased part 2.
Attachment #712309 - Attachment is obsolete: true
Attachment #723106 - Flags: superreview+
Attachment #723106 - Flags: review?(irving)
Comment on attachment 723106 [details] [diff] [review]
Part 2: Tweak the other methods on nsIMimeConverter

Review of attachment 723106 [details] [diff] [review]:
-----------------------------------------------------------------

>> Is it OK to have "nsACstring &result" parameter in an extern "C" declaration?

> extern "C" means "don't mangle the function name" (which makes it easier to link against).

In some cases it can also mean "Stack your parameters using the C calling convention"; I wasn't sure whether that would affect the reference (probably not, since either way it's a pointer on the stack, just less likely to be null)

r+ if you get rid of the static converter.

::: mailnews/compose/src/nsSmtpUrl.cpp
@@ +205,5 @@
>    } // if rest && *rest
>  
> +  // Get a global converter
> +  if (!sMimeConverter)
> +    CallGetService(NS_MIME_CONVERTER_CONTRACTID, &sMimeConverter);

You're getting this service, using it in a bunch of function calls, then releasing it all within a single method invocation - so store it in a local autoptr and pass it as a parameter to UnescapeAndConvert rather than using a static
Attachment #723106 - Flags: review?(irving) → review+
Pushed:
https://hg.mozilla.org/comm-central/rev/5fb5abbe3b3d
https://hg.mozilla.org/comm-central/rev/93e89494abfd
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 22.0
Depends on: 857678
You need to log in before you can comment on or make changes to this bug.