Opened on 04/09/2016 at 02:14:04 PM

Closed on 04/09/2016 at 02:28:13 PM

Last modified on 04/09/2016 at 03:07:47 PM

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

Attachments (0)

Change History (5)

comment:1 Changed on 04/09/2016 at 02:28:13 PM 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 on 04/09/2016 at 02:28:41 PM by sebastian

comment:2 Changed on 04/09/2016 at 02:37:29 PM 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 on 04/09/2016 at 02:40:47 PM by kzar

  • Review URL(s) modified (diff)

comment:4 Changed on 04/09/2016 at 03:02:27 PM by kzar

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

comment:5 Changed on 04/09/2016 at 03:07:47 PM 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.

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 kzar.
 
Note: See TracTickets for help on using tickets.