Closed Bug 559559 Opened 14 years ago Closed 14 years ago

Show the file size of attachments in attachment panel of *any* messages (received/sent/deleted etc.)

Categories

(Thunderbird :: Message Compose Window, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 3.3a1

People

(Reporter: dfghjkjhg, Assigned: squib)

References

(Blocks 3 open bugs)

Details

Attachments

(2 files, 11 obsolete files)

35.34 KB, image/png
clarkbw
: ui-review+
Details
36.35 KB, patch
squib
: review+
Details | Diff | Splinter Review
User-Agent:       
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.9.2.2pre) Gecko/20100302 Lightning/1.0b2pre Lanikai/3.1b1

Currently the file size of attachments is not shown.
Would be a big gain to see file sizes (in the compose window   as well as  in received/sent/deleted... mails).

Reproducible: Always
I'm pretty sure this is a dup, but I was unable to find it.
Whiteboard: dupme?
bug 195702
(but I fail to see how 195702 blocks bug 516787)
Status: UNCONFIRMED → RESOLVED
Closed: 14 years ago
Component: General → Message Compose Window
QA Contact: general → message-compose
Resolution: --- → DUPLICATE
Bug 195702 only requests attachment sizes in the compose window.
However would be nice to see attachment sizes in all mails (received/sent/deleted etc.) not only in the compose window.
But hopefully this will also be implemented when fixing bug 195702. Otherwise please re-open this bug here. Thanks.
Status: RESOLVED → UNCONFIRMED
Resolution: DUPLICATE → ---
I believe there are other bugs which cover this last request, which is why I duped on the first issue. please check the list of bugs that mention attachment sizes. http://bit.ly/aP7NIl
Severity: normal → enhancement
Just found out that I already voted for a similar bug 26517 once.
But that one is for SeaMonkey.
Don't know if that bug will also affect Thunderbird and whether it covers the whole requested here ?
-> This bug should be flagged "wanted-thunderbird+"

