1245603 - Implement chrome.downloads.search()

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect)

Description

[Andrew Swan [:aswan]]

Part of supporting chrome.downloads in WebExtensions

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

Comment 1

[Andrew Swan [:aswan]]

Attachment: MozReview Request: bug 1245603 - Implement browser.downloads.search() r=kmag

Review commit: https://reviewboard.mozilla.org/r/36223/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/36223/

Comment 2

[Andrew Swan [:aswan]]

Eh, I'm stumbling my way around reviewboard.  I meant for the request to be f?, not r?.  In particular, the test is totally inadequate, I just wanted to get some feedback on the actual implementation of DownloadItem and search.  Once we've got something we like there, I'll fill out the tests before asking for a final review.
Also, this patch should be applied on top of the not-yet-landed patch for bug 1245597.

Comment 3

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/36223/#review32793

::: toolkit/components/extensions/ext-downloads.js:91
(Diff revision 1)
> +  // fields into simple properties (instead of getters).

Please use JSDoc-style comments.

::: toolkit/components/extensions/ext-downloads.js:92
(Diff revision 1)
> +  pullup() {

We'd generally call this `serialize`

::: toolkit/components/extensions/ext-downloads.js:115
(Diff revision 1)
> +    let self = this;

No need for this. Arrow functions capture `this`.

::: toolkit/components/extensions/ext-downloads.js:123
(Diff revision 1)
> +        return list.getAll();

Generally I'd prefer to add a handler directly to `list.getAll()` so `list` is still in scope for the subsequent handlers, rather than hoisting it to the outer scope.

::: toolkit/components/extensions/ext-downloads.js:125
(Diff revision 1)
> +        downloads.every(download => self.newFromDownload(download, null));

Please use either a for-of loop or `downloads.forEach`. And if you use the latter, please put braces around the body of the arrow function, since you're not using its return value.

::: toolkit/components/extensions/ext-downloads.js:130
(Diff revision 1)
> +            if (self.fromDownload(download) == null) {

`fromDownload` doesn't seem to exist?

::: toolkit/components/extensions/ext-downloads.js:135
(Diff revision 1)
> +        downloadList.addView(listener).then(() => { self.loaded = true; });

I think thlis line needs a `return`

::: toolkit/components/extensions/ext-downloads.js:136
(Diff revision 1)
> +      });

I think this has a race. If you call getAll multiple times before it's finished loading, you're going to wind up loading multiple times. I'd get rid of `this.loaded`, add `self.loadedPromise`, and initialize it the first time it's needed when it's null.

Also, I'd rather do this from a separate `initialize` (or `lazyInit` as we use elsewhere) method.

::: toolkit/components/extensions/ext-downloads.js:140
(Diff revision 1)
> +      return Object.keys(self.downloads).map(id => self.downloads[id]);

It would be better to make `downloads` a Map, and then just use `this.downloads.values()`

::: toolkit/components/extensions/ext-downloads.js:191
(Diff revision 1)
> +  const urlRegex = new RegExp(query.urlRegex || ".*");

/.?/ would be faster. Or just an empty string, for that matter.

::: toolkit/components/extensions/ext-downloads.js:196
(Diff revision 1)
> +      filenameRegex = new RegExp(`^${query.filename}$`);

This isn't right. Filenames can contain regexp metacharacters. Same for the URL below. Maybe we should just make these tests functions rather than regexps. (Or `RegExp.prototype.test.bind(regexp)`)

::: toolkit/components/extensions/ext-downloads.js:212
(Diff revision 1)
> +      return false;

Perhaps rather than a `nomatch` variable we should just return a function which always returns false?

::: toolkit/components/extensions/ext-downloads.js:216
(Diff revision 1)
> +                        || (item.localname.indexOf(term) == -1))) {

You can use `.includes()` rather than `.indexOf`.

Are these matches supposed to be case sensitive?

Also, the body of the subsequent lines of the closure body should be aligned after the '=>' of the arrow function. I'd prefer something like:

  if (!queryTerms.every(term => (item.url.includes(term) ||
                                 item.localname.includes(term))))

::: toolkit/components/extensions/ext-downloads.js:232
(Diff revision 1)
> +    }

Shouldn't we return false if we have a start time query, but not start time in the item? The same goes for the items below, I think.

