IntersectionObserver: use nullptr for implicit root.

IntersectionObserver: use nullptr for implicit root.

If the observer is tracking intersections with the top-level frame
viewport (AKA the "implicit root"), it's not necessary to store a
pointer to the root node on the IntersectionObserver.  Doing so
makes bookkeeping more difficult when DOM modifications happen.
It's also strictly incompatible with OOPIF.

Methods like mapToVisualRectInAncestorSpace and localToAbsolute already
set a precedent by allowing a nullptr "ancestor" argument to indicate
"map all the way to the top frame".  Changing IntersectionObserver to
let nullptr signify the implict root keeps with this convention.

The new trackingDocument() method serves two purposes: it fixes a bug
where only IntersectionObservers registered on the top-level FrameView
run the observation steps (test added); and it is a necessary wart
until OOPIF support lands (https://codereview.chromium.org/2431473003).

Because IntersectionObserver::m_root is a weak pointer, we must track
explicitly whether the IO is tracking the implicit root, to distinguish
that from an IO whose root has been garbage collected; hence the
new m_rootIsImplicit field.

R=ojan@chromium.org,kenrb@chromium.org
BUG=

Committed: https://crrev.com/3520351e84d8d16b06d3f09a109f1296ec620c23
Cr-Commit-Position: refs/heads/master@{#438055}

[szager1, 2016-12-07 23:02:38]

Description was changed from

==========
IntersectionObserver: use nullptr for implicit root.

If the observer is tracking intersections with the top-level frame
viewport (AKA the "implicit root"), it's not necessary to store a
pointer to the root node on the IntersectionObserver.  Doing so
makes bookkeeping more difficult when DOM modifications happen.
It's also strictly incompatible with OOPIF.

Methods like mapToVisualRectInAncestorSpace and localToAbsolute already
set a precedent by allowing a nullptr "ancestor" argument to indicate
"map all the way to the top frame".  Changing IntersectionObserver to
let nullptr signify the implict root keeps with this convention.

The new trackingDocument() method serves two purposes: it fixes a bug
where only IntersectionObservers registered on the top-level FrameView
run the observation steps (test added); and it is a necessary wart
until OOPIF support lands (https://codereview.chromium.org/2431473003).

Because IntersectionObserver::m_root is a weak pointer, we must track
explicitly whether the IO is tracking the implicit root, to distinguish
that from an IO whose root has been garbage collected; hence the
new m_rootIsImplicit field.

R=ojan@chromium.org,kenrb@chromium.org
BUG=
==========

to

==========
IntersectionObserver: use nullptr for implicit root.

If the observer is tracking intersections with the top-level frame
viewport (AKA the "implicit root"), it's not necessary to store a
pointer to the root node on the IntersectionObserver.  Doing so
makes bookkeeping more difficult when DOM modifications happen.
It's also strictly incompatible with OOPIF.

Methods like mapToVisualRectInAncestorSpace and localToAbsolute already
set a precedent by allowing a nullptr "ancestor" argument to indicate
"map all the way to the top frame".  Changing IntersectionObserver to
let nullptr signify the implict root keeps with this convention.

The new trackingDocument() method serves two purposes: it fixes a bug
where only IntersectionObservers registered on the top-level FrameView
run the observation steps (test added); and it is a necessary wart
until OOPIF support lands (https://codereview.chromium.org/2431473003).

Because IntersectionObserver::m_root is a weak pointer, we must track
explicitly whether the IO is tracking the implicit root, to distinguish
that from an IO whose root has been garbage collected; hence the
new m_rootIsImplicit field.

R=ojan@chromium.org,kenrb@chromium.org
BUG=
==========

[szager1, 2016-12-07 23:02:38]

szager@chromium.org changed reviewers:
+ skobes@chromium.org

[szager1, 2016-12-07 23:02:47]

+skobes

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

https://codereview.chromium.org/2553343004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/IntersectionObserver.h (right):

https://codereview.chromium.org/2553343004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/IntersectionObserver.h:66: bool
rootIsImplicit() const { return m_rootIsImplicit; }
Add comments explaining what this means.

https://codereview.chromium.org/2553343004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/IntersectionObserver.h:67: Document&
trackingDocument() const;
The observer does the tracking, right?  So this should be trackedDocument or
documentBeingTracked.

https://codereview.chromium.org/2553343004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/IntersectionObserver.h:99: // Did the
explicit root Element get deleted?
The answer to this question is the inverse of the return value.  Perhaps begin
with "// True unless ..."

https://codereview.chromium.org/2553343004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/IntersectionObserver.h:103:
WeakMember<Element> m_root;
What happens if we want to track a Document in an iframe?

https://codereview.chromium.org/2553343004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/IntersectionObserver.h:112: unsigned
m_initialState : 1;
Why is the type not InitialState?

[szager1, 2016-12-08 19:39:59]

https://codereview.chromium.org/2553343004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/IntersectionObserver.h (right):

https://codereview.chromium.org/2553343004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/IntersectionObserver.h:66: bool
rootIsImplicit() const { return m_rootIsImplicit; }
On 2016/12/08 01:08:56, skobes wrote:
> Add comments explaining what this means.

Done.

https://codereview.chromium.org/2553343004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/IntersectionObserver.h:67: Document&
trackingDocument() const;
On 2016/12/08 01:08:56, skobes wrote:
> The observer does the tracking, right?  So this should be trackedDocument or
> documentBeingTracked.

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.

https://codereview.chromium.org/2553343004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/IntersectionObserver.h:99: // Did the
explicit root Element get deleted?
On 2016/12/08 01:08:56, skobes wrote:
> The answer to this question is the inverse of the return value.  Perhaps begin
> with "// True unless ..."

Done.

https://codereview.chromium.org/2553343004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/IntersectionObserver.h:103:
WeakMember<Element> m_root;
On 2016/12/08 01:08:56, skobes wrote:
> What happens if we want to track a Document in an iframe?

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.

https://codereview.chromium.org/2553343004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/IntersectionObserver.h:112: unsigned
m_initialState : 1;
On 2016/12/08 01:08:56, skobes wrote:
> Why is the type not InitialState?

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]

https://codereview.chromium.org/2553343004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/IntersectionObserver.h (right):

https://codereview.chromium.org/2553343004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/IntersectionObserver.h:103:
WeakMember<Element> m_root;
On 2016/12/08 19:39:58, szager1 wrote:
> On 2016/12/08 01:08:56, skobes wrote:
> > What happens if we want to track a Document in an iframe?
> 
> The only choices are to track an Element, or to track the top-level frame's
> viewport (the "implict root").

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.

https://codereview.chromium.org/2553343004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/IntersectionObserver.h:112: unsigned
m_initialState : 1;
On 2016/12/08 19:39:58, szager1 wrote:
> On 2016/12/08 01:08:56, skobes wrote:
> > Why is the type not InitialState?
> 
> Just for efficient struct packing.  I think the enum values are still useful,
> because they document what the different states mean.

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

  InitialState m_initialState : 1;

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

https://codereview.chromium.org/2553343004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/IntersectionObserver.h (right):

https://codereview.chromium.org/2553343004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/IntersectionObserver.h:103:
WeakMember<Element> m_root;
On 2016/12/08 20:02:43, skobes wrote:
> On 2016/12/08 19:39:58, szager1 wrote:
> > On 2016/12/08 01:08:56, skobes wrote:
> > > What happens if we want to track a Document in an iframe?
> > 
> > The only choices are to track an Element, or to track the top-level frame's
> > viewport (the "implict root").
> 
> 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.

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.

https://codereview.chromium.org/2553343004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/IntersectionObserver.h:112: unsigned
m_initialState : 1;
On 2016/12/08 20:02:43, skobes wrote:
> On 2016/12/08 19:39:58, szager1 wrote:
> > On 2016/12/08 01:08:56, skobes wrote:
> > > Why is the type not InitialState?
> > 
> > Just for efficient struct packing.  I think the enum values are still
useful,
> > because they document what the different states mean.
> 
> C++ allows enum bitfields, so you should be able to do
> 
>   InitialState m_initialState : 1;

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]

https://codereview.chromium.org/2553343004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/IntersectionObserver.h (right):

https://codereview.chromium.org/2553343004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/IntersectionObserver.h:112: unsigned
m_initialState : 1;
On 2016/12/08 20:12:11, szager1 wrote:
> On 2016/12/08 20:02:43, skobes wrote:
> > On 2016/12/08 19:39:58, szager1 wrote:
> > > On 2016/12/08 01:08:56, skobes wrote:
> > > > Why is the type not InitialState?
> > > 
> > > Just for efficient struct packing.  I think the enum values are still
> useful,
> > > because they document what the different states mean.
> > 
> > C++ allows enum bitfields, so you should be able to do
> > 
> >   InitialState m_initialState : 1;
> 
> 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]

"InitialiState" looks like a typo.

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

https://codereview.chromium.org/2553343004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/IntersectionObserver.h (right):

https://codereview.chromium.org/2553343004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/IntersectionObserver.h:112: unsigned
m_initialState : 1;
On 2016/12/08 21:12:21, skobes wrote:
> On 2016/12/08 20:12:11, szager1 wrote:
> > On 2016/12/08 20:02:43, skobes wrote:
> > > On 2016/12/08 19:39:58, szager1 wrote:
> > > > On 2016/12/08 01:08:56, skobes wrote:
> > > > > Why is the type not InitialState?
> > > > 
> > > > Just for efficient struct packing.  I think the enum values are still
> > useful,
> > > > because they document what the different states mean.
> > > 
> > > C++ allows enum bitfields, so you should be able to do
> > > 
> > >   InitialState m_initialState : 1;
> > 
> > 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]
> 
> "InitialiState" looks like a typo.