I don't see any bug in comment 4's list (http://bit.ly/aP7NIl) we could duplicate this against... There are some similar bugs, but they seem to have different UI proposals... This is a highly useful and frequently used basic feature that should be added asap. -> re-confirming
Status: UNCONFIRMED → NEW
Depends on: 195702
Ever confirmed: true
See Also: → 26517, 430288
Summary: Show the file size of attachments → Show the file size of attachments in attachment panel of *any* messages (received/sent/deleted etc.)
Whiteboard: dupme?
Version: unspecified → Trunk
(In reply to comment #5)
> similar bug 26517 for SeaMonkey. Don't know if that bug will also affect 
> Thunderbird and whether it covers the whole requested here?
SM is a different product, and it's rather unlikely that this will affect TB (unless it is fixed in the Shared Core).
If someone can figure out how to get the size of an attachment in code (I'm not going to use the "Attachment Sizes" addon[1] method of reading the contents of all the attachments to deduce the value), I'll do this. I need to figure out how to do this if I want to cover all the cases in bug 195702 anyway.

[1] https://addons.mozilla.org/en-US/thunderbird/addon/878/
Bug 436555 has also the potential to add the size of the attachment, for received messages.
Jim, what about bug 561851? I've done the required work to count the attachment sizes in libmime, so I could provide guidance on how to it should you be willing to do so. Roughly (not checked), you'd need to:
- remove the checks in libmime that currently disable attachment size counting unless talking to the JS MIME emitter,
- figure out where exactly in the C++ side is the information about attachments received from libmime (you want to look here http://mxr.mozilla.org/comm-central/source/mailnews/mime/emitters/src/nsMimeHtmlEmitter.cpp#471)
- because http://mxr.mozilla.org/comm-central/source/mail/base/content/msgHdrViewOverlay.js#556 is the place where notifications are received (from here http://mxr.mozilla.org/comm-central/source/mailnews/mime/emitters/src/nsMimeHtmlEmitter.cpp#386),
- and that's probably where you'd want to be notified of the size and display it in the UI.

I'm onto other things now, so I'm not interested in fixing this, but it sounds pretty straightforward :-).
(In reply to comment #10)
> - remove the checks in libmime that currently disable attachment size counting
> unless talking to the JS MIME emitter,

Where are these checks? I looked at the diffs in bug 561851, but didn't see anything like this jump out at me.

In more general news, I think this is probably a good starting point to fix bug 195702 in the general case, since I need to have the attachment size carried along when an attachment is dragged and dropped onto a compose window.
(In reply to comment #11)
> (In reply to comment #10)
> > - remove the checks in libmime that currently disable attachment size counting
> > unless talking to the JS MIME emitter,
> 
> Where are these checks? I looked at the diffs in bug 561851, but didn't see
> anything like this jump out at me.

Well, I found the check in mimei.cpp, but manually setting the emitter=js options for all cases doesn't seem to pick up the sizes for non-inline attachments. Is there something else I'm missing?
Attached patch First cut (obsolete) — Splinter Review
This seems to work, though my change to MimeDisplayOptions is almost certainly a bad idea. Any ideas on how to handle that better would be useful.
Assignee: nobody → squibblyflabbetydoo
Status: NEW → ASSIGNED
Hey, you're fast, plus you figured it all by yourself. Congrats!

Setting MimeDisplayOptions is actually the right thing to do. What I'm afraid is that it will trigger more calls to MimeObject_write, because we're actually running through attachments we didn't use to run through before (because we want to explicitely count the bytes). How does this play with a .zip attachment for instance?
Attachment #479910 - Flags: feedback+
Comment on attachment 479910 [details] [diff] [review]
First cut

>@@ -1801,7 +1809,14 @@
>     {
>       // Create a new attachment widget
>       var displayName = createAttachmentDisplayName(attachment);
>-      var attachmentView = attachmentList.appendItem(displayName);
>+      var nameAndSize = displayName;
>+      if (attachment.size != null) {
>+        var messenger = Components.classes["@mozilla.org/messenger;1"]
>+                                  .createInstance(Components.interfaces.nsIMessenger);
Don't you have that one already defined in scope somewhere as messenger or gMessenger? I'm afraid it might be expensive to create it everytime, you might want to at least define it in that file so that it's not recreated for each attachment.
>+
>+        nameAndSize += " ("+messenger.formatFileSize(attachment.size)+")";
You probably want to ask :clarkbw for his opinion on the best way to display the information (on another line perhaps?)
>diff --git a/mailnews/base/util/nsMsgUtils.cpp b/mailnews/base/util/nsMsgUtils.cpp
>--- a/mailnews/base/util/nsMsgUtils.cpp
>+++ b/mailnews/base/util/nsMsgUtils.cpp
>@@ -540,6 +540,9 @@
>                             sizeAbbr.get(),
>                             (unitIndex != 0) && (unitSize < 99.95) ? 1 : 0,
>                             unitSize);
>+
>+  // Get rid of the null-terminator
>+  formattedSize.Cut(formattedSize.Length()-1, 1);
I'm wary of such changes, this function is used by C++ code that probably depends on the null terminator, why such a change? You should definitely do that only on the JS side of things.
>@@ -460,7 +460,9 @@
> nsresult
> nsMimeHtmlDisplayEmitter::AddAttachmentField(const char *field, const char *value)
> {
>-  if (mSkipAttachment || BroadCastHeadersAndAttachments())
>+  nsresult rv = NS_OK;
I know it's done someplace else in this file, but I feel NS_OK is superfluous. 
>+
>+  if (mSkipAttachment)
>     return NS_OK;
> 
>   // Don't let bad things happen
>@@ -471,25 +473,34 @@
>   if (!strcmp(field, HEADER_X_MOZILLA_PART_URL))
>     return NS_OK;
> 
>-  char  *newValue = MsgEscapeHTML(value);
>+  nsCOMPtr<nsIMsgHeaderSink> headerSink;
>+  rv = GetHeaderSink(getter_AddRefs(headerSink));
> 
>-  UtilityWrite("<tr>");
>+  if (NS_SUCCEEDED(rv) && headerSink) {
You don't need to test for both.
>+    headerSink->AddAttachmentField(field, value);
>+  }
>+  else {
>+    char  *newValue = MsgEscapeHTML(value);
> 
>-  UtilityWrite("<td>");
>-  UtilityWrite("<div align=right class=\"headerdisplayname\" style=\"display:inline;\">");
>+    UtilityWrite("<tr>");
> 
>-  UtilityWrite(field);
>-  UtilityWrite(":");
>-  UtilityWrite("</div>");
>-  UtilityWrite("</td>");
>-  UtilityWrite("<td>");
>+    UtilityWrite("<td>");
>+    UtilityWrite("<div align=right class=\"headerdisplayname\" style=\"display:inline;\">");
> 
>-  UtilityWrite(newValue);
>+    UtilityWrite(field);
>+    UtilityWrite(":");
>+    UtilityWrite("</div>");
>+    UtilityWrite("</td>");
>+    UtilityWrite("<td>");
> 
>-  UtilityWrite("</td>");
>-  UtilityWrite("</tr>");
>+    UtilityWrite(newValue);
> 
>-  PR_Free(newValue);
>+    UtilityWrite("</td>");
>+    UtilityWrite("</tr>");
>+
>+    PR_Free(newValue);
>+  }
>+
>   return NS_OK;
> }
> 
Pretty cool, if we ever are to add more attachment fields (although I doubt it).
>diff --git a/mailnews/mime/public/nsIMimeMiscStatus.idl b/mailnews/mime/public/nsIMimeMiscStatus.idl
>--- a/mailnews/mime/public/nsIMimeMiscStatus.idl
>+++ b/mailnews/mime/public/nsIMimeMiscStatus.idl
>@@ -61,7 +61,7 @@
> // this is a simple interface which allows someone to listen to all the headers 
> // that are discovered by mime. We can use this when displaying a message to update
> // the msg header in JS. 
>-[scriptable, uuid(8AA47266-1226-490B-9467-5E865CC58074)]
>+[scriptable, uuid(AD0CB800-CCD0-11DF-BD3B-0800200C9A66)]
> interface nsIMsgHeaderSink : nsISupports 
> {
>   // You must finish consuming the iterators before returning from processHeaders. aHeaderNames and aHeaderValues will ALWAYS have the same
>@@ -70,6 +70,7 @@
> 
>   void handleAttachment(in string contentType, in string url, in wstring displayName, in string uri,
>                         in boolean aNotDownloaded);
>+  void addAttachmentField(in string field, in string value);
>   void onEndAllAttachments();
>     
>   // onEndMsgHeaders is called after libmime is done processing a message. At this point it is safe for
>diff --git a/mailnews/mime/src/mimemoz2.cpp b/mailnews/mime/src/mimemoz2.cpp
>--- a/mailnews/mime/src/mimemoz2.cpp
>+++ b/mailnews/mime/src/mimemoz2.cpp
>@@ -1461,7 +1461,7 @@
>   quote_attachment_inline_p = PR_FALSE;
>   notify_nested_bodies = PR_FALSE;
>   write_pure_bodies = PR_FALSE;
>-  stream_all_attachments = PR_FALSE;
>+  stream_all_attachments = PR_TRUE;
> }
> 
> MimeDisplayOptions::~MimeDisplayOptions()
Hem, I wonder if we should get rid of the flag totally, since it's always true with that patch...

All in all, that's a pretty good patch, you managed to plug all parts together, that's great! I suggest getting clarkbw in the loop so that he can tell us what he thinks, maybe bwinton as well (that impacts the message reader, so that's his part), then I can review your patch and forward sr to someone else (maybe bienvenu or asuth). Thanks for the patch!
Attached patch Address comments (obsolete) — Splinter Review
(In reply to comment #15)
> Comment on attachment 479910 [details] [diff] [review]
> First cut
> 
> >@@ -1801,7 +1809,14 @@
> >     {
> >       // Create a new attachment widget
> >       var displayName = createAttachmentDisplayName(attachment);
> >-      var attachmentView = attachmentList.appendItem(displayName);
> >+      var nameAndSize = displayName;
> >+      if (attachment.size != null) {
> >+        var messenger = Components.classes["@mozilla.org/messenger;1"]
> >+                                  .createInstance(Components.interfaces.nsIMessenger);
> Don't you have that one already defined in scope somewhere as messenger or
> gMessenger? I'm afraid it might be expensive to create it everytime, you might
> want to at least define it in that file so that it's not recreated for each
> attachment.

It doesn't seem to be defined yet, so I added it.

> >+
> >+        nameAndSize += " ("+messenger.formatFileSize(attachment.size)+")";
> You probably want to ask :clarkbw for his opinion on the best way to display
> the information (on another line perhaps?)

Yeah, I expect that there's a better way to display this, though that might be part of a larger change to the attachments panel as a whole.

> >diff --git a/mailnews/base/util/nsMsgUtils.cpp b/mailnews/base/util/nsMsgUtils.cpp
> >--- a/mailnews/base/util/nsMsgUtils.cpp
> >+++ b/mailnews/base/util/nsMsgUtils.cpp
> >@@ -540,6 +540,9 @@
> >                             sizeAbbr.get(),
> >                             (unitIndex != 0) && (unitSize < 99.95) ? 1 : 0,
> >                             unitSize);
> >+
> >+  // Get rid of the null-terminator
> >+  formattedSize.Cut(formattedSize.Length()-1, 1);
> I'm wary of such changes, this function is used by C++ code that probably
> depends on the null terminator, why such a change? You should definitely do
> that only on the JS side of things.

Maybe I'm getting lucky because that function is currently only used in the thread pane (for the size column), but my limited understanding of nsAString was that the null terminator wasn't necessary.

> >@@ -460,7 +460,9 @@
> > nsresult
> > nsMimeHtmlDisplayEmitter::AddAttachmentField(const char *field, const char *value)
> > {
> >-  if (mSkipAttachment || BroadCastHeadersAndAttachments())
> >+  nsresult rv = NS_OK;
> I know it's done someplace else in this file, but I feel NS_OK is superfluous. 
> >+
> >+  if (mSkipAttachment)
> >     return NS_OK;
> > 
> >   // Don't let bad things happen
> >@@ -471,25 +473,34 @@
> >   if (!strcmp(field, HEADER_X_MOZILLA_PART_URL))
> >     return NS_OK;
> > 
> >-  char  *newValue = MsgEscapeHTML(value);
> >+  nsCOMPtr<nsIMsgHeaderSink> headerSink;
> >+  rv = GetHeaderSink(getter_AddRefs(headerSink));
> > 
> >-  UtilityWrite("<tr>");
> >+  if (NS_SUCCEEDED(rv) && headerSink) {
> You don't need to test for both.

Fixed. I was just copying from elsewhere anyway. :)

> Hem, I wonder if we should get rid of the flag totally, since it's always true
> with that patch...

Probably. I guess the other option would be to pass in an emitter argument like gloda does, but I'm not sure that's actually useful.

I also fixed the cloneAttachment function (it copies the size now, too), which is necessary for bug 195702.
Attachment #479910 - Attachment is obsolete: true
Attached image What it looks like
Clarkbw, what's your opinion on how this looks? Definitely not the best possible UI, but maybe it's close enough until more serious UI work happens on the attachment pane...
Attachment #479946 - Flags: ui-review?(clarkbw)
CCing clarkbw because this patches touches UI stuff so it needs approval.
CCing bienvenu so that he can check you're correct on the nsAString thing (I'm not an expert for these things, I try to stay away from the C++ string API).
Depends on: 601096
So the extra null character is a bug, for which a patch is sitting in bug 323376. With that patch applied, and with your .Cut() removed (the one that gets rid of the extra \0), the patch seems to work fine, although:
1) it won't be accepted without at least a test,
2) I can't open attachments anymore with it (probably the code that opens attachments relies on the exact value of the field to figure out which part to download, so you'll have to come up with something else to display sizes, probably a separate field, I don't really know),
3) attachments that haven't been downloaded yet show a size that's 27 bytes, which is quite inconsistent. 

For the last point, I'm not sure there's a way to properly detect this...
4) you broke inline image display
5) an email I have that contains three attached images now only displays one in the attachments pane
(In reply to comment #19)
> So the extra null character is a bug, for which a patch is sitting in bug
> 323376. With that patch applied, and with your .Cut() removed (the one that
> gets rid of the extra \0), the patch seems to work fine, although:
> 1) it won't be accepted without at least a test,

As you've noticed, the patch is still a bit of a mess, and I haven't even done a whole lot of manual testing yet. Hopefully once the obvious issues are fixed, I can get a decent test written up.

> 2) I can't open attachments anymore with it (probably the code that opens
> attachments relies on the exact value of the field to figure out which part to
> download, so you'll have to come up with something else to display sizes,
> probably a separate field, I don't really know),

It's possible that this (and some of the other issues you mentioned) are some libmime weirdness. The "stream_all_attachments" seems like it occasionally also means "I just want metadata". That said, the file's display name should be attached to the node in the DOM as a separate attribute, but who knows if the "open attachment" code actually uses that.

> 3) attachments that haven't been downloaded yet show a size that's 27 bytes,
> which is quite inconsistent. 
> 
> For the last point, I'm not sure there's a way to properly detect this...

I'm sure libmime could figure that out. I'm not sure what locale you use Thunderbird in; would 27 bytes happen to be the length of that "This body part will be downloaded on demand" message in your locale?

(In reply to comment #20)
> 4) you broke inline image display
> 5) an email I have that contains three attached images now only displays one in
> the attachments pane

Yeah, like I said for (2), I think a lot of this is related to libmime weirdness. I'll mess around with it, but I vaguely recall inline stuff behaving sanely when stream_all_attachments was false.
(In reply to comment #21)
> I'm sure libmime could figure that out. I'm not sure what locale you use
> Thunderbird in; would 27 bytes happen to be the length of that "This body part
> will be downloaded on demand" message in your locale?

Looking at it more, I see the 27 byte thing for attached images, but 44 bytes for inline text files (not coincidentally, the length of that "body part" message).
Ok, so numbers 2 and 4 above are definitely related to stream_all_attachments, since setting it back to false fixes them. Maybe what we really want to do is to turn "stream_all_attachments" into "only_metadata", since as I understand it, gloda doesn't care about the contents of the attachment, only its metadata (read: size).

Maybe there's even a way to use this with the nsMimeHtmlEmitter so that you only get metadata when making the attachment list, but you get all the "real" data back when displaying/opening it. I'm not sure where the relevant streamMessage (?) call is though.
Comment on attachment 479946 [details]
What it looks like

This looks good to me.  Nice work Jim!
Attachment #479946 - Flags: ui-review?(clarkbw) → ui-review+
Ok, I added a new flag for the js-mime emitter: "metadata_only". This *should* fix issues (2) and (4) above (comment 19 and comment 20). Protz, can you see if this fixes issue (5) for you?

No tests yet, but if everything seems to work on the surface, then I'll get started on that. :)
Attachment #479941 - Attachment is obsolete: true
Attachment #480695 - Flags: feedback?(jonathan.protzenko)
Whoops, the previous version dumps binary attachment data into the message. This should fix it, though I feel like I'm starting to go down a rabbit hole here...
Attachment #480695 - Attachment is obsolete: true
Attachment #480765 - Flags: feedback?(jonathan.protzenko)
Attachment #480695 - Flags: feedback?(jonathan.protzenko)
Comment on attachment 480765 [details] [diff] [review]
Keep binary attachments from being dumped into the message

Ah, I wrote down a long explanation just to find out that you actually did the right thing. So yes, you're going down a rabbit hole, it's called libmime, and it's bad :-). But the good thing is you're getting close.

What confused me is you didn't remove the stream_all_attachments option. You should definitely remove it, it's used only by the JS mime emitter, so no one will complain if it disappears suddenly.

>-  if (mSkipAttachment || BroadCastHeadersAndAttachments())
>+  if (mSkipAttachment)
>     return NS_OK;
> 
Why this change? I can't figure out why it's needed.

>-  if (obj->options && obj->options->stream_all_attachments)
>+  if (obj->options && (obj->options->metadata_only ||
>+                       obj->options->write_html_p))
Yes, that's the right test, and it's much more understandable now. So metadata_only is TRUE for the JS mime emitter, and FALSE for the HTML mime emitter, is that correct?

>-  if (obj->options && obj->options->stream_all_attachments)
>+  if (obj->options && obj->options->metadata_only)
>     return 0;
So this means, "unless we're the JS mime emitter, and we don't care about inline images, go ahead and stream the inline image". Is that correct?

>+
>+  /**
>+   * When true, only processes metadata (i.e. size) for streamed attachments.
>+   *  At the moment, only the JS mime emitter uses this. Other mime emitters
>+   *  would probably choke.
>+   */
>+  PRBool metadata_only;
> };
This is not true anymore, other emitters would probably just miss important stuff, like inline images. You need to update the comment.

Please, please don't hesitate to comment critical parts (the if (obj->options && blah blah blah) tests) in plain English. libmime is hard enough to understand, so we should definitely annotate these important tests to explain the intent.

My questions:
1) Can we open attachments now with this patch (by double-clicking)?
2) Have you checked that mailnews/db/gloda/test/unit/{test_mime_emitter.js,test_mime_attachments_size.js} still pass? The latter will probably fail if you did something wrong here.

