Closed Bug 1245599 Opened 5 years ago Closed 5 years ago

Implement chrome.downloads.onCreated

Categories

(WebExtensions :: Untriaged, defect)

defect
Not set
normal

Tracking

(firefox48 fixed)

RESOLVED FIXED
mozilla48
Iteration:
48.1 - Mar 21
Tracking Status
firefox48 --- fixed

People

(Reporter: aswan, Assigned: aswan)

References

(Blocks 1 open bug)

Details

(Whiteboard: [downloads])

Attachments

(1 file)

Part of supporting chrome.downloads in WebExtensions

https://developer.chrome.com/extensions/downloads#event-onCreated
Blocks: 1213445
Whiteboard: [downloads]
Assignee: nobody → aswan
Iteration: --- → 48.1 - Mar 21
Attachment #8727990 - Flags: review?(kmaglione+bmo) → review+
Comment on attachment 8727990 [details]
MozReview Request: Bug 1245599 - Implement chrome.downloads.onCreated r?kmag

https://reviewboard.mozilla.org/r/38725/#review35427

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_downloads_misc.html:57
(Diff revision 1)
> +            || !expected.every((item, i) => compare(events[i], item))) {

Can we explicitly fail if one of these doesn't match?

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_downloads_misc.html:122
(Diff revision 1)
> +  extension = ExtensionTestUtils.loadExtension({

Can you return this rather than making it a global?
https://reviewboard.mozilla.org/r/38725/#review35427

> Can you return this rather than making it a global?

You're looking at this partway through its journey into becoming a richer test suite, I plan to add additional tests in separate tasks, so things like setup() and runInExtension() would be outside the actual test tasks.  extension is still scoped to this script, is the desire for it to not be global just for general tidiness or is there another reason to prefer a narrower scope?
https://reviewboard.mozilla.org/r/38725/#review35427

> You're looking at this partway through its journey into becoming a richer test suite, I plan to add additional tests in separate tasks, so things like setup() and runInExtension() would be outside the actual test tasks.  extension is still scoped to this script, is the desire for it to not be global just for general tidiness or is there another reason to prefer a narrower scope?

I generally prefer test tasks to be as self-contained as possible, but I guess this is fine.
Comment on attachment 8727990 [details]
MozReview Request: Bug 1245599 - Implement chrome.downloads.onCreated r?kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38725/diff/1-2/
Keywords: checkin-needed
backed out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=7893663&repo=fx-team
Flags: needinfo?(aswan)
Comment on attachment 8727990 [details]
MozReview Request: Bug 1245599 - Implement chrome.downloads.onCreated r?kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38725/diff/2-3/
Pfft.  Now passing lint.
Keywords: checkin-needed
Flags: needinfo?(aswan)
You might want to enable the ESLint pre-commit hook[1] to prevent that from happening again.

[1]: https://wiki.mozilla.org/WebExtensions/Hacking#Version_control_hooks
https://hg.mozilla.org/mozilla-central/rev/0813fdd079bf
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.