Converts usage of SetProp/GetProp to a map. Even after making sure we

clean up props we still leak in a handful of cases that are causing
test grief. By and large our usage of properties is for inside the
application, so that a map works fine.

BUG=61528 44991

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

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

6 files changed, 235 insertions, 0 deletions

diff --git a/app/app.gyp b/app/app.gyp
index 2c8e7f7..ce64955 100644
--- a/app/app.gyp
+++ b/app/app.gyp
@@ -56,6 +56,7 @@
         'text_elider_unittest.cc',
         'tree_node_iterator_unittest.cc',
         'tree_node_model_unittest.cc',
+        'view_prop_unittest.cc',
         'win_util_unittest.cc',
       ],
       'include_dirs': [
@@ -75,6 +76,7 @@
         ['OS!="win"', {
           'sources!': [
             'os_exchange_data_win_unittest.cc',
+            'view_prop_unittest.cc',
             'win_util_unittest.cc',
           ],
         }],
diff --git a/app/app_base.gypi b/app/app_base.gypi
index 37e0640..a121a50 100644
--- a/app/app_base.gypi
+++ b/app/app_base.gypi
@@ -221,6 +221,8 @@
         'throb_animation.h',
         'tween.cc',
         'tween.h',
+        'view_prop.cc',
+        'view_prop.h',
         'win/drag_source.cc',
         'win/drag_source.h',
         'win/drop_target.cc',
@@ -315,6 +317,8 @@
             'gfx/native_theme_win.cc',
             'gfx/native_theme_win.h',
             'os_exchange_data.cc',
+            'view_prop.cc',
+            'view_prop.h',
             'win/iat_patch_function.cc',
             'win/iat_patch_function.h',
           ],