All in all, a very good patch. Thanks for the effort!
Attachment #480765 - Flags: feedback?(jonathan.protzenko) → feedback+
Adding the libmime bug into the depends field so that patches are committed in the right order when branching 3.2. If I understood correctly, you need this bug to be fixed first before being able to show attachment sizes in the compose window, so changing the blocks field to reflect this.
Blocks: 195702
Depends on: 561851, 323376
No longer depends on: 195702
I have a patch that addresses comment 27, but I'm going to wait uploading it until I've written up tests, since I think things are pretty stable now. Unless someone has a better idea, I'll probably do them as mozmill tests, since a) the gloda xpcshell tests should be sufficient for unit testing and b) most of the changes are on the UI side. If someone thinks more tests are necessary, I can do them, but I'll err on the side of "less work" for now. :)

(In reply to comment #27)
> What confused me is you didn't remove the stream_all_attachments option. You
> should definitely remove it, it's used only by the JS mime emitter, so no one
> will complain if it disappears suddenly.

I just did that because I was still moving stuff around and didn't want to wake the libmime dragon any sooner than necessary. :)

> >-  if (mSkipAttachment || BroadCastHeadersAndAttachments())
> >+  if (mSkipAttachment)
> >     return NS_OK;
> > 
> Why this change? I can't figure out why it's needed.

BroadCastHeadersAndAttachments() is basically "is there a message sink?". I use the message sink to send the attachment metadata (size), so if I left it in, the function would return before it could report the size.

> >-  if (obj->options && obj->options->stream_all_attachments)
> >+  if (obj->options && (obj->options->metadata_only ||
> >+                       obj->options->write_html_p))
> Yes, that's the right test, and it's much more understandable now. So
> metadata_only is TRUE for the JS mime emitter, and FALSE for the HTML mime
> emitter, is that correct?

Correct.

> >-  if (obj->options && obj->options->stream_all_attachments)
> >+  if (obj->options && obj->options->metadata_only)
> >     return 0;
> So this means, "unless we're the JS mime emitter, and we don't care about
> inline images, go ahead and stream the inline image". Is that correct?
> 

Correct.

> >+  /**
> >+   * When true, only processes metadata (i.e. size) for streamed attachments.
> >+   *  At the moment, only the JS mime emitter uses this. Other mime emitters
> >+   *  would probably choke.
> >+   */
> >+  PRBool metadata_only;
> > };
> This is not true anymore, other emitters would probably just miss important
> stuff, like inline images. You need to update the comment.

