D24962: [DeleteJob] Use a separate worker thread to run actual IO operation

[David Faure]

dfaure requested changes to this revision.
dfaure added a comment.
This revision now requires changes to proceed.


  Almost there ;)

INLINE COMMENTS

> deletejob.cpp:73
> +    /**
> +     * Deletes the file @p points to
> +     * The file must be a LocalFile

Missing "url" after @p (which introduces the name of an argument). Not that we're going to run doxygen here, but well ;)

> deletejob.cpp:132
> +    void rmdirResult(bool result, const QUrl& url);
> +    void deleteFileUsingJob (const QUrl& url, bool isLink);
> +    void deleteDirUsingJob (const QUrl& url);

no space after method name (same on next line)

> deletejob.cpp:188
> +
> +    m_ioworker = new DeleteJobIOWorker;
> +    m_ioworker->moveToThread(&m_thread);

One problem left: creating the worker and its thread is done even if we're deleting only remote URLs. This is a bit wasteful. Suggestion: create a worker() method which does all this (everything that you added to this method) on demand, if m_ioworker is null.

> deletejob.cpp:340
> +
> +        if (isLink) { // not a link
> +            symlinks.removeFirst();

That is a really weird comment, for a bool called isLink :-)

> deletejob.cpp:348
> +    } else {
> +        // fallback if unlink() failed (we'll use the job's error handling in that case)
> +        deleteFileUsingJob(url, isLink);

(pre-existing) s/unlink/QFile::remove/ in this comment would be less confusing

> deletejob.cpp:353
> +
> +void DeleteJobPrivate::deleteFileUsingJob (const QUrl &url, bool isLink)
>  {

please remove spaces after method names everywhere

> dfaure wrote in deletejob.cpp:412
> removeLast here too?

marked as done but I still see removeFirst, I'm confused.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D24962

To: meven, dfaure
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20191027/6bb0c4db/attachment-0001.html>