IntersectionObserver: second cut.

third_party/WebKit/Source/core/dom/IntersectionObservation.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.
+
+#include "platform/geometry/LayoutRect.h"
+#include "platform/heap/Handle.h"
+#include "wtf/HashSet.h"
+#include "wtf/RefPtr.h"
+#include "wtf/WeakPtr.h"
+
+#ifndef IntersectionObservation_h
+#define IntersectionObservation_h
+
+namespace blink {
+
+class Element;
+class IntersectionObserver;
+
+class IntersectionObservation : public GarbageCollectedFinalized<IntersectionObservation> {
+public:
+    typedef std::pair<IntersectionObserver*, Element*> HashKey;
+    typedef Member<IntersectionObservation> ValueType;
+    typedef WeakMember<IntersectionObservation> WeakValueType;
+    typedef typename WTF::HashTraits<ValueType>::PeekInType PeekInType;
+    struct ValueTypeHash {

[esprehn, 2015/12/12 00:14:14]

hmm, we've never had to do this before for a normal web feature, why do you want
custom hash traits like this? I'd rather you didn't do that.

If we do want to do this move it all to a separate header, not inside the class
def, it obscures all the other code here.

[szager1, 2015/12/16 19:15:35]

Removed the custom hash_traits.

+        static unsigned hash()
+        {
+            return DefaultHash<HashKey>::Hash::hash(HashKey(nullptr, nullptr));
+        }
+        static unsigned hash(PeekInType keyRef)
+        {
+            IntersectionObservation* key = keyRef;
+            if (!key)
+                return hash();
+            return DefaultHash<HashKey>::Hash::hash(HashKey(key->observer(), key->target()));
+        }
+        static bool equal(const PeekInType& aRef, const PeekInType& bRef)
+        {
+            IntersectionObservation* a = aRef;
+            IntersectionObservation* b = bRef;
+            if (!a)
+                return b;
+            if (!b)
+                return true;
+            return a->observer() == b->observer() && a->target() == b->target();
+        }
+        static const bool safeToCompareToEmptyOrDeleted = true;
+    };
+    struct HashKeyTranslator {
+        static unsigned hash(const HashKey& key) { return DefaultHash<HashKey>::Hash::hash(key); }
+        static bool equal(const ValueType& a, const HashKey& b)
+        {

[esprehn, 2015/12/12 00:14:14]

yeah I'd really rather we didn't do this

[szager1, 2015/12/16 19:15:35]

Done.

+            return a && a->observer() == b.first && a->target() == b.second;
+        }
+        static bool equal(const WeakValueType& a, const HashKey& b)
+        {
+            return a && a->observer() == b.first && a->target() == b.second;
+        }
+    };
+    typedef HeapHashSet<ValueType, ValueTypeHash> HashSet;
+    typedef HeapHashSet<WeakValueType, ValueTypeHash> WeakHashSet;
+
+    template<typename HashSetType>
+    static typename HashSetType::iterator find(HashSetType& hashSet, IntersectionObserver* observer, Element* target)

[esprehn, 2015/12/12 00:14:14]

remove this, no templates or statics like this, add methods to your controller
for the specific purposes

[szager1, 2015/12/16 19:15:35]

Done.

+    {
+        return hashSet.template find<HashKeyTranslator, HashKey>(HashKey(observer, target));
+    }
+
+    template<typename HashSetType>
+    static bool contains(HashSetType& hashSet, IntersectionObserver* observer, Element* target)
+    {
+        return hashSet.template contains<HashKeyTranslator, HashKey>(HashKey(observer, target));
+    }
+
+    template<typename HashSetType>
+    static void remove(HashSetType& hashSet, IntersectionObserver* observer, Element* target)

[esprehn, 2015/12/12 00:14:14]

ditto for all of these

[szager1, 2015/12/16 19:15:35]

Done.

+    {
+        hashSet.remove(hashSet.template find<HashKeyTranslator, HashKey>(HashKey(observer, target)));
+    }
+
+    IntersectionObservation(IntersectionObserver&, Element*, bool);

[esprehn, 2015/12/12 00:14:14]

bool needs an argument name, why not an enum? We don't usually do bools like
this, also the Element should be a reference

[szager1, 2015/12/16 19:15:35]

The bool is canReportRootBounds, is there some other idiomatic way to do this? 
The value is computed in IntersectionObserver::observe() because it's needed
there, and I didn't think it made sense to recompute it here.

Changed the Element argument to a reference.

+
+    const IntersectionObserver* observer() const { return m_observer; }
+    IntersectionObserver* observer() { return m_observer; }
+    const Element* target() const { return m_target.get(); }
+    Element* target() { return m_target.get(); }
+    bool isActive() const { return m_active; }
+    void setActive(bool active) { m_active = active; }
+    float lastVisibleRatio() const { return m_lastVisibleRatio; }
+    void setLastVisibleRatio(float ratio) { m_lastVisibleRatio = ratio; }
+    bool canReportRootBounds() const { return m_canReportRootBounds; }
+
+    void computeIntersectionObservations(int);

[esprehn, 2015/12/12 00:14:14]

int needs an argument name, primitive arguments always need variable names
unless the method contains the type in it like setWidth(int)

[szager1, 2015/12/16 19:15:35]

Done.

+    void disconnect();
+
+    DECLARE_TRACE();
+
+private:
+    bool computeGeometry(LayoutRect& targetRect, LayoutRect& rootRect, LayoutRect& intersectionRect);
+
+    Member<IntersectionObserver> m_observer;
+    WeakPtrWillBeWeakMember<Element> m_target;
+    bool m_active;
+    bool m_canReportRootBounds;

[esprehn, 2015/12/12 00:14:14]

I'd swap these with the float, and make them both bitfields so they pack, but I
guess you don't have many observers so it's not a big deal

[ojan, 2015/12/12 01:06:28]

Bikeshed nit: We typically call this m_shouldReportRootBounds.

[szager1, 2015/12/16 19:15:35]

Done.

[szager1, 2015/12/16 19:15:35]

Done.

+    float m_lastVisibleRatio;
+};
+
+} // namespace blink {
+
+#endif // IntersectionObservation_h