Only show Last N Pages in the UI when the corresponding tab is visible.

components/offline_pages/core/downloads/download_ui_adapter.cc

Index: components/offline_pages/core/downloads/download_ui_adapter.cc
diff --git a/components/offline_pages/core/downloads/download_ui_adapter.cc b/components/offline_pages/core/downloads/download_ui_adapter.cc
index 118efd999d448fbfa05af0708044a97edab37698..9d5546369b5d723277142618483d41f71fec54da 100644
--- a/components/offline_pages/core/downloads/download_ui_adapter.cc
+++ b/components/offline_pages/core/downloads/download_ui_adapter.cc
@@ -18,19 +18,22 @@
 
 namespace offline_pages {
 
-DownloadUIAdapter::ItemInfo::ItemInfo(const OfflinePageItem& page)
+DownloadUIAdapter::ItemInfo::ItemInfo(Delegate* delegate,
+                                      const OfflinePageItem& page)
     : ui_item(base::MakeUnique<DownloadUIItem>(page)),
       is_request(false),
       offline_id(page.offline_id),
       client_id(page.client_id),
-      temporarily_hidden(false) {}
+      temporarily_hidden(delegate->IsTemporarilyHiddenInUI(page.client_id)) {}
 
-DownloadUIAdapter::ItemInfo::ItemInfo(const SavePageRequest& request)
+DownloadUIAdapter::ItemInfo::ItemInfo(Delegate* delegate,
+                                      const SavePageRequest& request)
     : ui_item(base::MakeUnique<DownloadUIItem>(request)),
       is_request(true),
       offline_id(request.request_id()),
       client_id(request.client_id()),
-      temporarily_hidden(false) {}
+      temporarily_hidden(
+          delegate->IsTemporarilyHiddenInUI(request.client_id())) {}
 
 DownloadUIAdapter::ItemInfo::~ItemInfo() {}
 
@@ -85,7 +88,7 @@ void DownloadUIAdapter::OfflinePageAdded(OfflinePageModel* model,
   if (!delegate_->IsVisibleInUI(added_page.client_id))
     return;
 
-  AddItemHelper(base::MakeUnique<ItemInfo>(added_page));
+  AddItemHelper(base::MakeUnique<ItemInfo>(delegate_.get(), added_page));
 }
 
 void DownloadUIAdapter::OfflinePageDeleted(int64_t offline_id,
@@ -100,7 +103,7 @@ void DownloadUIAdapter::OnAdded(const SavePageRequest& added_request) {
   if (!delegate_->IsVisibleInUI(added_request.client_id()))
     return;
 
-  AddItemHelper(base::MakeUnique<ItemInfo>(added_request));
+  AddItemHelper(base::MakeUnique<ItemInfo>(delegate_.get(), added_request));
 }
 
 // RequestCoordinator::Observer
@@ -123,7 +126,7 @@ void DownloadUIAdapter::OnChanged(const SavePageRequest& request) {
     return;
 
   std::string guid = request.client_id().id;
-  items_[guid] = base::MakeUnique<ItemInfo>(request);
+  items_[guid] = base::MakeUnique<ItemInfo>(delegate_.get(), request);
 
   if (state_ != State::LOADED)
     return;
@@ -186,6 +189,9 @@ void DownloadUIAdapter::DeleteItem(const std::string& guid) {
 }
 
 int64_t DownloadUIAdapter::GetOfflineIdByGuid(const std::string& guid) const {
+  if (deleting_item_ && deleting_item_->ui_item->guid == guid)
+    return deleting_item_->offline_id;
+
   DownloadUIItems::const_iterator it = items_.find(guid);
   if (it != items_.end())
     return it->second->offline_id;
@@ -220,9 +226,8 @@ void DownloadUIAdapter::OnOfflinePagesLoaded(
     if (delegate_->IsVisibleInUI(page.client_id)) {
       std::string guid = page.client_id.id;
       DCHECK(items_.find(guid) == items_.end());
-      std::unique_ptr<ItemInfo> item = base::MakeUnique<ItemInfo>(page);
-      item->temporarily_hidden =
-          delegate_->IsTemporarilyHiddenInUI(page.client_id);
+      std::unique_ptr<ItemInfo> item =
+          base::MakeUnique<ItemInfo>(delegate_.get(), page);
       items_[guid] = std::move(item);
     }
   }
@@ -246,9 +251,7 @@ void DownloadUIAdapter::OnRequestsLoaded(
       std::string guid = request->client_id().id;
       DCHECK(items_.find(guid) == items_.end());
       std::unique_ptr<ItemInfo> item =
-          base::MakeUnique<ItemInfo>(*request.get());
-      item->temporarily_hidden =
-          delegate_->IsTemporarilyHiddenInUI(request->client_id());
+          base::MakeUnique<ItemInfo>(delegate_.get(), *request.get());
       items_[guid] = std::move(item);
     }
   }
@@ -285,27 +288,35 @@ void DownloadUIAdapter::AddItemHelper(std::unique_ptr<ItemInfo> item_info) {
     return;
 
   DownloadUIItem* download_ui_item = items_[guid]->ui_item.get();
+
   if (request_to_page_transition) {
     download_ui_item->download_state = DownloadUIItem::DownloadState::COMPLETE;
-    for (Observer& observer : observers_)
-      observer.ItemUpdated(*download_ui_item);
+    if (!items_[guid]->temporarily_hidden) {
+      for (Observer& observer : observers_)
+        observer.ItemUpdated(*download_ui_item);
+    }
   } else {
-    for (Observer& observer : observers_)
-      observer.ItemAdded(*download_ui_item);
+    if (!items_[guid]->temporarily_hidden) {
+      for (Observer& observer : observers_)
+        observer.ItemAdded(*download_ui_item);
+    }
   }
 }
 
 void DownloadUIAdapter::DeleteItemHelper(const std::string& guid) {
-  DownloadUIItems::const_iterator it = items_.find(guid);
+  DownloadUIItems::iterator it = items_.find(guid);
   if (it == items_.end())
     return;
+  DCHECK(deleting_item_ == nullptr);
+  deleting_item_ = std::move(it->second);

[vitaliii, 2017/02/15 10:12:16]

This deleting_item_ approach feels a bit weird.
Since it is used only for obtaining offline_id from GUID, why not providing
offline_id in the ItemDeleted notification?

[dewittj, 2017/02/15 19:49:45]

I'm thinking of a future scenario where we stop using tab ID as the GUID and
provide a helper for getting the tab ID from a GUID.  Then that would also need
to be passed to ItemDeleted.  Perhaps it should just pass the whole ItemInfo
struct back.

Dmitry, any thoughts about this?

   items_.erase(it);
 
-  if (state_ != State::LOADED)
-    return;
+  if (state_ == State::LOADED) {
+    for (Observer& observer : observers_)
+      observer.ItemDeleted(guid);
+  }
 
-  for (Observer& observer : observers_)
-    observer.ItemDeleted(guid);
+  deleting_item_.reset();
 }
 
 }  // namespace offline_pages