Opened 14 months ago

Closed 14 months ago

Last modified 7 months ago

#6870 closed change (fixed)

Remove support for legacy :-abp-properties() syntax

Reported by: mjethani Assignee: jsonesen
Priority: P2 Milestone:
Module: Core Keywords:
Cc: sergz, kzar, arthur, sebastian, jsonesen, hfiguiere Blocked By:
Blocking: #6888 Platform: Unknown / Cross platform
Ready: yes Confidential: no
Tester: Ross Verified working: yes
Review URL(s):

https://codereview.adblockplus.org/29863644/

Description (last modified by mjethani)

Background

It has been more than a year since the legacy syntax for :-abp-properties() was deprecated. According to our internal Hub ticket no. 7571, none of the filter lists should be using this syntax any more. The additional parsing of the legacy syntax is responsible for the slowing down of the loading of the initial subscriptions (partially addressed by patch #29855590).

ABP should now stop supporting the legacy syntax.

What to change

Remove the additional parsing of the :-abp-properties() legacy syntax from Filter.fromText in lib/filterClasses.js and update any tests accordingly.

Hints for testers

See the :-abp-properties() section at https://adblockplus.org/development-builds/new-syntax-for-advanced-element-hiding-rules

The filter ##[-abp-properties="background-color: rgb(0, 0, 0)"] should now be a valid filter (even though it doesn't have an active domain) and should hide the element <div -abp-properties="background-color: rgb(0, 0, 0)">Ad</div> on any page.

Integration notes

After this is released, the filters documentation should be updated to say: "Note: The older syntax for the CSS property filters is no longer supported."

Change History (15)

comment:1 Changed 14 months ago by mjethani

  • Cc jsonesen added

comment:2 follow-up: Changed 14 months ago by mjethani

Arthur, is it OK to remove support for the legacy syntax now?

comment:3 Changed 14 months ago by mjethani

  • Cc hfiguiere added

comment:4 in reply to: ↑ 2 Changed 14 months ago by arthur

Replying to mjethani:

Arthur, is it OK to remove support for the legacy syntax now?

I think so but Sergei wanted to check that still as far as I know.

comment:5 Changed 14 months ago by mjethani

I ran this script in adblockpluscore:

#!/bin/bash
for url in $(sed -nE 's/.*url="(.*)".*/\1/p' chrome/content/ui/subscriptions.xml)
do
  curl -sS "$url" | grep '\[-abp-'
done

And I got nothing. It seems none of the lists we include by default are using the old syntax.

comment:6 Changed 14 months ago by mjethani

  • Priority changed from P4 to P2
  • Ready set

comment:7 Changed 14 months ago by jsonesen

Hey guys, I will take this task thanks for filing!

comment:8 Changed 14 months ago by jsonesen

  • Owner set to jsonesen

comment:9 Changed 14 months ago by jsonesen

  • Review URL(s) modified (diff)
  • Status changed from new to reviewing

comment:10 Changed 14 months ago by hfiguiere

  • Blocking 6888 added

comment:11 Changed 14 months ago by abpbot

A commit referencing this issue has landed:
Issue 6870 - Remove support for legacy :-abp-properties() syntax

comment:12 Changed 14 months ago by mjethani

  • Description modified (diff)

comment:13 Changed 14 months ago by jsonesen

  • Resolution set to fixed
  • Status changed from reviewing to closed

comment:14 Changed 13 months ago by mjethani

  • Description modified (diff)

comment:15 Changed 12 months ago by Ross

  • Tester changed from Unknown to Ross
  • Verified working set

Done. Working as described.

ABP 3.3.2.2175
Firefox 62 / 51 / Windows 10
Chrome 69 / 49 / Windows 10
Opera 56 / 36 / Windows 10

Note: See TracTickets for help on using tickets.