Same error.

[skobes, 2016-12-08 21:59:22]

On 2016/12/08 21:30:58, szager1 wrote:
>
https://codereview.chromium.org/2553343004/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/dom/IntersectionObserver.h (right):
> 
>
https://codereview.chromium.org/2553343004/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/dom/IntersectionObserver.h:112: unsigned
> m_initialState : 1;
> On 2016/12/08 21:12:21, skobes wrote:
> > On 2016/12/08 20:12:11, szager1 wrote:
> > > On 2016/12/08 20:02:43, skobes wrote:
> > > > On 2016/12/08 19:39:58, szager1 wrote:
> > > > > On 2016/12/08 01:08:56, skobes wrote:
> > > > > > Why is the type not InitialState?
> > > > > 
> > > > > Just for efficient struct packing.  I think the enum values are still
> > > useful,
> > > > > because they document what the different states mean.
> > > > 
> > > > C++ allows enum bitfields, so you should be able to do
> > > > 
> > > >   InitialState m_initialState : 1;
> > > 
> > > 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]
> > 
> > "InitialiState" looks like a typo.
> 
> Same error.

Try an enum class inheriting from unsigned int as described in
https://groups.google.com/a/chromium.org/d/msg/style-dev/TB0Uva6pAus/BrYA0JLH....

