Mirror app list hierarchy data in profile prefs.

chrome/browser/ui/app_list/app_list_prefs.h

+// Copyright 2014 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 CHROME_BROWSER_UI_APP_LIST_APP_LIST_PREFS_H_
+#define CHROME_BROWSER_UI_APP_LIST_APP_LIST_PREFS_H_
+
+#include <set>
+#include <string>
+#include <vector>
+
+#include "base/basictypes.h"
+#include "base/observer_list.h"
+#include "base/values.h"
+#include "components/keyed_service/core/keyed_service.h"
+#include "sync/api/string_ordinal.h"
+
+class PrefService;
+
+namespace content {
+class BrowserContext;
+}
+
+namespace user_prefs {
+class PrefRegistrySyncable;
+}
+
+namespace app_list {
+
+class AppListPrefs : public KeyedService {
+ public:
+  static void RegisterProfilePrefs(user_prefs::PrefRegistrySyncable* registry);
+
+  static AppListPrefs* Get(content::BrowserContext* context);
+
+  static AppListPrefs* Create(PrefService* pref_service);
+
+  // An app list item's information. This is enough data to reconstruct the full
+  // hierarchy and ordering information of the app list.
+  struct AppListInfo {

[Matt Giuca, 2014/09/30 02:49:47]

This definition should appear before the methods.

[calamity, 2014/09/30 05:40:08]

Done.

+    enum ItemType {
+      ITEM_TYPE_INVALID = 0,
+      ITEM_TYPE_BEGIN,
+      APP_ITEM = ITEM_TYPE_BEGIN,
+      FOLDER_ITEM,
+
+      // Do not change the order of this enum.
+      // When adding values, remember to update ITEM_TYPE_END.
+      ITEM_TYPE_END = FOLDER_ITEM,

[stevenjb, 2014/09/26 16:22:16]

nit: I think it would be simpler to explicitly assign values to these enums, and
handle them explicitly rather than using any loops, especially since we don't
expect there to be very many.

[Matt Giuca, 2014/09/30 02:49:47]

I agree. You currently only use ITEM_TYPE_BEGIN/ITEM_TYPE_END to check validity.
You can do that explicitly and future-proof-ly using a switch statement. Make a
function "bool IsItemTypeValid(ItemType type)" with a switch statement that
returns false for ITEM_TYPE_INVALID and true for each other type, and then if
someone adds a new item type, the compiler will complain and they will need to
add a new item to the switch.

[calamity, 2014/09/30 05:40:08]

I'm using the begin and end values for validity checking.

Bleh. I tried to do this with an IsValidItemType(int) function but it loses the
future-proofness because a switch statement doesn't properly cover all cases of
the int. If I cast to an ItemType first, then the switch may encounter a value
that's invalid and the checks wouldn't really cover bad cases anyway.

[Matt Giuca, 2014/09/30 06:04:42]

Acknowledged.

+    };
+    explicit AppListInfo(const base::DictionaryValue* item_dict);

[stevenjb, 2014/09/26 16:22:16]

nit: Multiple constructors like this can be confusing, I think this would be
better as a static CreateFromDictionary method (or even just a file local method
if it is only used here).

[Matt Giuca, 2014/09/30 02:49:47]

+1

(You could also add another constructor that takes a folder_id, name, position
and item_type, if that would be useful. For a struct, I think it's fine to have
both a default constructor and a constructor that populates the fields directly;
if you want a cleverer constructor, make it a static factory method.)

[calamity, 2014/09/30 05:40:08]

Done.

+    AppListInfo();
+    ~AppListInfo();
+
+    // The id of the folder containing this item.
+    std::string folder_id;

[stevenjb, 2014/09/26 16:22:16]

All of the sync code uses 'parent_id', we should be consistent here.

[calamity, 2014/09/30 05:40:08]

Done.

+
+    // The name of this item.
+    std::string name;
+
+    // The position of this item in the app list.
+    syncer::StringOrdinal position;

[stevenjb, 2014/09/26 16:22:16]

We currently do not use the page_ordinal field from AppListSpecifics in the UI,
but we do sync it so that if/when we do start using it everything should just
work. We should track it here also.

[Matt Giuca, 2014/09/30 02:49:47]

We're explicitly trying to avoid having page_ordinal modelled here. (And we were
wondering what it was doing in AppListSpecifics.) Pages aren't part of the app
list model and never have been -- they are merely an artifact of the view.

[calamity, 2014/09/30 05:40:08]

The page ordinal field is completely unused at the moment. I have a CL to kill
that field as well which is at https://codereview.chromium.org/597873002/ .

It's used for the NTP page numbering but the app list doesn't have a concept of
model pages. If we ever want it, we can re-add it.

[stevenjb, 2014/09/30 18:16:23]

We should probably have an "App Launcher" meeting to decide the fate of pages.

Originally, UX wanted users to be able to organize apps by page, similar to how
iPhones behave. The page_id was added to support that.

It is easier to ignore a sync field than it is to add one, so we put it in there
initially, but because of design complications have not yet implemented it.

If we are sure we are never going to implement this than we can go ahead and
eliminate it from the AppListSyncableService code, but if we think we might
implement it, the overhead of having it is pretty minimal imho.

+
+    // The type of app list item being represented.
+    ItemType item_type;
+  };
+
+  virtual ~AppListPrefs();
+
+  // Sets the app list info for |id|.
+  void SetAppListInfo(const std::string& id, const AppListInfo& info);
+
+  // Gets the app list info for |id|.
+  scoped_ptr<AppListInfo> GetAppListInfo(const std::string& id) const;
+
+  // Gets a map of all AppListInfo objects in the prefs.
+  std::map<std::string, AppListInfo> GetAllAppListInfos() const;

[stevenjb, 2014/09/26 16:22:16]

This is passing a potentially very large map by value. Instead the method should
take a pointer to the map to fill in as input to avoid unnecessary copying.

[calamity, 2014/09/30 05:40:08]

Done.

+
+  // Deletes the app list info for |id|.
+  void DeleteAppListInfo(const std::string& id);
+
+ private:
+  explicit AppListPrefs(PrefService* pref_service);
+
+  PrefService* pref_service_;
+
+  DISALLOW_COPY_AND_ASSIGN(AppListPrefs);
+};
+
+}  // namespace app_list
+
+#endif  // CHROME_BROWSER_UI_APP_LIST_APP_LIST_PREFS_H_