Mila Davydova Reporter
	Description • 12 years ago

Unagi, build ID 20121217070202

 After receiving an email with image attachment user is able to tap on the attachment icon to download it. Spinner icon  appears to indicate that file is being downloaded. After file is downloaded user can tap on the "View" button to open the file. Tapping  "View" button takes user to the blank page with no image displayed.

Repro steps:
1. Install build #20121217070202
2. Set up an email account on the device
3. From a secondary account send an email with an image attachment to the account on the device (I used jpg file as an attachment)
4. Open the email and tap on the image attachment
5. Tap on the "View" button to open the file

Expected result:
User is able to open the attachment

Actual result: 
User is not able to open the attachment. Attachment icon is shown at the top of the email, but user is not able to open it, tapping it is not loading its content.   

Additional comments: When user receives an email with pdf file attached, user is able to see details (attachment icon,title,format,size) but has no option to download it.

	Andrew Sutherland [:asuth] (he/him) Assignee
	Comment 2 • 12 years ago

I'm confused about what's going on here.  I was able to reproduce this on my unagi device, but the problem went away when I did "make reset-gaia".

My over-the-air updates may not have been working for gaia, possibly because I think I had installed a local gaia some days ago for development/testing.  My lock screen still had the bouncy curved pull-up-to-get-the-unlock-button implementation, whereas after reset I now have the horizontal line one.

As such, I think it would be great if people could try to duplicate this with other devices, and if it does fail, see if flashing a current build in a device-wiping way fixes the problem.

Note that the "NaNK" problem is expected until the fix for bug 824190 gets reviewed and landed.

	Mila Davydova Reporter
	Comment 3 • 12 years ago

Unagi build 20121231070201
This defect does not reproduce with a currant build(20121231070201). User is able to download and view an image attachment.
 However, when user receives an email with pdf file attached, user is able to see the attachment icon, but does not have an option to download it. tapping the attachment icon does not load its content.

	Andrew Sutherland [:asuth] (he/him) Assignee
	Comment 4 • 12 years ago

(In reply to Mila Davydova from comment #3)
> Unagi build 20121231070201
> This defect does not reproduce with a currant build(20121231070201). User is
> able to download and view an image attachment.

Excellent.

> However, when user receives an email with pdf file attached, user is able
> to see the attachment icon, but does not have an option to download it.
> tapping the attachment icon does not load its content.

Yes, this is the expected behaviour for v1.  Handling non-image attachments was explicitly declared out-of-scope after discussion with product.  I've updated the bug subject and marked this as an enhancement.

	ayman maat :maat
	Comment 8 • 11 years ago

[TEF_REQ] as Feature required for TEF build.

	Jeni
	Comment 9 • 11 years ago

Issue repros.  Unable to open PDF file in email application, Test case 3994


Build ID: 20130130070201
Kernel: Dec 5
Gecko  http://hg.mozilla.org/releases/mozilla-b2g18/rev/4593f3e765eb
Gaia   f7f5a0cd17e3d04308cc5850b254947e127122b9

	Darren
	Comment 10 • 11 years ago

Issue appears to be a duplicate of bug 823410.

	Andrew Sutherland [:asuth] (he/him) Assignee
	Comment 11 • 11 years ago

Not a dupe of the pdf.js bug; the e-mail app does not currently support downloading any mime types but those of images and only will trigger 'open'ing of images.

	Naoki Hirata :nhirata (please use needinfo instead of cc)
	Comment 12 • 11 years ago

The test case actually calls for testing against an image.  I'm glad they ran a non test case here to check for pdf, having said that this isn't for v1.  looks like it's slated for v1.1

	Tim Guan-tin Chien [:timdream] (please needinfo)
	Comment 13 • 11 years ago

Sounds like the image comes with a corrupted web activities registry, a Gecko issue maybe?

Is it possible to do some forensics on the registry of the given image?
needinfo Fabice for some ideas. thanks.

	Tim Guan-tin Chien [:timdream] (please needinfo)
	Comment 14 • 11 years ago

* Fabrice, my apologies.

	[:fabrice] Fabrice Desré
	Comment 15 • 11 years ago

The activity registry is created at startup and updated when we apply an update. It's stored in an indexedDB database in /data/local/indexedDB/chrome. If we get that it's possible to dump the list of registered activities.

	Tim Guan-tin Chien [:timdream] (please needinfo)
	Comment 16 • 11 years ago

I am sorry, apparently this is a v1.1 feature request, not a bug. (See comment 4, comment 11). So we will need an assignee here...

	Dominic Kuo [:dkuo]
	Comment 17 • 11 years ago

What are the target formats we are going to support in v1.1?

As I know at least we should support image, audio and video, but the detail file extentions and mimetypes are not defined yet, can anyone clarify this? thanks.

	Dominic Kuo [:dkuo]
	Comment 18 • 11 years ago

Bug 838009 is for image, audio and video, setting dependency.

	Andrew Sutherland [:asuth] (he/him) Assignee
	Comment 19 • 11 years ago

As Dominic says, we only have user stories for image/audio/video.  The big problem here is that we need to white-list specific MIME types because I don't believe web activities let us know *before* we download the attachment whether there is any web activity that can meaningfully service the MIME type.

Fabrice, correct me if I'm wrong, but there's no way for us to say "hey, mozWebActivity, would you be able to service 'application/vnd.openxmlformats-officedocument.wordprocessingml.document' if I gave it to you".  Without that, if we download something that lacks an activity with the ability to open the file type *and delete the file type*, we are up a creek.  I understand UX has plans for a document manager that would act as a filesystem browser, but I don't believe that's been implemented.

The variety of options available to us are:
1) White-list only PDF attachments / things we know we are shipping an app for.
2) Make the e-mail app UI capable of deleting documents it has downloaded from its UI
3) Make the e-mail app automatically delete documents that don't match a white-list when the mail message is purged from the system
4) Implement a document manager app that browses all of the sd card
5) Have mozWebActivities allow us to probe for apps that cover a MIME type.  This might require us to define an activity like "manage" in addition to "open" so that we could know that pdf.js can open PDF files, but it can't manage them.

	[:fabrice] Fabrice Desré
	Comment 20 • 11 years ago

I would go for 1) for now, since 5) would be very a very strange UX : when calling mozactivity('manage', 'application/foobar'), we would open the matching app, or show the app picker.

As a side note, we need 4) for other use cases also (like the generic <input type=file>) but I think this is a project on its own.

	Andrew Sutherland [:asuth] (he/him) Assignee
	Comment 21 • 11 years ago

Peter, this bug now needs some clarification from product about the user story that is leo+ here.

Is it okay for the e-mail app to just whitelist PDF files?  What about deleting the PDF's once downloaded (see comment 19)?  And do we actually ship a PDF app with the device?

	Dominic Kuo [:dkuo]
	Comment 22 • 11 years ago

okay, sounds like except image/audio/video, we should at least also support download/view PDF attachments(because we have pdf viewer in gaia), how about only download and view image/audio/video/pdf in email? we can start to modify PDFJS app to support open activity, or just use the existed view activity.

	Dominic Kuo [:dkuo]
	Comment 23 • 11 years ago

Just check again and I found we shouldn't modify pdfjs from gaia level, so when email has pdf attachments, we should use "view" activity to view the pdf files.

	Dominic Kuo [:dkuo]
	Comment 24 • 11 years ago

Ian can you help to start on supporting pdf in email first? thanks.

	Peter Dolanjski [:pdol]
	Comment 25 • 11 years ago