[szager1, 2016-12-08 22:10:25]

On 2016/12/08 21:59:22, skobes wrote:
> On 2016/12/08 21:30:58, szager1 wrote:
> >
>
https://codereview.chromium.org/2553343004/diff/40001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/core/dom/IntersectionObserver.h (right):
> > 
> >
>
https://codereview.chromium.org/2553343004/diff/40001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/core/dom/IntersectionObserver.h:112: unsigned
> > m_initialState : 1;
> > On 2016/12/08 21:12:21, skobes wrote:
> > > On 2016/12/08 20:12:11, szager1 wrote:
> > > > On 2016/12/08 20:02:43, skobes wrote:
> > > > > On 2016/12/08 19:39:58, szager1 wrote:
> > > > > > On 2016/12/08 01:08:56, skobes wrote:
> > > > > > > Why is the type not InitialState?
> > > > > > 
> > > > > > Just for efficient struct packing.  I think the enum values are
still
> > > > useful,
> > > > > > because they document what the different states mean.
> > > > > 
> > > > > C++ allows enum bitfields, so you should be able to do
> > > > > 
> > > > >   InitialState m_initialState : 1;
> > > > 
> > > > 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]
> > > 
> > > "InitialiState" looks like a typo.
> > 
> > Same error.
> 
> Try an enum class inheriting from unsigned int as described in
>
https://groups.google.com/a/chromium.org/d/msg/style-dev/TB0Uva6pAus/BrYA0JLH....

