657856 - Improve interface for attachment objects in the message reader

Status: RESOLVED FIXED

(Thunderbird :: Message Reader UI, defect)

Description

[Jim Porter (:squib)]

Attachment: Improve the interface for attachment objects

For some bugs I'm working on, it would be very nice if the interface for attachment objects in Javascript was improved. Currently, they are "just" dictionaries, which is a bit clumsy, especially when checking for deleted attachments (attachment.contentType == "text/x-moz-deleted").

Attached is a patch to improve the interface by giving the objects some useful methods. I also eliminated the cloneAttachment function, since as far as I can tell, it served no useful purpose.

Existing tests should cover most everything here already.

Comment 1

[Jim Porter (:squib)]

Attachment: Tiny change

This is just a tiny change over the last version, to keep some code from exceeding 80 characters in a subsequent patch. :)

Comment 2

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 533165 [details] [diff] [review]
Tiny change

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

I've listed a couple of nits below, but this patch seems like a decent improvement, so r=me with those fixed.

::: mail/base/content/msgHdrViewOverlay.js
@@ +1682,5 @@
> +   */
> +  get isDeleted()
> +  {
> +    return this.contentType == "text/x-moz-deleted";
> +  },

Do you want to change mail/base/content/mailCore.js line 591 to use this attribute, too?

@@ +1692,5 @@
> +   */
> +  get isEmpty()
> +  {
> +    let io = Components.classes["@mozilla.org/network/io-service;1"]
> +                       .getService(Components.interfaces.nsIIOService);

Please use Services.io instead, if you can.

@@ +1977,3 @@
>        item.setAttribute("context", "attachmentListContext");
>  
> +      item.attachment = attachment;

Why aren't we cloning the attachment anymore?

@@ +2300,5 @@
> +       case 'saveAs':
> +         actionFunction = function(attachment) { attachment.save(); };
> +       case 'detach':
> +         actionFunction = function(attachment) { attachment.detach(true); };
> +     }

Do we want a default case that logs an error, in case we accidentally type "saveas" sometime?

Comment 3

[Jim Porter (:squib)]

(In reply to comment #2)
> Comment on attachment 533165 [details] [diff] [review] [review]
> Tiny change
> 
> Review of attachment 533165 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> I've listed a couple of nits below, but this patch seems like a decent
> improvement, so r=me with those fixed.
> 
> ::: mail/base/content/msgHdrViewOverlay.js
> @@ +1682,5 @@
> > +   */
> > +  get isDeleted()
> > +  {
> > +    return this.contentType == "text/x-moz-deleted";
> > +  },
> 
> Do you want to change mail/base/content/mailCore.js line 591 to use this
> attribute, too?
> 
> @@ +1692,5 @@
> > +   */
> > +  get isEmpty()
> > +  {
> > +    let io = Components.classes["@mozilla.org/network/io-service;1"]
> > +                       .getService(Components.interfaces.nsIIOService);
> 
> Please use Services.io instead, if you can.

Done.

> @@ +1977,3 @@
> >        item.setAttribute("context", "attachmentListContext");
> >  
> > +      item.attachment = attachment;
> 
> Why aren't we cloning the attachment anymore?

As far as I can tell, it's totally unnecessary, and just leads to extra objects being created. I could only see it being useful if we were modifying any of the values in the AttachmentInfo objects, but we aren't.

> @@ +2300,5 @@
> > +       case 'saveAs':
> > +         actionFunction = function(attachment) { attachment.save(); };
> > +       case 'detach':
> > +         actionFunction = function(attachment) { attachment.detach(true); };
> > +     }
> 
> Do we want a default case that logs an error, in case we accidentally type
> "saveas" sometime?

That switch statement only gets hit when action is 'open' or 'saveAs'. The default case is after that (the 'detach' case shouldn't be there, and I've since removed it).

Comment 4

[Jim Porter (:squib)]

Attachment: Address review comments

Pulling forward r+. I think this should go into Miramar, since it blocks bug 656045, which should also be in Miramar if at all possible.

Comment 5

[Mark Banner (:standard8)]

Comment on attachment 534978 [details] [diff] [review]
Address review comments

Sorry, but I don't want the added risk for this on beta. There's no dataloss afaict, just strange files if you do hit it. I also doubt it will be hit that frequently. I think we can take it for the next version, but I've not yet set up the approval flag for aurora.

Comment 6

[Jim Porter (:squib)]

Checked in (with a tiny change to use Services.io): http://hg.mozilla.org/comm-central/rev/ad6e9c9ea574