Reland r51414: "GTK: Fix sorting in content exception window."

I can't reproduce the heapcheck failure locally, so add a speculative fix.
The class under test uses DeleteSoon(), so let's try adding a RunAllPending().

First Review URL: http://codereview.chromium.org/2876032
BUG=47841
TEST=ContentExceptionsWindowGtkUnittest.* + see bug. Passes valgrind+heapcheck.

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

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

4 files changed, 264 insertions, 18 deletions

diff --git a/chrome/browser/gtk/options/content_exceptions_window_gtk.cc b/chrome/browser/gtk/options/content_exceptions_window_gtk.cc
index 5894e9f..06ad488 100644
--- a/chrome/browser/gtk/options/content_exceptions_window_gtk.cc
+++ b/chrome/browser/gtk/options/content_exceptions_window_gtk.cc
@@ -51,12 +51,15 @@ ContentExceptionsWindowGtk::ContentExceptionsWindowGtk(
     HostContentSettingsMap* off_the_record_map,
     ContentSettingsType type)
     : allow_off_the_record_(off_the_record_map) {
-  // Build the model adapters that translate views and TableModels into
-  // something GTK can use.
-  list_store_ = gtk_list_store_new(
-      COL_COUNT, G_TYPE_STRING, G_TYPE_STRING, PANGO_TYPE_STYLE);
-  treeview_ = gtk_tree_view_new_with_model(GTK_TREE_MODEL(list_store_));
+  // Build the list backing that GTK uses, along with an adapter which will
+  // sort stuff without changing the underlying backing store.
+  list_store_ = gtk_list_store_new(COL_COUNT, G_TYPE_STRING, G_TYPE_STRING,
+                                   PANGO_TYPE_STYLE);
+  sort_list_store_ = gtk_tree_model_sort_new_with_model(
+      GTK_TREE_MODEL(list_store_));
+  treeview_ = gtk_tree_view_new_with_model(GTK_TREE_MODEL(sort_list_store_));
   g_object_unref(list_store_);
+  g_object_unref(sort_list_store_);
 
   // Set up the properties of the treeview
   gtk_tree_view_set_headers_visible(GTK_TREE_VIEW(treeview_), TRUE);
@@ -218,7 +221,7 @@ void ContentExceptionsWindowGtk::UpdateButtonState() {
   int num_selected = gtk_tree_selection_count_selected_rows(
       treeview_selection_);
   int row_count = gtk_tree_model_iter_n_children(
-      GTK_TREE_MODEL(list_store_), NULL);
+      GTK_TREE_MODEL(sort_list_store_), NULL);
 
   // TODO(erg): http://crbug.com/34177 , support editing of more than one entry
   // at a time.
@@ -235,10 +238,10 @@ void ContentExceptionsWindowGtk::Add(GtkWidget* widget) {
 }
 
 void ContentExceptionsWindowGtk::Edit(GtkWidget* widget) {
-  std::set<int> indices;
-  gtk_tree::GetSelectedIndices(treeview_selection_, &indices);
+  std::set<std::pair<int, int> > indices;
+  GetSelectedModelIndices(&indices);
   DCHECK_GT(indices.size(), 0u);
-  int index = *indices.begin();
+  int index = indices.begin()->first;
   const HostContentSettingsMap::PatternSettingPair& entry =
       model_->entry_at(index);
   new ContentExceptionEditor(GTK_WINDOW(dialog_), this, model_.get(),
@@ -248,14 +251,16 @@ void ContentExceptionsWindowGtk::Edit(GtkWidget* widget) {
 }
 
 void ContentExceptionsWindowGtk::Remove(GtkWidget* widget) {
-  std::set<int> selected_indices;
-  gtk_tree::GetSelectedIndices(treeview_selection_, &selected_indices);
-
-  int selected_row = 0;
-  for (std::set<int>::reverse_iterator i = selected_indices.rbegin();
-       i != selected_indices.rend(); ++i) {
-    model_->RemoveException(*i);
-    selected_row = *i;
+  std::set<std::pair<int, int> > model_selected_indices;
+  GetSelectedModelIndices(&model_selected_indices);
+
+  int selected_row = gtk_tree_model_iter_n_children(
+      GTK_TREE_MODEL(sort_list_store_), NULL);
+  for (std::set<std::pair<int, int> >::reverse_iterator i =
+           model_selected_indices.rbegin();
+       i != model_selected_indices.rend(); ++i) {
+    model_->RemoveException(i->first);
+    selected_row = std::min(selected_row, i->second);
   }
 
   int row_count = model_->RowCount();
@@ -292,6 +297,25 @@ std::string ContentExceptionsWindowGtk::GetWindowTitle() const {
   return std::string();
 }
 
+void ContentExceptionsWindowGtk::GetSelectedModelIndices(
+    std::set<std::pair<int, int> >* indices) {
+  GtkTreeModel* model;
+  GList* paths = gtk_tree_selection_get_selected_rows(treeview_selection_,
+                                                      &model);
+  for (GList* item = paths; item; item = item->next) {
+    GtkTreePath* sorted_path = reinterpret_cast<GtkTreePath*>(item->data);
+    int sorted_row = gtk_tree::GetRowNumForPath(sorted_path);
+    GtkTreePath* path = gtk_tree_model_sort_convert_path_to_child_path(
+        GTK_TREE_MODEL_SORT(sort_list_store_), sorted_path);
+    int row = gtk_tree::GetRowNumForPath(path);
+    gtk_tree_path_free(path);
+    indices->insert(std::make_pair(row, sorted_row));
+  }
+
+  g_list_foreach(paths, (GFunc)gtk_tree_path_free, NULL);
+  g_list_free(paths);
+}
+
 void ContentExceptionsWindowGtk::OnTreeViewRowActivate(
     GtkWidget* sender,
     GtkTreePath* path,
diff --git a/chrome/browser/gtk/options/content_exceptions_window_gtk.h b/chrome/browser/gtk/options/content_exceptions_window_gtk.h
index aadd3ad..b36d8d1 100644
--- a/chrome/browser/gtk/options/content_exceptions_window_gtk.h
+++ b/chrome/browser/gtk/options/content_exceptions_window_gtk.h
@@ -69,6 +69,10 @@ class ContentExceptionsWindowGtk : public gtk_tree::TableAdapter::Delegate,
   // was set to in the constructor).
   std::string GetWindowTitle() const;
 
+  // Gets the selected indicies in the two list stores. Indicies are returned
+  // in <list_store_, sort_list_store_> order.
+  void GetSelectedModelIndices(std::set<std::pair<int, int> >* indices);
+
   // GTK Callbacks
   CHROMEGTK_CALLBACK_2(ContentExceptionsWindowGtk, void,
                        OnTreeViewRowActivate, GtkTreePath*, GtkTreeViewColumn*);
@@ -76,7 +80,12 @@ class ContentExceptionsWindowGtk : public gtk_tree::TableAdapter::Delegate,
   CHROMEGTK_CALLBACK_0(ContentExceptionsWindowGtk, void,
                        OnTreeSelectionChanged);
 
-  // The list presented in |treeview_|; a gobject instead of a C++ object.
+  // The list presented in |treeview_|. Separate from |list_store_|, the list
+  // that backs |sort_model_|. This separation comes so the user can sort the
+  // data on screen without changing the underlying |list_store_|.
+  GtkTreeModel* sort_list_store_;
+
+  // The backing to |sort_list_store_|. Doesn't change when sorted.
   GtkListStore* list_store_;
 
   // The C++, views-ish, cross-platform model class that actually contains the
@@ -103,6 +112,8 @@ class ContentExceptionsWindowGtk : public gtk_tree::TableAdapter::Delegate,
   GtkWidget* edit_button_;
   GtkWidget* remove_button_;
   GtkWidget* remove_all_button_;
+
+  friend class ContentExceptionsWindowGtkUnittest;
 };
 
 #endif  // CHROME_BROWSER_GTK_OPTIONS_CONTENT_EXCEPTIONS_WINDOW_GTK_H_
diff --git a/chrome/browser/gtk/options/content_exceptions_window_gtk_unittest.cc b/chrome/browser/gtk/options/content_exceptions_window_gtk_unittest.cc
new file mode 100644
index 0000000..6e603b5
--- /dev/null
+++ b/chrome/browser/gtk/options/content_exceptions_window_gtk_unittest.cc
@@ -0,0 +1,210 @@
+// Copyright (c) 2010 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.
+
+#include "chrome/browser/gtk/options/content_exceptions_window_gtk.h"
+
+#include "chrome/test/testing_profile.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+using ::testing::ElementsAre;
+
+class ContentExceptionsWindowGtkUnittest : public testing::Test {
+ public:
+  ContentExceptionsWindowGtkUnittest()
+      : ui_thread_(ChromeThread::UI, &message_loop_),
+        host_content_settings_map_(profile_.GetHostContentSettingsMap()),
+        window_(NULL) {
+  }
+
+  ~ContentExceptionsWindowGtkUnittest() {
+    // This will delete window_ too.
+    gtk_widget_destroy(window_->dialog_);
+    message_loop_.RunAllPending();
+  }
+
+  void AddException(const std::string& pattern, ContentSetting value) {
+    host_content_settings_map_->SetContentSetting(
+        HostContentSettingsMap::Pattern(pattern),
+        CONTENT_SETTINGS_TYPE_JAVASCRIPT,
+        value);
+  }
+
+  void BuildWindow() {
+    window_ = new ContentExceptionsWindowGtk(
+        NULL,
+        host_content_settings_map_,
+        NULL,
+        CONTENT_SETTINGS_TYPE_JAVASCRIPT);
+  }
+
+  // Actions:
+  void SelectRows(const std::vector<int>& rows) {
+    gtk_tree_selection_unselect_all(window_->treeview_selection_);
+
+    for (std::vector<int>::const_iterator it = rows.begin(); it != rows.end();
+         ++it) {
+      GtkTreeIter iter;
+      if (!gtk_tree_model_iter_nth_child(window_->sort_list_store_,
+                                         &iter, NULL, *it)) {
+        NOTREACHED();
+        return;
+      }
+
+      gtk_tree_selection_select_iter(window_->treeview_selection_, &iter);
+    }
+  }
+
+  void Remove() {
+    window_->Remove(window_->remove_button_);
+  }
+
+  // Getters:
+
+  void GetSelectedRows(std::set<std::pair<int, int> >* selected) {
+    window_->GetSelectedModelIndices(selected);
+  }
+
+  int StoreListStoreCount() {
+    return gtk_tree_model_iter_n_children(
+        GTK_TREE_MODEL(window_->sort_list_store_), NULL);
+  }
+
+  int ListStoreCount() {
+    return gtk_tree_model_iter_n_children(
+        GTK_TREE_MODEL(window_->list_store_), NULL);
+  }
+
+  std::set<std::string> Paths() {
+    std::set<std::string> paths;
+    GtkTreeIter iter;
+    bool valid = gtk_tree_model_get_iter_first(
+        GTK_TREE_MODEL(window_->sort_list_store_), &iter);
+
+    while (valid) {
+      gchar* str = NULL;
+      gtk_tree_model_get(GTK_TREE_MODEL(window_->sort_list_store_), &iter,
+                         0, &str, -1);
+      paths.insert(str);
+      g_free(str);
+
+      valid = gtk_tree_model_iter_next(
+          GTK_TREE_MODEL(window_->sort_list_store_), &iter);
+    }
+
+    return paths;
+  }
+
+  // Whether certain widgets are enabled:
+  bool EditButtonSensitive() {
+    return GTK_WIDGET_SENSITIVE(window_->edit_button_);
+  }
+
+  bool RemoveButtonSensitive() {
+    return GTK_WIDGET_SENSITIVE(window_->remove_button_);
+  }
+
+  bool RemoveAllSensitive() {
+    return GTK_WIDGET_SENSITIVE(window_->remove_all_button_);
+  }
+
+ private:
+  MessageLoop message_loop_;
+  ChromeThread ui_thread_;
+
+  TestingProfile profile_;
+  HostContentSettingsMap* host_content_settings_map_;
+
+  ContentExceptionsWindowGtk* window_;
+};
+
+TEST_F(ContentExceptionsWindowGtkUnittest, TestEmptyDisplay) {
+  BuildWindow();
+
+  EXPECT_EQ(0, StoreListStoreCount());
+  EXPECT_EQ(0, ListStoreCount());
+
+  EXPECT_FALSE(RemoveAllSensitive());
+}
+
+TEST_F(ContentExceptionsWindowGtkUnittest, TestDisplay) {
+  AddException("a", CONTENT_SETTING_BLOCK);
+  AddException("b", CONTENT_SETTING_ALLOW);
+
+  BuildWindow();
+
+  EXPECT_EQ(2, StoreListStoreCount());
+  EXPECT_EQ(2, ListStoreCount());
+
+  EXPECT_TRUE(RemoveAllSensitive());
+}
+
+TEST_F(ContentExceptionsWindowGtkUnittest, TestSelection) {
+  AddException("a", CONTENT_SETTING_BLOCK);
+
+  BuildWindow();
+
+  EXPECT_FALSE(EditButtonSensitive());
+  EXPECT_FALSE(RemoveButtonSensitive());
+
+  std::vector<int> rows;
+  rows.push_back(0);
+  SelectRows(rows);
+
+  EXPECT_TRUE(EditButtonSensitive());
+  EXPECT_TRUE(RemoveButtonSensitive());
+}
+
+TEST_F(ContentExceptionsWindowGtkUnittest, TestSimpleRemoval) {
+  AddException("a", CONTENT_SETTING_BLOCK);
+  AddException("b", CONTENT_SETTING_ALLOW);
+  AddException("c", CONTENT_SETTING_BLOCK);
+  AddException("d", CONTENT_SETTING_ALLOW);
+
+  BuildWindow();
+
+  std::vector<int> rows;
+  rows.push_back(1);
+  SelectRows(rows);
+
+  Remove();
+
+  EXPECT_EQ(3, StoreListStoreCount());
+  EXPECT_EQ(3, ListStoreCount());
+  EXPECT_TRUE(EditButtonSensitive());
+  EXPECT_TRUE(RemoveButtonSensitive());
+
+  std::set<std::pair<int, int> > selected;
+  GetSelectedRows(&selected);
+
+  ASSERT_EQ(1u, selected.size());
+  EXPECT_EQ(1, selected.begin()->first);
+  EXPECT_EQ(1, selected.begin()->second);
+  EXPECT_THAT(Paths(), ElementsAre("a", "c", "d"));
+}
+
+TEST_F(ContentExceptionsWindowGtkUnittest, TestComplexRemoval) {
+  AddException("a", CONTENT_SETTING_BLOCK);
+  AddException("b", CONTENT_SETTING_ALLOW);
+  AddException("c", CONTENT_SETTING_BLOCK);
+  AddException("d", CONTENT_SETTING_ALLOW);
+  AddException("e", CONTENT_SETTING_BLOCK);
+
+  BuildWindow();
+
+  std::vector<int> rows;
+  rows.push_back(1);
+  rows.push_back(3);
+  SelectRows(rows);
+
+  Remove();
+
+  std::set<std::pair<int, int> > selected;
+  GetSelectedRows(&selected);
+
+  ASSERT_EQ(1u, selected.size());
+  EXPECT_EQ(1, selected.begin()->first);
+  EXPECT_EQ(1, selected.begin()->second);
+  EXPECT_THAT(Paths(), ElementsAre("a", "c", "e"));
+}
diff --git a/chrome/chrome_tests.gypi b/chrome/chrome_tests.gypi
index bb0eb26..4014fd8 100644
--- a/chrome/chrome_tests.gypi
+++ b/chrome/chrome_tests.gypi
@@ -831,6 +831,7 @@
         'browser/gtk/gtk_expanded_container_unittest.cc',
         'browser/gtk/gtk_theme_provider_unittest.cc',
         'browser/gtk/keyword_editor_view_unittest.cc',
+        'browser/gtk/options/content_exceptions_window_gtk_unittest.cc',
         'browser/gtk/options/cookies_view_unittest.cc',
         'browser/gtk/options/languages_page_gtk_unittest.cc',
         'browser/gtk/reload_button_gtk_unittest.cc',