Review Request: Automatic AdBlock filter list updates in Konqueror

[Thomas Fischer]

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

(Updated 2010-01-24 21:25:29.523114)


Review request for kdelibs.


Changes
-------

> Yes. Netaccess is dangerous. Not only does it block the GUI for an undetermined amount of time,
> it also leads to re-entrancy in complex apps where timers and sockets are involved, and KHTML is
> exactly in that case. See the documentation of KJob::exec() for more about the dangers of NetAccess
> (all that doc applies to NetAccess too; I'll improve the NetAccess docs accordingly).
> Basically any timer or socket event might access a not-fully-constructed KHTMLPart because it will
> still be inside KHTMLPart::init(), downloading stuff. (KHTMLPart::init -> KHTMLSettings::init ->
> NetAccess -> nested event loop -> timers and sockets calling slots in khtml or konqueror).
I see the problem. Rewrote KHTMLSettings accordingly using two slots to handle data from the TransferJob.

> I know that this will mean the settings are only fully loaded after init() returns, but that should
> be fine, at worse the currently-loading page will use an older version of the adblocks.
> Maybe that means the systemsettings module should perform a first download of the file, so that the
> cache is ready for the next khtmlpart?
Currently, the case where the cached lists are too old is treated as if the lists are not there at all.
I will look into changing this behaviour later. First, I'd like to get some feedback on my changes,
i.e. if the new changes are what you requested in your original commented.


Summary
-------

This is the patch from bug report 143495 slightly rewritten to fit KDE trunk (though not compiled as I am lacking Qt 4.6). The original patch was written for 4.2.4, later updated for 4.3.1. Please refer to the bug report in bugs.kde.org for a small history of this patch set.
I wrote a blog entry explaining the background of this patch, too: http://blog.t-fischer.net/2009/08/05/automatic-adblock-filter-list-updates-in-konqueror/


Diffs (updated)
-----

  /trunk/KDE/kdebase/apps/konqueror/settings/konqhtml/filteropts.h 1079192 
  /trunk/KDE/kdebase/apps/konqueror/settings/konqhtml/filteropts.cpp 1079192 
  /trunk/KDE/kdelibs/khtml/khtml_settings.h 1079192 
  /trunk/KDE/kdelibs/khtml/khtml_settings.cpp 1079192 

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


Testing
-------


Thanks,

Thomas