Ensure RemoteViewportIntersection rect persists across frame navigations

Ensure RemoteViewportIntersection rect persists across frame navigations

To prevent excessive IPC messages, a parent frame of an OOPIF only
updates its remote viewport intersection rect when the intersection
changes. However, a frame can navigate without the parent frame being
aware of it, in which case the rect is lost without being resent.

To address that problem, this CL caches the rect on
CrossProcessFrameConnector in the browser process, and sends it to the
renderer whenever RenderWidgetHostImpl::SendScreenRects() is called.

This CL also moves the rect's storage in Blink from LocalFrameView to
LocalFrame so that it persists across same-process navigations.

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

Review-Url: https://codereview.chromium.org/2910023002
Cr-Commit-Position: refs/heads/master@{#475756}
Committed: https://chromium.googlesource.com/chromium/src/+/1e68f60e90fc7b0b12bbf884291458e5e4aa3b2f

[kenrb, 2017-05-29 22:14:36]

Description was changed from

==========
Ensure RemoteViewportIntersection rect persists across frame navigations

To prevent excessive IPC messages, a parent frame of an OOPIF only
updates its remote viewport intersection rect when the intersection
changes. However, a frame can navigate without the parent frame being
aware of it, in which case the rect is lost without being resent.

To address that problem, this CL caches the rect on
CrossProcessFrameConnector in the browser process, and sends it to the
renderer whenever RenderWidgetHostImpl::SendScreenRects() is called.

This CL also moves the rect's storage in Blink from LocalFrameView to
LocalFrame so that it persists across same-process navigations.

BUG=720342
==========

to

==========
Ensure RemoteViewportIntersection rect persists across frame navigations

To prevent excessive IPC messages, a parent frame of an OOPIF only
updates its remote viewport intersection rect when the intersection
changes. However, a frame can navigate without the parent frame being
aware of it, in which case the rect is lost without being resent.

To address that problem, this CL caches the rect on
CrossProcessFrameConnector in the browser process, and sends it to the
renderer whenever RenderWidgetHostImpl::SendScreenRects() is called.

This CL also moves the rect's storage in Blink from LocalFrameView to
LocalFrame so that it persists across same-process navigations.

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

[kenrb, 2017-05-29 22:14:48]

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

[commit-bot: I haz the power, 2017-05-29 22:15: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, 2017-05-30 00:02:39]

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

[commit-bot: I haz the power, 2017-05-30 00:02:40]

Dry run: This issue passed the CQ dry run.

[kenrb, 2017-05-30 01:48:33]

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

[kenrb, 2017-05-30 01:48:33]

PTAL?
 - dcheng for Blink
 - ncarter for content/

[ncarter (slow), 2017-05-30 17:51:09]

lgtm

https://codereview.chromium.org/2910023002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/cross_process_frame_connector.h (right):

https://codereview.chromium.org/2910023002/diff/1/content/browser/frame_host/...
content/browser/frame_host/cross_process_frame_connector.h:31: class
MockCrossProcessFrameConnector;
 "friend class" doesn't actually require forward declaration, so you can drop
this line.

https://codereview.chromium.org/2910023002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_view_base.h (right):

https://codereview.chromium.org/2910023002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_base.h:322: virtual void
OnSendScreenRects() {}
Optional: "WillSendScreenRects" would be my pick to name this method. To me at
least, "OnX" connotes having gotten a legacy IPC message named X, though there
are certainly already a bunch of methods in this class (OnSurfaceChanged) that
don't follow that pattern -- so I am ok sticking with the On name if we want to
be consistent with those.

[kenrb, 2017-05-30 18:25:01]

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

[kenrb, 2017-05-30 18:25:10]

Thanks.

https://codereview.chromium.org/2910023002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/cross_process_frame_connector.h (right):

https://codereview.chromium.org/2910023002/diff/1/content/browser/frame_host/...
content/browser/frame_host/cross_process_frame_connector.h:31: class
MockCrossProcessFrameConnector;
On 2017/05/30 17:51:09, ncarter (slow) wrote:
>  "friend class" doesn't actually require forward declaration, so you can drop
> this line.

Done.

https://codereview.chromium.org/2910023002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_view_base.h (right):

https://codereview.chromium.org/2910023002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_base.h:322: virtual void
OnSendScreenRects() {}
On 2017/05/30 17:51:09, ncarter (slow) wrote:
> Optional: "WillSendScreenRects" would be my pick to name this method. To me at
> least, "OnX" connotes having gotten a legacy IPC message named X, though there
> are certainly already a bunch of methods in this class (OnSurfaceChanged) that
> don't follow that pattern -- so I am ok sticking with the On name if we want
to
> be consistent with those.

That makes sense. Done.

[commit-bot: I haz the power, 2017-05-30 18:25:38]

Dry run: CQ is trying da patch.

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

[dcheng, 2017-05-30 19:54:11]

https://codereview.chromium.org/2910023002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/cross_process_frame_connector.h (right):

https://codereview.chromium.org/2910023002/diff/20001/content/browser/frame_h...
content/browser/frame_host/cross_process_frame_connector.h:136: gfx::Rect
viewport_intersection() const {
Nit: consider returning a const ref to avoid a copy

https://codereview.chromium.org/2910023002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/render_widget_host_view_child_frame_unittest.cc
(right):

https://codereview.chromium.org/2910023002/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_widget_host_view_child_frame_unittest.cc:68:
void SetViewportIntersection(gfx::Rect intersection) {
Ditto: const ref

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

https://codereview.chromium.org/2910023002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/LocalFrame.cpp:1209:
View()->ScheduleAnimation();
Btw, why do we need to call ScheduleAnimation() here?

https://codereview.chromium.org/2910023002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/LocalFrame.h (right):

https://codereview.chromium.org/2910023002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/LocalFrame.h:315: IntRect
remote_viewport_intersection_;
It feels a little weird to have layout information on the LocalFrame, but I
guess you could argue it's similar-ish to zoom factors.

[commit-bot: I haz the power, 2017-05-30 20:18:58]

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

[commit-bot: I haz the power, 2017-05-30 20:18:59]

Dry run: This issue passed the CQ dry run.

[kenrb, 2017-05-30 21:19:05]

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

[kenrb, 2017-05-30 21:19:21]

https://codereview.chromium.org/2910023002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/cross_process_frame_connector.h (right):

https://codereview.chromium.org/2910023002/diff/20001/content/browser/frame_h...
content/browser/frame_host/cross_process_frame_connector.h:136: gfx::Rect
viewport_intersection() const {
On 2017/05/30 19:54:11, dcheng wrote:
> Nit: consider returning a const ref to avoid a copy

Done.

https://codereview.chromium.org/2910023002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/render_widget_host_view_child_frame_unittest.cc
(right):

https://codereview.chromium.org/2910023002/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_widget_host_view_child_frame_unittest.cc:68:
void SetViewportIntersection(gfx::Rect intersection) {
On 2017/05/30 19:54:11, dcheng wrote:
> Ditto: const ref

Done.

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

https://codereview.chromium.org/2910023002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/LocalFrame.cpp:1209:
View()->ScheduleAnimation();
On 2017/05/30 19:54:11, dcheng wrote:
> Btw, why do we need to call ScheduleAnimation() here?

It causes intersection recalculations through all the FrameViews. It affects
frame throttling, as well as causing updated viewport intersection rects to be
sent to any OOPIFs nested within this one.

https://codereview.chromium.org/2910023002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/LocalFrame.h (right):

https://codereview.chromium.org/2910023002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/LocalFrame.h:315: IntRect
remote_viewport_intersection_;
On 2017/05/30 19:54:11, dcheng wrote:
> It feels a little weird to have layout information on the LocalFrame, but I
> guess you could argue it's similar-ish to zoom factors.

Similar idea, yes. I think frame-specific values that are set from external to
Blink end up needing to live on LocalFrame, even if they are inputs to layout or
painting, since LocalFrame survives navigations but LocalFrameView does not.

[commit-bot: I haz the power, 2017-05-30 21:19:52]

Dry run: CQ is trying da patch.

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

[dcheng, 2017-05-30 22:14:26]

LGTM

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

https://codereview.chromium.org/2910023002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/LocalFrame.cpp:1209:
View()->ScheduleAnimation();
On 2017/05/30 21:19:21, kenrb wrote:
> On 2017/05/30 19:54:11, dcheng wrote:
> > Btw, why do we need to call ScheduleAnimation() here?
> 
> It causes intersection recalculations through all the FrameViews. It affects
> frame throttling, as well as causing updated viewport intersection rects to be
> sent to any OOPIFs nested within this one.

I guess the name just feels a little funny.

https://codereview.chromium.org/2910023002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp (right):

https://codereview.chromium.org/2910023002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp:769: if
(local_root_->GetFrame()) {
Hmm... I wonder if we can get this when there's no attached frame. Do you mind
poking at this in a followup and trying to DCHECK() these instead?

[commit-bot: I haz the power, 2017-05-31 01:31:53]

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

[commit-bot: I haz the power, 2017-05-31 01:31:54]

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-05-31 01:37:29]

Thanks for the reviews.

https://codereview.chromium.org/2910023002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp (right):

https://codereview.chromium.org/2910023002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp:769: if
(local_root_->GetFrame()) {
On 2017/05/30 22:14:26, dcheng wrote:
> Hmm... I wonder if we can get this when there's no attached frame. Do you mind
> poking at this in a followup and trying to DCHECK() these instead?

I'll take a look.

[kenrb, 2017-05-31 01:37:38]

The CQ bit was checked by kenrb@chromium.org

[kenrb, 2017-05-31 01:37:39]

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

[commit-bot: I haz the power, 2017-05-31 01:38:22]

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-31 02:47:43]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1496194658807110,
"parent_rev": "35ec955d7d925e1daa08b0944790cc689016add9", "commit_rev":
"1e68f60e90fc7b0b12bbf884291458e5e4aa3b2f"}

[commit-bot: I haz the power, 2017-05-31 02:47:54]

Description was changed from

==========
Ensure RemoteViewportIntersection rect persists across frame navigations

To prevent excessive IPC messages, a parent frame of an OOPIF only
updates its remote viewport intersection rect when the intersection
changes. However, a frame can navigate without the parent frame being
aware of it, in which case the rect is lost without being resent.

To address that problem, this CL caches the rect on
CrossProcessFrameConnector in the browser process, and sends it to the
renderer whenever RenderWidgetHostImpl::SendScreenRects() is called.

This CL also moves the rect's storage in Blink from LocalFrameView to
LocalFrame so that it persists across same-process navigations.

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

to

==========
Ensure RemoteViewportIntersection rect persists across frame navigations

To prevent excessive IPC messages, a parent frame of an OOPIF only
updates its remote viewport intersection rect when the intersection
changes. However, a frame can navigate without the parent frame being
aware of it, in which case the rect is lost without being resent.

To address that problem, this CL caches the rect on
CrossProcessFrameConnector in the browser process, and sends it to the
renderer whenever RenderWidgetHostImpl::SendScreenRects() is called.

This CL also moves the rect's storage in Blink from LocalFrameView to
LocalFrame so that it persists across same-process navigations.

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

Review-Url: https://codereview.chromium.org/2910023002
Cr-Commit-Position: refs/heads/master@{#475756}
Committed:
https://chromium.googlesource.com/chromium/src/+/1e68f60e90fc7b0b12bbf8842914...
==========

[commit-bot: I haz the power, 2017-05-31 02:47:55]

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