Remove the dependency on apps status in the Downloads api code



3 years ago
2 years ago


(Reporter: fabrice, Assigned: arroway)


Firefox Tracking Flags

(Not tracked)



(1 attachment, 1 obsolete attachment)



3 years ago
Keeping the permission is ok, but everything that is gated by AvailableIn=CertifiedApps needs to be chrome only instead.
No longer blocks: 1252143

Comment 1

3 years ago
Posted patch bug-1254283-wip.patch (obsolete) — Splinter Review
The 'downloads' certified permission is used in the system and settings app for the DownloadManager, and in the email apps. What needs to be removed is the code related to the adoptDownload() method which seems to be used only in the email app (which will become unprivileged) [1].

It's been implemented as part of bug 825318 (see comment, for allowing the mozDownloadManager to manage completed downloads from other (certified) apps. This was implemented in order to be able to see files attached to an email which are of various formats such as PDF. Other possible fixes for this bug have been listed by asuth in one of his comments [2].

It looks like it's not such a big breakage (correct me if I'm wrong), so I've started by removing the code of adoptDownload(). I'm attaching a WIP patch to get feedback to know if this is the right approach.
Gabriele, since Fabrice is on PTO, would you be able to have a look at this and tell me if this looks ok?

Assignee: nobody → stephouillon
Attachment #8737787 - Flags: feedback?(gsvelto)
Comment on attachment 8737787 [details] [diff] [review]

I think this is OK for two main reasons:

- This is a most unwebby API, IMHO we should be able to re-implement this functionality in the future using foreign fetch so it's got a replacement coming. No need to keep it around.

- If we want the e-mail app to become unprivileged it will need to handle downloads on its own, which for our use-cases might involve bundling pdf.js or something, either way it's easy to reclaim most of the functionality here even without using foreign fetch. That means that the functionality loss is rather limited and I think we can live with that until someone steps up to fix it.
Attachment #8737787 - Flags: feedback?(gsvelto) → feedback+

Comment 3

3 years ago
Ok, thank you for the feedback Gabriele!
I'll update the patch (updating relevant tests, etc), then.

Comment 4

3 years ago
I updated the patch with the removal of tests but I'm still waiting to get access again to try to run the tests and check nothing is broken.
Attachment #8737787 - Attachment is obsolete: true


3 years ago
Attachment #8741013 - Flags: review?(fabrice)

Comment 5

3 years ago
I pushed the patch on try, but it ran on gecko instead on pine

Comment 6

3 years ago
Comment on attachment 8741013 [details] [diff] [review]

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

Looks fine except for the ondowloadstart event.

Can you run the tests locally?

::: dom/webidl/Downloads.webidl
@@ -63,5 @@
> -  [AvailableIn=CertifiedApps]
> -  Promise<DOMDownload> adoptDownload(optional AdoptDownloadDict download);
> -
> -  // Fires when a new download starts.
> -  attribute EventHandler ondownloadstart;

We need to keep this event, it's triggered also for non-adopted downloads.
Attachment #8741013 - Flags: review?(fabrice) → feedback+


2 years ago
Last Resolved: 2 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.