Opened on 01/14/2016 at 11:40:17 PM

Closed on 11/10/2017 at 11:15:18 AM

#3518 closed change (rejected)

Use fetch API instead XMLHttpRequest

Reported by: sebastian Assignee:
Priority: Unknown Milestone:
Module: Adblock-Plus-for-Firefox Keywords:
Cc: trev Blocked By:
Blocking: Platform: Unknown / Cross platform
Ready: no Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

Description

Background

The fetch() API is a more modern interface, with a more streamlined API than XMLHttpRequest, to do essentially the same thing, sending HTTP requests with JavaScript. Most notably, the new API is based on promises, which make asynchronous code easier to write and read. But it is also a little more powerful; so for example fetch() let's you specify the caching strategy, which we so far rely on non-standard properties for that are only supported on Firefox.

With #3515 we will have a polyfill in place for older Chrome/Opera versions as well as Safari, which don't have native support for fetch(). So it can even be used in the core code which is shared cross-platform.

What to change

Replace all usage of XMLHttpRequest with the fetch() API.

Attachments (0)

Change History (5)

comment:1 follow-up: Changed on 01/15/2016 at 05:02:03 PM by trev

There are significant issues with this as far as Firefox is concerned. Fetch API is only supported starting with Firefox 39 (we are stuck supporting Firefox 38 for at least a few more months). Worse yet, originally it was only supported on windows - we are running in a sandbox and support was only added there in Firefox 41. And of course the sandbox created for bootstrapped add-ons doesn't have Fetch API exposed on it. So we would either have to sandbox our modules (will likely degrade performance) or create a sandbox just for the purpose of getting the Fetch API out of it (should be preferable but I wonder whether it will work outside of that sandbox just like that).

Altogether, I think that this is premature - we should wait until we can drop support for everything before Firefox 41 rather than messing with polyfills now and making this unnecessarily complicated.

Note that libadblockplus will have to be adjusted as well if we do that, it currently implements a fake XMLHttpRequest object. I wonder whether V8 version used there is recent enough to suppport promises natively...

comment:2 Changed on 01/19/2016 at 12:31:00 PM by sebastian

  • Status changed from new to reviewing

comment:3 Changed on 01/19/2016 at 12:31:29 PM by sebastian

  • Status changed from reviewing to reopened

comment:4 in reply to: ↑ 1 Changed on 01/20/2016 at 02:18:39 PM by sebastian

Replying to trev:

There are significant issues with this as far as Firefox is concerned. Fetch API is only supported starting with Firefox 39 (we are stuck supporting Firefox 38 for at least a few more months). Worse yet, originally it was only supported on windows - we are running in a sandbox and support was only added there in Firefox 41. And of course the sandbox created for bootstrapped add-ons doesn't have Fetch API exposed on it. So we would either have to sandbox our modules (will likely degrade performance) or create a sandbox just for the purpose of getting the Fetch API out of it (should be preferable but I wonder whether it will work outside of that sandbox just like that).

Altogether, I think that this is premature - we should wait until we can drop support for everything before Firefox 41 rather than messing with polyfills now and making this unnecessarily complicated.

Ouch, I hoped we could simply drop support for Firefox 38, or migrate to fetch() once we did so. But also considering the limitation on FF <41, I'd rather go with a polyfill. Note that polyfilling just the subset of the Fetch API we need, as we already do for old Chrome and Safari version, is quite trivial. Also note that half of the code in that polyfill would be duplicated anyway when using XMLHttpRequest directly. So if you don't like polyfills, just consider it a convenience wrapper. ;)

Note that libadblockplus will have to be adjusted as well if we do that, it currently implements a fake XMLHttpRequest object. I wonder whether V8 version used there is recent enough to suppport promises natively...

Well, if we use a polyfill in the core code, things would keep working for the time being. But yeah, sooner or later we would have to replace the fake XMLHttpRequest object there with a fake fetch() function.

comment:5 Changed on 11/10/2017 at 11:15:18 AM by trev

  • Resolution set to rejected
  • Status changed from reopened to closed

Mass-closing all bugs in Adblock Plus for Firefox module, the codebase of Adblock Plus 3.0 belongs into Platform and User-Interface modules. Old bugs are unlikely to still apply.

Add Comment

Modify Ticket

Change Properties
Action
as closed .
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from (none).
 
Note: See TracTickets for help on using tickets.