Introduce RenderView::Observer interface so that RenderView doesn't have to k...

chrome/renderer/page_click_tracker_unittest.cc

Index: chrome/renderer/page_click_tracker_unittest.cc
===================================================================
--- chrome/renderer/page_click_tracker_unittest.cc	(revision 71253)
+++ chrome/renderer/page_click_tracker_unittest.cc	(working copy)
@@ -49,10 +49,14 @@
 
 // Tests that PageClickTracker does notify correctly when a node is clicked.
 TEST_F(RenderViewTest, PageClickTracker) {
+  // RenderView creates PageClickTracker but it doesn't keep it around.  Rather
+  // than make it do so for the test, we create a new object.
+  PageClickTracker* page_click_tracker = new PageClickTracker(view_);

[dhollowa, 2011/01/14 16:40:48]

Leak?  I'm not seeing where |page_click_tracker| goes away.  A simple stack
variable would be better.

[jam, 2011/01/14 23:00:03]

RenderView manages the observers and will delete them when it goes away

[dhollowa, 2011/01/14 23:21:23]

Really?  But there is a raw "new PageClickTracker(...);" call here, and I see no
transfer of ownership anywhere.  RemoveListener doesn't delete |this| or
anything.  What am I missing?

Certainly the original code had a |view_| that clearly owned the tracker, but
not with the new code.

On 2011/01/14 23:00:03, John Abd-El-Malek wrote:

[jam, 2011/01/15 00:06:54]

check out render_view_observer.h.  The objects become owned by RenderView.

+
   TestPageClickListener test_listener1;
   TestPageClickListener test_listener2;
-  view_->page_click_tracker()->AddListener(&test_listener1);
-  view_->page_click_tracker()->AddListener(&test_listener2);
+  page_click_tracker->AddListener(&test_listener1);
+  page_click_tracker->AddListener(&test_listener2);
 
   LoadHTML("<form>"
            "  <input type='text' id='text'></input><br>"
@@ -106,14 +110,14 @@
   test_listener1.ClearResults();
 
   // Make sure removing a listener work.
-  view_->page_click_tracker()->RemoveListener(&test_listener1);
+  page_click_tracker->RemoveListener(&test_listener1);
   EXPECT_TRUE(SimulateElementClick("text"));
   EXPECT_FALSE(test_listener1.called_);
   EXPECT_TRUE(test_listener2.called_);
   test_listener2.ClearResults();
 
   // Make sure we don't choke when no listeners are registered.
-  view_->page_click_tracker()->RemoveListener(&test_listener2);
+  page_click_tracker->RemoveListener(&test_listener2);
   EXPECT_TRUE(SimulateElementClick("text"));
   EXPECT_FALSE(test_listener1.called_);
   EXPECT_FALSE(test_listener2.called_);