Improve the ProfileInfoCache API.

chrome/browser/profiles/profile_metadata_storage.h

+// Copyright (c) 2015 The Chromium Authors. All rights reserved.

[noms (inactive), 2015/06/29 17:10:40]

The 2014-and-after style says there's no (c) needed anymore:
https://www.chromium.org/developers/coding-style

[anthonyvd, 2015/06/30 21:24:52]

Done.

+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CHROME_BROWSER_PROFILES_PROFILE_METADATA_STORAGE_H_
+#define CHROME_BROWSER_PROFILES_PROFILE_METADATA_STORAGE_H_
+
+class ProfileInfoCache;
+
+class ProfileMetadataStorage {
+ public:
+  // Represents one profile's metadata.
+  class ProfileMetadataEntry {

[noms (inactive), 2015/06/29 17:10:40]

I don't love `metadata` (to me, metadata is like the headers of a table, but
this actually contains legit information, doesn't just describe it). How about:
ProfileAttributes or ProfileInfoEntry?

[Mike Lerman, 2015/06/29 19:14:37]

I was okay with ProfileMetadata, though I'm also okay with ProfileAttributes.

Also - does this have to be an internal/nested class? it's going to be
referenced a lot, is part of the public api, and will likely be modified
independently of the Storage. Perhaps put it into its own file?

[anthonyvd, 2015/06/30 21:24:53]

Done and done.

I preferred moving away from ProfileInfoSomething since a ProfileInfo is already
a thing. Also, good points about "metadata". Is everyone OK with Attributes like
this?

+   public:
+    base::string16 GetName() const;

[noms (inactive), 2015/06/29 17:10:40]

I'm so confused. This just has getters, but doesn't define the members? That's
kind of weird. :/

(I know they're in the profile info cache, but they probably should live here,
with their happy interface family. No? Maybe? Who knows, I write JavaScript now)

[anthonyvd, 2015/06/30 21:24:53]

I completely agree that it's kind of weird. The idea is to have it this way to
ease the transition away from the ProfileInfoCache's interface by implementing
this super thin layer that just speaks with the ProfileInfoCache.

Once everyone is moved to the new interface, we'll move the logic that currently
lives in Get*ForProfileAtIndex methods to this class. At that point its only
member will be a base::DictionaryValue* from which specific fields will be read.

This first very thin layer is just a good way to start refactoring the callers
before the actual implementation work is done, which I felt was more manageable.
It also avoids the clunky situation where some callers are migrated but not
others, requiring duplicate logic in both the ProfileInfoCache and this class.

Hopefully all of this makes sense :)

+    base::string16 GetShortcutName() const;
+    base::FilePath GetPath() const;
+    base::Time GetActiveTime() const;
+    base::string16 GetUserName() const;
+    const gfx::Image& GetAvatarIcon();
+    std::string GetLocalAuthCredentials() const;
+    std::string GetPasswordChangeDetectionToken() const;
+    // Note that a return value of false could mean an error in collection or
+    // that there are currently no background apps running. However, the action
+    // which results is the same in both cases (thus far).
+    bool GetBackgroundStatus() const;
+    base::string16 GetGAIAName() const;
+    base::string16 GetGAIAGivenName() const;
+    std::string GetGAIAId() const;
+    // Returns the GAIA picture for the given profile. This may return NULL
+    // if the profile does not have a GAIA picture or if the picture must be
+    // loaded from disk.
+    const gfx::Image* GetGAIAPicture() const;
+    bool IsUsingGAIAPicture() const;
+    bool IsSupervised() const;
+    bool IsChild() const;
+    bool IsLegacySupervised() const;
+    bool IsOmitted() const;
+    bool IsSigninRequired() const;
+    std::string GetSupervisedUserId() const;
+    bool IsEphemeral() const;
+    bool IsUsingDefaultName() const;
+    bool IsAuthenticated() const;
+    bool IsUsingDefaultAvatar() const;
+    bool IsAuthError() const;
+    size_t GetAvatarIconIndex() const;
+
+    void SetName(const base::string16& name);
+    void SetShortcutName(const base::string16& name);
+    void SetIsOmitted(bool is_omitted);
+    void SetSupervisedUserId(const std::string& id);
+    void SetLocalAuthCredentials(const std::string& auth);
+    void SetPasswordChangeDetectionToken(const std::string& token);
+    void SetBackgroundStatus(bool running_background_apps);
+    void SetGAIAName(const base::string16& name);
+    void SetGAIAGivenName(const base::string16& name);
+    void SetGAIAPicture(const gfx::Image* image);
+    void SetIsUsingGAIAPicture(bool value);
+    void SetIsSigninRequired(bool value);
+    void SetIsEphemeral(bool value);
+    void SetIsUsingDefaultName(bool value);
+    void SetIsUsingDefaultAvatar(bool value);
+    void SetIsAuthError(bool value);
+    void SetAvatarIconIndex(size_t icon_index);
+
+    void SetAuthInfo(const std::string& gaia_id,
+                     const base::string16& user_name);
+
+   private:
+    // These members are an implementation detail meant to smooth the migration

[noms (inactive), 2015/06/29 17:10:40]

Probably safe to add a TODO here :)

