[Oilpan] Remove a raw pointer to PlatformEventController class on Oilpan build

Source/core/frame/PlatformEventDispatcher.cpp

 // Copyright 2014 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 "config.h"
 #include "core/frame/PlatformEventDispatcher.h"
 
 #include "core/frame/PlatformEventController.h"
 #include "wtf/TemporaryChange.h"
 
 namespace blink {
 
 PlatformEventDispatcher::PlatformEventDispatcher()
-    : m_needsPurge(false)
-    , m_isDispatching(false)
+    : m_isDispatching(false)
+    , m_isListening(false)
 {
 }
 
 void PlatformEventDispatcher::addController(PlatformEventController* controller)
 {
-    bool wasEmpty = m_controllers.isEmpty();
-    if (!m_controllers.contains(controller))
-        m_controllers.append(controller);
-    if (wasEmpty)
+    ASSERT(controller);
+
+    m_controllers.add(controller);
+    if (!m_isListening) {
         startListening();
+        m_isListening = true;
+    }
 }
 
 void PlatformEventDispatcher::removeController(PlatformEventController* controller)
 {
-    // Do not actually remove the controller from the vector, instead zero them out.
-    // The zeros are removed in these two cases:
-    // 1. either immediately if we are not dispatching any events,
-    // 2. or after events to all controllers have dispatched (see notifyControllers()).
-    // This is to correctly handle the re-entrancy case when a controller is destroyed
-    // while the events are still being dispatched.
-    size_t index = m_controllers.find(controller);
-    if (index == kNotFound)
+    if (!m_controllers.contains(controller))

[timvolodine, 2015/08/21 15:03:32]

I think you can drop this check. m_controllers.remove() seems to already have an
early return if the controller is not there (from looking at HashSet
implementation)

[peria, 2015/08/24 02:46:03]

Done.

         return;
 
-    m_controllers[index] = nullptr;
-    m_needsPurge = true;
-
-    if (!m_isDispatching)
-        purgeControllers();
-}
-
-void PlatformEventDispatcher::purgeControllers()
-{
-    ASSERT(m_needsPurge);
-
-    size_t i = 0;
-    while (i < m_controllers.size()) {
-        if (!m_controllers[i]) {
-            m_controllers[i] = m_controllers.last();
-            m_controllers.removeLast();
-        } else {
-            ++i;
-        }
+    m_controllers.remove(controller);
+    if (!m_isDispatching && m_controllers.isEmpty()) {
+        stopListening();
+        m_isListening = false;
     }
-
-    m_needsPurge = false;
-
-    if (m_controllers.isEmpty())
-        stopListening();
 }
 
 void PlatformEventDispatcher::notifyControllers()
 {
+    if (m_controllers.isEmpty())
+        return;
+
     {
         TemporaryChange<bool> changeIsDispatching(m_isDispatching, true);
-        // Don't notify controllers removed or added during event dispatch.
-        size_t size = m_controllers.size();
-        for (size_t i = 0; i < size; ++i) {
-            if (m_controllers[i])
-                m_controllers[i]->didUpdateData();
+        // PlatformEventController::didUpdateData() can add/remove elements of m_controllers.

[timvolodine, 2015/08/21 15:03:32]

this is basically re-entrancy on the JavaScript side i.e. event callback can
remove another event listener and its controller for example. technically
PlatformEventController::didUpdateData() is pure virtual..

[peria, 2015/08/24 02:46:02]

Thank you for the explanation.
I got it.

+        // It can change the structure of m_controllers and we cannot iterate it.
+        // Thus we take a snapshot and restore into a Vector to access all elements.

[timvolodine, 2015/08/21 15:03:32]

"restore" -> "store" ?

[peria, 2015/08/24 02:46:03]

Done.

+        HeapVector<PlatformEventController*> snapshotVector;

[timvolodine, 2015/08/21 15:03:32]

you could supply an initial capacity to the vector constructor here for
efficiency

[peria, 2015/08/24 02:46:03]

Done.

+        for (PlatformEventController* controller : m_controllers)
+            snapshotVector.append(controller);
+        for (PlatformEventController* controller : snapshotVector) {
+            if (m_controllers.contains(controller))
+                controller->didUpdateData();
         }
     }
 
-    if (m_needsPurge)
-        purgeControllers();
+    if (m_controllers.isEmpty()) {
+        stopListening();
+        m_isListening = false;
+    }
 }
 
 DEFINE_TRACE(PlatformEventDispatcher)
 {
 #if ENABLE(OILPAN)
-    // Trace the backing store, the weak(&bare) element references won't be.
     visitor->trace(m_controllers);
-    visitor->template registerWeakMembers<PlatformEventDispatcher, &PlatformEventDispatcher::clearWeakMembers>(this);
 #endif
 }
 
-#if ENABLE(OILPAN)
-void PlatformEventDispatcher::clearWeakMembers(Visitor* visitor)
-{
-    for (size_t i = 0; i < m_controllers.size(); ++i) {
-        if (!Heap::isHeapObjectAlive(m_controllers[i])) {
-            m_controllers[i] = nullptr;
-            m_needsPurge = true;
-        }
-    }
-    // Next notification will purge the empty slots.
-}
-#endif
-
 } // namespace blink