This is the webidl definition of the api.

Use the page at https://people.mozilla.org/~fdesre/downloads/ for testing. Starting downloads and getting state change works, getDownloads() and other download-specific methods not yet.

Updated to match the implementation.

Updated implementation. Everything in the webidl is implemented, basic smoketests from https://people.mozilla.org/~fdesre/downloads/ look ok.

Gregor & Aus, here's how the implementation is organized: - DownloadsAPI.jsm runs in the parent, and register a view with the platform api to listen for new downloads, etc. - DownloadsIPC.jsm runs in the child, it's the singleton that talks with the parent for each process. - DownloadsAPI.js runs in the child, one instance per window. This setup means that we can have many DOM objects that are only using a single IPC channel. DownloadsAPI.js calls directly in DownloadsIPC.jsm and listen for download state changes using observer notifications (using a weak ref).

Hi Fabrice! Do you think that makes sense to add an extra field, id, to identify the download. Right now with url it's not enough since user can download a file twice, and in the fronted would be handy to have it for the edit mode. Cheers, F.

(In reply to Francisco Jordano [:arcturus] (on PTO till 18/11) from comment #6) > Hi Fabrice! > > Do you think that makes sense to add an extra field, id, to identify the > download. Right now with url it's not enough since user can download a file > twice, and in the fronted would be handy to have it for the edit mode. We can, and I actually have one internally so it would be trivial. You're right that the url can't be used as an id but the file path should be unique: we create names like file(2).txt, file(3).txt etc. Either way is fine for me.

Nice Fabrice, temporarily we have it https://github.com/mozilla-b2g/gaia/blob/master/shared/js/download/download_formatter.js#L77 We can change this implementation when download.id will be available

Changes from v2 are: - added an id property. - added an error property, which is a DOMError that can be checked when a donwload is stopped because something failed.

Implementation changes to match the webidl. I also cleaned up the state transitions. The valid states are now: - stopped, when no network transfer is happening. - downloading. - done, when everything succeeded. - removed, when the download is not active anymore, but the dom object is still alive.

Hi Fabrice, Taking a look to the API I've realized that when requesting 'getDownloads', we are retrieving an array. Would it be possible to use a cursor instead? Currently in SMS App we moved from Array to Cursor long time ago when requesting 'getMessages' (http://mxr.mozilla.org/mozilla-central/source/dom/mobilemessage/interfaces/nsIDOMMobileMessageManager.idl#58) and the change in the performance was great! This is due to instead of waiting Gecko to generate the entire list of items before rendering, we are rendering from the very beginning, letting the user to show the most recent items asap, and adding the rest of items with a lazy-loading scheme. I don't know if it would be added as a followup of performance, or if it could be part of the current patch. Wdyt? Thanks!

Hi Borja, I don't plan to move to cursor for now, unless we have real performance issues showing up. I don't expect the list of downloads to be as big as the SMS ones, and the platform backend doesn't expose a cursor either so that would need some work that I'd rather punt to 1.4.

Right, taking the time constrains, sure we can live with the getDownloads for 1.3 ;)

It sounds good to me. Thanks Fabrice!

Very minor changes compared to v3 (removing a useless NoInterfaceObject).

Fixed a few bugs found while writing the tests.

These mochitests are b2g-only. They cover most of the API, the notable missing piece being the resume() method (we test pause() though).

Comment on attachment 8334367 [details] [diff] [review] implementation v4 Review of attachment 8334367 [details] [diff] [review]: ----------------------------------------------------------------- fixing a few issues I saw on try.

Hi, I cannot stop playing around this WIP, and have a little suggestion, current download.id strings, the hash may contain characters that are not suitable if we want to use that string as a DOM id (like '/' and '=') We could replace them in the frontend, but could be nice if the id generated is already clean and we can use it straight forward. Thanks guys!

