Fix visual viewport for OOPIF.

Fix visual viewport for OOPIFs.

When called for a subframe,
RenderWidgetHostViewChildFrame::GetVisibleViewportSize currently
returns the subframe's size, yet this value is plumbed through
ResizeParams::visible_viewport_size and into
WebWidget::resizeVisualViewport, which resizes the visual viewport.
This appears wrong: visual viewport should be a page-level concept,
independent of which frame is asking for it.  From
WebWidget::resizeVisualViewport:

  // Normally the unscaled visual viewport is the same size as the
  // main frame.

This CL changes the plumbing to instead return the main frame bounds
in RWHVCF, and adds plumbing in WebFrameWidget to set the page's
visual viewport in resizeVisualViewport.  I hit this while working on
fullscreen: without this, fullscreening an element in an OOPIF
incorrectly sized it using the subframe's bounds rather than the main
frame's bounds (which had been resized to occupy the whole screen).

BUG=550497
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/69d253f82d3ebaa9d43253ffc630ebea94f874a0
Cr-Commit-Position: refs/heads/master@{#384597}

[alexmos, 2016-03-25 01:29:22]

Description was changed from

==========
Fix visual viewport for OOPIF.

BUG=550497
==========

to

==========
Fix visual viewport for OOPIF.

BUG=550497
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[alexmos, 2016-03-25 23:24:27]

Description was changed from

==========
Fix visual viewport for OOPIF.

BUG=550497
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Fix visual viewport for OOPIFs.

When called for a subframe,
RenderWidgetHostViewChildFrame::GetVisibleViewportSize currently
returns the subframe's size, yet this value is plumbed through
ResizeParams::visible_viewport_size and into
WebWidget::resizeVisualViewport, which resizes the visual viewport.
This appears wrong: visual viewport should be a page-level concept,
independent of which frame is asking for it.  From
WebWidget::resizeVisualViewport:

  // Normally the unscaled visual viewport is the same size as the
  // main frame.

This CL changes the plumbing to instead return the main frame bounds
in RWHVCF, and adds plumbing in WebFrameWidget to set the page's
visual viewport in resizeVisualViewport.  I hit this while working on
fullscreen: without this, fullscreening an element in an OOPIF
incorrectly sized it using the subframe's bounds rather than the main
frame's bounds (which had been resized to occupy the whole screen).

BUG=550497
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[alexmos, 2016-03-26 00:00:47]

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

[alexmos, 2016-03-26 00:00:48]

Hi Ken, can you please take a look at this when you're back?

This just splits off the visual viewport piece off of my fullscreen changes.  No
tests yet, since I didn't find any way this was exposed to JS in an OOPIF other
than fullscreen.  Once I finish a followup fullscreen CL, this should become
very easy to test, so I'm planning to cover this in the tests then.  Let me know
if that's ok.

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

https://codereview.chromium.org/1834913002/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_widget_host_view_child_frame.cc:133: // to be
a main frame.  This should be cleaned up eventually.
I'm not quite sure about this, and in general about what visual viewport means
for guests, so it'd be great to double-check this comment/logic.  I added the
|is_guest| exclusion because of tests like
WebViewTest.Shim_TestNavigateAfterResize/0.  With
--use-cross-process-frames-for-guests, this would return incorrect
window.innerWidth/Height for guest (embedder's size rather than guest's size)
without that exception.

For reference, here's the code from getViewportSize in LocalDOMWindow.cpp, which
is what's used for innerWidth/innerHeight:

    return frame->isMainFrame() && !host->settings().inertVisualViewport()
        ? FloatSize(host->visualViewport().visibleRect().size())
        : FloatSize(view->visibleContentRect(IncludeScrollbars).size());

For OOP subframes, we take the second path.  For a guest, we take the first
path.

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

https://codereview.chromium.org/1834913002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp:204:
page()->frameHost().visualViewport().clampToBoundaries();
Note that I didn't have to add any plumbing for sending resize messages for
OOPIFs, as this kind of happens for free today.  Here's the flow of events when
you resize the window:
- ViewMsg_Resize is sent to main frame's renderer
- eventually, this hits FrameView::frameRectsChanged(), which goes and calls
frameRectsChanged on its children and hits RemoteFrameView::frameRectsChanged()
for an OOPIF.
- that sends FrameHostMsg_FrameRectChanged back to browser via the proxy.
- browser calls CrossProcessFrameConnector::SetRect()
- that gets to RenderWidgetHostViewChildFrame::SetSize(), which calls
RenderWidgetHostImpl::WasResized().
- before this CL, no resize message would be sent because ResizeParams stayed
the same.  But now that RenderWidgetHostViewChildFrame::GetVisibleViewportSize()
returns a different viewport size (due to window resize), this proceeds to send
ViewMsg_Resize to the subframe widget.
- RenderWidget::Resize() calls WebWidget::resizeVisualViewport(), which ends up
here.

We could probably optimize this in the future to avoid the extra roundtrip.

[kenrb, 2016-03-31 17:41:58]

lgtm, but I've cc'd James to see if he has any insight on what visual viewport
means to webview. I'm curious about that also.

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

https://codereview.chromium.org/1834913002/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_widget_host_view_child_frame.cc:133: // to be
a main frame.  This should be cleaned up eventually.
On 2016/03/26 00:00:48, alexmos wrote:
> I'm not quite sure about this, and in general about what visual viewport means
> for guests, so it'd be great to double-check this comment/logic.  I added the
> |is_guest| exclusion because of tests like
> WebViewTest.Shim_TestNavigateAfterResize/0.  With
> --use-cross-process-frames-for-guests, this would return incorrect
> window.innerWidth/Height for guest (embedder's size rather than guest's size)
> without that exception.
> 
> For reference, here's the code from getViewportSize in LocalDOMWindow.cpp,
which
> is what's used for innerWidth/innerHeight:
> 
>     return frame->isMainFrame() && !host->settings().inertVisualViewport()
>         ? FloatSize(host->visualViewport().visibleRect().size())
>         : FloatSize(view->visibleContentRect(IncludeScrollbars).size());
> 
> For OOP subframes, we take the second path.  For a guest, we take the first
> path.

I don't quite understand what having a separate visual viewport for a guest
means, either. This seems reasonable to me, but do we expect to be able to
separately scale a guest from its embedder?

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

https://codereview.chromium.org/1834913002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp:204:
page()->frameHost().visualViewport().clampToBoundaries();
On 2016/03/26 00:00:48, alexmos wrote:
> Note that I didn't have to add any plumbing for sending resize messages for
> OOPIFs, as this kind of happens for free today.  Here's the flow of events
when
> you resize the window:
> - ViewMsg_Resize is sent to main frame's renderer
> - eventually, this hits FrameView::frameRectsChanged(), which goes and calls
> frameRectsChanged on its children and hits
RemoteFrameView::frameRectsChanged()
> for an OOPIF.
> - that sends FrameHostMsg_FrameRectChanged back to browser via the proxy.
> - browser calls CrossProcessFrameConnector::SetRect()
> - that gets to RenderWidgetHostViewChildFrame::SetSize(), which calls
> RenderWidgetHostImpl::WasResized().
> - before this CL, no resize message would be sent because ResizeParams stayed
> the same.  But now that
RenderWidgetHostViewChildFrame::GetVisibleViewportSize()
> returns a different viewport size (due to window resize), this proceeds to
send
> ViewMsg_Resize to the subframe widget.
> - RenderWidget::Resize() calls WebWidget::resizeVisualViewport(), which ends
up
> here.
> 
> We could probably optimize this in the future to avoid the extra roundtrip.

Eventually we might want to change some of the resizing behavior to Page
messages. It doesn't seem like the visual viewport should be set once for each
local root. This isn't a high priority, but I might suggest filing a bug for
that so we can get around to it at some point.

[wjmaclean, 2016-03-31 18:43:08]

On 2016/03/31 17:41:58, kenrb wrote:
> lgtm, but I've cc'd James to see if he has any insight on what visual viewport
> means to webview. I'm curious about that also.
> 
>
https://codereview.chromium.org/1834913002/diff/20001/content/browser/frame_h...
> File content/browser/frame_host/render_widget_host_view_child_frame.cc
(right):
> 
>
https://codereview.chromium.org/1834913002/diff/20001/content/browser/frame_h...
> content/browser/frame_host/render_widget_host_view_child_frame.cc:133: // to
be
> a main frame.  This should be cleaned up eventually.
> On 2016/03/26 00:00:48, alexmos wrote:
> > I'm not quite sure about this, and in general about what visual viewport
means
> > for guests, so it'd be great to double-check this comment/logic.  I added
the
> > |is_guest| exclusion because of tests like
> > WebViewTest.Shim_TestNavigateAfterResize/0.  With
> > --use-cross-process-frames-for-guests, this would return incorrect
> > window.innerWidth/Height for guest (embedder's size rather than guest's
size)
> > without that exception.
> > 
> > For reference, here's the code from getViewportSize in LocalDOMWindow.cpp,
> which
> > is what's used for innerWidth/innerHeight:
> > 
> >     return frame->isMainFrame() && !host->settings().inertVisualViewport()
> >         ? FloatSize(host->visualViewport().visibleRect().size())
> >         : FloatSize(view->visibleContentRect(IncludeScrollbars).size());
> > 
> > For OOP subframes, we take the second path.  For a guest, we take the first
> > path.
> 
> I don't quite understand what having a separate visual viewport for a guest
> means, either. This seems reasonable to me, but do we expect to be able to
> separately scale a guest from its embedder?
> 
>
https://codereview.chromium.org/1834913002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp (right):
> 
>
https://codereview.chromium.org/1834913002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp:204:
> page()->frameHost().visualViewport().clampToBoundaries();
> On 2016/03/26 00:00:48, alexmos wrote:
> > Note that I didn't have to add any plumbing for sending resize messages for
> > OOPIFs, as this kind of happens for free today.  Here's the flow of events
> when
> > you resize the window:
> > - ViewMsg_Resize is sent to main frame's renderer
> > - eventually, this hits FrameView::frameRectsChanged(), which goes and calls
> > frameRectsChanged on its children and hits
> RemoteFrameView::frameRectsChanged()
> > for an OOPIF.
> > - that sends FrameHostMsg_FrameRectChanged back to browser via the proxy.
> > - browser calls CrossProcessFrameConnector::SetRect()
> > - that gets to RenderWidgetHostViewChildFrame::SetSize(), which calls
> > RenderWidgetHostImpl::WasResized().
> > - before this CL, no resize message would be sent because ResizeParams
stayed
> > the same.  But now that
> RenderWidgetHostViewChildFrame::GetVisibleViewportSize()
> > returns a different viewport size (due to window resize), this proceeds to
> send
> > ViewMsg_Resize to the subframe widget.
> > - RenderWidget::Resize() calls WebWidget::resizeVisualViewport(), which ends
> up
> > here.
> > 
> > We could probably optimize this in the future to avoid the extra roundtrip.
> 
> Eventually we might want to change some of the resizing behavior to Page
> messages. It doesn't seem like the visual viewport should be set once for each
> local root. This isn't a high priority, but I might suggest filing a bug for
> that so we can get around to it at some point.

I think once we're at the level of RWHVCF we're at the interface between guest
and embedder, and the notion of "What is the embedder's visual viewport" becomes
relevant. Within the guest I would imagine it should be generally oblivious and
think it has its own visual viewport (it considers itself "top-level").

Having said all that, I'm not really all that sure ... I've never really thought
about this distinction before.

[alexmos, 2016-03-31 23:03:48]

Thanks!

On 2016/03/31 18:43:08, wjmaclean wrote:
> On 2016/03/31 17:41:58, kenrb wrote:
> > lgtm, but I've cc'd James to see if he has any insight on what visual
viewport
> > means to webview. I'm curious about that also.
> > 
> >
>
https://codereview.chromium.org/1834913002/diff/20001/content/browser/frame_h...
> > File content/browser/frame_host/render_widget_host_view_child_frame.cc
> (right):
> > 
> >
>
https://codereview.chromium.org/1834913002/diff/20001/content/browser/frame_h...
> > content/browser/frame_host/render_widget_host_view_child_frame.cc:133: // to
> be
> > a main frame.  This should be cleaned up eventually.
> > On 2016/03/26 00:00:48, alexmos wrote:
> > > I'm not quite sure about this, and in general about what visual viewport
> means
> > > for guests, so it'd be great to double-check this comment/logic.  I added
> the
> > > |is_guest| exclusion because of tests like
> > > WebViewTest.Shim_TestNavigateAfterResize/0.  With
> > > --use-cross-process-frames-for-guests, this would return incorrect
> > > window.innerWidth/Height for guest (embedder's size rather than guest's
> size)
> > > without that exception.
> > > 
> > > For reference, here's the code from getViewportSize in LocalDOMWindow.cpp,
> > which
> > > is what's used for innerWidth/innerHeight:
> > > 
> > >     return frame->isMainFrame() && !host->settings().inertVisualViewport()
> > >         ? FloatSize(host->visualViewport().visibleRect().size())
> > >         : FloatSize(view->visibleContentRect(IncludeScrollbars).size());
> > > 
> > > For OOP subframes, we take the second path.  For a guest, we take the
first
> > > path.
> > 
> > I don't quite understand what having a separate visual viewport for a guest
> > means, either. This seems reasonable to me, but do we expect to be able to
> > separately scale a guest from its embedder?
> > 
>
> I think once we're at the level of RWHVCF we're at the interface between guest
> and embedder, and the notion of "What is the embedder's visual viewport"
becomes
> relevant. Within the guest I would imagine it should be generally oblivious
and
> think it has its own visual viewport (it considers itself "top-level").
> 
> Having said all that, I'm not really all that sure ... I've never really
thought
> about this distinction before.

OK, it sounds like the current code might be ok, so I'll leave it as-is for now.
 I'd be surprised if we wanted to scale a guest separately from its embedder
though, so maybe eventually we can clean this up so that there's one viewport
and fix Blink to handle things like innerHeight correctly for guests.

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

https://codereview.chromium.org/1834913002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp:204:
page()->frameHost().visualViewport().clampToBoundaries();
On 2016/03/31 17:41:58, kenrb wrote:
> On 2016/03/26 00:00:48, alexmos wrote:
> > Note that I didn't have to add any plumbing for sending resize messages for
> > OOPIFs, as this kind of happens for free today.  Here's the flow of events
> when
> > you resize the window:
> > - ViewMsg_Resize is sent to main frame's renderer
> > - eventually, this hits FrameView::frameRectsChanged(), which goes and calls
> > frameRectsChanged on its children and hits
> RemoteFrameView::frameRectsChanged()
> > for an OOPIF.
> > - that sends FrameHostMsg_FrameRectChanged back to browser via the proxy.
> > - browser calls CrossProcessFrameConnector::SetRect()
> > - that gets to RenderWidgetHostViewChildFrame::SetSize(), which calls
> > RenderWidgetHostImpl::WasResized().
> > - before this CL, no resize message would be sent because ResizeParams
stayed
> > the same.  But now that
> RenderWidgetHostViewChildFrame::GetVisibleViewportSize()
> > returns a different viewport size (due to window resize), this proceeds to
> send
> > ViewMsg_Resize to the subframe widget.
> > - RenderWidget::Resize() calls WebWidget::resizeVisualViewport(), which ends
> up
> > here.
> > 
> > We could probably optimize this in the future to avoid the extra roundtrip.
> 
> Eventually we might want to change some of the resizing behavior to Page
> messages. It doesn't seem like the visual viewport should be set once for each
> local root. This isn't a high priority, but I might suggest filing a bug for
> that so we can get around to it at some point.

Good point.  I suppose this means that in cases like A-B-A we might have some
redundant updates.  I filed https://crbug.com/599688 and referenced here.

[alexmos, 2016-03-31 23:05:30]

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

[alexmos, 2016-03-31 23:05:31]

Daniel, can you please review the Blink side for owners?

[dcheng, 2016-03-31 23:06:51]

lgtm

[dcheng, 2016-03-31 23:07:55]

https://codereview.chromium.org/1834913002/diff/60001/content/browser/frame_h...
File content/browser/frame_host/render_widget_host_view_child_frame.cc (right):

https://codereview.chromium.org/1834913002/diff/60001/content/browser/frame_h...
content/browser/frame_host/render_widget_host_view_child_frame.cc:135:
static_cast<RenderViewHostImpl*>(RenderViewHost::From(host_)));
By the way, I think you can write RenderViewHostImpl::From(host_) and avoid the
cast.

