Opened 3 years ago

Last modified 20 months ago

#4579 closed change

Target promise-based browserext API — at Version 29

Reported by: kzar Assignee: mjethani
Priority: P3 Milestone: Adblock-Plus-3.0.3-for-Chrome-Opera-Firefox
Module: Platform Keywords:
Cc: sebastian, oleksandr, greiner Blocked By: #4551, #4783, #5028
Blocking: #4580 Platform: Unknown / Cross platform
Ready: no Confidential: no
Tester: Ross Verified working: yes
Review URL(s):

https://codereview.adblockplus.org/29573892/
https://codereview.adblockplus.org/29678657/

Description (last modified by kzar)

Background

We want to target the browserext standard instead of the Chrome extension API. This should hopefully mean Edge and in the future Gecko can be supported without pollyfills.

What to change

  • Add a polyfill for the Chrome extension to add browserext compatibility, adding the browser namespace, wrapping asynchronous APIs to use promises and mapping the others directly. (See this existing polyfill
  • Replace any other use of the Chrome APIs with the with the browserext API.

Change History (29)

comment:1 Changed 3 years ago by kzar

  • Blocked By 4551 added; 4511 removed

comment:2 Changed 3 years ago by kzar

  • Description modified (diff)

comment:3 Changed 3 years ago by kzar

  • Description modified (diff)
  • Summary changed from Remove ext compatibility layer, target browser extension API to Target the browser extension API

comment:4 Changed 3 years ago by sebastian

  • Priority changed from Unknown to P3
  • Ready set

comment:5 Changed 3 years ago by kzar

  • Blocking 4580 added

comment:6 Changed 3 years ago by kzar

  • Summary changed from Target the browser extension API to Target the browserext API

comment:7 Changed 3 years ago by kzar

  • Cc oleksandr added
  • Owner set to kzar

comment:8 Changed 3 years ago by kzar

  • Blocked By 4783 added

comment:9 Changed 3 years ago by greiner

  • Cc greiner added

comment:10 Changed 3 years ago by sebastian

Giving it a second though (and having a go myself) it seems too early to target (and polyfill) the Promise based API. As of now, neither Chrome nor Microsoft Edge supports Promises with their API. Only Firefox does, which also optionally accepts callbacks. As a result callbacks (as opposed to Promises) are the common denominator of all targeted browsers. Implementing a polyfill that allows us to use Promises will add complexity and overhead.

But what I think we should already do is replacing all occurrences of chrome with browser, aliasing browser = chrome on Chrome. We already did it the other way around in the edge branch (#3695), and we might need it for Firefox/WebExtensions as well. However, it feels backwards, to keep using the chrome.* namespace, while this code runs on other browser, and browser is going to be standardized.

comment:11 Changed 3 years ago by kzar

I already made a lot of progress towards targeting the browser API, if you're curious to have a look here's my branch. It has been a while since I looked at it but IIRC I didn't quite finish, and also I planned to try using a lighter weight polyfill that Wladimir suggested. (Also I need to rebase again...) I did have mostly everything working however.

But I kind of agree with you, I think it might well be a little early to make the switch.

comment:12 Changed 3 years ago by kzar

  • Owner kzar deleted

Unassigned myself since I don't have the time to continue working on this currently and as per the above discussion we're not sure if it's the right time to make the switch anyway.

comment:13 Changed 3 years ago by sebastian

  • Blocked By 5028 added

comment:14 Changed 3 years ago by sebastian

  • Ready unset

I moved the part about using browser.* with callbacks (instead of promises), for now, into #5028.

comment:15 Changed 3 years ago by sebastian

  • Summary changed from Target the browserext API to Target promise-based browserext API

comment:16 Changed 2 years ago by mjethani

  • Owner set to mjethani
  • Review URL(s) modified (diff)

comment:17 Changed 2 years ago by mjethani

  • Status changed from new to reviewing

comment:18 Changed 2 years ago by abpbot

A commit referencing this issue has landed:
Issue 4579 - Promisify APIs

comment:19 Changed 2 years ago by mjethani

  • Milestone set to Adblock-Plus-for-Chrome-Opera-Firefox-next
  • Resolution set to fixed
  • Status changed from reviewing to closed

comment:20 Changed 2 years ago by mjethani

  • Resolution fixed deleted
  • Status changed from closed to reopened

comment:21 Changed 2 years ago by mjethani

  • Milestone Adblock-Plus-for-Chrome-Opera-Firefox-next deleted

comment:22 Changed 2 years ago by kzar

I notice a whole bunch of exceptions in the background console since this landed:

Uncaught (in promise) {message: "The message port closed before a response was received."}
func.call.result @ polyfill.js:80
sendResponseAndClearCallback @ extensions::messaging:415
disconnectListener @ extensions::messaging:439
EventImpl.dispatchToListener @ extensions::event_bindings:403
publicClassPrototype.(anonymous function) @ extensions::utils:140
EventImpl.dispatch_ @ extensions::event_bindings:387
EventImpl.dispatch @ extensions::event_bindings:409
publicClassPrototype.(anonymous function) @ extensions::utils:140
dispatchOnDisconnect @ extensions::messaging:376

comment:23 Changed 2 years ago by mjethani

It seems that when we call runtime.sendMessage with a callback, and we don't get a response, Chrome considers that to be something worth reporting in runtime.lastError

We may have to handle this case in the wrapper, since it's usually not an error the caller is interested in.

comment:24 Changed 2 years ago by abpbot

A commit referencing this issue has landed:
Issue 4579 - Wrap rejection reason in Error object

comment:25 Changed 2 years ago by abpbot

A commit referencing this issue has landed:
Issue 4579 - Ignore runtime.lastError caused by wrapper

comment:26 Changed 2 years ago by abpbot

A commit referencing this issue has landed:
Issue 4579 - Do not wrap non-existent APIs

comment:27 follow-up: Changed 22 months ago by abpbot

A commit referencing this issue has landed:
Issue 4579 - Wrap runtime.onMessage

comment:28 in reply to: ↑ 27 Changed 22 months ago by kzar

Replying to abpbot:

A commit referencing this issue has landed:
Issue 4579 - Wrap runtime.onMessage

This last commit is causing a couple of linting errors Manish:

adblockpluschrome/polyfill.js

  133:39  error  'hasListener' is assigned a value but never used  no-unused-vars
  160:15  error  Unsafe usage of ThrowStatement                    no-unsafe-finally
Last edited 22 months ago by kzar (previous) (diff)

comment:29 Changed 22 months ago by mjethani

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