Same error.

[skobes, 2016-12-09 01:44:29]

On 2016/12/08 22:10:25, szager1 wrote:
> On 2016/12/08 21:59:22, skobes wrote:
> > On 2016/12/08 21:30:58, szager1 wrote:
> > >
> >
>
https://codereview.chromium.org/2553343004/diff/40001/third_party/WebKit/Sour...
> > > File third_party/WebKit/Source/core/dom/IntersectionObserver.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2553343004/diff/40001/third_party/WebKit/Sour...
> > > third_party/WebKit/Source/core/dom/IntersectionObserver.h:112: unsigned
> > > m_initialState : 1;
> > > On 2016/12/08 21:12:21, skobes wrote:
> > > > On 2016/12/08 20:12:11, szager1 wrote:
> > > > > On 2016/12/08 20:02:43, skobes wrote:
> > > > > > On 2016/12/08 19:39:58, szager1 wrote:
> > > > > > > On 2016/12/08 01:08:56, skobes wrote:
> > > > > > > > Why is the type not InitialState?
> > > > > > > 
> > > > > > > Just for efficient struct packing.  I think the enum values are
> still
> > > > > useful,
> > > > > > > because they document what the different states mean.
> > > > > > 
> > > > > > C++ allows enum bitfields, so you should be able to do
> > > > > > 
> > > > > >   InitialState m_initialState : 1;
> > > > > 
> > > > > 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]
> > > > 
> > > > "InitialiState" looks like a typo.
> > > 
> > > Same error.
> > 
> > Try an enum class inheriting from unsigned int as described in
> >
>
https://groups.google.com/a/chromium.org/d/msg/style-dev/TB0Uva6pAus/BrYA0JLH....
> 
> Same error.

A search for the error message string reveals that this presubmit check is using
a regex to find the keyword "unsigned" in the declaration:

 
https://cs.chromium.org/chromium/src/third_party/WebKit/Tools/Scripts/webkitp...

Anyway the bitfield seems like a premature optimization to me.  It's fragile
since the bit count needs to stay in sync with the number of enum values, and
the potential savings of 7 bits of memory in an IntersectionObserver object is
not worth the loss in type safety and readability.  Let's just declare it as

  InitialState m_initialState;

[kenrb, 2016-12-09 21:01:43]

https://codereview.chromium.org/2553343004/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/IntersectionObservation.cpp (right):

https://codereview.chromium.org/2553343004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/IntersectionObservation.cpp:31:
m_shouldReportRootBounds);
There is something I don't quite understand here. m_observer->root() can now
return nullptr, but IntersectionGeometry::computeGeometry() on the next line
will always return early if it has a null root. Is this intentional? How do we
get intersection calculations in that case?

https://codereview.chromium.org/2553343004/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/IntersectionObserver.h (right):

https://codereview.chromium.org/2553343004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/IntersectionObserver.h:77: LayoutObject*
rootLayoutObject() const;
This does not appear to have any callers.

[szager1, 2016-12-09 21:04:01]