Comment on attachment 832489 [details] [diff] [review] implementation v3 Review of attachment 832489 [details] [diff] [review]: ----------------------------------------------------------------- This is looking good but I have some comments and concerns noted below. I'll take a look at the updated patch next. I'd say this specific patch is an r- but easily turned into r+ if comments are addressed. :) ::: b2g/chrome/content/shell.js @@ +1522,5 @@ > +Components.manager.QueryInterface(Ci.nsIComponentRegistrar) > + .registerFactory(kTransferCid, "", > + kTransferContractId, null); > + > +dump("XXX ===== XXX transert components registered"); We should possibly make this message a little more descriptive and also point out where it's originating from if it is to remain there in the final patch. ::: b2g/components/DownloadsUI.js @@ +65,5 @@ > + if (DownloadsCommon.useToolkitUI) { > + this._toolkitUI.show(aWindowContext, aDownload, aReason, aUsePrivateUI); > + return; > + } > + Since we end up branching like this for every method in nsIDownloadManagerUI, it would be nice to generate different JavaScript functions in the factory depending on the 'useToolkitUI' pref so that we avoid branching after it's constructed. Might be more trouble than it's worth though. @@ +96,5 @@ > + { > + // If we're still using the toolkit downloads manager, delegate the call > + // to it. Otherwise, return true for now, until we decide on how we want > + // to indicate that a new download has started if a browser window is > + // not available or minimized. I think it would be good to add a big angry indicator that this isn't the final way it will work. Maybe something like "XXX TODO!!!" or "XXX TEMPORARY!!!" if it is to change _after_ initial commit. @@ +129,5 @@ > + > + // If window is private then show it in a tab. > + if (PrivateBrowsingUtils.isWindowPrivate(parentWindow)) { > + parentWindow.openUILinkIn("about:downloads", "tab"); > + return; This return; statement isn't super useful since we're going to return anyway. :) @@ +130,5 @@ > + // If window is private then show it in a tab. > + if (PrivateBrowsingUtils.isWindowPrivate(parentWindow)) { > + parentWindow.openUILinkIn("about:downloads", "tab"); > + return; > + } else { This 'else' statement and the one following it use a different style than the ones earlier in the file. I'm not sure which is our agreed standard but it would be nice if they were all the same. ::: dom/apps/src/PermissionsTable.jsm @@ +303,5 @@ > certified: PROMPT_ACTION > }, > + "downloads": { > + app: DENY_ACTION, > + privileged: DENY_ACTION, Is it really the case that we'll _only_ allow use of this API for the time being to certified applications? Seems like other applications are likely to want to trigger downloads that are managed by the system. ::: dom/base/DOMRequestHelper.jsm @@ +184,5 @@ > if (this.uninit) { > this.uninit(); > } > + > + this._window = null; Looks like this might fix other random issues for cases where Objects were expecting _window to still be available. Woohoo! ::: dom/downloads/src/DownloadsAPI.js @@ +67,5 @@ > + let dom = createDOMDownloadObject(self._window, aDownloads[id]); > + array.push(self._prepareForContent(dom)); > + } > + aResolve(array); > + }, Is there a reason why we can't use .bind(this) here instead of var self = this; ? ::: dom/downloads/src/DownloadsAPI.jsm @@ +49,5 @@ > + /** > + * Returns a unique id for each download, hashing the url and the path. > + */ > + downloadId: function(aDownload) { > + let id = this._ids.get(aDownload, null); It's already a TODO but let's make sure we share the hashing function. @@ +104,5 @@ > + !aDownload.succeeded && > + !aDownload.DownloadError) { > + res.state = "downloading"; > + } > + if (aDownload.succeeded) { we could "else if" here instead to avoid two checks when object is in "downloading" state (which is probably most of the time). @@ +138,5 @@ > + receiveMessage: function(aMessage) { > + /*if (!aMessage.target.assertPermission("downloads")) { > + debug("No 'downloads' permission!"); > + return; > + }*/ if this is to be used later, let's add a little TODO note :)