diff --git a/app/view_prop.cc b/app/view_prop.cc
new file mode 100644
index 0000000..778f381
--- /dev/null
+++ b/app/view_prop.cc
@@ -0,0 +1,106 @@
+// 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 "app/view_prop.h"
+
+#include <set>
+
+namespace app {
+namespace win {
+
+// Maints the actual view, key and data.
+class ViewProp::Data : public base::RefCounted<ViewProp::Data> {
+ public:
+  // Returns the Data* for the view/key pair. If |create| is false and |Get|
+  // has not been invoked for the view/key pair, NULL is returned.
+  static void Get(gfx::NativeView view,
+                  const char* key,
+                  bool create,
+                  scoped_refptr<Data>* data) {
+    if (!data_set_)
+      data_set_ = new DataSet;
+    scoped_refptr<Data> new_data(new Data(view, key));
+    DataSet::const_iterator i = data_set_->find(new_data.get());
+    if (i != data_set_->end()) {
+      *data = *i;
+      return;
+    }
+    if (!create)
+      return;
+    data_set_->insert(new_data.get());
+    *data = new_data.get();
+  }
+
+  // The data.
+  void set_data(void* data) { data_ = data; }
+  void* data() const { return data_; }
+
+  const char* key() const { return key_; }
+
+ private:
+  friend class base::RefCounted<Data>;
+
+  // Used to order the Data in the map.
+  class DataComparator {
+   public:
+    bool operator()(const Data* d1, const Data* d2) const {
+      return (d1->view_ == d2->view_) ? (d1->key_ < d2->key_) :
+                                        (d1->view_ < d2->view_);
+    }
+  };
+
+  typedef std::set<Data*, DataComparator> DataSet;
+
+  Data(gfx::NativeView view, const char* key)
+      : view_(view),
+        key_(key),
+        data_(NULL) {}
+
+  ~Data() {
+    DataSet::iterator i = data_set_->find(this);
+    // Also check for equality using == as |Get| creates dummy values in order
+    // to look up a value.
+    if (i != data_set_->end() && *i == this)
+      data_set_->erase(i);
+  }
+
+  // The existing set of Data is stored here. ~Data removes from the set.
+  static DataSet* data_set_;
+
+  const gfx::NativeView view_;
+  const char* key_;
+  void* data_;
+
+  DISALLOW_COPY_AND_ASSIGN(Data);
+};
+
+// static
+ViewProp::Data::DataSet* ViewProp::Data::data_set_ = NULL;
+
+ViewProp::ViewProp(gfx::NativeView view, const char* key, void* data) {
+  Data::Get(view, key, true, &data_);
+  data_->set_data(data);
+}
+
+ViewProp::~ViewProp() {
+  // This is done to provide similar semantics to SetProp. In particular it's
+  // assumed that ~ViewProp should behave as though RemoveProp was invoked.
+  data_->set_data(NULL);
+}
+
+// static
+void* ViewProp::GetValue(gfx::NativeView view, const char* key) {
+  scoped_refptr<Data> data;
+  Data::Get(view, key, false, &data);
+  return data.get() ? data->data() : NULL;
+}
+
+// static
+const char* ViewProp::Key() const {
+  return data_->key();
+}
+
+}  // namespace win
+
+}  // namespace app
diff --git a/app/view_prop.h b/app/view_prop.h
new file mode 100644
index 0000000..c319460
--- /dev/null
+++ b/app/view_prop.h
@@ -0,0 +1,47 @@
+// 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.
+
+#ifndef APP_VIEW_PROP_H_
+#define APP_VIEW_PROP_H_
+#pragma once
+
+#include "base/basictypes.h"
+#include "base/ref_counted.h"
+#include "gfx/native_widget_types.h"
+
+namespace app {
+
+// ViewProp maintains a key/value pair for a particular view. ViewProp is
+// designed as a replacement for the Win32's SetProp, but does not make use of
+// window manager memory. ViewProp shares similar semantics as SetProp, the
+// value for a particular view/key pair comes from the last ViewProp created.
+class ViewProp {
+ public:
+  // Associates data with a view/key pair. If a ViewProp has already been
+  // created for the specified pair |data| replaces the current value.
+  //
+  // ViewProp does *not* make a copy of the char*, the pointer is used for
+  // sorting.
+  ViewProp(gfx::NativeView view, const char* key, void* data);
+  ~ViewProp();
+
+  // Returns the value associated with the view/key pair, or NULL if there is
+  // none.
+  static void* GetValue(gfx::NativeView view, const char* key);
+
+  // Returns the key.
+  const char* Key() const;
+
+ private:
+  class Data;
+
+  // Stores the actual data.
+  scoped_refptr<Data> data_;
+
+  DISALLOW_COPY_AND_ASSIGN(ViewProp);
+};
+
+}  // namespace app
+
+#endif  // APP_VIEW_PROP_H_
diff --git a/app/view_prop_unittest.cc b/app/view_prop_unittest.cc
new file mode 100644
index 0000000..9b81cd5
--- /dev/null
+++ b/app/view_prop_unittest.cc
@@ -0,0 +1,70 @@
+// 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 "testing/gtest/include/gtest/gtest.h"
+
+#include "app/view_prop.h"
+#include "base/scoped_ptr.h"
+
+typedef testing::Test ViewPropTest;
+
+static const char* kKey1 = "key_1";
+static const char* kKey2 = "key_2";
+
+using app::ViewProp;
+
+// Test a handful of viewprop assertions.
+TEST_F(ViewPropTest, Basic) {
+  gfx::NativeView nv1 = reinterpret_cast<gfx::NativeView>(1);
+  gfx::NativeView nv2 = reinterpret_cast<gfx::NativeView>(2);
+
+  void* data1 = reinterpret_cast<void*>(11);
+  void* data2 = reinterpret_cast<void*>(12);
+
+  // Initial value for a new view/key pair should be NULL.
+  EXPECT_EQ(NULL, ViewProp::GetValue(nv1, kKey1));
+
+  {
+    // Register a value for a view/key pair.
+    ViewProp prop(nv1, kKey1, data1);
+    EXPECT_EQ(data1, ViewProp::GetValue(nv1, kKey1));
+  }
+
+  // The property fell out of scope, so the value should now be NULL.
+  EXPECT_EQ(NULL, ViewProp::GetValue(nv1, kKey1));
+
+  {
+    // Register a value for a view/key pair.
+    scoped_ptr<ViewProp> v1(new ViewProp(nv1, kKey1, data1));
+    EXPECT_EQ(data1, ViewProp::GetValue(nv1, kKey1));
+
+    // Register a value for the same view/key pair.
+    scoped_ptr<ViewProp> v2(new ViewProp(nv1, kKey1, data2));
+    // The new value should take over.
+    EXPECT_EQ(data2, ViewProp::GetValue(nv1, kKey1));
+
+    // Null out the first ViewProp, which should NULL out the value.
+    v1.reset(NULL);
+    EXPECT_EQ(NULL, ViewProp::GetValue(nv1, kKey1));
+  }
+
+  // The property fell out of scope, so the value should now be NULL.
+  EXPECT_EQ(NULL, ViewProp::GetValue(nv1, kKey1));
+
+  {
+    // Register a value for a view/key pair.
+    scoped_ptr<ViewProp> v1(new ViewProp(nv1, kKey1, data1));
+    scoped_ptr<ViewProp> v2(new ViewProp(nv2, kKey2, data2));
+    EXPECT_EQ(data1, ViewProp::GetValue(nv1, kKey1));
+    EXPECT_EQ(data2, ViewProp::GetValue(nv2, kKey2));
+
+    v1.reset(NULL);
+    EXPECT_EQ(NULL, ViewProp::GetValue(nv1, kKey1));
+    EXPECT_EQ(data2, ViewProp::GetValue(nv2, kKey2));
+
+    v2.reset(NULL);
+    EXPECT_EQ(NULL, ViewProp::GetValue(nv1, kKey1));
+    EXPECT_EQ(NULL, ViewProp::GetValue(nv2, kKey2));
+  }
+}
diff --git a/app/win/scoped_prop.h b/app/win/scoped_prop.h
index 14e6a6a..c309e52 100644
--- a/app/win/scoped_prop.h
+++ b/app/win/scoped_prop.h
@@ -18,6 +18,12 @@ namespace win {
 // cleanup. ScopedProp must be destroyed before the window is destroyed, else
 // you're going to leak a property, which could lead to failure to set a
 // property later on.
+//
+// *WARNING*
+// SetProp is very fragile. SetProp makes use of a finite chunk of memory that
+// is very easy to exhaust. Unless you need to share a property across process
+// boundaries you should instead use ViewProp, which does not cause leaks at the
+// window manager.
 class ScopedProp {
  public:
   // Registers the key value pair for the specified window. ScopedProp does not