IntersectionObserver: second cut.

third_party/WebKit/Source/core/dom/IntersectionObserver.cpp

+// 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 "config.h"
+#include "core/dom/IntersectionObserver.h"
+
+#include "bindings/core/v8/ExceptionState.h"
+#include "core/css/parser/CSSParserTokenRange.h"
+#include "core/css/parser/CSSTokenizer.h"
+#include "core/dom/ExceptionCode.h"
+#include "core/dom/ExecutionContext.h"
+#include "core/dom/IntersectionObserverCallback.h"
+#include "core/dom/IntersectionObserverEntry.h"
+#include "core/dom/IntersectionObserverInit.h"
+#include "core/dom/IntersectionObserverRegistry.h"
+#include "core/html/HTMLFrameOwnerElement.h"
+#include "platform/Timer.h"
+#include "wtf/MainThread.h"
+#include <algorithm>
+
+namespace blink {
+
+IntersectionObserver* IntersectionObserver::create(const IntersectionObserverInit& observerInit, IntersectionObserverCallback* callback, ExceptionState& exceptionState)
+{
+    ASSERT(isMainThread());

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

no need for thread checks, the DOM is not thread safe, all the code is covered
in this

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

Fixed.

+
+    RefPtrWillBeRawPtr<Element> root = observerInit.root();
+    if (!root) {
+        ASSERT(callback);
+        ExecutionContext* context = callback->executionContext();
+        if (context->isDocument()) {

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

can we actually get here with an execution context that isn't a document? I
don't think we can, this can just be toDocument() I think and we'll crash if the
type was wrong

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

Changed to ASSERT(context->isDocument());

+            Frame* mainFrame = static_cast<Document*>(context)->frame()->tree().top();

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

toDocument, never static_cast DOM types, it misses the type checks

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

Done.

+            if (mainFrame && mainFrame->isLocalFrame())
+                root = toLocalFrame(mainFrame)->document()->documentElement();

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

documentElement can be null, does the spec say that throws? Normally the spec
would use the ICB of the document then

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

If there's no root specified, then the root should be the Document, not the
documentElement I think. Which means that they type needs to be Node, not
Element.

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

That's going to make the code a lot grosser.  Currently, the code throws an
exception if the document element is null.  Under what circumstances would the
document element be null, and do we have to allow observers to be created in
those circumstances?

+        }
+    }
+    if (!root) {
+        exceptionState.throwDOMException(HierarchyRequestError, "Unable to get root element in main frame to track.");
+        return nullptr;
+    }
+
+    Vector<Length> rootMargin;
+    if (observerInit.hasRootMargin()) {
+        CSSTokenizer::Scope tokenizerScope(observerInit.rootMargin());

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

move this into a static (or anon namespace) parse* method

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

Done.

+        CSSParserTokenRange tokenRange = tokenizerScope.tokenRange();
+        while (rootMargin.size() < 5 && tokenRange.peek().type() != EOFToken && !exceptionState.hadException()) {
+            const CSSParserToken& token = tokenRange.consumeIncludingWhitespace();
+            switch (token.type()) {
+            case NumberToken:
+                rootMargin.append(Length(static_cast<int>(floor(token.numericValue())), Fixed));
+                break;
+            case PercentageToken:
+                rootMargin.append(Length(token.numericValue(), Percent));
+                break;
+            case DimensionToken:
+                switch (token.unitType()) {
+                case CSSPrimitiveValue::UnitType::Number:

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

Is this Number thing correct? In regular CSS, you can only use Number inside of
quirksmode with 0 as a special value in standards mode. We should match CSS
here.

Is it not possible to share some of this code with the margin parser that CSS
uses?

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

This is very unfamiliar territory for me, but the guidance I received was to
take this approach.  I removed the NumberToken and UnitType::Number cases.

+                case CSSPrimitiveValue::UnitType::Pixels:
+                    rootMargin.append(Length(static_cast<int>(floor(token.numericValue())), Fixed));
+                    break;
+                case CSSPrimitiveValue::UnitType::Percentage:
+                    rootMargin.append(Length(token.numericValue(), Percent));
+                    break;
+                default:
+                    exceptionState.throwTypeError("rootMargin must be specified in pixels or percent.");
+                }
+                break;
+            default:
+                exceptionState.throwTypeError("rootMargin must be specified in pixels or percent.");
+            }
+        }
+    }
+    if (exceptionState.hadException())
+        return nullptr;
+
+    Vector<float> thresholds;
+    if (observerInit.hasThreshold()) {
+        const DoubleOrDoubleArray& thresholdParam = observerInit.threshold();
+        if (thresholdParam.isDouble()) {
+            thresholds.append(static_cast<float>(thresholdParam.getAsDouble()));
+        } else {
+            for (auto thresholdValue: thresholdParam.getAsDoubleArray())

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

ditto

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

Done.

+                thresholds.append(static_cast<float>(thresholdValue));
+        }
+    } else {
+        thresholds.append(0);
+    }
+    for (auto thresholdValue: thresholds) {

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

space before :

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

Done.

+        if (thresholdValue < 0.0 || thresholdValue > 1.0) {
+            exceptionState.throwTypeError("Threshold values must be between 0 and 1");
+            break;
+        }
+    }
+    std::sort(thresholds.begin(), thresholds.end());
+    if (exceptionState.hadException())
+        return nullptr;
+
+    return new IntersectionObserver(callback, root.get(), rootMargin, thresholds);

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

root can't be null, so this should be *root

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

Done.

+}
+
+IntersectionObserver::IntersectionObserver(IntersectionObserverCallback* callback, Element* root, const Vector<Length>& rootMargin, const Vector<float>& thresholds)
+    : m_callback(callback)
+    , m_root(root->createWeakPtr())
+    , m_observations(new IntersectionObservation::WeakHashSet())
+    , m_entries(new IntersectionObserverEntryVector())

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

write types, why are the sets heap allocated though? just put them as members

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

Done.

+    , m_thresholds(thresholds)
+{
+    switch (rootMargin.size()) {

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

what happens if the size is > 4? Shouldn't we throw or assert?

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

I figured we would just ignore extra junk at the end, is it better to throw?

+    case 0:
+        break;
+    case 1:
+        m_topMargin = m_rightMargin = m_bottomMargin = m_leftMargin = rootMargin[0];
+        break;
+    case 2:
+        m_topMargin = m_bottomMargin = rootMargin[0];
+        m_rightMargin = m_leftMargin = rootMargin[1];
+        break;
+    case 3:
+        m_topMargin = rootMargin[0];
+        m_rightMargin = m_leftMargin = rootMargin[1];
+        m_bottomMargin = rootMargin[2];
+        break;
+    default:
+        m_topMargin = rootMargin[0];
+        m_rightMargin = rootMargin[1];
+        m_bottomMargin = rootMargin[2];
+        m_leftMargin = rootMargin[3];

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

we could be clever and I think you can actually express this with mod math in
the index? You don't need to do that though

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

Acknowledged.

+        break;
+    }
+    root->document().intersectionObserverRegistry()->addTrackedObserver(*this);
+}
+
+bool IntersectionObserver::isDescendantOfRoot(Element* target) const

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

reference

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

Same comment as header: since this method assigns to target and null-checks it,
I would argue it makes more sense to leave it as a pointer.

+{
+    Element* rootElement = m_root.get();
+    if (!rootElement)
+        return false;
+
+    if (target == rootElement || !target->inDocument() || !rootElement->inDocument())

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

multiple return statements

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

Done.

+        return false;
+    LocalFrame* rootFrame = rootElement->document().frame();
+    LocalFrame* targetFrame = target->document().frame();
+    if (!rootFrame || !targetFrame || !targetFrame->tree().isDescendantOf(rootFrame))
+        return false;
+    while (targetFrame != rootFrame) {

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

why do you need this loop if you have the
targetFrame->tree().isDescendantOf(rootFrame) check? What are you looking for
here?

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

I need to make sure that the root element is an ancestor of the iframe element
whose document contains the target.

+        FrameOwner* targetOwner = targetFrame->owner();
+        if (!targetOwner || !targetOwner->isLocal())
+            return false;
+        target = toHTMLFrameOwnerElement(targetOwner);

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

use ownerElement() and loop through the document ptrs, that makes this loop tiny

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

Done.

+        ASSERT(target);

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

this isn't possible, you null checked targetOwner above, then this is just a
cast, no reason to assert.

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

Fixed.

+        targetFrame = target->document().frame();
+        if (!targetFrame)

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

normally you'd loop through document().ownerElement() pointers, and not use 
frame() or need to cast like this.

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

Done.

+            return false;
+    }
+    return target->isDescendantOf(rootElement);
+}
+
+void IntersectionObserver::observe(Element* target, ExceptionState& exceptionState)
+{
+    checkRootAndDetachIfNecessary();
+    if (!m_root)
+        exceptionState.throwDOMException(HierarchyRequestError, "Invalid observer: root element or containing document has been deleted.");
+    else if (!target)
+        exceptionState.throwTypeError("Observation target must be an element.");
+    else if (m_root.get() == target)
+        exceptionState.throwDOMException(HierarchyRequestError, "Cannot use the same element for root and target.");

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

Is this not covered by the isDescendantOfRoot check?

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

I thought it might be nice to call this error out specifically, but I don't feel
strongly about it.

+    else if (!isDescendantOfRoot(target))
+        exceptionState.throwDOMException(HierarchyRequestError, "Observed element must be a descendant of the observer's root element.");
+    if (exceptionState.hadException())

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

we'd normally do each one as a separate block with a return.

if (!m_root) {
  exceptionState.throwDOMException(HierarchyRequestError, ...);
  return;
}
if (...) {
  exceptionState.throwDOMException(...);
  return;
}

instead of checking at the end like this

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

Done.

+        return;
+
+    bool canReportRootBounds = target->document().frame()->securityContext()->securityOrigin()->canAccess(root()->document().frame()->securityContext()->securityOrigin());
+    if (!canReportRootBounds && hasPercentMargin()) {
+        exceptionState.throwDOMException(HierarchyRequestError, "Cannot observe a cross-origin target because the observer has a root margin value specified as a percent.");
+        return;
+    }
+
+    if (IntersectionObservation::contains(*m_observations, this, target))
+        return;
+
+    IntersectionObservation* observation = new IntersectionObservation(*this, target, canReportRootBounds);
+    m_observations->add(observation);
+}
+
+void IntersectionObserver::unobserve(Element* target, ExceptionState&)
+{
+    checkRootAndDetachIfNecessary();
+    if (!target)
+        return;
+    IntersectionObservation::WeakHashSet::iterator observationIterator = IntersectionObservation::find(*m_observations, this, target);

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

auto for the type here, also use the controller, not these static methods.

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

Done.

+    // TODO: unobserve callback
+    if (observationIterator != m_observations->end())
+        (*observationIterator)->disconnect();
+}
+
+void IntersectionObserver::computeIntersectionObservations(int timestamp)

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

double?

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

Fixed.

+{
+    checkRootAndDetachIfNecessary();
+    if (!m_root)
+        return;
+    for (auto& observation: *m_observations)

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

space

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

Done.

+        observation->computeIntersectionObservations(timestamp);
+}
+
+void IntersectionObserver::disconnect(IntersectionObservation& observation)
+{
+    m_observations->remove(&observation);
+}
+
+void IntersectionObserver::disconnect()
+{
+    checkRootAndDetachIfNecessary();
+    HeapVector<Member<IntersectionObservation>> toDisconnect;
+    for (auto& observation: *m_observations)

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

missing space before colon in all your loops

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

Fixed.

+        toDisconnect.append(observation);
+    for (auto& observation: toDisconnect)

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

itto

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

Done.

+        observation->disconnect();
+    ASSERT(!m_observations->size());

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

->isEmpty()

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

Done.

+}
+
+IntersectionObserverEntryVector IntersectionObserver::takeRecords()

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

put type here

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

Done.

+{
+    checkRootAndDetachIfNecessary();
+    IntersectionObserverEntryVector entries;
+    entries.swap(*m_entries);
+    return entries;
+}
+
+void IntersectionObserver::enqueueIntersectionObserverEntry(IntersectionObserverEntry* entry)

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

reference

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

Done.

+{
+    ASSERT(isMainThread());

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

remove

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

Done.

+    m_entries->append(entry);
+    static_cast<Document*>(m_callback->executionContext())->intersectionObserverRegistry()->scheduleIntersectionObserverForDelivery(*this);

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

toDocument()

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

Done.

+}
+
+static int computeMargin(const Length& length, LayoutUnit referenceLength)
+{
+    if (length.type() == Percent)
+        return static_cast<int>(referenceLength.toFloat() * length.percent() / 100.);

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

.0

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

Done.

+    return length.intValue();
+}
+
+void IntersectionObserver::applyRootMargin(LayoutRect& rect) const
+{
+    int topMargin = computeMargin(m_topMargin, rect.height());
+    int rightMargin = computeMargin(m_rightMargin, rect.width());
+    int bottomMargin = computeMargin(m_bottomMargin, rect.height());
+    int leftMargin = computeMargin(m_leftMargin, rect.width());
+
+    rect.setX(rect.x() - leftMargin);
+    rect.setWidth(rect.width() + leftMargin + rightMargin);
+    rect.setY(rect.y() - topMargin);
+    rect.setHeight(rect.height() + topMargin + bottomMargin);
+}
+
+size_t IntersectionObserver::firstThresholdGreaterThan(float ratio) const
+{
+    size_t result = 0;
+    size_t max = m_thresholds.size();
+    while (result < max && m_thresholds[result] < ratio)

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

no reason to cache the max on the stack like that

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

Done.

+        ++result;
+    return result;
+}
+
+bool IntersectionObserver::shouldBeSuspended() const
+{
+    return m_callback->executionContext() && m_callback->executionContext()->activeDOMObjectsAreSuspended();
+}
+
+void IntersectionObserver::deliver()
+{
+    checkRootAndDetachIfNecessary();
+
+    ASSERT(!shouldBeSuspended());
+
+    if (m_entries->isEmpty())

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

why is m_entries a pointer and not just a member?

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

Fixed.

+        return;
+
+    IntersectionObserverEntryVector entries;
+    entries.swap(*m_entries);
+    m_callback->handleEvent(entries, this);

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

*this

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

Done.

+}
+
+void IntersectionObserver::setActive(bool active)
+{
+    checkRootAndDetachIfNecessary();
+    for (auto& observation: *m_observations)

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

space

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

Done.

+        observation->setActive(m_root && active && isDescendantOfRoot(observation->target()));
+}
+
+bool IntersectionObserver::hasPercentMargin() const
+{
+    return m_topMargin.type() == Percent || m_rightMargin.type() == Percent || m_bottomMargin.type() == Percent || m_leftMargin.type() == Percent;
+}
+
+void IntersectionObserver::checkRootAndDetachIfNecessary()
+{
+    if (m_root)
+        return;
+    m_callback.clear();
+    HeapVector<Member<IntersectionObservation>> toDisconnect;
+    for (auto& observation: *m_observations)

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

space

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

Done.

+        toDisconnect.append(observation);
+    for (auto& observation: toDisconnect)

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

space

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

Done.

+        observation->disconnect();
+    ASSERT(!m_observations->size());

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

isEmpty()

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

Done.

+    // TODO: should we deliver pending notifications?
+    m_entries.clear();
+}
+
+DEFINE_TRACE(IntersectionObserver)
+{
+    visitor->trace(m_callback);
+    visitor->trace(m_root);
+    visitor->trace(m_observations);
+    visitor->trace(m_entries);
+}
+
+} // namespace blink