Intersection Observer support for OOPIF

Intersection Observer support for OOPIF

Plumb a viewport intersection rect from a parent frame to any out of
process iframes that it embeds, and allow IntersectionObserver to
account for that rect when it computes viewport intersection.

This modifies LayoutView::MapToVisualRectInAncestorSpace and
mapLocalToAncestor to treat null ancestor pointers at implicit
references to the root frame, even if the root is remote.

For testing this enables the Intersection Observer layout tests that
were disabled with the --site-per-process flag.

BUG=615156
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2431473003
Cr-Commit-Position: refs/heads/master@{#443559}
Committed: https://chromium.googlesource.com/chromium/src/+/ea73179285a43f60275fd7a057d44bca6d4d9291

[kenrb, 2016-10-18 17:25:24]

Description was changed from

==========
[WIP] Intersection Observer support for OOPIF

Plumb a viewport intersection rect from a parent frame to any out of
process iframes that it embeds, and allow IntersectionObserver to
account for that rect when it computes viewport intersection.

BUG=615156
==========

to

==========
[WIP] Intersection Observer support for OOPIF

Plumb a viewport intersection rect from a parent frame to any out of
process iframes that it embeds, and allow IntersectionObserver to
account for that rect when it computes viewport intersection.

BUG=615156
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[kenrb, 2016-10-18 17:27:51]

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

[kenrb, 2016-10-18 17:27:52]

Stefan: Do you have time to look at this? It is still rough, but works at least
in basic cases and I'd be interested in knowing before going much further if you
think the approach is reasonable.

[szager1, 2016-10-24 05:12:35]

The basic approach looks OK, but a couple of questions:

- What triggers the viewport intersection update in the parent?  Is that TBD?

- Why pass both a rect and a point?  Presumably the rect is in the coordinate
system of the local child frame, and that should be sufficient.

[kenrb, 2016-10-24 16:28:45]

Thanks for having a look.

On 2016/10/24 05:12:35, szager1 wrote:
> The basic approach looks OK, but a couple of questions:
> 
> - What triggers the viewport intersection update in the parent?  Is that TBD?
> 

I was originally expecting to have to plumb a backward message (from the child
frame to its parent) about needing intersections, but this appears to not be
necessary. the m_needsViewportIntersection flag is set by FrameRectsChanged(),
then the intersection gets calculated on the next call to
updateAllLifecyclePhases(). Is there more to it than this?

> - Why pass both a rect and a point?  Presumably the rect is in the coordinate
> system of the local child frame, and that should be sufficient.

The rect is in the coordinate space of the parent frame's local root (which is
the main frame, if there is no nesting of OOPIFs). The trouble is that the child
frame has no concept of this. I could convert the rect to the parent frame's
coordinate space (so an unclipped frame would send (0,0,w,h)) but I was trying
to make m_viewportIntersection in FrameView accurate in the main frame's
coordinate space. Maybe that is unnecessary; should I ignore that rect?

[szager1, 2016-10-24 16:54:52]

OK, the fog is lifting a bit.

I think the viewport intersection should be computed and passed in the local
coordinate space of the child frame, and root offset is probably extraneous
(that should be derivable from the widget tree anyway, right?).  The really
important information is "what part of my layout is visible?", so I think local
coordinates are appropriate.

https://codereview.chromium.org/2431473003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/RemoteFrameView.cpp (right):

https://codereview.chromium.org/2431473003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/RemoteFrameView.cpp:47: IntRect
viewportIntersection = parent->contentsToRootFrame(frameRect());
I don't think this is right, because parentsToRootFrame does not calculate
clipping up the layout tree.  For example:

<div style="overflow:scroll; width:100px; height:100px;">
  <iframe></iframe>
</div>

contentToRootFrame will not apply the overflow clip of the div.  Instead, use
frame()->ownerLayoutObject()->mapToVisualRectInAncestorSpace().

[kenrb, 2016-10-28 18:47:11]

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

[commit-bot: I haz the power, 2016-10-28 18:47:53]

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-10-28 18:50:22]

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

[commit-bot: I haz the power, 2016-10-28 18:50:23]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[kenrb, 2016-10-28 18:51:57]

Thanks, PS#2 changes the approach based on your suggestions, although I am not
entirely certain that RemoteFrameView::updateRemoteViewportIntersection()
accounts for everything it needs to.

https://codereview.chromium.org/2431473003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/RemoteFrameView.cpp (right):

https://codereview.chromium.org/2431473003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/RemoteFrameView.cpp:47: IntRect
viewportIntersection = parent->contentsToRootFrame(frameRect());
On 2016/10/24 16:54:52, szager1 wrote:
> I don't think this is right, because parentsToRootFrame does not calculate
> clipping up the layout tree.  For example:
> 
> <div style="overflow:scroll; width:100px; height:100px;">
>   <iframe></iframe>
> </div>
> 
> contentToRootFrame will not apply the overflow clip of the div.  Instead, use
> frame()->ownerLayoutObject()->mapToVisualRectInAncestorSpace().
> 

Done.

[kenrb, 2016-10-28 19:33:28]

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

[commit-bot: I haz the power, 2016-10-28 19:34:13]

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-10-28 20:04:56]

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

[commit-bot: I haz the power, 2016-10-28 20:04:56]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[kenrb, 2016-10-31 20:50:13]

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

[commit-bot: I haz the power, 2016-10-31 20:50:58]

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-10-31 21:27:17]

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

[commit-bot: I haz the power, 2016-10-31 21:27:17]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[kenrb, 2016-11-01 16:13:54]

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

[commit-bot: I haz the power, 2016-11-01 16:14:16]

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-11-01 17:45:29]

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

[commit-bot: I haz the power, 2016-11-01 17:45:31]

Dry run: Try jobs failed on following builders:
  linux_site_isolation on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_site_isol...)

[szager1, 2016-11-01 22:33:13]

This looks basically right to me.  Before landing, can you please create a
virtual test suite that runs LayoutTests/intersection-observer and
LayoutTests/http/tests/intersection-observer with OOPIF enabled?

[kenrb, 2016-11-01 22:41:25]

Thanks Stefan.