[anthonyvd, 2015/06/30 21:24:53]

Done.

+    // of the ProfileInfoCache to the ProfileMetadataStorage interface. They can
+    // be safely removed once the ProfileInfoCache stops using indices
+    // internally.
+    friend class ProfileInfoCache;
+    size_t profile_index_;
+    ProfileInfoCache* profile_info_cache_;

[Mike Lerman, 2015/06/29 19:14:37]

I imagine you will one day have member variables for all of those pieces of
information that are listed above? Perhaps write a TODO to that effect?

[anthonyvd, 2015/06/30 21:24:53]

Do you think the own above w.r.t. Monica's comment covers this as well?

+  };
+
+  ProfileMetadataStorage() {}
+  ~ProfileMetadataStorage() {}
+
+  // If the |supervised_user_id| is non-empty, the profile will be marked to be
+  // omitted from the avatar-menu list on desktop versions. This is used while a
+  // supervised user is in the process of being registered with the server. Use
+  // SetIsOmittedProfileAtIndex() to clear the flag when the profile is ready to

[anthonyvd, 2015/06/25 21:38:37]

Oops, need to update that comment.

[anthonyvd, 2015/06/30 21:24:53]

Done.

+  // be shown in the menu.
+  virtual void AddProfile(const base::FilePath& profile_path,
+                          const base::string16& name,
+                          const std::string& gaia_id,
+                          const base::string16& user_name,
+                          size_t icon_index,
+                          const std::string& supervised_user_id) = 0;
+  virtual void DeleteProfile(const base::FilePath& profile_path) = 0;
+
+  // Returns a vector containing one metadata entry per known profile. They are
+  // not sorted in any particular order.
+  virtual std::vector<ProfileMetadataEntry> GetAllProfilesMetadata() = 0;
+
+  // Populates |entry| with the data for the profile at |path| and returns true
+  // if the operation is successful and |entry| can be used. Returns false
+  // otherwise.
+  // |entry| should not be cached as it will not reflect subsequent changes to
+  // the profile's metadata.
+  virtual bool GetProfileMetadataWithPath(
+      const base::FilePath& path, ProfileMetadataEntry* entry) = 0;

[Mike Lerman, 2015/06/29 19:14:37]

Perhaps make the second parameter a *& rather than a *? Down the road you may
want to cache the ProfileMetadataEntry objects rather than re-creating them all
the time from prefs, and a *& will let you just return the cached object. For
now, you can just assert that the *& (or **) passed actually points to
something.

[anthonyvd, 2015/06/30 21:24:53]

I'm not too sure about that. The pointer-to-pointer version kind of seems like
the storage owns the object, which is not the case. The reference-to-pointer on
the other hand doesn't make it obvious enough at the call site that the
pointer's value can change (in my opinion).

Also, I think there are 2 ways the future of this class can go: either it stores
a (not owned) pointer to the underlying base::DictionaryValue or it extracts and
stores the data at construction time. The first case (which is what I at least
planned to start with) wouldn't prevent us from copying the cached object for
cheap. The second version would change the semantics of the interface to make it
so the storage owns the objects, which might be reasonable but I think the
change to a pointer-to-pointer should happen at that point (this way the current
interface doesn't have conflicting semantics).

Happy to discuss this more though, it's a great point! :)

[Mike Lerman, 2015/07/02 18:01:30]

I agree with your analysis of the two options, and I lean towards preferring the
second eventually since it means that there won't be out-of-date objects people
may accidentally hold on to and also minimizes memory requirements.

I see what you mean about not wanting to change form 1) to 2) without changing
the interface. What do you think about implementing 2) from the get-go, in this
CL?

[anthonyvd, 2015/07/07 21:05:32]

I went ahead and implemented 2) without the actual internal fields. I think this
is a good middle ground where we get the stable interface right away without
writing the code to keep both storages in sync that would be removed after the
migration to strictly using the new interface.

What do you think?

+
+  // Returns the amount of known profiles.

[Mike Lerman, 2015/06/29 19:14:37]

s/amount/count? quantity? numeracy?/

[anthonyvd, 2015/06/30 21:24:53]

Done.

+  virtual size_t GetNumberOfProfiles() const = 0;
+
+  DISALLOW_COPY_AND_ASSIGN(ProfileMetadataStorage);
+};
+
+#endif  // CHROME_BROWSER_PROFILES_PROFILE_METADATA_STORAGE_H_