On 2016/12/09 01:44:29, skobes wrote:
> On 2016/12/08 22:10:25, szager1 wrote:
> > On 2016/12/08 21:59:22, skobes wrote:
> > > On 2016/12/08 21:30:58, szager1 wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/2553343004/diff/40001/third_party/WebKit/Sour...
> > > > File third_party/WebKit/Source/core/dom/IntersectionObserver.h (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2553343004/diff/40001/third_party/WebKit/Sour...
> > > > third_party/WebKit/Source/core/dom/IntersectionObserver.h:112: unsigned
> > > > m_initialState : 1;
> > > > On 2016/12/08 21:12:21, skobes wrote:
> > > > > On 2016/12/08 20:12:11, szager1 wrote:
> > > > > > On 2016/12/08 20:02:43, skobes wrote:
> > > > > > > On 2016/12/08 19:39:58, szager1 wrote:
> > > > > > > > On 2016/12/08 01:08:56, skobes wrote:
> > > > > > > > > Why is the type not InitialState?
> > > > > > > > 
> > > > > > > > Just for efficient struct packing.  I think the enum values are
> > still
> > > > > > useful,
> > > > > > > > because they document what the different states mean.
> > > > > > > 
> > > > > > > C++ allows enum bitfields, so you should be able to do
> > > > > > > 
> > > > > > >   InitialState m_initialState : 1;
> > > > > > 
> > > > > > 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]
> > > > > 
> > > > > "InitialiState" looks like a typo.
> > > > 
> > > > Same error.
> > > 
> > > Try an enum class inheriting from unsigned int as described in
> > >
> >
>
https://groups.google.com/a/chromium.org/d/msg/style-dev/TB0Uva6pAus/BrYA0JLH....
> > 
> > Same error.
> 
> A search for the error message string reveals that this presubmit check is
using
> a regex to find the keyword "unsigned" in the declaration:
> 
>  
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Tools/Scripts/webkitp...
> 
> Anyway the bitfield seems like a premature optimization to me.  It's fragile
> since the bit count needs to stay in sync with the number of enum values, and
> the potential savings of 7 bits of memory in an IntersectionObserver object is
> not worth the loss in type safety and readability.  Let's just declare it as
> 
>   InitialState m_initialState;

How about a compromise:

unsigned m_initialState : 7;

That won't use any extra space since all of the bit fields together will add up
to 8 bits; and it leaves room for up to 126 additional values for what is
essentially a boolean flag.  There is precedent for this, even within
IntersectionObserver:

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Inter...

[skobes, 2016-12-09 22:22:20]

On 2016/12/09 21:04:01, szager1 wrote:
> unsigned m_initialState : 7;

I can live with it, but please add a comment stating what the real type is.

[szager1, 2016-12-10 01:50:07]

On 2016/12/09 22:22:20, skobes wrote:
> On 2016/12/09 21:04:01, szager1 wrote:
> > unsigned m_initialState : 7;
> 
> I can live with it, but please add a comment stating what the real type is.

Done.

[skobes, 2016-12-10 02:03:14]

lgtm once kenrb's concern is addressed

[szager1, 2016-12-10 20:51:18]

https://codereview.chromium.org/2553343004/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/IntersectionObservation.cpp (right):

https://codereview.chromium.org/2553343004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/IntersectionObservation.cpp:31:
m_shouldReportRootBounds);
On 2016/12/09 21:01:43, kenrb wrote:
> There is something I don't quite understand here. m_observer->root() can now
> return nullptr, but IntersectionGeometry::computeGeometry() on the next line
> will always return early if it has a null root. Is this intentional? How do we
> get intersection calculations in that case?

