1245597 - Implement chrome.downloads.download()

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect)

Description

[Andrew Swan [:aswan]]

Part of supporting chrome.downloads in WebExtensions

https://developer.chrome.com/extensions/downloads#method-download

Comment 1

[Andrew Swan [:aswan]]

Attachment: wip on chrome.downloads.download()

Comment 2

[Andrew Swan [:aswan]]

A note about the attached patch for feedback: this is just an effort to get the basic mechanics of download() working.  We won't be able to support options.method, options.headers, or options.body without some changes to Downloads.jsm, I'll file a separate bug for that.  And options.saveAs should be straightforward but a non-trivial chunk of code, leaving that for a subsequent revision for now.

I think the implementation should be uncontroversial, the testing is where things get a little trickier.  Until we implement onChanged, we don't have a good way to know when a download has finished, so I used some timeout hacks for now, with the intention of coming back and fixing those when onChanged is done.

I started trying to use OS.File.stat() to actually check the size (and potentially other attributes like modification time) of the downloaded file, but I couldn't get it to work.  I left what I wrote commented out in the test file, if I'm missing something obvious to make it work, please let me know...

Comment 3

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8717997 [details] [diff] [review]
wip on chrome.downloads.download()

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

Hi Andrew,
follows an initial feedback on the attached patch:

