Protect AppListItemList Add/Remove and fix sync bugs

ui/app_list/app_list_model.h

 // Copyright (c) 2012 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.
 
 #ifndef UI_APP_LIST_APP_LIST_MODEL_H_
 #define UI_APP_LIST_APP_LIST_MODEL_H_
 
 #include "base/basictypes.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/observer_list.h"
 #include "ui/app_list/app_list_export.h"
 #include "ui/app_list/app_list_item_list.h"
+#include "ui/app_list/app_list_item_list_observer.h"
 #include "ui/base/models/list_model.h"
 
 namespace app_list {
 
+class AppListFolderItem;
 class AppListItem;
 class AppListItemList;
 class AppListModelObserver;
 class SearchBoxModel;
 class SearchResult;
 
 // Master model of app list that consists of three sub models: AppListItemList,
 // SearchBoxModel and SearchResults. The AppListItemList sub model owns a list
 // of AppListItems and is displayed in the grid view. SearchBoxModel is
 // the model for SearchBoxView. SearchResults owns a list of SearchResult.
-class APP_LIST_EXPORT AppListModel {
+// NOTE: Currently this class observes |item_list_|. The View code may
+// move entries in the item list directly (but can not add or remove them) and
+// the model needs to notify its observers when this occurs.
+
+class APP_LIST_EXPORT AppListModel : public AppListItemListObserver {
  public:
   enum Status {
     STATUS_NORMAL,
     STATUS_SYNCING,  // Syncing apps or installing synced apps.
   };
 
   typedef ui::ListModel<SearchResult> SearchResults;
 
   AppListModel();
-  ~AppListModel();
+  virtual ~AppListModel();
 
   void AddObserver(AppListModelObserver* observer);
   void RemoveObserver(AppListModelObserver* observer);
 
   void SetStatus(Status status);
 
+  // Finds the item matching |id|.
+  AppListItem* FindItem(const std::string& id);
+
+  // Adds |item| to the model.
+  void AddItem(AppListItem* item);

[tapted, 2014/01/28 11:56:53]

I think the preferred way to document the ownership transferral here is to take
a `scoped_ptr<AppListItem> item` (and .Pass, etc. when not called with a raw
pointer, rather than .release()). More below, but those are private functions so
you could maybe argue against scoped_ptr

[stevenjb, 2014/01/28 18:28:08]

If you'll pardon the pun, I wanted to limit the scope of this change. We call
this a lot from tests (previously we called model->item_list->AddItem), and
changing those all to use scoped pointers would be tedious. Maybe in another
cleanup pass. I will add a comment about ownership though. (I don't personally
believe strongly in using scoping to enforce ownership; this is still C++, not
Java).

[tapted, 2014/01/28 23:33:54]

I was going by a chromium dev thread -
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/9Q2pc9jqxMQ/94AZw...
. Latest word was "use raw pointers only when ownership is not passed."

TBH, the ownership transferrals here are pretty hard to follow, so I would be in
favour of scoped_ptr.

In tests, I think it mostly goes through AppListTestModel so shouldn't make too
much churn.

[stevenjb, 2014/01/29 00:11:31]

Yeah, that's generally the consensus, and I don't disagree that it would be more
clear here. You make a good point about AppListTestModel, the change may not be
as bad as I fear, I will definitely consider it the next time I am in this code.

+
+  // Merges two items. If the target item is a folder, the source item is added
+  // to that folder, otherwise a new folder is created in the same position as
+  // the target item.  Returns the id of the target folder.
+  const std::string& MergeItems(const std::string& target_item_id,
+                                const std::string& source_item_id);
+
+  // Sets the position of |item| in |item_list_|.
+  void SetItemPosition(AppListItem* item,
+                       const syncer::StringOrdinal& new_position);
+
+  // Deletes the item matching |id| from |item_list_|.
+  void DeleteItem(const std::string& id);
+
   AppListItemList* item_list() { return item_list_.get(); }
   SearchBoxModel* search_box() { return search_box_.get(); }
   SearchResults* results() { return results_.get(); }
   Status status() const { return status_; }
 
  private:
+  // AppListItemListObserver
+  virtual void OnListItemMoved(size_t from_index,

[tapted, 2014/01/28 11:56:53]

nit: OVERRIDEs last?

[stevenjb, 2014/01/28 18:28:08]

I'm unaware of that convention, and prefer to list overrides before fully
private methods which tend to be implementation details.

[tapted, 2014/01/28 23:33:54]

I thought it was convention, but don't really feel strongly. I've seen it in
codereview a few times like ~"newly introduced functions come first in each
section", but it comes up more often in non-private sections.

[stevenjb, 2014/01/29 00:11:31]

If it is I'm unaware. Frequently they are public and first. 

+                               size_t to_index,
+                               AppListItem* item) OVERRIDE;
+
+  // Returns the position of the last item in |item_list_| or an initial one.
+  syncer::StringOrdinal GetLastItemPosition();
+
+  // Adds |item| to |item_list_| and notifies observers.
+  void AddItemToItemList(AppListItem* item);
+
+  // Adds |item| to |folder|.
+  void AddItemToFolder(AppListFolderItem* folder, AppListItem* item);
+
   scoped_ptr<AppListItemList> item_list_;
   scoped_ptr<SearchBoxModel> search_box_;
   scoped_ptr<SearchResults> results_;
 
   Status status_;
   ObserverList<AppListModelObserver> observers_;
 
   DISALLOW_COPY_AND_ASSIGN(AppListModel);
 };
 
 }  // namespace app_list
 
 #endif  // UI_APP_LIST_APP_LIST_MODEL_H_