Clementine Vkontakte internet service

[davidsansome, 2012-01-07 17:24:46]

http://codereview.appspot.com/5492070/diff/1/src/CMakeLists.txt
File src/CMakeLists.txt (right):

http://codereview.appspot.com/5492070/diff/1/src/CMakeLists.txt#newcode351
src/CMakeLists.txt:351: core/throttlednetworkmanager.h
Can you move this line up so it's in alphabetical order?

http://codereview.appspot.com/5492070/diff/1/src/core/throttlednetworkmanager...
File src/core/throttlednetworkmanager.cpp (right):

http://codereview.appspot.com/5492070/diff/1/src/core/throttlednetworkmanager...
src/core/throttlednetworkmanager.cpp:23: mutex_ = new QMutex();
Create these in the constructor's initialisation list.  Also it looks like
mutex_ will leak, make it a normal member instead of a pointer so you don't have
to delete it.

http://codereview.appspot.com/5492070/diff/1/src/core/throttlednetworkmanager...
src/core/throttlednetworkmanager.cpp:40: locker.unlock();
You probably don't need to unlock this here.  In fact won't there be a race with
stopping/starting the timer again in ProcessQueue()?

http://codereview.appspot.com/5492070/diff/1/src/core/throttlednetworkmanager...
src/core/throttlednetworkmanager.cpp:41: emit TimeToProcessQueue();
Instead of having the overhead of signal emission here, just do
metaObject()->invokeMethod(this, "ProcessQueue", Qt::QueuedConnection);

http://codereview.appspot.com/5492070/diff/1/src/core/throttlednetworkmanager.h
File src/core/throttlednetworkmanager.h (right):

http://codereview.appspot.com/5492070/diff/1/src/core/throttlednetworkmanager...
src/core/throttlednetworkmanager.h:15: class NetworkReply: public QObject
Instead of having this wrapper, could you return the real QNetworkReply* from
throttledGet and moveToThread(thread()) on it?

http://codereview.appspot.com/5492070/diff/1/src/core/throttlednetworkmanager...
src/core/throttlednetworkmanager.h:36: class ThrottledNetworkManager : public
QNetworkAccessManager
Maybe inherit from NetworkAccessManager in network.h so the User-Agent and
caching gets set correctly.

http://codereview.appspot.com/5492070/diff/1/src/internet/vkontakteservice.cpp
File src/internet/vkontakteservice.cpp (right):

http://codereview.appspot.com/5492070/diff/1/src/internet/vkontakteservice.cp...
src/internet/vkontakteservice.cpp:67: qRegisterMetaType< QList<QStringList>
>("QList<QStringList>");
Maybe move this to main.cpp with the others.

http://codereview.appspot.com/5492070/diff/1/src/internet/vkontakteservice.cp...
src/internet/vkontakteservice.cpp:191: foreach (QModelIndex i, selection) {
const QModelIndex& i

http://codereview.appspot.com/5492070/diff/1/src/internet/vkontakteservice.cp...
src/internet/vkontakteservice.cpp:598: void VkontakteService::Log(QString error)
{
const QString&

http://codereview.appspot.com/5492070/diff/1/src/internet/vkontakteservice.h
File src/internet/vkontakteservice.h (right):

http://codereview.appspot.com/5492070/diff/1/src/internet/vkontakteservice.h#...
src/internet/vkontakteservice.h:35: typedef
BackgroundThreadImplementation<VkontakteApiWorker,VkontakteApiWorker>
BgApiWorker;
Put these typedefs in a namespace or a class.

http://codereview.appspot.com/5492070/diff/1/src/internet/vkontaktesettingspa...
File src/internet/vkontaktesettingspage.h (right):

http://codereview.appspot.com/5492070/diff/1/src/internet/vkontaktesettingspa...
src/internet/vkontaktesettingspage.h:40: void RequestCaptcha(QUrl url);
const QUrl&

http://codereview.appspot.com/5492070/diff/1/src/internet/vkontaktesettingspa...
src/internet/vkontaktesettingspage.h:56: void LoginFinished(VkontakteAuthResult
result);
const VkontakteAuthResult&

http://codereview.appspot.com/5492070/diff/1/src/internet/vkontaktesettingspa...
src/internet/vkontaktesettingspage.h:69: bool logged_in_;
Doesn't the VkontakteService have some of these members too?

http://codereview.appspot.com/5492070/diff/1/src/internet/vkontakteworker.cpp
File src/internet/vkontakteworker.cpp (right):

http://codereview.appspot.com/5492070/diff/1/src/internet/vkontakteworker.cpp...
src/internet/vkontakteworker.cpp:16: timer_ = new QTimer(this);
Do this in the initialisation list

http://codereview.appspot.com/5492070/diff/1/src/internet/vkontakteworker.cpp...
src/internet/vkontakteworker.cpp:43: foreach (QUrl url, urls) {
const QUrl&

http://codereview.appspot.com/5492070/diff/1/src/internet/vkontakteworker.cpp...
src/internet/vkontakteworker.cpp:66:
timer_->singleShot(1500,this,SLOT(ProcessAddQueue()));
I'd prefer these delays to be put in constants

http://codereview.appspot.com/5492070/diff/1/src/internet/vkontakteworker.cpp...
src/internet/vkontakteworker.cpp:72: foreach (QUrl url, urls) {
const QUrl&

http://codereview.appspot.com/5492070/diff/1/src/internet/vkontakteworker.cpp...
src/internet/vkontakteworker.cpp:72: foreach (QUrl url, urls) {
const QUrl&

http://codereview.appspot.com/5492070/diff/1/src/internet/vkontakteworker.cpp...
src/internet/vkontakteworker.cpp:110: foreach (QUrl url, songs) {
const QUrl&

http://codereview.appspot.com/5492070/diff/1/src/internet/vkontakteworker.cpp...
src/internet/vkontakteworker.cpp:145:
songs_ids_to_move_.remove(songs_ids_to_move_.size()-1,1); //remove last comma
Instead of doing this, how about making songs_ids_to_move_ a QStringList, and
doing .join(",") on it here.