Refactor Reading List Model to use URL as key.

ios/chrome/browser/reading_list/reading_list_download_service.cc

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "ios/chrome/browser/reading_list/reading_list_download_service.h"
 
 #include <utility>
 
 #include "base/bind.h"
 #include "base/files/file_path.h"
@@ skipping 40 matching lines @@
 void ReadingListDownloadService::Shutdown() {
   reading_list_model_->RemoveObserver(this);
 }
 
 void ReadingListDownloadService::ReadingListModelLoaded(
     const ReadingListModel* model) {
   DCHECK_EQ(reading_list_model_, model);
   DownloadAllEntries();
 }
 
-void ReadingListDownloadService::ReadingListWillRemoveReadEntry(
+void ReadingListDownloadService::ReadingListWillRemoveEntry(
     const ReadingListModel* model,
-    size_t index) {
+    const GURL& url) {
   DCHECK_EQ(reading_list_model_, model);
-  RemoveDownloadedEntry(model->GetReadEntryAtIndex(index));
+  DCHECK(model->GetEntryByURL(url));
+  RemoveDownloadedEntry(url);
 }
 
-void ReadingListDownloadService::ReadingListWillRemoveUnreadEntry(
+void ReadingListDownloadService::ReadingListDidAddEntry(
     const ReadingListModel* model,
-    size_t index) {
+    const GURL& url) {
   DCHECK_EQ(reading_list_model_, model);
-  RemoveDownloadedEntry(model->GetUnreadEntryAtIndex(index));
-}
-
-void ReadingListDownloadService::ReadingListWillAddUnreadEntry(
-    const ReadingListModel* model,
-    const ReadingListEntry& entry) {
-  DCHECK_EQ(reading_list_model_, model);
-  ScheduleDownloadEntry(entry);
-}
-
-void ReadingListDownloadService::ReadingListWillAddReadEntry(
-    const ReadingListModel* model,
-    const ReadingListEntry& entry) {
-  DCHECK_EQ(reading_list_model_, model);
-  ScheduleDownloadEntry(entry);
+  ScheduleDownloadEntry(url);
 }
 
 void ReadingListDownloadService::DownloadAllEntries() {
   DCHECK(reading_list_model_->loaded());
-  size_t size = reading_list_model_->unread_size();
-  for (size_t i = 0; i < size; i++) {
-    const ReadingListEntry& entry =
-        reading_list_model_->GetUnreadEntryAtIndex(i);
-    this->ScheduleDownloadEntry(entry);
-  }
-  size = reading_list_model_->read_size();
-  for (size_t i = 0; i < size; i++) {
-    const ReadingListEntry& entry = reading_list_model_->GetReadEntryAtIndex(i);
-    this->ScheduleDownloadEntry(entry);
+  for (const auto& iterator : *reading_list_model_) {
+    this->ScheduleDownloadEntry(iterator.first);
   }
 }
 
-void ReadingListDownloadService::ScheduleDownloadEntry(
-    const ReadingListEntry& entry) {
+void ReadingListDownloadService::ScheduleDownloadEntry(const GURL& url) {
   DCHECK(reading_list_model_->loaded());
-  if (entry.DistilledState() == ReadingListEntry::ERROR ||
-      entry.DistilledState() == ReadingListEntry::PROCESSED)
+  const ReadingListEntry* entry = reading_list_model_->GetEntryByURL(url);
+  if (!entry || entry->DistilledState() == ReadingListEntry::ERROR ||
+      entry->DistilledState() == ReadingListEntry::PROCESSED)
     return;
-
+  GURL local_url(url);

[jif-google, 2016/11/28 16:28:21]

I don't understand why we need to use a local GURL.
|local_url| won't outlive |url|.

[Olivier, 2016/11/28 17:58:25]

Because it will be posted on another thread.
I will ask sdefresne tomorrow.

[jif, 2016/11/28 18:15:31]

Spoiler:
https://cs.chromium.org/chromium/src/docs/callback.md?q=callback.md&sq=packag...

So it seems you can use |url| directly.

[Olivier, 2016/11/30 09:15:22]

Done.

   web::WebThread::PostDelayedTask(
       web::WebThread::UI, FROM_HERE,
-      base::Bind(&ReadingListDownloadService::DownloadEntryFromURL,
-                 weak_ptr_factory_.GetWeakPtr(), entry.URL()),
-      entry.TimeUntilNextTry());
+      base::Bind(&ReadingListDownloadService::DownloadEntry,
+                 weak_ptr_factory_.GetWeakPtr(), local_url),
+      entry->TimeUntilNextTry());
 }
 
-void ReadingListDownloadService::ScheduleDownloadEntryFromURL(const GURL& url) {
-  auto download_callback =
-      base::Bind(&ReadingListDownloadService::ScheduleDownloadEntry,
-                 base::Unretained(this));
-  reading_list_model_->CallbackEntryURL(url, download_callback);
-}
-
-void ReadingListDownloadService::DownloadEntryFromURL(const GURL& url) {
-  auto download_callback = base::Bind(
-      &ReadingListDownloadService::DownloadEntry, base::Unretained(this));
-  reading_list_model_->CallbackEntryURL(url, download_callback);
-}
-
-void ReadingListDownloadService::DownloadEntry(const ReadingListEntry& entry) {
+void ReadingListDownloadService::DownloadEntry(const GURL& url) {
   DCHECK(reading_list_model_->loaded());
-  if (entry.DistilledState() == ReadingListEntry::ERROR ||
-      entry.DistilledState() == ReadingListEntry::PROCESSED)
+  const ReadingListEntry* entry = reading_list_model_->GetEntryByURL(url);
+  if (!entry || entry->DistilledState() == ReadingListEntry::ERROR ||
+      entry->DistilledState() == ReadingListEntry::PROCESSED)
     return;
 
   if (net::NetworkChangeNotifier::IsOffline()) {
     // There is no connection, save it for download only if we did not exceed
     // the maximaxum number of tries.
-    if (entry.FailedDownloadCounter() < kNumberOfFailsBeforeWifiOnly)
-      url_to_download_cellular_.push_back(entry.URL());
-    if (entry.FailedDownloadCounter() < kNumberOfFailsBeforeStop)
-      url_to_download_wifi_.push_back(entry.URL());
+    if (entry->FailedDownloadCounter() < kNumberOfFailsBeforeWifiOnly)
+      url_to_download_cellular_.push_back(entry->URL());
+    if (entry->FailedDownloadCounter() < kNumberOfFailsBeforeStop)
+      url_to_download_wifi_.push_back(entry->URL());
     return;
   }
 
   // There is a connection.
-  if (entry.FailedDownloadCounter() < kNumberOfFailsBeforeWifiOnly) {
+  if (entry->FailedDownloadCounter() < kNumberOfFailsBeforeWifiOnly) {
     // Try to download the page, whatever the connection.
-    reading_list_model_->SetEntryDistilledState(entry.URL(),
+    reading_list_model_->SetEntryDistilledState(entry->URL(),
                                                 ReadingListEntry::PROCESSING);
-    url_downloader_->DownloadOfflineURL(entry.URL());
+    url_downloader_->DownloadOfflineURL(entry->URL());
 
-  } else if (entry.FailedDownloadCounter() < kNumberOfFailsBeforeStop) {
+  } else if (entry->FailedDownloadCounter() < kNumberOfFailsBeforeStop) {
     // Try to download the page only if the connection is wifi.
     if (net::NetworkChangeNotifier::GetConnectionType() ==
         net::NetworkChangeNotifier::CONNECTION_WIFI) {
       // The connection is wifi, download the page.
-      reading_list_model_->SetEntryDistilledState(entry.URL(),
+      reading_list_model_->SetEntryDistilledState(entry->URL(),
                                                   ReadingListEntry::PROCESSING);
-      url_downloader_->DownloadOfflineURL(entry.URL());
+      url_downloader_->DownloadOfflineURL(entry->URL());
 
     } else {
       // The connection is not wifi, save it for download when the connection
       // changes to wifi.
-      url_to_download_wifi_.push_back(entry.URL());
+      url_to_download_wifi_.push_back(entry->URL());
     }
   }
 }
 
-void ReadingListDownloadService::RemoveDownloadedEntry(
-    const ReadingListEntry& entry) {
+void ReadingListDownloadService::RemoveDownloadedEntry(const GURL& url) {
   DCHECK(reading_list_model_->loaded());
-  url_downloader_->RemoveOfflineURL(entry.URL());
+  url_downloader_->RemoveOfflineURL(url);
 }
 
 void ReadingListDownloadService::OnDownloadEnd(
     const GURL& url,
     URLDownloader::SuccessState success,
     const base::FilePath& distilled_path,
     const std::string& title) {
   DCHECK(reading_list_model_->loaded());
   if ((success == URLDownloader::DOWNLOAD_SUCCESS ||
        success == URLDownloader::DOWNLOAD_EXISTS) &&
       !distilled_path.empty()) {
     reading_list_model_->SetEntryDistilledPath(url, distilled_path);
 
   } else if (success == URLDownloader::ERROR_RETRY) {
     reading_list_model_->SetEntryDistilledState(url,
                                                 ReadingListEntry::WILL_RETRY);
-    ScheduleDownloadEntryFromURL(url);
-
+    ScheduleDownloadEntry(url);
   } else if (success == URLDownloader::ERROR_PERMANENT) {
     reading_list_model_->SetEntryDistilledState(url, ReadingListEntry::ERROR);
   }
 }
 
 void ReadingListDownloadService::OnDeleteEnd(const GURL& url, bool success) {
   // Nothing to update as this is only called when deleting reading list entries
 }
 
 void ReadingListDownloadService::OnConnectionTypeChanged(
     net::NetworkChangeNotifier::ConnectionType type) {
   if (type == net::NetworkChangeNotifier::CONNECTION_NONE) {
     had_connection_ = false;
     return;
   }
 
   if (!had_connection_) {
     had_connection_ = true;
     for (auto& url : url_to_download_cellular_) {
-      ScheduleDownloadEntryFromURL(url);
+      ScheduleDownloadEntry(url);
     }
   }
   if (type == net::NetworkChangeNotifier::CONNECTION_WIFI) {
     for (auto& url : url_to_download_wifi_) {
-      ScheduleDownloadEntryFromURL(url);
+      ScheduleDownloadEntry(url);
     }
   }
 }