This patch depends on another patch (https://codereview.chromium.org/2553103004)
that fixes IntersectionGeometry::computeGeometry() to treat null as the implicit
root.

https://codereview.chromium.org/2553343004/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/IntersectionObserver.h (right):

https://codereview.chromium.org/2553343004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/IntersectionObserver.h:77: LayoutObject*
rootLayoutObject() const;
On 2016/12/09 21:01:43, kenrb wrote:
> This does not appear to have any callers.

Removed.

[kenrb, 2016-12-11 02:59:42]

Thanks, lgtm. I'll update the OOPIF IntersectionObserver CL as soon as this
lands.

[Z_DONOTUSE, 2016-12-11 03:15:15]

On 2016/12/09 at 22:22:20, skobes wrote:
> On 2016/12/09 21:04:01, szager1 wrote:
> > unsigned m_initialState : 7;
> 
> I can live with it, but please add a comment stating what the real type is.

Hah. This compromise doesn't match what we do anywhere else in the code. It's
quite common for us to put enums in bitfields. How about going back to ": 1",
but adding in something like:

  enum InitialState {
    kAuto = 0,
    kHidden = 1,
    kDoNotUseMax = 2,
  };

  // InitialState is packed tightly into a single bit.
  // If making it bigger, increase the number of bits we store.
  static_assert(InitialState::kDoNotUseMax == 2);

Alternately, we can just rely on the fact that the new code that increases the
number of states will presumably have tests that rely on actually using the new
state.

[szager1, 2016-12-11 08:45:21]

On 2016/12/11 03:15:15, Z_DONOTUSE wrote:
> On 2016/12/09 at 22:22:20, skobes wrote:
> > On 2016/12/09 21:04:01, szager1 wrote:
> > > unsigned m_initialState : 7;
> > 
> > I can live with it, but please add a comment stating what the real type is.
> 
> Hah. This compromise doesn't match what we do anywhere else in the code. It's
> quite common for us to put enums in bitfields. How about going back to ": 1",
> but adding in something like:
> 
>   enum InitialState {
>     kAuto = 0,
>     kHidden = 1,
>     kDoNotUseMax = 2,
>   };
> 
>   // InitialState is packed tightly into a single bit.
>   // If making it bigger, increase the number of bits we store.
>   static_assert(InitialState::kDoNotUseMax == 2);

Done.

[szager1, 2016-12-13 00:35:34]

The CQ bit was checked by szager@chromium.org

[szager1, 2016-12-13 00:35:34]

The patchset sent to the CQ was uploaded after l-g-t-m from skobes@chromium.org,
kenrb@chromium.org
Link to the patchset: https://codereview.chromium.org/2553343004/#ps160001
(title: "rebase")

[commit-bot: I haz the power, 2016-12-13 00:37:04]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-13 04:17:13]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1481589334384550,
"parent_rev": "22700454313e4287b9bb31d2a4dad7dd8366105e", "commit_rev":
"80fec3d2c09e40d8dc6348f0b81cae3fc43f3384"}

[commit-bot: I haz the power, 2016-12-13 04:18:03]

Description was changed from

==========
IntersectionObserver: use nullptr for implicit root.

If the observer is tracking intersections with the top-level frame
viewport (AKA the "implicit root"), it's not necessary to store a
pointer to the root node on the IntersectionObserver.  Doing so
makes bookkeeping more difficult when DOM modifications happen.
It's also strictly incompatible with OOPIF.

Methods like mapToVisualRectInAncestorSpace and localToAbsolute already
set a precedent by allowing a nullptr "ancestor" argument to indicate
"map all the way to the top frame".  Changing IntersectionObserver to
let nullptr signify the implict root keeps with this convention.

