Opened 9 months ago

Closed 9 months ago

#7406 closed change (fixed)

Stop using Utils.runAsync

Reported by: kzar Assignee: mjethani
Priority: P2 Milestone:
Module: Core Keywords: manifestv3
Cc: mjethani, sebastian, hfiguiere, greiner Blocked By: #7381
Blocking: #7338 Platform: Unknown / Cross platform
Ready: yes Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

https://gitlab.com/eyeo/adblockplus/adblockpluscore/merge_requests/40

Description (last modified by mjethani)

Background

With #7338 we're no longer able to use some browser APIs from our background page. One thing we can no longer provide therefore is the Utils.runAsync function.

What to change

Stop using Utils.runAsync, namely in adblockpluscore/lib/downloader.js

Integration notes

Remove Utils.runAsync() from lib/utils.js in adblockpluschrome.

Hints for testers

Same as #7376.

Change History (13)

comment:1 Changed 9 months ago by mjethani

  • Cc hfiguiere added

comment:2 Changed 9 months ago by mjethani

  • Component changed from Unknown to Core

comment:3 Changed 9 months ago by kzar

  • Keywords manifestv3 added

comment:4 Changed 9 months ago by sebastian

  • Description modified (diff)

comment:5 follow-up: Changed 9 months ago by mjethani

I have looked into this and I would like to suggest the following:

  1. In the download() method in lib/downloader.js in adblockpluscore, we return a promise that resolves or rejects when the download is complete or has failed; this would make it asynchronous anyway
  2. We modify lib/subscriptionInit.js in adblockpluschrome and messageResponder.js in adblockplusui to do what formerly Utils.runAsync() would do for the case when the document is not in the ready state; adblockplusui would check the ready state, while adblockpluschrome would do the same for now but later switch to using ServiceWorkerContainer.ready or whatever other extension API is appropriate

comment:6 Changed 9 months ago by mjethani

  • Cc greiner added

comment:7 Changed 9 months ago by mjethani

  • Blocked By 7381 added

comment:8 in reply to: ↑ 5 Changed 9 months ago by mjethani

Replying to mjethani:

  1. In the download() method in lib/downloader.js in adblockpluscore, we return a promise that resolves or rejects when the download is complete or has failed; this would make it asynchronous anyway

Just to be clear, it doesn't have to return the promise for the purpose of resolving this issue. It could just keep the promise internal. In fact it could just use setTimeout() with a timeout of 0 as Utils.runAsync() does.

I have to point this out because returning a promise would entail further changes in the _download() method's implementation to make sure everything resolves/rejects correctly.

comment:9 Changed 9 months ago by sebastian

For reference, the logic in Utils.runAsync() was a workaround that makes sure to call the callback not before all background scripts have been loaded/executed. There is a good chance that this workaround is no longer necessary, as most of our code is bundled together with webpack, and it might have been a browser bug in the first place that deferred loading of background scripts.

comment:10 Changed 9 months ago by mjethani

  • Owner set to mjethani

comment:11 Changed 9 months ago by mjethani

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

comment:12 Changed 9 months ago by abpbot

A commit referencing this issue has landed:
Issue 7406 - Replace Utils.runAsync() with promise

comment:13 Changed 9 months ago by mjethani

  • Description modified (diff)
  • Resolution set to fixed
  • Status changed from reviewing to closed
Note: See TracTickets for help on using tickets.