Ok, I updated it.

> Please, please don't hesitate to comment critical parts (the if (obj->options
> && blah blah blah) tests) in plain English. libmime is hard enough to
> understand, so we should definitely annotate these important tests to explain
> the intent.

I've added some comments to those areas.

> My questions:
> 1) Can we open attachments now with this patch (by double-clicking)?

That works for all the cases I tried (inline text, inline image, binary).

> 2) Have you checked that
> mailnews/db/gloda/test/unit/{test_mime_emitter.js,test_mime_attachments_size.js}
> still pass? The latter will probably fail if you did something wrong here.

Both of these pass. Incidentally, do you know how to run just one xpcshell test (or just one directory)?

> All in all, a very good patch. Thanks for the effort!

And thanks for looking over my patches!
I have a cheat sheet for this!

If you want to run one test:

  protz@sleet-2:~/Code/objdir-comm-central/mailnews/db/gloda/test $
    make SOLO_FILE=test_mime_emitter.js check-one

Of course, if you're at the root of your objdir, you'll use this instead:

    make -C mailnews/db/gloda/test SOLO_FILE=... check-one

If you want to run all gloda tests (from the root of your objdir):

  make -C mailnews/db/gloda/test/ xpcshell-tests

So what was wrong with opening an attachment through a double-click? I didn't get exactly what was wrong with your previous iteration of the patch.

Concerning testing, Mozmill seems like the way to go for this, so I totally approve :-).
(In reply to comment #30)
> I have a cheat sheet for this!
[snip]

Great, thanks!

> So what was wrong with opening an attachment through a double-click? I didn't
> get exactly what was wrong with your previous iteration of the patch.

There were a couple spots[1] where, if stream_all_attachments were true, the *_parse_decoded_buffer functions would return early since the JS mime emitter didn't care about the data. The effect was that the attachments appeared to have zero size, which was a) inaccurate, and b) confused Thunderbird when it tried to open the attachment.

[1] https://bugzilla.mozilla.org/attachment.cgi?id=480765&action=diff#a/mailnews/mime/src/mimeeobj.cpp_sec1 and https://bugzilla.mozilla.org/attachment.cgi?id=480765&action=diff#a/mailnews/mime/src/mimeiimg.cpp_sec1

> Concerning testing, Mozmill seems like the way to go for this, so I totally
> approve :-).

Sounds good.
> There were a couple spots[1] where, if stream_all_attachments were true, the
> *_parse_decoded_buffer functions would return early since the JS mime emitter
> didn't care about the data. The effect was that the attachments appeared to
> have zero size, which was a) inaccurate, and b) confused Thunderbird when it
> tried to open the attachment.
> 

That makes perfect sense, thanks for the explanation. Good luck for writing the tests!
This patch removes stream_all_attachments, adds a few comments explaining what's happening in libmime, and adds some mozmill tests for the attachment pane display.

Not sure if this needs sr or not, or if I should have someone else review it (blake, maybe?), so feel free to change the review flags to whatever seems best.
Attachment #480765 - Attachment is obsolete: true
Attachment #481065 - Flags: review?(jonathan.protzenko)
Comment on attachment 481065 [details] [diff] [review]
Address issues from comment 27 and add tests

Ok, so I've tested this, and everything works just fine. The test works great, it looks rock solid to me. I'm giving r+ because I think it's worthwhile to have this landed on trunk so that we can get better feedback, but work on this is not complete.

The following issues remain:
1) David, I've requested feedback from you. nsImapBodyShell:557 has "This body part will be downloaded on demand" hardcoded. What happens is the message is displayed even though attachments haven't been downloaded yet. So while waiting for the attachment to be downloaded, the size we display is basically the length of this string (44 bytes). Is there any way for libmime to know that this part is not downloaded? If libmime could be aware of that, it could return something like -1, so that the UI understands that the size is unknown, and displays something better than a bogus size.

Attached images are 22 bytes long when not downloaded, could it be that it's something along the lines of <img src="unknown.png"> or whatever?

2) Jim, your formatFileSize function returns 44 byte instead of bytes, I think this should be fixed (possibly in a separate bug that blocks this one). Although the plural is tricky there with localization... I don't think PluralForm.jsm is available from C++ ;-).

3) Jim, you will have to issue a followup patch as soon as bug 323376 lands, is that ok for you?
Attachment #481065 - Flags: review?(jonathan.protzenko)
Attachment #481065 - Flags: review+
Attachment #481065 - Flags: feedback?(bienvenu)
(In reply to comment #34)
> 2) Jim, your formatFileSize function returns 44 byte instead of bytes, I think
> this should be fixed (possibly in a separate bug that blocks this one).
> Although the plural is tricky there with localization... I don't think
> PluralForm.jsm is available from C++ ;-).

I don't see that (to be fair, I'm not at my dev box right now). Two possibilities spring to mind: either you have bug 323376 applied (I still have that .Cut call in there to strip the trailing null), or you're in a different locale and the name wasn't translated correctly.(In reply 

> 3) Jim, you will have to issue a followup patch as soon as bug 323376 lands, is
> that ok for you?

Not a problem. Depending on what gets checked in first, I'll either update this patch, or just make a new one that removes that line.

Oh also: one thing I should probably test is whether printing still works. Some of my changes indirectly affect that, though the result should be (almost) identical.
You're right, I had the patch from bug 323376 applied, sorry about that!
(In reply to comment #34)

> The following issues remain:
> 1) David, I've requested feedback from you. nsImapBodyShell:557 has "This body
> part will be downloaded on demand" hardcoded. What happens is the message is
> displayed even though attachments haven't been downloaded yet. So while waiting
> for the attachment to be downloaded, the size we display is basically the
> length of this string (44 bytes). Is there any way for libmime to know that
> this part is not downloaded? If libmime could be aware of that, it could return
> something like -1, so that the UI understands that the size is unknown, and
> displays something better than a bogus size.

