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/#review2424
-----------------------------------------------------------


By mail, http://lists.kde.org/?l=kfm-devel&m=125354748517107&w=2

Date: Mon, 21 Sep 2009 10:53:09 -0400 (EDT)
Subject: Re: Review Request: khtml adblock filter lookup API
From: "Maksim Orlovich" <mo85 at cornell.edu>
To: kfm-devel at kde.org
Cc: "Jonathan Marten" <jjm2 at keelhaul.demon.co.uk>

Looks fine, though I can't be 100% sure that what it returns in complex
cases is identical to the input.

Sorry for missing your previous e-mail.

- Jonathan


On 2009-08-23 09:32:15, Jonathan Marten wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/1382/
> -----------------------------------------------------------
> 
> (Updated 2009-08-23 09:32:15)
> 
> 
> Review request for kdelibs.
> 
> 
> 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
> -----
> 
>   /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
> 
>