Opened on 11/21/2014 at 03:52:04 PM

Closed on 11/24/2014 at 10:44:51 AM

Last modified on 11/24/2014 at 11:49:13 AM

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

Attachments (0)

Change History (7)

comment:1 Changed on 11/21/2014 at 04:09:44 PM by sergz

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

comment:2 follow-up: Changed on 11/24/2014 at 03:37:38 AM 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 on 11/24/2014 at 09:14:42 AM by philll

And what could be tested here? :-)

comment:4 in reply to: ↑ 2 Changed on 11/24/2014 at 10:10:18 AM 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 on 11/24/2014 at 10:27:21 AM 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 on 11/24/2014 at 10:44:51 AM by sergz

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

comment:7 in reply to: ↑ 3 Changed on 11/24/2014 at 11:49:13 AM 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.

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