838009 - [E-Mail][User Story] E-Mail attachment saving

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::E-Mail, defect, P1)

Description

[Peter Dolanjski [:pdol]]

UCID: Email-137

User Story:
As a user, I want to be able to save and view/play received image, audio or video attachments (based on currently supported MIME types) to allow me to access received content.

Comment 1

[Andrew Sutherland [:asuth] (he/him)]

Breaking this down by type:

- Images: Already supported.  We save images to the 'pictures' device-storage mechanism and use the 'open' web activity to display the image by passing a Blob.

- Video: Not currently supported.  We can modify our code to save videos to the 'videos' device-storage mechanism.  The video app currently only supports a 'view' activity, not an 'open' activity.  The 'view' activity takes a URL and does not accept a Blob.  Although we can generate urls for Blob access, I believe that is constrained to the calling window.  I have filed bug 838655 to track the necessary work in the video app to accomplish this.

- Audio/Music: Not currently supported.  We can modify our code to save audio/music to the 'music' device-storage mechanism.  The music app already supports an 'open' activity that we can use.  (Note that the activity code does a weird dance to re-get the file from device storage that isn't entirely justified by the comments at hand.  There may be some inefficiency there.  If the comments are accurate, however, it may mean the video app has to do a similar dance.)

NOTE: This will not add support for viewing PDF's or anything else.

Comment 2

[Kai-Chih Hu [:khu]]

Dominic, since you're working on bug 836497, do you think you can handle this one as well? Welcome to find someone to help you. 

Thanks.

Comment 3

[Rob MacDonald [:robmac]]

Hi Dominic...

I had a few questions while working on the spec for this issue.

First, if a user were to download an attachment while in a message, does the attachment continue to download if the user closes the message? And what would happen if they tasked away from email?

Second, does a file need to be downloaded before the phone can recognize whether it can be opened?

Third, can the downloads be automatic if connected via wi-fi and manual if connected over the carrier network? Or, in other words, does the email app know the connection type?

Feel free to ping me on IRC (robmac) if these questions don't make sense.

- Rob

Comment 4

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Rob MacDonald from comment #3)
> First, if a user were to download an attachment while in a message, does the
> attachment continue to download if the user closes the message? And what
> would happen if they tasked away from email?

Yes, it continues to download.

It would continue to download in the background unless the e-mail app is killed because of a low memory situation.
 
> Second, does a file need to be downloaded before the phone can recognize
> whether it can be opened?

Generally, no.  MIME type information is available with the attachment's file name (if any) and approximate file size.

The special case where the answer might be 'yes' is for Microsoft TNEF encoded stuff.  We're not going to worry about that right now, though.

> Third, can the downloads be automatic if connected via wi-fi and manual if
> connected over the carrier network? Or, in other words, does the email app
> know the connection type?

Downloads could be automatic.  The mozConnection API is not implemented, but there are more privileged APIs we can use to detect if we are on a wireless data connection or a wi-fi connection.

Comment 5

[Rob MacDonald [:robmac]]

The UX flows for this bug are located within the general email attachment flows on dropbox:

https://www.dropbox.com/s/5giljes7ao9gyrg/email-attachments.pdf

The final part of the document includes flows for saving as well as a proposal for a document manager, which is not in scope for this story.

Comment 6

[Dominic Kuo [:dkuo]]

Attachment: Part 1 - Enable saving/playing music in email attachments

Andrew,

This is the first patch for enabling download/open audio files for email app.
I have made some changes in same-frame-setup.js but not sure I am doing right or not, feel free to point out any problems, thanks.

Comment 7

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 722168 [details]
Part 1 - Enable saving/playing music in email attachments

This looks good!

In terms of same-frame-setup.js, the patch shouldn't make changes to files in apps/email/js/ext/* without a corresponding pull request to the gaia-email-libs-and-more repo that makes the changes in the source file.  And then the changes in the gaia repo should be made by using the "make install-into-gaia" target to make sure everything propagates correctly.

The file you want to modify is:
https://github.com/mozilla-b2g/gaia-email-libs-and-more/blob/master/data/lib/mailapi/jobmixins.js#L240

It gets built into same-frame-setup.js by the "make install-into-gaia" process.

Instead of having separate if statements, I think we should probably use an else-tree.  For now, I think it makes sense to throw as the final 'else', but eventually we would probably just end up writing to 'sdcard'.  (Or would we always just use sdcard in that case?  Is there any benefit to using the discrete device storage groups?)

Clearing review to wait for follow-up patch.  Let me know if you have any issues doing the build from gaia-email-libs-and-more.  :squib can also be of assistance, but the key things are to: 1) do a --recursive checkout, as per the README.md, 2) make sure you setup the symlinks as per the README.md.

