Opened 6 months ago

Closed 6 months ago

Last modified 7 weeks ago

#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
https://gitlab.com/eyeo/adblockplus/abpui/adblockpluschrome/merge_requests/18

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 6 months ago by greiner

  • Description modified (diff)
  • Priority changed from Unknown to P3
  • Ready set

comment:2 Changed 6 months ago by kzar

  • Owner set to kzar

comment:3 Changed 6 months ago by kzar

  • Review URL(s) modified (diff)
  • Status changed from new to reviewing

comment:4 Changed 6 months ago by greiner

  • Blocking 6936 added
  • Resolution set to fixed
  • Status changed from reviewing to closed

comment:5 Changed 6 months ago by greiner

  • Description modified (diff)

Added integration notes.

comment:6 Changed 6 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 6 months ago by greiner

  • Review URL(s) modified (diff)
  • Status changed from reopened to reviewing

comment:8 Changed 6 months ago by greiner

  • Resolution set to fixed
  • Status changed from reviewing to closed

comment:9 Changed 6 months ago by kzar

  • Blocking 7271 removed

comment:10 Changed 6 months ago by kzar

  • Description modified (diff)
  • Ready unset

We need to rethink this API, see the discussion here.

comment:11 Changed 6 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 6 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 3 months ago by abpbot

A commit referencing this issue has landed:
Issue 7327 - Update showOptions calls to use Promises

comment:14 Changed 3 months ago by kzar

  • Description modified (diff)

comment:15 Changed 7 weeks ago by ukacar

  • Verified working set
Note: See TracTickets for help on using tickets.