mash: Fold ShelfItemDelegateManager into ShelfModel

ash/common/shelf/shelf_model.cc

Index: ash/common/shelf/shelf_model.cc
diff --git a/ash/common/shelf/shelf_model.cc b/ash/common/shelf/shelf_model.cc
index 7577aec266401e04729694051979798bf839a5cd..00a7c227669087615e2fd98a73c598e75cfe8b6d 100644
--- a/ash/common/shelf/shelf_model.cc
+++ b/ash/common/shelf/shelf_model.cc
@@ -6,6 +6,7 @@
 
 #include <algorithm>
 
+#include "ash/common/shelf/shelf_item_delegate.h"
 #include "ash/common/shelf/shelf_model_observer.h"
 
 namespace ash {
@@ -49,6 +50,12 @@ ShelfModel::ShelfModel() : next_id_(1), status_(STATUS_NORMAL) {}
 
 ShelfModel::~ShelfModel() {}
 
+void ShelfModel::Shutdown() {
+  // Some ShelfItemDelegates access this model in their destructors and hence

[msw, 2016/07/21 21:30:53]

Would it be sufficient to inline this in the dtor? Seems so to me?

[James Cook, 2016/07/22 01:20:07]

I need to do this in an explicit cleanup pass from WmShell such that it will run
before ~WmShell and ~ShelfModel.

[msw, 2016/07/22 01:49:51]

It's not clear to me why this would need to happen before (instead of during)
~ShelfModel and ~WmShell. Do the ShelfItemDelegates access some other
Shelf/Shell state that is destroyed before ShelfModel/WmShell, but after
ShelfModel::Shutdown is called? Does inling fail some automated testing, surface
in some other manner, or is it just assumed given the existing pattern? It's
fine to leave as-is for now, but maybe worth further clarifying this need.

+  // need early cleanup.
+  id_to_item_delegate_map_.clear();
+}
+
 int ShelfModel::Add(const ShelfItem& item) {
   return AddAt(items_.size(), item);
 }
@@ -68,6 +75,10 @@ void ShelfModel::RemoveItemAt(int index) {
          items_[index].type != TYPE_BROWSER_SHORTCUT);
   ShelfID id = items_[index].id;
   items_.erase(items_.begin() + index);
+  RemoveShelfItemDelegate(id);
+  // TODO(jamescook): Fold this into ShelfItemRemoved in existing observers.
+  FOR_EACH_OBSERVER(ShelfModelObserver, observers_,
+                    OnSetShelfItemDelegate(id, nullptr));
   FOR_EACH_OBSERVER(ShelfModelObserver, observers_,
                     ShelfItemRemoved(index, id));
 }
@@ -151,6 +162,28 @@ int ShelfModel::FirstPanelIndex() const {
          items_.begin();
 }
 
+void ShelfModel::SetShelfItemDelegate(
+    ShelfID id,
+    std::unique_ptr<ShelfItemDelegate> item_delegate) {
+  // If another ShelfItemDelegate is already registered for |id|, we assume
+  // that this request is replacing ShelfItemDelegate for |id| with
+  // |item_delegate|.
+  RemoveShelfItemDelegate(id);
+
+  FOR_EACH_OBSERVER(ShelfModelObserver, observers_,
+                    OnSetShelfItemDelegate(id, item_delegate.get()));
+
+  id_to_item_delegate_map_[id] = std::move(item_delegate);
+}
+
+ShelfItemDelegate* ShelfModel::GetShelfItemDelegate(ShelfID id) {
+  if (ItemIndexByID(id) == -1)

[msw, 2016/07/21 21:30:53]

Is checking for the item index useful? Consider this instead:
  if (id_to_item_delegate_map_.find(id) != id_to_item_delegate_map_.end())
    return id_to_item_delegate_map_[id].get();
  return nullptr;

[James Cook, 2016/07/22 01:20:07]

Done. I wonder if this could be related to the shelf view title crash you
(speculatively) fixed in https://codereview.chromium.org/1994253004/ -- maybe
sometimes there's a delegate with no item.

+    return nullptr;
+  // Each ShelfItem has to have a ShelfItemDelegate.
+  DCHECK(id_to_item_delegate_map_.find(id) != id_to_item_delegate_map_.end());
+  return id_to_item_delegate_map_[id].get();
+}
+
 void ShelfModel::AddObserver(ShelfModelObserver* observer) {
   observers_.AddObserver(observer);
 }
@@ -177,4 +210,9 @@ int ShelfModel::ValidateInsertionIndex(ShelfItemType type, int index) const {
   return index;
 }
 
+void ShelfModel::RemoveShelfItemDelegate(ShelfID id) {
+  if (id_to_item_delegate_map_.find(id) != id_to_item_delegate_map_.end())
+    id_to_item_delegate_map_.erase(id);
+}
+
 }  // namespace ash