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

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

Comment on attachment 8731240 [details] MozReview Request: Bug 1245651 Implement chrome.downloads.onErased r?kmag https://reviewboard.mozilla.org/r/40457/#review36965 ::: toolkit/components/extensions/ext-downloads.js:161 (Diff revision 1) > }, > > onDownloadRemoved(download) { > const item = self.byDownload.get(download); > if (item != null) { > + self.emit("erase", item); I guess we'll need to handle this differently for downloads that come from history? ::: toolkit/components/extensions/test/mochitest/test_chrome_ext_downloads_misc.html:86 (Diff revision 1) > } > - if (expected.length > events.length) { > + if (exact && expected.length > events.length) { > reject(new Error(`Got ${events.length} events but only expected ${expected.length}`)); > } > > + let eventspos = 0; I'm having trouble following this. Can you try to simplify it a bit?

https://reviewboard.mozilla.org/r/40457/#review36965 > I guess we'll need to handle this differently for downloads that come from history? Yes, bug 1255507 > I'm having trouble following this. Can you try to simplify it a bit? With the various permutations of exact and inorder, I don't have any great ideas for simplifying, but I added some comments that should clarify...

Comment on attachment 8731240 [details] MozReview Request: Bug 1245651 Implement chrome.downloads.onErased r?kmag Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40457/diff/1-2/

https://reviewboard.mozilla.org/r/40457/#review37931 Hi Andrew, Here is a couple of things that I found in the test cases and that I think that worth a mention (I marked as issues just the ones that I think that could be important, but if you are unsure about them, feel free to wait to double-check them with Kris during the final review and clear the issues if they end up to be non-important, or if my assumptions were wrong). ::: toolkit/components/extensions/test/mochitest/test_chrome_ext_downloads_misc.html:74 (Diff revision 2) > return Object.keys(expected).every(fld => compare(received[fld], expected[fld])); > } > return (received == expected); > } > + > + options = options || {}; I think that it can be nice to turn this into the ES6 syntax for parameters default value: function waitForEvents(expected, options = {}) or even better (because we make it clear which are the available options and their default value): function waitForEvents(expected, options = { exact: true, inorder: true }) { ... const { exact, inorder } = options; ::: toolkit/components/extensions/test/mochitest/test_chrome_ext_downloads_misc.html:82 (Diff revision 2) > return new Promise((resolve, reject) => { > function check() { > if (events.length < expected.length) { > return; > } > - if (expected.length > events.length) { > + if (exact && expected.length > events.length) { By reading the error message logged, it seems to me that here we wanted to check the opposite: if (exact && expected.length < events.length) { ... } If I'm right and this check is supposed to be reversed (so that we check that "the number of expected events is less than the number of the actual events collected"), then we need to fix a couple of test cases which are going to fail if we don't set the options to |{exact: false}| in the callers. If the check is right, maybe we can tweak the logged error message. ::: toolkit/components/extensions/test/mochitest/test_chrome_ext_downloads_misc.html:83 (Diff revision 2) > function check() { > if (events.length < expected.length) { > return; > } > - if (expected.length > events.length) { > + if (exact && expected.length > events.length) { > reject(new Error(`Got ${events.length} events but only expected ${expected.length}`)); These error messages (this one and the one which follows) are useful, expecially if the tests fail on try, to understand the failure reasons, but they are not currently logged anywhere (the callers are currently testing only the status field to be "success" when it is supposed to) I propose to log it here as a failure message and just reject the Promise: if (...) { browser.test.fail(`Got ${events.length}...`); reject(); } so that we cannot forget to check or log the error message on the returned Promise. ::: toolkit/components/extensions/test/mochitest/test_chrome_ext_downloads_misc.html:113 (Diff revision 2) > + // longer for the event, so return without failing, check() will be > + // called again when a new event arrives. > + if (found.length != expected.length) { > + if (exact) { > + const pos = found.length; > + reject(new Error(`Mismatch in event ${pos}, expecting ${JSON.stringify(expected[pos])} but got ${JSON.stringify(events[pos])}`)); same as the above (browser.test.fail with the error message and just reject the promise) ::: toolkit/components/extensions/test/mochitest/test_chrome_ext_downloads_misc.html:145 (Diff revision 2) > let what = match[1]; > if (what == "waitForEvents") { > - waitForEvents(arguments[1]).then(() => { > + waitForEvents(...args).then(() => { > browser.test.sendMessage("waitForEvents.done", {status: "success"}); > }).catch(error => { > browser.test.sendMessage("waitForEvents.done", {status: "error", errmsg: error.message}); The errmsg doesn't seem to be used from the callers of this functions (I'm pretty sure that it was, one more reason to immediatelly log the error as a failure as soon as it happens). If we opt to log the failure message using the |browser.test.fail| method as suggested in the previous comments, this can be turned into: browser.test.sendMessage("...", {status: "error"}); ::: toolkit/components/extensions/test/mochitest/test_chrome_ext_downloads_misc.html:158 (Diff revision 2) > Promise.resolve().then(() => { > return browser.downloads[what](...args); > }).then(result => { > browser.test.sendMessage(`${what}.done`, {status: "success", result}); > }).catch(error => { > browser.test.sendMessage(`${what}.done`, {status: "error", errmsg: error.message}); same as the above. ::: toolkit/components/extensions/test/mochitest/test_chrome_ext_downloads_misc.html:563 (Diff revision 2) > + ]); > + is(msg.status, "success", "received onErased event"); > + > msg = yield runInExtension("search", {}); > is(msg.status, "success", "search succeded"); > is(msg.result.length, 2, "search found 2 downloads"); How about adding some assertions here that ensure only the expected download item has been removed?