The linux_site_isolation bot already runs all layout tests with
--site-per-process, and there is one Intersection Observer test that is disabled
on it (unsurprisingly, the one that creates a cross-site iframe). The test still
fails with my patch, and I am currently working through why that is. I will
upload a new PS when I figure that out.

[Z_DONOTUSE, 2016-11-05 02:36:31]

ojan@google.com changed reviewers:
+ ojan@google.com

[Z_DONOTUSE, 2016-11-05 02:36:32]

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

https://codereview.chromium.org/2431473003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/IntersectionObservation.cpp:56:
m_observer->intersectWithRemoteAncestorsIfNeeded(rect);
This doesn't look right to me. Won't applying root margin after this step be
wrong?

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

https://codereview.chromium.org/2431473003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/IntersectionObserver.cpp:451: void
IntersectionObserver::intersectWithRemoteAncestorsIfNeeded(
This isn't quite what I was picturing. I think you just need to modify
LayoutView::mapToVisualRectInAncestorSpace to understand when the LayoutView's
parent frame is remote than then doing the below intersection.

That way, IntersectionObserver doesn't need to know about intersecting with
remote frames, it doesn't need this extra bool, and any future callers of
mapToVisualRectInAncestorSpace will work correctly with remote frames.

[kenrb, 2016-11-07 18:43:51]

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

[commit-bot: I haz the power, 2016-11-07 18:44:17]

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-11-07 18:47:55]

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

[commit-bot: I haz the power, 2016-11-07 18:47:56]

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_...)

[kenrb, 2016-11-07 18:48:48]

kenrb@chromium.org changed reviewers:
+ ojan@chromium.org
- ojan@google.com

[kenrb, 2016-11-07 18:48:50]

szager: PTAL again? I have enabled the cross-site IO layout test, and it passes
with --additional-driver-flag=--site-per-process. We have a bot that runs layout
tests with that flag, so regressions should be apparent.

There were some bugs in the earlier version that I had to address, so the most
recent patchset isn't trivial.

https://codereview.chromium.org/2431473003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2431473003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.cpp:4565:
updateAllLifecyclePhases();
Is this call okay? I was finding that the test was failing because lifecycle
phases didn't get updated after the remote ancestor frame scrolled, even though
we got the correct intersection update. Or is there an indirect way to trigger
that to happen?

[kenrb, 2016-11-07 20:06:18]

Ojan: Sorry, I had missed your comments from Friday. Responses below. I also
have to upload another patch because there is a non-trivial conflict with
skyostil@'s frame throttling CL from last week:
https://codereview.chromium.org/2272773002

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

https://codereview.chromium.org/2431473003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/IntersectionObservation.cpp:56:
m_observer->intersectWithRemoteAncestorsIfNeeded(rect);
On 2016/11/05 02:36:32, Z_DONOTUSE wrote:
> This doesn't look right to me. Won't applying root margin after this step be
> wrong? 

This is addressed in the subsequent patchset. For remote frames we set
m_shouldReportRootBounds to be false.

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

https://codereview.chromium.org/2431473003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/IntersectionObserver.cpp:451: void
IntersectionObserver::intersectWithRemoteAncestorsIfNeeded(
On 2016/11/05 02:36:32, Z_DONOTUSE wrote:
> This isn't quite what I was picturing. I think you just need to modify
> LayoutView::mapToVisualRectInAncestorSpace to understand when the LayoutView's
> parent frame is remote than then doing the below intersection.
> 
> That way, IntersectionObserver doesn't need to know about intersecting with
> remote frames, it doesn't need this extra bool, and any future callers of
> mapToVisualRectInAncestorSpace will work correctly with remote frames.

I'm not sure this is simple. In terms of the layout tree, OOPIFs have no
ancestors, because their parent RemoteFrames don't have layout objects (or
Documents). So there is nothing to pass as the first argument to
mapToVisualRectInAncestorSpace.

We could add an equivalent to mapToVisualRectInAncestorSpace (say,
mapToVisualRectInRemoteMainFrame), but we still have to keep the boolean so that
we know when to call it, and I don't think that ends up looking much different
from how this patch looks now. I have no objection to adding a method like that
if you think it is valuable.

[kenrb, 2016-11-09 18:22:50]

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

[commit-bot: I haz the power, 2016-11-09 18:23:07]

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-11-09 18:26:12]

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

[commit-bot: I haz the power, 2016-11-09 18:26:13]

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

[kenrb, 2016-11-09 20:17:33]

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

[commit-bot: I haz the power, 2016-11-09 20:18:02]

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-11-09 21:38:42]

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

[commit-bot: I haz the power, 2016-11-09 21:38:43]

Dry run: This issue passed the CQ dry run.

[kenrb, 2016-11-09 21:41:04]

Stefan and Ojan: PTAL again? I had to rework the intersection calculation in the
parent frame, and now I have the RemoteFrameView instantiating an
IntersectionObserver to get that.

Ojan's question still isn't resolved, I'd appreciate further input.

[kenrb, 2016-11-09 21:41:23]

Description was changed from

==========
[WIP] Intersection Observer support for OOPIF

Plumb a viewport intersection rect from a parent frame to any out of
process iframes that it embeds, and allow IntersectionObserver to
account for that rect when it computes viewport intersection.

BUG=615156
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Intersection Observer support for OOPIF

Plumb a viewport intersection rect from a parent frame to any out of
process iframes that it embeds, and allow IntersectionObserver to
account for that rect when it computes viewport intersection.

BUG=615156
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[kenrb, 2016-11-14 16:27:58]

Friendly ping for reviewers?

[szager1, 2016-11-16 18:57:58]

I'm still reviewing this, but a couple of quick comments...

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

