Refactor ProfileInfoCache in c/b/ui/app_list

chrome/browser/profiles/profile_attributes_entry.h

Index: chrome/browser/profiles/profile_attributes_entry.h
diff --git a/chrome/browser/profiles/profile_attributes_entry.h b/chrome/browser/profiles/profile_attributes_entry.h
index 081c5c55557a49fa77cd061df1cae86cf7c506bf..d158f1179352427050691d3c7135f09f19b18bdc 100644
--- a/chrome/browser/profiles/profile_attributes_entry.h
+++ b/chrome/browser/profiles/profile_attributes_entry.h
@@ -11,8 +11,10 @@
 
 #include "base/files/file_path.h"
 #include "base/macros.h"
+#include "base/memory/scoped_ptr.h"
 #include "base/strings/string16.h"
 #include "base/time/time.h"
+#include "third_party/icu/source/i18n/unicode/coll.h"

[tapted, 2016/02/02 00:20:55]

this doesn't compile for me on mac: unicode/utypes.h' file not found - not sure
why

[lwchkg, 2016/02/03 17:18:38]

Oh. Can you start the try bot about Mac for me? It's easier to troubleshoot with
the log. (Anyway, missing include path? If yes we probably need to shout for
help in chromium-dev.)

Anthony: looks like I need to apply for try bot access.

[lwchkg, 2016/02/04 14:27:06]

Find the cause of the error. There's a new dependency in test_support_ui when I
include a file from icu.

 
 namespace gfx {
 class Image;
@@ -22,6 +24,20 @@ class ProfileInfoCache;
 
 class ProfileAttributesEntry {
  public:
+  // Compares two ProfileAttributesEntry using locale-sensitive comparison of
+  // their names. For ties, the profile path is compared next.
+  class SortComparator {
+   public:
+    explicit SortComparator(const icu::Collator* const collator)
+        : collator_(collator) {}
+    bool operator()(const ProfileAttributesEntry* const a,
+                    const ProfileAttributesEntry* const b) const;
+    // Returns a Collator which is used to construct SortComparator.
+    static scoped_ptr<icu::Collator> GetCollator();
+   private:
+    const icu::Collator* const collator_;

[tapted, 2016/01/31 23:04:37]

The ownership in this class is not right. You shouldn't instantiate a class with
scoped_ptr::get() -- it makes the API very easy to abuse.

I think the SortComparator() constructor shouldn't take an argument and
|collator_| should be scoped_ptr. (GetCollator() isn't needed).

If you need more flexibility in future, the SortComparator constructor should
take a scoped_ptr and use std::move or scoped_ptr::swap (previously we would use
scoped_ptr::Pass() but std::move is the new vogue).

[lwchkg, 2016/02/01 06:31:20]

Before coming to this version, actually I tried what you say and the compiler
failed to compile, due to copy constructing in std::sort with scoped_ptr. BTW,
I'll try again by enforcing Pred = SortComparator& instead of Pred =
SortComparator in std::sort.

[tapted, 2016/02/02 00:20:55]

I think you just need to add a move constructor to SortComparator, and std::sort
will accept it via std::move.

e.g. something like

class SortComparator {
 public:
  SortComparator()
  SortComparator(SortComparator&&) = default;
  bool operator()(...
...
 private:
  scoped_ptr<icu::Collator*> collator_;
  DISALLOW_COPY_AND_ASSIGN(..)
};


Failing that, I'd suggest a simplified version of SortVectorWithStringKey from
ui/base/l10n/l10n_util_collator.h so that instances of the comparator don't
escape the API. (perhaps in app_list_view_delegate.cc unless you think something
else might need it). Sadly, I don't think you can use SortVectorWithStringKey
directly since the vector contains pointers, not values.

You could also change SortVectorWithStringKey to replace the calls like
`lhs.GetStringKey();` to GetStringKey(lhs), then provide a template
specialization for ProfileAttributesEntry* that concatenates the name and path.

+  };
+
   ProfileAttributesEntry();
   virtual ~ProfileAttributesEntry() {}
 
@@ -39,7 +55,7 @@ class ProfileAttributesEntry {
   base::string16 GetUserName() const;
   // Gets the icon used as this profile's avatar. This might not be the icon
   // displayed in the UI if IsUsingGAIAPicture() is true.
-  const gfx::Image& GetAvatarIcon();
+  const gfx::Image& GetAvatarIcon() const;
   std::string GetLocalAuthCredentials() const;
   std::string GetPasswordChangeDetectionToken() const;
   // Note that a return value of false could mean an error in collection or