Closed
Bug 1254285
Opened 8 years ago
Closed 7 years ago
Remove the dependency on apps status in the Downloads api code
Categories
(Firefox OS Graveyard :: General, defect)
Tracking
(Not tracked)
RESOLVED
WONTFIX
People
(Reporter: fabrice, Assigned: arroway)
References
Details
Attachments
(1 file, 1 obsolete file)
45.91 KB,
patch
|
fabrice
:
feedback+
|
Details | Diff | Splinter Review |
Keeping the permission is ok, but everything that is gated by AvailableIn=CertifiedApps needs to be chrome only instead. http://mxr.mozilla.org/mozilla-central/source/dom/webidl/Downloads.webidl#63
Assignee | ||
Comment 1•8 years ago
|
||
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 https://bugzilla.mozilla.org/show_bug.cgi?id=825318#c57), 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? Thanks! [1] https://github.com/mozilla-b2g/gaia/blob/kanikani/apps/email/js/ext/worker-support/devicestorage-main.js#L47 [2] https://bugzilla.mozilla.org/show_bug.cgi?id=825318#c19 [3] https://dxr.mozilla.org/mozilla-central/source/dom/downloads/DownloadsAPI.jsm#105
Assignee: nobody → stephouillon
Status: NEW → ASSIGNED
Attachment #8737787 -
Flags: feedback?(gsvelto)
Comment 2•8 years ago
|
||
Comment on attachment 8737787 [details] [diff] [review] bug-1254283-wip.patch 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+
Assignee | ||
Comment 3•8 years ago
|
||
Ok, thank you for the feedback Gabriele! I'll update the patch (updating relevant tests, etc), then.
Assignee | ||
Comment 4•8 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
Assignee | ||
Updated•8 years ago
|
Attachment #8741013 -
Flags: review?(fabrice)
Assignee | ||
Comment 5•8 years ago
|
||
I pushed the patch on try, but it ran on gecko instead on pine https://treeherder.mozilla.org/#/jobs?repo=try&revision=8a44ebeceec0
Reporter | ||
Comment 6•8 years ago
|
||
Comment on attachment 8741013 [details] [diff] [review] bug-1254285-Remove-the-dependency-on-apps-status-in-Downloads-API.patch 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+
Assignee | ||
Updated•7 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → WONTFIX
You need to log in
before you can comment on or make changes to this bug.
Description
•