IntersectionObserver: use nullptr for implicit root.

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

Index: third_party/WebKit/Source/core/dom/IntersectionObserver.h
diff --git a/third_party/WebKit/Source/core/dom/IntersectionObserver.h b/third_party/WebKit/Source/core/dom/IntersectionObserver.h
index cb3913595144df13c8ef9e14cccd5fa577744997..4cd0b77b2d4ea3dc005eed7388f0bdc1e8653dd5 100644
--- a/third_party/WebKit/Source/core/dom/IntersectionObserver.h
+++ b/third_party/WebKit/Source/core/dom/IntersectionObserver.h
@@ -21,6 +21,7 @@ class Element;
 class ExceptionState;
 class IntersectionObserverCallback;
 class IntersectionObserverInit;
+class LayoutObject;
 
 class CORE_EXPORT IntersectionObserver final
     : public GarbageCollectedFinalized<IntersectionObserver>,
@@ -35,10 +36,10 @@ class CORE_EXPORT IntersectionObserver final
   // Defines the assumed initial state of the observed element. If the actual
   // state is the same as the initial state, then no observation will be
   // delivered. kAuto means the initial observation will always get sent.
-  enum class InitialState {
+  enum InitialState {
     // TODO(skyostil): Add support for kVisible.
-    kAuto,
-    kHidden,
+    kAuto = 0,
+    kHidden = 1,
   };
 
   static IntersectionObserver* create(const IntersectionObserverInit&,
@@ -58,11 +59,13 @@ class CORE_EXPORT IntersectionObserver final
   HeapVector<Member<IntersectionObserverEntry>> takeRecords(ExceptionState&);
 
   // API attributes.
-  Element* root() const;
+  Element* root() const { return m_root.get(); }
   String rootMargin() const;
   const Vector<float>& thresholds() const { return m_thresholds; }
 
-  Node* rootNode() const { return m_root.get(); }
+  bool rootIsImplicit() const { return m_rootIsImplicit; }

[skobes, 2016/12/08 01:08:56]

Add comments explaining what this means.

[szager1, 2016/12/08 19:39:58]

Done.

+  Document& trackingDocument() const;

[skobes, 2016/12/08 01:08:56]

The observer does the tracking, right?  So this should be trackedDocument or
documentBeingTracked.

[szager1, 2016/12/08 19:39:58]

I added a comment to clarify this: "this is the document which is responsible
for running computeIntersectionObservations at frame generation time."  The
tracking document is not involved in the actual intersection calculation.

+  LayoutObject* rootLayoutObject() const;
   const Length& topMargin() const { return m_topMargin; }
   const Length& rightMargin() const { return m_rightMargin; }
   const Length& bottomMargin() const { return m_bottomMargin; }
@@ -88,13 +91,16 @@ class CORE_EXPORT IntersectionObserver final
 
  private:
   explicit IntersectionObserver(IntersectionObserverCallback&,
-                                Node&,
+                                Element*,
                                 const Vector<Length>& rootMargin,
                                 const Vector<float>& thresholds);
   void clearWeakMembers(Visitor*);
 
+  // Did the explicit root Element get deleted?

[skobes, 2016/12/08 01:08:56]

The answer to this question is the inverse of the return value.  Perhaps begin
with "// True unless ..."

[szager1, 2016/12/08 19:39:58]

Done.

+  bool rootIsValid() const;
+
   Member<IntersectionObserverCallback> m_callback;
-  WeakMember<Node> m_root;
+  WeakMember<Element> m_root;

[skobes, 2016/12/08 01:08:56]

What happens if we want to track a Document in an iframe?

[szager1, 2016/12/08 19:39:58]

The only choices are to track an Element, or to track the top-level frame's
viewport (the "implict root").  Previously, when tracking the implicit root, we
would keep a reference to the top-level frame's Document node in m_root; but
that breaks down in the OOPIF case, because the top-level frame might be in a
different process.

[skobes, 2016/12/08 20:02:43]

So which element is m_root set to if we want to track an iframe's viewport?  Is
it its document.scrollingElement, the owning <iframe> element in the parent
document, or something else?

We're losing the ability for m_root to be a Document by making it an Element so
I'm just trying to understand how that use case is satisfied.

[szager1, 2016/12/08 20:12:11]

If you read through the code paths that get to the IntersectionObserver
constructor, the only way that m_root could be a Document is the implicit root
case.  This restriction is part of the IntersectionObserver spec:

"An IntersectionObserver with a root Element can observe any target Element that
is a descendant of the root in the containing block chain.

An IntersectionObserver with no root Element will automatically observe
intersections with the implicit root..."

If you want to track the intersection between a target in an iframe and that
iframe's viewport, then you would using iframe.contentDocument.scrollingElement
as the root.

   HeapLinkedHashSet<WeakMember<IntersectionObservation>> m_observations;
   HeapVector<Member<IntersectionObserverEntry>> m_entries;
   Vector<float> m_thresholds;
@@ -102,7 +108,8 @@ class CORE_EXPORT IntersectionObserver final
   Length m_rightMargin;
   Length m_bottomMargin;
   Length m_leftMargin;
-  InitialState m_initialState;
+  unsigned m_rootIsImplicit : 1;
+  unsigned m_initialState : 1;

[skobes, 2016/12/08 01:08:56]

Why is the type not InitialState?

[szager1, 2016/12/08 19:39:58]

Just for efficient struct packing.  I think the enum values are still useful,
because they document what the different states mean.

[skobes, 2016/12/08 20:02:43]

C++ allows enum bitfields, so you should be able to do

  InitialState m_initialState : 1;

[szager1, 2016/12/08 20:12:11]

That gives a presubmit error:

$ git cl uplodad
Running presubmit upload checks ...

** Presubmit ERRORS **
check-webkit-style failed
  third_party/WebKit/Source/core/dom/IntersectionObserver.h:122:  Member
m_initialState of class CORE_EXPORT defined as a bitfield of type InitialiState.
Please declare all bitfields as unsigned.  [runtime/bitfields] [4]

[skobes, 2016/12/08 21:12:21]

"InitialiState" looks like a typo.

[szager1, 2016/12/08 21:30:58]

Same error.

 };
 
 }  // namespace blink