https://codereview.chromium.org/2431473003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/IntersectionObserver.cpp:451: void
IntersectionObserver::intersectWithRemoteAncestorsIfNeeded(
On 2016/11/07 20:06:18, kenrb wrote:
> On 2016/11/05 02:36:32, Z_DONOTUSE wrote:
> > This isn't quite what I was picturing. I think you just need to modify
> > LayoutView::mapToVisualRectInAncestorSpace to understand when the
LayoutView's
> > parent frame is remote than then doing the below intersection.
> > 
> > That way, IntersectionObserver doesn't need to know about intersecting with
> > remote frames, it doesn't need this extra bool, and any future callers of
> > mapToVisualRectInAncestorSpace will work correctly with remote frames.
> 
> I'm not sure this is simple. In terms of the layout tree, OOPIFs have no
> ancestors, because their parent RemoteFrames don't have layout objects (or
> Documents). So there is nothing to pass as the first argument to
> mapToVisualRectInAncestorSpace.
> 
> We could add an equivalent to mapToVisualRectInAncestorSpace (say,
> mapToVisualRectInRemoteMainFrame), but we still have to keep the boolean so
that
> we know when to call it, and I don't think that ends up looking much different
> from how this patch looks now. I have no objection to adding a method like
that
> if you think it is valuable.

If the ancestor argument to mapToVisualRectInAncestorSpace is nullptr, then it
maps up to the top-level frame.  It should be possible to modify this code to do
the right thing with remote frames:

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/La...

https://codereview.chromium.org/2431473003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2431473003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.cpp:4565:
updateAllLifecyclePhases();
On 2016/11/07 18:48:49, kenrb wrote:
> Is this call okay? I was finding that the test was failing because lifecycle
> phases didn't get updated after the remote ancestor frame scrolled, even
though
> we got the correct intersection update. Or is there an indirect way to trigger
> that to happen?

This is not kosher.  I think you just want to:

if (m_remoteViewportIntersection != viewportIntersection)
  scheduleAnimation();

[szager1, 2016-11-16 22:11:22]

https://codereview.chromium.org/2431473003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/IntersectionObserver.cpp (right):

