Offline Pages: Replace Observer::OfflinePageModelChanged with OfflinePageAdded.

components/offline_pages/downloads/download_ui_adapter.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 "components/offline_pages/downloads/download_ui_adapter.h"
 
 #include "base/bind.h"
 #include "base/guid.h"
 #include "base/logging.h"
 #include "base/memory/ptr_util.h"
@@ skipping 58 matching lines @@
   DCHECK(observer);
   if (!observers_.HasObserver(observer))
     return;
   observers_.RemoveObserver(observer);
   --observers_count_;
   // Once the last observer is gone, clear cached data.
   if (observers_count_ == 0)
     ClearCache();
 }
 
-void DownloadUIAdapter::OfflinePageModelLoaded(OfflinePageModel* model) {
-  // This signal is not used here.
-}
+void DownloadUIAdapter::OfflinePageAdded(OfflinePageModel* model,
+                                         SavePageResult result,
+                                         const OfflinePageItem* added) {
+  DCHECK(model == model_);
+  if (!added || !IsVisibleInUI(added->client_id))
+    return;
 
-void DownloadUIAdapter::OfflinePageModelChanged(OfflinePageModel* model) {
-  DCHECK(model == model_);
-  model_->GetAllPages(
-      base::Bind(&DownloadUIAdapter::OnOfflinePagesChanged,
-                 weak_ptr_factory_.GetWeakPtr()));
+  const std::string& guid = added->client_id.id;
+  if (items_.find(guid) != items_.end())  // Item already exists.

[fgorski, 2016/11/02 15:51:06]

If item already exists this should probably never happen, therefore at least a
DCHECK? Or am I missing something?

[dewittj, 2016/11/02 16:34:37]

I was trying not to change the logic from OnOfflinePagesChanged, but this could
indeed be a DCHECK.

+    return;
+
+  items_[guid] = base::MakeUnique<ItemInfo>(*added);
+  const DownloadUIItem& item = *(items_[guid]->ui_item);
+  for (Observer& observer : observers_)
+    observer.ItemAdded(item);
 }
 
 void DownloadUIAdapter::OfflinePageDeleted(
   int64_t offline_id, const ClientId& client_id) {
   if (!IsVisibleInUI(client_id))
     return;
   std::string guid = client_id.id;
   DownloadUIItems::const_iterator it = items_.find(guid);
   if (it == items_.end())
     return;
@@ skipping 80 matching lines @@
   state_ = State::LOADED;
   for (Observer& observer : observers_)
     observer.ItemsLoaded();
 }
 
 void DownloadUIAdapter::NotifyItemsLoaded(Observer* observer) {
   if (observer && observers_.HasObserver(observer))
     observer->ItemsLoaded();
 }
 
-// This method is only called by OPM when a single item added.
-// TODO(dimich): change OPM to have real OnPageAdded/OnPageUpdated and
-// simplify this code.
-void DownloadUIAdapter::OnOfflinePagesChanged(
-    const MultipleOfflinePageItemResult& pages) {
-  std::vector<std::string> added_guids;
-  for (const auto& page : pages) {
-    if (!IsVisibleInUI(page.client_id))  // Item should be filtered out.
-      continue;
-    const std::string& guid = page.client_id.id;
-    if (items_.find(guid) != items_.end())  // Item already exists.
-      continue;
-    items_[guid] = base::MakeUnique<ItemInfo>(page);
-    added_guids.push_back(guid);
-  }
-  for (auto& guid : added_guids) {
-    const DownloadUIItem& item = *(items_.find(guid)->second->ui_item.get());
-    for (Observer& observer : observers_)
-      observer.ItemAdded(item);
-  }
-}
 
 void DownloadUIAdapter::OnDeletePagesDone(DeletePageResult result) {
   // TODO(dimich): Consider adding UMA to record user actions.
 }
 
 bool DownloadUIAdapter::IsVisibleInUI(const ClientId& client_id) {
   const std::string& name_space = client_id.name_space;
   return model_->GetPolicyController()->IsSupportedByDownload(name_space) &&
          base::IsValidGUID(client_id.id);
 }
 
 }  // namespace offline_pages