Comment 8

[Dominic Kuo [:dkuo]]

Attachment: support sdcard storage for saving all types of attachments

Since we need to support image, audio and video for saving attachments in v1.1, we can just use sdcard storage for all these types, and in future, I believe we are going to support the other types like pdf or txt, so use sdcard storage should be a good choice.

Comment 9

[Dominic Kuo [:dkuo]]

Comment on attachment 722168 [details]
Part 1 - Enable saving/playing music in email attachments

Andrew, with attachment 723344 [details] and this updated patch, email should be able to support download/play audio attachments.

* Note that there is a logic bug in buildBodyDom(), please see
https://github.com/mozilla-b2g/gaia/blob/master/apps/email/js/message-cards.js#L1358
you can see the getAttachmentBlob() is a async function but we called it in a for loop, so after the callback of getAttachmentBlob(), it will always get the last attachment variable which is the right variable we wanted, so I also removed that logic and fix it.

Comment 10

[Dominic Kuo [:dkuo]]

(In reply to Dominic Kuo [:dkuo] from comment #9)
> * Note that there is a logic bug in buildBodyDom(), please see
> https://github.com/mozilla-b2g/gaia/blob/master/apps/email/js/message-cards.
> js#L1358
> you can see the getAttachmentBlob() is a async function but we called it in
> a for loop, so after the callback of getAttachmentBlob(), it will always get
> the last attachment variable which is the right variable we wanted, so I
                               ^^^^^^^^^^^^^ I meant "not" the right variable
> also removed that logic and fix it.

Comment 11

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 722168 [details]
Part 1 - Enable saving/playing music in email attachments

Dominic, good catch on the loop being broken.  I hadn't realized that our pre-fetching of the blobs was no longer required since the mozWebActivity security policy had been relaxed.  (Fabrice says we can create activities whenever as long as we are foreground.)

Comment 12

[Andrew Sutherland [:asuth] (he/him)]

Dominic landed the patches last night:

landed on gaia-email-libs-and-more/master:
https://github.com/mozilla-b2g/gaia-email-libs-and-more/pull/145
https://github.com/mozilla-b2g/gaia-email-libs-and-more/commit/91936670845062c2d30cf79c2a1f119bc7aa919b

landed on gaia/master (note: without gelam change included):
https://github.com/mozilla-b2g/gaia/pull/8516
https://github.com/mozilla-b2g/gaia/commit/000f078564077b540529b94fc34689b8c407c9d9


a small 'sdcard' fix was required on gaia-email-libs-and-more (which did not impact correctness, just unit tests and secret debug mode):
landed on gaia-email-libs-and-more/master:
https://github.com/mozilla-b2g/gaia-email-libs-and-more/pull/147
https://github.com/mozilla-b2g/gaia-email-libs-and-more/commit/b821c3a8e241b7c98f0824dc5cc4d1a14f05c115


and then the 'sdcard' changes were propagated to gaia.  landed in gaia/master:
https://github.com/mozilla-b2g/gaia/pull/8624
https://github.com/mozilla-b2g/gaia/commit/f53c1d8d66eb1c679e9b7060776bc23fcf7a4fa9

Comment 13

[Dominic Kuo [:dkuo]]

Andrew, I just noticed this bug includes supporting video attachments, and I had enabled the open activity in bug 838655, so we should be able to support video.

I have tried to test the mimetypes of mp4, webm, ogv and 3gp(formats that we currently support in video app), and found mp4 and ogv report "video/mp4" and "video/ogg", the others report "application/octet-stream", do you have any suggestion that we can distinguish the mimetypes that are not starts with "video/"? or we can just take the file extension to do some mappings?

Comment 14

[Andrew Sutherland [:asuth] (he/him)]

The MIME types we use are:
1) Whatever the sending e-mail client tells us, unless it's application/octet-stream, at which point we...
2) Look up the file extension in https://github.com/andris9/mimelib/blob/master/lib/content-types.js and try and use that instead

