Fix code review comments for r313.

src/albumcoverfetcher.cpp

@@ -9,15 +9,12 @@
 
 const int AlbumCoverFetcher::kMaxConcurrentRequests = 5;
 
-AlbumCoverFetcher::AlbumCoverFetcher(QObject* parent, QNetworkAccessManager* network)
+AlbumCoverFetcher::AlbumCoverFetcher(QNetworkAccessManager* network, QObject* parent)
     : QObject(parent),
       network_(network),
       next_id_(0),
       request_starter_(new QTimer(this))
 {
-  if (!network_.get()) {
-    network_.reset(new QNetworkAccessManager);
-  }
   request_starter_->setInterval(1000);
   connect(request_starter_, SIGNAL(timeout()), SLOT(StartRequests()));
 }

src/albumcoverfetcher.h

@@ -18,7 +18,7 @@ class AlbumCoverFetcher : public QObject {
   Q_OBJECT
 
  public:
-  AlbumCoverFetcher(QObject* parent = 0, QNetworkAccessManager* network_ = 0);
+  AlbumCoverFetcher(QNetworkAccessManager* network, QObject* parent = 0);
   virtual ~AlbumCoverFetcher() {}
 
   static const int kMaxConcurrentRequests;
@@ -42,7 +42,7 @@ class AlbumCoverFetcher : public QObject {
     QString album;
   };
 
-  boost::scoped_ptr<QNetworkAccessManager> network_;
+  QNetworkAccessManager* network_;
   quint64 next_id_;
 
   QQueue<QueuedRequest> queued_requests_;

src/albumcovermanager.cpp

@@ -19,11 +19,11 @@
 
 const char* AlbumCoverManager::kSettingsGroup = "CoverManager";
 
-AlbumCoverManager::AlbumCoverManager(QWidget *parent)
+AlbumCoverManager::AlbumCoverManager(QNetworkAccessManager* network, QWidget *parent)
   : QDialog(parent),
     constructed_(false),
     cover_loader_(new BackgroundThread<AlbumCoverLoader>(this)),
-    cover_fetcher_(new AlbumCoverFetcher(this)),
+    cover_fetcher_(new AlbumCoverFetcher(network, this)),
     artist_icon_(":/artist.png"),
     all_artists_icon_(":/album.png"),
     context_menu_(new QMenu(this))

src/albumcovermanager.h

@@ -13,10 +13,12 @@
 class LibraryBackend;
 class AlbumCoverFetcher;
 
+class QNetworkAccessManager;
+
 class AlbumCoverManager : public QDialog {
   Q_OBJECT
  public:
-  AlbumCoverManager(QWidget *parent = 0);
+  AlbumCoverManager(QNetworkAccessManager* network, QWidget *parent = 0);
   ~AlbumCoverManager();
 
   static const char* kSettingsGroup;

src/main.cpp

@@ -14,6 +14,7 @@
 #include <QLibraryInfo>
 #include <QTranslator>
 #include <QDir>
+#include <QNetworkAccessManager>
 
 void LoadTranslation(const QString& prefix, const QString& path) {
   QTranslator* t = new QTranslator;
@@ -52,8 +53,10 @@ int main(int argc, char *argv[]) {
   LoadTranslation("clementine", a.applicationDirPath());
   LoadTranslation("clementine", QDir::currentPath());
 
+  QNetworkAccessManager network;
+
   // Window
-  MainWindow w;
+  MainWindow w(&network);;
   a.setActivationWindow(&w);
 
   QObject::connect(&a, SIGNAL(messageReceived(QString)), &w, SLOT(show()));

src/mainwindow.cpp

@@ -39,7 +39,7 @@
 const int MainWindow::kStateVersion = 1;
 const char* MainWindow::kSettingsGroup = "MainWindow";
 
-MainWindow::MainWindow(QWidget *parent)
+MainWindow::MainWindow(QNetworkAccessManager* network, QWidget *parent)
   : QMainWindow(parent),
     tray_icon_(new SystemTrayIcon(this)),
     osd_(new OSD(tray_icon_, this)),
@@ -55,7 +55,7 @@ MainWindow::MainWindow(QWidget *parent)
     settings_dialog_(new SettingsDialog(this)),
     add_stream_dialog_(new AddStreamDialog(this)),
     shortcuts_dialog_(new ShortcutsDialog(this)),
-    cover_manager_(new AlbumCoverManager(this)),
+    cover_manager_(new AlbumCoverManager(network, this)),
     playlist_menu_(new QMenu(this)),
     library_sort_model_(new QSortFilterProxyModel(this)),
     track_position_timer_(new QTimer(this))

src/mainwindow.h

@@ -27,11 +27,13 @@ class AlbumCoverManager;
 class QSortFilterProxyModel;
 class SystemTrayIcon;
 
+class QNetworkAccessManager;
+
 class MainWindow : public QMainWindow {
   Q_OBJECT
 
  public:
-  MainWindow(QWidget *parent = 0);
+  MainWindow(QNetworkAccessManager* network, QWidget *parent = 0);
   ~MainWindow();
 
   void SetHiddenInTray(bool hidden);

tests/albumcoverfetcher_test.cpp

@@ -23,6 +23,7 @@ class AlbumCoverFetcherTest : public ::testing::Test {
   }
 
   void SetUp() {
+    // Lastfm takes ownership of this.
     network_ = new MockNetworkAccessManager;
     lastfm::setNetworkAccessManager(network_);
   }
@@ -44,7 +45,7 @@ TEST_F(AlbumCoverFetcherTest, FetchesAlbumCover) {
   params.clear();
   MockNetworkReply* album_reply = network_->ExpectGet("http://example.com/image.jpg", params, 200, "");
 
-  AlbumCoverFetcher fetcher(NULL, network_);
+  AlbumCoverFetcher fetcher(network_, NULL);
   QSignalSpy spy(&fetcher, SIGNAL(AlbumCoverFetched(quint64, const QImage&)));
   ASSERT_TRUE(spy.isValid());
   fetcher.FetchAlbumCover("Foo", "Bar");

tests/mock_networkaccessmanager.cpp

@@ -26,8 +26,8 @@ class RequestForUrlMatcher : public MatcherInterface<const QNetworkRequest&> {
       return false;
     }
 
-    for (QMap<QString, QString>::const_iterator it = expected_params_.begin();
-         it != expected_params_.end(); ++it) {
+    for (QMap<QString, QString>::const_iterator it = expected_params_.constBegin();
+         it != expected_params_.constEnd(); ++it) {
       if (!url.hasQueryItem(it.key()) ||
           url.queryItemValue(it.key()) != it.value()) {
         return false;
@@ -55,9 +55,9 @@ MockNetworkReply* MockNetworkAccessManager::ExpectGet(
     const QString& contains,
     const QMap<QString, QString>& expected_params,
     int status,
-    const char* data) {
+    const QByteArray& data) {
   MockNetworkReply* reply = new MockNetworkReply(data);
-  reply->setAttribute(QNetworkRequest::HttpStatusCodeAttribute, 200);
+  reply->setAttribute(QNetworkRequest::HttpStatusCodeAttribute, status);
 
   EXPECT_CALL(*this, createRequest(
       GetOperation, RequestForUrl(contains, expected_params), NULL)).
@@ -66,32 +66,26 @@ MockNetworkReply* MockNetworkAccessManager::ExpectGet(
   return reply;
 }
 
-MockNetworkAccessManager::~MockNetworkAccessManager() {
-}
-
 MockNetworkReply::MockNetworkReply()
-    : data_(NULL),
-      size_(0) {
+    : data_(NULL) {
 }
 
-MockNetworkReply::MockNetworkReply(const char* data)
+MockNetworkReply::MockNetworkReply(const QByteArray& data)
     : data_(data),
-      size_(strlen(data)),
       pos_(0) {
 }
 
-void MockNetworkReply::SetData(const char* data) {
+void MockNetworkReply::SetData(const QByteArray& data) {
   data_ = data;
-  size_ = strlen(data);
   pos_ = 0;
 }
 
 qint64 MockNetworkReply::readData(char* data, qint64 size) {
-  if (size_ == pos_) {
+  if (data_.size() == pos_) {
     return -1;
   }
-  qint64 bytes_to_read = min(size_ - pos_, size);
-  memcpy(data, data_, bytes_to_read);
+  qint64 bytes_to_read = min(data_.size() - pos_, size);
+  memcpy(data, data_.constData() + pos_, bytes_to_read);
   pos_ += bytes_to_read;
   return bytes_to_read;
 }

tests/mock_networkaccessmanager.h

@@ -1,7 +1,7 @@
 #ifndef MOCK_NETWORKACCESSAMANGER_H
 #define MOCK_NETWORKACCESSMANAGER_H
 
-#include <QList>
+#include <QByteArray>
 #include <QMap>
 #include <QNetworkAccessManager>
 #include <QNetworkReply>
@@ -10,16 +10,23 @@
 #include "test_utils.h"
 #include "gmock/gmock.h"
 
+// Usage:
+// Create a MockNetworkAccessManager.
+// Call ExpectGet() with appropriate expectations and the data you want back.
+// This will return a MockNetworkReply*. When you are ready for the reply to
+// arrive, call MockNetworkReply::Done().
+
 class MockNetworkReply : public QNetworkReply {
   Q_OBJECT
  public:
   MockNetworkReply();
-  MockNetworkReply(const char* data);
-  virtual ~MockNetworkReply() {}
+  MockNetworkReply(const QByteArray& data);
 
-  void SetData(const char* data);
+  // Use these to set expectations.
+  void SetData(const QByteArray& data);
   virtual void setAttribute(QNetworkRequest::Attribute code, const QVariant& value);
 
+  // Call this when you are ready for the finished() signal.
   void Done();
 
  protected:
@@ -27,8 +34,7 @@ class MockNetworkReply : public QNetworkReply {
   virtual qint64 readData(char* data, qint64);
   virtual qint64 writeData(const char* data, qint64);
 
-  const char* data_;
-  qint64 size_;
+  QByteArray data_;
   qint64 pos_;
 };
 
@@ -36,12 +42,11 @@ class MockNetworkReply : public QNetworkReply {
 class MockNetworkAccessManager : public QNetworkAccessManager {
   Q_OBJECT
  public:
-  virtual ~MockNetworkAccessManager();
   MockNetworkReply* ExpectGet(
-      const QString& contains,
-      const QMap<QString, QString>& params,
-      int status,
-      const char* ret_data);
+      const QString& contains,  // A string that should be present in the URL.
+      const QMap<QString, QString>& params,  // Required URL parameters.
+      int status,  // Returned HTTP status code.
+      const QByteArray& ret_data);  // Returned data.
  protected:
   MOCK_METHOD3(createRequest, QNetworkReply*(Operation, const QNetworkRequest&, QIODevice*));
 };