Comment on attachment 8334366 [details] [diff] [review] webidl v4 Review of attachment 8334366 [details] [diff] [review]: ----------------------------------------------------------------- r+ with minor documentation additions for DOMDownloadManager and DOMDownload interfaces. ::: dom/webidl/Downloads.webidl @@ +6,5 @@ > + > +[NavigatorProperty="mozDownloadManager", > + JSImplementation="@mozilla.org/downloads/manager;1", > + Pref="dom.mozDownloads.enabled"] > +interface DOMDownloadManager : EventTarget { It would be nice to have an overall description of what this interface is expected to be responsible for and some implementation guidelines in case it is reimplemented later. @@ +23,5 @@ > +}; > + > +[JSImplementation="@mozilla.org/downloads/download;1", > + Pref="dom.mozDownloads.enabled"] > +interface DOMDownload : EventTarget { Same here.

(In reply to Francisco Jordano [:arcturus] (on PTO till 18/11) from comment #19) > Hi, > > I cannot stop playing around this WIP, and have a little suggestion, current > download.id strings, the hash may contain characters that are not suitable > if we want to use that string as a DOM id (like '/' and '=') > > We could replace them in the frontend, but could be nice if the id generated > is already clean and we can use it straight forward. > > Thanks guys! We could ensure that in gecko, but I'm a bit worried about the front-end making this kind of assumptions. Can't you just btoa(id) to get what you want?

Sure we can, also would like not to do it in the front-end, but can use the wip meanwhile, so no problem. Thanks a lot!

(In reply to Fabrice Desré [:fabrice] from comment #22) > (In reply to Francisco Jordano [:arcturus] (on PTO till 18/11) from comment > #19) > > Hi, > > > > I cannot stop playing around this WIP, and have a little suggestion, current > > download.id strings, the hash may contain characters that are not suitable > > if we want to use that string as a DOM id (like '/' and '=') > > > > We could replace them in the frontend, but could be nice if the id generated > > is already clean and we can use it straight forward. > > > > Thanks guys! > > We could ensure that in gecko, but I'm a bit worried about the front-end > making this kind of assumptions. Can't you just btoa(id) to get what you > want? Actually btoa transforms to Base 64 and = is a valid base64 character... so probably not what Francisco wants :)

Thanks Antonio for the tip, totally miss it :)

Comment on attachment 8334366 [details] [diff] [review] webidl v4 Review of attachment 8334366 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/webidl/Downloads.webidl @@ +61,5 @@ > + > + // Resumes the download. > + Promise resume(); > + > + // Will trigger anytime one on the properties of the object changes. Hm that needs some rewording.

Hi Fabrice, could you help us with a question? We would like to know what are the meaning of the states for our implementation: * "paused" -> did the user pause the download? * "canceled" -> the download was stopped because of network, rebooted device, ...? * "downloading" -> obvious ;) * "stopped" -> no idea... because we have "paused" and "canceled" although I am sure that I forgot something :) * "done" -> complete? * "removed" -> the file was removed from device, right?