If the part has an X-Mozilla-IMAP-Part header, I think that means it has not been downloaded. mime_imap_part_address (obj) will tell you that it's a not-downloaded part.
similarly, I think you want to worry about external attachments - you might want to report the size of the external file.
(In reply to comment #37)
> If the part has an X-Mozilla-IMAP-Part header, I think that means it has not
> been downloaded. mime_imap_part_address (obj) will tell you that it's a
> not-downloaded part.

Do you know if there's a reliable way of ensuring that an attachment isn't downloaded temporarily so I can test this?

(In reply to comment #38)
> similarly, I think you want to worry about external attachments - you might
> want to report the size of the external file.

What's an external attachment? If you mean just a regular file that you're attaching when composing a message, I'm doing that in bug 195702.
(In reply to comment #39)
> (In reply to comment #37)
> > If the part has an X-Mozilla-IMAP-Part header, I think that means it has not
> > been downloaded. mime_imap_part_address (obj) will tell you that it's a
> > not-downloaded part.
> 
> Do you know if there's a reliable way of ensuring that an attachment isn't
> downloaded temporarily so I can test this?
Yes, turn off offline download for a folder (e.g., your inbox) using folder properties dialog, and send yourself an e-mail with an external attachment > 25K.

> 
> What's an external attachment? If you mean just a regular file that you're
> attaching when composing a message, I'm doing that in bug 195702.

No, a detached attachment. Right click on an attachment and select detach.
Ok, this version handles the above cases. No tests for the new stuff yet, but I figured I'd upload this so people could take a look at it.
Attachment #481065 - Attachment is obsolete: true
Attachment #481065 - Flags: feedback?(bienvenu)
When you come up with the right tests, ping me and I'll push this to a try-server near you, so that we can check everything still passes.

While I haven't tested this, what happens if the file corresponding to the detached attachment is deleted?
(In reply to comment #42)
> While I haven't tested this, what happens if the file corresponding to the
> detached attachment is deleted?

Probably chaos. I'll check for that.
Hmmm, is there a way to detach an attachment via Mozmill? It seems like there's no easy way to do that, since it requires interacting with OS-specific save dialogs.
There's a detach without prompts method on nsIMessenger.
(In reply to comment #45)
> There's a detach without prompts method on nsIMessenger.

Both detachAttachmentsWOPrompts and detachAttachment (if saveFirst is true) cause Mozmill to crash. I have no idea why, since doing it by simulating clicks works, assuming I intervene when the save dialog appears.

Deleting attachments with detachAttachment works fine, however.
It also seems pretty difficult to set up an IMAP folder in Mozmill, which makes testing non-downloaded attachments difficult too. It seems like the infrastructure for the tests I need to write doesn't exist...
Mozmill doesn't do imap. You'd need to do an xpcshell test for imap, which I believe you can do, with a mime listener.
We already have xpcshell tests for attachment sizes, what we'd need to test here is the interaction with the UI, which I believe can only be done with Mozmill.

Jim, what about inserting a message brutally in some local folder? This is possible, some people do it, they just create the message source from scratch, and insert it into the folder. You could create fake sources for the various states (not downloaded yet, detached) and then insert them into the folder, which would lift the requirement on the IMAP server. See attachment/test-attachment.js

I'm CCing asuth who might have some insight about how to deal with comment #46
Well, I did change some of the backend behavior, so it's probably reasonable to do xpcshell tests for those new cases.
(In reply to comment #49)
> I'm CCing asuth who might have some insight about how to deal with comment #46

I would need to see a callstack / crash reporter reported thing...
(In reply to comment #51)
> I would need to see a callstack / crash reporter reported thing...

Is there an easy way to do that from mozmill?
Oh, I guess I should mention that this is on Linux, since I'm sure the process varies.
Attached patch Update tests (obsolete) — Splinter Review
I updated the tests to handle the following cases (by injecting raw attachment data into the message):

 * deleted attachments
 * detached attachments
 * detached attachments with missing external files

I don't test for not-yet-downloaded attachments because I have no idea what such a message looks like (and I suspect that only IMAP handles that case anyway). It may or may not be worth writing xpcshell tests for some of this too. I'm not sure.
Attachment #481374 - Attachment is obsolete: true
Attachment #482996 - Flags: review?(jonathan.protzenko)
Attached patch Fix printing (obsolete) — Splinter Review
(In reply to comment #15)
> >+  if (NS_SUCCEEDED(rv) && headerSink) {
> You don't need to test for both.

It turns out I *do* need to test for both. :( Otherwise it segfaults when you try to print a message with an attachment.

Anyway, except for testing not-yet-downloaded messages, I think (hope) this is done.
Attachment #482996 - Attachment is obsolete: true
Attachment #483009 - Flags: review?(jonathan.protzenko)
Attachment #482996 - Flags: review?(jonathan.protzenko)
Comment on attachment 483009 [details] [diff] [review]
Fix printing

https://bugzilla.mozilla.org/attachment.cgi?id=483009&action=diff#a/mail/base/content/msgHdrViewOverlay.js_sec2

I'd rather have a comment here along the lines of
// This is a part that hasn't been downloaded yet.

It all looks good to me except that Thunderbird now segfaults when opening a saved message (.eml file) with the following backtrace, so I'm sorry, but I'm r-'ing you...

(gdb) bt
#0  0x00007f74281a63ef in CallQueryInterface<nsISupports, nsWrapperCache> (
    lccx=..., d=0x7fff954c45f8, s=<value optimized out>, type=..., 
    iid=<value optimized out>, scope=0x7f740b7f9270, pErr=0x0)
    at ../../../../dist/include/nsISupportsUtils.h:184
#1  xpcObjectHelper (lccx=..., d=0x7fff954c45f8, s=<value optimized out>, 
    type=..., iid=<value optimized out>, scope=0x7f740b7f9270, pErr=0x0)
    at /home/jonathan/Sources/comm-central/mozilla/js/src/xpconnect/src/xpcprivate.h:3065
#2  XPCConvert::NativeData2JS (lccx=..., d=0x7fff954c45f8, 
    s=<value optimized out>, type=..., iid=<value optimized out>, 
    scope=0x7f740b7f9270, pErr=0x0)
    at /home/jonathan/Sources/comm-central/mozilla/js/src/xpconnect/src/xpcconvert.cpp:489
#3  0x00007f74281b4fe2 in XPCConvert::NativeData2JS (this=0x7f740b6827c0, 
    wrapper=<value optimized out>, methodIndex=<value optimized out>, 
    info=0x7f7418d7ff90, nativeParams=0x7fff954c46c0)
    at /home/jonathan/Sources/comm-central/mozilla/js/src/xpconnect/src/xpcprivate.h:3175
#4  nsXPCWrappedJSClass::CallMethod (this=0x7f740b6827c0, 
    wrapper=<value optimized out>, methodIndex=<value optimized out>, 
    info=0x7f7418d7ff90, nativeParams=0x7fff954c46c0)
    at /home/jonathan/Sources/comm-central/mozilla/js/src/xpconnect/src/xpcwrappedjsclass.cpp:1587
---Type <return> to continue, or q <return> to quit---
#5  0x00007f74286b95ed in PrepareAndDispatch (self=0x7f740bc891e0, 
    methodIndex=<value optimized out>, args=0x7fff954c4840, 
    gpregs=0x7fff954c47c0, fpregs=0x7fff954c47f0)
    at /home/jonathan/Sources/comm-central/mozilla/xpcom/reflect/xptcall/src/md/unix/xptcstubs_x86_64_linux.cpp:153
#6  0x00007f74286b8aaf in SharedStub ()
   from /home/jonathan/Sources/objdir-comm-central/mozilla/dist/bin/libxul.so
#7  0x00007f742846e6c5 in nsMessenger::LoadURL (this=0x7f74165f94c0, 
    aWin=<value optimized out>, aURL=...)
    at /home/jonathan/Sources/comm-central/mailnews/base/src/nsMessenger.cpp:690
#8  0x00007f74284a461f in nsMsgDBView::LoadMessageByUrl (this=0x7f740be1a400, 
    aUrl=<value optimized out>)
    at /home/jonathan/Sources/comm-central/mailnews/base/src/nsMsgDBView.cpp:1047
#9  0x00007f74286b8a22 in NS_InvokeByIndex_P (that=0x7fff954c4910, 
    methodIndex=682834992, paramCount=2504802616, params=0xffffffffff635594)
    at /home/jonathan/Sources/comm-central/mozilla/xpcom/reflect/xptcall/src/md/unix/xptcinvoke_x86_64_unix.cpp:208
#10 0x00007f74281bb15e in CallMethodHelper::Invoke (ccx=<value optimized out>, 
    mode=<value optimized out>)
    at /home/jonathan/Sources/comm-central/mozilla/js/src/xpconnect/src/xpcwrappednative.cpp:3054
---Type <return> to continue, or q <return> to quit---
#11 CallMethodHelper::Call (ccx=<value optimized out>, 
    mode=<value optimized out>)
    at /home/jonathan/Sources/comm-central/mozilla/js/src/xpconnect/src/xpcwrappednative.cpp:2321
