mash: Fold ShelfItemDelegateManager into ShelfModel

ash/common/shelf/shelf_model.cc

 // Copyright 2013 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 "ash/common/shelf/shelf_model.h"
 
 #include <algorithm>
 
+#include "ash/common/shelf/shelf_item_delegate.h"
 #include "ash/common/shelf/shelf_model_observer.h"
 
 namespace ash {
 
 namespace {
 
 int ShelfItemTypeToWeight(ShelfItemType type) {
   switch (type) {
     case TYPE_APP_LIST:
       // TODO(skuhne): If the app list item becomes movable again, this need
@@ skipping 23 matching lines @@
 bool CompareByWeight(const ShelfItem& a, const ShelfItem& b) {
   return ShelfItemTypeToWeight(a.type) < ShelfItemTypeToWeight(b.type);
 }
 
 }  // namespace
 
 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);
 }
 
 int ShelfModel::AddAt(int index, const ShelfItem& item) {
   index = ValidateInsertionIndex(item.type, index);
   items_.insert(items_.begin() + index, item);
   items_[index].id = next_id_++;
   FOR_EACH_OBSERVER(ShelfModelObserver, observers_, ShelfItemAdded(index));
   return index;
 }
 
 void ShelfModel::RemoveItemAt(int index) {
   DCHECK(index >= 0 && index < item_count());
   // The app list and browser shortcut can't be removed.
   DCHECK(items_[index].type != TYPE_APP_LIST &&
          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));
 }
 
 void ShelfModel::Move(int index, int target_index) {
   if (index == target_index)
     return;
   // TODO: this needs to enforce valid ranges.
   ShelfItem item(items_[index]);
   items_.erase(items_.begin() + index);
@@ skipping 63 matching lines @@
 }
 
 int ShelfModel::FirstPanelIndex() const {
   ShelfItem weight_dummy;
   weight_dummy.type = TYPE_APP_PANEL;
   return std::lower_bound(items_.begin(), items_.end(), weight_dummy,
                           CompareByWeight) -
          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);
 }
 
 void ShelfModel::RemoveObserver(ShelfModelObserver* observer) {
   observers_.RemoveObserver(observer);
 }
 
 int ShelfModel::ValidateInsertionIndex(ShelfItemType type, int index) const {
   DCHECK(index >= 0 && index <= item_count() + 1);
 
   // Clamp |index| to the allowed range for the type as determined by |weight|.
   ShelfItem weight_dummy;
   weight_dummy.type = type;
   index = std::max(std::lower_bound(items_.begin(), items_.end(), weight_dummy,
                                     CompareByWeight) -
                        items_.begin(),
                    static_cast<ShelfItems::difference_type>(index));
   index = std::min(std::upper_bound(items_.begin(), items_.end(), weight_dummy,
                                     CompareByWeight) -
                        items_.begin(),
                    static_cast<ShelfItems::difference_type>(index));
 
   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