Opened 4 months ago

Closed 3 months ago

Last modified 7 weeks 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 4 months ago by mjethani

  • Cc jsonesen added

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

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

comment:3 Changed 4 months ago by mjethani

  • Cc hfiguiere added

comment:4 in reply to: ↑ 2 Changed 4 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 4 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 4 months ago by mjethani

  • Priority changed from P4 to P2
  • Ready set

comment:7 Changed 4 months ago by jsonesen

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

comment:8 Changed 4 months ago by jsonesen

  • Owner set to jsonesen

comment:9 Changed 4 months ago by jsonesen

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

comment:10 Changed 4 months ago by hfiguiere

  • Blocking 6888 added

comment:11 Changed 4 months ago by abpbot

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

comment:12 Changed 4 months ago by mjethani

  • Description modified (diff)

comment:13 Changed 3 months ago by jsonesen

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

comment:14 Changed 3 months ago by mjethani

  • Description modified (diff)

comment:15 Changed 7 weeks 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.