Review Request: [PATCH] Make KIO::Scheduler correctly re-use ioslaves that have been put on hold...

[Dawit Alemayehu]

> On 2011-01-04 13:44:51, Andreas Hartmetz wrote:
> > /trunk/KDE/kdelibs/kio/kio/scheduler.cpp, line 924
> > <http://svn.reviewboard.kde.org/r/6271/diff/3/?file=43485#file43485line924>
> >
> >     Better move everything into a mayReturnContent(KIO::Job *) method and avoid the long function name.

Indeed... that is much cleaner and descriptive except for the parameter. Need to pass the job comand and the protocol to avoid repeating the protocol lookup in the new function. Fixed.


> On 2011-01-04 13:44:51, Andreas Hartmetz wrote:
> > /trunk/KDE/kdelibs/kio/kio/scheduler.cpp, line 1130
> > <http://svn.reviewboard.kde.org/r/6271/diff/3/?file=43485#file43485line1130>
> >
> >     This is the core of the patch, right...
> >     I don't know if the slave-on-hold mechanism was meant to be used between KParts in the same app or only between applications. In the latter case more changes in KIO will probably be necessary. In the former case, I screwed up something around this line in the scheduler rewrite.

Well the original intent of the slave-on-hold mechansim was to make an ioslave started in one application available in another. Its creation was mostly intended to handle the scenario where a job in App A needs to be passed to App B because App A cannot handle it. As such the mechanism used to handle both the same app with different KParts and two separate apps is actually the same. The only difference is that for the latter case you would need to call publishSlaveOnHold, which returns the slave-on-hold to klauncher, so that the other application can find and reuse it. Obviously, publishing the presence of a slave-on-hold is not necessary for the same app.

Anyhow, you did not screw up anything that concerns this mechanism during the rewrite. However, my original analysis and patch were wrong. I have amended both the analysis as well as the patch...


- Dawit


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


On 2011-01-04 09:55:29, Dawit Alemayehu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://svn.reviewboard.kde.org/r/6271/
> -----------------------------------------------------------
> 
> (Updated 2011-01-04 09:55:29)
> 
> 
> Review request for kdelibs.
> 
> 
> Summary
> -------
> 
> The patch attempts to fix the following two oustanding problems in KIO::Scheduler that have been around for a while (see the possibly related bug reports in the Bugs section above):
> 
> #1. Set the m_checkOnHold flag to true every time Scheduler::publishSlaveOnHold is invoked. Right now that flag is only set to true when an instance of KIO::Scheduler is created. This results in the flag never being true after the first ioslave has been put on hold and reused unless the programmer explicitly calls KIO::Scheduler::checkSlaveOnHold which is not documented at all. See the description about putting ioslaves on hold in KIO::get's API documentation.
> 
> #2. Modify SchedulerPrivate::doJob to correctly set the m_checkOnHold flag for http requests when a job's command is CMD_SPECIAL.  That is necessary because HTTP_POST, which is handled as a special command, can return content just like a get request.
> 
> 
> This addresses bugs 123121 and 148307.
>     https://bugs.kde.org/show_bug.cgi?id=123121
>     https://bugs.kde.org/show_bug.cgi?id=148307
> 
> 
> Diffs
> -----
> 
>   /trunk/KDE/kdelibs/kio/kio/scheduler.cpp 1211439 
> 
> Diff: http://svn.reviewboard.kde.org/r/6271/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Dawit
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20110104/e79ec2ba/attachment.htm>