Enables sorting of the tables in the password editor and the URL

picker dialog.

BUG=2949
TEST=Try sorting in the password manager dialog table as well as when
  adding a URL to the list of URLs to open on startup.

Review URL: http://codereview.chromium.org/6053

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@3010 0039d316-1c4b-4281-b951-d872f2087c98

5 files changed, 150 insertions, 77 deletions

diff --git a/chrome/browser/views/password_manager_view.cc b/chrome/browser/views/password_manager_view.cc
index 678b036..926ce17 100644
--- a/chrome/browser/views/password_manager_view.cc
+++ b/chrome/browser/views/password_manager_view.cc
@@ -7,6 +7,8 @@
 #include "chrome/browser/profile.h"
 #include "chrome/browser/views/password_manager_view.h"
 #include "chrome/browser/views/standard_layout.h"
+#include "chrome/common/pref_names.h"
+#include "chrome/common/pref_service.h"
 #include "chrome/views/background.h"
 #include "chrome/views/grid_layout.h"
 #include "chrome/views/native_button.h"
@@ -52,14 +54,18 @@ void MultiLabelButtons::GetPreferredSize(CSize *out) {
 }
 
 ////////////////////////////////////////////////////////////////////
+// PasswordManagerTableModel::PasswordRow
+PasswordManagerTableModel::PasswordRow::~PasswordRow() {
+  delete form;
+}
+
+////////////////////////////////////////////////////////////////////
 // PasswordManagerTableModel
-PasswordManagerTableModel::PasswordManagerTableModel(
-    WebDataService* profile_web_data_service)
-    : saved_signons_deleter_(&saved_signons_),
-      observer_(NULL),
+PasswordManagerTableModel::PasswordManagerTableModel(Profile* profile)
+    : observer_(NULL),
       pending_login_query_(NULL),