The new trackingDocument() method serves two purposes: it fixes a bug
where only IntersectionObservers registered on the top-level FrameView
run the observation steps (test added); and it is a necessary wart
until OOPIF support lands (https://codereview.chromium.org/2431473003).

Because IntersectionObserver::m_root is a weak pointer, we must track
explicitly whether the IO is tracking the implicit root, to distinguish
that from an IO whose root has been garbage collected; hence the
new m_rootIsImplicit field.

R=ojan@chromium.org,kenrb@chromium.org
BUG=
==========

to

==========
IntersectionObserver: use nullptr for implicit root.

If the observer is tracking intersections with the top-level frame
viewport (AKA the "implicit root"), it's not necessary to store a
pointer to the root node on the IntersectionObserver.  Doing so
makes bookkeeping more difficult when DOM modifications happen.
It's also strictly incompatible with OOPIF.

Methods like mapToVisualRectInAncestorSpace and localToAbsolute already
set a precedent by allowing a nullptr "ancestor" argument to indicate
"map all the way to the top frame".  Changing IntersectionObserver to
let nullptr signify the implict root keeps with this convention.

The new trackingDocument() method serves two purposes: it fixes a bug
where only IntersectionObservers registered on the top-level FrameView
run the observation steps (test added); and it is a necessary wart
until OOPIF support lands (https://codereview.chromium.org/2431473003).

Because IntersectionObserver::m_root is a weak pointer, we must track
explicitly whether the IO is tracking the implicit root, to distinguish
that from an IO whose root has been garbage collected; hence the
new m_rootIsImplicit field.

R=ojan@chromium.org,kenrb@chromium.org
BUG=

Review-Url: https://codereview.chromium.org/2553343004
==========

[commit-bot: I haz the power, 2016-12-13 04:18:04]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-12-13 04:21:09]

Description was changed from

==========
IntersectionObserver: use nullptr for implicit root.

If the observer is tracking intersections with the top-level frame
viewport (AKA the "implicit root"), it's not necessary to store a
pointer to the root node on the IntersectionObserver.  Doing so
makes bookkeeping more difficult when DOM modifications happen.
It's also strictly incompatible with OOPIF.

Methods like mapToVisualRectInAncestorSpace and localToAbsolute already
set a precedent by allowing a nullptr "ancestor" argument to indicate
"map all the way to the top frame".  Changing IntersectionObserver to
let nullptr signify the implict root keeps with this convention.

The new trackingDocument() method serves two purposes: it fixes a bug
where only IntersectionObservers registered on the top-level FrameView
run the observation steps (test added); and it is a necessary wart
until OOPIF support lands (https://codereview.chromium.org/2431473003).

Because IntersectionObserver::m_root is a weak pointer, we must track
explicitly whether the IO is tracking the implicit root, to distinguish
that from an IO whose root has been garbage collected; hence the
new m_rootIsImplicit field.

R=ojan@chromium.org,kenrb@chromium.org
BUG=

Review-Url: https://codereview.chromium.org/2553343004
==========

to

==========
IntersectionObserver: use nullptr for implicit root.

If the observer is tracking intersections with the top-level frame
viewport (AKA the "implicit root"), it's not necessary to store a
pointer to the root node on the IntersectionObserver.  Doing so
makes bookkeeping more difficult when DOM modifications happen.
It's also strictly incompatible with OOPIF.

Methods like mapToVisualRectInAncestorSpace and localToAbsolute already
set a precedent by allowing a nullptr "ancestor" argument to indicate
"map all the way to the top frame".  Changing IntersectionObserver to
let nullptr signify the implict root keeps with this convention.

The new trackingDocument() method serves two purposes: it fixes a bug
where only IntersectionObservers registered on the top-level FrameView
run the observation steps (test added); and it is a necessary wart
until OOPIF support lands (https://codereview.chromium.org/2431473003).

Because IntersectionObserver::m_root is a weak pointer, we must track
explicitly whether the IO is tracking the implicit root, to distinguish
that from an IO whose root has been garbage collected; hence the
new m_rootIsImplicit field.

R=ojan@chromium.org,kenrb@chromium.org
BUG=

Committed: https://crrev.com/3520351e84d8d16b06d3f09a109f1296ec620c23
Cr-Commit-Position: refs/heads/master@{#438055}
==========

[commit-bot: I haz the power, 2016-12-13 04:21:10]

Patchset 9 (id:??) landed as
https://crrev.com/3520351e84d8d16b06d3f09a109f1296ec620c23
Cr-Commit-Position: refs/heads/master@{#438055}