#12 XPCWrappedNative::CallMethod (ccx=<value optimized out>, 
    mode=<value optimized out>)
    at /home/jonathan/Sources/comm-central/mozilla/js/src/xpconnect/src/xpcwrappednative.cpp:2285
#13 0x00007f74281c094d in XPC_WN_CallMethod (cx=0x7f7409051000, argc=1, 
    vp=<value optimized out>)
    at /home/jonathan/Sources/comm-central/mozilla/js/src/xpconnect/src/xpcwrappednativejsops.cpp:1631
#14 0x00007f74289567e6 in CallJSNative (cx=0x7f7409051000, 
    entryFrame=<value optimized out>, inlineCallCount=1, 
    interpFlags=<value optimized out>)
    at /home/jonathan/Sources/comm-central/mozilla/js/src/jscntxtinlines.h:652
#15 js::Interpret (cx=0x7f7409051000, entryFrame=<value optimized out>, 
    inlineCallCount=1, interpFlags=<value optimized out>)
    at /home/jonathan/Sources/comm-central/mozilla/js/src/jsinterp.cpp:4714
#16 0x00007f7428796052 in RunScript (cx=0x7f7409051000, 
    argsRef=<value optimized out>, flags=<value optimized out>)
    at /home/jonathan/Sources/comm-central/mozilla/js/src/jsinterp.cpp:638
#17 js::Invoke (cx=0x7f7409051000, argsRef=<value optimized out>, 
---Type <return> to continue, or q <return> to quit---
    flags=<value optimized out>)
    at /home/jonathan/Sources/comm-central/mozilla/js/src/jsinterp.cpp:747
#18 0x00007f7428796a86 in js::ExternalInvoke (cx=0x7f7409051000, thisv=..., 
    fval=..., argc=1, argv=<value optimized out>, rval=0x7fff954c5c68)
    at /home/jonathan/Sources/comm-central/mozilla/js/src/jsinterp.cpp:871
#19 0x00007f7428726f61 in ExternalInvoke (cx=0x7f7409051000, 
    obj=<value optimized out>, fval=18445617572696035808, argc=4284700052, 
    argv=0x7fff954c45a0, rval=0x7f740b7f9270)
    at /home/jonathan/Sources/comm-central/mozilla/js/src/jsinterp.h:950
#20 JS_CallFunctionValue (cx=0x7f7409051000, obj=<value optimized out>, 
    fval=18445617572696035808, argc=4284700052, argv=0x7fff954c45a0, 
    rval=0x7f740b7f9270)
    at /home/jonathan/Sources/comm-central/mozilla/js/src/jsapi.cpp:4961
#21 0x00007f7427f343ef in nsJSContext::CallEventHandler (this=0x7f740b61d760, 
    aTarget=<value optimized out>, aScope=<value optimized out>, 
    aHandler=<value optimized out>, aargv=<value optimized out>, 
    arv=0x7fff954c5e10)
    at /home/jonathan/Sources/comm-central/mozilla/dom/base/nsJSEnvironment.cpp:2142
#22 0x00007f7427f52821 in nsGlobalWindow::RunTimeout (this=0x7f7409051400, 
    aTimeout=0x7f73ff028350)
    at /home/jonathan/Sources/comm-central/mozilla/dom/base/nsGlobalWindow.cpp:8872
---Type <return> to continue, or q <return> to quit---
#23 0x00007f7427f52b22 in nsGlobalWindow::TimerCallback (
    aTimer=<value optimized out>, aClosure=0x7f7428b33c30)
    at /home/jonathan/Sources/comm-central/mozilla/dom/base/nsGlobalWindow.cpp:9217
#24 0x00007f74286af247 in nsTimerImpl::Fire (this=0x7f7414ffc3d0)
    at /home/jonathan/Sources/comm-central/mozilla/xpcom/threads/nsTimerImpl.cpp:425
#25 0x00007f74286af311 in nsTimerEvent::Run (this=<value optimized out>)
    at /home/jonathan/Sources/comm-central/mozilla/xpcom/threads/nsTimerImpl.cpp:517
#26 0x00007f74286acb2b in nsThread::ProcessNextEvent (this=0x7f741e3364c0, 
    mayWait=1, result=0x7fff954c5f5c)
    at /home/jonathan/Sources/comm-central/mozilla/xpcom/threads/nsThread.cpp:547
#27 0x00007f742867caa1 in NS_ProcessNextEvent_P (thread=0x7fff954c4910, 
    mayWait=682834992) at nsThreadUtils.cpp:250
#28 0x00007f74283aca31 in nsBaseAppShell::Run (this=0x7f741e3e9fd0)
    at /home/jonathan/Sources/comm-central/mozilla/widget/src/xpwidgets/nsBaseAppShell.cpp:183
#29 0x00007f742829dc9e in nsAppStartup::Run (this=0x7f74166e8ec0)
    at /home/jonathan/Sources/comm-central/mozilla/toolkit/components/startup/src/nsAppStartup.cpp:191
#30 0x00007f7427b2b3f7 in XRE_main (argc=<value optimized out>, 
---Type <return> to continue, or q <return> to quit---
    argv=<value optimized out>, aAppData=<value optimized out>)
    at /home/jonathan/Sources/comm-central/mozilla/toolkit/xre/nsAppRunner.cpp:3670
#31 0x0000000000401a16 in main (argc=6, argv=0x7fff954c68b8)
    at /home/jonathan/Sources/comm-central/mail/app/nsMailApp.cpp:101
(gdb) q

Another thing I'd love you to test is yencoded-attachments. Their size should be properly counted, but I was unable to test it because gloda doesn't recognize them as attachments. Could you possibly add one of these in your tests? You will find one such attachment commented out in my gloda xpcshell attachment size test. Alternatively, here's one such message: http://www.yenc.org/yenc1.zip

Thanks for your work! We're getting close...
Attachment #483009 - Flags: review?(jonathan.protzenko)
Attachment #483009 - Flags: review-
Attachment #483009 - Flags: feedback+
(In reply to comment #56)
> It all looks good to me except that Thunderbird now segfaults when opening a
> saved message (.eml file) with the following backtrace, so I'm sorry, but I'm
> r-'ing you...

I don't see this on my build (latest trunk on 64-bit Linux). The only patches I have applied are this one and attachment 483612 [details] [diff] [review], which is unrelated.

Does this happen with a particular kind of email? I tried all the cases I could think of (inline, binary, deleted, detached, no attachments), and everything seems to work.

> Another thing I'd love you to test is yencoded-attachments. Their size should
> be properly counted, but I was unable to test it because gloda doesn't
> recognize them as attachments. Could you possibly add one of these in your
> tests? You will find one such attachment commented out in my gloda xpcshell
> attachment size test. Alternatively, here's one such message:
> http://www.yenc.org/yenc1.zip

Maybe this would be best in its own xpcshell test? I know your gloda test does pretty much the same thing, but it's probably worth semi-exhaustively testing this stuff without the JS mime emitter. Not that I'm really enthusiastic about writing more tests. ;)
 
