User-Agent: Mozilla/5.0 (X11; U; Linux ppc; en-US; rv:1.9.0.7) Gecko/2009032813 Iceweasel/3.0.7 (Debian-3.0.7-1) Build Identifier: Mozilla 5.0 (X11; U; Linux i686; en-US; rv:1,9.1b4pre) Gecko/20090420 Thunderbird/3.b3pre when clicking on 0-byte attachments in received mail nothing happens. Reproducible: Always Steps to Reproduce: 1. send yourself a mail with a zero byte attachment (e.g. by attaching the resoult of "touch test.pdf" to a mail adressed to yourself) 2. click on test.pdf Actual Results: no dialog opens Expected Results: An error dialog should open or at least the possibility to "save" the 0 byte file. It happens quiet often that our users receive 0 byte attachments (from broken mail clients, stripped of by firewalls or whatever). Since clicking on the attachment produces no action at all they usually blame thunderbird for being broken. It usually needs a sysadmin to explain what happened and even then they say: "but the sender assured me that he sent the correct attachment".

Hmm, that's not good. We could at least have a dialog that blames the attachment for being empty. Perhaps on click we can open a dialog along the lines of: This attachment seems to be empty, you might want to check with the person who sent it. ( Cancel ) ( Save Anyway... )

This patch implements the dialog which tells the user that the attachment is empty and offers him to save the attachment or cancle the opening task

Comment on attachment 379702 [details] [diff] [review] Open dialog if attachment is empty file this is TB-only, so you only need one reviewer...

I'm not sure I liked my original text as it was just a placeholder. I'd really like to include something about possible causes. Maybe this: This attachment appears to be empty. Please check with the person who sent this. Often company firewalls or antivirus programs will destroy attachments. Ideas or suggestions are welcome! Add me as a ui-review to the final please. Thanks.

Now that bug 507137 has been fixed (which resulted in false warnings when opening attachments of .eml files), any news from the reviewer?

dmose ^^^

Comment on attachment 379702 [details] [diff] [review] Open dialog if attachment is empty file Thanks for the patch, and sorry for the delay reviewing. I'm slowly working my way through my review backlog. >+function onLoad() >+{ >+ var arguments = window.arguments[0]; Why save the arguments in a local variable? >+ dialog = {}; Why is this necessary? >+ var messengerBundle = document.getElementById("bundle_messenger"); >+ var okButtonText = messengerBundle.getString("emptyAttachmentSaveButtonText"); > >+ dialog.OKButton = document.documentElement.getButton("accept"); >+ dialog.OKButton.label = okButtonText; > >+ dialog.OKButton.focus(); Typically, one would accomplish the above actions using the buttonlabelaccept and defaultButton attributes on the XUL dialog along with a DTD entity. However, I've convinced Bryan that it doesn't make a lot of sense to have a "save anyway" button since neither of us can think of a single use case where it would be valuable to the end-user to create a zero-length file. So let's just ditch this button altogether, and simply have the dialog show the text Bryan suggests in comment 4, along with an "OK" button. >+ var url = io.newURI(aAttachment.url, null, null); >+ var stream = io.newChannelFromURI(url).open(); >+ if (stream.available() == 0) { The docs for nsInputStream.available() include the phrase "A non-blocking stream that does not yet have any data to read should return 0 bytes from this method", which makes me concerned that if this stream is non-blocking, we could incorrectly pop up an "attachment empty" dialog in cases where the attachment hasn't yet started streaming. Whether this is actually a concern in the real world, I don't know. bienvenu, do you have any advice here? Is there any guarantee that this stream will be blocking? Sadly, the docs for nsIInputStream are self-contradictory about how to detect end-of-file using available(). This makes me wonder if a better strategy would be simply attempting to read from the stream to find that out.

The new patch uses nsIPromptService to show the message box with the message from comment #4 and tries to read 1 byte from the stream instead on relying on available() method.

Comment on attachment 397617 [details] [diff] [review] Open dialog if attachment is empty file Good forward progress; definitely feels easier to use now; thanks! General comment: it's helpful to put slightly more comments by the chunks of code you write: it makes it faster to read through the code, and it makes it possible to tell when the code is behaving is intended or not. >+ chunk = inputStream.readBytes(1); I just spent some time discussing our streaming architecture with bienvenu, and it turns out that some of our streams will be blocking, so this could cause the UI to freeze. Making this code use callbacks or async streams is going to make it harder to read. Since this is all really a workaround for poor documentation of .available, my belief now is that it's be over for the best if we push forward with figuring out how to make .available give us the info we need. Perhaps using isNonBlocking in combination with available will be enough information to make the decision we need? >+ let promptService = Components.classes["@mozilla.org/embedcomp/prompt-service;1"] >+ .getService(Components.interfaces.nsIPromptService); Using nsMsgWindow.promptDialog will make things easier on embedders. >+emptyAttachmentWarning=This attachment appears to be empty. Please check with the person who sent this.\nOften company firewalls or antivirus programs will destroy attachments. Having a single \n here results in a dialog that looks somewhat strange. I'd suggest either zero or two newlines.

The new patch differs between blocking and non-blocking streams now and the test for empty attachment is located in a separated method now to keep openAttachment() small. I omitted the mozmill tests, as the framework is missing the functionality to wait for the dialogs to open.

Comment on attachment 401192 [details] [diff] [review] Updated patch that addresses previous remarks (checked in) tried this out, seems to work pretty well.

Comment on attachment 401192 [details] [diff] [review] Updated patch that addresses previous remarks (checked in) Looks good; r=dmose. Thanks much for the patch and for you patience with the review process. Checked in: <http://build.mozillamessaging.com/mercurial/comm-central/rev/cd39fd1c7ae9>.