Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#3910 closed change (rejected)

Improve features message responder to include legacy Safari API support

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

https://codereview.adblockplus.org/29339627/

Description

Background

messageResponder.js already responds to the "features" message with an object containing true/false depending on which features the user's browser supports. One of the features checked is safariContentBlocker supported by versions of Safari >= 9.

With future versions of Safari the legacy blocking APIs will likely be removed. We can no longer assume that those APIs are supported.

What to change

Improve the code in messageResponder.js so that responses to feature messages also include the key safariLegacyApi. This should be true if the platform is Safari and Element.prototype contains the "onbeforeload" key.

Change History (5)

comment:1 Changed 4 years ago by sebastian

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

When checking for the legacy API, we should check for safari.self.tab.canLoad as well. Therefore this check needs to be done by the options page itself rather than by the background page. This has already been implemented for the new options page by #3880.

Last edited 4 years ago by sebastian (previous) (diff)

comment:2 Changed 4 years ago by kzar

I know the check has already been performed in the options page, I was going to change it to look at safariLegacyApi instead. The reason was that I now need to perform the check in the popup as well, as it stands we'll just have to duplicate the logic...

comment:3 Changed 4 years ago by kzar

  • Review URL(s) modified (diff)

comment:4 Changed 4 years ago by kzar

Nevermind, I see a better way to implement things now. Will leave this as rejected.

comment:5 Changed 4 years ago by sebastian

The legacy API is a content script API, and therefore needs to be checked for by a content script. With your implementation however, we'd assume that if there is Element.onbeforeload that there is also safari.self.tab.canLoad. But since we cannot be sure that both legacy APIs are removed at the same time, I'd rather avoid such assumptions.

Note: See TracTickets for help on using tickets.