Fix incorrect use of AppListItemList::FindItem

This CL should: * Fix a Folders bugs where existing items in folders were not found (causing them not to be updated and potentially creating duplicate items) * Protect the code against similar errors in the future * Correctly protect against an edge case where multiple items might have the same ordianl BUG=350165, 346117 For sync_app_list_helper.cc: R=jennyz@chromium.org, tapted@chromium.org TBR=zea@chromium.org Review URL: https://codereview.chromium.org/190953012 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@256603 0039d316-1c4b-4281-b951-d872f2087c98
cbchan · Mar 12, 2014 · 0939e16 · 0939e16
1 parent db4d28a
commit 0939e16
Show file tree

Hide file tree

Showing 26 changed files with 365 additions and 221 deletions.
diff --git a/chrome/browser/sync/test/integration/sync_app_list_helper.cc b/chrome/browser/sync/test/integration/sync_app_list_helper.cc
@@ -29,11 +29,9 @@ SyncAppListHelper* SyncAppListHelper::GetInstance() {
   return instance;
 }
 
-SyncAppListHelper::SyncAppListHelper() : test_(NULL), setup_completed_(false) {
-}
+SyncAppListHelper::SyncAppListHelper() : test_(NULL), setup_completed_(false) {}
 
-SyncAppListHelper::~SyncAppListHelper() {
-}
+SyncAppListHelper::~SyncAppListHelper() {}
 
 void SyncAppListHelper::SetupIfNecessary(SyncTest* test) {
   if (setup_completed_) {
@@ -43,11 +41,11 @@ void SyncAppListHelper::SetupIfNecessary(SyncTest* test) {
   test_ = test;
 
   for (int i = 0; i < test->num_clients(); ++i) {
-    extensions::ExtensionSystem::Get(
-        test_->GetProfile(i))->InitForRegularProfile(true);
+    extensions::ExtensionSystem::Get(test_->GetProfile(i))
+        ->InitForRegularProfile(true);
   }
-  extensions::ExtensionSystem::Get(
-      test_->verifier())->InitForRegularProfile(true);
+  extensions::ExtensionSystem::Get(test_->verifier())
+      ->InitForRegularProfile(true);
 
   setup_completed_ = true;
 }
@@ -59,32 +57,35 @@ bool SyncAppListHelper::AppListMatchesVerifier(Profile* profile) {
       AppListSyncableServiceFactory::GetForProfile(test_->verifier());
   // Note: sync item entries may not exist in verifier, but item lists should
   // match.
-  if (service->model()->item_list()->item_count() !=
-      verifier->model()->item_list()->item_count()) {
+  if (service->model()->top_level_item_list()->item_count() !=
+      verifier->model()->top_level_item_list()->item_count()) {
     LOG(ERROR) << "Model item count: "
-               << service->model()->item_list()->item_count()
-               << " != " << verifier->model()->item_list()->item_count();
+               << service->model()->top_level_item_list()->item_count()
+               << " != "
+               << verifier->model()->top_level_item_list()->item_count();
     return false;
   }
   bool res = true;
-  for (size_t i = 0; i < service->model()->item_list()->item_count(); ++i) {
-    AppListItem* item1 = service->model()->item_list()->item_at(i);
-    AppListItem* item2 = verifier->model()->item_list()->item_at(i);
+  for (size_t i = 0; i < service->model()->top_level_item_list()->item_count();
+       ++i) {
+    AppListItem* item1 = service->model()->top_level_item_list()->item_at(i);
+    AppListItem* item2 = verifier->model()->top_level_item_list()->item_at(i);
     if (item1->CompareForTest(item2))
       continue;
 
     LOG(ERROR) << "Item(" << i << "): " << item1->ToDebugString()
                << " != " << item2->ToDebugString();
     size_t index2;
-    if (!verifier->model()->item_list()->FindItemIndex(item1->id(), &index2)) {
+    if (!verifier->model()->top_level_item_list()->FindItemIndex(item1->id(),
+                                                                 &index2)) {
       LOG(ERROR) << " Item(" << i << "): " << item1->ToDebugString()
                  << " Not in verifier.";
     } else {
       LOG(ERROR) << " Item(" << i << "): " << item1->ToDebugString()
                  << " Has different verifier index: " << index2;
-      item2 = verifier->model()->item_list()->item_at(index2);
-      LOG(ERROR) << " Verifier Item(" << index2 << "): "
-                 << item2->ToDebugString();
+      item2 = verifier->model()->top_level_item_list()->item_at(index2);
+      LOG(ERROR) << " Verifier Item(" << index2
+                 << "): " << item2->ToDebugString();
     }
     res = false;
   }
@@ -118,7 +119,7 @@ bool SyncAppListHelper::AllProfilesHaveSameAppListAsVerifier() {
 void SyncAppListHelper::MoveApp(Profile* profile, size_t from, size_t to) {
   AppListSyncableService* service =
       AppListSyncableServiceFactory::GetForProfile(profile);
-  service->model()->item_list()->MoveItem(from, to);
+  service->model()->top_level_item_list()->MoveItem(from, to);
 }
 
 void SyncAppListHelper::MoveAppToFolder(Profile* profile,
@@ -127,7 +128,7 @@ void SyncAppListHelper::MoveAppToFolder(Profile* profile,
   AppListSyncableService* service =
       AppListSyncableServiceFactory::GetForProfile(profile);
   service->model()->MoveItemToFolder(
-      service->model()->item_list()->item_at(index), folder_id);
+      service->model()->top_level_item_list()->item_at(index), folder_id);
 }
 
 void SyncAppListHelper::MoveAppFromFolder(Profile* profile,
@@ -150,18 +151,18 @@ void SyncAppListHelper::CopyOrdinalsToVerifier(Profile* profile,
       AppListSyncableServiceFactory::GetForProfile(profile);
   AppListSyncableService* verifier =
       AppListSyncableServiceFactory::GetForProfile(test_->verifier());
-  verifier->model()->item_list()->SetItemPosition(
-      verifier->model()->item_list()->FindItem(id),
-      service->model()->item_list()->FindItem(id)->position());
+  verifier->model()->top_level_item_list()->SetItemPosition(
+      verifier->model()->FindItem(id),
+      service->model()->FindItem(id)->position());
 }
 
 void SyncAppListHelper::PrintAppList(Profile* profile) {
   AppListSyncableService* service =
       AppListSyncableServiceFactory::GetForProfile(profile);
-  for (size_t i = 0; i < service->model()->item_list()->item_count(); ++i) {
-    AppListItem* item = service->model()->item_list()->item_at(i);
-    std::string label =
-        base::StringPrintf("Item(%d): ", static_cast<int>(i));
+  for (size_t i = 0; i < service->model()->top_level_item_list()->item_count();
+       ++i) {
+    AppListItem* item = service->model()->top_level_item_list()->item_at(i);
+    std::string label = base::StringPrintf("Item(%d): ", static_cast<int>(i));
     PrintItem(profile, item, label);
   }
 }
@@ -183,8 +184,8 @@ void SyncAppListHelper::PrintItem(Profile* profile,
     }
     return;
   }
-  VLOG(1)
-      << label << item->ToDebugString()
-      << " Page: " << s->GetPageOrdinal(id).ToDebugString().substr(0, 8)
-      << " Item: " << s->GetAppLaunchOrdinal(id).ToDebugString().substr(0, 8);
+  VLOG(1) << label << item->ToDebugString()
+          << " Page: " << s->GetPageOrdinal(id).ToDebugString().substr(0, 8)
+          << " Item: "
+          << s->GetAppLaunchOrdinal(id).ToDebugString().substr(0, 8);
 }
diff --git a/chrome/browser/ui/app_list/extension_app_item.cc b/chrome/browser/ui/app_list/extension_app_item.cc
@@ -203,11 +203,6 @@ void ExtensionAppItem::UpdateIcon() {
   SetIcon(icon, true);
 }
 
-void ExtensionAppItem::UpdateIconOverlay() {
-  if (has_overlay_ != NeedsOverlay())
-    UpdateIcon();
-}
-
 void ExtensionAppItem::Move(const ExtensionAppItem* prev,
                             const ExtensionAppItem* next) {
   if (!prev && !next)
@@ -335,6 +330,11 @@ ui::MenuModel* ExtensionAppItem::GetContextMenuModel() {
   return context_menu_->GetMenuModel();
 }
 
+void ExtensionAppItem::OnExtensionPreferenceChanged() {
+  if (has_overlay_ != NeedsOverlay())
+    UpdateIcon();
+}
+
 // static
 const char ExtensionAppItem::kItemType[] = "ExtensionAppItem";
 

diff --git a/chrome/browser/ui/app_list/extension_app_item.h b/chrome/browser/ui/app_list/extension_app_item.h
@@ -51,9 +51,6 @@ class ExtensionAppItem : public app_list::AppListItem,
   // it gray.
   void UpdateIcon();
 
-  // Only updates the icon if the overlay needs to be added/removed.
-  void UpdateIconOverlay();
-
   // Update page and app launcher ordinals to put the app in between |prev| and
   // |next|. Note that |prev| and |next| could be NULL when the app is put at
   // the beginning or at the end.
@@ -92,6 +89,8 @@ class ExtensionAppItem : public app_list::AppListItem,
   // Overridden from AppListItem:
   virtual void Activate(int event_flags) OVERRIDE;
   virtual ui::MenuModel* GetContextMenuModel() OVERRIDE;
+  // Updates the icon if the overlay needs to be added/removed.
+  virtual void OnExtensionPreferenceChanged() OVERRIDE;
   virtual const char* GetItemType() const OVERRIDE;
 
   // Overridden from app_list::AppContextMenuDelegate:

diff --git a/chrome/browser/ui/app_list/extension_app_model_builder.cc b/chrome/browser/ui/app_list/extension_app_model_builder.cc
@@ -59,14 +59,14 @@ ExtensionAppModelBuilder::ExtensionAppModelBuilder(
 
 ExtensionAppModelBuilder::~ExtensionAppModelBuilder() {
   OnShutdown();
-  model_->item_list()->RemoveObserver(this);
+  if (!service_)
+    model_->top_level_item_list()->RemoveObserver(this);
 }
 
 void ExtensionAppModelBuilder::InitializeWithService(
     app_list::AppListSyncableService* service) {
   DCHECK(!service_ && !profile_);
   model_ = service->model();
-  model_->item_list()->AddObserver(this);
   service_ = service;
   profile_ = service->profile();
   InitializePrefChangeRegistrar();
@@ -79,7 +79,7 @@ void ExtensionAppModelBuilder::InitializeWithProfile(
     app_list::AppListModel* model) {
   DCHECK(!service_ && !profile_);
   model_ = model;
-  model_->item_list()->AddObserver(this);
+  model_->top_level_item_list()->AddObserver(this);
   profile_ = profile;
   InitializePrefChangeRegistrar();
 
@@ -91,6 +91,8 @@ void ExtensionAppModelBuilder::InitializePrefChangeRegistrar() {
           switches::kEnableStreamlinedHostedApps))
     return;
 
+  // TODO(calamity): analyze the performance impact of doing this every
+  // extension pref change.
   extensions::ExtensionsBrowserClient* client =
       extensions::ExtensionsBrowserClient::Get();
   extension_pref_change_registrar_.Init(
@@ -102,16 +104,7 @@ void ExtensionAppModelBuilder::InitializePrefChangeRegistrar() {
 }
 
 void ExtensionAppModelBuilder::OnExtensionPreferenceChanged() {
-  // TODO(calamity): analyze the performance impact of doing this every
-  // extension pref change.
-  app_list::AppListItemList* item_list = model_->item_list();
-  for (size_t i = 0; i < item_list->item_count(); ++i) {
-    app_list::AppListItem* item = item_list->item_at(i);
-    if (item->GetItemType() != ExtensionAppItem::kItemType)
-      continue;
-
-    static_cast<ExtensionAppItem*>(item)->UpdateIconOverlay();
-  }
+  model_->NotifyExtensionPreferenceChanged();
 }
 
 void ExtensionAppModelBuilder::OnBeginExtensionInstall(
@@ -262,8 +255,7 @@ void ExtensionAppModelBuilder::SetHighlightedApp(
 
 ExtensionAppItem* ExtensionAppModelBuilder::GetExtensionAppItem(
     const std::string& extension_id) {
-  app_list::AppListItem* item =
-      model_->item_list()->FindItem(extension_id);
+  app_list::AppListItem* item = model_->FindItem(extension_id);
   LOG_IF(ERROR, item &&
          item->GetItemType() != ExtensionAppItem::kItemType)
       << "App Item matching id: " << extension_id
@@ -285,15 +277,14 @@ void ExtensionAppModelBuilder::UpdateHighlight() {
 void ExtensionAppModelBuilder::OnListItemMoved(size_t from_index,
                                                size_t to_index,
                                                app_list::AppListItem* item) {
+  DCHECK(!service_);
+
   // This will get called from AppListItemList::ListItemMoved after
   // set_position is called for the item.
-  app_list::AppListItemList* item_list = model_->item_list();
   if (item->GetItemType() != ExtensionAppItem::kItemType)
     return;
 
-  if (service_)
-    return;
-
+  app_list::AppListItemList* item_list = model_->top_level_item_list();
   ExtensionAppItem* prev = NULL;
   for (size_t idx = to_index; idx > 0; --idx) {
     app_list::AppListItem* item = item_list->item_at(idx - 1);

diff --git a/chrome/browser/ui/app_list/extension_app_model_builder_unittest.cc b/chrome/browser/ui/app_list/extension_app_model_builder_unittest.cc
@@ -35,10 +35,10 @@ const char kPackagedApp2Id[] = "jlklkagmeajbjiobondfhiekepofmljl";
 // Get a string of all apps in |model| joined with ','.
 std::string GetModelContent(app_list::AppListModel* model) {
   std::string content;
-  for (size_t i = 0; i < model->item_list()->item_count(); ++i) {
+  for (size_t i = 0; i < model->top_level_item_list()->item_count(); ++i) {
     if (i > 0)
       content += ',';
-    content += model->item_list()->item_at(i)->name();
+    content += model->top_level_item_list()->item_at(i)->name();
   }
   return content;
 }
@@ -163,7 +163,7 @@ class ExtensionAppModelBuilderTest : public ExtensionServiceTestBase {
 
 TEST_F(ExtensionAppModelBuilderTest, Build) {
   // The apps list would have 3 extension apps in the profile.
-  EXPECT_EQ(kDefaultAppCount, model_->item_list()->item_count());
+  EXPECT_EQ(kDefaultAppCount, model_->top_level_item_list()->item_count());
   EXPECT_EQ(std::string(kDefaultApps), GetModelContent(model_.get()));
 }
 
@@ -359,6 +359,6 @@ TEST_F(ExtensionAppModelBuilderTest, BookmarkApp) {
   EXPECT_TRUE(err.empty());
 
   service_->AddExtension(bookmark_app.get());
-  EXPECT_EQ(kDefaultAppCount + 1, model_->item_list()->item_count());
+  EXPECT_EQ(kDefaultAppCount + 1, model_->top_level_item_list()->item_count());
   EXPECT_NE(std::string::npos, GetModelContent(model_.get()).find(kAppName));
 }
diff --git a/chrome/browser/ui/app_list/fast_show_pickler.cc b/chrome/browser/ui/app_list/fast_show_pickler.cc
@@ -201,19 +201,21 @@ scoped_ptr<Pickle> FastShowPickler::PickleAppListModelForFastShow(
   scoped_ptr<Pickle> result(new Pickle);
   if (!result->WriteInt(kVersion))
     return scoped_ptr<Pickle>();
-  if (!result->WriteInt((int) model->item_list()->item_count()))
+  if (!result->WriteInt((int)model->top_level_item_list()->item_count()))
     return scoped_ptr<Pickle>();
-  for (size_t i = 0; i < model->item_list()->item_count(); ++i) {
-    if (!PickleAppListItem(result.get(), model->item_list()->item_at(i)))
+  for (size_t i = 0; i < model->top_level_item_list()->item_count(); ++i) {
+    if (!PickleAppListItem(result.get(),
+                           model->top_level_item_list()->item_at(i))) {
       return scoped_ptr<Pickle>();
+    }
   }
   return result.Pass();
 }
 
 void FastShowPickler::CopyOver(AppListModel* src, AppListModel* dest) {
-  DCHECK_EQ(0u, dest->item_list()->item_count());
-  for (size_t i = 0; i < src->item_list()->item_count(); i++) {
-    AppListItem* src_item = src->item_list()->item_at(i);
+  DCHECK_EQ(0u, dest->top_level_item_list()->item_count());
+  for (size_t i = 0; i < src->top_level_item_list()->item_count(); i++) {
+    AppListItem* src_item = src->top_level_item_list()->item_at(i);
     scoped_ptr<AppListItem> dest_item(new AppListItem(src_item->id()));
     CopyOverItem(src_item, dest_item.get());
     dest->AddItemToFolder(dest_item.Pass(), src_item->folder_id());

diff --git a/chrome/browser/ui/app_list/test/fast_show_pickler_unittest.cc b/chrome/browser/ui/app_list/test/fast_show_pickler_unittest.cc
@@ -18,16 +18,17 @@ using app_list::AppListModel;
 class AppListModelPicklerUnitTest : public testing::Test {
  protected:
   void CheckIsSame(AppListModel* m1, AppListModel* m2) {
-    ASSERT_EQ(m1->item_list()->item_count(), m2->item_list()->item_count());
-    for (size_t i = 0; i < m1->item_list()->item_count(); i++) {
-      ASSERT_EQ(m1->item_list()->item_at(i)->id(),
-                m2->item_list()->item_at(i)->id());
-      ASSERT_EQ(m1->item_list()->item_at(i)->name(),
-                m2->item_list()->item_at(i)->name());
-      ASSERT_EQ(m1->item_list()->item_at(i)->short_name(),
-                m2->item_list()->item_at(i)->short_name());
-      CompareImages(m1->item_list()->item_at(i)->icon(),
-                    m2->item_list()->item_at(i)->icon());
+    ASSERT_EQ(m1->top_level_item_list()->item_count(),
+              m2->top_level_item_list()->item_count());
+    for (size_t i = 0; i < m1->top_level_item_list()->item_count(); i++) {
+      ASSERT_EQ(m1->top_level_item_list()->item_at(i)->id(),
+                m2->top_level_item_list()->item_at(i)->id());
+      ASSERT_EQ(m1->top_level_item_list()->item_at(i)->name(),
+                m2->top_level_item_list()->item_at(i)->name());
+      ASSERT_EQ(m1->top_level_item_list()->item_at(i)->short_name(),
+                m2->top_level_item_list()->item_at(i)->short_name());
+      CompareImages(m1->top_level_item_list()->item_at(i)->icon(),
+                    m2->top_level_item_list()->item_at(i)->icon());
     }
   }
 

diff --git a/ui/app_list/app_list_folder_item.cc b/ui/app_list/app_list_folder_item.cc
@@ -178,6 +178,11 @@ size_t AppListFolderItem::ChildItemCount() const {
   return item_list_->item_count();
 }
 
+void AppListFolderItem::OnExtensionPreferenceChanged() {
+  for (size_t i = 0; i < item_list_->item_count(); ++i)
+    item_list_->item_at(i)->OnExtensionPreferenceChanged();
+}
+
 bool AppListFolderItem::CompareForTest(const AppListItem* other) const {
   if (!AppListItem::CompareForTest(other))
     return false;

diff --git a/ui/app_list/app_list_folder_item.h b/ui/app_list/app_list_folder_item.h
@@ -54,6 +54,7 @@ class APP_LIST_EXPORT AppListFolderItem : public AppListItem,
   virtual ui::MenuModel* GetContextMenuModel() OVERRIDE;
   virtual AppListItem* FindChildItem(const std::string& id) OVERRIDE;
   virtual size_t ChildItemCount() const OVERRIDE;
+  virtual void OnExtensionPreferenceChanged() OVERRIDE;
   virtual bool CompareForTest(const AppListItem* other) const OVERRIDE;
 
   // Calculates the top item icons' bounds inside |folder_icon_bounds|.

diff --git a/ui/app_list/app_list_item.cc b/ui/app_list/app_list_item.cc
@@ -84,6 +84,8 @@ size_t AppListItem::ChildItemCount() const {
   return 0;
 }
 
+void AppListItem::OnExtensionPreferenceChanged() {}
+
 bool AppListItem::CompareForTest(const AppListItem* other) const {
   return id_ == other->id_ &&
       folder_id_ == other->folder_id_ &&

diff --git a/ui/app_list/app_list_item.h b/ui/app_list/app_list_item.h
@@ -82,6 +82,10 @@ class APP_LIST_EXPORT AppListItem {
   // Returns the number of child items if it has any (e.g. is a folder) or 0.
   virtual size_t ChildItemCount() const;
 
+  // Called when the extension preference changed. Used by ExtensionAppItem
+  // to update icon overlays.
+  virtual void OnExtensionPreferenceChanged();
+
   // Utility functions for sync integration tests.
   virtual bool CompareForTest(const AppListItem* other) const;
   virtual std::string ToDebugString() const;