https://codereview.chromium.org/2431473003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/IntersectionObserver.cpp:463: void
IntersectionObserver::intersectWithRemoteAncestorsIfNeeded(
I agree with Ojan's point that this should be handled by
LayoutView::mapToVisualRectInAncestorSpace without the need for an
intersectWithRemoteAncestors flag here.

https://codereview.chromium.org/2431473003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/RemoteFrameView.cpp (right):

https://codereview.chromium.org/2431473003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/RemoteFrameView.cpp:43:
m_intersectionObserver = IntersectionObserver::create(
This won't work, because the observer will only fire when you cross the given
threshold.  So, if the remote frame goes from 10% visible to 20% visible -- or
if the intersection rect changes without changing the visible ratio -- no
callback will fire.

[kenrb, 2016-11-21 19:30:02]

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

[commit-bot: I haz the power, 2016-11-21 19:30:39]

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

[kenrb, 2016-11-21 19:37:35]

szager: I have uploaded a CL that mostly addresses your concerns. Unfortunately
I still don't see a way to get rid of the flag in the IntersectionObserver
constructor, as discussed below.

https://codereview.chromium.org/2431473003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2431473003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.cpp:4565:
updateAllLifecyclePhases();
On 2016/11/16 18:57:58, szager1 wrote:
> On 2016/11/07 18:48:49, kenrb wrote:
> > Is this call okay? I was finding that the test was failing because lifecycle
> > phases didn't get updated after the remote ancestor frame scrolled, even
> though
> > we got the correct intersection update. Or is there an indirect way to
trigger
> > that to happen?
> 
> This is not kosher.  I think you just want to:
> 
> if (m_remoteViewportIntersection != viewportIntersection)
>   scheduleAnimation();

Done.

https://codereview.chromium.org/2431473003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/IntersectionObserver.cpp (right):

https://codereview.chromium.org/2431473003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/IntersectionObserver.cpp:463: void
IntersectionObserver::intersectWithRemoteAncestorsIfNeeded(
On 2016/11/16 22:11:22, szager1 wrote:
> I agree with Ojan's point that this should be handled by
> LayoutView::mapToVisualRectInAncestorSpace without the need for an
> intersectWithRemoteAncestors flag here.

In the most recent PS I have modified mapToVisualRectInAncestorSpace as you
suggested, and removed this method (intersectWithRemoteAncestorsIfNeeded), but
the flag is still necessary.

IntersectionObserver still needs a reference to the local frame root for most of
the things it does with the root, but then it needs to know to call
mapToVisualRectInAncestorSpace with nullptr instead of the local root pointer.
The way I have implemented it now adds a new layout object accessor to
IntersectionObserver for the call to mapToVisualRectInAncestorSpace. If there is
a different way you'd prefer to have it done, then I can change it.

https://codereview.chromium.org/2431473003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/RemoteFrameView.cpp (right):

https://codereview.chromium.org/2431473003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/RemoteFrameView.cpp:43:
m_intersectionObserver = IntersectionObserver::create(
On 2016/11/16 22:11:22, szager1 wrote:
> This won't work, because the observer will only fire when you cross the given
> threshold.  So, if the remote frame goes from 10% visible to 20% visible -- or
> if the intersection rect changes without changing the visible ratio -- no
> callback will fire.

Thanks, I didn't realize this. I've reworked it to be more similar to the
original approach.

[commit-bot: I haz the power, 2016-11-21 20:17:14]

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

[commit-bot: I haz the power, 2016-11-21 20:17:15]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[kenrb, 2016-11-21 20:23:40]

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

[commit-bot: I haz the power, 2016-11-21 20:24:50]

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-11-21 23:28:50]

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

[commit-bot: I haz the power, 2016-11-21 23:28:51]

Dry run: This issue passed the CQ dry run.

[ojan, 2016-11-23 19:14:00]

https://codereview.chromium.org/2431473003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/IntersectionObserver.cpp (right):

https://codereview.chromium.org/2431473003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/IntersectionObserver.cpp:463: void
IntersectionObserver::intersectWithRemoteAncestorsIfNeeded(
On 2016/11/21 at 19:37:35, kenrb wrote:
> On 2016/11/16 22:11:22, szager1 wrote:
> > I agree with Ojan's point that this should be handled by
> > LayoutView::mapToVisualRectInAncestorSpace without the need for an
> > intersectWithRemoteAncestors flag here.
> 
> In the most recent PS I have modified mapToVisualRectInAncestorSpace as you
suggested, and removed this method (intersectWithRemoteAncestorsIfNeeded), but
the flag is still necessary.
> 
> IntersectionObserver still needs a reference to the local frame root for most
of the things it does with the root, but then it needs to know to call
mapToVisualRectInAncestorSpace with nullptr instead of the local root pointer.
The way I have implemented it now adds a new layout object accessor to
IntersectionObserver for the call to mapToVisualRectInAncestorSpace. If there is
a different way you'd prefer to have it done, then I can change it.

Can we always pass nullptr to mapToVisualRectInAncestorSpace if no root was
provided to IntersectionObserver::create? That way we won't need any
special-casing of code for remote frames outside of the couple lines in
mapToVisualRectInAncestorSpace that intersect with
frameView()->remoteViewportIntersection().

With IntersectionObserver, the only case where we need to cross frame boundaries
is when the root is null. We can add an assert to mapToVisualRectInAncestorSpace
that the provided ancestor is in the same Document as the LayoutView to ensure
other code doesn't try to cross frame boundaries to something other than the
top-most frame.

[kenrb, 2016-11-23 20:07:29]

https://codereview.chromium.org/2431473003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/IntersectionObserver.cpp (right):

https://codereview.chromium.org/2431473003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/IntersectionObserver.cpp:463: void
IntersectionObserver::intersectWithRemoteAncestorsIfNeeded(
On 2016/11/23 19:14:00, ojan wrote:
> On 2016/11/21 at 19:37:35, kenrb wrote:
> > On 2016/11/16 22:11:22, szager1 wrote:
> > > I agree with Ojan's point that this should be handled by
> > > LayoutView::mapToVisualRectInAncestorSpace without the need for an
> > > intersectWithRemoteAncestors flag here.
> > 
> > In the most recent PS I have modified mapToVisualRectInAncestorSpace as you
> suggested, and removed this method (intersectWithRemoteAncestorsIfNeeded), but
> the flag is still necessary.
> > 
> > IntersectionObserver still needs a reference to the local frame root for
most
> of the things it does with the root, but then it needs to know to call
> mapToVisualRectInAncestorSpace with nullptr instead of the local root pointer.
> The way I have implemented it now adds a new layout object accessor to
> IntersectionObserver for the call to mapToVisualRectInAncestorSpace. If there
is
> a different way you'd prefer to have it done, then I can change it.
> 
> Can we always pass nullptr to mapToVisualRectInAncestorSpace if no root was
> provided to IntersectionObserver::create? That way we won't need any
> special-casing of code for remote frames outside of the couple lines in
> mapToVisualRectInAncestorSpace that intersect with
> frameView()->remoteViewportIntersection().
> 
> With IntersectionObserver, the only case where we need to cross frame
boundaries
> is when the root is null. We can add an assert to
mapToVisualRectInAncestorSpace
> that the provided ancestor is in the same Document as the LayoutView to ensure
> other code doesn't try to cross frame boundaries to something other than the
> top-most frame.

We still need a boolean in the IntersectionObserver constructor though, right?
We need a reference to the local frame root for other reasons (there is no path
to the layout tree, otherwise), and the information that there was originally a
null root pointer passed to create() is lost. I had previously considered
passing a second pointer in the constructor for this purpose, but then realized
that it would always either be null or equal to |root|, which seems like an
over-complicated way of saying 'boolean'. Your suggestion seems to amount to us
changing the name of the boolean to m_intersectWithViewportOnly (or something
similar), and using it also for local cross-frame intersections.

However, does this create a problem for knowing whether or not to apply root
margins? Currently this CL only sets shouldReportRootBounds when intersecting
with the viewport and the main frame is local, which seems to correctly make IO
in an OOPIF process ignore margins from the local frame root. I don't know how
to handle that if we combine those situations.

[ojan, 2016-11-23 22:55:54]

On 2016/11/23 at 20:07:29, kenrb wrote:
>
https://codereview.chromium.org/2431473003/diff/120001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/dom/IntersectionObserver.cpp (right):
> 
>
https://codereview.chromium.org/2431473003/diff/120001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/dom/IntersectionObserver.cpp:463: void
IntersectionObserver::intersectWithRemoteAncestorsIfNeeded(
> On 2016/11/23 19:14:00, ojan wrote:
> > On 2016/11/21 at 19:37:35, kenrb wrote:
> > > On 2016/11/16 22:11:22, szager1 wrote:
> > > > I agree with Ojan's point that this should be handled by
> > > > LayoutView::mapToVisualRectInAncestorSpace without the need for an
> > > > intersectWithRemoteAncestors flag here.
> > > 
> > > In the most recent PS I have modified mapToVisualRectInAncestorSpace as
you
> > suggested, and removed this method (intersectWithRemoteAncestorsIfNeeded),
but
> > the flag is still necessary.
> > > 
> > > IntersectionObserver still needs a reference to the local frame root for
most
> > of the things it does with the root, but then it needs to know to call
> > mapToVisualRectInAncestorSpace with nullptr instead of the local root
pointer.
> > The way I have implemented it now adds a new layout object accessor to
> > IntersectionObserver for the call to mapToVisualRectInAncestorSpace. If
there is
> > a different way you'd prefer to have it done, then I can change it.
> > 
> > Can we always pass nullptr to mapToVisualRectInAncestorSpace if no root was
> > provided to IntersectionObserver::create? That way we won't need any
> > special-casing of code for remote frames outside of the couple lines in
> > mapToVisualRectInAncestorSpace that intersect with
> > frameView()->remoteViewportIntersection().
> > 
> > With IntersectionObserver, the only case where we need to cross frame
boundaries
> > is when the root is null. We can add an assert to
mapToVisualRectInAncestorSpace
> > that the provided ancestor is in the same Document as the LayoutView to
ensure
> > other code doesn't try to cross frame boundaries to something other than the
> > top-most frame.
> 
> We still need a boolean in the IntersectionObserver constructor though, right?
We need a reference to the local frame root for other reasons (there is no path
to the layout tree, otherwise), and the information that there was originally a
null root pointer passed to create() is lost. I had previously considered
passing a second pointer in the constructor for this purpose, but then realized
that it would always either be null or equal to |root|, which seems like an
over-complicated way of saying 'boolean'. Your suggestion seems to amount to us
changing the name of the boolean to m_intersectWithViewportOnly (or something
similar), and using it also for local cross-frame intersections.

Ah. My problem isn't with adding the new bool. It's with IntersectionObserver
needing to know about remote frames. The more places where we have control logic
based of whether a frame is remote, the more potential places we have for bugs
around OOPIF.

> However, does this create a problem for knowing whether or not to apply root
margins? Currently this CL only sets shouldReportRootBounds when intersecting
with the viewport and the main frame is local, which seems to correctly make IO
in an OOPIF process ignore margins from the local frame root. I don't know how
to handle that if we combine those situations.

Not sure I understand the changes in this patch to this behavior.

https://codereview.chromium.org/2431473003/diff/170001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/IntersectionObserver.cpp (right):

https://codereview.chromium.org/2431473003/diff/170001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/IntersectionObserver.cpp:293: if
(!m_intersectWithRemoteAncestors)
Isn't this always true if target and root are in the same frame?

[kenrb, 2016-11-23 23:22:56]

On 2016/11/23 22:55:54, ojan wrote:
> Ah. My problem isn't with adding the new bool. It's with IntersectionObserver
> needing to know about remote frames. The more places where we have control
logic
> based of whether a frame is remote, the more potential places we have for bugs
> around OOPIF.
>

Understood.

> Not sure I understand the changes in this patch to this behavior.
> 

See below.

[kenrb, 2016-11-23 23:23:25]

Oops... forgot to publish draft.

https://codereview.chromium.org/2431473003/diff/170001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/IntersectionObserver.cpp (right):

https://codereview.chromium.org/2431473003/diff/170001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/IntersectionObserver.cpp:293: if
(!m_intersectWithRemoteAncestors)
On 2016/11/23 22:55:54, ojan wrote:
> Isn't this always true if target and root are in the same frame?

This is the special case problem I am thinking about. If the target is in the
real root frame (not an OOPIF) then we want to set shouldReportRootBounds to
true, and the observations will take the root margins into account (see
IntersectionObservation::applyRootMargin). For OOPIFs, we are using the local
frame root as rootNode(), which isn't the top-level frame. In that case we don't
want to apply root margins.

[szager1, 2016-11-26 08:55:22]

Sorry it has taken me so long to review this.  I don't see anything obviously
wrong with the latest patch, but I can't help wondering if the handling of OOPIF
is overly complicated.

There are only two possibilities for an IntersectionObserver root: either it's
in the same document as the target, or else it's the top-level document.  It
seems like it should be possible to modify the code so that a null rootNode
indicates unambiguously that the root is the top-level document, and all of the
algorithmic code should just act accordingly.

I'm in the process of reading through all of the IntersectionObserver code to
see if this would cause any problems...

[kenrb, 2016-11-28 16:58:17]

On 2016/11/26 08:55:22, szager1 wrote:
> Sorry it has taken me so long to review this.  I don't see anything obviously
> wrong with the latest patch, but I can't help wondering if the handling of
OOPIF
> is overly complicated.
> 
> There are only two possibilities for an IntersectionObserver root: either it's
> in the same document as the target, or else it's the top-level document.  It
> seems like it should be possible to modify the code so that a null rootNode
> indicates unambiguously that the root is the top-level document, and all of
the
> algorithmic code should just act accordingly.
> 
> I'm in the process of reading through all of the IntersectionObserver code to
> see if this would cause any problems...

It looked to me like it the IntersectionObserver constructor's call to
addTrackedObserver() on the root document's IntersectionObserverController would
be important. If the root is null then there is no way to do that.

[kenrb, 2016-12-14 20:10:20]

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

[commit-bot: I haz the power, 2016-12-14 20:11:06]

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-14 22:25:21]

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

[commit-bot: I haz the power, 2016-12-14 22:25:22]

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

[kenrb, 2016-12-15 16:53:02]

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

[commit-bot: I haz the power, 2016-12-15 16:53:42]

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-15 18:12:34]

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

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

Dry run: This issue passed the CQ dry run.

[kenrb, 2016-12-15 20:32:56]

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

[commit-bot: I haz the power, 2016-12-15 20:33:43]

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

[kenrb, 2016-12-15 21:50:02]

Description was changed from

==========
Intersection Observer support for OOPIF

Plumb a viewport intersection rect from a parent frame to any out of
process iframes that it embeds, and allow IntersectionObserver to
account for that rect when it computes viewport intersection.

BUG=615156
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Intersection Observer support for OOPIF

Plumb a viewport intersection rect from a parent frame to any out of
process iframes that it embeds, and allow IntersectionObserver to
account for that rect when it computes viewport intersection.

This modifies LayoutView::MapToVisualRectInAncestorSpace and
mapLocalToAncestor to treat null ancestor pointers at implicit
references to the root frame, even if the root is remote.

For testing this enables the Intersection Observer layout tests that
were disabled with the --site-per-process flag.

BUG=615156
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[kenrb, 2016-12-15 21:50:41]

szager: Would you mind taking another pass on this, please?

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

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

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

Dry run: This issue passed the CQ dry run.

[szager1, 2016-12-16 03:42:34]

https://codereview.chromium.org/2431473003/diff/230001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2431473003/diff/230001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:4711: (ancestor &&
ancestor->frame() != frame().tree().top())) {
I think the 'ancestor->frame() != frame().tree().top()' is not right.  I think
it should just be:

if (ancestor || frame().localFrameRoot() == frame().tree().top())

Even if ancestor->frame() is not the top frame, its scroll offset still will not
be applied by LayoutView::maplocalToAncestor, so it needs to be subtracted here.

https://codereview.chromium.org/2431473003/diff/230001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/FrameView.h (right):

https://codereview.chromium.org/2431473003/diff/230001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.h:777: void
mapQuadToAncestorFrameIncludingScrollOffset(
Please put a comment here about why this is necessary (why localToAncestorQuad
isn't sufficient).

https://codereview.chromium.org/2431473003/diff/230001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/IntersectionGeometry.cpp (right):

https://codereview.chromium.org/2431473003/diff/230001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/IntersectionGeometry.cpp:169: m_rootRect,
m_root, m_root->document().layoutView(),
The ancestor argument should be:

rootIsImplicit() ? nullptr : m_root->document().layoutView()

[kenrb, 2016-12-16 16:17:09]

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

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

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

[kenrb, 2016-12-16 16:17:35]

https://codereview.chromium.org/2431473003/diff/230001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2431473003/diff/230001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:4711: (ancestor &&
ancestor->frame() != frame().tree().top())) {
On 2016/12/16 03:42:34, szager1 wrote:
> I think the 'ancestor->frame() != frame().tree().top()' is not right.  I think
> it should just be:
> 
> if (ancestor || frame().localFrameRoot() == frame().tree().top())
> 
> Even if ancestor->frame() is not the top frame, its scroll offset still will
not
> be applied by LayoutView::maplocalToAncestor, so it needs to be subtracted
here.

I think that situation is covered, because this evaluates to true if the
ancestor frame is not the top frame.

One of the things I am trying to account for is if the main frame is local, but
there is a remote frame in the tree in between the current frame and the top. In
that case, mapLocalToAncestor acts the same as if the main frame is remote, but
it is possible that it could be called with a non-null ancestor.

That said, I have made a small improvement to the condition in the next patch
set.

https://codereview.chromium.org/2431473003/diff/230001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/FrameView.h (right):

https://codereview.chromium.org/2431473003/diff/230001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.h:777: void
mapQuadToAncestorFrameIncludingScrollOffset(
On 2016/12/16 03:42:34, szager1 wrote:
> Please put a comment here about why this is necessary (why localToAncestorQuad
> isn't sufficient).

Done.

https://codereview.chromium.org/2431473003/diff/230001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/IntersectionGeometry.cpp (right):

https://codereview.chromium.org/2431473003/diff/230001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/IntersectionGeometry.cpp:169: m_rootRect,
m_root, m_root->document().layoutView(),
On 2016/12/16 03:42:34, szager1 wrote:
> The ancestor argument should be:
> 
> rootIsImplicit() ? nullptr : m_root->document().layoutView()

Done.

[szager1, 2016-12-16 17:44:32]

https://codereview.chromium.org/2431473003/diff/250001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2431473003/diff/250001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:4712:
ancestor->frame()->localFrameRoot() != frame().localFrameRoot())) {
I may be confused, but shouldn't this be == rather than != ?

As I understand it: if the frame traversal passes through an OOPIF frame, then
all scroll offsets have already been applied, and there's nothing to do here. 
That's true even if the top frame and bottom frame are in the same process.  So
we're looking for the case where the ancestor and descendant have an OOPIF frame
between them.

[commit-bot: I haz the power, 2016-12-16 17:52:01]

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

[commit-bot: I haz the power, 2016-12-16 17:52:03]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[kenrb, 2016-12-16 17:57:36]

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

[kenrb, 2016-12-16 17:57:40]

https://codereview.chromium.org/2431473003/diff/250001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2431473003/diff/250001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:4712:
ancestor->frame()->localFrameRoot() != frame().localFrameRoot())) {
On 2016/12/16 17:44:32, szager1 wrote:
> I may be confused, but shouldn't this be == rather than != ?
> 
> As I understand it: if the frame traversal passes through an OOPIF frame, then
> all scroll offsets have already been applied, and there's nothing to do here. 
> That's true even if the top frame and bottom frame are in the same process. 
So
> we're looking for the case where the ancestor and descendant have an OOPIF
frame
> between them.

Yes, you are right, I had it mixed up. Now fixed.

[commit-bot: I haz the power, 2016-12-16 17:58:06]

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

[szager1, 2016-12-16 18:08:39]

lgtm, thanks!

[kenrb, 2016-12-16 18:41:33]

kenrb@chromium.org changed reviewers:
+ dcheng@chromium.org, nick@chromium.org

[kenrb, 2016-12-16 18:41:35]

Thanks, Stefan.

ncarter: PTAL content/?
dcheng: PTAL WebKit/Source/web/?

[ncarter (slow), 2016-12-16 19:22:49]

content lgtm

[commit-bot: I haz the power, 2016-12-16 22:30:38]

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

[commit-bot: I haz the power, 2016-12-16 22:30:40]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[kenrb, 2017-01-03 20:00:27]

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

[commit-bot: I haz the power, 2017-01-03 20:00:43]

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, 2017-01-03 21:52:18]

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

[commit-bot: I haz the power, 2017-01-03 21:52:19]

Dry run: This issue passed the CQ dry run.

[dcheng, 2017-01-05 07:10:20]

https://codereview.chromium.org/2431473003/diff/290001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2431473003/diff/290001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:202:
m_remoteViewportIntersection(0, 0, 0, 0),
Nit: omit this, as the default constructor does this

https://codereview.chromium.org/2431473003/diff/290001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/FrameView.h (right):

https://codereview.chromium.org/2431473003/diff/290001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.h:819: void
mapQuadToAncestorFrameIncludingScrollOffset(
Can we inline this directly, since it's only used from one place?

https://codereview.chromium.org/2431473003/diff/290001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/RemoteFrameView.cpp (right):

https://codereview.chromium.org/2431473003/diff/290001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/RemoteFrameView.cpp:39:
!m_remoteFrame->client() || !m_remoteFrame->ownerLayoutObject())
Can this be simplified to just checking if it has an ownerLayoutObject()? I
think the last condition being true should imply all the others.

https://codereview.chromium.org/2431473003/diff/290001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/RemoteFrameView.cpp:123:
m_remoteFrame->client()->visibilityChanged(false);
How come these need to null check while frameRectsChanged() doesn't? Do we call
this on something that's detached?

https://codereview.chromium.org/2431473003/diff/290001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutView.cpp (right):

https://codereview.chromium.org/2431473003/diff/290001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutView.cpp:481: // This is the case of
a Frame with a remote parent.
While I'm OK with this patch as written, my original understanding is we'd make
these bits fairly agnostic to remote frames. Ideally, I'd like it to be so if
ancestor is nullptr, we have a generic codepath for intersecting with the
viewport that's used for local and remote frames.

Is this an impractical goal?

https://codereview.chromium.org/2431473003/diff/290001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp (right):

https://codereview.chromium.org/2431473003/diff/290001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp:731: if
(!m_localRoot->parent() || !m_localRoot->parent()->isWebRemoteFrame())
Are these DCHECKable conditions? It seems like we shouldn't be sending the IPC
to begin with if these conditions aren't true.

https://codereview.chromium.org/2431473003/diff/290001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp:733: if
(m_localRoot->frameView()) {
Why would it not have a frame view? IPC races?

https://codereview.chromium.org/2431473003/diff/290001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebWidget.h (right):

https://codereview.chromium.org/2431473003/diff/290001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebWidget.h:237: virtual void
setRemoteViewportIntersection(const WebRect&) {}
Should we add this directly on WebFrameWidget instead? There doesn't seem to be
a valid case for sending it to any other type of widget.

[szager1, 2017-01-05 20:25:22]

https://codereview.chromium.org/2431473003/diff/290001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/FrameView.h (right):

https://codereview.chromium.org/2431473003/diff/290001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.h:819: void
mapQuadToAncestorFrameIncludingScrollOffset(
On 2017/01/05 07:10:19, dcheng wrote:
> Can we inline this directly, since it's only used from one place?

This is a generally useful method, I expect it to be used elsewhere in the
future.

https://codereview.chromium.org/2431473003/diff/290001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutView.cpp (right):

https://codereview.chromium.org/2431473003/diff/290001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutView.cpp:481: // This is the case of
a Frame with a remote parent.
On 2017/01/05 07:10:19, dcheng wrote:
> While I'm OK with this patch as written, my original understanding is we'd
make
> these bits fairly agnostic to remote frames. Ideally, I'd like it to be so if
> ancestor is nullptr, we have a generic codepath for intersecting with the
> viewport that's used for local and remote frames.

+1, it should be straightforward to add something to FrameView for this,
something like:

Element* owner = document().localOwner();
if (!owner)
  return frameView()->mapToVisualRectInTopFrameSpace(rect);

[kenrb, 2017-01-09 19:15:09]

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

[commit-bot: I haz the power, 2017-01-09 19:16:12]

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

[kenrb, 2017-01-09 19:31:20]

https://codereview.chromium.org/2431473003/diff/290001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2431473003/diff/290001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:202:
m_remoteViewportIntersection(0, 0, 0, 0),
On 2017/01/05 07:10:19, dcheng wrote:
> Nit: omit this, as the default constructor does this

Done.

https://codereview.chromium.org/2431473003/diff/290001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/RemoteFrameView.cpp (right):

https://codereview.chromium.org/2431473003/diff/290001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/RemoteFrameView.cpp:39:
!m_remoteFrame->client() || !m_remoteFrame->ownerLayoutObject())
On 2017/01/05 07:10:19, dcheng wrote:
> Can this be simplified to just checking if it has an ownerLayoutObject()? I
> think the last condition being true should imply all the others.

Done. I had concern with lifetime issues here, since this can get called while a
frame is getting detached from the tree, but if you are satisfied with this as
an invariant then I'm fine to simplify the check.

https://codereview.chromium.org/2431473003/diff/290001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/RemoteFrameView.cpp:123:
m_remoteFrame->client()->visibilityChanged(false);
On 2017/01/05 07:10:19, dcheng wrote:
> How come these need to null check while frameRectsChanged() doesn't? Do we
call
> this on something that's detached?

The checks on RemoteFrame::visibilityChanged() were in that method's initial
version from a year ago, but I don't see any problem running without them so I
have taken them out.

https://codereview.chromium.org/2431473003/diff/290001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutView.cpp (right):

https://codereview.chromium.org/2431473003/diff/290001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutView.cpp:481: // This is the case of
a Frame with a remote parent.
On 2017/01/05 20:25:22, szager1 wrote:
> On 2017/01/05 07:10:19, dcheng wrote:
> > While I'm OK with this patch as written, my original understanding is we'd
> make
> > these bits fairly agnostic to remote frames. Ideally, I'd like it to be so
if
> > ancestor is nullptr, we have a generic codepath for intersecting with the
> > viewport that's used for local and remote frames.
> 
> +1, it should be straightforward to add something to FrameView for this,
> something like:
> 
> Element* owner = document().localOwner();
> if (!owner)
>   return frameView()->mapToVisualRectInTopFrameSpace(rect);
> 

Done. I similarly moved the logic out of mapLocalToAncestor, above. Please let
me know if there is any concern with that.

https://codereview.chromium.org/2431473003/diff/290001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp (right):

https://codereview.chromium.org/2431473003/diff/290001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp:733: if
(m_localRoot->frameView()) {
On 2017/01/05 07:10:19, dcheng wrote:
> Why would it not have a frame view? IPC races?

Yes, I don't know how to justify these as invariants in cases where an update is
happening concurrently with frame detachment or a swap, for this and the
preceding comment.

https://codereview.chromium.org/2431473003/diff/290001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebWidget.h (right):

https://codereview.chromium.org/2431473003/diff/290001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebWidget.h:237: virtual void
setRemoteViewportIntersection(const WebRect&) {}
On 2017/01/05 07:10:20, dcheng wrote:
> Should we add this directly on WebFrameWidget instead? There doesn't seem to
be
> a valid case for sending it to any other type of widget.

Done.

[commit-bot: I haz the power, 2017-01-09 21:11:45]

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

[commit-bot: I haz the power, 2017-01-09 21:11:47]

Dry run: This issue passed the CQ dry run.

[dcheng, 2017-01-12 11:32:08]

Two more minor comments (sorry for missing them in the first pass).

At a high-level, I think the way I originally envisioned it was this:
- Frame would have some helper for intersecting with the viewport.
- LocalFrame would implement some version of it.
- RemoteFrame would implement some other version of it (which involves
consulting the remote view intersection rect if necessary).
- Then IO checks if it's intersecting with the viewport and does so first using
the helper interface, early returning. Then it goes through the other stuff
(which requires contiguous local frames, etc)

I'm not familiar enough with IO to know if this is feasible or not, but perhaps
something to think about for followups.

https://codereview.chromium.org/2431473003/diff/310001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp (right):

https://codereview.chromium.org/2431473003/diff/310001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp:731: if
(!m_localRoot->parent() || !m_localRoot->parent()->isWebRemoteFrame())
I would expect content to not send us the message incorrectly. Is it possible to
DCHECK() on these things instead?

https://codereview.chromium.org/2431473003/diff/310001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp:733: if
(m_localRoot->frameView()) {
Though I suppose this check is probably necessary due to potential races?

[kenrb, 2017-01-12 19:33:44]

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

[commit-bot: I haz the power, 2017-01-12 19:34:38]

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

[kenrb, 2017-01-12 19:51:11]

On 2017/01/12 11:32:08, dcheng wrote:
> Two more minor comments (sorry for missing them in the first pass).
> 
> At a high-level, I think the way I originally envisioned it was this:
> - Frame would have some helper for intersecting with the viewport.
> - LocalFrame would implement some version of it.
> - RemoteFrame would implement some other version of it (which involves
> consulting the remote view intersection rect if necessary).
> - Then IO checks if it's intersecting with the viewport and does so first
using
> the helper interface, early returning. Then it goes through the other stuff
> (which requires contiguous local frames, etc)
> 

I think there are a couple of issues with this. First, it isn't really useful to
have intersection data on a RemoteFrame that has a LocalFrame as a child,
because if it has multiple LocalFrame children, then they all have different
viewport intersections. So the LocalFrame (or FrameView, as it is implemented)
needs to know where it is when it is a local frame root.

More fundamentally, intersection calculations are done via upward recursion
through the layout tree, not the frame tree or DOM. There is no notion of remote
content in the layout tree, so at some point we have to break out and check for
an exception. As it stands, this CL adds some theoretically generic methods in
FrameView which account for remote viewport intersection, and the layout code
defers to these methods to do what needs to be done without having to know
whether anything is remote.

[kenrb, 2017-01-12 19:51:27]

https://codereview.chromium.org/2431473003/diff/310001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp (right):

https://codereview.chromium.org/2431473003/diff/310001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp:731: if
(!m_localRoot->parent() || !m_localRoot->parent()->isWebRemoteFrame())
On 2017/01/12 11:32:08, dcheng wrote:
> I would expect content to not send us the message incorrectly. Is it possible
to
> DCHECK() on these things instead?

Done.

https://codereview.chromium.org/2431473003/diff/310001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp:733: if
(m_localRoot->frameView()) {
On 2017/01/12 11:32:08, dcheng wrote:
> Though I suppose this check is probably necessary due to potential races?

Agreed.

[commit-bot: I haz the power, 2017-01-12 21:28:20]

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

[commit-bot: I haz the power, 2017-01-12 21:28:21]

Dry run: This issue passed the CQ dry run.

[dcheng, 2017-01-13 09:51:01]

On 2017/01/12 19:51:11, kenrb wrote:
> On 2017/01/12 11:32:08, dcheng wrote:
> > Two more minor comments (sorry for missing them in the first pass).
> > 
> > At a high-level, I think the way I originally envisioned it was this:
> > - Frame would have some helper for intersecting with the viewport.
> > - LocalFrame would implement some version of it.
> > - RemoteFrame would implement some other version of it (which involves
> > consulting the remote view intersection rect if necessary).
> > - Then IO checks if it's intersecting with the viewport and does so first
> using
> > the helper interface, early returning. Then it goes through the other stuff
> > (which requires contiguous local frames, etc)
> > 
> 
> I think there are a couple of issues with this. First, it isn't really useful
to
> have intersection data on a RemoteFrame that has a LocalFrame as a child,
> because if it has multiple LocalFrame children, then they all have different
> viewport intersections. So the LocalFrame (or FrameView, as it is implemented)
> needs to know where it is when it is a local frame root.

Oops, yeah, good point -- we'd never need to intersect with RemoteFrame
directly.

> 
> More fundamentally, intersection calculations are done via upward recursion
> through the layout tree, not the frame tree or DOM. There is no notion of
remote
> content in the layout tree, so at some point we have to break out and check
for
> an exception. As it stands, this CL adds some theoretically generic methods in
> FrameView which account for remote viewport intersection, and the layout code
> defers to these methods to do what needs to be done without having to know
> whether anything is remote.

At some point, it would be good to have a deep dive into this code to see if we
can improve the abstractions for handling OOPIF. I think I just don't know
layout / IO well enough to provide accurate guidance atm. This CL LGTM though.

[kenrb, 2017-01-13 15:05:26]

The CQ bit was checked by kenrb@chromium.org

[kenrb, 2017-01-13 15:05:26]

The patchset sent to the CQ was uploaded after l-g-t-m from nick@chromium.org,
szager@chromium.org
Link to the patchset: https://codereview.chromium.org/2431473003/#ps350001
(title: "dcheng comments addressed")

[commit-bot: I haz the power, 2017-01-13 15:05:43]

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, 2017-01-13 15:10:52]

CQ is committing da patch.

Bot data: {"patchset_id": 350001, "attempt_start_ts": 1484319926269440,
"parent_rev": "2c293ce393a71ddb86c7fe2a844536b5c95aec4b", "commit_rev":
"ea73179285a43f60275fd7a057d44bca6d4d9291"}

[commit-bot: I haz the power, 2017-01-13 15:11:25]

Description was changed from

==========
Intersection Observer support for OOPIF

Plumb a viewport intersection rect from a parent frame to any out of
process iframes that it embeds, and allow IntersectionObserver to
account for that rect when it computes viewport intersection.

This modifies LayoutView::MapToVisualRectInAncestorSpace and
mapLocalToAncestor to treat null ancestor pointers at implicit
references to the root frame, even if the root is remote.

For testing this enables the Intersection Observer layout tests that
were disabled with the --site-per-process flag.

BUG=615156
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Intersection Observer support for OOPIF

Plumb a viewport intersection rect from a parent frame to any out of
process iframes that it embeds, and allow IntersectionObserver to
account for that rect when it computes viewport intersection.

This modifies LayoutView::MapToVisualRectInAncestorSpace and
mapLocalToAncestor to treat null ancestor pointers at implicit
references to the root frame, even if the root is remote.

For testing this enables the Intersection Observer layout tests that
were disabled with the --site-per-process flag.

BUG=615156
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2431473003
Cr-Commit-Position: refs/heads/master@{#443559}
Committed:
https://chromium.googlesource.com/chromium/src/+/ea73179285a43f60275fd7a057d4...
==========

[commit-bot: I haz the power, 2017-01-13 15:11:27]

Committed patchset #19 (id:350001) as
https://chromium.googlesource.com/chromium/src/+/ea73179285a43f60275fd7a057d4...