IntersectionObserver: second cut.

third_party/WebKit/Source/core/dom/ElementIntersectionObserverData.h

+// Copyright 2015 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 ElementIntersectionObserverData_h
+#define ElementIntersectionObserverData_h
+
+#include "platform/heap/Handle.h"
+
+namespace blink {
+
+class Element;
+class IntersectionObservation;
+class IntersectionObserver;
+
+class ElementIntersectionObserverData : public GarbageCollectedFinalized<ElementIntersectionObserverData> {
+public:
+    DECLARE_TRACE();
+    ElementIntersectionObserverData();
+    ~ElementIntersectionObserverData();
+
+    bool hasIntersectionObserver() const;
+    bool hasIntersectionObservation() const;
+    bool hasObservationFor(IntersectionObserver&) const;
+    void addObservation(IntersectionObservation&);
+    void removeObservation(IntersectionObserver&);
+    void activateValidIntersectionObservers(Element&);
+    void deactivateAllIntersectionObservers(Element&);
+
+#if !ENABLE(OILPAN)
+    void dispose();
+#endif
+
+    WeakPtrWillBeRawPtr<Element> createWeakPtr(Element*);
+
+private:
+    // IntersectionObservers for which the Element owning this data is root.
+    HeapHashSet<WeakMember<IntersectionObserver>> m_intersectionObservers;
+    // IntersectionObservations for which the Element owning this data is target.
+    HeapHashMap<Member<IntersectionObserver>, Member<IntersectionObservation>> m_intersectionObservations;

[haraken, 2016/01/02 13:47:40]

I think this hash map is key for getting rid of checkRootAndDetachIfNeeded().

I don't fully understand why this hash map is needed.

- (As I commented in the previous review,) this hash map is adding a strong
reference from the Observer to the Observation (whose target element is this
element). We won't want to have the strong reference because the Observer
shouldn't keep alive the Observation.

- The Observer already knows a set of Observations. It looks strange if we have
to keep track of the Observations on the target element as well.

If we can remove this hash map, I think it's possible to remove
IntersectionObservation::disconnect(). Then we no longer need
checkRootAndDetachIfNeeded(). The problem will be solved.

[szager1, 2016/01/02 19:18:34]

I don't understand this comment.  How does this add a strong reference from
Observer to Observation?  Rather it adds a strong reference from target to
Observer and a strong reference from target to Observation.

The target needs to hold a strong reference to the Observation, because that's
the only thing keeping the Observation alive; nothing else holds a strong
reference to the Observation.

The Observation needs to hold a strong reference to the Observer, to keep the
Observer alive when there are no more javascript references to the Observer; for
example, this code:

new IntersectionObserver().observe(target);

... should work, i.e., the observer should stay alive as long as it has an
active observation.

In m_intersectionObservations, the strong reference from target to
IntersectionObserver (which is the hash key) is just an implementation detail,
it does not change the lifetime or ownership model; it's just the transitive
closure of target=>IntersectionObservation=>IntersectionObserver, which must
exist.

[haraken, 2016/01/03 16:34:20]

Yeah, I was confused. You're right.

This may not be a big issue for removing checkRootAndDetachIfNecessary(), but I
still don't understand why the target element needs to keep a direct strong
reference to the Observer. Given that the Observation already holds a strong
reference to the Observer, the target element just needs to hold a strong
reference to the Obseravation, doesn't it? (i.e.,
HeapHashSet<Member<IntersectionObvervation>> is enough.)

[szager1, 2016/01/04 03:53:04]

IntersectionObserver::observe() calls into
m_intersectionObservations.contains(observer), to see if there's already an
observer for this given observer/target pair.

A previous version of this patch used custom hash_traits to make
m_intersectionObservations a HashSet<IntersectionObservation> that supported
contains(target, observer).  esprehn objected that this use case did not merit
the complexity of custom hash_traits, so I switched to
HashMap<IntersectionObserver, IntersectionObservation>.

+
+#if !ENABLE(OILPAN)
+    OwnPtr<WeakPtrFactory<Element>> m_weakPointerFactory;
+#endif
+};
+
+} // namespace blink
+
+#endif // ElementIntersectionObserverData_h