Refactor IntersectionGeometry class and move it to core/layout.

Refactor IntersectionGeometry class and move it to core/layout.

IntersectionGeometry is an ephemeral object, so make it stack
allocated, let it use raw pointers to LayoutObject, and move it to
core/layout.

Also, be explicit about the implicit root by introducing
rootIsImplicit.

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

Committed: https://crrev.com/80f26da6153e051225f68c526979bf39f88472a3
Cr-Commit-Position: refs/heads/master@{#437949}

[xjz, 2016-12-07 01:01:14]

https://codereview.chromium.org/2553103004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/IntersectionObservation.h (right):

https://codereview.chromium.org/2553103004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/IntersectionObservation.h:21: bool
shouldReportRootBounds);
ooc, I did this bool to enum change as suggested by ojan@. Any reason to revert
it?

https://codereview.chromium.org/2553103004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/IntersectionGeometry.cpp (right):

https://codereview.chromium.org/2553103004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/IntersectionGeometry.cpp:3: // found in
the LICENSE file.
Can you adjust the git similarity setting when upload the patch, so that we can
see the diff? `git cl upload --similarity=30` might be good. :)

https://codereview.chromium.org/2553103004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/IntersectionGeometry.h (right):

https://codereview.chromium.org/2553103004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/IntersectionGeometry.h:49: bool
shouldReportRootBounds() const { return m_shouldReportRootBounds; }
This seems is not used anywhere in this CL. Maybe delete it?

https://codereview.chromium.org/2553103004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/IntersectionGeometry.h:53: bool isValid()
const { return m_isValid; }
All these added APIs seems are only used by IntersectionGeometry itself. Can
they be private?

https://codereview.chromium.org/2553103004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/IntersectionGeometry.h:79: unsigned
m_shouldReportRootBounds : 1;
const?

https://codereview.chromium.org/2553103004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/IntersectionGeometry.h:80: unsigned
m_rootIsImplicit : 1;
ditto: const

https://codereview.chromium.org/2553103004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/IntersectionGeometry.h:82: unsigned
m_isValid : 1;
ditto: const

[szager1, 2016-12-07 01:30:49]

https://codereview.chromium.org/2553103004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/IntersectionObservation.h (right):

https://codereview.chromium.org/2553103004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/IntersectionObservation.h:21: bool
shouldReportRootBounds);
On 2016/12/07 01:01:14, xjz wrote:
> ooc, I did this bool to enum change as suggested by ojan@. Any reason to
revert
> it? 

I should talk to Ojan about that.  My reasons:

- Fewer files need to #include "core/layout/IntersectionGeometry."
- Using an enum for bool values is, IMO, the worst kind of syntactic sugar.

Maybe Ojan was thinking that we would want to support other values in the
future?  I can't think of what they might be.

https://codereview.chromium.org/2553103004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/IntersectionGeometry.cpp (right):

https://codereview.chromium.org/2553103004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/IntersectionGeometry.cpp:3: // found in
the LICENSE file.
On 2016/12/07 01:01:14, xjz wrote:
> Can you adjust the git similarity setting when upload the patch, so that we
can
> see the diff? `git cl upload --similarity=30` might be good. :)

Will do.

https://codereview.chromium.org/2553103004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/IntersectionGeometry.h (right):

https://codereview.chromium.org/2553103004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/IntersectionGeometry.h:49: bool
shouldReportRootBounds() const { return m_shouldReportRootBounds; }
On 2016/12/07 01:01:14, xjz wrote:
> This seems is not used anywhere in this CL. Maybe delete it?

There is one direct usage of the member variable; I switched it to use the
accessor method, and made the method private.

https://codereview.chromium.org/2553103004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/IntersectionGeometry.h:53: bool isValid()
const { return m_isValid; }
On 2016/12/07 01:01:14, xjz wrote:
> All these added APIs seems are only used by IntersectionGeometry itself. Can
> they be private?

Done.

https://codereview.chromium.org/2553103004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/IntersectionGeometry.h:79: unsigned
m_shouldReportRootBounds : 1;
On 2016/12/07 01:01:14, xjz wrote:
> const?

Done.

https://codereview.chromium.org/2553103004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/IntersectionGeometry.h:80: unsigned
m_rootIsImplicit : 1;
On 2016/12/07 01:01:14, xjz wrote:
> ditto: const

