Opened 6 months ago

Closed 4 months ago

Last modified 4 weeks ago

#7334 closed change (fixed)

Remove ability to pass callback function into wrapped browser.* APIs

Reported by: kzar Assignee: mjethani
Priority: P2 Milestone: Adblock-Plus-for-Chrome-Opera-Firefox-next
Module: Platform Keywords:
Cc: sebastian, greiner, mjethani Blocked By: #7271, #7272
Blocking: Platform: Unknown / Cross platform
Ready: yes Confidential: no
Tester: Ross Verified working: yes
Review URL(s):

https://codereview.adblockplus.org/30024555/

Description (last modified by mjethani)

Background

Once we have ensured that no code that calls browser.* APIs passes in callbacks, we can simplify the adblockpluschrome/polyfill.js code. This will also make sure that new code doesn't start passing callbacks again.

What to change

Simplify the API wrapping code so that passing in a callback function is no longer supported.

In polyfill.js:

  function wrapAsyncAPI(api)
  {
    ...

        if (typeof args[args.length - 1] == "function")
          return func.apply(object, args);

        // If the last argument is undefined, we drop it from the list assuming
        // it stands for the optional callback. We must do this, because we have
        // to replace it with our own callback. If we simply append our own
        // callback to the list, it won't match the signature of the function
        // and will cause an exception.
        if (typeof args[args.length - 1] == "undefined")
          args.pop();

    ...

Remove this part of the code.

Hints for testers

Run some basic smoke tests, e.g. open the options page, add/remove subscriptions, update filter lists manually, change some preferences, browse to some web pages, and make sure there are no errors in the background page console and the options page console.

Change History (13)

comment:1 Changed 6 months ago by mjethani

Just to share my two cents here:

  function wrapAsyncAPI(api)
  {
    ...

        if (typeof args[args.length - 1] == "function")
          return func.apply(object, args);

        // If the last argument is undefined, we drop it from the list assuming
        // it stands for the optional callback. We must do this, because we have
        // to replace it with our own callback. If we simply append our own
        // callback to the list, it won't match the signature of the function
        // and will cause an exception.
        if (typeof args[args.length - 1] == "undefined")
          args.pop();

    ...

In both the function and undefined cases, now we should throw an error. If any errors appear in the console we know we need to fix that part.

Last edited 6 months ago by mjethani (previous) (diff)

comment:2 Changed 6 months ago by kzar

Sounds good to me. Feel free to flesh out the issue description, sounds like you have a good idea how it should work.

comment:3 Changed 6 months ago by mjethani

  • Description modified (diff)

comment:4 Changed 6 months ago by mjethani

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

comment:5 Changed 6 months ago by mjethani

I have uploaded a patch but first the issues that this depends on need to be addressed.

Last edited 6 months ago by mjethani (previous) (diff)

comment:6 Changed 6 months ago by kzar

Nice one, thanks. I'll take a look once the other stuff's sorted.

comment:7 Changed 6 months ago by sebastian

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

comment:8 Changed 4 months ago by abpbot

A commit referencing this issue has landed:
Issue 7334 - Remove support for callbacks in API wrappers

comment:9 Changed 4 months ago by mjethani

  • Description modified (diff)
  • Resolution set to fixed
  • Status changed from new to closed

comment:10 Changed 4 months ago by mjethani

  • Description modified (diff)

comment:11 Changed 4 months ago by sebastian

  • Milestone set to Adblock-Plus-for-Chrome-Opera-Firefox-next

comment:12 Changed 3 months ago by abpbot

Removed. This message was posted because of a mistake in the commit message.

Last edited 3 months ago by kvas (previous) (diff)

comment:13 Changed 4 weeks ago by Ross

  • Tester changed from Unknown to Ross
  • Verified working set

Done. Doesn't look to have caused any regressions.

ABP 0.9.15.2339
Microsoft Edge 44.17763.1.0 / Windows 10 1809

ABP 3.5.2.2339
Chrome 49.0.2623.75 / Windows 10 1809
Chrome 75.0.3770.142 / Windows 10 1809
Opera 36.0.2130.65 / Windows 10 1809
Opera 62.0.3331.72 / Windows 10 1809
Firefox 51.0 / Windows 10 1809
Firefox 68.0 / Windows 10 1809
Firefox Mobile 68.0 / Android 7.2.2

Note: See TracTickets for help on using tickets.