Opened 13 months ago

Last modified 9 months ago

#7327 closed change

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

Reported by: kzar Assignee: kzar
Priority: P3 Milestone:
Module: User-Interface Keywords:
Cc: greiner, sebastian Blocked By:
Blocking: #6936, #7271 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 greiner)

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

Something like this:

  • background.js

    diff --git a/background.js b/background.js
    index 479be40..1bf0e18 100644
    a b  
    453453  }; 
    454454 
    455455  modules.options = { 
    456     showOptions(callback) 
     456    showOptions() 
    457457    { 
    458458      if (!/\/(?:mobile|desktop)-options\.html\b/.test(top.location.href)) 
    459459        window.open("desktop-options.html", "_blank"); 
    460460 
    461       if (callback) 
    462         callback(); 
     461      return Promise.resolve(); 
    463462    } 
    464463  }; 
    465464 
  • messageResponder.js

    diff --git a/messageResponder.js b/messageResponder.js
    index 3f2a1ac..e0f68a7 100644
    a b  
    258258  { 
    259259    if (message.what == "options") 
    260260    { 
    261       showOptions(() => 
     261      showOptions().then(() => 
    262262      { 
    263263        if (!message.action) 
    264264          return; 
     
    405405      if ("homepage" in message) 
    406406        subscription.homepage = message.homepage; 
    407407 
    408       showOptions(() => 
     408      showOptions().then(() => 
    409409      { 
    410410        sendMessage("app", "addSubscription", subscription); 
    411411      }); 

See also ui#348.

Integration notes

showOptions() in lib/options.js is expected to return a Promise.

Change History (7)

comment:1 Changed 13 months ago by greiner

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

comment:2 Changed 13 months ago by kzar

  • Owner set to kzar

comment:3 Changed 13 months ago by kzar

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

comment:4 Changed 13 months ago by greiner

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

comment:5 Changed 13 months ago by greiner

  • Description modified (diff)

Added integration notes.

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

  • Review URL(s) modified (diff)
  • Status changed from reopened to reviewing
Note: See TracTickets for help on using tickets.