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