Protect AppListItemList Add/Remove and fix sync bugs

ui/app_list/app_list_model.h

Index: ui/app_list/app_list_model.h
diff --git a/ui/app_list/app_list_model.h b/ui/app_list/app_list_model.h
index 76619ec80f2c9ee274b670f9ddd1369f7d3e3e68..f50ed949bbadf530738080db6647616091120064 100644
--- a/ui/app_list/app_list_model.h
+++ b/ui/app_list/app_list_model.h
@@ -10,10 +10,12 @@
 #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;
@@ -24,7 +26,11 @@ class SearchResult;
 // 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,
@@ -34,19 +40,52 @@ class APP_LIST_EXPORT AppListModel {
   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_;