https://reviewboard.mozilla.org/r/40457/#review37931 > I think that it can be nice to turn this into the ES6 syntax for parameters default value: > > function waitForEvents(expected, options = {}) > > or even better (because we make it clear which are the available options and their default value): > > function waitForEvents(expected, options = { exact: true, inorder: true }) { > > ... > const { exact, inorder } = options; Using the default for options is a no-brainer, but I don't think the defaults for the individual properties will help, if I pass { exact: false }, then options.inorder will be undefined but i want it to default to true in that case. Maybe there's some clever way to do it with a destructuring parameter or something but for the time being, I'd prefer to keep it as is. > By reading the error message logged, it seems to me that here we wanted to check the opposite: > > if (exact && expected.length < events.length) { > ... > } > > If I'm right and this check is supposed to be reversed (so that we check that "the number of expected events is less than the number of the actual events collected"), then we need to fix a couple of test cases which are going to fail if we don't set the options to |{exact: false}| in the callers. > > If the check is right, maybe we can tweak the logged error message. yikes, good catch. > These error messages (this one and the one which follows) are useful, expecially if the tests fail on try, to understand the failure reasons, but they are not currently logged anywhere (the callers are currently testing only the status field to be "success" when it is supposed to) > > I propose to log it here as a failure message and just reject the Promise: > > if (...) { > browser.test.fail(`Got ${events.length}...`); > reject(); > } > > so that we cannot forget to check or log the error message on the returned Promise. But this code is running in the extension, I don't think browser.test.* is available there? You're right though, it would be ideal to get the error message in these cases. I'll make a wrapper from the main test script that will log the error message if we're expecting success and we get an error.

Comment on attachment 8731240 [details] MozReview Request: Bug 1245651 Implement chrome.downloads.onErased r?kmag Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40457/diff/2-3/

Fixing the reversed check that Luca pointed out uncovered some other problems that need to be fixed before this will be ready, but I wanted to get the changes for the other suggestions he made into the review quickly...

Comment on attachment 8731240 [details] MozReview Request: Bug 1245651 Implement chrome.downloads.onErased r?kmag Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40457/diff/3-4/

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

Comment on attachment 8731240 [details] MozReview Request: Bug 1245651 Implement chrome.downloads.onErased r?kmag Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40457/diff/4-5/

There was a bunch of semi-related test refactoring in the same commit as the new code for this issue so I split it into two commits. I've got several other changes dependent on the test refactoring so hoping to get this in in the next day or two...

Comment on attachment 8733174 [details] MozReview Request: Bug 1245651 - Rework chrome.downloads misc tests r?kmag Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41617/diff/1-2/

Comment on attachment 8731240 [details] MozReview Request: Bug 1245651 Implement chrome.downloads.onErased r?kmag Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40457/diff/5-6/

https://reviewboard.mozilla.org/r/40457/#review36965 > With the various permutations of exact and inorder, I don't have any great ideas for simplifying, but I added some comments that should clarify... I think something like this should work: https://gist.github.com/48a4303e8c23a6cb1765