(In reply to Andrew Sutherland (:asuth) from comment #21)
> Peter, this bug now needs some clarification from product about the user
> story that is leo+ here.
> 
> Is it okay for the e-mail app to just whitelist PDF files?  What about
> deleting the PDF's once downloaded (see comment 19)?  And do we actually
> ship a PDF app with the device?

No PDF app that I'm aware of.  
Just to clarify, does whitelisting imply the user can only download MIME types that have been whitelisted?  

Rob, this could use your UX input on deletion of files (in the case where a standalone app to be able to delete the file doesn't exist - like PDF).

	Andrew Sutherland [:asuth] (he/him) Assignee
	Comment 26 • 11 years ago

(In reply to Dominic Kuo [:dkuo] from comment #23)
> Just check again and I found we shouldn't modify pdfjs from gaia level, so
> when email has pdf attachments, we should use "view" activity to view the
> pdf files.

This seems very awkward; unless we get number 5 from comment 19, then we either need to be using exclusively "open" or "view" long term.  I really wish :djf's efforts to normalize our web activity use hadn't been killed.

But yeah, for leo, we should just whitelist the mime-type and do whatever "view" requires.

	Andrew Sutherland [:asuth] (he/him) Assignee
	Comment 27 • 11 years ago

(In reply to Peter Dolanjski [:pdol] from comment #25)
> No PDF app that I'm aware of.  
> Just to clarify, does whitelisting imply the user can only download MIME
> types that have been whitelisted?  

Yes.
 
> Rob, this could use your UX input on deletion of files (in the case where a
> standalone app to be able to delete the file doesn't exist - like PDF).

If we have a standalone app that can manage these files, then we can stop whitelisting and just allow blanket downloads.  Especially if we also gain a "get an app for this mime-type!" app as a fallback when we are missing an app that can handle the MIME type once downlodaed.

	Rob MacDonald [:robmac]
	Comment 28 • 11 years ago

(In reply to Peter Dolanjski [:pdol] from comment #25)
> 
> Rob, this could use your UX input on deletion of files (in the case where a
> standalone app to be able to delete the file doesn't exist - like PDF).

Assuming there's no PDF viewer or document manager, I would propose that we delete any PDF attachments when the email is no longer available. Otherwise there's no way I know of for the user to delete the files from the phone and they would just consume valuable space.

Although it's not a v1.1 requirement, UX has raised the idea of a document manager as a potential solution for allowing users to identify, share and delete files that are not associated with any application on the phone. The proposal rose out of the email attachment specs, but would also apply to bluetooth, SMS, SD card, USB, browser downloads and other file sharing methods.

The draft document manager screens and flows are available in the "Proposed features" section of the email attachment spec (https://www.dropbox.com/s/5giljes7ao9gyrg/email-attachments.pdf) and in Bug 849970.

	[:fabrice] Fabrice Desré
	Comment 29 • 11 years ago

There is a pdf viewer (pdf.js).

	Rob MacDonald [:robmac]
	Comment 30 • 11 years ago

(In reply to Fabrice Desré [:fabrice] from comment #29)
> There is a pdf viewer (pdf.js).

Whoops. I should have said a "PDF app" (that can be accessed by the user from the app grid) as opposed to "PDF viewer". Thanks for clarifying!

	iliu@mozilla.com, ianliu.moz@gmail.com
	Comment 31 • 11 years ago

I have tried to use 'view' activity request via pdf app. Looks like cannot access the url which is created from blob. Please reference my patch(https://github.com/ian-liu/gaia/compare/email;Bug825318_support_for_viewing_pdf) if you want to test it.

===== Note log errror: =====
E/GeckoConsole( 2640): [JavaScript Error: "uncaught exception: NS_ERROR_DOM_BAD_URI: Access to restricted URI denied"]

We could request pdf app to support 'open' activity for the specific url.

	iliu@mozilla.com, ianliu.moz@gmail.com
	Comment 32 • 11 years ago

also attach the screen shot

	Chris Lee [:clee]
	Comment 33 • 11 years ago

(In reply to Dominic Kuo [:dkuo] from comment #23)
> Just check again and I found we shouldn't modify pdfjs from gaia level, so
> when email has pdf attachments, we should use "view" activity to view the
> pdf files.

We need to re-confirm that this is leo+ and a P1.  Product will do that asap. 

Assuming it is, I propose we keep the interaction here simple.

1) For any email with a .pdf attachment, we allow a view only option.
2) User clicks on attachment and we call the pdf.js viewer (likely inline disposition if that's the easiest path forward)
3) Once the file is closed, I propose we take one of two options:
   a) Cache that file so if the user clicks on the attachment again to view, it doesn't need to reload 
   b) purge when we close the 'view' mode for the .pdf
   *I recommend a) and we set a limit on the size of the cache before we start overwriting previously opened attachments.  
4) We do not support deletion of pdfs given it's view only and we do not want to support a separate pdf app.

Concerns with this approach?

	Andrew Sutherland [:asuth] (he/him) Assignee
	Comment 34 • 11 years ago

It sounds like by "view only" what you mean is that we know we don't provide an affordance for the user to delete the attachment or otherwise interact with the attachment outside of the e-mail app?  But otherwise we download the attachment like normal and save to DeviceStorage.  Is that correct?

In terms of a cache, the easiest way for us to handle this right now without adding new code would just be for us to delete the PDF attachments from DeviceStorage when we purge the body from our offline message store[1].  This is currently how embedded attachments get purged, although since they are stored in IndexedDB, that happens transparently.  Otherwise, the cache constitutes potentially non-trivial new development.  (Which might not be a bad idea, though.)


Note that complicating cases if we store the PDF's in DeviceStorage and purge from there are:
- our current database upgrade code would lose/leak the attachments when we simply blow away the database
- our current account deletion code is equally naive.

This wouldn't happen with IndexedDB, but the disk space usage growth factor would be a bad idea there.  We could simplify this case by potentially doing some trickery with our use of DeviceStorage so that it would be easier for us to enumerate the files and delete them just using DeviceStorage rather than having to scan all of our stored messages or maintain additional data structures.

Addressing this could be beneficial since storing images in DeviceStorage is arguably not a great idea for the same disk space growth factor.  That will still have MIME type issues to address that we would probably need to deal with.  Namely, we don't want the gallery to see our embedded images, so we could provide gibberish MIME types, but that might then complicate just creating blob URLs from.


1: Bodies are currently purged if they're no longer on the server/relevant to our ActiveSync sync, or in the case of IMAP we periodically do a sweep of a folder once there has been sufficient growth in the folder.  For IMAP, we don't purge messages that have been synchronized (implying seen in the message list by scrolling) in the last 2 weeks or that are inside the 'synchronize' window defined on the account.  So if that window is set to 2 months, a message would have to be older than 2 months before it would be purged.  The exception is that we have a hard per-folder limit on block storage so that extremely busy mail accounts don't nuke the world.

	Chris Lee [:clee]
	Comment 35 • 11 years ago

(In reply to Andrew Sutherland (:asuth) from comment #34)
> It sounds like by "view only" what you mean is that we know we don't provide
> an affordance for the user to delete the attachment or otherwise interact
> with the attachment outside of the e-mail app?  But otherwise we download
> the attachment like normal and save to DeviceStorage.  Is that correct?

Yes, this is correct. 

> 
> In terms of a cache, the easiest way for us to handle this right now without
> adding new code would just be for us to delete the PDF attachments from
> DeviceStorage when we purge the body from our offline message store[1]. 
> This is currently how embedded attachments get purged, although since they
> are stored in IndexedDB, that happens transparently.  Otherwise, the cache
> constitutes potentially non-trivial new development.  (Which might not be a
> bad idea, though.)

This sounds acceptable.  One question - what's the logic on pre-downloading any attachment when checking for new mail?  Do we load anything prior to the user interacting with a given message?

> 
> 
> Note that complicating cases if we store the PDF's in DeviceStorage and
> purge from there are:
> - our current database upgrade code would lose/leak the attachments when we
> simply blow away the database

Is the user impact here that they would just have to reload the attachment from the email?  (i.e. more time to view an attachment)

> 1: Bodies are currently purged if they're no longer on the server/relevant
> to our ActiveSync sync, or in the case of IMAP we periodically do a sweep of
> a folder once there has been sufficient growth in the folder.  For IMAP, we
> don't purge messages that have been synchronized (implying seen in the
> message list by scrolling) in the last 2 weeks or that are inside the
> 'synchronize' window defined on the account.  So if that window is set to 2
> months, a message would have to be older than 2 months before it would be
> purged.  The exception is that we have a hard per-folder limit on block
> storage so that extremely busy mail accounts don't nuke the world.

This sounds like the right logic here.

	Andrew Sutherland [:asuth] (he/him) Assignee
	Comment 36 • 11 years ago

(In reply to Chris Lee [:clee] from comment #35)
> This sounds acceptable.  One question - what's the logic on pre-downloading
> any attachment when checking for new mail?  Do we load anything prior to the
> user interacting with a given message?

We currently never pre-download attachments.  The user must explicitly choose to download an attachment.

We currently *do* automatically download message bodies as part of the synchronization process.  If you can see the message, we already have the body.  *THIS IS CHANGING*.  For e-mail bandwidth usage and synchronization speed reasons, we are, at the very explicit directive of :gal, going to stop automatically synchronizing bodies for the time being.  So when you click on a message, we will only then fetch the message body.  There will actually be 3 phases of life for a message: 1) header without snippet or body, 2) header with potentially useless snippet but no body, 3) header with proper snippet because we have the body.  We only get to 3 by the user showing the message.

There are enhancement features about choosing automatic message body download (and embedded attachment download, based on size limit) that will eventually be implemented, but those are beyond v1.1 at this point.
 

> > Note that complicating cases if we store the PDF's in DeviceStorage and
> > purge from there are:
> > - our current database upgrade code would lose/leak the attachments when we
> > simply blow away the database
> 
> Is the user impact here that they would just have to reload the attachment
> from the email?  (i.e. more time to view an attachment)

So, yes, the e-mail app would need to re-download the attachment.  But the greater concern I was pointing out was that we will end up leaving the attachment on disk, so the PDFs will just clutter up the SD card with no way to be deleted on the phone currently.  Even if we had previously downloaded a PDF and the user wants to download it again, unless we stashed meta-data on DeviceStorage, we will just see that there happens to be a file with the same filename and so download the PDF a second time.  So user downloads "document.pdf", we delete the account, forgetting about it.  User download s "document.pdf" again, but we save it as "document-TIMESTAMP.pdf" the second time because there's already a file with that name.  (We could perform limited de-duplication by manually computing hashes, or make it DeviceStorage's problem by demanding it grow that feature.)

	Joe Cheng [:jcheng] (please needinfo)
	Comment 37 • 11 years ago

(In reply to Chris Lee [:clee] from comment #33)
> (In reply to Dominic Kuo [:dkuo] from comment #23)
> > Just check again and I found we shouldn't modify pdfjs from gaia level, so
> > when email has pdf attachments, we should use "view" activity to view the
> > pdf files.
> 
> We need to re-confirm that this is leo+ and a P1.  Product will do that
> asap. 
> 

ni? product

	Jason Smith [:jsmith]
	Comment 38 • 11 years ago

Do we have coverage in MozTrap for this?

	Chris Lee [:clee]
	Comment 39 • 11 years ago

(In reply to Joe Cheng [:jcheng] from comment #37)
> (In reply to Chris Lee [:clee] from comment #33)
> > (In reply to Dominic Kuo [:dkuo] from comment #23)
> > > Just check again and I found we shouldn't modify pdfjs from gaia level, so
> > > when email has pdf attachments, we should use "view" activity to view the
> > > pdf files.
> > 
> > We need to re-confirm that this is leo+ and a P1.  Product will do that
> > asap. 
> > 
> 
> ni? product

I have checked the list of requests for Leo and this is not a P1 requirement.  Marking P2.

	Jason Smith [:jsmith]
	Comment 40 • 11 years ago

Renom per comment 39. Comment 39 implies that this a strongly wanted feature for leo, but not blocking.

	Jason Smith [:jsmith]
	Comment 41 • 11 years ago

Killing in-moztrap for now if this is going to P2 feature. If it manages to make it in time for landing, then let's reconsider.

	Alex Keybl [:akeybl]
	Comment 42 • 11 years ago

(In reply to Jason Smith [:jsmith] from comment #40)
> Renom per comment 39. Comment 39 implies that this a strongly wanted feature
> for leo, but not blocking.

Agreed, given above P2 status. We can take as an approval, if a low risk fix is ofund on the right timeline.

	Naoki Hirata :nhirata (please use needinfo instead of cc)
	Comment 43 • 11 years ago

Moztrap test case created anyhow : #6892

	Naoki Hirata :nhirata (please use needinfo instead of cc)
	Comment 44 • 11 years ago

Note the test case is set to draft at this moment.  Once this bug becomes fixed, we'll turn the test case on.

	psingapati
	Comment 45 • 11 years ago

Hello Andrew,

Would like to know this issue will fix only pdf attachments or 
will it work for txt files also?

We see theat image/Audio/Video attachments are supported now.

Please let us know about this.

Thanks.

	psingapati
	Comment 46 • 11 years ago

Hello Andrew,

Would like to know this issue will fix only pdf attachments or 
will it work for txt files also?

We see that image/Audio/Video attachments are supported now.

Please let us know about this.

Thanks.

	Andrew Sutherland [:asuth] (he/him) Assignee
	Comment 47 • 11 years ago

The changes called for in this bug only call for white-listing PDF's in https://github.com/mozilla-b2g/gaia/blob/master/shared/js/mime_mapper.js

We could white-list other MIME types too, like text/plain, although I don't know that we have an app to display them.  Are you aware of any?

	Wayne Chang [:wchang]
	Comment 49 • 11 years ago

Nom'ing this for koi? As it seems to be an expected feature.

	Dylan Oliver [:doliver]
	Comment 50 • 11 years ago

triage: feature request, not blocking v1.2

	psingapati
	Comment 51 • 11 years ago

please let us know the download manager interfaces for the email application to support all types of attachments (ex: pdf, txt etc...)

	Wayne Chang [:wchang]
	Comment 52 • 10 years ago

There's an offline mail thread on this, let's clarify there. Also most of the teams are out until next week, apologies but please allow some response delay.

	Wayne Chang [:wchang]
	Comment 53 • 10 years ago

Partner is studying the download manager work in progress and the feasibility to extend it for email attachment uses.

	psingapati
	Comment 54 • 10 years ago

The below is the analysis of download manager.
From the Gaia , Settings application shows the downloads of below types
1. Currently downloading files from the webAPI
2. Already downloaded files from the download data store (shared/js/download/download_store.js)

Whenever a file is downloaded, system application is getting the notification and it is storing the downloaded file on to data store.

From email perspective below are the 3 things that we want to support for MADAI.

1.User should be allowed to download all types of attachments from email application.

-- Currently we are checking the mime mapper to limit the download types. If we can bypass/remove the checking of the mimemapper, we can allow all types of files to download from email app
-- As per existing implementation attachments are downloaded and stored onto device storage (file system) by email app

2.User should be allowed to open the downloaded attachments from email application. In case there is no corresponding application, user should be notified.

-- Email application can use MozActivity to find corresponding application to handle downloaded content
-- If there are no applications to handle the downloaded content Email application can notify end user

3.User should be allowed to view/share/delete the downloaded attachments from non email application, in this case it will be download manager.

-- We can store the downloaded attachments from email to download data store. 
(Tested this on the device with the latest master, added code at message_reader.js)
-- settings/downloads allow user to open, share and delete files.

Please give your inputs on the above scenarios.

we also need to consider the following scenario,
-- Whenever user deletes the files from downloads, we need to enable the user to re download the file from email application.

Thanks

	psingapati
	Comment 55 • 10 years ago

Attached zip contains 
1. settings/downloads before downloading the email attachments
2. various email attachments
3. settings/downloads after downloading the email attachments.

	psingapati
	Comment 56 • 10 years ago

Hi Aus,

Please check the comment#54 and comment#55 for our work in progress.
Please give your comments on our proposal for storing the email attachments (downloaded from email) on to download store.

Thanks

	Aus Lacroix [:aus]
	Comment 57 • 10 years ago

Hi All, sorry about the wait, but here are our implementation guidelines for this feature and more.

* Conceptually we want the mozDownloadManager to manage all downloads in FxOS in some form or another in the long term.

* mozDownloadManager will accept managing completed downloads from other certified applications via an ‘adoptDownload’ method. This could be expanded later to other applications but is not necessary for the scope of this feature.

* adoptDownload will be simple for the purposes we need it at this time. It will only accepted completed downloads. This will generate a standard ‘ondownloadstart’ event which the Gaia Download Manager (and UI) use to track all downloads. This will cause the Download Manager to add it to it's downloads in the datastore and enable management from then on.

* adoptDownload will accept DOM Download objects as it’s input. The DOM Download object should be extended to allow an origin application (a manifestURL) for the download. This could be very useful later to differentiate where each download comes from in the Download Manager UI.

* The Download Manager Downloads List UI (Settings -> Downloads) should be where the user shares, and manages their downloaded attachments.

* The mozDownloadManager changes will be reviewed by Aus Lacroix (:aus on bugzilla).

* The email application should check for an sdcard before attempting to download attachments. James Burke (:jrburke on bugzilla) will be available for code reviews for this change.

If you have any technical questions along the way or partial patches you want me to look at, attach them to this bug and flag me for needinfo? for tech questions and feedback? for partial patches and works in progress.

	Sharaf
	Comment 59 • 10 years ago

Hi Aus,

I am trying to implement this new function proposal from you.
I have some queries regarding this proposal, request your comment on those.

(In reply to Ghislain Aus Lacroix [:aus] from comment #57)
> Hi All, sorry about the wait, but here are our implementation guidelines for
> this feature and more.
> 
> * Conceptually we want the mozDownloadManager to manage all downloads in
> FxOS in some form or another in the long term.
>

Fine
 
> * mozDownloadManager will accept managing completed downloads from other
> certified applications via an ‘adoptDownload’ method. This could be expanded
> later to other applications but is not necessary for the scope of this
> feature.
> 

Fine

> * adoptDownload will be simple for the purposes we need it at this time. It
> will only accepted completed downloads. This will generate a standard
> ‘ondownloadstart’ event which the Gaia Download Manager (and UI) use to
> track all downloads. This will cause the Download Manager to add it to it's
> downloads in the datastore and enable management from then on.
>

Does adopDownload need to return anything? 
Shall we return updated DOMDownload object with Id and status? It may be used 
by apps later for tracking. Or any other info need to be returned.
Please give us your opinion.

> * adoptDownload will accept DOM Download objects as it’s input. The DOM
> Download object should be extended to allow an origin application (a
> manifestURL) for the download. This could be very useful later to
> differentiate where each download comes from in the Download Manager UI.
> 

As of now DOMDownload interface dont have a constructor. So we cant create Objects of it.
Shall we add constructor property to interface? Any suggestion in here?

Also we feel that the app should initialize the DOMDownload object with url, totalBytes, path, contentType and app origin properties. Please comment.


> * The Download Manager Downloads List UI (Settings -> Downloads) should be
> where the user shares, and manages their downloaded attachments.

Fine

ni? to Aus for his comments.

	Sharaf
	Comment 60 • 10 years ago

Work in progress patch based on Comment #57 and Comment #59.

Two major deviation from Comment #57 are

* DOMDownload constructor support
* adoptDownload returns DOMDownload with Id

Please comment :-)

	Aus Lacroix [:aus]
	Comment 61 • 10 years ago

(In reply to Sharaf from comment #59)
> > * adoptDownload will be simple for the purposes we need it at this time. It
> > will only accepted completed downloads. This will generate a standard
> > ‘ondownloadstart’ event which the Gaia Download Manager (and UI) use to
> > track all downloads. This will cause the Download Manager to add it to it's
> > downloads in the datastore and enable management from then on.
> >
> 
> Does adopDownload need to return anything? 
> Shall we return updated DOMDownload object with Id and status? It may be
> used 
> by apps later for tracking. Or any other info need to be returned.
> Please give us your opinion.

Yes, adoptDownload should return a DOM Download object with ID and status (set to succeeded). adoptDownload should only adopt downloads that are completed. This should be noted in the documentation in the webidl file.

> 
> > * adoptDownload will accept DOM Download objects as it’s input. The DOM
> > Download object should be extended to allow an origin application (a
> > manifestURL) for the download. This could be very useful later to
> > differentiate where each download comes from in the Download Manager UI.
> > 
> 
> As of now DOMDownload interface dont have a constructor. So we cant create
> Objects of it.
> Shall we add constructor property to interface? Any suggestion in here?
> 
> Also we feel that the app should initialize the DOMDownload object with url,
> totalBytes, path, contentType and app origin properties. Please comment.

There are two options, we can add a contructor to DOM Download, or accept mock DOM download objects which provide the required properties. Something like this:

var mockDownload = {
  totalBytes: 1024,
  currentBytes: 1024,
  url: http://path/to/download/origin.bin,
  path: /path/to/downloaded/origin.bin,
  contentType: "application/some-content",
  startTime: (TIMESTAMP),
}

All other fields aren't necessary. Internally, the downloads api can create dom download objects already, without a constructor. This would enable simpler use of this api call.

> 
> > * The Download Manager Downloads List UI (Settings -> Downloads) should be
> > where the user shares, and manages their downloaded attachments.
> 
> Fine

	Aus Lacroix [:aus]
	Comment 62 • 10 years ago

Comment on attachment 8390448 [details] [diff] [review]
WIP Patch for 'adoptDownload' Web API

I like where this is going. Good work! Definitely what we're looking for.

	Andrew Sutherland [:asuth] (he/him) Assignee
	Comment 63 • 10 years ago

(my feedback and Adam's feedback were integrated into the discussion that led to the strategy in comment 57)

	Sharaf
	Comment 64 • 10 years ago

This is the proposed patch for 'adoptDownload' implementation based on feedbacks on Comment #61, with the below modifications

* If startTime is not provided by application, current time is set.
* If currentBytes is not provided by application its initialized to totalBytes.
* Application will set applicationOrigin property.

Also please note that due to webIDL code generation error had to use 'optional' for the adoptDownload argument in Web IDL.

	iliu@mozilla.com, ianliu.moz@gmail.com
	Comment 65 • 10 years ago

Leave assigned since I'm working on other bug for a long time.

	Aus Lacroix [:aus]
	Comment 66 • 10 years ago

Comment on attachment 8394742 [details] [diff] [review]
Proposed patch for 'adoptDownload' web API

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

r-, but! we are *really* close! After you address the review comments (and don't forget, add a test for this new method!) -- I will push this to try and see how it does.

Here's an example of a simple Downloads API test: https://mxr.mozilla.org/mozilla-central/source/dom/downloads/tests/test_downloads_bad_file.html?force=1

::: dom/downloads/src/DownloadsIPC.jsm
@@ +135,5 @@
>          if (changed) {
>            this.notifyChanges(download.id);
>          }
>          break;
> +      case "Downloads:Adopt:Return":

You must add this message to the list of valid messages.

See https://mxr.mozilla.org/mozilla-central/source/dom/downloads/src/DownloadsIPC.jsm#35

@@ +198,5 @@
> +    return res;
> +  },
> +
> +  adoptDownload: function(aDownload) {
> +    debug("adoptDownload ");

Nit: Extra space before closing "

@@ +203,5 @@
> +    let deferred = Promise.defer();
> +    let pId = this.promiseId();
> +    this.downloadPromises[pId] = deferred;
> +    let data = JSON.stringify(this._creatAdoptedDownloadJson(aDownload));
> +    debug("JSON: "+data);

Nit: Missing space after +

::: dom/webidl/Downloads.webidl
@@ +92,5 @@
>    // - when the transfer progresses, updating currentBytes.
>    // - when the state and/or error attributes change.
>    attribute EventHandler onstatechange;
> +
> +  // Owner application of this download

We should note here that this will only be available for adopted downloads and mark it optional (can be null) using the applicationOrigin?; notation.

	Wayne Chang [:wchang]
	Comment 67 • 10 years ago

ni Sharaf just in case.

Sharaf, since your patches is heading the right direction, can you assign this bug to yourself for tracking purposes?
thanks.

	Wayne Chang [:wchang]
	Comment 68 • 10 years ago

Moving bug 825849 from depends on to see also.

Since this bug 825318 is addressing the 'download' part via download manager, it shouldn't be blocked by pdfjs not being able to open the downloaded file. So removing dependency.

	Marek Raida
	Comment 69 • 10 years ago

IMHO similar case is for large images, those who cannot be viewed cannot be downloaded as well => frustrating a bit...

	jongsoo.oh
	Comment 70 • 10 years ago

sharaf could not follow up this issue. Other engineer is needed to solve this issue.

	Andrew Sutherland [:asuth] (he/him) Assignee
	Comment 76 • 10 years ago

Going forward, when implementing this, we also want this change to go along with the originally discussed download/document manager changes discussed in-person in Oslo where we change to cache email downloads in IndexedDB and only transfer them to DeviceStorage when the user explicitly chooses to save them.

	Ralph Daub [:rdaub]
	Comment 77 • 10 years ago

For tracking purposes -
A user in the SUMO forums posted a question about downloading PDF and DOCX filetypes: https://support.mozilla.org/en-US/questions/1028390

Thanks,
- Ralph

	David Palomino [:dpalomino]
	Comment 78 • 10 years ago

Hi, 

Just to remind that this feature is critical for most of our partners, it'd be important to include it in 2.2. 

Thanks, 
David

	Dylan Oliver [:doliver]
	Comment 79 • 10 years ago

David, the 2.2 release was reduced in time/scope and this item is no longer on the list of features we are targeting.

	Andrew Sutherland [:asuth] (he/him) Assignee
	Comment 80 • 10 years ago

Given the altered v2.2 timeline and risk/waste factors, I don't think want to do the bug 1017924 changes for v2.2.  However it could make sense to just finish out the patch here on this bug so that we can:
a) Download everything
b) Have the download manager "adopt" either things that we did not previously download (leave it to gallery/videos/music to manage those files), or everything (including images/videos/music).  The download manager doesn't seem to have the ability to "forget" files, just delete them.  This seems appropriate for files that can't be managed any other way, but a bit of a hassle for images I might want to keep around.  So I'd lean towards leaving it to gallery/videos/music to manage those type of files.  Although that's definitely a consistency/UX call.

	Dylan Oliver [:doliver]
	Comment 82 • 10 years ago

[Tracking Requested - why for this release]: see context in comment #80, nominating for tracking to see if this is feasible to fix in 2.2.

	David Palomino [:dpalomino]
	Comment 83 • 9 years ago

Hi, 

Thanks Dylan. This is a feature requested from the very beginning by most of partners, and I think important from the user perspective. It'd be great if we can at least support that for 2.2. 

Thks, 
David

	Wilfred Mathanaraj [:WDM]
	Comment 84 • 9 years ago

stephany  -are you still tracking this as part of 2.2 landing?

	Stephany Wilkes
	Comment 85 • 9 years ago

The comments on this came in while I was on PTO. Flagging Tif on UX, though I believe we (Tif, Hema and I) all agree that, per Andrew's comment #80, having delete only is fine and having media apps handle things is fine, provided they can do so already today and that this does not create additional work. 

Also, last I checked, this was on the 2.2 spreadsheet, so it's only a matter of whether or not we can finish it in time.

	Tiffanie Shakespeare [:tif]
	Comment 86 • 9 years ago

+1 thanks!

	Stephany Wilkes
	Comment 87 • 9 years ago

Thanks, Tif. Andrew and I discussed this in IRC as well, as follows:

Our download policy is that we only allow downloads for which we have built-in apps that can handle the file type, which for our purposes means 1) it can both view/play the file and 2) it has a UI to subsequently download the file. 

The current mapping lives at https://github.com/mozilla-b2g/gaia/blob/master/shared/js/mime_mapper.js

This bug is about us downloading everything because we can make the download manager responsible for the task of being able to delete the files after they are downloaded, and otherwise track them. There is still the issue that a user might download something we don't actually have an app to view it with (which would violate our stated policy), so the primary enhancement was to make the download manager be able to "adopt" files that the email app downloaded. (The download manager can't download the files for us because it doesn't speak the mail server protocols we use.)

This bug already has a patch, so if we finish that small set of changes, that works, hurray. The only other stuff we need to add then is an error screen that indicates we couldn't find an activity to view a given file. The download manager already has an error string for this, so we can copy that (probably easiest and fastest for 2.2). 

Assigning to Andrew who can assign it to mcav later if need be. Let's do this.

	Ralph Daub [:rdaub]
	Comment 88 • 9 years ago

For tracking purposes -
A user in the SUMO forums posted a question about downloading PDF from the Email application: https://support.mozilla.org/en-US/questions/1041691

Thanks,
- Ralph

	Andrew Sutherland [:asuth] (he/him) Assignee
	Comment 91 • 9 years ago

Here's a cleaned up and tested version of the patch.  I spent a lot of time trying to get a mochitest for all of this, but ran into the following problems that I was unable to really get around:
- All of the mochitests are currently disabled.  (Bug 979446)
- nsIVolumeService is only available on gonk-platforms, the downloads API requires it, and there's no sane way to implement a fallback by directly interrogating device storage.  Specifically, device storage has platform-specific directory-service logic and a bunch of custom preferences that control what the 'sdcard' directory is.  Logic would effectively have to be duplicated.
- Given the gonk/nsIVolumeService deps, the only way to run the (disabled) tests is to create an emulator build.  Which it turns out you can't really built on a 64-bit linux box.  Which is about the point I gave up.

I also spent a fair bit of time getting the mochitest to be able to run the app in a certified app context so that we could check the [AvailableIn=CertifiedApps] guard on adoptDownload and so that we could verify that we were correctly interrogating/saving the app manifest of the triggering app.  That all worked (under mulet) up to the nsIVolumeService problem.

I did also consider implementing a JS marionette test, but given how flakey those tests have still been recently (I believe they're still hidden on treeherder), and the number of moving parts crossing apps, etc., I thought the tests were more likely to do harm than good.  (In terms of having intermittent failures and needing to be updated very frequently as the mozDownloads API changes, system app, settings app, etc. change.)

I will attach the test stuff as a separate diff for historical consideration shortly.


Here are the debug traces of using the email app with all of the attached patches and debug forced on in the download API code:

=====
02-05 13:24:51.073 I/GeckoDump( 3983): WAR: have downloadManager? true have adoptDownload? true
02-05 13:24:51.073 I/GeckoDump( 3983): LOG: adopting download sdcard /sdcard/f1040.pdf
02-05 13:24:51.083 I/Gecko   ( 3983): -*- DownloadsAPI.js : adoptDownload
02-05 13:24:51.083 I/Gecko   ( 3983): -*- DownloadsIPC.jsm : adoptDownload
02-05 13:24:51.083 I/Gecko   (28485): -*- DownloadsAPI.jsm : message: Downloads:Adopt
02-05 13:24:51.083 I/Gecko   (28485): -*- DownloadsAPI.jsm : adoptDownload ({totalBytes:160607, currentBytes:160607, url:"", path:"/storage/sdcard/f1040.pdf", contentType:"application/pdf", startTime:1423160691091, sourceAppManifestURL:"app://email.gaiamobile.org/manifest.webapp"})
02-05 13:24:51.093 I/Gecko   (28485): -*- DownloadsAPI.jsm : onDownloadAdded ({totalBytes:160607, currentBytes:0, url:"", path:"/storage/sdcard/f1040.pdf", contentType:"application/pdf", startTime:1423160691091, sourceAppManifestURL:"app://email.gaiamobile.org/manifest.webapp", id:"download-8", state:"succeeded"})
02-05 13:24:51.093 I/Gecko   ( 3983): -*- DownloadsIPC.jsm : message: Downloads:Added
02-05 13:24:51.093 I/Gecko   (28485): -*- DownloadsAPI.jsm : download adopted
02-05 13:24:51.093 I/Gecko   (28485): -*- DownloadsAPI.jsm : sendPromiseMessage Downloads:Adopt:Return
02-05 13:24:51.093 I/Gecko   ( 3983): -*- DownloadsIPC.jsm : notifyChanges notifying changes for download-8
02-05 13:24:51.093 I/Gecko   ( 3983): -*- DownloadsAPI.js : Adding ({totalBytes:160607, currentBytes:0, url:"", path:"/storage/sdcard/f1040.pdf", contentType:"application/pdf", startTime:1423160691091, sourceAppManifestURL:"app://email.gaiamobile.org/manifest.webapp", id:"download-8", state:"succeeded"})
02-05 13:24:51.093 I/Gecko   ( 3983): -*- DownloadsAPI.js : Adding download download-8 to cache.
02-05 13:24:51.093 I/Gecko   (28485): -*- DownloadsIPC.jsm : message: Downloads:Added
02-05 13:24:51.093 I/Gecko   ( 3983): -*- DownloadsAPI.js : DOMDownloadImpl constructor 
02-05 13:24:51.093 I/Gecko   (28485): -*- DownloadsIPC.jsm : notifyChanges notifying changes for download-8
02-05 13:24:51.093 I/Gecko   (28485): -*- DownloadsAPI.js : Adding ({totalBytes:160607, currentBytes:0, url:"", path:"/storage/sdcard/f1040.pdf", contentType:"application/pdf", startTime:1423160691091, sourceAppManifestURL:"app://email.gaiamobile.org/manifest.webapp", id:"download-8", state:"succeeded"})
02-05 13:24:51.093 I/Gecko   ( 3983): -*- DownloadsAPI.js : update ({totalBytes:160607, currentBytes:0, url:"", path:"/storage/sdcard/f1040.pdf", contentType:"application/pdf", startTime:1423160691091, sourceAppManifestURL:"app://email.gaiamobile.org/manifest.webapp", id:"download-8", state:"succeeded"})
02-05 13:24:51.103 I/Gecko   (28485): -*- DownloadsAPI.js : Adding download download-8 to cache.
02-05 13:24:51.103 I/Gecko   (28485): -*- DownloadsAPI.js : DOMDownloadImpl constructor 
02-05 13:24:51.103 I/Gecko   (28485): -*- DownloadsAPI.js : update ({totalBytes:160607, currentBytes:0, url:"", path:"/storage/sdcard/f1040.pdf", contentType:"application/pdf", startTime:1423160691091, sourceAppManifestURL:"app://email.gaiamobile.org/manifest.webapp", id:"download-8", state:"succeeded"})
02-05 13:24:51.103 I/Gecko   (28485): -*- DownloadsAPI.js : observer set for download-8
02-05 13:24:51.113 I/Gecko   ( 3983): -*- DownloadsAPI.js : observer set for download-8
02-05 13:24:51.113 I/Gecko   ( 3983): -*- DownloadsIPC.jsm : message: Downloads:Adopt:Return
02-05 13:24:51.113 I/GeckoDump( 3983): LOG: registered download with download manager
02-05 13:24:51.113 I/Gecko   ( 3983): WLOG: saved attachment to sdcard /sdcard/f1040.pdf type: application/pdf registered: true
02-05 13:24:51.133 I/Gecko   (28897): -*- DownloadsIPC.jsm : message: Downloads:Added
02-05 13:24:51.143 I/Gecko   (28897): -*- DownloadsIPC.jsm : notifyChanges notifying changes for download-8
02-05 13:24:51.143 I/Gecko   (28897): -*- DownloadsAPI.js : Adding ({totalBytes:160607, currentBytes:0, url:"", path:"/storage/sdcard/f1040.pdf", contentType:"application/pdf", startTime:1423160691091, sourceAppManifestURL:"app://email.gaiamobile.org/manifest.webapp", id:"download-8", state:"succeeded"})
02-05 13:24:51.143 I/Gecko   (28897): -*- DownloadsAPI.js : Adding download download-8 to cache.
02-05 13:24:51.143 I/Gecko   (28897): -*- DownloadsAPI.js : DOMDownloadImpl constructor 
02-05 13:24:51.143 I/Gecko   (28897): -*- DownloadsAPI.js : update ({totalBytes:160607, currentBytes:0, url:"", path:"/storage/sdcard/f1040.pdf", contentType:"application/pdf", startTime:1423160691091, sourceAppManifestURL:"app://email.gaiamobile.org/manifest.webapp", id:"download-8", state:"succeeded"})
02-05 13:24:51.183 I/Gecko   (28897): -*- DownloadsAPI.js : observer set for download-8
=====

	Andrew Sutherland [:asuth] (he/him) Assignee
	Comment 92 • 9 years ago

This is how far the tests got before I ran into the nsIVolumeService problem and gave up.  All of the stuff about installing the app as a certified app and the preliminary checks work.  Assuming the nsIVolumeService problem could be addressed, I think this could pretty easily be made to worm under mulet.

I can spin these off to a follow-up bug if desired.

	Andrew Sutherland [:asuth] (he/him) Assignee
	Comment 93 • 9 years ago

Comment on attachment 8559955 [details] [review]
[PullReq] GAIA asutherland:email-download-everything to mozilla-b2g:master

== download_notification.js: :crdlc/arthurcc

Currently download_notification.js assumes that all downloads it hears about are in the 'started' state.  This is not correct with the changes to adoptDownload.

I'm not quite sure who owns this code; the file has a lot of reviews by :crdlc and arthurcc, so I'm asking you both.  Probably only one of you needs to review?  Please redirect as appropriate as needed.

I tested on a Flame device with all of these patches applied, and the right thing happens.  Namely:
- The notification shows a completion notification.
- The download gets properly persisted to the data store so that I can see it in the settings UI's "downloads" UI and in the secret app that provides "pick" support.

I've isolated the change into this commit https://github.com/asutherland/gaia/commit/95b4e55fb3a4d064c18fb5c42773c856884ecfad in the pull request.

=== email stuff: :jrburke

The changes are mainly using the new affordance provided by the back-end to register the download.  Also the error UI if we can't trigger an open activity correctly.  I've kept the actual gaia email changes in the 2nd commit and the gaia-email-libs-and-more changes in the 3rd commit.

	Andrew Sutherland [:asuth] (he/him) Assignee
	Comment 94 • 9 years ago

Comment on attachment 8559952 [details] [review]
[PullReq] GELAM asutherland:adopt-downloads to mozilla-b2g:master

This is the gelam bits.  Comments in the commits.  I think James' review should suffice here, but requesting feedback from :mcav so he has a chance to weigh in.  Note that I'm not crazy about all the parallel list stuff going on under the hood to propagate things, but downloads are something I'm also expecting for us to overhaul for v3.0, so I didn't spend a lot of time worrying about that.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 95 • 9 years ago

(In reply to Andrew Sutherland [:asuth] from comment #91)
> - nsIVolumeService is only available on gonk-platforms, the downloads API
> requires it, and there's no sane way to implement a fallback by directly
> interrogating device storage.  Specifically, device storage has
> platform-specific directory-service logic and a bunch of custom preferences
> that control what the 'sdcard' directory is.  Logic would effectively have
> to be duplicated.

Can you file a followup bug on this. And cc :gerard-majax and dhylands. This should be fixable.

	James Burke [:jrburke]
	Comment 96 • 9 years ago

Comment on attachment 8559955 [details] [review]
[PullReq] GAIA asutherland:email-download-everything to mozilla-b2g:master

r+ for the gaia parts. Had a possible nit on the specific text for an l10n string, but that was it.

I tested on flame device, but without the gecko changes for adoptDownload. So I did not test that specific piece, but verified the "no app for this mime type" case with a .txt file, and that I could share a .vcf from Contacts to email, send the email to myself, then download and view the .vcf file in the Contacts app.

For the gelam parts, had a question around a possible change for pop3.js, will use the pull request to make notes and wait for conversation in that pull request to reach an end point before flipping the review flag. Otherwise looking good.

	Arthur Chen [:arthurcc][inactive after 6/30]
	Comment 97 • 9 years ago

Comment on attachment 8559955 [details] [review]
[PullReq] GAIA asutherland:email-download-everything to mozilla-b2g:master

I've reviewed the download manager code in settings app but not download_notificaton.js. I guess :crdlc should be able to review it.

	Andrew Sutherland [:asuth] (he/him) Assignee
	Comment 98 • 9 years ago

(In reply to Jonas Sicking (:sicking) from comment #95)
> (In reply to Andrew Sutherland [:asuth] from comment #91)
> > - nsIVolumeService is only available on gonk-platforms, the downloads API
> > requires it, and there's no sane way to implement a fallback by directly
> > interrogating device storage.  Specifically, device storage has
> > platform-specific directory-service logic and a bunch of custom preferences
> > that control what the 'sdcard' directory is.  Logic would effectively have
> > to be duplicated.
> 
> Can you file a followup bug on this. And cc :gerard-majax and dhylands. This
> should be fixable.

Bug 1130264 has been filed to this end.  (And I cc'ed them, but not you, please cc yourself if you don't watch Firefox OS :: General and you care about the bug.  Although it might get moved someplace better.)

	Andrew Sutherland [:asuth] (he/him) Assignee
	Comment 99 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=803c573a085d

	Cristian Rodriguez (:crdlc)
	Comment 100 • 9 years ago

Comment on attachment 8559955 [details] [review]
[PullReq] GAIA asutherland:email-download-everything to mozilla-b2g:master

LGTM the part related to downloads, thanks

	James Burke [:jrburke]
	Comment 101 • 9 years ago

Comment on attachment 8559952 [details] [review]
[PullReq] GELAM asutherland:adopt-downloads to mozilla-b2g:master

Changes since last review look good, as well as the gaia change, to always register the downloads with the download manager in gaia.

	Aus Lacroix [:aus]
	Comment 102 • 9 years ago

Comment on attachment 8559960 [details] [diff] [review]
adopt-download-impl-v2.diff

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

Looking pretty great! One thing I would like to see in addition to the comments in the review is a test for this new method. We have mochitests for the Downloads API so it should be straightforward to add one for this. I'm giving this a feedback+ but would like to see the final patch before it's committed.

::: dom/downloads/DownloadsAPI.js
@@ +25,5 @@
> +  * The content process implementations of navigator.mozDownloadManager and its
> +  * DOMDownload download objects.  Uses DownloadsIPC.jsm to communicate with
> +  * DownloadsAPI.jsm in the parent process.
> +  */
> +

Nice addition :)

@@ +164,5 @@
> +        aReject("InvalidDownload");
> +        return;
> +      }
> +      let computedPath = volume.mountPoint + '/' +
> +                           aAdoptDownloadDict.storagePath;

Nit: It would be best to check that the file exists before continuing.

@@ +180,5 @@
> +
> +      DownloadsIPC.adoptDownload(jsonDownload).then(
> +        function(aResult) {
> +          let dom = createDOMDownloadObject(this._window, aResult.id);
> +          aResolve(this._prepareForContent(dom));

Nit: Not a huge fan of this variable name. How about domDownload instead?

@@ +368,5 @@
> +  /**
> +    * Initialize a DOMDownload instance for the given window using the
> +    * 'jsonDownload' serialized format of the download encoded by
> +    * DownloadsAPI.jsm.
> +    */

Also a nice addition :)

::: dom/downloads/DownloadsAPI.jsm
@@ +23,5 @@
> +  * DownloadAPI.js instances in content processeses.  The actual work of managing
> +  * downloads is done by Toolkit's Downloads.jsm.  This module is loaded by B2G's
> +  * shell.js
> +  */
> +

\o/

@@ +298,5 @@
> +          path: adoptJsonRep.path,
> +        },
> +        startTime: adoptJsonRep.startTime,
> +        // kPlainSerializableDownloadProperties propagations
> +        succeeded: true, // (all adopted downloads are required to be completed)

If that's the case, let's make sure we check that the file exists higher up in the chain.

::: dom/webidl/Downloads.webidl
@@ +53,5 @@
> +  // widened at a later time.
> +  //
> +  // Note that "download" is not actually optional, but WebIDL requires that it
> +  // be marked as such because it is not followed by a required argument.  The
> +  // promise will be rejected if the dictionary is omitted.

Let's indicate here that non-existing files will be rejected as well.

	Andrew Sutherland [:asuth] (he/him) Assignee
	Comment 103 • 9 years ago

Comment on attachment 8559960 [details] [diff] [review]
adopt-download-impl-v2.diff

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

I've cleaned up my previous test work and folded it into the patch along with some other mozDownloadManager test cleanup.  The is somewhat overkill (it does a dance to create an installed certified app, mainly so we can have a more realistic permission checking and so we can check the origin, which I accidentally forgot to do, fix coming soon.)  A try push is up at https://hg.mozilla.org/try/rev/e23abb4be1ce (treeherder: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e23abb4be1ce).

Notes about that:
- There's a lot of debug in there that I will take out for the final review patch, but I want the debug so we can help turn these tests back on (disabled in bug 979446).
- I commented out the mochitest.ini guards, which is too much.  The goal was to get the tests to run for gonk/b2g, but they will fail on mulet without nsIVolumesService (which we have the spin-off bug about).  I'll pull the mulet guard back in if it looks like I've made the tests more stable.
- I did forget to check the origin logic, although it works.
- I fixed at least 2 of the intermittent test problems in the mozDownloadManager tests, I hope.  I'll probably do some retriggering to get extra intermittent checks out of this run with all the debug cranked up.

Hopefully final review patch up in a few hours with those changes made and an additional on-device manual-testing-by-me pass done.

::: dom/downloads/DownloadsAPI.js
@@ +164,5 @@
> +        aReject("InvalidDownload");
> +        return;
> +      }
> +      let computedPath = volume.mountPoint + '/' +
> +                           aAdoptDownloadDict.storagePath;

Since it's my understanding that all file I/O is initiated by the parent process (although I think fd's can be sent down), I've added logic in DownloadsAPI.jsm to perform this check and added a comment here that DownloadsAPI.jsm is responsible for the check.

Since we're now touching the file-system, I've also removed the totalBytes field from the AdoptDownloadDict in favor of what OS.File.stat tells us.  (The use of OS.File.stat over OS.File.exists also lets us unensure that the file isn't a directory.)

@@ +180,5 @@
> +
> +      DownloadsIPC.adoptDownload(jsonDownload).then(
> +        function(aResult) {
> +          let dom = createDOMDownloadObject(this._window, aResult.id);
> +          aResolve(this._prepareForContent(dom));

This is a copied and pasted idiom, but I agree :).  Now domDownload in the revised patch.

	Andrew Sutherland [:asuth] (he/him) Assignee
	Comment 104 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=75f38568e465

	Andrew Sutherland [:asuth] (he/him) Assignee
	Comment 105 • 9 years ago

I ran into test problems where it turned out OS.File.* could potentially hang on certain requests.  This is bug 1125989 that I have a fix up for review for.  Note that the try push in comment 104 includes that fix in the stack.  The patch I'm about to put up absolutely needs that fix or the emulator builds will experience failure with a high probability.

	Andrew Sutherland [:asuth] (he/him) Assignee
	Comment 106 • 9 years ago

I believe this fix addresses your functionality requests (most specifically the OS.File.stat check is added, which led to those additional horrible testing horrors) and the test request.  It also fixes the most obvious intermittent problems with the existing dom/downloads tests, re-enabling them.

Note that as alluded to previously, the test infrastructure required to run our adoptDownload test in an app context is not entirely trivial, but it's also not all that complex either.  I've tried to keep the code clean and fairly well documented.  But to provide a quick summary:

- We have a helper shim_app_as_test.js that you point at a manifest and a test file to run as an app.

- It uses SpecialPower's magic powers to run a chrome helper in the parent process, shim_app_as_test_chrome.js.  That chrome helper installs the app, bypassing the protections against installing certified apps.  That chrome helper then also runs the app because the mochitest is running OOP and bug 1097479 means that our mozbrowser/mozapp thing won't actually work when we try and do that from inside the OOP process.  So the chrome helper creates a sibling-to-the-mochitest-iframe in the content process that runs our app.

- The app uses the mozbrowser hack that uses alert() to perform messaging from the app to the mochitest.  This is based on the existing code in dom/requestsync/tests/*.  We also do some extra marshalling because of the sibling iframe stuff.

I recognize there is some potential maintenance burden to the code; I do volunteer to be on the hook to keep this up-to-date for the specific use-case.  (And I'm not just saying stuff; the gaia email app's back-end runner operates very similarly and so breakage in one would be breakage in the other.)

	Andrew Sutherland [:asuth] (he/him) Assignee
	Comment 107 • 9 years ago

James, I've pushed two additional gaia email changes to the pull request that you probably want to look at, though I don't think it will change your review state.  (Hopefully! :)  Both are to deal with problems I noticed after performing more on-device testing:

- Our MIME type logic was restricting us to MIME types it knew about.  Simple logic change here, but with a fairly extensive comment block.

- The PDF viewer app was expecting us to pass a "url" in the activity just like the settings download UI passes when using the shared/js/download_helper.js logic.  The PDF viewer just uses the URL for filename purposes and breaks rather horribly without it.  download_helper.js uses the download's "path" as the URL which is somewhat nonsensical (it's the absolute local filesystem path which is meaningless to anything but chrome-privileged code) but works for this purpose.

	Andrew Sutherland [:asuth] (he/him) Assignee
	Comment 108 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=dcab656bd414

	Andrew Sutherland [:asuth] (he/him) Assignee
	Comment 109 • 9 years ago

v3 had a screw-up with the mochitest.ini skip-if logic.  We were trying to run the tests on b2g-desktop too, which was not what I intended.  This changes us to use:
run-if = toolkit == 'gonk'

It's perhaps important to note that the non-adopt tests did pass on b2g-desktop, but I believe this to be dumb luck and could indeed be a source of intermittent failures on its own.  Specifically, DownloadsAPI.js's DOMDownloadImpl.prototype._update method will throw an exception and die when it gets to processing changedProps["path"].  (At least, as long as navigator.getDeviceStorages("sdcard") returns any storages, volumeService will be accessed and a property lookup against the null value will occur and it will throw, never having updated storageName or storagePath)  The reason anything would work at all is that on subsequent calls to _update, path should remain the same and so the exception won't throw.  (And no test cases except the newly added adoption test look at storageName/storagePath.)

I therefore make the argument that it's misleading/a bad idea to run tests against a known-broken implementation and that only running on gonk until the spin-off bug 1130264 is fixed is reasonable and right and fair and just and a tremendously all-around good idea.

	James Burke [:jrburke]
	Comment 110 • 9 years ago

The two extra additions look good on inspection, the comments make it fairly self explanatory, so still r+ from me.

	Aus Lacroix [:aus]
	Comment 111 • 9 years ago

Comment on attachment 8567539 [details] [diff] [review]
adoptDownload implementation with tests and re-enabling existing mozDownloadManager tests v4

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

Fantastic! r=me

	Andrew Sutherland [:asuth] (he/him) Assignee
	Comment 112 • 9 years ago

(I am about to land the gecko fix on b2g-inbound; this should not be fixed until the gaia changes are also landed.  Although the email app feature-detects, the behavioural changes are such that it makes sense to not land the gaia changes until the gecko changes have definitely hit mozilla-central.)

	Andrew Sutherland [:asuth] (he/him) Assignee
	Comment 113 • 9 years ago

Comment on attachment 8567539 [details] [diff] [review]
adoptDownload implementation with tests and re-enabling existing mozDownloadManager tests v4

Jonas, the landing hook does not want to let me land without a DOM peer signing off on the patch because of the changes to a WebIDL file.  Since you have previously commented on this bug, you seem like a great candidate for this.  Please note that while I have helpfully updated comments, the only substantive change is the introduction of the certified-only adoptDownload API (and its dictionary type).

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 114 • 9 years ago

Comment on attachment 8567539 [details] [diff] [review]
adoptDownload implementation with tests and re-enabling existing mozDownloadManager tests v4

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

r=me on the webidl changes

	Andrew Sutherland [:asuth] (he/him) Assignee
	Comment 115 • 9 years ago

https://hg.mozilla.org/integration/b2g-inbound/rev/4d0ef24554ca

	Ryan VanderMeulen [:RyanVM]
	Comment 116 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/4d0ef24554ca

	Andrew Sutherland [:asuth] (he/him) Assignee
	Comment 117 • 9 years ago

Landing the gaia and GELAM parts since the adoptDownload platform portion stuck.  Autolander will resolve this as fixed.

	GitHub Autolander Bot
	Comment 118 • 9 years ago

Pull request has landed in master: https://github.com/mozilla-b2g/gaia-email-libs-and-more/commit/e593fbe95cec668462e0a77a3e1c343a95c525f4

	GitHub Autolander Bot
	Comment 119 • 9 years ago

Pull request has landed in master: https://github.com/mozilla-b2g/gaia/commit/a9d1ab8011720c350d2866ce4d82db67c163420a

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 120 • 9 years ago

I had to revert this in https://github.com/mozilla-b2g/gaia/commit/e61b815b3ea17daeb45d50fbbc8f7e351257d7ea for Gu orange on b2g-inbound:

https://treeherder.mozilla.org/logviewer.html#?job_id=1421888&repo=b2g-inbound

	Andrew Sutherland [:asuth] (he/him) Assignee
	Comment 122 • 9 years ago

Comment on attachment 8570806 [details] [review]
[gaia] asutherland:email-download-everything > mozilla-b2g:master

Very sorry about that.  I was assuming autolander had magical powers that it doesn't yet have and was negligent in checking try results prior to landing.  I am also now up-to-speed on the existence of our new accessibility unit tests for email and have corrected the deficiency in the use of the mock header by the test.

Carrying forward r+ and will manually re-land after a clean try.

	Kevin Grandon :kgrandon
	Comment 123 • 9 years ago

(In reply to Andrew Sutherland [:asuth] from comment #122)
> Comment on attachment 8570806 [details] [review]
> [gaia] asutherland:email-download-everything > mozilla-b2g:master
> 
> Very sorry about that.  I was assuming autolander had magical powers that it
> doesn't yet have

Sorry about that. We're currently working on that in bug 1130549 and it should be fire-and-forget very soon.

	Andrew Sutherland [:asuth] (he/him) Assignee
	Comment 124 • 9 years ago

green try on the fixed test, re-landed:
https://github.com/mozilla-b2g/gaia/pull/28531
https://github.com/mozilla-b2g/gaia/commit/455413a28df8d8b54543e2553268204d301ef773

	Shine(Leave from Mozilla)
	Comment 126 • 9 years ago

Per comment 4 and comment 11, this problem is verified pass on latest build of Flame 3.0. Now the result is: There's no option to download pdf/txt/doc/xls files and can't open them by tapping the attachment icon.
See attachment: Flame3.0_screenshot.png
Rate:0/3

Flame 3.0 build:
Build ID               20150302010223
Gaia Revision          f34ce82a840ad3c0aed3bfff18517b3f6a0eb37f
Gaia Date              2015-02-27 15:48:31
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/eea6188b9b05
Gecko Version          39.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150302.043726
Firmware Date          Mon Mar  2 04:37:37 EST 2015
Bootloader             L1TC000118D0

	Noemí Freire (:noemi)
	Comment 127 • 9 years ago

Hi,

Just gathering all the scenarios checked in master after landing this patch:
*pdf: the attachment can be downloaded from E-mail app and opened from E-mail app, via notification or through Download Manager
*txt,doc,xls: those types of files can be downloaded from E-mail app but not opened. The files are available in the Download Manager list and it could be accessible from an doc viewer app since they are locally stored
*vcf: some bugs have been opened to track the issues found while checking vcard attachments, please see Bug 1138372, Bug 1138371, Bug 1138405 and Bug 1138377

ni to Shine in order to confirm if we all are seeing the same. Thanks!

Environmental Variables:
Device: Flame Master
Build ID: 20150302073244    
Gaia: cf3b761
Gecko: 75fc721
Platform Version: 39.0a1 (Master)
Firmware Version: v18D

	Andrew Sutherland [:asuth] (he/him) Assignee
	Comment 128 • 9 years ago

(In reply to Shine from comment #126)
> Per comment 4 and comment 11, this problem is verified pass on latest build
> of Flame 3.0. Now the result is: There's no option to download
> pdf/txt/doc/xls files and can't open them by tapping the attachment icon.
> See attachment: Flame3.0_screenshot.png
> Rate:0/3
> 
> Flame 3.0 build:
> Build ID               20150302010223
> Gaia Revision          f34ce82a840ad3c0aed3bfff18517b3f6a0eb37f
> Gaia Date              2015-02-27 15:48:31

I'm not sure what's going on here, but your gaia build is mismatched with your gecko build.  This gaia revision doesn't actually include the fix on this bug, which explains what you're seeing here.  (The gaia revision pre-dates the landing of the fix, the back-out, and the re-landing.)

	Shine(Leave from Mozilla)
	Comment 129 • 9 years ago

(In reply to Noemí Freire (:noemi) from comment #127)
> Just gathering all the scenarios checked in master after landing this patch:
> *pdf: the attachment can be downloaded from E-mail app and opened from
> E-mail app, via notification or through Download Manager
> *txt,doc,xls: those types of files can be downloaded from E-mail app but not
> opened. The files are available in the Download Manager list and it could be
> accessible from an doc viewer app since they are locally stored
> *vcf: some bugs have been opened to track the issues found while checking
> vcard attachments, please see Bug 1138372, Bug 1138371, Bug 1138405 and Bug
> 1138377

Hi Noemí,
My verify result is different from you:
pdf: it cannot be downloaded and opened from Email app; 
txt,doc,xls: it cannot be downloaded and opened from Email app;
Thanks!

	Noemí Freire (:noemi)
	Comment 130 • 9 years ago

Hi Shine,

Could you please confirm which version of gaia build and gecko build are you testing with?. 
Per comment 128, it seems the gaia revision you mentioned (f34ce82a840ad3c0aed3bfff18517b3f6a0eb37f) on comment 126 does not include the patch corresponding to this bug. Thanks!

	Teri Wen [:twen]
	Comment 131 • 9 years ago

Hi,

Using a file manager downloaded from marketplace, I can send/view pdf and txt attachments. 

*pdf - can be attached and send, downloaded and opened 
*txt - can be attached and send, downloaded and opened  
*doc - can be attached and send, downloaded but unable to view or open

STR:
Prerequisite: Have files on sd card. 
1. Open Marketplace app - search and install "File Manager"
2. Open File Manager
3. Tap sd card
4. Long press on the file you want to send for attachment
5. Tap share -> E-Mail
6. Directed to email app. Fill in recipient and subject.
7. Send mail.
8. Go to email app to receive mail. 
9. Open the mail
10. Tap the check mark icon next to the attachment to download.
11. Tap the eye icon to open.

Gaia-Rev        4352d56f8c79a51eb44e43658472236a38d6f1d8
Gecko-Rev       https://hg.mozilla.org/mozilla-central/rev/44bcd21e59fe
Build-ID        20150303160224
Version         39.0a1
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20150303.192928
FW-Date         Tue Mar  3 19:29:39 EST 2015
Bootloader      L1TC000118D0

	Andrew Sutherland [:asuth] (he/him) Assignee
	Comment 132 • 9 years ago

Comment on attachment 8567539 [details] [diff] [review]
adoptDownload implementation with tests and re-enabling existing mozDownloadManager tests v4

This is the mozDownloadManager gecko patch.  It requires that bug 1125989's patch is approved for uplift as a pre-condition.

[Approval Request Comment]
Bug caused by:
Not a regression.  New functionality.

User impact if declined:
Users will still only be able to download audio/video/image files and vcards.


Testing completed (note, I have repurposed my comments from an email thread about this bug, so deja vu is expected):

AUTOMATED:
This patch adds a reasonably thorough mochitest for adoptDownload, plus it re-enabled the mozDownloadManager tests that had been disabled for over 11 months.  This covers the interplay of the jsdownloads mechanism and mozDownloadManager.

There's a python marionette UI test that long-presses on an image in the browser and chooses to save it.  This test triggers the pre-patch download control/data-flow path and verifies that it results in a notification being displayed and the file ending up stored on disk.  This is particularly end-to-end, covering the browser element implementation, the jsdownloads implementation, the mozDownloadManager implementation, and the hand-off from the mozDownloadManager implementation to the system app implementation.  The data-flow path for downloads added via adoptDownload and normal downloads is the same.  This file can be found at:
https://github.com/mozilla-b2g/gaia/blob/master/tests/python/gaia-ui-tests/gaiatest/tests/functional/browser/test_browser_save_image.py

The system app and settings apps have surprisingly extensive unit tests for their aspects of download management.

MANUAL:
I've tested on device, :jrburke has tested on device, and various QA people have tested on device.  There has been some confusion on this bug, but all evidence suggests this is due to using the wrong build rather than any problem in the patch.  (And quite specifically, it's the gaia patch that decides whether or not we try to download new file types, this gecko patch is not involved at all.)


Risk to taking this patch (and alternatives if risky):
Very low risk for the gecko change.  The gecko change does not meaningfully impact b2g behaviour at all.  (There was a minor change to the return value of the call to clear downloads, but its return value was never used and race-prone.)


String or UUID changes made by this patch:
No strings in this patch.  Yes a string in the gaia patch.

	Andrew Sutherland [:asuth] (he/him) Assignee
	Comment 133 • 9 years ago

Comment on attachment 8570806 [details] [review]
[gaia] asutherland:email-download-everything > mozilla-b2g:master

[Approval Request Comment]
[Bug caused by]:
Ditto per the above: new functionality, not a regression.

[User impact] if declined:
Ditto per the above: Users will still only be able to download audio/video/image files and vcards.

[Testing completed]:
Basically ditto per the above, a variety of manual testing has been performed and we have automated tests that help verify we don't break the non-email paths.  There is no specific automated coverage of the end-to-end email path here, however email continues to have back-end download tests and through transitive closure over the new adoptDownload test and the uniform data-handling path for downloads and the existing automated tests for downloads for the standard browser download path, we believe we have coverage.  (Integration tests were considered, but I estimated that they would be a significant effort outlay to produce something with a high maintenance burder and a high rate of intermittent failures.  They would have been systems front-end heavy and systems front-end gets most of its download tests through unit tests; there are some integration tests that observe the notification being produced, but that's it.)

[Risk to taking this patch] (and alternatives if risky):
Low risk.

(deja vu:) The biggest potential for regret from this is just that people will download more attachments of different file types and this will result in new possibilities for things to go wrong in other apps and "email" will be in the STR.  For example, the PDF viewer refuses to render images that will be larger than 1 megabyte in size when rendered.  It would not be surprising for someone to file a bug against email for this, though it would of course be incorrect.  The good news is that since attachments can already be downloaded from the web and "open"ed via web activities from the downloads list, email isn't introducing any new permutations.  The same problems could already be reproduced regardless of this uplift.

Note that comment 127 covers some issues in the contacts app related to vcards and one settings download issue.  All issues are pre-existing and orthogonal to these changes.
- vCards were already whitelisted in MimeMapper prior to this bug (I believe for v2.2 and perhaps even in v2.1).  There apparently was just less investigation into the contacts/email interaction prior to this bug.
- The settings download UI issue is I think related to it interpreting any error from a triggered activity as something meriting displaying an "oh no! an error, should I delete the file?" message.  There is an idiom for explicit cancellation used by various apps which is probably triggering this and should likely be whitelisted.

[String changes made]:
We added 2 strings for the error message we display if there is no activity available to service the activity request.  It's derived from the one in the downloads logic.  late-l10n should be added if approving this patch.

	James Burke [:jrburke]
	Comment 135 • 9 years ago

Comment on attachment 8572739 [details] [review]
[gaia] jrburke:bug825318-email-v2.2-email-download-everything > mozilla-b2g:v2.2

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #):
[User impact] if declined:
[Testing completed]:
[Risk to taking this patch] (and alternatives if risky):
[String changes made]:

Because of the uplift in bug 1138624 comment 7, :RyanVM asked for a 2.2-specific pull request that merges the result of that uplift and this uplift, so this is the one to use for approval granting on 2.2.

Carrying over r+ from the master reviews (the merge from bug 1138624 was just removing the OK string from attachment_did_not_open_alert.html).

Copy of the previous approval request:

[Approval Request Comment]
[Bug caused by]:
Ditto per the above: new functionality, not a regression.

[User impact] if declined:
Ditto per the above: Users will still only be able to download audio/video/image files and vcards.

[Testing completed]:
Basically ditto per the above, a variety of manual testing has been performed and we have automated tests that help verify we don't break the non-email paths.  There is no specific automated coverage of the end-to-end email path here, however email continues to have back-end download tests and through transitive closure over the new adoptDownload test and the uniform data-handling path for downloads and the existing automated tests for downloads for the standard browser download path, we believe we have coverage.  (Integration tests were considered, but I estimated that they would be a significant effort outlay to produce something with a high maintenance burder and a high rate of intermittent failures.  They would have been systems front-end heavy and systems front-end gets most of its download tests through unit tests; there are some integration tests that observe the notification being produced, but that's it.)

[Risk to taking this patch] (and alternatives if risky):
Low risk.

(deja vu:) The biggest potential for regret from this is just that people will download more attachments of different file types and this will result in new possibilities for things to go wrong in other apps and "email" will be in the STR.  For example, the PDF viewer refuses to render images that will be larger than 1 megabyte in size when rendered.  It would not be surprising for someone to file a bug against email for this, though it would of course be incorrect.  The good news is that since attachments can already be downloaded from the web and "open"ed via web activities from the downloads list, email isn't introducing any new permutations.  The same problems could already be reproduced regardless of this uplift.

Note that comment 127 covers some issues in the contacts app related to vcards and one settings download issue.  All issues are pre-existing and orthogonal to these changes.
- vCards were already whitelisted in MimeMapper prior to this bug (I believe for v2.2 and perhaps even in v2.1).  There apparently was just less investigation into the contacts/email interaction prior to this bug.
- The settings download UI issue is I think related to it interpreting any error from a triggered activity as something meriting displaying an "oh no! an error, should I delete the file?" message.  There is an idiom for explicit cancellation used by various apps which is probably triggering this and should likely be whitelisted.

[String changes made]:
We added 2 strings for the error message we display if there is no activity available to service the activity request.  It's derived from the one in the downloads logic.  late-l10n should be added if approving this patch.

	Shine(Leave from Mozilla)
	Comment 136 • 9 years ago

Hi Noemí,
The version of gaia build and gecko build in comment 126 is:
Flame 3.0 build:
Build ID               20150302010223
Gaia Revision          f34ce82a840ad3c0aed3bfff18517b3f6a0eb37f
Gaia Date              2015-02-27 15:48:31
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/eea6188b9b05
Gecko Version          39.0a1

I have verify this problem on today's build, and the verify result is as below:
STR：
1. Log in an mail account on device.
2. Send a mail with pdf,txt,doc,xls,vcf attachments from my PC Outlook/Android device to test device.
3. Launch mail app on test device and received the mail.
4. Tap the check mark icon next to the attachment to download.
5. Tap the eye icon to open.
**pdf: the attachment can be downloaded from E-mail app and opened from E-mail app, via notification or through Download Manager; But tap “x” icon to close pdf file, it will go back to Homescreen.
**doc, xls: those types of files can be downloaded from E-mail app but not opened. 
**txt: the types of file can be downloaded from E-mail app and opened from E-mail app, via notification or through Download Manager.
**vcf: the types of file can be downloaded from E-mail app and opened from E-mail app, via notification or through Download Manager.
See attachment: Video_2.MP4

Flame 3.0 build:
Build ID               20150304010324
Gaia Revision          3fc0ac309f5fb0c1fe82c12223b955a4efce27e6
Gaia Date              2015-03-03 21:58:43
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/c5b90c003be8
Gecko Version          39.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150304.041952
Firmware Date          Wed Mar  4 04:20:03 EST 2015
Bootloader             L1TC000118D0

	Noemí Freire (:noemi)
	Comment 137 • 9 years ago

(In reply to Shine from comment #136)
> **pdf: the attachment can be downloaded from E-mail app and opened from
> E-mail app, via notification or through Download Manager; But tap “x” icon
> to close pdf file, it will go back to Homescreen.

> 
> Flame 3.0 build:
> Build ID               20150304010324
> Gaia Revision          3fc0ac309f5fb0c1fe82c12223b955a4efce27e6
> Gaia Date              2015-03-03 21:58:43
> Gecko Revision        
> https://hg.mozilla.org/mozilla-central/rev/c5b90c003be8
> Gecko Version          39.0a1
> Device Name            flame
> Firmware(Release)      4.4.2
> Firmware(Incremental)  eng.cltbld.20150304.041952
> Firmware Date          Wed Mar  4 04:20:03 EST 2015
> Bootloader             L1TC000118D0

Hi,

Bug 1139870 has been opened to track the issue you mention above, sometimes a crash occurs when opening/closing PDF viewer.

	Noemí Freire (:noemi)
	Comment 138 • 9 years ago

(In reply to Shine from comment #136) 
> **txt: the types of file can be downloaded from E-mail app and opened from
> E-mail app, via notification or through Download Manager. 
> Flame 3.0 build:
> Build ID               20150304010324
> Gaia Revision          3fc0ac309f5fb0c1fe82c12223b955a4efce27e6
> Gaia Date              2015-03-03 21:58:43
> Gecko Revision        
> https://hg.mozilla.org/mozilla-central/rev/c5b90c003be8
> Gecko Version          39.0a1
> Device Name            flame
> Firmware(Release)      4.4.2
> Firmware(Incremental)  eng.cltbld.20150304.041952
> Firmware Date          Wed Mar  4 04:20:03 EST 2015
> Bootloader             L1TC000118D0

We are not able to open .txt files from E-mail or Download Manager, Bug 1139903 has been opened to track this issue. Thanks!

	bhavana bajaj [:bajaj]
	Comment 139 • 9 years ago

was waiting for some comments for #138(Bug 1139903 ) before approving but spoke offline with dylan and this could be out of scope of this and confirmed we should not hold up this feature on this gap. 

So approving what we have for now and flagging :twen for verification.

	Ryan VanderMeulen [:RyanVM]
	Comment 140 • 9 years ago

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/46767b87fb14

I'm holding off on the Gaia uplift for now until the Gecko patches from this bug and bug 1125989 have for sure stuck so backouts are less complicated in the event that there's a problem.

	Teri Wen [:twen]
	Comment 141 • 9 years ago

Waiting for build, will check again tomorrow.

	Ryan VanderMeulen [:RyanVM]
	Comment 142 • 9 years ago

v2.2: https://github.com/mozilla-b2g/gaia/commit/50e6d85c26beb25357fbd8cce8d85a17e18df9c0

	Teri Wen [:twen]
	Comment 143 • 9 years ago

Verified on v2.2, all files(pdf, doc, txt, and vcf) can be downloaded. Doc and txt files could not be viewed as bug 1139903 described. Vcf files couldn't be opened, which is related to bug 1138405. 

Receiving mail:
PDF - downloaded, viewed
DOC - downloaded, not viewed
TXT - downloaded, not viewed
VCF - downloaded, opened via contacts with a message "Not enabled yet" 
     

Sending mail:
Following comment 131, all file types can be attached and sent with email app. 


Gaia-Rev        572d60e0a440ee4af50bc6b6adad8876eadbdb4d
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/3db27b3c9298
Build-ID        20150312162502
Version         37.0
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20150312.200118
FW-Date         Thu Mar 12 20:01:31 EDT 2015
Bootloader      L1TC000118D0

	Noemí Freire (:noemi)
	Comment 146 • 9 years ago

(In reply to Teri Wen [:twen] from comment #144)
> Created attachment 8577144 [details]
> Not enabled yet error message when opening vcf attachment in email

Hi Teri,

That's the expected behavior in 2.2 branch. The vcf file you are testing with contains several contacts, notice that importing multiple contacts within a vcard file support is not landed in 2.2 branch please see Bug 849729 for further details. Thanks!

	Teri Wen [:twen]
	Comment 147 • 9 years ago

Thanks Noemi. Tested with a vcard containing just one contact, and it opened and viewed perfectly in contacts app.

V2.2:
Gaia-Rev        4e0633463571377ad4badc680b666771684e862d
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/535ec28fb36f
Build-ID        20150319162506
Version         37.0
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20150316.195035
FW-Date         Mon Mar 16 19:50:48 EDT 2015
Bootloader      L1TC000118D0

master:
Gaia-Rev        c39e15f631de80c69467fda0d4ea0bcda9e194ca
Gecko-Rev       https://hg.mozilla.org/mozilla-central/rev/cbd0efcd976c
Build-ID        20150319160212
Version         39.0a1
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20150316.195035
FW-Date         Mon Mar 16 19:50:48 EDT 2015
Bootloader      L1TC000118D0