We can/should add all MIME types we support to that file that don't currently exist.  Probably the right course of action for that is for us to: fork mimelib into mozilla-b2g, add the missing MIME types, issue a pull request to get the additional types upstreamed, update gaia-email-libs-and-more to use the mozilla-b2g repo.  When the pull request gets merged in, update the commit we're using in GELAM to reference the updated master.


I had thought there was another bug for video for open, but it appears I was mistaken.  Since this bug is leo+ it probably makes sense to track that fix on this bug still.

Comment 15

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to Dominic Kuo [:dkuo] from comment #8)
> Created attachment 723344 [details]
> support sdcard storage for saving all types of attachments
> 
> Since we need to support image, audio and video for saving attachments in
> v1.1, we can just use sdcard storage for all these types, and in future, I
> believe we are going to support the other types like pdf or txt, so use
> sdcard storage should be a good choice.

Note that this makes the code more device dependent. For example in the leo release I believe we might by default store pictures on internal storage which means that if you just use the 'sdcard' storage you'll save them in the wrong place.

It's not a huge deal though, we just have to make sure to always use the right storage area for each piece of hardware.

Comment 16

[Julien Wajsberg [:julienw]]

The commits in comment 12 were landed on v1.0.1 (but disabled) as part of Bug 851124 uplift. I won't change the flags as this is disabled.

Comment 17

[Joe Cheng [:jcheng] (please needinfo)]

:dkuo,:asuth, mind updating the latest on this bug? seem like it's not moving forward? or there is a separate discussion? thanks

Comment 18

[Dominic Kuo [:dkuo]]

Joe, to this user story we should also support video attachment, so it was reopend becasue we just finished the image/audio parts. Video needs another patch to complete this bug.

Comment 19

[Julien Wajsberg [:julienw]]

dkuo> is this possible to do it in another bug ?

I'd like to keep the simple scheme: "one bug = one patch" or rather "one bug = several patches landed in the same time".

Also, the already commited patch will land today in v1-train, I don't like to have only part of bugs landed.

Comment 20

[Dominic Kuo [:dkuo]]

(In reply to Julien Wajsberg [:julienw] from comment #19)
> dkuo> is this possible to do it in another bug ?
> 
> I'd like to keep the simple scheme: "one bug = one patch" or rather "one bug
> = several patches landed in the same time".
> 
> Also, the already commited patch will land today in v1-train, I don't like
> to have only part of bugs landed.

yes, and I also think it's a good idea to open another bug to trace the video problem.
Let's close this one first and I will file a new one, sorry for the inconvenience, I will try to keep it simple and clean next time, thanks.

Comment 21

[Julien Wajsberg [:julienw]]

np, I'm closing this then :) thanks !

Comment 22

[Julien Wajsberg [:julienw]]

Landed in v1-train as part of Bug 851124 big uplift.

Comment 23

[Jason Smith [:jsmith]]

I believe we have test case coverage for this, right?

Comment 24

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

rough draft test case : #6561