(In reply to Kris Maglione [:kmag] from comment #15) > https://reviewboard.mozilla.org/r/40457/#review36965 > > > With the various permutations of exact and inorder, I don't have any great ideas for simplifying, but I added some comments that should clarify... > > I think something like this should work: > https://gist.github.com/48a4303e8c23a6cb1765 :/ Apparently I forgot to hit the 'Publish' button after I wrote this 5 days ago... Sorry

Comment on attachment 8731240 [details] MozReview Request: Bug 1245651 Implement chrome.downloads.onErased r?kmag https://reviewboard.mozilla.org/r/40457/#review38293 ::: toolkit/components/extensions/test/mochitest/test_chrome_ext_downloads_misc.html:117 (Diff revisions 1 - 6) > // longer for the event, so return without failing, check() will be > // called again when a new event arrives. > if (found.length != expected.length) { > if (exact) { > const pos = found.length; > - reject(new Error(`Mismatch in event ${pos}, expecting ${JSON.stringify(expected[pos])} but got ${JSON.stringify(events[pos])}`)); > + fail(`Mismatch in event ${pos}, expecting ${JSON.stringify(expected[pos])} but got ${JSON.stringify(events[pos])}`); I think we still need to reject here, and above. Also, as far as I know, there is no `fail` function available here. ::: toolkit/components/extensions/test/mochitest/test_chrome_ext_downloads_misc.html:591 (Diff revisions 1 - 6) > is(msg.status, "success", "erase by id succeeded"); > > msg = yield runInExtension("waitForEvents", [ > {type: "onErased", data: ids.dl1}, > ]); > - is(msg.status, "success", "received onErased event"); > + expect_success(msg, "received onErased event"); Where does `expect_success` come from?

https://reviewboard.mozilla.org/r/40457/#review38293 > I think we still need to reject here, and above. > > Also, as far as I know, there is no `fail` function available here. Oh, I see. I didn't realize there was another commit above this one.

https://reviewboard.mozilla.org/r/40457/#review36965 > I think something like this should work: https://gist.github.com/48a4303e8c23a6cb1765 Its not functional as-is (I think the declaration of events need to change from an array to a Set, but then events doesn't have a .every method. I'm not seeing a huge advantage to one approach over the other, but I'll get your version working and push a revision.

https://reviewboard.mozilla.org/r/40457/#review36965 > Its not functional as-is (I think the declaration of events need to change from an array to a Set, but then events doesn't have a .every method. > I'm not seeing a huge advantage to one approach over the other, but I'll get your version working and push a revision. I think `events` can stay an array, but `events.size` needs to be changed to `events.length`, and `remaining` needs to be converted to an array before being assigned. There may be other problems. This was meant as a general suggestion rather than a drop-in replacement. I'm not sure there's a huge advantage, but I find the current code extremely difficult to follow, and this version not easy to follow, but at least much easier.

Comment on attachment 8733174 [details] MozReview Request: Bug 1245651 - Rework chrome.downloads misc tests r?kmag Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41617/diff/2-3/

Comment on attachment 8731240 [details] MozReview Request: Bug 1245651 Implement chrome.downloads.onErased r?kmag Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40457/diff/6-7/

Comment on attachment 8733174 [details] MozReview Request: Bug 1245651 - Rework chrome.downloads misc tests r?kmag https://reviewboard.mozilla.org/r/41617/#review38637 Please include a bug number in the commit message. ::: toolkit/components/extensions/test/mochitest/test_chrome_ext_downloads_misc.html:35 (Diff revision 3) > const INT_PARTIAL_LEN = 15; > const INT_TOTAL_LEN = 31; > > function backgroundScript() { > - let events = []; > + let events = new Set(); > let eventWaiter = null; This test file is currently disabled. If this patch fixes the intermittents, please re-enable it. Otherwise, please at least enable it for one try run.

Comment on attachment 8731240 [details] MozReview Request: Bug 1245651 Implement chrome.downloads.onErased r?kmag https://reviewboard.mozilla.org/r/40457/#review38639

Comment on attachment 8733174 [details] MozReview Request: Bug 1245651 - Rework chrome.downloads misc tests r?kmag Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41617/diff/3-4/

Comment on attachment 8731240 [details] MozReview Request: Bug 1245651 Implement chrome.downloads.onErased r?kmag Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40457/diff/7-8/

Mac and win8 tests are lagging but we're clear on linux and win7...

https://hg.mozilla.org/integration/fx-team/rev/d6d9e7411319 https://hg.mozilla.org/integration/fx-team/rev/4b988c94fd14

https://hg.mozilla.org/mozilla-central/rev/d6d9e7411319 https://hg.mozilla.org/mozilla-central/rev/4b988c94fd14