Part of supporting chrome.downloads in WebExtensions https://developer.chrome.com/extensions/downloads#method-search

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

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.

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.

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 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/

(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 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 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)

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.

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?

(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.

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...

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; }

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.

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

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/

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.

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 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 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.

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 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/

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 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/

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

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 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/

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

I replaced a lingering SpecialPowers.Ci reference before pushing.

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

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 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/

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 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.

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.

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 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 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/

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 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 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/

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

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

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/

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