#7327 closed change (fixed)
Update options.showOptions calls to use Promises
| Reported by: | kzar | Assignee: | kzar |
|---|---|---|---|
| Priority: | P3 | Milestone: | |
| Module: | User-Interface | Keywords: | |
| Cc: | greiner, sebastian | Blocked By: | |
| Blocking: | #6936 | Platform: | Unknown / Cross platform |
| Ready: | yes | Confidential: | no |
| Tester: | Unknown | Verified working: | yes |
| Review URL(s): |
https://gitlab.com/eyeo/adblockplus/abpui/adblockplusui/merge_requests/237 |
||
Description (last modified by kzar)
Background
With #7271 we are updating our use of the browser.* APIs to use Promises instead of callbacks where possible. Generally the changes are trivial, but it made sense to restructure some of the options.showOptions logic around Promises too.
What to change
- Update options.showOptions calls to make use of the returned Promise, instead of passing in a callback function.
- Update the mock showOptions function in background.js
See also ui#348.
Integration notes
showOptions() in lib/options.js is expected to return a Promise.
Hints for testers
Test the logic to open options page, both from popup, notifications and subscription links. Including focusing the page if it's already open (even if that's in a different window), logic to add dialogues e.g. when clicking on subscription links. There are quite a few different code paths, so it's important to test thoroughly with the different browsers (e.g. Chrome, Firefox, Firefox mobile and Edge) and versions (e.g. Chrome 49, modern Chrome, etc).
Change History (15)
comment:1 Changed 9 months ago by greiner
- Description modified (diff)
- Priority changed from Unknown to P3
- Ready set
comment:2 Changed 9 months ago by kzar
- Owner set to kzar
comment:3 Changed 9 months ago by kzar
- Review URL(s) modified (diff)
- Status changed from new to reviewing
comment:4 Changed 9 months ago by greiner
- Blocking 6936 added
- Resolution set to fixed
- Status changed from reviewing to closed
comment:6 Changed 9 months ago by greiner
- Resolution fixed deleted
- Status changed from closed to reopened
Reopening the ticket to make the necessary changes to our adblockpluschrome fork.
comment:7 Changed 9 months ago by greiner
- Review URL(s) modified (diff)
- Status changed from reopened to reviewing
comment:8 Changed 9 months ago by greiner
- Resolution set to fixed
- Status changed from reviewing to closed
comment:9 Changed 9 months ago by kzar
- Blocking 7271 removed
comment:10 Changed 9 months ago by kzar
- Description modified (diff)
- Ready unset
We need to rethink this API, see the discussion here.
comment:11 Changed 9 months ago by greiner
Are you saying that we don't yet know whether showOptions() will be returning a promise or simply what data it will resolve to?
Because for the latter no further changes would be necessary.
comment:12 Changed 9 months ago by kzar
- Ready set
Oh yea, good point. Sorry I think I changed this before you pointed out that we don't happen to care about the resolved value from showOptions in the adblockplusui code.
comment:13 Changed 7 months ago by abpbot
A commit referencing this issue has landed:
Issue 7327 - Update showOptions calls to use Promises
comment:14 Changed 7 months ago by kzar
- Description modified (diff)
comment:15 Changed 5 months ago by ukacar
- Verified working set
Done: https://gitlab.com/eyeo/adblockplus/abpui/adblockplusui/commit/6815616add5b37dce1c6dda956135869548a889e