Opened 13 days ago

Last modified 12 days ago

#7334 new change

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

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

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

Description (last modified by sebastian)

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();

    ...

If the type of the last argument is either function or undefined, the wrapper should throw an error.

Change History (7)

comment:1 Changed 13 days 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 13 days ago by mjethani (previous) (diff)

comment:2 Changed 13 days 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 13 days ago by mjethani

  • Description modified (diff)

comment:4 Changed 13 days ago by mjethani

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

comment:5 Changed 13 days ago by mjethani

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

Last edited 13 days ago by mjethani (previous) (diff)

comment:6 Changed 13 days ago by kzar

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

comment:7 Changed 12 days ago by sebastian

  • Description modified (diff)
  • Priority changed from Unknown to P2
  • Ready set
Note: See TracTickets for help on using tickets.