Review Request: Automatic AdBlock filter list updates in Konqueror

[David Faure]

> On 2009-11-30 22:00:56, David Faure wrote:
> > /trunk/KDE/kdelibs/khtml/khtml_settings.cpp, line 374
> > <http://reviewboard.kde.org/r/2265/diff/1/?file=14938#file14938line374>
> >
> >     Please use KIO::get instead of NetAccess::download. Which means moving the rest of the code to a slot, but I guess that's not a big problem here.

> Actually, that is a problem, as does not use any slots so far, does not inherit from QObject etc.
> Would require more extensive changes. Is changing to KIO::get really worth the effort?

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 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?


- David


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


On 2010-01-23 21:01:37, Thomas Fischer wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/2265/
> -----------------------------------------------------------
> 
> (Updated 2010-01-23 21:01:37)
> 
> 
> Review request for kdelibs.
> 
> 
> 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
> -----
> 
>   /trunk/KDE/kdebase/apps/konqueror/settings/konqhtml/filteropts.cpp 1066357 
>   /trunk/KDE/kdelibs/khtml/khtml_settings.cpp 1066280 
>   /trunk/KDE/kdebase/apps/konqueror/settings/konqhtml/filteropts.h 1066357 
> 
> Diff: http://reviewboard.kde.org/r/2265/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Thomas
> 
>