::: toolkit/components/extensions/ext-downloads.js
@@ +27,5 @@
>      downloads: {
> +      download(options) {
> +        // First check for options that we don't yet implement.
> +        if (options.saveAs) {
> +          return Promise.reject({ message: "saveAs is not yet supported" });

No need to do this kind of parameter validation manually, thanks to the JSON schema we can just mark these properties as |"unsupported": true| and the parameters validation code will raise a nice Error message, e.g.:

    Error: Type error for parameter options (Property "saveAs" is unsupported by Firefox) for downloads.download.

@@ +46,5 @@
> +
> +          let target = new FileUtils.File(downloadsDir);
> +          if (options.filename != null) {
> +            if (options.filename.length == 0) {
> +              throw new context.cloneScope.Error("filename must not be empty");

This validation (and the following two) can be done in the beginning of the method before we get the preferred downloads directory (and return a rejected promise if needed).

@@ +68,5 @@
> +          }
> +
> +          if (target.exists()) {
> +            switch (options.conflictAction) {
> +              case null:

can we use "default:" instead of "case null:"?

@@ +78,5 @@
> +                break;
> +
> +              case "prompt":
> +                // TODO
> +                throw new context.cloneScope.Error("prompt not yet implemented");

instead of raising an error (which will catched and turned as a rejected Promise from the WebExtensions internals), how about returning a new Promise from our API method and directly resolve/reject it when needed? e.g.:

   download(options) {
     let error = validateFilename(options.filename);
     if (error) {
       return Promise.reject(error);
     }

     return new Promise((resolve, reject) => {
       Downloads.getPreferredDownloadsDirectory().then(downloadsDir => {
         ...
         if (target.exists()) {
           switch (options.conflicAction) {
           case "prompt":
             reject({ message: "..." });
             return;
           }
         }

         ...
       });

     });
   }

@@ +92,5 @@
> +        }).then(download => {
> +          download.tryToKeepPartialData = true;
> +          download.start();
> +
> +          return currentId++;

I'm wondering if on Chrome this "downloadId" associated with a downloaded file are:

- persisted between browser restarts
- associated to a downloaded files even if it wasn't this API to start the download

::: toolkit/components/extensions/schemas/downloads.json
@@ +284,5 @@
>                }
>              ]
>            }
> +        ],
> +        "async": "callback"

I would prefer if the "async" properties is near the function name (mainly to be able to spot it by looking at the function schema), eg.

   {
      "name": "download",
      "type": "function",
      "async": "callback"

::: toolkit/components/extensions/test/mochitest/mochitest.ini
@@ +30,5 @@
>  skip-if = buildapp == 'b2g' # runat != document_idle is not supported.
>  [test_ext_contentscript_create_iframe.html]
>  [test_ext_contentscript_api_injection.html]
>  [test_ext_downloads.html]
> +[test_ext_downloads_download.html]

The test_ext_downloads.html for the previous patch is basically a skeleton file for the real test cases,
probably we can merge this new test file into test_ext_downloads.html and defer the creation of new test files when the first one is starting to becoming too crowed (or if it increases the risk of intermittent failures)

::: toolkit/components/extensions/test/mochitest/test_ext_downloads_download.html
@@ +78,5 @@
> +function touch(file) {
> +  file.create(SpecialPowers.Ci.nsIFile.NORMAL_FILE_TYPE, FileUtils.PERMS_FILE);
> +}
> +
> +add_task(function* setup() {

it would be probably better to define the |setup| function and use it from the real test case (instead of passing it to the add_task).

@@ +79,5 @@
> +  file.create(SpecialPowers.Ci.nsIFile.NORMAL_FILE_TYPE, FileUtils.PERMS_FILE);
> +}
> +
> +add_task(function* setup() {
> +  SimpleTest.requestFlakyTimeout("this is temporary until we have download progress notification");

setTimeout usage in a test case highly discouraged, let's evaluate a couple of alternatives, follows a couple of ideas:

- use another internal service that is able to detect when the download has been completed (e.g. WebProgressListener?)

- partially implement the onChanged event (just as much is needed to be able to detect the end of the download process without using a setTimeout)
  e.g. the Download object seems to support an onchange property which can be helpful: https://dxr.mozilla.org/mozilla-central/source/toolkit/components/jsdownloads/src/DownloadCore.jsm#287)

@@ +87,5 @@
> +  downloadDir = FileUtils.getDir("TmpD", ["downloads"]);
> +  downloadDir.createUnique(nsIFile.DIRECTORY_TYPE, FileUtils.PERMS_DIRECTORY);
> +  info(`Using download directory ${downloadDir.path}.`);
> +
> +  SpecialPowers.Services.prefs.setIntPref("browser.download.folderList", 2);

You should be able to set preferences directly from the SpecialPowers API, e.g.:

     SpecialPowers.setIntPref("browser.download.folderList", 2);

The preferences you set for a test case should be cleaned up on exit, e.g.:

     SimpleTest.registerCleanupFunction(() => {  
       SpecialPowers.clearUserPref("browser.download.folderList");
     });

Comment 4

[Andrew Swan [:aswan]]

Thanks for the detailed and thoughtful feedback, I'll revise the patch.  A couple of things though:

(In reply to Luca Greco [:rpl] from comment #3)
> ::: toolkit/components/extensions/ext-downloads.js
> @@ +92,5 @@
> > +        }).then(download => {
> > +          download.tryToKeepPartialData = true;
> > +          download.start();
> > +
> > +          return currentId++;
> 
> I'm wondering if on Chrome this "downloadId" associated with a downloaded
> file are:
> 
> - persisted between browser restarts
> - associated to a downloaded files even if it wasn't this API to start the
> download

Right I'm trying to work on this in small chunks, this handling of ids is obviously inadequate for the long run, but until we have other methods implemented on chrome.downloads you can't actually do anything with an id.  My plan is to fill out the DownloadItem type and associated methods in subsequent revisions, at which point this code will change (though not radically I expect).

The issue about ids that persist across restarts is a separate one, we'll need changes to the Downloads module to support that, I plan to follow up on that separately.

> ::: toolkit/components/extensions/test/mochitest/mochitest.ini
> @@ +30,5 @@
> >  skip-if = buildapp == 'b2g' # runat != document_idle is not supported.
> >  [test_ext_contentscript_create_iframe.html]
> >  [test_ext_contentscript_api_injection.html]
> >  [test_ext_downloads.html]
> > +[test_ext_downloads_download.html]
> 
> The test_ext_downloads.html for the previous patch is basically a skeleton
> file for the real test cases,
> probably we can merge this new test file into test_ext_downloads.html and
> defer the creation of new test files when the first one is starting to
> becoming too crowed (or if it increases the risk of intermittent failures)

I actually started doing this and the communication between the extension and the test code felt clunky as it was trying to support two different types of tests.  Stylistically I would prefer having a larger number of more targeted test files rather than one huge one that tests all the different things that fall under chrome.downloads.  Are there reasons to prefer a single large test file that I'm overlooking?

> :::
> toolkit/components/extensions/test/mochitest/test_ext_downloads_download.html
> @@ +79,5 @@
> > +  file.create(SpecialPowers.Ci.nsIFile.NORMAL_FILE_TYPE, FileUtils.PERMS_FILE);
> > +}
> > +
> > +add_task(function* setup() {
> > +  SimpleTest.requestFlakyTimeout("this is temporary until we have download progress notification");
> 
> setTimeout usage in a test case highly discouraged, let's evaluate a couple
> of alternatives, follows a couple of ideas:

Again, I'm trying to tackle this in small chunks, and the long-term (by which I mean hopefully next week, not weeks or months from now) solution here is to use onChanged.  This is meant to be a temporary placeholder so I can work separately on onChanged and keep the individual patches modest in scope and  focused on a single thing.  Doing anything else right now feels like unnecessary throwaway work.

Comment 5

[Luca Greco [:rpl] [:luca] [:lgreco]]

(In reply to Andrew Swan [:aswan] from comment #4) 
> Again, I'm trying to tackle this in small chunks, and the long-term (by
> which I mean hopefully next week, not weeks or months from now) solution
> here is to use onChanged.  This is meant to be a temporary placeholder so I
> can work separately on onChanged and keep the individual patches modest in
> scope and  focused on a single thing.  Doing anything else right now feels
> like unnecessary throwaway work.

Have you tried nsIDownloadProgressListener[1]?

maybe it can be simple and useful enough to complete the "downloads.download" test scenario without
implementing onChanged.
  
[1]: https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIDownloadProgressListener

Comment 6

[Andrew Swan [:aswan]]

Attachment: implement the basics of chrome.downloads.download()

Comment 7

[Andrew Swan [:aswan]]

Filed new bugs for the documented options to download() that are not implemented by the attached patch:

Bug 1247791 - Implement options.saveAs for chrome.downloads.download()
Bug 1247793 - Implement POST for chrome.downloads.download()
Bug 1247794 - support persistent identifiers in chrome.downloads.*

Comment 8

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8718612 [details] [diff] [review]
implement the basics of chrome.downloads.download()

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

Thanks Andrew, I think this new version is almost ready.

Follow a couple of more considerations:

::: toolkit/components/extensions/ext-downloads.js
@@ +19,5 @@
>  const {
>    ignoreEvent,
>  } = ExtensionUtils;
>  
> +let currentId = 0;

As we don't keep track of the currentId associated to the download files and we don't yet have a query API method neither, we can probably just return a fake downloadId (e.g. -1?) until we really manage them.

@@ +42,5 @@
> +          }
> +        }
> +
> +        let download;
> +        return Downloads.getPreferredDownloadsDirectory().then(downloadsDir => {

I find this long chain of ".then" difficult to read, how about extracting them into helper functions and align the returned ".then" chain (with the dot aligned with the one of the previous line, bugzilla isn't very helpful with code snippets) e.g.:

        // downloadDirPath -> Promise<Download>
        function createDownload(downloadsDir) {
          ...
        }

        // (Download, DownloadList) -> (Download)
        function addDownloadToList(download, list) {
          list.add(download);
          return download;
        }

        // Download -> Promise<Download>
        function putIntoDownloadsList(download) {
          return Downloads.getList(Downloads.ALL)
                          .then((list) => addDownloadToList(download, list));
        }

        // Download -> DownloadId
        function startDownloadAndResolveDownloadId(download) {
          download.tryToKeepPartialData = true;
          download.start();

          return -1;
        }

        // () -> Promise<DownloadId>
        return Downloads.getPreferredDownloadsDirectory()
                        .then(createDownload)
                        .then(putIntoDownloadsList)
                        .then(startDownloadAndResolveDownloadId);

::: toolkit/components/extensions/test/mochitest/test_ext_downloads_download.html
@@ +19,5 @@
> +// This function is a bit of a sledgehammer, it looks at every download
> +// the browser knows about and waits for all active downloads to complete.
> +// But we only start one at a time and only do a handful in total, so
> +// this lets us test download() without depending on anything else.
> +function waitForDownloads() {

This is way better than resorting to setTimeout, great! Thanks Andrew.

@@ +33,5 @@
> +                  });
> +}
> +
> +
> +const BASE = "http://mochi.test:8888/tests/toolkit/components/extensions/test/mochitest";

I prefer constants to be declared on top of the test file to be easily recognizable (if in doubt look to the other test files in the same directory)

@@ +34,5 @@
> +}
> +
> +
> +const BASE = "http://mochi.test:8888/tests/toolkit/components/extensions/test/mochitest";
> +const SAMPLE_HTML_LEN = 120;

we are making assumptions about the "file_sample.html", which is an asset shared with other test cases, 
to be sure that this test will not break because when that file change in an unrelated patch, we probably should:

- create a separate asset file for our test case (e.g. a new "file_download.txt")
- or calculating the real file size (e.g. from nsIFile.fileSize)

@@ +36,5 @@
> +
> +const BASE = "http://mochi.test:8888/tests/toolkit/components/extensions/test/mochitest";
> +const SAMPLE_HTML_LEN = 120;
> +
> +function backgroundScript() {

I would move this near the |add_task| to be easily recognized when we look at the test case.

@@ +63,5 @@
> +  file.create(SpecialPowers.Ci.nsIFile.NORMAL_FILE_TYPE, FileUtils.PERMS_FILE);
> +}
> +
> +function* setup() {
> +  SimpleTest.requestFlakyTimeout("this is temporary until we have download progress notification");

Probably this is not needed anymore, right?

@@ +72,5 @@
> +  downloadDir.createUnique(nsIFile.DIRECTORY_TYPE, FileUtils.PERMS_DIRECTORY);
> +  info(`Using download directory ${downloadDir.path}.`);
> +
> +  SpecialPowers.setIntPref("browser.download.folderList", 2);
> +  // SpecialPowers.setComplexValue("browser.download.dir", nsIFile, downloadDir);

For some reason SpecialPowers.setComplexValue doesn't seem to work, and I've not found any other usage of it on dxr/mxr,
could you file a separate issue for it? (as it is not related to the WebExtension component)

@@ +102,5 @@
> +      permissions: ["downloads"],
> +    },
> +  };
> +
> +  let extension = ExtensionTestUtils.loadExtension(extensionData);

if you don't need |extensionData| object itself, just pass the object to |loadExtension|

@@ +130,5 @@
> +  yield extension.awaitMessage("ready");
> +  info("extension started");
> +
> +  // Call download() with just the url property.
> +  yield testDownload({ url: BASE + "/file_sample.html" },

This sequence of test cases seems a good candidate for a refactoring, how about turning them into something like the following (pseudocode)?:

  let testCases = [
    { 
      /* ... object which contains precondition, postcondition, actions, final assertion values/helper functions */ 
    },
    ...  
  ];

  for (let testCase of testCases) {
    let { ... } = testCase;
    info(/* test case description */);
    yield /* generator which run the test case actions and assertions*/
    info(/* test case done */);
  }

Comment 9

[Andrew Swan [:aswan]]

Attachment: implement the basics of chrome.downloads.download()

Comment 10

[Andrew Swan [:aswan]]

Thanks for the feedback, I made (most of) the suggested changes.

(In reply to Luca Greco [:rpl] from comment #8)
> ::: toolkit/components/extensions/ext-downloads.js
> @@ +42,5 @@
> > +          }
> > +        }
> > +
> > +        let download;
> > +        return Downloads.getPreferredDownloadsDirectory().then(downloadsDir => {
> 
> I find this long chain of ".then" difficult to read, how about extracting
> them into helper functions

I pulled out the code for determining the target file, which was the majority of that section.  Since what's left is now down to about 15 lines, I'd like to keep it as-is so that it reads linearly -- I find helper functions a hindrance to following the flow if they're not abstracting away something that is a logical unit.

> :::
> toolkit/components/extensions/test/mochitest/test_ext_downloads_download.html
> @@ +72,5 @@
> > +  downloadDir.createUnique(nsIFile.DIRECTORY_TYPE, FileUtils.PERMS_DIRECTORY);
> > +  info(`Using download directory ${downloadDir.path}.`);
> > +
> > +  SpecialPowers.setIntPref("browser.download.folderList", 2);
> > +  // SpecialPowers.setComplexValue("browser.download.dir", nsIFile, downloadDir);
> 
> For some reason SpecialPowers.setComplexValue doesn't seem to work, and I've
> not found any other usage of it on dxr/mxr,
> could you file a separate issue for it? (as it is not related to the
> WebExtension component)

There's already an open bug, I added a comment referencing it.

> @@ +130,5 @@
> > +  yield extension.awaitMessage("ready");
> > +  info("extension started");
> > +
> > +  // Call download() with just the url property.
> > +  yield testDownload({ url: BASE + "/file_sample.html" },
> 
> This sequence of test cases seems a good candidate for a refactoring, how
> about turning them into something like the following (pseudocode)?:
> 
>   let testCases = [
>     { 
>       /* ... object which contains precondition, postcondition, actions,
> final assertion values/helper functions */ 
>     },
>     ...  
>   ];
> 
>   for (let testCase of testCases) {
>     let { ... } = testCase;
>     info(/* test case description */);
>     yield /* generator which run the test case actions and assertions*/
>     info(/* test case done */);
>   }

Hm, I think we have a difference in taste here, I find the way it is currently written much easier to follow than what's proposed above.  Common code for download() and testDownload() has already been factored out.  The bits for testing different conflictAction options are ad hoc so they don't feel like a good candidate for further refactoring.

Comment 11

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8719674 [details] [diff] [review]
implement the basics of chrome.downloads.download()

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

Thanks Andrew, don't worry about the different in taste, my feedback or review comments do not always needs to be intented as mandatory changes ;-)
(sometimes can be just a starting point for a discussion or for a different kind of final solution)

Unfortunately, we have to tweak this patch further because the test file does not work with e10s enabled. 
(I'm sorry that I didn't pointed out before that the test cases need to be tested with e10s enabled and disabled to ensure that new test cases don't break on e10s once landed).

To run a test file with e10s enabled, you have to use the "--e10s" options in the mochitest command line, e.g.:

      ./mach mochitest toolkit/components/extensions/test/mochitest/test_ext_downloads_download.html --e10s

The issue on e10s are related to the following support methods in the test file:

- setup: |SpecialPowers.Services.prefs.setComplexValue("browser.download.dir", nsIFile, downloadDir)| raises an exception when e10s is enabled
- waitForDownloads: it seems that |Downloads.getList| is not able to get the expected results on e10s

I've tried to look into it a bit and the only workaround that I found is based on |SpecialPowers.loadChromeScript| to move the above two test helpers into a chromescript
and wait for a defined message from it (which means that the chromescript has completed its job and you can continue with the test cases), more on these in the following inline comments.

::: toolkit/components/extensions/test/mochitest/test_ext_downloads_download.html
@@ +16,5 @@
> +let { FileUtils } = SpecialPowers.Cu.import("resource://gre/modules/FileUtils.jsm");
> +let { Downloads } = SpecialPowers.Cu.import("resource://gre/modules/Downloads.jsm");
> +
> +const BASE = "http://mochi.test:8888/tests/toolkit/components/extensions/test/mochitest";
> +const FILE_LEN = 46;

how about defining as a constant the download file name as well? 
(e.g. const FILE_NAME = "file_download.txt")

I think it would be helpful to immediately recognize that FILE_LEN is related to above file without looking into the test case.

@@ +22,5 @@
> +// This function is a bit of a sledgehammer, it looks at every download
> +// the browser knows about and waits for all active downloads to complete.
> +// But we only start one at a time and only do a handful in total, so
> +// this lets us test download() without depending on anything else.
> +function waitForDownloads() {

This is how I changed |waitForDownloads| to make it work on e10s:

  function waitForDownloads() {
     return new Promise((resolve) => {
       let chromeScriptURL = SimpleTest.getTestFileURL("file_chromescript_wait_downloads_completed.js");
       let chromeScript = SpecialPowers.loadChromeScript(chromeScriptURL);
       let waitDownloadsCompleted = () => {
         chromeScript.removeMessageListener("wait-downloads-completed.request", waitDownloadsCompleted);
         resolve();
       }
       chromeScript.addMessageListener("wait-downloads-completed.done", waitDownloadsCompleted);
       chromeScript.sendAsyncMessage("wait-downloads-completed.request");
     });
  }

and the real implementation moved in the "file_chromescript_wait_downloads_completed.js" file:

  const Cu = Components.utils;
  const { Downloads } = Cu.import("resource://gre/modules/Downloads.jsm");

  addMessageListener("wait-downloads-completed.request", () => {
    Downloads.getList(Downloads.ALL)
             .then(list => list.getAll())
             .then(downloads => {
               let inprogress = downloads.filter(dl => !dl.stopped);
               Promise.all(inprogress.map(dl => dl.whenSucceeded()))
                      .then(() => sendAsyncMessage("wait-downloads-completed.done"));
             });
  });

@@ +49,5 @@
> +function touch(file) {
> +  file.create(SpecialPowers.Ci.nsIFile.NORMAL_FILE_TYPE, FileUtils.PERMS_FILE);
> +}
> +
> +function* setup() {

Side note: This function doesn't use |yield|, it doesn't need to be a generator.

Follows how I changed this |setup| function to make it work with e10s enabled:

  function setup() {
    ... 
    return new Promise((resolve) => {
      let chromeScriptURL = SimpleTest.getTestFileURL("file_chromescript_set_download_dir.js");
      let chromeScript = SpecialPowers.loadChromeScript(chromeScriptURL);
      let waitForDownloadDirSet = () => {
        chromeScript.removeMessageListener("download-dir-set.request", waitForDownloadDirSet);
        resolve();
      }
      chromeScript.addMessageListener("download-dir-set.done", waitForDownloadDirSet);
      chromeScript.sendAsyncMessage("download-dir-set.request", { path: downloadDir.path });
    });
  }

And the following code moved in the new "file_chromescript_set_download_dir.js" file:

  const {
    interfaces: Ci,
    utils: Cu,
    classes: Cc,
  } = Components;

  Cu.import("resource://gre/modules/Services.jsm");

  addMessageListener("download-dir-set.request", (msg) => {
    let downloadDir = Cc["@mozilla.org/file/local;1"].createInstance(Ci.nsILocalFile);
    downloadDir.initWithPath(msg.path);
    Services.prefs.setComplexValue("browser.download.dir", Ci.nsIFile, downloadDir);
    sendAsyncMessage("download-dir-set.done");
  });

Comment 12

[Andrew Swan [:aswan]]

Attachment: implement the basics of chrome.downloads.download()

Comment 13

[Andrew Swan [:aswan]]

Thanks for pointing me in the right direction with e10s, but I had to make much more extensive changes than those from comment 11 since all file i/o (ie creating the download directory, checking downloaded file sizes) needed to move out of the content process.
Anyhow, this is passing now with --e10s and hopefully ready to go.

Comment 14

[Andrew Swan [:aswan]]

Attachment: implement the basics of chrome.downloads.download()

once more, including all the newly added files this time.

Comment 15

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8720063 [details] [diff] [review]
implement the basics of chrome.downloads.download()

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

Thanks Andrew, your new chromescript is definitely better than mine, good job!

I think that we are almost there, there are just a couple of more things:

- "mach eslint" on my machine still raises a number of errors due to the spaces around objects ("{ prop: 123 }" should be "{prop: 123}"), e.g.
  
     58:62   error  There should be no space after '{'   object-curly-spacing

  doesn't it raise these errors on your installation? have you pushed this patch on try? (eslint is now running of every try push and merge)

- I'd like to cleanup the test case a little bit to make it more readable (some ideas in the inline comments).

::: toolkit/components/extensions/test/mochitest/.eslintrc
@@ +5,5 @@
>      "webextensions": true,
>    },
>  
>    "globals": {
> +    "addMessageListener": false,

adding addMessageListener to the known globals for the test files makes sense, but as this file is shared with the other test files, it is probably something that worth its own issue on bugzilla.

In the mean time you can fix the eslint validation error on this global using an "eslint comment" in your file.

::: toolkit/components/extensions/test/mochitest/file_chromescript_downloads.js
@@ +1,2 @@
> +"use strict";
> +

We can use the eslint inline comment configuration here, e.g.:
 
     /* global addMessageListener */

::: toolkit/components/extensions/test/mochitest/test_ext_downloads_download.html
@@ +74,5 @@
> +  });
> +
> +  function download(options) {
> +    extension.sendMessage("download", options);
> +    return extension.awaitMessage("okay");

How about changing this message name from "okay" into something more specific? (e.g. "download-done", "download-completed")

One of the reasons why I prefer a different name is because "okay" makes me think about a successful completed operation,
but it is used to carry both success or failure results.

@@ +77,5 @@
> +    extension.sendMessage("download", options);
> +    return extension.awaitMessage("okay");
> +  }
> +
> +  function testDownload(options, localFile, expectedSize, description) {

I have the feeling that if we merge the |download| and |testDownload| helpers functions together we can make this test case more readable, e.g. if we turn the helper function into something like the following:

  function testDownload(description, options, expectedScenario) {
    let { localFile, fileSize, errorMessage } = expectedScenario;
    info(`test downloads.download() with ${description}`);

    extension.sendMessage("download", options);

    return extension.awaitMessage("okay").then(msg => {
      if (fileSize) {
        if (msg.status == "error") {
          ok(false, msg.errmsg);
          return;
        }

        ok(true, `downloads.download() should complete successfully with ${description}`);

        return runInChrome("check-download", { localFile, fileSize });
      } else if (errorMessage) {
        if (msg.status != "error") {
          ok(false, `downloads.download() should fail with ${description}`);
          return;
        }

        is(msg.errmsg, errorMessage, `download.download() should fail with the expected error message`);
      }
      return msg;
    });
  }

and then, tweaking the test scenarios sequence into:

  // Call download() with just the url property.
  yield testDownload("just source", { url: FILE_URL },
                     { localFile: FILE_NAME, fileSize: FILE_LEN });

  ...
  // Check conflictAction of "uniquify".
  yield runInChrome("touch", FILE_NAME);
  yield testDownload("conflictAction=uniquify", { url: FILE_URL, conflictAction: "uniquify" },
                     { localFile: FILE_NAME_UNIQUE, fileSize: FILE_LEN });
  yield runInChrome("remove", FILE_NAME);
  ...
  // Try to download to an empty path.
  yield testDownload("empty filename", { url: FILE_URL, filename: ""},
                     { errorMessage: "filename must not be empty"});
  ...

Comment 16

[Andrew Swan [:aswan]]

(In reply to Luca Greco [:rpl] from comment #15)
> - "mach eslint" on my machine still raises a number of errors due to the
> spaces around objects ("{ prop: 123 }" should be "{prop: 123}"), e.g.
>   
>      58:62   error  There should be no space after '{'   object-curly-spacing
> 
>   doesn't it raise these errors on your installation? have you pushed this
> patch on try? (eslint is now running of every try push and merge)

Ah, my local branch is now so old that new eslint options have been added.  I just rebased and fixed those errors.

> :::
> toolkit/components/extensions/test/mochitest/test_ext_downloads_download.html
> @@ +74,5 @@
> > +  });
> > +
> > +  function download(options) {
> > +    extension.sendMessage("download", options);
> > +    return extension.awaitMessage("okay");
> 
> How about changing this message name from "okay" into something more
> specific? (e.g. "download-done", "download-completed")
> 
> One of the reasons why I prefer a different name is because "okay" makes me
> think about a successful completed operation,
> but it is used to carry both success or failure results.

Right, I changed it to use "download.request" and "download.done", so it now matches the naming used for messages passed to and from the chrome script.

> I have the feeling that if we merge the |download| and |testDownload|
> helpers functions together we can make this test case more readable,

I'm not keen on doing that -- |download| is also used for tests that check the handling of bad inputs and I think that putting both the happy-path and error-path test checking code into one big function would reduce, rather than improve readability.

New patch addressing the other comments coming momentarily.

Comment 17

[Andrew Swan [:aswan]]

Attachment: implement the basics of chrome.downloads.download()

Comment 18

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8720564 [details] [diff] [review]
implement the basics of chrome.downloads.download()

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

Hi Andrew,there are just two things from the following comments, that I noticed in this last review and that, in my opinion, are important enough and have to be worked out (or at least explicitly discussed) before landing this:

- the one about the removing of all the remaining files from the temporary downloads dir
- and the one about clearing of the custom preferences

Besides these two above, all the other comments are small nits, one coding styles rules to fix and a convenient change to be sure that unexpected errors are collected during downloads will be reported in the final report of test failures.

Could you then push the final patch to try and attach the link to treeherder.mozilla.org in a comment, before you proceed to landing?

::: toolkit/components/extensions/test/mochitest/file_chromescript_downloads.js
@@ +11,5 @@
> +Cu.import("resource://gre/modules/Downloads.jsm");
> +
> +let downloadDir;
> +
> +/* global addMessageListener */

nit: I would move this to the head (just under "use strict") to be immediatelly visible (it doesn't make any difference for eslint)

@@ +35,5 @@
> +    let entry = entries.getNext();
> +    entry.QueryInterface(Ci.nsIFile);
> +    success = false;
> +    message = `Leftover file ${entry.path} in download directory`;
> +    entry.remove(false);

I'm a bit scared by this "cleanup all the file in the downloadDir".
it shouldn't be a problem because we are setting it explicitly as an unique named dir, 
but if I follow the test correctly, it shouldn't leave any files in the download dir, so maybe we can just warn about the leftover without removing them.

what do you think? do you see any problem in don't cleanup the remaining files in case of test running issues?

@@ +38,5 @@
> +    message = `Leftover file ${entry.path} in download directory`;
> +    entry.remove(false);
> +  }
> +
> +  downloadDir.remove(false);

If we decide to change this method as described above, this will probably throw if there're still files in the temporary downloads directory.

@@ +42,5 @@
> +  downloadDir.remove(false);
> +  downloadDir = null;
> +
> +  Services.prefs.clearUserPref("browser.download.folderList");
> +  Services.prefs.clearUserPref("browser.download.dir");

If because of some issues in the test run, we fail to clear the user preferences after the test case, we risk that the changed preferences will make other unrelated tests to fail.

Probably it is better to clean them from the directly from the |SimpleTest.registerCleanupFunction| callback in the test file (using |SpecialPowers.Services.prefs|), 
just for extra-safety.

::: toolkit/components/extensions/test/mochitest/test_ext_downloads_download.html
@@ +19,5 @@
> +const FILE_NAME_UNIQUE = "file_download(1).txt";
> +const FILE_LEN = 46;
> +
> +let chromeScript;
> +function runInChrome(msg, params) {

nit: put an empty line between "let chromeScript;" and "function runInChrome(...)"

@@ +21,5 @@
> +
> +let chromeScript;
> +function runInChrome(msg, params) {
> +  const requestMsg = msg + ".request";
> +  const doneMsg = msg + ".done";

nit: change these to into `${msg}.request` and `${msg}.done`

@@ +79,5 @@
> +  }
> +
> +  function testDownload(options, localFile, expectedSize, description) {
> +    return download(options).then(msg => {
> +      if (msg.status == "error") { info(msg.errmsg); }

for coding styles reasons this cannot be a oneline.

another thing is that in case of failures this info will not be collected and reported in the final report and we will need to search for it in the logs.

e.g. one strategy to make it be collected and reported as a failure in the final report:

   if (msg.status == "error") {
     is(msg.errmsg, undefined, "unexpected error on download");
   }

Comment 19

[Andrew Swan [:aswan]]

(In reply to Luca Greco [:rpl] from comment #18)
> I'm a bit scared by this "cleanup all the file in the downloadDir".
> it shouldn't be a problem because we are setting it explicitly as an unique
> named dir, 
> but if I follow the test correctly, it shouldn't leave any files in the
> download dir, so maybe we can just warn about the leftover without removing
> them.
> 
> what do you think? do you see any problem in don't cleanup the remaining
> files in case of test running issues?

Well as you point out, the download directory is created specifically for this test -- I don't know if there's a larger pattern for other tests, but it seems to me like a test should never leave any files around, or else a repeated series of failed runs could leave a bunch of stranded files behind.  If that eventually leads to a full disk or something, its going to be extraordinarily tedious for somebody to work back to the original source of the problem.

I think the only argument for leaving the files behind would be as an aid to somebody trying to debug a test failure, my preference would be to have that person just tweak the test locally to save the files rather than making that the default behavior for everybody.

I'll make the rest of the changes and push to try.

Comment 20

[Andrew Swan [:aswan]]

(In reply to Luca Greco [:rpl] from comment #18)
> :::
> toolkit/components/extensions/test/mochitest/test_ext_downloads_download.html
> @@ +79,5 @@
> > +  }
> > +
> > +  function testDownload(options, localFile, expectedSize, description) {
> > +    return download(options).then(msg => {
> > +      if (msg.status == "error") { info(msg.errmsg); }
> 
> for coding styles reasons this cannot be a oneline.
> 
> another thing is that in case of failures this info will not be collected
> and reported in the final report and we will need to search for it in the
> logs.
> 
> e.g. one strategy to make it be collected and reported as a failure in the
> final report:
> 
>    if (msg.status == "error") {
>      is(msg.errmsg, undefined, "unexpected error on download");
>    }

Oops, this was just me doing some debugging and neglecting to take it out before posting the patch, I'll just take it out.

Comment 21

[Andrew Swan [:aswan]]

Attachment: implement the basics of chrome.downloads.download()

Comment 22

[Andrew Swan [:aswan]]

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

Comment 23

[Andrew Swan [:aswan]]

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

Comment 24

[Andrew Swan [:aswan]]

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

Comment 25

[Andrew Swan [:aswan]]

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

Comment 26

[Andrew Swan [:aswan]]

Attachment: implement the basics of chrome.downloads.download()

Comment 27

[Kris Maglione [:kmag]]

Comment on attachment 8721375 [details] [diff] [review]
implement the basics of chrome.downloads.download()

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

::: browser/components/extensions/test/browser/browser_ext_runtime_setUninstallURL.js
@@ +109,5 @@
>    info("uninstalled");
>  
>    let uninstalledTab = yield uninstallPromise;
>    isnot(uninstalledTab, null, "opened tab with uninstall url");
> +//  yield BrowserTestUtils.removeTab(uninstalledTab);

Why?

::: toolkit/components/extensions/ext-downloads.js
@@ +29,5 @@
> +          if (options.filename.length == 0) {
> +            return Promise.reject({message: "filename must not be empty"});
> +          }
> +
> +          if (("winIsAbsolute" in OS.Path) && OS.Path.winIsAbsolute(options.filename)) {

This is redundant with the next check.

@@ +55,5 @@
> +            let uri = NetUtil.newURI(options.url);
> +            let urlobj = uri.QueryInterface(Ci.nsIURL);
> +
> +            target.append(urlobj.fileName);
> +          }

Please use `OS.Path.join(downloadsDir, ...)` rather than the above.

@@ +57,5 @@
> +
> +            target.append(urlobj.fileName);
> +          }
> +
> +          if (target.exists()) {

Please use `OS.File.exists` instead.

Also, there's a race here if the target doesn't exist when we check, but does by the time we attempt to save the file. Probably worth fixing for now, but please add a comment.

@@ +69,5 @@
> +                break;
> +
> +              case "prompt":
> +                // TODO
> +                return Promise.reject({message: "prompt not yet implemented"});

I don't think `createDownload` will handle this in any useful way.

@@ +83,5 @@
> +            target: createTarget(downloadsDir),
> +          })
> +        ).then(dl => {
> +          download = dl;
> +          download.tryToKeepPartialData = true;

Why?

@@ +88,5 @@
> +          download.start();
> +
> +          return Downloads.getList(Downloads.ALL);
> +        }).then(list => {
> +          list.add(download);

We should add the download to the list before we start it, ideally.

@@ +91,5 @@
> +        }).then(list => {
> +          list.add(download);
> +          // Without other chrome.downloads methods, we can't actually
> +          // do anything with the id so just return a dummy value for now.
> +          return 0;

We should return a unique ID anyway, even if we don't do anything with it yet. It should be simple enough.

::: toolkit/components/extensions/test/mochitest/file_chromescript_downloads.js
@@ +22,5 @@
> +  downloadDir = FileUtils.getDir("TmpD", ["downloads"]);
> +  downloadDir.createUnique(Ci.nsIFile.DIRECTORY_TYPE, FileUtils.PERMS_DIRECTORY);
> +
> +  Services.prefs.setIntPref("browser.download.folderList", 2);
> +  Services.prefs.setComplexValue("browser.download.dir", Ci.nsIFile, downloadDir);

It's best to register a cleanup function immediately after making changes like this.

Ideally we should register a directory service provider to change the system downloads directory, though. This is fine for now, but can you add a comment to that effect?

@@ +32,5 @@
> +
> +  let entries = downloadDir.directoryEntries;
> +  while (entries.hasMoreElements()) {
> +    let entry = entries.getNext();
> +    entry.QueryInterface(Ci.nsIFile);

`let entry = entries.getNext().QueryInterface(Ci.nsIFile);

@@ +34,5 @@
> +  while (entries.hasMoreElements()) {
> +    let entry = entries.getNext();
> +    entry.QueryInterface(Ci.nsIFile);
> +    success = false;
> +    message = `Leftover file ${entry.path} in download directory`;

Just use 'ok(false, `Leftover file ${entry.path} in download directory`)' rather than the message and success variables.

::: toolkit/components/extensions/test/mochitest/test_ext_downloads_download.html
@@ +41,5 @@
> +}
> +
> +function setup() {
> +  let chromeScriptURL = SimpleTest.getTestFileURL("file_chromescript_downloads.js");
> +  chromeScript = SpecialPowers.loadChromeScript(chromeScriptURL);

Let's just make this a chrome mochitest rather than trying to split things this way. We don't gain anything from having this run in a content scope.

Just add a line to the moz.build file like the following:

MOCHITEST_CHROME_MANIFESTS += ['test/mochitest/chrome.ini']

and create a chrome.ini like mochitest.ini to register the test file.

@@ +69,5 @@
> +add_task(function* test_downloads() {
> +  yield setup();
> +
> +  let extension = ExtensionTestUtils.loadExtension({
> +    background: "(" + backgroundScript.toString() + ")()",

Please don't use `.toString()`

@@ +128,5 @@
> +    is(msg.errmsg, "filename must not be empty", "error message for empty filename is correct");
> +  });
> +
> +  // Try to download to an absolute path.
> +  const root = WINDOWS ? "\\" : "/";

The windows variant needs a drive letter, or two backslashes. We really shouldn't accept paths with a leading backslash either way, though...

@@ +140,5 @@
> +
> +  if (WINDOWS) {
> +    yield download({
> +      url: FILE_URL,
> +      filename: "C:\\file_download.txt",

I'm not sure why this is necessary in addition to the above...

@@ +150,5 @@
> +
> +  // Try to download to a relative path containing ..
> +  yield download({
> +    url: FILE_URL,
> +    filename: OS.Path.join("..", "file_download.txt"),

We should also test when the ".." is not the first component, e.g., "foo/../../bar"

Comment 28

[Andrew Swan [:aswan]]

Attachment: implement the basics of chrome.downloads.download()

Comment 29

[Kris Maglione [:kmag]]

Comment on attachment 8721560 [details] [diff] [review]
implement the basics of chrome.downloads.download()

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

::: toolkit/components/extensions/ext-downloads.js
@@ +52,5 @@
> +          if (options.filename) {
> +            target = OS.Path.join(downloadsDir, options.filename);
> +          } else {
> +            let uri = NetUtil.newURI(options.url);
> +            let urlobj = uri.QueryInterface(Ci.nsIURL);

`let uri = NetUtil.newURI(options.url).QueryInterface(Ci.nsIURL)`

Sorry, I should have mentioned this last time, but either of these can throw if the URL isn't valid, or is a protocol like "about:", which we should handle.

We also need to make sure the extension is allowed to load the URL. `context.checkLoadURL()` should do for that.

Or, adding `"format": "url"` to that property in the schema should take care of both.

@@ +61,5 @@
> +          // This has a race, something else could come along and create
> +          // the file between this test and them time the download code
> +          // creates the target file.  But we can't easily fix it without
> +          // modifying DownloadCore so we live with it for now.
> +          if (OS.File.exists(target)) {

This returns a promise, so it will always evaluate as true.

The easiest way to handle this is to just wrap most of this method in a `Task.spawn` call, so you can use `yield` for promises, rather than using the promise chaining below.

@@ +87,5 @@
> +          return Downloads.getList(Downloads.ALL);
> +        }).then(list => {
> +          list.add(download);
> +
> +          download.tryToKeepPartialData = true;

Again, why?

@@ +92,5 @@
> +          download.start();
> +
> +          // Without other chrome.downloads methods, we can't actually
> +          // do anything with the id so just return a dummy value for now.
> +          return 0;

I still think we should be returning a unique ID for each download.

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_downloads_download.html
@@ +33,5 @@
> +const FILE_URL = BASE + "/" + FILE_NAME;
> +const FILE_NAME_UNIQUE = "file_download(1).txt";
> +const FILE_LEN = 46;
> +
> +SimpleTest.requestCompleteLog();

This is fine for debugging, but please remove it before landing.

@@ +168,5 @@
> +
> +  if (WINDOWS) {
> +    yield download({
> +      url: FILE_URL,
> +      filename: "C:\\file_download.txt",

I still don't understand why this is necessary. It doesn't look any different from the above test to me.

@@ +206,5 @@
> +    entry.remove(false);
> +  }
> +
> +  downloadDir.remove(false);
> +  return Promise.resolve();

This isn't necessary if you make this a star function, which you should always do for test tasks in any case.

Comment 30

[Andrew Swan [:aswan]]

(In reply to Kris Maglione [:kmag] from comment #29)
> ::: toolkit/components/extensions/ext-downloads.js
> @@ +61,5 @@
> > +          // This has a race, something else could come along and create
> > +          // the file between this test and them time the download code
> > +          // creates the target file.  But we can't easily fix it without
> > +          // modifying DownloadCore so we live with it for now.
> > +          if (OS.File.exists(target)) {
> 
> This returns a promise, so it will always evaluate as true.

I don't think so:
https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/OSFile.jsm/OS.File_for_workers#OS.File.exists%28%29

https://dxr.mozilla.org/mozilla-central/source/toolkit/components/osfile/modules/osfile_unix_front.jsm#338

> @@ +87,5 @@
> > +          return Downloads.getList(Downloads.ALL);
> > +        }).then(list => {
> > +          list.add(download);
> > +
> > +          download.tryToKeepPartialData = true;
> 
> Again, why?

Same as before, so we can eventually pause and resume.

> :::
> toolkit/components/extensions/test/mochitest/
> test_chrome_ext_downloads_download.html
> @@ +168,5 @@
> > +
> > +  if (WINDOWS) {
> > +    yield download({
> > +      url: FILE_URL,
> > +      filename: "C:\\file_download.txt",
> 
> I still don't understand why this is necessary. It doesn't look any
> different from the above test to me.

I was trying to create an absolute path without the drive letter, fixed in the version I'll push shortly.

Comment 31

[Kris Maglione [:kmag]]

(In reply to Andrew Swan [:aswan] from comment #30)
> (In reply to Kris Maglione [:kmag] from comment #29)
> > ::: toolkit/components/extensions/ext-downloads.js
> > @@ +61,5 @@
> > > +          // This has a race, something else could come along and create
> > > +          // the file between this test and them time the download code
> > > +          // creates the target file.  But we can't easily fix it without
> > > +          // modifying DownloadCore so we live with it for now.
> > > +          if (OS.File.exists(target)) {
> > 
> > This returns a promise, so it will always evaluate as true.
> 
> I don't think so:
> https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/
> OSFile.jsm/OS.File_for_workers#OS.File.exists%28%29

OS.File methods are only synchronous in Workers. In the main thread, they
return promises:

https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/OSFile.jsm/OS.File_for_the_main_thread#OS.File.exists()

> Same as before, so we can eventually pause and resume.

OK. Please add a comment to that effect.

Comment 32

[Andrew Swan [:aswan]]

Attachment: implement the basics of chrome.downloads.download()

Comment 33

[Kris Maglione [:kmag]]

Comment on attachment 8722091 [details] [diff] [review]
implement the basics of chrome.downloads.download()

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

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_downloads_download.html
@@ +162,5 @@
> +    is(msg.errmsg, "filename must not be empty", "error message for empty filename is correct");
> +  });
> +
> +  // Try to download to an absolute path.
> +  const absolutePath = OS.Path.join(WINDOWS ? "\\tmp" : "/tmp", "file_download.txt");

Why are we back to this again? And why still a separate testcase for Windows?

Comment 34

[Andrew Swan [:aswan]]

(In reply to Kris Maglione [:kmag] from comment #33)
> :::
> toolkit/components/extensions/test/mochitest/
> test_chrome_ext_downloads_download.html
> @@ +162,5 @@
> > +    is(msg.errmsg, "filename must not be empty", "error message for empty filename is correct");
> > +  });
> > +
> > +  // Try to download to an absolute path.
> > +  const absolutePath = OS.Path.join(WINDOWS ? "\\tmp" : "/tmp", "file_download.txt");
> 
> Why are we back to this again? And why still a separate testcase for Windows?

I wanted to cover absolute paths both with and without a drive letter.  Do you think that's excessive?
And for the second qeustion, the "with a drive letter" case obviously doesn't apply on non-windows platforms so that test case is only run on windows.

Comment 35

[Kris Maglione [:kmag]]

Comment on attachment 8722091 [details] [diff] [review]
implement the basics of chrome.downloads.download()

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

(In reply to Andrew Swan [:aswan] from comment #34)
> I wanted to cover absolute paths both with and without a drive letter.  Do
> you think that's excessive?
> And for the second qeustion, the "with a drive letter" case obviously
> doesn't apply on non-windows platforms so that test case is only run on
> windows.

I thought your version that used tmpDir was better, but I guess this is fine
too, if the tests pass.

Comment 36

[Andrew Swan [:aswan]]

(In reply to Kris Maglione [:kmag] from comment #35)
> I thought your version that used tmpDir was better, but I guess this is fine
> too, if the tests pass.

Yeah I did too, but tmpDir has an explicit drive letter.  So if that was the cross-platform test it would be with a drive letter on windows, without on other platforms, and the windows-specific test would be "absolute path without a drive letter".  That all felt clunkier than doing it this way.

Comment 37

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/418bf315261c

Comment 38

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for failing M(c) tests on Android:

Backout: https://hg.mozilla.org/integration/fx-team/rev/f5a59776ddd3

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=f3dcf982a76f
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=7516336&repo=fx-team

08:39:03     INFO -  260 INFO TEST-START | toolkit/components/extensions/test/mochitest/test_chrome_ext_downloads_download.html
08:44:16     INFO -  261 INFO Using download directory /data/data/org.mozilla.fennec/app_tmpdir/downloads
08:44:16     INFO -  262 INFO Extension loaded
08:44:16     INFO -  263 INFO TEST-UNEXPECTED-FAIL | toolkit/components/extensions/test/mochitest/test_chrome_ext_downloads_download.html | Test timed out.
08:44:16     INFO -  reportError@chrome://mochikit/content/tests/SimpleTest/TestRunner.js:114:7
08:44:16     INFO -  TestRunner._checkForHangs@chrome://mochikit/content/tests/SimpleTest/TestRunner.js:134:7

Comment 39

[Kris Maglione [:kmag]]

You just need to add `skip-if = os == 'android'` to chrome.ini

Comment 40

[Andrew Swan [:aswan]]

Attachment: implement the basics of chrome.downloads.download()

Comment 41

[Andrew Swan [:aswan]]

Comment on attachment 8723160 [details] [diff] [review]
implement the basics of chrome.downloads.download()

Revised patch, we skip Android for now

Comment 42

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/5b53d1dcb009

Comment 43

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

https://hg.mozilla.org/mozilla-central/rev/5b53d1dcb009