Opened 15 months ago

Closed 15 months ago

Last modified 11 months ago

#7327 closed change (fixed)

Update options.showOptions calls to use Promises — at Version 14

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 (14)

comment:1 Changed 15 months ago by greiner

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

comment:2 Changed 15 months ago by kzar

  • Owner set to kzar

comment:3 Changed 15 months ago by kzar

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

comment:4 Changed 15 months ago by greiner

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

comment:5 Changed 15 months ago by greiner

  • Description modified (diff)

Added integration notes.

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

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

comment:8 Changed 15 months ago by greiner

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

comment:9 Changed 15 months ago by kzar

  • Blocking 7271 removed

comment:10 Changed 15 months ago by kzar

  • Description modified (diff)
  • Ready unset

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

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

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

comment:14 Changed 12 months ago by kzar

  • Description modified (diff)
Note: See TracTickets for help on using tickets.