Review Request: Extend khtml API to allow retrieval of the adblock filter that was matched (for display in GUI)

[Jonathan Marten]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/1382/
-----------------------------------------------------------

(Updated 2009-08-23 09:32:15.394432)


Review request for kdelibs.


Changes
-------

Updated in accordance with comments from Maksim Orlovich.

No need to ensure BC for private classes, so default argument added to StringsMatcher::isMatched() instead of overloading.

The logic and result of KHTMLSettings::adFilteredBy() was confusing.  Since there are two filter lists, a blacklist and a whitelist, and only a single result to return, originally the matching filter from either would be returned and it would be up to the caller to use isAdFiltered() to determine whether the match was in the blacklist or whitelist.  However, this effectively meant that 2 filter lookups were needed.  Now there is an optional 'bool' result which indicates whether the match was in the whitelist or blacklist.

API documentation added to new function KHTMLSettings::adFilteredBy() and existing KHTMLSettings::isAdFiltered().


Summary
-------

This patch to khtml allows the adblock filter that matched a blocked or whitelisted URL to be identified.  Currently the only lookup function available is 'bool KHTMLSettings::isAdFiltered(const QString &url) const', which returns whether an URL is filtered but does not identify the filter that matched.  The proposed addition is 'QString adFilteredBy(const QString &url) const' which returns the filter expression that matched.

It is intended that the matching filter will be displayed in the "Blockable items on this page" dialogue, as a tooltip over a blocked item.

This change should be BC according to the rules at http://techbase.kde.org/Policies/Binary_Compatibility_Issues_With_C++ because it only adds new non-virtual functions (but StringsMatcher::isMatched() becomes overloaded, so not SC).  When possible (KDE5?), the two overloads of this can be merged into one with a default argument.

Performance should not be affected, as the only additional code run in the normal case (checking for a blocked URL) is a pointer comparison against NULL which will always succeed and do nothing more.  Only when being called from the adblock GUI will the string-returning functions be used and the filter result stored.


Diffs (updated)
-----

  /trunk/KDE/kdelibs/khtml/khtml_filter.cpp 1014194 
  /trunk/KDE/kdelibs/khtml/khtml_filter_p.h 1014194 
  /trunk/KDE/kdelibs/khtml/khtml_settings.h 1014194 
  /trunk/KDE/kdelibs/khtml/khtml_settings.cpp 1014194 

Diff: http://reviewboard.kde.org/r/1382/diff


Testing
-------

Tested against a modified version of the adblock plugin (not committed yet, this addition to KHTML is needed first).  The matching filter is retrieved and displayed as a tooltip.


Thanks,

Jonathan