Done.

https://codereview.chromium.org/2553103004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/IntersectionGeometry.h:82: unsigned
m_isValid : 1;
On 2016/12/07 01:01:14, xjz wrote:
> ditto: const

Done.

[szager1, 2016-12-07 01:31:49]

The CQ bit was checked by szager@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-07 01:32:37]

Dry run: 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-07 01:36:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-07 01:36:07]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[xjz, 2016-12-07 01:53:56]

lgtm

https://codereview.chromium.org/2553103004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/IntersectionObservation.h (right):

https://codereview.chromium.org/2553103004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/IntersectionObservation.h:21: bool
shouldReportRootBounds);
On 2016/12/07 01:30:49, szager1 wrote:
> On 2016/12/07 01:01:14, xjz wrote:
> > ooc, I did this bool to enum change as suggested by ojan@. Any reason to
> revert
> > it? 
> 
> I should talk to Ojan about that.  My reasons:
> 
> - Fewer files need to #include "core/layout/IntersectionGeometry."
> - Using an enum for bool values is, IMO, the worst kind of syntactic sugar.
> 
> Maybe Ojan was thinking that we would want to support other values in the
> future?  I can't think of what they might be.

I agree with you that using bool makes codes simpler.
FYI, the following is what he commented on my CL:
"Also, we generally try to avoid boolean arguments to functions unless it's
obvious what the bool is because it makes it hard to know what they are. From
the C++ style guide "Consider changing the function signature to replace a bool
argument with an enum argument. This will make the argument values
self-describing."

The typical solution here is to create an enum:
enum class IntersectionGeometryBounds {
  kShouldReportRootBounds,
  kShouldNotReportRootBounds,
}"

[szager1, 2016-12-07 02:03:49]

https://codereview.chromium.org/2553103004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/IntersectionObservation.h (right):

https://codereview.chromium.org/2553103004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/IntersectionObservation.h:21: bool
shouldReportRootBounds);
On 2016/12/07 01:53:56, xjz wrote:
> On 2016/12/07 01:30:49, szager1 wrote:
> > On 2016/12/07 01:01:14, xjz wrote:
> > > ooc, I did this bool to enum change as suggested by ojan@. Any reason to
> > revert
> > > it? 
> > 
> > I should talk to Ojan about that.  My reasons:
> > 
> > - Fewer files need to #include "core/layout/IntersectionGeometry."
> > - Using an enum for bool values is, IMO, the worst kind of syntactic sugar.
> > 
> > Maybe Ojan was thinking that we would want to support other values in the
> > future?  I can't think of what they might be.
> 
> I agree with you that using bool makes codes simpler.
> FYI, the following is what he commented on my CL:
> "Also, we generally try to avoid boolean arguments to functions unless it's
> obvious what the bool is because it makes it hard to know what they are. From
> the C++ style guide "Consider changing the function signature to replace a
bool
> argument with an enum argument. This will make the argument values
> self-describing."
> 
> The typical solution here is to create an enum:
> enum class IntersectionGeometryBounds {
>   kShouldReportRootBounds,
>   kShouldNotReportRootBounds,
> }"

I would argue that "it's obvious what the bool is."  Using values like
kShouldReportRootBounds for a variable named shouldReportRootBounds is pretty
redundant.

[ojan, 2016-12-10 01:18:26]

https://codereview.chromium.org/2553103004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/2553103004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:4029:
IntersectionGeometry geometry(nullptr, *this, Vector<Length>(), true);
This "true" was the reason I asked for it to be an enum. We have a general
policy to not pass bools as arguments that don't have a clear name. It's about
the calling location, not about the function definition location. The other
locations are fine because you have variables with the right name.

https://codereview.chromium.org/2553103004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/IntersectionGeometry.cpp (right):