(In reply to Francisco Jordano [:arcturus] (on PTO till 18/11) from comment #19) > I cannot stop playing around this WIP, and have a little suggestion, current > download.id strings, the hash may contain characters that are not suitable > if we want to use that string as a DOM id (like '/' and '=') Actually, the current solution of generating an ID by hashing the source and target of the download does not really make strong guarantees on the uniqueness of the ID, making it unsuitable for use as a DOM ID or strong identifier in any case. Since the target file is removed when a download is explicitly cancelled by the user, it's possible (and expected) to have two download objects with the same source and target when the user starts a download from a web page, cancels the download from the control UI, then restarts it from the web page again. This is how the Downloads back-end works (both with the old and the new API) and cannot be easily changed due to how the code that starts downloads works. On Desktop, for the Downloads Panel, we simply generate non-persistent numeric IDs starting from "1" onwards. We may assign a new ID to the same download when restored in a different session. A persistent ID has never been a requirement, as the only use for the ID is to link UI elements to back-end elements, or to identify a target for stateless IPC calls. We even dropped the download's GUID, as we don't sync downloads and any download cloud service would use a totally different back-end design in any case. Speaking of cloud downloads, we may even lack a local target altogether. Do you have any requirement for using a persistent ID that we don't have on Desktop?

(In reply to Ghislain 'Aus' Lacroix from comment #20) > ::: b2g/components/DownloadsUI.js The nsIDownloadManagerUI interface is currently used only in these cases: - By the command to show all downloads in the Desktop UI (REASON_USER_INTERACTED). - By the old API, when a new download is started (REASON_NEW_DOWNLOAD). - By the old Toolkit Window UI (still in use by Thunderbird and SeaMonkey). Since you are using the new API and have your own way to start the applications that control downloads, I don't think you need an nsIDownloadManagerUI implementation at all.

(In reply to Cristian Rodriguez (:crdlc) from comment #27) > Hi Fabrice, could you help us with a question? We would like to know what > are the meaning of the states for our implementation I see that these have since been cleaned up, just the comment on the IDL needs to be updated. We did quite a bit of brainstorming on the download state names when designing the new API, so I have a couple of suggestions for improvements: (In reply to Fabrice Desré [:fabrice] from comment #10) > Implementation changes to match the webidl. I also cleaned up the state > transitions. The valid states are now: > - stopped, when no network transfer is happening. > - downloading. > - done, when everything succeeded. This may be "succeeded" (as opposed to done but failed), as the description itself suggests. > - removed, when the download is not active anymore, but the dom object is > still alive. To prevent confusion with a removed target file, the could be "disposed" (like for C# objects) or "finalized" (less clear, but it's the terminology we use in the back-end API itself and the Java equivalent of "disposed").

Hi Fabrice, just some observation after using the patch for a while. When a download finish correctly, after next boot, when asking for |getDownloads|, that previous download won't appear in the list of downloads returned by the api. Does that mean that we need to store which downloads finished correctly? Thanks!

(In reply to Francisco Jordano [:arcturus] (on PTO till 18/11) from comment #31) > When a download > finish correctly, after next boot, when asking for |getDownloads|, that > previous download won't appear in the list of downloads returned by the api. > > Does that mean that we need to store which downloads finished correctly? This is the default behavior because, on Desktop, we store finished downloads using the Places API. We don't persist finished downloads in "downloads.json", resulting in a very compact file containing mostly the in-progress downloads. In the Downloads view in the Library, we mix data from Places and the Downloads API. Depending on your use cases, you may want to keep finished downloads in "downloads.json", or obtain data about past downloads separately from the file system. The logic that decides which downloads are persisted can be controlled here: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/jsdownloads/src/DownloadIntegration.jsm#286 In case you want to persist finished downloads in "downloads.json", I suggest setting a time limit so that older downloads are deleted, otherwise you could observe performance issues due to the linear growth of the list with time, like it used to happen on older Firefox versions (most users don't really care about cleaning up the list manually).

Carrying r+, I just updated the documentation.

Updated implementation with the new state names.

Added a test for resume().

Try run: https://tbpl.mozilla.org/?tree=Try&rev=fd2c56aa30f6

(In reply to Fabrice Desré [:fabrice] from comment #36) > Try run: > https://tbpl.mozilla.org/?tree=Try&rev=fd2c56aa30f6 Almost green... I can't repro the download tests failure locally, and I'm gonna look into the B2G M8

(In reply to :Paolo Amadini from comment #32) > This is the default behavior because, on Desktop, we store finished > downloads using the Places API. We don't persist finished downloads in > "downloads.json", resulting in a very compact file containing mostly the > in-progress downloads. In the Downloads view in the Library, we mix data > from Places and the Downloads API. > > Depending on your use cases, you may want to keep finished downloads in > "downloads.json", or obtain data about past downloads separately from the > file system. The logic that decides which downloads are persisted can be > controlled here: > > http://mxr.mozilla.org/mozilla-central/source/toolkit/components/jsdownloads/ > src/DownloadIntegration.jsm#286 > > In case you want to persist finished downloads in "downloads.json", I > suggest setting a time limit so that older downloads are deleted, otherwise > you could observe performance issues due to the linear growth of the list > with time, like it used to happen on older Firefox versions (most users > don't really care about cleaning up the list manually). If that's the default in desktop, let's try to do the same in B2G then :) Thanks folks!

When do you think API will be landed?

(In reply to Marcelino Veiga Tuimil [:sonmarce] from comment #39) > When do you think API will be landed? Aus is taking care of finalizing it, hopefully this week.

Indeed, the only thing we need to take care of is get rid of the intermittent test failures.

\o/ thanks folks! Looking forward for have it ready and land our code in Gaia as well :)

Thanks for you help! :-)

To be applied on top of the other patches.

Comment on attachment 8338859 [details] [diff] [review] patch-v1-rate-limited-response.patch Review of attachment 8338859 [details] [diff] [review]: ----------------------------------------------------------------- It's not everyday that tests slaves are too fast at something! ::: dom/downloads/tests/serve_file.sjs @@ +1,1 @@ > // Serves a file with a given mime type and size. nit: update comment. @@ +92,5 @@ > + Components.classes["@mozilla.org/timer;1"] > + .createInstance(Components.interfaces.nsITimer); > + > + // sending at a specific rate requires our response to be asynchronous so > + // we handle all requests asynchrnously. See handleResponse(). nit: s/asynchrnously/asynchronously

Tests are green with this patch on pine: https://tbpl.mozilla.org/?tree=Pine Looks like we're good to go guys! :)

(In reply to Ghislain 'Aus' Lacroix from comment #46) > Tests are green with this patch on pine: https://tbpl.mozilla.org/?tree=Pine > > Looks like we're good to go guys! :) Almost! we still need to first add the 'downloads' permission to the test-container app in gaia, and uncomment the permission check in DownloadsAPI.jsm

\o/

Updated diff with small changes addressed. r+ carries from Fabrice & Gregor.

Final Download API (WebIDL). r+ carries from Aus and Gregor.

Download API (Implementation). r+ carries over from Aus and Gregor.

Download API (Tests). r+ carries over from Aus and Gregor.

r+ from Gregor on this one. This was already tested on pine.

The gaia tree is currently closed but we have to push the gaia bits first :(

Gaiaaaaaaaaaaaaa :S Thanks guys for the effort!

I'll be watching the whole Gaia situation while I'm on a plane tomorrow, if we get some green I'll merge the Gaia changes.

Just merged :) https://github.com/mozilla-b2g/gaia/commit/256c2ac4a5e587d6d2afa0c51dcb70d6d11705bb

remote: https://hg.mozilla.org/integration/b2g-inbound/rev/19fbd3fb0373 remote: https://hg.mozilla.org/integration/b2g-inbound/rev/4811dfce0407 remote: https://hg.mozilla.org/integration/b2g-inbound/rev/401fdb5f8c76 remote: https://hg.mozilla.org/integration/b2g-inbound/rev/8146150d4df8

https://hg.mozilla.org/mozilla-central/rev/19fbd3fb0373 https://hg.mozilla.org/mozilla-central/rev/4811dfce0407 https://hg.mozilla.org/mozilla-central/rev/401fdb5f8c76 https://hg.mozilla.org/mozilla-central/rev/8146150d4df8

What part of "// IMPORTANT: Do not change the list below without review from a DOM peer!" in dom/tests/mochitest/general/test_interfaces.html isn't clear enough? I'm very close to just backing this all out.

I'm closer. https://hg.mozilla.org/integration/b2g-inbound/rev/6e3f11358300 https://hg.mozilla.org/integration/b2g-inbound/rev/b7bfd7471bd7

This needs to be backed out from aurora too.

This patch was merged almost a month ago, and it is working as part of Download Manager. Why do you want to back it out?

AFAICT this exposes a new name on global objects, which is something that a DOM peer needs to sign off on. We have a test in the tree (dom/tests/mochitest/general/test_interfaces.html) which makes sure that that happens. There are warnings in the file, and when the test fails the error also makes it clear that the review is needed. I'd like to understand why that is just all ignored and instead the test is just changed without the review? Now, I think we shouldn't have backed out, but I can't really blame Ms2ger. We're trying very hard to make it clear how serious this is, to have it just be ignored is quite frustrating.

Thanks for clarifying it, I would suggest to postpone back out (if possible) meanwhile you are evaluating the situation, because otherwise it will affect dependent Gaia stuff already landed

More generally, we need to stop making Gecko changes from bugs in Firefox OS::General, where nobody who works in Gecko is looking for them.

Note - this is already being backed out on Aurora in bug 948040.

Comment on attachment 8339529 [details] [diff] [review] Patch - Final - Download API (Test Interfaces Settings) - Apply Last Review of attachment 8339529 [details] [diff] [review]: ----------------------------------------------------------------- So this is just the test change, but really it's about exposing those interfaces. It seems odd that we're exposing these interfaces everywhere (on B2G), they're only usable if the "downloads" permission is set? If that's the case I think the interfaces should have a |Func="..."| extended attribute calling a C++ function that checks for the "downloads" permission (similar to what the code in Navigator::DoNewResolve does). I'll file a bug on making this nicer, we should be able to actually generate these permission checks from an extended attribute on the interface. But that shouldn't block this for now.

(In reply to :Ms2ger from comment #63) > I'm closer. > > https://hg.mozilla.org/integration/b2g-inbound/rev/6e3f11358300 > https://hg.mozilla.org/integration/b2g-inbound/rev/b7bfd7471bd7 This was not a good idea to do, as Marce comments are right - there's a tight binding between Gaia & Gecko here. If this gets backed out in gecko without the gaia piece, then gaia will be broken in so different many ways. I'm asking for a re-landing by a sheriff to ensure that the Gaia tree doesn't turn red with a busted build.

This should absolutely not reland without review from a DOM peer.

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #72) > This should absolutely not reland without review from a DOM peer. It must be re-landed - otherwise, the Gaia tree is going to be broken along with the build potentially. If we really need to do a backout here, then we need safely back this out on gaia & gecko in the right order.

Comment on attachment 8339522 [details] [diff] [review] Patch - Final - Download API (WebIDL) Review of attachment 8339522 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/webidl/Downloads.webidl @@ +44,5 @@ > + // "stopped" : No network tranfer is happening. > + // "succeeded" : The resource has been downloaded successfully. > + // "finalized" : We won't try to download this resource, but the DOM > + // object is still alive. > + readonly attribute DOMString state; Seems like this should be an enum?

(In reply to Jason Smith [:jsmith] from comment #73) > (In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #72) > > This should absolutely not reland without review from a DOM peer. > > It must be re-landed - otherwise, the Gaia tree is going to be broken along > with the build potentially. If we really need to do a backout here, then we > need safely back this out on gaia & gecko in the right order. Oh and the backout order would be - you would need to backout all relevant patches on Gaia first with a green travis build. Then, you would back this out in gecko.

(In reply to Peter Van der Beken [:peterv] from comment #70) > I'll file a bug on making this nicer, we should be able to actually generate > these permission checks from an extended attribute on the interface. That's now bug 952486.

(In reply to Jason Smith [:jsmith] from comment #75) > (In reply to Jason Smith [:jsmith] from comment #73) > > (In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #72) > > > This should absolutely not reland without review from a DOM peer. > > > > It must be re-landed - otherwise, the Gaia tree is going to be broken along > > with the build potentially. If we really need to do a backout here, then we > > need safely back this out on gaia & gecko in the right order. > > Oh and the backout order would be - you would need to backout all relevant > patches on Gaia first with a green travis build. Then, you would back this > out in gecko. Talked with Ryan about this in IRC - we're going to hold off merging b2g inbound to m-c to prevent turning the Gaia tree red. To resolve this, we either need to fix this bugs here to allow a re-landing of the downloads API or back out the relevant Gaia patches. Gregor - Let me know what you'd like to do here (forward fix quickly in gecko or backout in Gaia).

(In reply to Jason Smith [:jsmith] from comment #77) > (In reply to Jason Smith [:jsmith] from comment #75) > > (In reply to Jason Smith [:jsmith] from comment #73) > > > (In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #72) > > > > This should absolutely not reland without review from a DOM peer. > > > > > > It must be re-landed - otherwise, the Gaia tree is going to be broken along > > > with the build potentially. If we really need to do a backout here, then we > > > need safely back this out on gaia & gecko in the right order. > > > > Oh and the backout order would be - you would need to backout all relevant > > patches on Gaia first with a green travis build. Then, you would back this > > out in gecko. > > Talked with Ryan about this in IRC - we're going to hold off merging b2g > inbound to m-c to prevent turning the Gaia tree red. To resolve this, we > either need to fix this bugs here to allow a re-landing of the downloads API > or back out the relevant Gaia patches. > > Gregor - Let me know what you'd like to do here (forward fix quickly in > gecko or backout in Gaia). Well it will take multiple days of people that are on vacation to back out the gaia changes so this is not an option. A broken phone for 2 weeks is also not an option. We should re-land and do a followup fix.

What Gregor said. I'm puzzled by backouts happening before we ever discuss a solution. Everyone loses.

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #68) > More generally, we need to stop making Gecko changes from bugs in Firefox > OS::General, where nobody who works in Gecko is looking for them. Unfortunately I don't see any appropriate component under Core (unless just using DOM is ok). That's the case for several other apis.

(In reply to Fabrice Desré [:fabrice] from comment #80) > (In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #68) > > More generally, we need to stop making Gecko changes from bugs in Firefox > > OS::General, where nobody who works in Gecko is looking for them. > > Unfortunately I don't see any appropriate component under Core (unless just > using DOM is ok). That's the case for several other apis. Well the code for this feature all lives in dom/ ...

(In reply to Fabrice Desré [:fabrice] from comment #79) > What Gregor said. I'm puzzled by backouts happening before we ever discuss a > solution. Everyone loses. Just to be clear, I didn't do the backout because I think it's too late now. But had I noticed this a couple of weeks ago I would have backed it out immediately. You're checking in code in the DOM module, so it should have had a DOM peer review regardless of all else. We've been fairly lenient about that, but not when it comes to exposing interfaces. We've added the test messages exactly so that *we* don't have to keep looking over all the checkins to make sure any new interfaces have been reviewed. Given that people keep ignoring the review requirement we will probably now just go back requiring a DOM peer review for all changes in the DOM module (which is most of dom/). It'll be painful for the DOM peers, we were hoping to avoid it by requiring that review just for exposing interfaces, but it clearly isn't working.

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #81) > (In reply to Fabrice Desré [:fabrice] from comment #80) > > (In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #68) > > > More generally, we need to stop making Gecko changes from bugs in Firefox > > > OS::General, where nobody who works in Gecko is looking for them. > > > > Unfortunately I don't see any appropriate component under Core (unless just > > using DOM is ok). That's the case for several other apis. > > Well the code for this feature all lives in dom/ ... This needs to be a larger discussion on dev-platform, as I know all bug tracking that lives under DOM doesn't necessarily fall under DOM components (e.g. NFC lives in FxOS product, getUserMedia lives under WebRTC). I agree there's definitely some confusion here for tracking, so we should try to figure out a consistent story here going forward.

Peter, how do you want to move forward now? I can fix what needs to be fixed today.

AFAIK a discussion happened with the sheriffs and Ms2ger, and the backout will be undone. I gave my approval, but I don't know where we are on that. I suppose it might then be easier to open a new bug to deal with the required changes.

(In reply to Peter Van der Beken [:peterv] from comment #85) > AFAIK a discussion happened with the sheriffs and Ms2ger, and the backout > will be undone. I gave my approval, but I don't know where we are on that. I > suppose it might then be easier to open a new bug to deal with the required > changes. Thanks Peter. And yes, we should do the fixes in a followup. From what I understand of your previous comments: - enforce the use of the permission. Would something like what we do for Nfc be ok? (https://mxr.mozilla.org/mozilla-central/source/dom/base/Navigator.cpp#1809 and https://mxr.mozilla.org/mozilla-central/source/dom/webidl/MozNfc.webidl#9) - use a webidl enum for the |state| property.

What's the status here? Has the back-out been undone on gecko master? I can't work on anything until I get an idea of if/when/how the Downloads API will come back.

As requested by fabrice on IRC, I reverted the backout in https://hg.mozilla.org/integration/b2g-inbound/rev/60c82259294f