Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#1591 closed change (fixed)

Add ELEMHIDE to AdblockPlus::FilterEngine::ContentType enum

Reported by: sergz Assignee:
Priority: P3 Milestone:
Module: Libadblockplus Keywords:
Cc: oleksandr, fhd, trev Blocked By:
Blocking: Platform: Unknown
Ready: yes Confidential: no
Tester: Verified working: no
Review URL(s):

http://codereview.adblockplus.org/4791095586193408/

Description

It's needed by IE addon otherwise it's not compilable. In the recent libadblockplus FilterEngine::Matches accepts only enum AdblockPlus::FilterEngine::ContentType, it does not accept string as before.
We should fix the exactly enum for consistency with other filter options. The another option is to add a special method for elemhide case and refactor (add) FilterEngine::CheckFilterMatch which accepts content type as string and we can call it from Main.cpp. It might be even better, because I'm not sure whether it makes sense to call CheckFilterMatch(url, 'document', url) for the case.

What to change

Add the enum element and fix ContentTypeMap.

Change History (7)

comment:1 Changed 5 years ago by sergz

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

comment:2 follow-up: Changed 5 years ago by fhd

  • Priority changed from Unknown to P3
  • Ready set

Yeah, lets'add ELEMHIDE to the enum. While we're at it: POPUP is also missing.

comment:3 follow-ups: Changed 5 years ago by philll

And what could be tested here? :-)

comment:4 in reply to: ↑ 2 Changed 5 years ago by sergz

Replying to fhd:

Yeah, lets'add ELEMHIDE to the enum. While we're at it: POPUP is also missing.

POPUP is not mentioned in https://adblockplus.org/en/filters although I see it in the filters. BTW, it's not used in IE. I would say we can add it in another commit.

comment:5 in reply to: ↑ 3 Changed 5 years ago by sergz

Replying to philll:

And what could be tested here? :-)

I'm not sure whether it makes sense to write any test now because we don't have such tests at all, which verify whether each filter option is supported and works perfectly. Although I would certainly like to have automatic tests. Currently I would classify the issue as some kind of typo.

The real trouble it causes is that it's not possible to use the recent version of libadblockplus in IE project without that enum entry. Of course we test that it works during debugging, despite it's not on what we should rely. May be testers can find some page which has elemhide filter and test whether ABP properly works but firstly we need to update the library client, for example IE addon.

comment:6 Changed 5 years ago by sergz

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

comment:7 in reply to: ↑ 3 Changed 5 years ago by trev

  • Cc trev added

Replying to philll:

And what could be tested here? :-)

Nothing - this is a libadblockplus issue. Testing can be done once these changes are "imported" into an actual product that uses libadblockplus, like Adblock Plus for IE.

Note: See TracTickets for help on using tickets.