Propagate viewport intersection across OOPIFs at the correct time

Propagate viewport intersection across OOPIFs at the correct time

This corrects a couple of issues with how viewport intersection rects
were being sent to OOPIF processes from their parents, which was causing
frame throttling bugs:

1. The update is now triggered from the parent FrameView's
UpdateViewportIntersectionsForSubtree, rather than the RemoteFrameView's
frameRectsChanged. Formerly, this would cause intersection calculations
in a dirty layout tree, and also fail to propagate intersection rect
changes in the case of nested OOPIFs.

2. Viewport intersections are now updated properly when the iframe
is scrolled off of screen. Previously it would return early when an
OOPIF had no viewport intersection, which was wrong.

BUG=682307, 712320
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2860903003
Cr-Commit-Position: refs/heads/master@{#469977}
Committed: https://chromium.googlesource.com/chromium/src/+/321a171abcca381e3ef72d01996e4ae4dd860212

[kenrb, 2017-05-04 16:30:49]

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

[commit-bot: I haz the power, 2017-05-04 16:31: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, 2017-05-04 16:34:51]

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

[commit-bot: I haz the power, 2017-05-04 16:34:55]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[kenrb, 2017-05-04 16:43:01]

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

[commit-bot: I haz the power, 2017-05-04 16:43:45]

Dry run: CQ is trying da patch.

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

[kenrb, 2017-05-04 17:27:43]

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

[kenrb, 2017-05-04 17:27:44]

szager@: PTAL?

The test is an adapted version of cross-origin-subframe.html.

[commit-bot: I haz the power, 2017-05-04 18:04:41]

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

[commit-bot: I haz the power, 2017-05-04 18:04:42]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[szager1, 2017-05-04 19:26:06]

https://codereview.chromium.org/2860903003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/RemoteFrameView.cpp (right):

https://codereview.chromium.org/2860903003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/RemoteFrameView.cpp:62: IntRect
viewport_intersection;
Should this be initialized to last_viewport_intersection_?

[kenrb, 2017-05-04 19:47:10]

https://codereview.chromium.org/2860903003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/RemoteFrameView.cpp (right):

https://codereview.chromium.org/2860903003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/RemoteFrameView.cpp:62: IntRect
viewport_intersection;
On 2017/05/04 19:26:06, szager1 wrote:
> Should this be initialized to last_viewport_intersection_?

I don't think so. It should be zero-initialized, because it is used if the
iframe is not in the viewport (i.e. the next call to
MapToVisualRectInAncestorSpace() returns false). If the iframe has just been
scrolled out of view, we need it to be different from last_viewport_intersection
so the update is sent to the other process.

[szager1, 2017-05-04 20:29:46]

lgtm, sorry the tests are so hard to write :(

You still need OWNERS lgtm

[kenrb, 2017-05-04 20:45:29]

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

[kenrb, 2017-05-04 20:45:30]

Thanks for the review.

dcheng@: PTAL as a Blink OWNER? Feel free to punt if you are too busy.

[dcheng, 2017-05-05 18:25:14]

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

https://codereview.chromium.org/2860903003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.cpp:4940:
DCHECK(child->IsRemoteFrame() || child->IsLocalFrame());
Isn't this DCHECK always going to be true?

https://codereview.chromium.org/2860903003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.cpp:4943:
view->UpdateRemoteViewportIntersection();
It almost feels like View() and <update intersections> should be virtuals.
*shrug*

[kenrb, 2017-05-05 19:01:31]

dcheng: Patch updated to remove the DCHECK.

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

https://codereview.chromium.org/2860903003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.cpp:4940:
DCHECK(child->IsRemoteFrame() || child->IsLocalFrame());
On 2017/05/05 18:25:14, dcheng wrote:
> Isn't this DCHECK always going to be true?

Aren't *all* DCHECKs supposed to be always true? :)

I see your point, though. Violating that would be a significant change to the
Frame code and would break Blink in a lot of ways. I'll take it out.

https://codereview.chromium.org/2860903003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.cpp:4943:
view->UpdateRemoteViewportIntersection();
On 2017/05/05 18:25:14, dcheng wrote:
> It almost feels like View() and <update intersections> should be virtuals.
> *shrug*

That might be reasonable for View(). For UpdateViewportIntersections I think the
difference in function arguments makes it more trouble than it's worth.

[dcheng, 2017-05-05 22:47:53]

LGTM

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

https://codereview.chromium.org/2860903003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.cpp:4943:
view->UpdateRemoteViewportIntersection();
On 2017/05/05 19:01:31, kenrb wrote:
> On 2017/05/05 18:25:14, dcheng wrote:
> > It almost feels like View() and <update intersections> should be virtuals.
> > *shrug*
> 
> That might be reasonable for View(). For UpdateViewportIntersections I think
the
> difference in function arguments makes it more trouble than it's worth.

Do you mind poking at this in a followup?

FWIW, I don't think combining the methods for RemoteFrameView/FrameView is that
bad: it's just one parameter to ignore, and it removes explicit downcasts from
the codebase. I think the explicit downcasts have been a pretty big headache
overall, and using virtual dispatch will result in overall simpler to reason
about code.

[kenrb, 2017-05-08 12:23:14]

On 2017/05/05 22:47:53, dcheng wrote:
> LGTM
> 
>
https://codereview.chromium.org/2860903003/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/frame/FrameView.cpp (right):
> 
>
https://codereview.chromium.org/2860903003/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/frame/FrameView.cpp:4943:
> view->UpdateRemoteViewportIntersection();
> On 2017/05/05 19:01:31, kenrb wrote:
> > On 2017/05/05 18:25:14, dcheng wrote:
> > > It almost feels like View() and <update intersections> should be virtuals.
> > > *shrug*
> > 
> > That might be reasonable for View(). For UpdateViewportIntersections I think
> the
> > difference in function arguments makes it more trouble than it's worth.
> 
> Do you mind poking at this in a followup?
> 
> FWIW, I don't think combining the methods for RemoteFrameView/FrameView is
that
> bad: it's just one parameter to ignore, and it removes explicit downcasts from
> the codebase. I think the explicit downcasts have been a pretty big headache
> overall, and using virtual dispatch will result in overall simpler to reason
> about code.

Filed https://bugs.chromium.org/p/chromium/issues/detail?id=719439.

[kenrb, 2017-05-08 12:23:20]

The CQ bit was checked by kenrb@chromium.org

[kenrb, 2017-05-08 12:23:20]

The patchset sent to the CQ was uploaded after l-g-t-m from szager@chromium.org
Link to the patchset: https://codereview.chromium.org/2860903003/#ps40001
(title: "DCHECK removed.")

[commit-bot: I haz the power, 2017-05-08 12:23:34]

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-05-08 14:13:36]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1494246200692970,
"parent_rev": "ce35edf5e480e4ac79316f9d2fd6ecd5b5812d72", "commit_rev":
"321a171abcca381e3ef72d01996e4ae4dd860212"}