https://codereview.chromium.org/2553103004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/IntersectionGeometry.cpp:27: while
(descendantFrame && descendantFrame != ancestorFrame) {
When is descendantFrame != ancestorFrame without the ancestor being the implicit
root?

https://codereview.chromium.org/2553103004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/IntersectionGeometry.cpp:77: frame = frame
? frame->localFrameRoot() : nullptr;
Can you create a new local variable here, e.g. frameRoot? That would make this
code more clear at a glance.

https://codereview.chromium.org/2553103004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/IntersectionGeometry.cpp:92:
m_rootIsImplicit(root ? 0 : 1),
Does !root work? I feel like I haven't seen this ternary pattern to set bools
elsewhere.

https://codereview.chromium.org/2553103004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/IntersectionGeometry.cpp:97: bool
IntersectionGeometry::initializeGeometry(Element* root, Element& target) {
This bool return value isn't super clear.

How would you feel about splitting this up into hasValidGeometry and
initializeGeometry and only calling the latter if m_isValid?

https://codereview.chromium.org/2553103004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/IntersectionGeometry.h (right):

https://codereview.chromium.org/2553103004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/IntersectionGeometry.h:80: const unsigned
m_shouldReportRootBounds : 1;
What does this "const" do?

[szager1, 2016-12-10 01:47:11]

https://codereview.chromium.org/2553103004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/2553103004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:4029:
IntersectionGeometry geometry(nullptr, *this, Vector<Length>(), true);
On 2016/12/10 01:18:26, ojan wrote:
> This "true" was the reason I asked for it to be an enum. We have a general
> policy to not pass bools as arguments that don't have a clear name. It's about
> the calling location, not about the function definition location. The other
> locations are fine because you have variables with the right name.

OK, I added a local variable here to clarify.

https://codereview.chromium.org/2553103004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/IntersectionGeometry.cpp (right):

https://codereview.chromium.org/2553103004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/IntersectionGeometry.cpp:27: while
(descendantFrame && descendantFrame != ancestorFrame) {
On 2016/12/10 01:18:26, ojan wrote:
> When is descendantFrame != ancestorFrame without the ancestor being the
implicit
> root?

If the observer was created with an explicit root, and then the target was
removed from the document and inserted into a different document.

https://codereview.chromium.org/2553103004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/IntersectionGeometry.cpp:77: frame = frame
? frame->localFrameRoot() : nullptr;
On 2016/12/10 01:18:26, ojan wrote:
> Can you create a new local variable here, e.g. frameRoot? That would make this
> code more clear at a glance.

Done.

https://codereview.chromium.org/2553103004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/IntersectionGeometry.cpp:92:
m_rootIsImplicit(root ? 0 : 1),
On 2016/12/10 01:18:26, ojan wrote:
> Does !root work? I feel like I haven't seen this ternary pattern to set bools
> elsewhere.

Switched to !root.

https://codereview.chromium.org/2553103004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/IntersectionGeometry.cpp:97: bool
IntersectionGeometry::initializeGeometry(Element* root, Element& target) {
On 2016/12/10 01:18:26, ojan wrote:
> This bool return value isn't super clear.
> 
> How would you feel about splitting this up into hasValidGeometry and
> initializeGeometry and only calling the latter if m_isValid?

Done.

https://codereview.chromium.org/2553103004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/IntersectionGeometry.h (right):

https://codereview.chromium.org/2553103004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/IntersectionGeometry.h:80: const unsigned
m_shouldReportRootBounds : 1;
On 2016/12/10 01:18:26, ojan wrote:
> What does this "const" do?

It means that it can only be assigned to in the constructor.

[ojan, 2016-12-12 19:53:31]

https://codereview.chromium.org/2553103004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/IntersectionGeometry.cpp (right):

https://codereview.chromium.org/2553103004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/IntersectionGeometry.cpp:27: while
(descendantFrame && descendantFrame != ancestorFrame) {
On 2016/12/10 at 01:47:11, szager1 wrote:
> On 2016/12/10 01:18:26, ojan wrote:
> > When is descendantFrame != ancestorFrame without the ancestor being the
implicit
> > root?
> 
> If the observer was created with an explicit root, and then the target was
removed from the document and inserted into a different document.

Do we deliver intersection records for that? I thought we got rid of
same-origin, cross-frame record delivery entirely. As per
https://github.com/WICG/IntersectionObserver/issues/87. So, you should be able
to just return false for the non-implicit root case where
descendantFrame!=ancestorFrame (which is this code, right?).

https://codereview.chromium.org/2553103004/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/IntersectionGeometry.h (right):

https://codereview.chromium.org/2553103004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/IntersectionGeometry.h:70: bool isValid()
const { return m_isValid; }
Nit: How about a more descriptive name? I wouldn't guess from the name that it
does what the comment describes. It's literally equal to the return value of
canComputeGeometry. How about calling this canComputeGeometry and
s/canComputeGeometry/initializeCanComputeGeometry/ above?

[ojan, 2016-12-12 20:00:24]

lgtm with the change we discussed offline for the cross-frame case.

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

https://codereview.chromium.org/2553103004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/IntersectionGeometry.cpp (right):

https://codereview.chromium.org/2553103004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/IntersectionGeometry.cpp:27: while
(descendantFrame && descendantFrame != ancestorFrame) {
On 2016/12/12 19:53:31, ojan wrote:
> On 2016/12/10 at 01:47:11, szager1 wrote:
> > On 2016/12/10 01:18:26, ojan wrote:
> > > When is descendantFrame != ancestorFrame without the ancestor being the
> implicit
> > > root?
> > 
> > If the observer was created with an explicit root, and then the target was
> removed from the document and inserted into a different document.
> 
> Do we deliver intersection records for that? I thought we got rid of
> same-origin, cross-frame record delivery entirely. As per
> https://github.com/WICG/IntersectionObserver/issues/87. So, you should be able
> to just return false for the non-implicit root case where
> descendantFrame!=ancestorFrame (which is this code, right?).

As discussed in-person, this function should not walk the frame tree at all,
since it only gets called in the explicit-root case, which must be
same-document.  I removed all of the frame-walking code.

https://codereview.chromium.org/2553103004/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/IntersectionGeometry.h (right):

https://codereview.chromium.org/2553103004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/IntersectionGeometry.h:70: bool isValid()
const { return m_isValid; }
On 2016/12/12 19:53:31, ojan wrote:
> Nit: How about a more descriptive name? I wouldn't guess from the name that it
> does what the comment describes. It's literally equal to the return value of
> canComputeGeometry. How about calling this canComputeGeometry and
> s/canComputeGeometry/initializeCanComputeGeometry/ above?

Done.

[szager1, 2016-12-12 20:16:19]

The CQ bit was checked by szager@chromium.org

[szager1, 2016-12-12 20:16:20]

The patchset sent to the CQ was uploaded after l-g-t-m from xjz@chromium.org,
ojan@chromium.org
Link to the patchset: https://codereview.chromium.org/2553103004/#ps80001
(title: "Simplify isContainingBlockChainDescendant")

[commit-bot: I haz the power, 2016-12-12 20:17:30]

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-12 23:18:06]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1481573779724650,
"parent_rev": "e4579fe199aad27d9cb3dacecf957b95eddeccd2", "commit_rev":
"703731322512b861196383bc36e8a2e50304a471"}

[commit-bot: I haz the power, 2016-12-12 23:18:35]

Description was changed from

==========
Refactor IntersectionGeometry class and move it to core/layout.

IntersectionGeometry is an ephemeral object, so make it stack
allocated, let it use raw pointers to LayoutObject, and move it to
core/layout.

Also, be explicit about the implicit root by introducing
rootIsImplicit.

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

to

==========
Refactor IntersectionGeometry class and move it to core/layout.

IntersectionGeometry is an ephemeral object, so make it stack
allocated, let it use raw pointers to LayoutObject, and move it to
core/layout.

Also, be explicit about the implicit root by introducing
rootIsImplicit.

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

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

[commit-bot: I haz the power, 2016-12-12 23:18:36]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-12-12 23:23:15]

Description was changed from

==========
Refactor IntersectionGeometry class and move it to core/layout.

IntersectionGeometry is an ephemeral object, so make it stack
allocated, let it use raw pointers to LayoutObject, and move it to
core/layout.

Also, be explicit about the implicit root by introducing
rootIsImplicit.

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

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

to

==========
Refactor IntersectionGeometry class and move it to core/layout.

IntersectionGeometry is an ephemeral object, so make it stack
allocated, let it use raw pointers to LayoutObject, and move it to
core/layout.

Also, be explicit about the implicit root by introducing
rootIsImplicit.

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

Committed: https://crrev.com/80f26da6153e051225f68c526979bf39f88472a3
Cr-Commit-Position: refs/heads/master@{#437949}
==========

[commit-bot: I haz the power, 2016-12-12 23:23:16]

Patchset 5 (id:??) landed as
https://crrev.com/80f26da6153e051225f68c526979bf39f88472a3
Cr-Commit-Position: refs/heads/master@{#437949}