Refactor ProfileInfoCache in c/b/ui/app_list

chrome/browser/profiles/profile_attributes_entry.h

 // Copyright 2015 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_PROFILES_PROFILE_ATTRIBUTES_ENTRY_H_
 #define CHROME_BROWSER_PROFILES_PROFILE_ATTRIBUTES_ENTRY_H_
 
 #include <stddef.h>
 
 #include <string>
 
 #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;
 }
 
 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() {}
 
   // Gets the name of the profile, which is the one displayed in the User Menu.
   base::string16 GetName() const;
 
   base::string16 GetShortcutName() const;
   // Gets the path to the profile. Should correspond to the path passed to
   // ProfileAttributesStorage::GetProfileAttributesWithPath to get this entry.
   base::FilePath GetPath() const;
   base::Time GetActiveTime() const;
   // Gets the user name of the signed in profile. This is typically the email
   // address used to sign in and the empty string for profiles that aren't
   // signed in to chrome.
   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
   // that there are currently no background apps running. However, the action
   // which results is the same in both cases (thus far).
   bool GetBackgroundStatus() const;
   // Gets the GAIA full name associated with this profile if it's signed in.
   base::string16 GetGAIAName() const;
   // Gets the GAIA given name associated with this profile if it's signed in.
   base::string16 GetGAIAGivenName() const;
@@ skipping 81 matching lines @@
   friend class ProfileInfoCache;
   void Initialize(ProfileInfoCache* cache, const base::FilePath& path);
   size_t profile_index() const;
   ProfileInfoCache* profile_info_cache_;
   base::FilePath profile_path_;
 
   DISALLOW_COPY_AND_ASSIGN(ProfileAttributesEntry);
 };
 
 #endif  // CHROME_BROWSER_PROFILES_PROFILE_ATTRIBUTES_ENTRY_H_