[commit-bot: I haz the power, 2017-05-08 14:13:49]

Description was changed from

==========
Propagate viewport intersection across OOPIFs at the correct time

This corrects a couple of issues with how viewport intersection rects
were being sent to OOPIF processes from their parents, which was causing
frame throttling bugs:

1. The update is now triggered from the parent FrameView's
UpdateViewportIntersectionsForSubtree, rather than the RemoteFrameView's
frameRectsChanged. Formerly, this would cause intersection calculations
in a dirty layout tree, and also fail to propagate intersection rect
changes in the case of nested OOPIFs.

2. Viewport intersections are now updated properly when the iframe
is scrolled off of screen. Previously it would return early when an
OOPIF had no viewport intersection, which was wrong.

BUG=682307, 712320
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Propagate viewport intersection across OOPIFs at the correct time

This corrects a couple of issues with how viewport intersection rects
were being sent to OOPIF processes from their parents, which was causing
frame throttling bugs:

1. The update is now triggered from the parent FrameView's
UpdateViewportIntersectionsForSubtree, rather than the RemoteFrameView's
frameRectsChanged. Formerly, this would cause intersection calculations
in a dirty layout tree, and also fail to propagate intersection rect
changes in the case of nested OOPIFs.

2. Viewport intersections are now updated properly when the iframe
is scrolled off of screen. Previously it would return early when an
OOPIF had no viewport intersection, which was wrong.

BUG=682307, 712320
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2860903003
Cr-Commit-Position: refs/heads/master@{#469977}
Committed:
https://chromium.googlesource.com/chromium/src/+/321a171abcca381e3ef72d01996e...
==========

[commit-bot: I haz the power, 2017-05-08 14:13:50]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/321a171abcca381e3ef72d01996e...