Adding a user list (to be consumed by GCM Client Implementation)

google_apis/gcm/engine/user_list.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 GOOGLE_APIS_GCM_ENGINE_USER_LIST_H_
+#define GOOGLE_APIS_GCM_ENGINE_USER_LIST_H_
+
+#include <string>
+
+#include "google_apis/gcm/engine/gcm_store.h"
+#include "google_apis/gcm/gcm_client.h"
+
+namespace gcm {
+
+class UserList {

[Nicolas Zea, 2014/01/13 19:26:06]

Comment what the class is for

[fgorski, 2014/01/14 22:25:42]

Done.

+ public:
+  UserList(GCMStore* gcm_store);

[Nicolas Zea, 2014/01/13 19:26:06]

nit: make this an explicit constructor

[fgorski, 2014/01/14 22:25:42]

Done.

+  virtual ~UserList();

[jianli, 2014/01/13 19:19:55]

Why virtual?

[fgorski, 2014/01/14 22:25:42]

Done.

+
+  // A callback invoked once the Backend is done loading the mappings.

[jianli, 2014/01/13 19:19:55]

The comment is very confusing.

[fgorski, 2014/01/14 22:25:42]

Done.

+  void Initialize(const GCMStore::LoadResult& result);

[Nicolas Zea, 2014/01/13 19:26:06]

Seems a bit strange that this consumes a GCMStore result, given that it contains
many things the user list doesn't care about. Perhaps have a custom structure
with the user information, and have the LoadResult contain an instance of that
structure?

[fgorski, 2014/01/14 22:25:42]

Done.

+
+  // Sets a user delegate for a |username|. It will create a new entry for the

[Nicolas Zea, 2014/01/13 19:26:06]

nit: for a |username| -> for |username|. Also, should this be called
SetDelegate? (presumably there can be only one delegate per username right?)

[fgorski, 2014/01/14 22:25:42]

Done.

+  // user if one does not exist.
+  void AddDelegate(const std::string& username, GCMClient::Delegate* delegate);
+
+  // Adds a |serial_number| to the user entry identified by the |username|.
+  // It expects an entry to already exist and not have a serial_number set.
+  // |callback| will be run once an entry is successfully persisted.
+  void AddSerialNumber(const std::string& username,

[Nicolas Zea, 2014/01/13 19:26:06]

Perhaps call this SetUserSerialNumber?

[fgorski, 2014/01/14 22:25:42]

I encapsulated the serial number related actions and scoped them to UserList
only, which means that one is no longer public.

+                       int64 serial_number,
+                       const GCMStore::UpdateCallback& callback);
+
+  // Returns a delegate for the user identified by |serial_number|.
+  GCMClient::Delegate* GetDelegateBySerialNumber(int64 serial_number) const;

[Nicolas Zea, 2014/01/13 19:26:06]

nit: mention what happens if the serial number isn't found (or the username
below)

[fgorski, 2014/01/14 22:25:42]

Done.

+
+  // Returns a delegate for the user identified by |username|.
+  GCMClient::Delegate* GetDelegateByUsername(const std::string& username) const;
+
+  // Returns a serial number of a user identified by |username|.
+  bool GetSerialNumberForUsername(const std::string& username,
+                                  int64* serial_number) const;
+
+  // Returns a next available serial number.
+  int64 GetNextSerialNumber();

[Nicolas Zea, 2014/01/13 19:26:06]

It still seems weird to me that the serial number incrementing logic is made
public like this, without being tightly coupled with adding a user. Could we not
have AddDelegate return the serial number for the user when it adds it?

[fgorski, 2014/01/14 22:25:42]

Done. Moved to private and renamed to IncrementSerialNumber (per an offline
suggestion from Jian Li)

+
+ private:
+  friend class UserListTest;
+
+  struct UserDelegate {
+    UserDelegate(const std::string& username, GCMClient::Delegate* delegate);
+    UserDelegate(const std::string& username, int64 serial_number);
+    ~UserDelegate();
+
+    std::string username;
+    int64 serial_number;
+    // Delegate related to the username. Not owned by the UserDelegate.
+    GCMClient::Delegate* delegate;
+  };
+  typedef std::vector<UserDelegate> DelegateList;

[jianli, 2014/01/13 19:19:55]

nit: please include vector.

In addition, I think it would be more efficient and simpler by switching to
using map, like:
  struct UserInfo {
    int64 serial_number;
    GCMClient::Delegate* delegate;
  };
  typedef std::map<std::string, UserInfo> UserInfoList;

By using map, you don't need to implement GetByUsername and
GetByUsernameForUpdate.

I think it would be better to name the struct as UserInfo since it contains the
data more than delegate.

[fgorski, 2014/01/14 22:25:42]

Done.

+
+  // Returns a const iterator pointing to the entry identified by |username|.
+  DelegateList::const_iterator GetByUsername(const std::string& username) const;
+
+  // Returns an iterator pointing to the entry identified by |username|.
+  DelegateList::iterator GetByUsernameForUpdate(const std::string& username);
+
+  // A callback invoked once the Backend is done updating the next serial
+  // number.
+  void NextSerialNumberUpdateCompleted(bool success);
+
+  // Sets the serial number related to the username. It expect the entry to not

[Nicolas Zea, 2014/01/13 19:26:06]

nit: expect -> expects

[fgorski, 2014/01/14 22:25:42]

Done.

+  // exist yet and will create it.
+  void SetSerialNumber(const std::string& username, int64 serial_number);
+
+  int64 next_serial_number_;
+  DelegateList delegates_;
+  GCMStore* gcm_store_;
+
+  DISALLOW_COPY_AND_ASSIGN(UserList);
+};
+
+}  // namespace gcm
+
+#endif  // GOOGLE_APIS_GCM_ENGINE_USER_LIST_H_