Part 1.

Part 2 (async replace of nsIBinaryInputStream)

These patches address prior comments and take care of comment 10. Also:

1. Make Bug 1411732 obsolete by displaying remote links in statusbar.
2. Make Bug 1411748 obsolete by not making that code path possible anymore.
3. Port some Fx overlink behavior.
4. Use fetch() to get remote and internal urls, async, with UI busy feedback.
5. An Open Containing Folder option has been added to detached file attachments (other suggestions are out of scope).

Tests will be forthcoming and need a bit of rework for the new async, ie converting existing test to use add_task(async ()) in addition to new tests for link urls.

Notes: debug has been left in for easy testing, and AttachmentInfo.ALWAYSFETCHSIZE is set to true to demonstrate getting sizes from imap/news attachment urls via fetch(); it will be set to false on the checkin patch since libmime already passes up the size.

(In reply to Richard Marti (:Paenglab) from comment #20)

Comment on attachment 9048945 [details] [diff] [review]
externalAttchExtras.patch

Thanks, looks good.

Tested on Windows. The "Open Containing Folder" doesn't work. I get
following error in console:
NS_ERROR_FILE_UNRECOGNIZED_PATH: Component returned failure code:
0x80520001 (NS_ERROR_FILE_UNRECOGNIZED_PATH) [nsIFile.initWithPath]
msgHdrView.js:1991

In downloads tab it works.

Thanks, trying to save a uri conversion, which trick works on linux but not win; to be fixed.

Tests and minor tweaks. aceman, this uses a sleep(), as I didn't see a way to wait for next message selection (a la add_task async in xpc) while an async piece is active, and the tests are autogenerated onto the global scope.

(In reply to Magnus Melin [:mkmelin] from comment #25)

Comment on attachment 9048945 [details] [diff] [review]
externalAttchExtras.patch

Review of attachment 9048945 [details] [diff] [review]:

::: mail/base/content/mailWidgets.xml
@@ +341,5 @@

       let border = this._childNodes[0].boxObject.width -
                    this._childNodes[0].clientWidth;

•      for (let child of this._childNodes) {
  

•        child.width = "";
  

what is this for?

The code calculates optimal layout for the list, if an item's size changes after resolving it, there will be false cropping for certain (short) widths unless it's laid out again. So the existing widths need to be reset.

All other comments have been incorporated. The forthcoming rollup patch includes the 3 separate patches, removes debug, changes ALWAYSFETCHSIZE to false for checkin, and moves the minor tweaks from the tests patch (you can see them in the 2 non test files in that attachment, but they're not re-review worthy imo).

rollup patch.

Regarding comment 5 and subsequent comments not wanting to do that, it should be noted that currently if a detached attachment is Save As, the result saved will be a 0 length file (possibly overriding the existing detached file, and thus datalossy), as the part is considered deleted. So I'd reiterate Save As should be disabled for detached, as the existing code wasn't designed to take a detached file and save it elsewhere.

Also, the patch from Bug 1411732 should be reverted. It's actually dead code as there is currently no such thing as a link attachment in a message that is not also a feed message.

That means, simply remove this hunk from all branches, right?
https://hg.mozilla.org/comm-central/rev/759b892f48b80de23a4d5a80c959f72b9c25f0ad#l1.12

Oh, and please add your email to user in the commit message. The commit message appears to have some junk at the end too

(In reply to Magnus Melin [:mkmelin] from comment #33)

Comment on attachment 9054766 [details] [diff] [review]
externalAttchExtrasFinal.patch

Review of attachment 9054766 [details] [diff] [review]:

::: mail/base/content/msgHdrView.js
@@ +633,5 @@

   // libmime returns -1 if it never managed to figure out the size.

•  // If we want fetch() to check again for internal attachments, set
  

•  // |last.size| to null (instead of -1, which means final fail resolution).
  

•  if (size == -1)
  

•    last.size = null;
  

Mixing types is really not nice :/

So that's what the original code did. What should the unset value be, undefined? How is that better. IMO changing this paradigm now is unnecessary work, so please confirm you want it redone.

(In reply to Jorg K (GMT+2) from comment #32)

That means, simply remove this hunk from all branches, right?
https://hg.mozilla.org/comm-central/rev/759b892f48b80de23a4d5a80c959f72b9c25f0ad#l1.12

Yes, but rather than bitrot this patch, I'll remove that hunk here; I didn't mean strictly hg revert. It doesn't need to be removed from branches unless you want.

(In reply to Magnus Melin [:mkmelin] from comment #27)

Comment on attachment 9052256 [details] [diff] [review]
externalAttchTestsFinal.patch

Review of attachment 9052256 [details] [diff] [review]:

::: mail/test/mozmill/attachment/test-attachment-menus.js
@@ +443,5 @@

• // Test funcs are generated in the global scope, and there isn't a way to
• // do this async (like within an async add_task in xpcshell) so await can
• // force serial execution of each test. Wait here for the fetch() to complete.
• controller.sleep(1000);

can't you make the generated function async and await here?
At
https://searchfox.org/comm-central/rev/
2a6de7ec2104e7efa2a3f29fb0d4162c49ad2b67/mail/test/mozmill/attachment/test-
attachment-menus.js#382

... = async function() {

}

That's the first thing of very many things I tried. I don't think js understands a syntax that adds a variable function member with a prefix to |this| or any other object. And even if it did, the main mozmill function calling test_* pattern names in |this| would also have to be changed to be async. Rather out of scope here. But perhaps someone does know of a way to autogen async funcs for mozmill that isn't hypothetical.

Another way is to just write out all the funcs in the file (2 files) but having a simple sleep() is much less ugly.

(In reply to alta88 from comment #35)

So that's what the original code did. What should the unset value be,
undefined? How is that better. IMO changing this paradigm now is unnecessary
work, so please confirm you want it redone.

Looks like there is only a few occurrences of size null checks.
I see the problem. I would add a boolean to AttachmentInfo, say fileSizeOk. For the fileSizeOk == false case (meaning size == null currently) I'd let size be whatever it is, but not use it for anything until fileSizeOk is true.

(In reply to Magnus Melin [:mkmelin] from comment #33)

Comment on attachment 9054766 [details] [diff] [review]
externalAttchExtrasFinal.patch

Review of attachment 9054766 [details] [diff] [review]:

I think there's a few things to do here, but r=mkmelin with these
fixed/addressed

::: mail/base/content/mailWidgets.xml
@@ +340,5 @@

       let border = this._childNodes[0].boxObject.width -
                    this._childNodes[0].clientWidth;

•      for (let child of this._childNodes) {
  

•        child.width = "";
  

could you add the explanation into the code?

sure.

but, something's wrong here. the loop sets the width (but doesn't use it),
then use the last width for everyone in the next loop.

combine those loops?

no, after width is set on each element, it has to be cleared otherwise the natural width as gotten from scrollWidth won't be.. natural. and each item has to be checked to get widest, and then each has to have the widest width set separately. if it were an html grid there wouldn't be this junk.

::: mail/base/content/mailWindow.js
@@ +259,5 @@

 if (status.length > 0)
   this.showStatusString(status);

},

• setOverLink(url, anchorElt) {

please add jsdoc documentation about this

sure.

::: mail/base/content/messageWindow.xul
@@ +46,5 @@

%quickFilterBarDTD;
<!ENTITY % msgViewPickerDTD SYSTEM "chrome://messenger/locale/msgViewPickerOverlay.dtd" >
%msgViewPickerDTD;
+<!ENTITY % aboutDownloadsDTD SYSTEM "chrome://messenger/locale/aboutDownloads.dtd">
+%aboutDownloadsDTD;

I think it would be better to add a new entitty for this menu instead of
reusing one from aboutDownloads

duplicitous, but fine.

::: mail/base/content/msgHdrView.js
@@ +633,5 @@

   // libmime returns -1 if it never managed to figure out the size.

•  // If we want fetch() to check again for internal attachments, set
  

•  // |last.size| to null (instead of -1, which means final fail resolution).
  

•  if (size == -1)
  

•    last.size = null;
  

Mixing types is really not nice :/

@@ +1717,5 @@

• // attachment here. For internal attachments and link attachments with a
• // reported size, libmime streams values to addAttachmentField() which
• // updates this object. For external file attachments, |size| is updated
• // in the isEmpty() function by fetch() when the list is built.
• // Deleted attachments are resolved to -1.

Instead of having size null and keeping track of that "special meaning", I
think you should pull that into another variable like sizeChecked

fine.

@@ +1815,5 @@

• • @returns true if the attachment is a detached file, false otherwise.
• */
• get isFileAttachment() {
• return this.isExternalAttachment && this.url.startsWith("file:");

came to mind, maybe we should make sure this is case insensitive. Maybe
easiest then to do the regexp like below, but add "i"

the path is gotten purely internally, the nsIFile spec is always lowercase, as per spec for URI schemes, so not at all necessary.

@@ +2349,5 @@

 gBuildAttachmentsForCurrentMsg = true;

}
}

+function displayAttachmentsForExpandedViewExternal() {

not sure exactly where this goes wrong, but for a detached attachment even
if you correct the path it still displays as not found (anymore).

it works for me to rename a file, load the message, get not found, rename it back, reload, and have it found again. so i don't understand your STR or what 'correct the path' means.

@@ +2544,5 @@

+/**

• • Calculate the total size of all attachments in the message as emitted to
• • |currentAttachments| and return a pretty string.
• • @returns {String} sizeStr (e.g. 123 KB or 3.1MB)

@returns {String} a description of the attachment size (e.g. 123 KB or 3.1MB)

@@ +2721,5 @@

item = popup.insertBefore(item, popup.childNodes[indexOfSeparator]);

• if (attachment.isExternalAttachment) {
• if (!attachment.hasFile) {

•  item.setAttribute("notfound", true);
  

can we please use a class for this too instead

fine.

unbitrot.

(In reply to alta88 from comment #39)

• get isFileAttachment() {
• return this.isExternalAttachment && this.url.startsWith("file:");

came to mind, maybe we should make sure this is case insensitive. Maybe
easiest then to do the regexp like below, but add "i"

Yeah, looks like it is normalized earlier. Hard to know that this was the case.

For the rename thing: I can't reproduce now. Well, I can, but not under normal circumstances, and I wasn't able to see why that test case doesn't work.

Sent to try: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=a857da17f04679c76a9cb0da738b4f1d8d298843

fix linting.

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/d167921e42eb
Enhance UI display/feedback for external (deleted, detached to file, and http link) attachments. r=mkmelin
https://hg.mozilla.org/comm-central/rev/a240e0e27ce5
Test adjustments and new tests. r=mkmelin

I took the liberty to shorten the commit message and to move the extra goodies below in the header.

followup fix, the change out of null forgot to update a check so links get checked again even if resolved.

kind sheriff, could you land at your convenience please.

Done.

(In reply to alta88 from comment #36)

(In reply to Jorg K (GMT+2) from comment #32)

That means, simply remove this hunk from all branches, right?
https://hg.mozilla.org/comm-central/rev/759b892f48b80de23a4d5a80c959f72b9c25f0ad#l1.12

Yes, but rather than bitrot this patch, I'll remove that hunk here; I didn't mean
strictly hg revert. It doesn't need to be removed from branches unless you want.

Just for the record: the hunk in question was removed here:
https://hg.mozilla.org/comm-central/rev/d167921e42eba41654952c307cec2f0fcaa022d2#l6.190

I assume that we won't backport the changes here, so I guess it will stay on the other branches.