-      web_data_service_(profile_web_data_service) {
-  DCHECK(web_data_service_);
+      profile_(profile) {
+  DCHECK(profile && profile->GetWebDataService(Profile::EXPLICIT_ACCESS));
 }
 
 PasswordManagerTableModel::~PasswordManagerTableModel() {
@@ -72,19 +78,26 @@ int PasswordManagerTableModel::RowCount() {
 
 std::wstring PasswordManagerTableModel::GetText(int row,
                                                 int col_id) {
-  PasswordForm* signon = GetPasswordFormAt(row);
-  DCHECK(signon);
   switch (col_id) {
     case IDS_PASSWORD_MANAGER_VIEW_SITE_COLUMN:  // Site.
-      return UTF8ToWide(signon->origin.spec());
+      return saved_signons_[row].display_url.display_url();
     case IDS_PASSWORD_MANAGER_VIEW_USERNAME_COLUMN:  // Username.
-      return signon->username_value;
+      return GetPasswordFormAt(row)->username_value;
     default:
       NOTREACHED() << "Invalid column.";
       return std::wstring();
   }
 }
 
+int PasswordManagerTableModel::CompareValues(int row1, int row2,
+                                             int column_id) {
+  if (column_id == IDS_PASSWORD_MANAGER_VIEW_SITE_COLUMN) {
+    return saved_signons_[row1].display_url.Compare(
+        saved_signons_[row2].display_url, GetCollator());
+  }
+  return TableModel::CompareValues(row1, row2, column_id);
+}
+
 void PasswordManagerTableModel::SetObserver(
     ChromeViews::TableModelObserver* observer) {
   observer_ = observer;
@@ -92,7 +105,7 @@ void PasswordManagerTableModel::SetObserver(
 
 void PasswordManagerTableModel::GetAllSavedLoginsForProfile() {
   DCHECK(!pending_login_query_);
-  pending_login_query_ = web_data_service_->GetAllAutofillableLogins(this);
+  pending_login_query_ = web_data_service()->GetAllAutofillableLogins(this);
 }
 
 void PasswordManagerTableModel::OnWebDataServiceRequestDone(
@@ -109,41 +122,46 @@ void PasswordManagerTableModel::OnWebDataServiceRequestDone(
   // Get the result from the database into a useable form.
   const WDResult<std::vector<PasswordForm*> >* r =
       static_cast<const WDResult<std::vector<PasswordForm*> >*>(result);
-  // Copy vector of *pointers* to saved_logins_, thus taking ownership of the
-  // PasswordForm's in the result.
-  saved_signons_ = r->GetValue();
+  std::vector<PasswordForm*> rows = r->GetValue();
+  saved_signons_.clear();
+  saved_signons_.resize(rows.size());
+  std::wstring languages =
+      profile_->GetPrefs()->GetString(prefs::kAcceptLanguages);
+  for (size_t i = 0; i < rows.size(); ++i) {
+    saved_signons_[i].form = rows[i];
+    saved_signons_[i].display_url =
+        gfx::SortedDisplayURL(rows[i]->origin, languages);
+  }
   if (observer_)
     observer_->OnModelChanged();
 }
 
 void PasswordManagerTableModel::CancelLoginsQuery() {
   if (pending_login_query_) {
-    web_data_service_->CancelRequest(pending_login_query_);
+    web_data_service()->CancelRequest(pending_login_query_);
     pending_login_query_ = NULL;
   }
 }
 
 PasswordForm* PasswordManagerTableModel::GetPasswordFormAt(int row) {
   DCHECK(row >= 0 && row < RowCount());
-  return saved_signons_[row];
+  return saved_signons_[row].form;
 }
 
 void PasswordManagerTableModel::ForgetAndRemoveSignon(int row) {
   DCHECK(row >= 0 && row < RowCount());
-  PasswordForms::iterator target_iter = saved_signons_.begin() + row;
+  PasswordRows::iterator target_iter = saved_signons_.begin() + row;
   // Remove from DB, memory, and vector.
-  web_data_service_->RemoveLogin(**target_iter);
-  delete *target_iter;
+  web_data_service()->RemoveLogin(*(target_iter->form));
   saved_signons_.erase(target_iter);
   if (observer_)
     observer_->OnItemsRemoved(row, 1);
 }
 
 void PasswordManagerTableModel::ForgetAndRemoveAllSignons() {
-  PasswordForms::iterator iter = saved_signons_.begin();
+  PasswordRows::iterator iter = saved_signons_.begin();
   while (iter != saved_signons_.end()) {
-    web_data_service_->RemoveLogin(**iter);
-    delete *iter;
+    web_data_service()->RemoveLogin(*(iter->form));
     iter = saved_signons_.erase(iter);
   }
   if (observer_)
@@ -177,7 +195,7 @@ PasswordManagerView::PasswordManagerView(Profile* profile)
           IDS_PASSWORD_MANAGER_VIEW_REMOVE_BUTTON)),
       remove_all_button_(l10n_util::GetString(
           IDS_PASSWORD_MANAGER_VIEW_REMOVE_ALL_BUTTON)),
-      table_model_(profile->GetWebDataService(Profile::EXPLICIT_ACCESS)) {
+      table_model_(profile) {
   Init();
 }
 
@@ -189,14 +207,22 @@ void PasswordManagerView::SetupTable() {
       ChromeViews::TableColumn(
           IDS_PASSWORD_MANAGER_VIEW_SITE_COLUMN,
           ChromeViews::TableColumn::LEFT, -1, 0.55f));
+  columns.back().sortable = true;
   columns.push_back(
       ChromeViews::TableColumn(
           IDS_PASSWORD_MANAGER_VIEW_USERNAME_COLUMN,
           ChromeViews::TableColumn::RIGHT, -1, 0.37f));
+  columns.back().sortable = true;
   table_view_ = new ChromeViews::TableView(&table_model_,
                                            columns,
                                            ChromeViews::TEXT_ONLY,
                                            true, true, true);
+  // Make the table initially sorted by host.
+  ChromeViews::TableView::SortDescriptors sort;
+  sort.push_back(
+      ChromeViews::TableView::SortDescriptor(
+          IDS_PASSWORD_MANAGER_VIEW_SITE_COLUMN, true));
+  table_view_->SetSortDescriptors(sort);
   table_view_->SetObserver(this);
 }
 
@@ -373,4 +399,3 @@ void PasswordManagerView::WindowClosing() {
 ChromeViews::View* PasswordManagerView::GetContentsView() {
   return this;
 }
-
diff --git a/chrome/browser/views/password_manager_view.h b/chrome/browser/views/password_manager_view.h
index f470bee..bbcba7d 100644
--- a/chrome/browser/views/password_manager_view.h
+++ b/chrome/browser/views/password_manager_view.h
@@ -8,6 +8,7 @@
 #include <vector>
 
 #include "chrome/browser/webdata/web_data_service.h"
+#include "chrome/common/gfx/url_elider.h"
 #include "chrome/views/dialog_delegate.h"
 #include "chrome/views/label.h"
 #include "chrome/views/native_button.h"
@@ -20,12 +21,13 @@ struct PasswordForm;
 class PasswordManagerTableModel : public ChromeViews::TableModel,
                                   public WebDataServiceConsumer {
  public:
-  explicit PasswordManagerTableModel(WebDataService* profile_web_data_service);
+  explicit PasswordManagerTableModel(Profile* profile);
   virtual ~PasswordManagerTableModel();
 
   // TableModel methods.
   virtual int RowCount();
   virtual std::wstring GetText(int row, int column);
+  virtual int CompareValues(int row1, int row2, int column_id);
   virtual void SetObserver(ChromeViews::TableModelObserver* observer);
 
   // Delete the PasswordForm at specified row from the database (and remove
@@ -46,24 +48,37 @@ class PasswordManagerTableModel : public ChromeViews::TableModel,
   PasswordForm* GetPasswordFormAt(int row);
 
  private:
+  // Wraps the PasswordForm from the database and caches the display URL for
+  // quick sorting.
+  struct PasswordRow {
+    ~PasswordRow();
+
+    // Contains the URL that is displayed along with the 
+    gfx::SortedDisplayURL display_url;
+
+    // The underlying PasswordForm. We own this.
+    PasswordForm* form;
+  };
+
   // Cancel any pending login query involving a callback.
   void CancelLoginsQuery();
 
+  // The web data service associated with the currently active profile.
+  WebDataService* web_data_service() {
+    return profile_->GetWebDataService(Profile::EXPLICIT_ACCESS);
+  }
+
   // The TableView observing this model.
   ChromeViews::TableModelObserver* observer_;
 
   // Handle to any pending WebDataService::GetLogins query.
   WebDataService::Handle pending_login_query_;
 
-  // PasswordForms returned by the web data service query.
-  typedef std::vector<PasswordForm*> PasswordForms;
-  PasswordForms saved_signons_;
-
-  // Deleter for saved_logins_.
-  STLElementDeleter<PasswordForms> saved_signons_deleter_;
+  // The set of passwords we're showing.
+  typedef std::vector<PasswordRow> PasswordRows;
+  PasswordRows saved_signons_;
 
-  // The web data service associated with the currently active profile.
-  WebDataService* web_data_service_;
+  Profile* profile_;
 
   DISALLOW_EVIL_CONSTRUCTORS(PasswordManagerTableModel);
 };
diff --git a/chrome/browser/views/shelf_item_dialog.cc b/chrome/browser/views/shelf_item_dialog.cc
index f08ccb3..9293619 100644
--- a/chrome/browser/views/shelf_item_dialog.cc
+++ b/chrome/browser/views/shelf_item_dialog.cc
@@ -77,7 +77,17 @@ class PossibleURLModel : public ChromeViews::TableModel {
 
   void OnHistoryQueryComplete(HistoryService::Handle h,
                               history::QueryResults* result) {
-    results_.Swap(result);
+    results_.resize(result->size());
+    std::wstring languages = profile_
+        ? profile_->GetPrefs()->GetString(prefs::kAcceptLanguages)
+        : std::wstring();
+    for (size_t i = 0; i < result->size(); ++i) {
+      results_[i].url = (*result)[i].url();
+      results_[i].index = i;
+      results_[i].display_url =
+          gfx::SortedDisplayURL((*result)[i].url(), languages);
+      results_[i].title = (*result)[i].title();
+    }
 
     // The old version of this code would filter out all but the most recent
     // visit to each host, plus all typed URLs and AUTO_BOOKMARK transitions. I
@@ -102,7 +112,7 @@ class PossibleURLModel : public ChromeViews::TableModel {
       NOTREACHED();
       return GURL::EmptyGURL();
     }
-    return results_[row].url();
+    return results_[row].url;
   }
 
   const std::wstring& GetTitle(int row) {
@@ -110,7 +120,7 @@ class PossibleURLModel : public ChromeViews::TableModel {
       NOTREACHED();
       return EmptyWString();
     }
-    return results_[row].title();
+    return results_[row].title;
   }
 
   virtual std::wstring GetText(int row, int col_id) {
@@ -124,9 +134,7 @@ class PossibleURLModel : public ChromeViews::TableModel {
 
     // TODO(brettw): this should probably pass the GURL up so the URL elider
     // can be used at a higher level when we know the width.
-    return gfx::ElideUrl(GetURL(row), ChromeFont(), 0, profile_ ?
-        profile_->GetPrefs()->GetString(prefs::kAcceptLanguages) :
-        std::wstring());
+    return results_[row].display_url.display_url();
   }
 
   virtual SkBitmap GetIcon(int row) {
@@ -135,9 +143,10 @@ class PossibleURLModel : public ChromeViews::TableModel {
       return *default_fav_icon;
     }
 
-    const history::URLResult& result = results_[row];
-    FavIconMap::iterator i = fav_icon_map_.find(result.id());
+    Result& result = results_[row];
+    FavIconMap::iterator i = fav_icon_map_.find(result.index);
     if (i != fav_icon_map_.end()) {
+      // We already requested the favicon, return it.
       if (!i->second.isNull())
         return i->second;
     } else if (profile_) {
@@ -146,14 +155,25 @@ class PossibleURLModel : public ChromeViews::TableModel {
       if (hs) {
         CancelableRequestProvider::Handle h =
             hs->GetFavIconForURL(
-                result.url(), &consumer_,
+                result.url, &consumer_,
                 NewCallback(this, &PossibleURLModel::OnFavIconAvailable));
-        consumer_.SetClientData(hs, h, result.id());
+        consumer_.SetClientData(hs, h, result.index);
+        // Add an entry to the map so that we don't attempt to request the
+        // favicon again.
+        fav_icon_map_[result.index] = SkBitmap();
       }
     }
     return *default_fav_icon;
   }
 
+  virtual int CompareValues(int row1, int row2, int column_id) {
+    if (column_id == IDS_ASI_URL_COLUMN) {
+      return results_[row1].display_url.Compare(
+          results_[row2].display_url, GetCollator());
+    }
+    return TableModel::CompareValues(row1, row2, column_id);
+  }
+
   virtual void OnFavIconAvailable(
       HistoryService::Handle h,
       bool fav_icon_available,
@@ -163,23 +183,14 @@ class PossibleURLModel : public ChromeViews::TableModel {
     if (profile_) {
       HistoryService* hs =
           profile_->GetHistoryService(Profile::EXPLICIT_ACCESS);
-      history::URLID pid = consumer_.GetClientData(hs, h);
-      if (pid) {
-        SkBitmap bm;
-        if (fav_icon_available) {
-          // The decoder will leave our bitmap empty on error.
-          PNGDecoder::Decode(&data->data, &bm);
-        }
-
-        // Store the bitmap. We store it even if it is empty to make sure we
-        // don't query it again.
-        fav_icon_map_[pid] = bm;
-        if (!bm.isNull() && observer_) {
-          for (size_t i = 0; i < results_.size(); ++i) {
-            if (results_[i].id() == pid)
-              observer_->OnItemsChanged(static_cast<int>(i), 1);
-          }
-        }
+      size_t index = consumer_.GetClientData(hs, h);
+      if (fav_icon_available) {
+        // The decoder will leave our bitmap empty on error.
+        PNGDecoder::Decode(&data->data, &(fav_icon_map_[index]));
+
+        // Notify the observer.
+        if (!fav_icon_map_[index].isNull() && observer_)
+          observer_->OnItemsChanged(static_cast<int>(index), 1);
       }
     }
   }
@@ -189,6 +200,19 @@ class PossibleURLModel : public ChromeViews::TableModel {
   }
 
  private:
+  // Contains the data needed to show a result.
+  struct Result {
+    Result() : index(0) {}
+
+    GURL url;
+    // Index of this Result in results_. This is used as the key into
+    // fav_icon_map_ to lookup the favicon for the url, as well as the index
+    // into results_ when the favicon is received.
+    size_t index;
+    gfx::SortedDisplayURL display_url;
+    std::wstring title;
+  };
+
   // The current profile.
   Profile* profile_;
 
@@ -196,14 +220,13 @@ class PossibleURLModel : public ChromeViews::TableModel {
   ChromeViews::TableModelObserver* observer_;
 
   // Our consumer for favicon requests.
-  CancelableRequestConsumerT<history::URLID, NULL> consumer_;
+  CancelableRequestConsumerT<size_t, NULL> consumer_;
 
-  // The results provided by the history service.
-  history::QueryResults results_;
+  // The results we're showing.
+  std::vector<Result> results_;
 
-  // Map URLID -> Favicon. If we queried for a favicon and there is none, that
-  // URL will have an entry, and the bitmap will be empty.
-  typedef std::map<history::URLID, SkBitmap> FavIconMap;
+  // Map Result::index -> Favicon.
+  typedef std::map<size_t, SkBitmap> FavIconMap;
   FavIconMap fav_icon_map_;
 
   DISALLOW_EVIL_CONSTRUCTORS(PossibleURLModel);
@@ -229,9 +252,11 @@ ShelfItemDialog::ShelfItemDialog(ShelfItemDialogDelegate* delegate,
   ChromeViews::TableColumn col1(IDS_ASI_PAGE_COLUMN,
                                 ChromeViews::TableColumn::LEFT, -1,
                                 50);
+  col1.sortable = true;
   ChromeViews::TableColumn col2(IDS_ASI_URL_COLUMN,
                                 ChromeViews::TableColumn::LEFT, -1,
                                 50);
+  col2.sortable = true;
   std::vector<ChromeViews::TableColumn> cols;
   cols.push_back(col1);
   cols.push_back(col2);
diff --git a/chrome/views/table_view.cc b/chrome/views/table_view.cc
index e50cf04..c3100f8 100644
--- a/chrome/views/table_view.cc
+++ b/chrome/views/table_view.cc
@@ -19,8 +19,6 @@
 #include "chrome/views/view_container.h"
 #include "SkBitmap.h"
 #include "SkColorFilter.h"
-#include "unicode/coll.h"
-#include "unicode/uchar.h"
 
 namespace ChromeViews {
 
@@ -45,16 +43,8 @@ int TableModel::CompareValues(int row1, int row2, int column_id) {
          row2 >= 0 && row2 < RowCount());
   std::wstring value1 = GetText(row1, column_id);
   std::wstring value2 = GetText(row2, column_id);
+  Collator* collator = GetCollator();
 
-  if (!collator) {
-    UErrorCode create_status = U_ZERO_ERROR;
-    collator = Collator::createInstance(create_status);
-    if (!U_SUCCESS(create_status)) {
-      collator = NULL;
-      NOTREACHED();
-    }
-  }
-  
   if (collator) {
     UErrorCode compare_status = U_ZERO_ERROR;
     UCollationResult compare_result = collator->compare(
@@ -70,6 +60,18 @@ int TableModel::CompareValues(int row1, int row2, int column_id) {
   return 0;
 }
 
+Collator* TableModel::GetCollator() {
+  if (!collator) {
+    UErrorCode create_status = U_ZERO_ERROR;
+    collator = Collator::createInstance(create_status);
+    if (!U_SUCCESS(create_status)) {
+      collator = NULL;
+      NOTREACHED();
+    }
+  }
+  return collator;
+}
+
 // TableView ------------------------------------------------------------------
 
 TableView::TableView(TableModel* model,
diff --git a/chrome/views/table_view.h b/chrome/views/table_view.h
index a2cfc40..6f2df47 100644
--- a/chrome/views/table_view.h
+++ b/chrome/views/table_view.h
@@ -8,6 +8,8 @@
 #include <windows.h>
 
 #include <map>
+#include <unicode/coll.h>
+#include <unicode/uchar.h>
 #include <vector>
 
 #include "base/logging.h"
@@ -147,6 +149,10 @@ class TableModel {
   // This implementation does a case insensitive locale specific string
   // comparison.
   virtual int CompareValues(int row1, int row2, int column_id);
+
+ protected:
+  // Returns the collator used by CompareValues.
+  Collator* GetCollator();
 };
 
 // TableColumn specifies the title, alignment and size of a particular column.