[alexmos, 2016-04-01 00:44:54]

Thanks!

https://codereview.chromium.org/1834913002/diff/60001/content/browser/frame_h...
File content/browser/frame_host/render_widget_host_view_child_frame.cc (right):

https://codereview.chromium.org/1834913002/diff/60001/content/browser/frame_h...
content/browser/frame_host/render_widget_host_view_child_frame.cc:135:
static_cast<RenderViewHostImpl*>(RenderViewHost::From(host_)));
On 2016/03/31 23:07:54, dcheng wrote:
> By the way, I think you can write RenderViewHostImpl::From(host_) and avoid
the
> cast.

Done.

[alexmos, 2016-04-01 00:45:17]

The CQ bit was checked by alexmos@chromium.org

[alexmos, 2016-04-01 00:45:18]

The patchset sent to the CQ was uploaded after l-g-t-m from kenrb@chromium.org,
dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/1834913002/#ps80001
(title: "Remove unnecessary cast")

[commit-bot: I haz the power, 2016-04-01 00:45:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1834913002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1834913002/80001

[commit-bot: I haz the power, 2016-04-01 06:44:27]

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

[commit-bot: I haz the power, 2016-04-01 06:44:27]

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

[alexmos, 2016-04-01 14:31:31]

The CQ bit was checked by alexmos@chromium.org

[commit-bot: I haz the power, 2016-04-01 14:31:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1834913002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1834913002/80001

[commit-bot: I haz the power, 2016-04-01 15:57:02]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-04-01 15:59:27]

Description was changed from

==========
Fix visual viewport for OOPIFs.

When called for a subframe,
RenderWidgetHostViewChildFrame::GetVisibleViewportSize currently
returns the subframe's size, yet this value is plumbed through
ResizeParams::visible_viewport_size and into
WebWidget::resizeVisualViewport, which resizes the visual viewport.
This appears wrong: visual viewport should be a page-level concept,
independent of which frame is asking for it.  From
WebWidget::resizeVisualViewport:

  // Normally the unscaled visual viewport is the same size as the
  // main frame.

This CL changes the plumbing to instead return the main frame bounds
in RWHVCF, and adds plumbing in WebFrameWidget to set the page's
visual viewport in resizeVisualViewport.  I hit this while working on
fullscreen: without this, fullscreening an element in an OOPIF
incorrectly sized it using the subframe's bounds rather than the main
frame's bounds (which had been resized to occupy the whole screen).

BUG=550497
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Fix visual viewport for OOPIFs.

When called for a subframe,
RenderWidgetHostViewChildFrame::GetVisibleViewportSize currently
returns the subframe's size, yet this value is plumbed through
ResizeParams::visible_viewport_size and into
WebWidget::resizeVisualViewport, which resizes the visual viewport.
This appears wrong: visual viewport should be a page-level concept,
independent of which frame is asking for it.  From
WebWidget::resizeVisualViewport:

  // Normally the unscaled visual viewport is the same size as the
  // main frame.

This CL changes the plumbing to instead return the main frame bounds
in RWHVCF, and adds plumbing in WebFrameWidget to set the page's
visual viewport in resizeVisualViewport.  I hit this while working on
fullscreen: without this, fullscreening an element in an OOPIF
incorrectly sized it using the subframe's bounds rather than the main
frame's bounds (which had been resized to occupy the whole screen).

BUG=550497
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/69d253f82d3ebaa9d43253ffc630ebea94f874a0
Cr-Commit-Position: refs/heads/master@{#384597}
==========

[commit-bot: I haz the power, 2016-04-01 15:59:28]

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