983747 - [Browser API] Add a method to download a file

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Aus Lacroix [:aus]]

Currently the Downloads API does not provide a way for a certified application to trigger a download directly. We should have one for convenience so that applications can offload downloads to the download manager.

Comment 1

[Aus Lacroix [:aus]]

Working on this starting today.

Comment 2

[Aus Lacroix [:aus]]

Attachment: Patch - v1 - Add 'download' method to Downloads API

Comment 3

[[:fabrice] Fabrice Desré]

Comment on attachment 8418457 [details] [diff] [review]
Patch - v1 - Add 'download' method to Downloads API

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

I haven't reviewed the test yet, but I have a general question: I guess on use case is to trigger downloads for use cases like "Save image" on a long tap. In this situation, how do we deal with cookies that are set on the page but are lost with this api? It's likely to make the download fail. Same issue with http auth if any.

::: dom/downloads/src/DownloadsAPI.js
@@ +53,5 @@
>    },
>  
> +  download: function(aURL, aOptions) {
> +    debug("download(aURL = " + aURL + ", aOptions = " + aOptions + ")");
> +    

nit: whitespace.

::: dom/downloads/src/DownloadsAPI.jsm
@@ +187,5 @@
> +      let downloadPath = aData.options.targetDirectory;
> +
> +      if (!downloadFilename) {
> +        let url = new URL(aData.url);
> +        downloadFilename = url.pathname.split('/').pop();

what happens if I donwload http://foo.com/ ?

@@ +194,5 @@
> +      if (!downloadPath) {
> +        downloadPath = defaultDir;
> +      }
> +
> +      debug('creating download > ');

here and for other strings: we use double quotes in this file.

@@ +224,5 @@
> +      let list = yield Downloads.getList(Downloads.ALL);
> +      // Add our download to it.
> +      list.add(download);
> +    }).then(null, Cu.reportError);
> +    

nit: whitespace.

