Separate the NTP app ordering from the app list app ordering

chrome/browser/ui/app_list/app_list_extension_ordering.cc

Index: chrome/browser/ui/app_list/app_list_extension_ordering.cc
diff --git a/chrome/browser/ui/app_list/app_list_extension_ordering.cc b/chrome/browser/ui/app_list/app_list_extension_ordering.cc
new file mode 100644
index 0000000000000000000000000000000000000000..e5d5e02d8a3f8a0033e0be103c1257c1e691bc40
--- /dev/null
+++ b/chrome/browser/ui/app_list/app_list_extension_ordering.cc
@@ -0,0 +1,233 @@
+// Copyright (c) 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 "chrome/browser/extensions/extension_prefs.h"
+#include "chrome/browser/extensions/extension_service.h"
+#include "chrome/browser/ui/app_list/app_list_extension_ordering.h"
+#include "chrome/common/chrome_notification_types.h"
+#include "content/public/browser/notification_service.h"
+
+namespace {
+  const char kPrefAppListOrdinal[] = "app_list_ordinal";
+}
+
+AppListExtensionOrdering::AppListExtensionOrdering(
+    ExtensionScopedPrefs* extension_scoped_prefs)
+    : extension_scoped_prefs_(extension_scoped_prefs),
+      extension_service_(NULL) {}
+
+AppListExtensionOrdering::~AppListExtensionOrdering() {}
+
+// ExtensionOrdering implementation.
+void AppListExtensionOrdering::Initialize(
+    const extensions::ExtensionIdList& extension_ids) {
+  for (extensions::ExtensionIdList::const_iterator ext_it =
+       extension_ids.begin(); ext_it != extension_ids.end(); ++ext_it) {
+    syncer::StringOrdinal ordinal = GetAppListOrdinal(*ext_it);
+    if (ordinal.IsValid()) {

[koz (OOO until 15th September), 2013/06/18 07:41:36]

nit: no curlies

[calamity, 2013/06/19 05:09:17]

Done.

+      SetAppListOrdinal(*ext_it, ordinal);

[koz (OOO until 15th September), 2013/06/18 07:41:36]

It's a bit weird that GetAppListOrdinal() reads from prefs but
SetAppListOrdinal() writes to a map. I suggest renaming GetAppListOrdinal() to
GetAppListOrdinalFromPrefs().

[calamity, 2013/06/19 05:09:17]

Done.

+    }
+
+    // Ensure that the web store app still isn't found in this list, since
+    // it is added after this loop.
+    DCHECK(*ext_it != extension_misc::kWebStoreAppId);
+    DCHECK(*ext_it != extension_misc::kChromeAppId);
+  }
+
+  // Include the Web Store App since it is displayed on the app list.
+  syncer::StringOrdinal web_store_app_ordinal =
+      GetAppListOrdinal(extension_misc::kWebStoreAppId);
+  if (web_store_app_ordinal.IsValid()) {

[koz (OOO until 15th September), 2013/06/18 07:41:36]

no curlies

[calamity, 2013/06/19 05:09:17]

Done.

+    SetAppListOrdinal(extension_misc::kWebStoreAppId, web_store_app_ordinal);
+  }
+  // Include the Chrome App since it is displayed on the app list.
+  syncer::StringOrdinal chrome_app_ordinal =
+      GetAppListOrdinal(extension_misc::kChromeAppId);
+  if (web_store_app_ordinal.IsValid()) {

[koz (OOO until 15th September), 2013/06/18 07:41:36]

no curlies

[calamity, 2013/06/19 05:09:17]

Done.

+    SetAppListOrdinal(extension_misc::kChromeAppId, chrome_app_ordinal);
+  }
+}
+
+void AppListExtensionOrdering::FixSyncCollisions() {
+  for (AppListOrdinalMap::iterator ordinal_map_it = app_list_ordinal_map_

[koz (OOO until 15th September), 2013/06/18 07:41:36]

nit: break this line after the =

[calamity, 2013/06/19 05:09:17]

Done.

+                                                    .begin();
+      ordinal_map_it != app_list_ordinal_map_.end(); ++ordinal_map_it) {
+    syncer::StringOrdinal repeated_ordinal = ordinal_map_it->first;
+    ExtensionIdSet& extension_set = ordinal_map_it->second;

[koz (OOO until 15th September), 2013/06/18 07:41:36]

I think this function would read better if it made use of a helper function,
something like:

AssignUniqueOrdinalsBetweenBounds(extension_set, lower_bound, upper_bound);

WDYT?

[calamity, 2013/06/19 05:09:17]

I'd rather keep all the logic in one place so anybody that reads this can do so
without having to scroll to the top of the page. Added some comments so that it
breaks down the logic blocks a bit more.

+    if (extension_set.size() == 1)
+      continue;
+    AppListOrdinalMap::iterator next_it = ordinal_map_it;
+    ++next_it;

[koz (OOO until 15th September), 2013/06/18 07:41:36]

next_it = ordinal_map_it + 1 ?

[calamity, 2013/06/19 05:09:17]

Nocompile =(

[tapted, 2013/06/19 10:47:58]

driveby trivia: C++11 has std::next(const iterator,
number_of_times_to_increment_it) -> iterator_copy

C++03 only has std::advance which modifies its argument, so not what you want
here.

+
+    syncer::StringOrdinal upper_bound_ordinal =
+        next_it == app_list_ordinal_map_.end() ? syncer::StringOrdinal()
+                                               : next_it->first;
+    syncer::StringOrdinal lower_bound_ordinal = repeated_ordinal;
+    std::vector<std::string> conflicting_ids(extension_set.begin(),
+                                             extension_set.end());
+    // Start at position 1 because the first extension can keep the conflicted
+    // value.
+    for (size_t i = 1; i < conflicting_ids.size(); ++i) {
+      syncer::StringOrdinal unique_ordinal;
+      if (upper_bound_ordinal.IsValid()) {
+        unique_ordinal =
+            lower_bound_ordinal.CreateBetween(upper_bound_ordinal);
+      } else {
+        unique_ordinal = lower_bound_ordinal.CreateAfter();
+      }
+      EraseAppListOrdinal(conflicting_ids[i]);
+      SetAppListOrdinal(conflicting_ids[i], unique_ordinal);
+      UpdatePrefs(conflicting_ids[i], unique_ordinal);
+      lower_bound_ordinal = unique_ordinal;
+    }
+  }
+
+  content::NotificationService::current()->Notify(
+      chrome::NOTIFICATION_APP_LIST_REORDERED,
+      content::Source<AppListExtensionOrdering>(this),
+      content::NotificationService::NoDetails());
+}
+
+void AppListExtensionOrdering::InsertAtFront(const std::string& extension_id) {
+  syncer::StringOrdinal ordinal = syncer::StringOrdinal::CreateInitialOrdinal();

[koz (OOO until 15th September), 2013/06/18 07:41:36]

Consider using the ternary operator here.

[calamity, 2013/06/19 05:09:17]

Done.

+  if (!app_list_ordinal_map_.empty()) {

[koz (OOO until 15th September), 2013/06/18 07:41:36]

nit: no curlies

[calamity, 2013/06/19 05:09:17]

N/A

+    ordinal = app_list_ordinal_map_.begin()->first.CreateBefore();
+  }
+  SetAppListOrdinal(extension_id, ordinal);
+  UpdatePrefs(extension_id, ordinal);
+}
+
+void AppListExtensionOrdering::InsertAtBack(const std::string& extension_id) {
+  syncer::StringOrdinal ordinal = syncer::StringOrdinal::CreateInitialOrdinal();

[koz (OOO until 15th September), 2013/06/18 07:41:36]

Consider using the ternary operator here.

[calamity, 2013/06/19 05:09:17]

Done.

+  if (!app_list_ordinal_map_.empty()) {
+    ordinal = app_list_ordinal_map_.rbegin()->first.CreateAfter();

[koz (OOO until 15th September), 2013/06/18 07:41:36]

nit: no curlies

[calamity, 2013/06/19 05:09:17]

N/A

+  }
+  SetAppListOrdinal(extension_id, ordinal);
+  UpdatePrefs(extension_id, ordinal);
+}
+
+void AppListExtensionOrdering::InsertAtNextAvailable(
+    const std::string& extension_id) {
+  InsertAtBack(extension_id);

[koz (OOO until 15th September), 2013/06/18 07:41:36]

Do callers actually distinguish between InsertAtNextAvailable() and
InsertAtBack()? I'd remove this function and replace it with calls to
InsertAtBack().

[calamity, 2013/06/19 05:09:17]

Ok. This was mostly so extension_sorting with its first not-full page placement
rules could fit underneath the extension_ordering interface but that's gone now.
Do you think this should be renamed app_list_extension_sorting for consistency
then?

[koz (OOO until 15th September), 2013/06/19 06:11:46]

Cool, that's a good idea.

+}
+
+void AppListExtensionOrdering::OnExtensionMoved(
+    const std::string& moved_extension_id,
+    const std::string& predecessor_extension_id,
+    const std::string& successor_extension_id) {
+  const syncer::StringOrdinal& predecessor_ordinal =
+      predecessor_extension_id.empty() ?
+          syncer::StringOrdinal() :
+          GetAppListOrdinal(predecessor_extension_id);
+  const syncer::StringOrdinal& successor_ordinal =
+      successor_extension_id.empty() ?
+          syncer::StringOrdinal() :
+          GetAppListOrdinal(successor_extension_id);
+  // We only need to change the StringOrdinal if there are neighbours.

[koz (OOO until 15th September), 2013/06/18 07:41:36]

nit: if there is at least one neighbour?

[calamity, 2013/06/19 05:09:17]

Done.

+  if (predecessor_ordinal.IsValid() || successor_ordinal.IsValid()) {
+    syncer::StringOrdinal ordinal;
+    if (!predecessor_ordinal.IsValid()) {
+      // Only a successor.
+      ordinal = successor_ordinal.CreateBefore();
+    } else if (!successor_ordinal.IsValid()) {
+      // Only a predecessor.
+      ordinal = predecessor_ordinal.CreateAfter();
+    } else {
+      // Both a successor and predecessor
+      ordinal = predecessor_ordinal.CreateBetween(successor_ordinal);
+    }
+    EraseAppListOrdinal(moved_extension_id);
+    SetAppListOrdinal(moved_extension_id, ordinal);
+    UpdatePrefs(moved_extension_id, ordinal);
+  }
+
+  content::NotificationService::current()->Notify(
+      chrome::NOTIFICATION_APP_LIST_REORDERED,
+      content::Source<AppListExtensionOrdering>(this),
+      content::Details<const std::string>(&moved_extension_id));
+}
+
+void AppListExtensionOrdering::Erase(const std::string& extension_id) {
+  EraseAppListOrdinal(extension_id);
+  UpdatePrefs(extension_id, syncer::StringOrdinal());
+}
+
+void AppListExtensionOrdering::EraseAppListOrdinal(
+    const std::string& extension_id) {
+  if (!Contains(extension_id))
+    return;
+  AppListOrdinalMap::iterator ordinal_map_it =
+      app_list_ordinal_map_.find(GetAppListOrdinal(extension_id));
+  if (ordinal_map_it == app_list_ordinal_map_.end())

[koz (OOO until 15th September), 2013/06/18 07:41:36]

DCHECK_NE() ?

[calamity, 2013/06/19 05:09:17]

DCHECK_NE tries to print the values which can't be done here.

+    NOTREACHED();
+  ordinal_map_it->second.erase(extension_id);
+  if (ordinal_map_it->second.empty())
+    app_list_ordinal_map_.erase(ordinal_map_it);
+}
+
+bool AppListExtensionOrdering::ExtensionPrecedes(
+    const std::string& extension1,
+    const std::string& extension2) {
+  if (!Contains(extension1))
+    return false;
+  if (!Contains(extension2))
+    return true;
+  return GetAppListOrdinal(extension1).LessThan(GetAppListOrdinal(extension2));
+}
+
+bool AppListExtensionOrdering::Contains(const std::string& extension) {
+  return GetAppListOrdinal(extension).IsValid();
+}
+
+syncer::StringOrdinal AppListExtensionOrdering::GetAppListOrdinal(
+    const std::string& extension_id) {
+  std::string raw_data;
+  // If the preference read fails then raw_data will still be unset and we will
+  // return an invalid StringOrdinal to signal that no app list ordinal was
+  // found.
+  extension_scoped_prefs_->ReadPrefAsString(
+      extension_id, kPrefAppListOrdinal, &raw_data);
+  return syncer::StringOrdinal(raw_data);
+}
+
+void AppListExtensionOrdering::SetAppListOrdinalForSync(
+  const std::string& extension_id,
+  const syncer::StringOrdinal& ordinal) {
+  DCHECK(!Contains(extension_id));

[koz (OOO until 15th September), 2013/06/18 07:41:36]

Can we just call Erase(extension_id) instead?

[calamity, 2013/06/19 05:09:17]

Done.

+  SetAppListOrdinal(extension_id, ordinal);
+  UpdatePrefs(extension_id, ordinal);
+}
+
+void AppListExtensionOrdering::SetAppListOrdinal(
+    const std::string& extension_id,
+    const syncer::StringOrdinal& ordinal) {
+  app_list_ordinal_map_[ordinal].insert(extension_id);
+}
+
+void AppListExtensionOrdering::UpdatePrefs(
+    const std::string& extension_id,
+    const syncer::StringOrdinal& ordinal) {
+  if (ordinal.EqualsOrBothInvalid(GetAppListOrdinal(extension_id)))
+    return;
+  Value* new_value = ordinal.IsValid() ?
+      Value::CreateStringValue(ordinal.ToInternalValue()) :
+      NULL;
+  extension_scoped_prefs_->UpdateExtensionPref(
+      extension_id,
+      kPrefAppListOrdinal,
+      new_value);
+
+  // Sync extension pref.
+  if (!extension_service_)
+    NOTREACHED();
+  const extensions::Extension* extension =
+      extension_service_->GetInstalledExtension(extension_id);
+  if (extension)
+    extension_service_->SyncExtensionChangeIfNeeded(*extension);
+}
+
+void AppListExtensionOrdering::SetExtensionService(
+    ExtensionServiceInterface* extension_service) {
+  extension_service_ = extension_service;
+}