Target promise-based browserext API

[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.

[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.

[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.

[kzar]

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.

[sebastian]

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

[abpbot]

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

[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

[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.

[abpbot]

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

[abpbot]

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

[abpbot]

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

[abpbot]

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

[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

[abpbot]

A commit referencing this issue has landed:
​Issue 4579 - Fix ESLint errors

[abpbot]

A commit referencing this issue has landed:
​Issue 4579 - Fix ESLint errors

[mjethani]

Changeset ​7247eb4632fa has caused a regression due to the way the wrappers are implemented. We expect tabs.insertCSS to throw an error, which it does, but that error is turned into a promise rejection by the wrapper. We could simply handle the rejection, but that would be asynchronous and would cause the code in lib/cssInjection.js to become more complicated, aside from being inconsistent with Firefox.

The wrapper should also throw an error if the wrapped function throws an error.

I have logged #6343 for this.

[abpbot]

A commit referencing this issue has landed:
​Issue 4579, 6343 - Let errors from wrapped API calls bubble up

[Ross]

Done. Didn't see any of the errors described above. And using browser.runtime.sendMessage (for example) in the console works.

ABP 3.0.2.1983
Firefox 51 / 58 / Windows 10
Chrome 49 / 65 / Windows 7
Opera 36 / 51 / Windows 7