> Thanks for your work! We're getting close...

Thanks! It does seem like we're nearing the bottom of the libmime rabbit hole...
Attached patch Add xpcshell tests (obsolete) — Splinter Review
Ok, I added a comment in addAttachmentField and made an xpshell test that checks a bunch of different cases, including yEnc.
Attachment #483009 - Attachment is obsolete: true
Comment on attachment 484136 [details] [diff] [review]
Add xpcshell tests

>diff --git a/mailnews/mime/test/unit/head_mime.js b/mailnews/mime/test/unit/head_mime.js
>--- a/mailnews/mime/test/unit/head_mime.js
>+++ b/mailnews/mime/test/unit/head_mime.js
>@@ -1,2 +1,4 @@
>+gDEPTH = "../../../../";
>+

What is this for?

Sorry about the yenc stuff, I was sleeping when you commented, I was thinking of something much more lightweight, and I would have given you r+ without it anyway. I was thinking of changing the png image in mail/test/mozmill/attachment/test-attachment-size.js to something encoded with yenc just to make sure this case is covered. Nothing more. But it's awesome you did it!

The crash issue was some stupid build mistake on my side. I guess I'm still not used to libxul-style builds. The funny thing is, I ran the make stuff with your patch, got a crash, removed your patch, did not experience crashes anymore, re-enabled your patch, got crashes again. Sorry for being suspicious!

So r+ for me, assuming you fix the nit above (maybe add var if there's a good reason, or remove the line). I'm requesting review from :bienvenu, as he's more proficient with the imap stuff and mime stuff than I am (esp. the PR_FREEIF stuff).

Awesome patch!
Attachment #484136 - Flags: superreview?(bienvenu)
Attachment #484136 - Flags: review+
(In reply to comment #59)
> Comment on attachment 484136 [details] [diff] [review]
> Add xpcshell tests
> 
> >diff --git a/mailnews/mime/test/unit/head_mime.js b/mailnews/mime/test/unit/head_mime.js
> >--- a/mailnews/mime/test/unit/head_mime.js
> >+++ b/mailnews/mime/test/unit/head_mime.js
> >@@ -1,2 +1,4 @@
> >+gDEPTH = "../../../../";
> >+
> 
> What is this for?

messageInjection.js (I think) gets mad without it. It's in a bunch of spots: http://mxr.mozilla.org/comm-central/search?string=gDEPTH+%3D+%22&find=&findi=&filter=[Gg]DEPTH&hitlimit=&tree=comm-central

Also, one thing I noticed is that I need to fix some of the comments in the xpcshell test, since I just copied the file from elsewhere.
Ok, thanks for the explanation. I had grepped for it in the mime/test/unit folder and I hadn't found a reference to it so I was just curious.
http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry

I've pushed your patch to Thunderbird-try, looks good so far.
Attached patch This time with proper comments (obsolete) — Splinter Review
Just fixing up the comments in the libmime test. Pulling review flags forward.
Attachment #484136 - Attachment is obsolete: true
Attachment #484334 - Flags: superreview?(bienvenu)
Attachment #484334 - Flags: review+
Attachment #484136 - Flags: superreview?(bienvenu)
Good. I just went through the ThunderbirdTry results and they're all good (there's a whole bunch of failures due to the compose window bustage but none of them seem related to your new tests).
One strange thing which might actually be a compose bug: attaching a message to an email, saving it as a draft, and then viewing the message in the drafts folder shows the attached message as being 4 GB (the attachment.size property is -1). Maybe I should add a check for size being negative just to be paranoid...
The patch I did back then for libmime initially sets a size that's -1 to indicate that we don't know the size (as opposed to an empty attachment with size 0). So it's very likely that for a draft message, the file isn't actually attached to the message, so libmime probably leaves the -1 to indicate that the size is unknown. There's a comment for that here and there.

https://bugzilla.mozilla.org/attachment.cgi?id=467086&action=diff#a/mailnews/db/gloda/modules/mimemsg.js_sec4
https://bugzilla.mozilla.org/attachment.cgi?id=467086&action=diff#a/mailnews/mime/src/mimemoz2.cpp_sec3

It's weird, because I thought the attachment would be a link to the file on disk that would allow you to get its size through the same method you used in your patch on the file sizes in the compose window.
Forget what I said, there shouldn't be anything different with a saved draft, attachments are encoded and saved as mime parts with the message, there's nothing specific... weird.
The part that confuses me the most is that it only does this for attached messages.
Attached patch Paranoia is good for you (obsolete) — Splinter Review
I added a check for when libmime says the size is -1. This doesn't actually fix the underlying bug (wherever it is), but it hides it, and this is technically the right thing to do anyway.
Attachment #484334 - Attachment is obsolete: true
Attachment #484484 - Flags: superreview?(bienvenu)
Attachment #484484 - Flags: review+
Attachment #484334 - Flags: superreview?(bienvenu)
Yes I do remember that thing now. The issue is that libmime does not treat the .eml file as an attachment, but rather as just another part of the MIME structure, which means it doesn't necessarily count the bytes for it. I got away with it like so (in the JS Mime representation):

http://mxr.mozilla.org/comm-central/source/mailnews/db/gloda/modules/mimemsg.js#545

We can open another bug afterwards for that and I might look into it when I have some time...
Comment on attachment 484484 [details] [diff] [review]
Paranoia is good for you

thx very much for the patch. Some nits:

Please wrap this extra long line:

+function createNewAttachmentInfo(contentType, url, displayName, uri, isExternalAttachment, size)

Don't need braces here:

+      if (attachment.size != null) {
+        nameAndSize += " ("+messenger.formatFileSize(attachment.size)+")";
+      }

2010
+ * Portions created by the Initial Developer are Copyright (C) 2009

Please add a newline at the end of the file:
+  check_no_attachment_size();
+}
\ No newline at end of file

Looks like bug 323376 got approval to land, so can you remove this:

+
+  // Get rid of the null-terminator; XXX remove this when bug 323376 is fixed!
+  formattedSize.Cut(formattedSize.Length()-1, 1);


early return after call to AddAttachment would be cleaner:

+  if (NS_SUCCEEDED(rv) && headerSink) {
+    headerSink->AddAttachmentField(field, value);
+  }

Please document this method and its arguments in the idl using the doxygen style comments:

+  void addAttachmentField(in string field, in string value);

sr=me, once you address those comments.
Attachment #484484 - Flags: superreview?(bienvenu) → superreview+
Bug 323376 is checked-in, yay! Although you're probably aware of it already, please make sure you un-bitrot your patch, as it doesn't apply cleanly right now.
Attached patch Address commentsSplinter Review
This patch addresses everything from comment 71. Pulling (s)r+ forward. I guess this is done now? Should I tag it with checkin-needed?
Attachment #484484 - Attachment is obsolete: true
Attachment #485140 - Flags: superreview+
Attachment #485140 - Flags: review+
Blocks: 544984
If this patch is ready for checkin, its better to mark it with the checkin-needed keyword, so it can be found by the devs. See: http://blog.bonardo.net/2010/06/22/so-youre-about-to-use-checkin-needed
I know. I'm just asking if :Bienvenu and :protz agree that it's checkin-needed.
I do and I think bienvenu does also since he gave sr+ in his previous comment, so we can just go ahead an check it in, it will give us some exposure.

