Review Request: Speed limit in ftp kio slave

[David Faure]

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



kioslave/ftp/ftp.cpp
<http://git.reviewboard.kde.org/r/102267/#comment4989>

    The construct "Type foo[variable]" is not portable (to compilers other than gcc). You have to use new[]+delete[], or QVarLengthArray or in the case of char like here, QByteArray. I would recommend QByteArray for this code.



kioslave/ftp/ftp.cpp
<http://git.reviewboard.kde.org/r/102267/#comment4987>

    What does the magic number represent?
    
    Ah, 1 second? Would be worth a comment.



kioslave/ftp/speedController.h
<http://git.reviewboard.kde.org/r/102267/#comment4984>

    This seems to be your own code, the copyright is yours, not mine :)



kioslave/ftp/speedController.h
<http://git.reviewboard.kde.org/r/102267/#comment4994>

    kde_file.h isn't used in this header -> move the #include to the .cpp file.



kioslave/ftp/speedController.h
<http://git.reviewboard.kde.org/r/102267/#comment4985>

    trailing whitespace



kioslave/ftp/speedController.h
<http://git.reviewboard.kde.org/r/102267/#comment4986>

    why not just return the int, like  int bytesToRead()?



kioslave/ftp/speedController.cpp
<http://git.reviewboard.kde.org/r/102267/#comment4992>

    Not my code :)



kioslave/ftp/speedController.cpp
<http://git.reviewboard.kde.org/r/102267/#comment4993>

    Make getters const, for good practice.



kioslave/ftp/speedController.cpp
<http://git.reviewboard.kde.org/r/102267/#comment4990>

    This doesn't seem to "add" anything, but to "set". It replaces any existing socket.
    
    Note: the naming is wrong. m_socket looks like a member variable, while "socket" is the actual member variable.
    
    I would suggest to use m_ for the actual member vars, in fact -- and for sure never for function parameters.



kioslave/ftp/speedController.cpp
<http://git.reviewboard.kde.org/r/102267/#comment4991>

    The problem with usleep is that it's not portable (e.g. to Windows).
    
    QThread::usleep would do the job -- but it's protected (for no good reason), so you will have to write
    
    class ThreadWorkaround : public QThread
    {
        using QThread::usleep;
    }
    
    and then you can write ThreadWorkaround::usleep().
    


- David


On Aug. 9, 2011, 7:16 p.m., Tushar Mehta wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/102267/
> -----------------------------------------------------------
> 
> (Updated Aug. 9, 2011, 7:16 p.m.)
> 
> 
> Review request for kdelibs.
> 
> 
> Summary
> -------
> 
> - This patch contains the basic code which will put the limit on download speed of the ftp data transfer.
> - It is looking for "speed-limit" meta-data for deciding how much speed control is required.
> - If this meta-data is not found, code will work as it was before and no speed control related code will come into picture.
> - This patch is the most basic one which I have testing on my system and to the extent it is controlling the speed.
> - Lets say if speed limit is 30 KBps then mostly will get the avg speed around 30 to 35 KBps.
> - I am using QTime for measuring time elapsed between two socket read call and its precision is in millisecond. Looping is taking place in microsecond and thats why I am getting almost all the time 0 as time elapsed in between two calls.
> - To solve the above problem usleep is introduced to make it sync with the timer.
> 
> 
> Diffs
> -----
> 
>   kioslave/ftp/CMakeLists.txt e080b02 
>   kioslave/ftp/ftp.h 0bd375b 
>   kioslave/ftp/ftp.cpp 655524a 
>   kioslave/ftp/speedController.h PRE-CREATION 
>   kioslave/ftp/speedController.cpp PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/102267/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Tushar
> 
>

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