::: toolkit/components/extensions/ext-downloads.js:256
(Diff revision 1)
> +                           "exists" ];

No spaces inside square brackets, please.

::: toolkit/components/extensions/ext-downloads.js:349
(Diff revision 1)
> +          for (let download of downloads) {

`downloads.filter` would be better.

::: toolkit/components/extensions/test/mochitest/test_ext_downloads_search.html:25
(Diff revision 1)
> +      return Promise.resolve().then(() => browser.downloads.download(arguments[1]))

Why `return`?

::: toolkit/components/extensions/test/mochitest/test_ext_downloads_search.html:26
(Diff revision 1)
> +             .then((id) => browser.test.sendMessage("download.done", {status: "success", id}))

The dots should be lined up with a dot from the subsequent line.

Please put braces around the arrow function bodies that aren't expected to return values, and put their bodies on separate lines.

Comment 4

[Andrew Swan [:aswan]]

https://reviewboard.mozilla.org/r/36223/#review32793

> Please use JSDoc-style comments.

I didn't realize we were using jsdoc?  Do you have a pointer to how to run it / where to view generated docs?

> We'd generally call this `serialize`

I don't feel strongly but I don't think the word serialize accurately describes what this method does.

> You can use `.includes()` rather than `.indexOf`.
> 
> Are these matches supposed to be case sensitive?
> 
> Also, the body of the subsequent lines of the closure body should be aligned after the '=>' of the arrow function. I'd prefer something like:
> 
>   if (!queryTerms.every(term => (item.url.includes(term) ||
>                                  item.localname.includes(term))))

> Are these matches supposed to be case sensitive?

The documentation doesn't say one way or the other: https://developer.chrome.com/extensions/downloads#method-search
which I would take to mean case sensitive by default.

> `downloads.filter` would be better.

Perhaps this is premature but I wanted to avoid doing a bunch of unnecessary filtering if we have lots of downloads but get passed a relatively small limit.

Comment 5

[Andrew Swan [:aswan]]

Comment on attachment 8722757 [details]
MozReview Request: bug 1245603 - Implement browser.downloads.search() r=kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36223/diff/1-2/

Comment 6

[Kris Maglione [:kmag]]

(In reply to Andrew Swan [:aswan] from comment #4)
> https://reviewboard.mozilla.org/r/36223/#review32793
> 
> > Please use JSDoc-style comments.
> 
> I didn't realize we were using jsdoc?  Do you have a pointer to how to run
> it / where to view generated docs?

We don't run JSDoc, it's just the preferred style of API doc comments in
browser/toolkit code.

> > We'd generally call this `serialize`
>
> I don't feel strongly but I don't think the word serialize accurately
> describes what this method does.

It's fairly accurate. Or, in any case, it's at least as accurate as it is for
the other methods that use the same name.

> > Are these matches supposed to be case sensitive?
>
> The documentation doesn't say one way or the other:
> https://developer.chrome.com/extensions/downloads#method-search
> which I would take to mean case sensitive by default.

It's generally best not to assume anything from the Chrome docs. We should
check what they actually do.

Comment 7

[Kris Maglione [:kmag]]

Comment on attachment 8722757 [details]
MozReview Request: bug 1245603 - Implement browser.downloads.search() r=kmag

https://reviewboard.mozilla.org/r/36223/#review33065

This looks good, aside from some minor issues.

However, revision 0 of this review request seems to have somehow moved, so I can only compare with the last revision of the patch I looked at, not with what's currently on mozilla-central. Can you try submitting again and make sure the commit contains your full set of changes?

::: toolkit/components/extensions/ext-downloads.js:121
(Diff revision 2)
> -      promise = Downloads.getList(Downloads.ALL).then(list => {
> +        downloads.foreEach(download => {

*forEach

::: toolkit/components/extensions/ext-downloads.js:151
(Diff revision 2)
> +    for (let item of this.downloads) {

It would be nice to have a separate WeakMap for this, so we don't have to iterate.

::: toolkit/components/extensions/ext-downloads.js:161
(Diff revision 2)
>      this.downloads[id] = new DownloadItem(id, download, extension);

We should really check for duplicates here, and either throw, or just return the original DownloadItem for this download.

::: toolkit/components/extensions/ext-downloads.js:208
(Diff revision 2)
> +      return input => (value == input);

Based on bug 1225743 comment 44, it looks like matching is case insensitive for that API, so I suspect the same goes for this one.

For the regexp matches, it should be enough to just add the `i` flag. For the plain string matches, I think we'll just have to convert both terms to lower case before comparing.

::: toolkit/components/extensions/ext-downloads.js:218
(Diff revision 2)
> -                        || (item.localname.indexOf(term) == -1))) {
> +    if (queryTerms.some(term => (!item.url.includes(term) ||

I still think this is more of a `!queryTerms.every` than a `queryTerms.some(!)`

::: toolkit/components/extensions/ext-downloads.js:228
(Diff revision 2)
> -    if (item.startTime) {
> +    const startTime = new Date(item.startTime);

The `new Date` shouldn't be necessary. Dates are compared by coercing both to numbers via `.valueOf()`, so you're converting from a number to a Date object, just so it can be coerced back to the original number again.

Comment 8

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

Comment on attachment 8722757 [details]
MozReview Request: bug 1245603 - Implement browser.downloads.search() r=kmag

https://reviewboard.mozilla.org/r/36223/#review32945

I'm commenting both the revisions because of some issues on the diff, my apologies if I'm commenting an obsolete piece of code.

::: toolkit/components/extensions/ext-downloads.js:23
(Diff revision 1)
> -let currentId = 0;
> +function DownloadItem(id, download, extension) {

is there any issue on defining DownloadItem with ES6 class? 
(it doesn't make a big difference, but we're already using "class" elsewhere in our WebExtensions internals and it will help to make the internals more uniform in coding style)

::: toolkit/components/extensions/ext-downloads.js:109
(Diff revision 1)
> +const DownloadInfo = {

I'd prefer a name which makes clear its role (I was initially confused by the name), e.g.
DownloadsManagement, DownloadsManager (or even DownloadsInfo instead of DownloadInfo)

::: toolkit/components/extensions/ext-downloads.js:124
(Diff revisions 1 - 2)
> -      }).then(downloads => {
> +        return list.addView({

we should probably call list.removeView at some point (e.g. when the last addon with the "downloads" permission has been unloaded? or maybe when the last extension context which makes use of the download API has been unloaded)

Comment 9

[Andrew Swan [:aswan]]

https://reviewboard.mozilla.org/r/36223/#review33065

> The `new Date` shouldn't be necessary. Dates are compared by coercing both to numbers via `.valueOf()`, so you're converting from a number to a Date object, just so it can be coerced back to the original number again.

item.startTime is actually a string, not a number, but you're right, there's too much conversion going on here, I'll clean it up.

Comment 10

[Andrew Swan [:aswan]]

https://reviewboard.mozilla.org/r/36223/#review32945

> we should probably call list.removeView at some point (e.g. when the last addon with the "downloads" permission has been unloaded? or maybe when the last extension context which makes use of the download API has been unloaded)

Is there an example of other existing code that does this?  I think this is worth doing, but if there's not a straightforward way to do it at the moment, any objection to filing another bug for that and doing it separately?

Comment 11

[Kris Maglione [:kmag]]

(In reply to Andrew Swan [:aswan] from comment #10)
> https://reviewboard.mozilla.org/r/36223/#review32945
> 
> > we should probably call list.removeView at some point (e.g. when the last addon with the "downloads" permission has been unloaded? or maybe when the last extension context which makes use of the download API has been unloaded)
> 
> Is there an example of other existing code that does this?  I think this is
> worth doing, but if there's not a straightforward way to do it at the
> moment, any objection to filing another bug for that and doing it separately?

I'm not sure this is necessary. We do that in a few places, but the pay-off is pretty small. It doesn't seem likely that it's going to happen very often, and when it does, the effects only last as long as the session.

Comment 12

[Andrew Swan [:aswan]]

https://reviewboard.mozilla.org/r/36223/#review33065

> I still think this is more of a `!queryTerms.every` than a `queryTerms.some(!)`

Hm, "if some terms are not present" and "if not every term is present" mean the same thing (so says De Morgan!).
Since this is just about which one is clearer to a reader, I think that keeping the structure similar to the handling of negative terms right below is most important for clarity...

Comment 13

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/36223/#review33065

> Hm, "if some terms are not present" and "if not every term is present" mean the same thing (so says De Morgan!).
> Since this is just about which one is clearer to a reader, I think that keeping the structure similar to the handling of negative terms right below is most important for clarity...

They technically mean the same thing, but that doesn't mean they're similarly easy to understand. We want to check that every term is in either the local name or the URL. Expressing that as `if one of these terms is not in the title, and is not in the URL, then fail` feels very strange (and in this case winds up with the wrong results, since it currently fails if any term is not in both the URL and the local name).

I'm not sure that keeping the structure similar for both the positive and the negative terms is really desirable, either, for that matter. I mean, we could manage it even with `every`:

  if (!negativeTerms.every(term => !item.url.includes(term) && !item.title.includes(term))

But I find that hard to follow. Where as this seems fairly clear:

    if (!terms.every(term => item.url.includes(term) || item.localname.includes(term))) {
      return false;
    }

    if (negativeTerms.some(term => item.url.includes(term) || item.localname.includes(term))) {
      return false;
    }

Comment 14

[Andrew Swan [:aswan]]

https://reviewboard.mozilla.org/r/36223/#review33065

> Based on bug 1225743 comment 44, it looks like matching is case insensitive for that API, so I suspect the same goes for this one.
> 
> For the regexp matches, it should be enough to just add the `i` flag. For the plain string matches, I think we'll just have to convert both terms to lower case before comparing.

Confirmed that chrome matches are case-insensitive, will update.

Comment 15

[Kris Maglione [:kmag]]

(In reply to Andrew Swan [:aswan] from comment #14)
> Confirmed that chrome matches are case-insensitive, will update.

Thanks.

Comment 16

[Andrew Swan [:aswan]]

Comment on attachment 8722757 [details]
MozReview Request: bug 1245603 - Implement browser.downloads.search() r=kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36223/diff/2-3/

Comment 17

[Andrew Swan [:aswan]]

https://reviewboard.mozilla.org/r/36223/#review32793

> Perhaps this is premature but I wanted to avoid doing a bunch of unnecessary filtering if we have lots of downloads but get passed a relatively small limit.

Also, in the case when orderBy is not specified, downloads is a map iterator, not an array.

Comment 18

[Andrew Swan [:aswan]]

Sorry patch 2 was botched along the way, 1-3 is probably the most useful interdiff or of course you can just look at the whole thing fresh since some time has gone by since the original review request...

Comment 19

[Kris Maglione [:kmag]]

Comment on attachment 8722757 [details]
MozReview Request: bug 1245603 - Implement browser.downloads.search() r=kmag

https://reviewboard.mozilla.org/r/36223/#review33823

Looks good. Thanks.

::: toolkit/components/extensions/ext-downloads.js:28
(Diff revision 3)
> +                               "byExtensionId", "byExtensionName" ];

No spaces inside square brackets, please. ESLint won't accept this.

::: toolkit/components/extensions/ext-downloads.js:124
(Diff revision 3)
> +      return;

I'd rather have lazyInit return the promise.

::: toolkit/components/extensions/ext-downloads.js:134
(Diff revision 3)
> +          onDownloadAdded(download) {

I think we also need onDownloadRemoved to remove downloads when they're cleared.

::: toolkit/components/extensions/ext-downloads.js:356
(Diff revision 3)
> +          return Promise.reject(err);

You'll need to convert this to a bare object `{message: err.message}`. This works for now, but I have a pending patch that will change it to "An unexpected error occurred".

Comment 20

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

Comment on attachment 8722757 [details]
MozReview Request: bug 1245603 - Implement browser.downloads.search() r=kmag

https://reviewboard.mozilla.org/r/36223/#review33957

Thanks Andrew,
here is a couple of more ideas from my side:

::: toolkit/components/extensions/ext-downloads.js:150
(Diff revisions 2 - 3)
>        throw new Error(`Bad download id ${id}`);

I would like to suggest to reword this error into `Invalid download id ${id}`.

::: toolkit/components/extensions/test/mochitest/test_ext_downloads_search.html:131
(Diff revisions 2 - 3)
> +  yield checkDownloadItem(downloadIds.txt1, {

we should also add a test with an invalid download id.

Comment 21

[Andrew Swan [:aswan]]

https://reviewboard.mozilla.org/r/36223/#review33823

> No spaces inside square brackets, please. ESLint won't accept this.

Addressed, but lint was happy with it as it was... (I understand that's an issue with the lint rule, not arguing about what's intended)

Comment 22

[Andrew Swan [:aswan]]

Comment on attachment 8722757 [details]
MozReview Request: bug 1245603 - Implement browser.downloads.search() r=kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36223/diff/3-4/

Comment 23

[Andrew Swan [:aswan]]

The test for this has been failing intermittently for me in local runs, let me address that before this lands, I certainly don't want to inflict that flakiness on everybody else.

Comment 24

[Andrew Swan [:aswan]]

Comment on attachment 8722757 [details]
MozReview Request: bug 1245603 - Implement browser.downloads.search() r=kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36223/diff/4-5/

Comment 25

[Pulsebot]

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

Comment 26

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

Backed out for failing its own test:

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

Push with failures (M(5)): https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=f09b8b1fa430
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=7723033&repo=fx-team

05:20:01     INFO -  1541 INFO TEST-START | toolkit/components/extensions/test/mochitest/test_ext_downloads_search.html
05:20:08     INFO -  JavaScript error: chrome://specialpowers/content/specialpowersAPI.js, line 143: NS_ERROR_NOT_AVAILABLE: Component returned failure code: 0x80040111 (NS_ERROR_NOT_AVAILABLE) [nsIPrefBranch.setComplexValue]
05:25:08     INFO -  TEST-INFO | started process screentopng
05:25:09     INFO -  TEST-INFO | screentopng: exit 0
05:25:09     INFO -  1542 INFO downloadDir /tmp/downloads
05:25:09     INFO -  1543 INFO TEST-UNEXPECTED-FAIL | toolkit/components/extensions/test/mochitest/test_ext_downloads_search.html | Test timed out.
05:25:09     INFO -      reportError@SimpleTest/TestRunner.js:114:7
05:25:09     INFO -      TestRunner._checkForHangs@SimpleTest/TestRunner.js:134:7
05:25:09     INFO -      setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:155:5
05:25:09     INFO -      setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:155:5
05:25:09     INFO -      setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:155:5
05:25:09     INFO -      setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:155:5
05:25:09     INFO -      setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:155:5
05:25:09     INFO -      setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:155:5
05:25:09     INFO -      setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:155:5
05:25:09     INFO -      setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:155:5
05:25:09     INFO -      setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:155:5
05:25:09     INFO -      setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:155:5
05:25:09     INFO -      setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:155:5
05:25:09     INFO -      TestRunner.runTests@SimpleTest/TestRunner.js:366:5
05:25:09     INFO -      RunSet.runtests@SimpleTest/setup.js:188:3
05:25:09     INFO -      RunSet.runall@SimpleTest/setup.js:167:5
05:25:09     INFO -      hookupTests@SimpleTest/setup.js:260:5
05:25:09     INFO -  parseTestManifest@http://mochi.test:8888/manifestLibrary.js:36:5
05:25:09     INFO -  getTestManifest/req.onload@http://mochi.test:8888/manifestLibrary.js:49:11
05:25:09     INFO -  EventHandlerNonNull*getTestManifest@http://mochi.test:8888/manifestLibrary.js:45:3
05:25:09     INFO -      hookup@SimpleTest/setup.js:240:5
05:25:09     INFO -  EventHandlerNonNull*@http://mochi.test:8888/tests?autorun=1&closeWhenDone=1&consoleLevel=INFO&hideResultsTable=1&manifestFile=tests.json&dumpOutputDirectory=%2Ftmp:11:1

Comment 27

[Kris Maglione [:kmag]]

Oh, right, this isn't a Chrome test.

We can't use nsIFile in child processes (which is why this fails on e10s). OS.File would be a better choice anyway, but we should probably just make this a chrome test.

Comment 28

[Andrew Swan [:aswan]]

Comment on attachment 8722757 [details]
MozReview Request: bug 1245603 - Implement browser.downloads.search() r=kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36223/diff/5-6/

Comment 29

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/fx-team/rev/63132b94b6a39f017e9952486e01b1041b5a2d9f
Bug 1245603 - Implement browser.downloads.search() r=kmag

Comment 30

[Kris Maglione [:kmag]]

I replaced a lingering SpecialPowers.Ci reference before pushing.

Comment 31

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

This seems to have broken some tests: https://treeherder.mozilla.org/logviewer.html#?job_id=7735808&repo=fx-team

Backed out in https://hg.mozilla.org/integration/fx-team/rev/5ea9a54a24d6

Comment 32

[Kris Maglione [:kmag]]

Ah, we probably also have to clear the downloads list at the start and end of the tests. They passed when I ran them locally, but I didn't run it with the rest of the suite.

Comment 33

[Andrew Swan [:aswan]]

Comment on attachment 8722757 [details]
MozReview Request: bug 1245603 - Implement browser.downloads.search() r=kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36223/diff/6-7/

Comment 34

[Andrew Swan [:aswan]]

Oof, latest revision passes with e10s and when the download() test is run right beforehand.
I had to initialize DownloadMap from download() since that's where we add the onDownloadRemoved listener.  Without that, downloads started from browser.downloads.download() can be removed from the DownloadList managed by Downloads.jsm but they lingered in the DownloadMap.  whew.

Comment 35

[Kris Maglione [:kmag]]

Comment on attachment 8722757 [details]
MozReview Request: bug 1245603 - Implement browser.downloads.search() r=kmag

https://reviewboard.mozilla.org/r/36223/#review34247

::: toolkit/components/extensions/ext-downloads.js:126
(Diff revisions 6 - 7)
> -        downloads.forEach(download => {
> +          downloads.forEach(download => {

We should probably do this after the `addView` promise resolves, just to be safe.

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_downloads_search.html:60
(Diff revisions 6 - 7)
> +function clearDownloads(callback) {

It would be better to just return the promise rather than accept a callback.

Comment 36

[Andrew Swan [:aswan]]

https://reviewboard.mozilla.org/r/36223/#review34247

> It would be better to just return the promise rather than accept a callback.

Maybe callback was a poor choice of names, but it is just used to give information about what's going to be cleaned so the caller can print out info if necessary (we print an info statement for stuff we clean before the test, but not when cleaning after the test has run).  The actual flow of control does use a regular promise that is resolved after everything has been cleared.

Comment 37

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/36223/#review34247

> Maybe callback was a poor choice of names, but it is just used to give information about what's going to be cleaned so the caller can print out info if necessary (we print an info statement for stuff we clean before the test, but not when cleaning after the test has run).  The actual flow of control does use a regular promise that is resolved after everything has been cleared.

I know, but it's still possible for the promise to resolve to the list (or count) of downloads after they've been cleared. There's no reason that message has to be printed before the downloads have been removed.

Comment 38

[Andrew Swan [:aswan]]

Comment on attachment 8722757 [details]
MozReview Request: bug 1245603 - Implement browser.downloads.search() r=kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36223/diff/7-8/

Comment 39

[Andrew Swan [:aswan]]

Comment on attachment 8722757 [details]
MozReview Request: bug 1245603 - Implement browser.downloads.search() r=kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36223/diff/7-8/

Comment 40

[Andrew Swan [:aswan]]

https://reviewboard.mozilla.org/r/36223/#review34247

> I know, but it's still possible for the promise to resolve to the list (or count) of downloads after they've been cleared. There's no reason that message has to be printed before the downloads have been removed.

Ah I see, pushing the change now...

Comment 41

[Andrew Swan [:aswan]]

Comment on attachment 8722757 [details]
MozReview Request: bug 1245603 - Implement browser.downloads.search() r=kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36223/diff/8-9/

Comment 42

[Andrew Swan [:aswan]]

Comment on attachment 8722757 [details]
MozReview Request: bug 1245603 - Implement browser.downloads.search() r=kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36223/diff/9-10/

Comment 43

[Andrew Swan [:aswan]]

alright try is good, failures are unrelated: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5476e256d8f9

Comment 44

[Ryan VanderMeulen [:RyanVM]]

I strongly beg to differ. Those leaks are hitting all platforms and all happen after running the toolkit/components/extensions/test/mochitest directory.

Comment 45

[Andrew Swan [:aswan]]

Comment on attachment 8722757 [details]
MozReview Request: bug 1245603 - Implement browser.downloads.search() r=kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36223/diff/10-11/

Comment 46

[Andrew Swan [:aswan]]

My education continues...
Latest revision features not leaking the world, but we picked up some new failures that once again appear to me to be unrelated?
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4c9214ffce99

Comment 47

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/09ada320af0b

Comment 48

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/09ada320af0b