Do you want me to file the followup bug on .eml attachments not having their size counted, or do you prefer to do it? If you do it, assign to me. Otherwise, I'll file tomorrow. Thanks for your great and hard work on this, and congrats for making it out of the libmime rabbit hole!
Keywords: checkin-needed
It sounds like you know more about the .eml attachment size issue than me, so I'll let you file it.
Followup on bug 606699. I'll do my best to provide a patch before the next release. In the meanwhile, .eml attachments will have no computed size in the attachments pane.
Checked in: http://hg.mozilla.org/comm-central/rev/41df9e16a197
Status: ASSIGNED → RESOLVED
Closed: 14 years ago14 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 3.3a1
(In reply to comment #78)
> Followup on bug 606699. I'll do my best to provide a patch before the next
> release. In the meanwhile, .eml attachments will have no computed size in the
> attachments pane.

I don't know if this is always the case. I know that in my tests, once I sent myself an attached .eml, it showed the size properly, but this may be because it was during the dark times when compose was broken and I was forced to use Gmail's web interface.
Comment on attachment 485140 [details] [diff] [review]
Address comments

I'd love to see this in 3.2 so setting the flag so that it does not fall off the radar!
Attachment #485140 - Flags: approval-thunderbird3.2a1?
In that case, setting dependency on bug 516787 (which adds the file size formatting function this bug uses).
Depends on: 516787
Depends on: 608202
Depends on: TB2SM
Blocks: TB2SM
No longer depends on: TB2SM
Jim, the fix for this bug looks flaky to me. I downloaded the 3.3a1 [Mozilla/5.0 (Windows NT 5.2; rv:2.0b8pre) Gecko/20101116 Thunderbird/3.3a1] yesterday and used it against my IMAP setup. Noticed that when viewing a message with attachments, the only time TB displayed the size was when the attachment was of a small size <10Kb. Any message with large attachment size would not show its size?
You have to keep in mind that a message will be displayed even though the attachments haven't been downloaded yet. This allows you to see the message quickly, while the attachments are downloaded in the background. If you're not offline syncing the folder, the attachments won't be downloaded automatically, which means there's no way we see their size.

Jim, feel free to correct me if I'm wrong. I dimly recall :bienvenu saying there's a 25kb limit for pre-fetching attachments.
Thanks for clarifying this. I don't have offline sync enabled as I like to keep everything on my IMAP server to be able to access it via multiple clients. So, if I read this correctly, this feature will only work if one has Message Sync enabled in the Account Settings?. In this case, it would make sense for the document people to clearly point this out how this is suppose to work. When I looked at the 3.3a1 details at: http://www.mozillamessaging.com/en-US/thunderbird/3.3a1/, it just says "Attachment sizes now displayed along with attachments" as one of the new changes. Which implies that offline sync is not needed for this to work correctly.
(In reply to comment #85)
> Thanks for clarifying this. I don't have offline sync enabled as I like to keep
> everything on my IMAP server to be able to access it via multiple clients.

It shouldn't be necessary to do this.
 
> So, if I read this correctly, this feature will only work if one has Message Sync
> enabled in the Account Settings?. In this case, it would make sense for the
> document people to clearly point this out how this is suppose to work.

I agree, although I don't know if there's going to be much detail in the documentation while Miramar is still in alpha. There are a couple more changes coming with regards to attachment sizes anyway (next up is showing sizes in the compose window), so any documentation would be in flux for a little while.
Jim, I'm bit confused now. Do I need to go to Tools->Account Settings->Synchronization & Storage->Message Synchronizing and check the box for Keep messages for this account on this computer, to enable this feature in order for the attachment size to be displayed?.
(In reply to comment #84)

> Jim, feel free to correct me if I'm wrong. I dimly recall :bienvenu saying
> there's a 25kb limit for pre-fetching attachments.

yes, there's a 30K threshold above which we'll fetch the message by parts, and leave large non-displayable parts on the server, when you click on a message. This doesn't affect inline parts, nor does it affect offline download.
Oh, and that is only for IMAP.
(In reply to comment #87)
> Jim, I'm bit confused now. Do I need to go to Tools->Account
> Settings->Synchronization & Storage->Message Synchronizing and check the box
> for Keep messages for this account on this computer, to enable this feature in
> order for the attachment size to be displayed?.

That should work, though it'll probably take some time to sync everything the first time. Note that you can also set this on a per-folder basis.
I haven't enabled message synchronization. I have both mail.imap.mime_parts_on_demand and mail.imap.fetch_by_chunks disabled. I believe that means when it displays a message in a remote folder that it has to fetch the entire message (including all attachments).

However Thunderbird 3.3a2pre ID:20101123032806 only lists attachments less than 10KB. Even if I hadn't disabled fetching messages by parts it should have listed the size of an attachment when the message has a empty message body, less than 3KB of headers and just a 15KB attachment.
If you have specific issues with this, please file a new bug report.
Well, while I was tinkering with the problem in comment 91, I figured out why non-downloaded attachments were reporting a size of 27 bytes (comment 19) is the length of "This body part will be downloaded on demand." after being base64-"decoded".

Eric, when you file the bug report for your issue, can you CC me and make a note that this is an issue in the IMAP code? (It's returning that "body part" message for attachments when it probably shouldn't be doing so.)
apologies for the spam, but hoping to get some of the cc list interested in a small patch to also emit "X-Mozilla-Altered" per Bug 633837.
Attachment #485140 - Flags: approval-thunderbird3.2a1?
Depends on: 696414
No longer depends on: 696414
Depends on: 696414
No longer depends on: 323376, 516787, 561851, 601096, 608202
Errr... it looks like there's a problem with Bugzilla when updating blockers/dependencies. I'm just going to step slowly away for now...
(In reply to Jim Porter (:squib) from comment #95)
> Errr... it looks like there's a problem with Bugzilla when updating
> blockers/dependencies. I'm just going to step slowly away for now...

I've had that feeling that the behaviour of updating blockers/dependencies is odd for a long time, but so far I couldn't nail it down to a specific problem.
E.g., why don't dependencies/blockers update automatically while viewing a bug, especially when they have not been edited yet?
We certainly do want to keep track of this valuable fix in the attach-ux-tracker, so I'll add that back for a start.
(In reply to Thomas D. from comment #97)
> We certainly do want to keep track of this valuable fix in the
> attach-ux-tracker, so I'll add that back for a start.

We should probably add them all back, but I'm a little afraid to do it, since I tried to fix things and just made it worse...
The biggest danger is this scenario:
- View bug 1, edit dependencies, but don't save yet
- View bug 2, and add dependency to bug 1
-> in database, depency is added to bug 1, but bug 1 on your screen is not updated
-> when you save bug 1, it will overwrite dependency to bug 2 because that wasn't in your dependency list of bug 1 when you started editing dependencies there.
Usually, you'll get the big warning though "... will overwrite all changes blabla"
What I don't like about that warning that there are at least some cases where it is completely unnecessary and overwriting won't lose a thing. But it's hard to tell when exactly. Or maybe I've killed some dependencies without noticing... ;)
(In reply to Thomas D. from comment #99)
> The biggest danger is this scenario:
[snip]

The page wasn't stale. The problem was that I didn't even touch the Blocks field, and it got totally wiped out. Anyway, let's see if I can fix this...
... yup. I specifically kept the attachUXtracker bug in the Blocks list, and it got gobbled up.
(In reply to Jim Porter (:squib) from comment #101)
> ... yup. I specifically kept the attachUXtracker bug in the Blocks list, and
> it got gobbled up.

Actually, it seems to be working now. Maybe it was a problem on my end.
(In reply to Jim Porter (:squib) from comment #100)
> (In reply to Thomas D. from comment #99)
> > The biggest danger is this scenario:
> [snip]
> The page wasn't stale. The problem was that I didn't even touch the Blocks
> field, and it got totally wiped out.

That sounds scary... :(

> Anyway, let's see if I can fix this...

Fixed! :)
Depends on: 655726
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: