Opened on 03/05/2019 at 03:11:49 PM
Closed on 03/12/2019 at 01:20:44 PM
Last modified on 07/09/2019 at 09:33:25 AM
#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).
Attachments (0)
Change History (15)
comment:1 Changed on 03/05/2019 at 03:19:40 PM by greiner
- Description modified (diff)
- Priority changed from Unknown to P3
- Ready set
comment:2 Changed on 03/05/2019 at 03:53:01 PM by kzar
- Owner set to kzar
comment:3 Changed on 03/05/2019 at 03:56:04 PM by kzar
- Review URL(s) modified (diff)
- Status changed from new to reviewing
comment:4 Changed on 03/06/2019 at 12:17:29 PM by greiner
- Blocking 6936 added
- Resolution set to fixed
- Status changed from reviewing to closed
comment:5 Changed on 03/11/2019 at 05:26:46 PM by greiner
- Description modified (diff)
Added integration notes.
comment:6 Changed on 03/11/2019 at 07:43:50 PM 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 on 03/11/2019 at 07:44:10 PM by greiner
- Review URL(s) modified (diff)
- Status changed from reopened to reviewing
comment:8 Changed on 03/12/2019 at 01:20:44 PM by greiner
- Resolution set to fixed
- Status changed from reviewing to closed
comment:9 Changed on 03/13/2019 at 10:49:51 AM by kzar
- Blocking 7271 removed
comment:10 Changed on 03/13/2019 at 11:46:50 AM by kzar
- Description modified (diff)
- Ready unset
We need to rethink this API, see the discussion here.
comment:11 Changed on 03/13/2019 at 02:58:13 PM 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 on 03/13/2019 at 03:02:04 PM 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 on 05/20/2019 at 10:58:19 AM by abpbot
A commit referencing this issue has landed:
Issue 7327 - Update showOptions calls to use Promises
comment:14 Changed on 05/21/2019 at 01:08:10 PM by kzar
- Description modified (diff)
comment:15 Changed on 07/09/2019 at 09:33:25 AM by ukacar
- Verified working set
Done: https://gitlab.com/eyeo/adblockplus/abpui/adblockplusui/commit/6815616add5b37dce1c6dda956135869548a889e