::: dom/downloads/src/DownloadsIPC.jsm
@@ +161,5 @@
> +    debug("download(aURL = " + aURL + ", aOptions = " + aOptions + ")");
> +    let deferred = Promise.defer();
> +    let pId = this.promiseId();
> +    this.downloadPromises[pId] = deferred;
> +    cpmm.sendSyncMessage("Downloads:Download", 

nit: whitespace.

::: dom/webidl/Downloads.webidl
@@ +73,5 @@
>  
>    // An opaque identifier for this download. All instances of the same
>    // download (eg. in different windows) will have the same id.
>    readonly attribute DOMString id;
> +  

nit: whitespace.

@@ +99,5 @@
> +  // Download requested via private browsing session?
> +  boolean isPrivate = false;
> +  // HTTP Referrer to send to server.
> +  DOMString? referrer = null;
> +  // Specify target filename

nit: full stop at the end of comment.

@@ +101,5 @@
> +  // HTTP Referrer to send to server.
> +  DOMString? referrer = null;
> +  // Specify target filename
> +  DOMString? targetFilename  = null;
> +  // Specify target directory (from primary writable storage root)

ditto

Comment 4

[Aus Lacroix [:aus]]

(In reply to Fabrice Desré [:fabrice] from comment #3)
> Comment on attachment 8418457 [details] [diff] [review]
> Patch - v1 - Add 'download' method to Downloads API
> 
> Review of attachment 8418457 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I haven't reviewed the test yet, but I have a general question: I guess on
> use case is to trigger downloads for use cases like "Save image" on a long
> tap. In this situation, how do we deal with cookies that are set on the page
> but are lost with this api? It's likely to make the download fail. Same
> issue with http auth if any.

Indeed. The way it's implemented currently, it would also fail since we're using a separate XHR to download the content and we initiate this from a different context than the browser window which has said content.

I would love for us to support these use cases (1) Cookies Required and (2) Auth Required.

That being said, I think sites which have content for user download will have a way to trigger this action on the page which will route the download in such a way that cookies and previous authentication are correctly used.
 
> ::: dom/downloads/src/DownloadsAPI.jsm
> @@ +187,5 @@
> > +      let downloadPath = aData.options.targetDirectory;
> > +
> > +      if (!downloadFilename) {
> > +        let url = new URL(aData.url);
> > +        downloadFilename = url.pathname.split('/').pop();
> 
> what happens if I donwload http://foo.com/ ?

I think it's probably best to error out if someone passes in a URL with no leaf name. I'll add a check for this.
 
I'll address the other nits as well. :)

Comment 5

[Aus Lacroix [:aus]]

Ehsan, Jonas, we're looking for your opinions on this new methods abilities.

See https://bugzilla.mozilla.org/show_bug.cgi?id=983747#c3 and https://bugzilla.mozilla.org/show_bug.cgi?id=983747#c4

Comment 6

[(no longer active)]

It's not clear to me what the use case for this API is exactly.  If it's the certified app just wanting to initiate a download on its own, why can't we use <a @download> for example?  If it's the certified app wanting to initiate a download on behalf of other pages (such as a web page loaded in a mozbrowser) then this is not the right way to do things because of the cookie and HTTP auth issue that Fabrice raised, and the issue is actually worse than not having the right cookie, the request will have the cookies of the certified app, which may leak unintended information to the server.  Can you please clarify what the use case is?  Note that Necko receives the appId for the app which initiates a network request, and that is how it decides which cookie jar to look at.

Also, looking at the patch, the isPrivate flag caught my attention as well.  What is that flag supposed to do?  Firefox OS doesn't currently support private browsing, and if we want to allow private browsing downloads, this is probably insufficient on its own.  What is the use case for private downloads without support for private browsing?

Comment 7

[Aus Lacroix [:aus]]

The specific case we want to cover here is the ability for apps to formally request a download via an api that enables specifying where and how the file is named. In the case of the browser app, it needs this to allow downloading content arbitrarily without the web page providing a way to do this. It also makes the download route through the fxos download manager so that the user can manage it.

Unfortunately, we cannot use the <a @download> trick. See https://bugzilla.mozilla.org/show_bug.cgi?id=982295#c5, it was the first thing we tried. I think we could get around this issue by injecting the <a @download> element in the actual mozbrowser displaying the content. This would enable that particular feature, but this doesn't cover the case of an app wanting to download something that is outside of it's origin.

The isPrivate flag is for future support of private downloads as part of private browsing in fxos. What it does right now is apply the private download flag to the underlying jsdownloads api and reflects this in the dom download object by have isPrivate be true. We're not sure what private browsing is going to look like, but this flag should be enough. I'm happy to add comments to note this fact.

Comment 8

[Aus Lacroix [:aus]]

Ehsan, Fabrice and I seem to be in agreement. Jonas, what do you think?

2:56:02 PM ehsan: can we just ipc into the right process and get it to start the download for us?
2:56:10 PM ehsan: (note the "just" 
2:56:28 PM auswerk: oh, man
2:56:35 PM auswerk: i hope you don't mean what i think you mean...
2:56:43 PM ehsan: I do...
2:56:52 PM ehsan: the thing is
2:56:53 PM auswerk: Ugh, gross. 
2:56:59 PM auswerk: but yes, i get why that works.
2:56:59 PM ehsan: the issue of cookies is not theoretical
2:57:10 PM ehsan: there are tons of sites which only let you download stuff after you log in
2:57:37 PM auswerk: so
2:57:42 PM ehsan: so I think we don't have a way out here
2:57:55 PM fabrice1: we could hook this up to the mozbrowser api instead
2:58:11 PM fabrice1: that would let us get the load context
2:58:15 PM auswerk: that would be so much easier
2:58:39 PM ehsan: and get the appId from the load context?
2:58:48 PM fabrice1: yeah
2:59:05 PM ehsan: but how do we lie to necko?
2:59:15 PM ehsan: let me check
2:59:17 PM auswerk: i think in this case we don't
2:59:19 PM fabrice1: no need to lie I think
2:59:23 PM ehsan: no?
2:59:31 PM auswerk: because we can get the docshell of the document of the content
2:59:36 PM ehsan: you need to stick the appId in your channel somehow right?
2:59:43 PM ehsan: auswerk: o_O
2:59:46 PM auswerk: which i can use to loadURI within the right context
2:59:58 PM fabrice1: ehsan: yes, you can set the loadcontext on the channel
3:00:03 PM ehsan: auswerk: I was assuming we don't have access to the docshell
3:00:16 PM ehsan: ok yeah that will work
3:00:24 PM ehsan: or, it's possible to make it work at least 
3:00:27 PM auswerk: if it was in the mozbrowserapi i would on the implementation side.
3:00:57 PM ehsan: actually forget what I said, the load context _is_ the docshell 
3:01:07 PM auswerk: sooo
3:01:25 PM auswerk: this feels good from a technical perspective and functionally it's awesome!
3:01:38 PM auswerk: but, is it weird to have a 'download' method on mozbrowser?
3:02:08 PM auswerk: it doesn't actually feel that odd to me

Comment 9

[(no longer active)]

Also, as I said later on IRC, I still think we should not do the isPrivate bit for now, and wait until we decide to implement proper support for PB.

Comment 10

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

Is the proposal to stick a download(url) function in the Browser API? I.e. make it some function on the <iframe mozbrowser>?

If so that sounds good to me.

Just make sure that we always use the cookiejar that the contents of the <iframe mozbrowser> would use.

This is likely something that the system app will need in order to allow triggering downloads from inside a particular app. So make it work for <iframe mozbrowser mozapp> as well!

Comment 11

[Aus Lacroix [:aus]]

(In reply to Jonas Sicking (:sicking) from comment #10)
> Is the proposal to stick a download(url) function in the Browser API? I.e.
> make it some function on the <iframe mozbrowser>?
> 
> If so that sounds good to me.

Yep, it sure is! I'll make it so!

> Just make sure that we always use the cookiejar that the contents of the
> <iframe mozbrowser> would use.

Yes, that's the main reason we're going to add it to the Browser API. It will give us access to the right load context to get the correct cookie jar set on the channel.

> This is likely something that the system app will need in order to allow
> triggering downloads from inside a particular app. So make it work for
> <iframe mozbrowser mozapp> as well!

Yep! Absolutely!

I'm hoping to have an updated patch early next week with this implemented.

Thanks everyone. :)

Comment 12

[Aus Lacroix [:aus]]

Updating Summary to reflect what will actually be done.

Comment 13

[Aus Lacroix [:aus]]

Attachment: Patch - v1 - Add 'download' method to Browser API

Comment 14

[Aus Lacroix [:aus]]

Comment on attachment 8423467 [details] [diff] [review]
Patch - v1 - Add 'download' method to Browser API

Note that the intention of the method is to use the cookie jar of the content window of the web page being displayed.

Comment 15

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 8423467 [details] [diff] [review]
Patch - v1 - Add 'download' method to Browser API

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

Nit: trailing white spaces.

We need to test this.

::: dom/browser-element/BrowserElementChildPreload.js
@@ +992,5 @@
>      webNav.stop(webNav.STOP_NETWORK);
>    },
>  
> +  _recvDownload: function(data) {
> +    let url = new content.URL(data.json.url);

It will throw here if the url is not valid.

I think we should make this API return a DOMRequest that fires onsuccess when the download finishes and onerror when the URL is invalid or channel error occurs, etc.

@@ +1040,5 @@
> +    // XXX We would set private browsing information prior to calling this.
> +    channel.notificationCallbacks = docShell.QueryInterface(Ci.nsIInterfaceRequestor);
> +
> +    // Since we're downloading our own local copy we'll want to bypass the
> +    // cache and local cache if the channel let's us specify this.

I don't understand why we would want to bypass the cache. Could you elaborate it in the comment?

Comment 16

[Aus Lacroix [:aus]]

(In reply to Kan-Ru Chen [:kanru] from comment #15)
> Comment on attachment 8423467 [details] [diff] [review]
> Patch - v1 - Add 'download' method to Browser API
> 
> Review of attachment 8423467 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nit: trailing white spaces.

Indeed, got rid of those.

> 
> We need to test this.
>

I will add a mochitest for this method.

> 
> ::: dom/browser-element/BrowserElementChildPreload.js
> @@ +992,5 @@
> >      webNav.stop(webNav.STOP_NETWORK);
> >    },
> >  
> > +  _recvDownload: function(data) {
> > +    let url = new content.URL(data.json.url);
> 
> It will throw here if the url is not valid.

There's a few things I should probably do here at the same time for the next patch.

* Catch invalid URL exceptions and reject the promise (or DOMRequest, see next talking point :)).
* Ensure that there is a filename. If not, reject.
* Ensure that there is a file extension (or forcibly add one via Mime Type Sniffing).

> 
> I think we should make this API return a DOMRequest that fires onsuccess
> when the download finishes and onerror when the URL is invalid or channel
> error occurs, etc.
> 

Would a Promise be acceptable here as well?

> @@ +1040,5 @@
> > +    // XXX We would set private browsing information prior to calling this.
> > +    channel.notificationCallbacks = docShell.QueryInterface(Ci.nsIInterfaceRequestor);
> > +
> > +    // Since we're downloading our own local copy we'll want to bypass the
> > +    // cache and local cache if the channel let's us specify this.
> 
> I don't understand why we would want to bypass the cache. Could you
> elaborate it in the comment?

It's the pattern that we follow in Firefox (Desktop, Android) when the user specifically requests downloading a file. I'm open to changing this but I opted to follow this standard in the initial implementation.

Comment 17

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Ghislain Aus Lacroix [:aus] from comment #16)
> > I think we should make this API return a DOMRequest that fires onsuccess
> > when the download finishes and onerror when the URL is invalid or channel
> > error occurs, etc.
> > 
> 
> Would a Promise be acceptable here as well?

I would like to use Promise as well. However, other mozbrowser APIs are still using DOMRequest so we shouldn't make one different :)

Comment 18

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

I'm totally fine with using Promise even though other functions on the same API uses DOMRequest.

Up to what gaia devs prefer really.

Comment 19

[Aus Lacroix [:aus]]

It'll be easier for the time being to keep the interface consistent, it will return a DOMRequest.

Comment 20

[Aus Lacroix [:aus]]

Attachment: Patch - v2 - Add 'download' method to Browser API

This patch omits a test for the new method. I will provide a follow-up patch with a mochitest for the new method.

This method executes in the parent to avoid needless back and forth between parent and child. This also makes all StreamListener methods work as expected which is what was required to fire success/error on the DOMRequest. The context for the connection is still the frame so the appropriate cookie jar is used.

Comment 21

[Aus Lacroix [:aus]]

Try run (with tests): https://tbpl.mozilla.org/?tree=Try&rev=d3b944aed4c1

Comment 22

[Aus Lacroix [:aus]]

Attachment: Patch - v2 - Add 'download' method to Browser API

Same as previous patch but with mochitests.

Comment 23

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 8434536 [details] [diff] [review]
Patch - v2 - Add 'download' method to Browser API

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

Please find someone to review the DownloadCore.jsm change.

Nit: trailing white spaces

You moved the download handling to the parent side. Would it still use the correct cookie jar if we initiate the download in parent?

Since we want the download to use the correct cookie jar, please test it in the test.

::: dom/browser-element/mochitest/browserElement_Download.js
@@ +1,4 @@
> +/* Any copyright is dedicated to the public domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +// Bug 983747 - FILL IN TEST DESCRIPTION

Fill in test description :)

@@ +8,5 @@
> +browserElementTestHelpers.setEnabledPref(true);
> +browserElementTestHelpers.addPermission();
> +
> +var iframe;
> +var downloadURL = 'http://test/tests/dom/browser-element/mochitest/file_bug983747.sjs';

Forgot to attach this file? Please rename it to file_download_bin.sjs and rename file_bug983747.bin to file_test.bin

@@ +16,5 @@
> +  SpecialPowers.wrap(iframe).mozbrowser = true;
> +
> +  document.body.appendChild(iframe);
> +
> +  var req = iframe.download(downloadURL, { filename: 'test.bin' });

You have to wait mozbrowserloadend here before you could call the mozbrowser API.

@@ +18,5 @@
> +  document.body.appendChild(iframe);
> +
> +  var req = iframe.download(downloadURL, { filename: 'test.bin' });
> +  req.onsuccess = function() {
> +    SimpleTest.finish();

Please add one ok(true, ...) before finish so it's obvious that test has actually been run.

Comment 24

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Ghislain Aus Lacroix [:aus] from comment #20)
> Created attachment 8434456 [details] [diff] [review]
> Patch - v2 - Add 'download' method to Browser API
> 
> This patch omits a test for the new method. I will provide a follow-up patch
> with a mochitest for the new method.
> 
> This method executes in the parent to avoid needless back and forth between
> parent and child. This also makes all StreamListener methods work as
> expected which is what was required to fire success/error on the DOMRequest.
> The context for the connection is still the frame so the appropriate cookie
> jar is used.

The context in the parent side is actually the embedder's so I think it doesn't use the appropriate cookie jar.

Comment 25

[Aus Lacroix [:aus]]

(In reply to Kan-Ru Chen [:kanru] from comment #24)
> (In reply to Ghislain Aus Lacroix [:aus] from comment #20)
> > Created attachment 8434456 [details] [diff] [review]
> > Patch - v2 - Add 'download' method to Browser API
> > 
> > This patch omits a test for the new method. I will provide a follow-up patch
> > with a mochitest for the new method.
> > 
> > This method executes in the parent to avoid needless back and forth between
> > parent and child. This also makes all StreamListener methods work as
> > expected which is what was required to fire success/error on the DOMRequest.
> > The context for the connection is still the frame so the appropriate cookie
> > jar is used.
> 
> The context in the parent side is actually the embedder's so I think it
> doesn't use the appropriate cookie jar.

It's a little confusing but ultimately it looked to me like the TabParent is what was used to determine context, in which case, doing it from the parent should be OK? I'll ask for help with verifying this tomorrow.

I'll also fix the tests tomorrow... It was definitely a rush job, I just wanted to get one run on try before you reviewed.

Comment 26

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Ghislain Aus Lacroix [:aus] from comment #25)
> (In reply to Kan-Ru Chen [:kanru] from comment #24)
> > (In reply to Ghislain Aus Lacroix [:aus] from comment #20)
> > > Created attachment 8434456 [details] [diff] [review]
> > > Patch - v2 - Add 'download' method to Browser API
> > > 
> > > This patch omits a test for the new method. I will provide a follow-up patch
> > > with a mochitest for the new method.
> > > 
> > > This method executes in the parent to avoid needless back and forth between
> > > parent and child. This also makes all StreamListener methods work as
> > > expected which is what was required to fire success/error on the DOMRequest.
> > > The context for the connection is still the frame so the appropriate cookie
> > > jar is used.
> > 
> > The context in the parent side is actually the embedder's so I think it
> > doesn't use the appropriate cookie jar.
> 
> It's a little confusing but ultimately it looked to me like the TabParent is
> what was used to determine context, in which case, doing it from the parent
> should be OK? I'll ask for help with verifying this tomorrow.

The |this._window| in BrowserElementParent is the parent window, usually the Browser app or System app in Gaia. They don't have a TabParent.

It seems you just have to use this._frameLoader as the load context. So

    let interfaceRequestor =
        this._frameLoader.loadContext.QueryInterface(Ci.nsIInterfaceRequestor);
    channel.notificationCallbacks = interfaceRequestor;

might work.

Comment 27

[Aus Lacroix [:aus]]

> The |this._window| in BrowserElementParent is the parent window, usually the
> Browser app or System app in Gaia. They don't have a TabParent.
> 
> It seems you just have to use this._frameLoader as the load context. So
> 
>     let interfaceRequestor =
>        
> this._frameLoader.loadContext.QueryInterface(Ci.nsIInterfaceRequestor);
>     channel.notificationCallbacks = interfaceRequestor;
> 
> might work.

Gotcha, that does seem to do the trick. I'll try and figure out how to determine in the test that that is also the case.

Comment 28

[Aus Lacroix [:aus]]

Attachment: Patch - v3 - Add 'download' method to Browser API

Review comments addressed. Confirmed proper usage of cookie jar as well. The only thing that is missing is somehow testing that the cookie jar we want is set.

Paolo, there is a small change to DownloadCore.jsm which I would like you to review as well before landing. :)

Comment 29

[Aus Lacroix [:aus]]

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

Comment 30

[:Paolo Amadini]

Comment on attachment 8435070 [details] [diff] [review]
Patch - v3 - Add 'download' method to Browser API

(In reply to Ghislain Aus Lacroix [:aus] from comment #28)
> Paolo, there is a small change to DownloadCore.jsm which I would like you to
> review as well before landing. :)

Looks straightforward, thanks!

There's actually a better syntax I would recommend here for catching selectively:

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/jsdownloads/src/DownloadCore.jsm#1969

Comment 31

[Aus Lacroix [:aus]]

(In reply to :Paolo Amadini from comment #30)
> Comment on attachment 8435070 [details] [diff] [review]
> Patch - v3 - Add 'download' method to Browser API
> 
> (In reply to Ghislain Aus Lacroix [:aus] from comment #28)
> > Paolo, there is a small change to DownloadCore.jsm which I would like you to
> > review as well before landing. :)
> 
> Looks straightforward, thanks!
> 
> There's actually a better syntax I would recommend here for catching
> selectively:
> 
> http://mxr.mozilla.org/mozilla-central/source/toolkit/components/jsdownloads/
> src/DownloadCore.jsm#1969

Ah yes! I'll update the syntax. Thanks!

Comment 32

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 8435070 [details] [diff] [review]
Patch - v3 - Add 'download' method to Browser API

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

Looks good.

Add file_download_bin.sjs to mochitest.ini

Comment 33

[Aus Lacroix [:aus]]

The test itself is exposing a flaw in HTTP channel diversion. I filed a follow-up bug to fix this issue during stabilization. In the meantime, the API method itself is usable, as expected when running under normal conditions in B2G and B2G Desktop (ergo Firefox should also work as expected).

Comment 34

[Aus Lacroix [:aus]]

Try Run: https://tbpl.mozilla.org/?tree=Try&rev=d638685893a2

Comment 35

[Ed Morley [:emorley]]

When you pushed to b2g-inbound, you accidentally pushed yet-to-be merged mozilla-central changesets there as well as the one changeset from this bug:
https://tbpl.mozilla.org/?tree=B2g-Inbound&rev=ceb784b90138

Those jobs were then cancelled after you pushed a backout of the changeset from this bug, however we still needed the jobs, since that push brought more than just 1 changeset. 

Please please please ask before mass-cancelling on non-try and also remember to use "hg outgoing" if ever in doubt as to what will be pushed (or to where) - since the results can often be quite problematic to resolve (eg worst-case merge conflicts or having to strip changesets). Thanks :-)

Comment 36

[Aki Sasaki (not active)]

This also broke vcs-sync with a non-ffwd push until :edmorley fixed it.

Comment 37

[Aus Lacroix [:aus]]

Pushed to b2g-inbound: https://hg.mozilla.org/integration/b2g-inbound/rev/b7d2b59e0eb9

Will get marked resolved fixed when it lands in it's final destination which is Aurora.

Comment 38

[Aus Lacroix [:aus]]

Comment on attachment 8435070 [details] [diff] [review]
Patch - v3 - Add 'download' method to Browser API

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 876376 982295
User impact if declined: Functionality that exists is broken under many scenarios
Testing completed (on m-c, etc.): try, b2g-inbound 
Risk to taking this patch (and alternatives if risky): low
String or IDL/UUID changes made by this patch: 'download' method added to Browser API. This is already tracked with keyword 'dev-doc-needed'.

Comment 39

[Aus Lacroix [:aus]]

Please see push log from https://bugzilla.mozilla.org/show_bug.cgi?id=983747#c37

Comment 40

[Aus Lacroix [:aus]]

Checkin needed on Aurora, see https://bugzilla.mozilla.org/show_bug.cgi?id=983747#c37 for changeset.

Comment 41

[Ryan VanderMeulen [:RyanVM]]

For perpetuity, this did land on m-c.
https://hg.mozilla.org/mozilla-central/rev/b7d2b59e0eb9

Comment 42

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c84a1d1b6612

Comment 43

[Aus Lacroix [:aus]]

Apologies... I somehow landed the wrong version of the patch on b2g-inbound. I've landed a follow-up here: https://hg.mozilla.org/integration/b2g-inbound/rev/ec628845dd82 

It will need merge to central and aurora. I didn't want to mess up the flags so I NI?'d you instead. I hope that's OK.

Comment 44

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

(In reply to Ghislain Aus Lacroix [:aus] from comment #43)
> Apologies... I somehow landed the wrong version of the patch on b2g-inbound.
> I've landed a follow-up here:
> https://hg.mozilla.org/integration/b2g-inbound/rev/ec628845dd82 
> 
> It will need merge to central and aurora. I didn't want to mess up the flags
> so I NI?'d you instead. I hope that's OK.

Can you attach that followup as a patch here and flag it for aurora approval?

Comment 45

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/ec628845dd82

Comment 46

[Aus Lacroix [:aus]]

Attachment: Patch - Correct mis-landing.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 983747
User impact if declined: Download method on Browser API is non-functional.
Testing completed (on m-c, etc.): On Flame device
Risk to taking this patch (and alternatives if risky): Entire method needs to be backed out from gecko and feature backed out from gaia.
String or IDL/UUID changes made by this patch: None.

Comment 48

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8443874 [details]
Patch - Correct mis-landing.

Aurora approval granted.

Comment 49

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b9875fb66979

Comment 50

[Chris Mills [:cmills]]

Documented:

https://developer.mozilla.org/en-US/docs/Web/API/HTMLIFrameElement/download