Propagate auto-resize viewport values to OOPIF processes

Propagate auto-resize viewport values to OOPIF processes

RenderWidgetHostViewChildFrame currently gets its visible viewport size
from the top-level RenderWidgetHostView, which typically comes from the
platform window. However, when the top-level frame has auto-resize
enabled, internally it sets its own visible viewport, and this
information not available to OOPIF children. This causes issues
in particular for extension popups which have window sizes of (0, 0)
and rely on auto-resize to modify the viewport in Blink.

This patch causes the WebContents to store the auto-resize value
reported by the top-level frame and make it available to OOPIF
RenderWidgetHostViews. They, in turn, allow an auto-resize size to
override the windows size when determining their visible viewport.

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

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

[kenrb, 2017-06-01 21:11:57]

Description was changed from

==========
Propagate auto-resize viewport values to OOPIF processes

RenderWidgetHostViewChildFrame currently gets its visible viewport size
from the top-level RenderWidgetHostView, which typically comes from the
platform window. However, when the top-level frame has auto-resize
enabled, internally it sets its own visible viewport, and this
information not available to OOPIF children. This causes issues
in particular for extension popups which have window sizes of (0, 0)
and rely on auto-resize to modify the viewport in Blink.

This patch causes the WebContents to store the auto-resize value
reported by the top-level frame and make it available to OOPIF
RenderWidgetHostViews. They, in turn, allow an auto-resize size to
override the windows size when determining their visible viewport.

BUG=726743
==========

to

==========
Propagate auto-resize viewport values to OOPIF processes

RenderWidgetHostViewChildFrame currently gets its visible viewport size
from the top-level RenderWidgetHostView, which typically comes from the
platform window. However, when the top-level frame has auto-resize
enabled, internally it sets its own visible viewport, and this
information not available to OOPIF children. This causes issues
in particular for extension popups which have window sizes of (0, 0)
and rely on auto-resize to modify the viewport in Blink.

This patch causes the WebContents to store the auto-resize value
reported by the top-level frame and make it available to OOPIF
RenderWidgetHostViews. They, in turn, allow an auto-resize size to
override the windows size when determining their visible viewport.

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

[kenrb, 2017-06-01 21:12:04]

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

[commit-bot: I haz the power, 2017-06-01 21:13:01]

Dry run: CQ is trying da patch.

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

[kenrb, 2017-06-01 21:15:41]

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

[kenrb, 2017-06-01 21:15:42]

alexmos@: PTAL for content owner review?

[commit-bot: I haz the power, 2017-06-01 22:51:42]

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

[commit-bot: I haz the power, 2017-06-01 22:51:43]

Dry run: This issue passed the CQ dry run.

[alexmos, 2017-06-02 00:45:22]

Thanks for the fix!  Looks good, just a couple of questions.

https://codereview.chromium.org/2914013004/diff/1/content/browser/frame_host/...
File
content/browser/frame_host/render_widget_host_view_child_frame_browsertest.cc
(right):

https://codereview.chromium.org/2914013004/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_widget_host_view_child_frame_browsertest.cc:80:
// RenderWidgetHostViews.
nit: might be useful to include the bug reference here.

https://codereview.chromium.org/2914013004/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_widget_host_view_child_frame_browsertest.cc:82:
ChildFrameAutoResizeUpdateTest) {
nit: I'd drop the "Test" suffix from the name, as it's redundant.

https://codereview.chromium.org/2914013004/diff/1/content/browser/web_content...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/2914013004/diff/1/content/browser/web_content...
content/browser/web_contents/web_contents_impl.cc:2794: for (FrameTreeNode* node
: frame_tree_.Nodes()) {
If any of the nodes correspond to OOPIF <webview>'s, do we need to descend down
into the inner WebContents as well?  (If yes, it's fine to leave a TODO for now
and look into that in a followup.)

https://codereview.chromium.org/2914013004/diff/1/content/browser/web_content...
content/browser/web_contents/web_contents_impl.cc:2808: return
auto_resize_size_;
Can auto-resize ever be disabled, and if so, do we need to clear this at that
point?  Otherwise, I'm wondering if we end up using this for OOPIFs even after
auto-resize is turned off.

[kenrb, 2017-06-02 15:44:11]

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

[commit-bot: I haz the power, 2017-06-02 15:44:37]

Dry run: CQ is trying da patch.

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

[kenrb, 2017-06-02 15:44:47]

Thanks for reviewing! PTAL again?

https://codereview.chromium.org/2914013004/diff/1/content/browser/frame_host/...
File
content/browser/frame_host/render_widget_host_view_child_frame_browsertest.cc
(right):

https://codereview.chromium.org/2914013004/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_widget_host_view_child_frame_browsertest.cc:80:
// RenderWidgetHostViews.
On 2017/06/02 00:45:22, alexmos wrote:
> nit: might be useful to include the bug reference here.

Done.

https://codereview.chromium.org/2914013004/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_widget_host_view_child_frame_browsertest.cc:82:
ChildFrameAutoResizeUpdateTest) {
On 2017/06/02 00:45:22, alexmos wrote:
> nit: I'd drop the "Test" suffix from the name, as it's redundant.

Done.

https://codereview.chromium.org/2914013004/diff/1/content/browser/web_content...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/2914013004/diff/1/content/browser/web_content...
content/browser/web_contents/web_contents_impl.cc:2794: for (FrameTreeNode* node
: frame_tree_.Nodes()) {
On 2017/06/02 00:45:22, alexmos wrote:
> If any of the nodes correspond to OOPIF <webview>'s, do we need to descend
down
> into the inner WebContents as well?  (If yes, it's fine to leave a TODO for
now
> and look into that in a followup.)

I looked at this briefly yesterday when writing this patch. GuestViews shouldn't
really be embedded within auto-resizing frames, which in general are special
dialogs or popups. The <webview> API has its own auto-resize capability, which
is not relevant here. If for some reason there was a case where <webview> was
within an auto-resizing view, I think this should just work, because the RWHVCF
for the <webview> should get the size off the top-level WebContentsImpl.

https://codereview.chromium.org/2914013004/diff/1/content/browser/web_content...
content/browser/web_contents/web_contents_impl.cc:2808: return
auto_resize_size_;
On 2017/06/02 00:45:22, alexmos wrote:
> Can auto-resize ever be disabled, and if so, do we need to clear this at that
> point?  Otherwise, I'm wondering if we end up using this for OOPIFs even after
> auto-resize is turned off.

The answer to your question is actually no, only <webview> auto-resize is ever
disabled, which isn't affected by this CL, but after thinking about it, I think
I want to cover it anyway.

I have made a change to RenderViewHostImpl::DisableAutoResize() that clears the
cached value in WebContents. This should future-proof this, because if we do
something like try to support OOPIFs within <webview>, it would be needed.

[commit-bot: I haz the power, 2017-06-02 16:29:21]

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

[commit-bot: I haz the power, 2017-06-02 16:29:22]

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

[alexmos, 2017-06-02 17:03:18]

Thanks!  One other quick clarification question below.

https://codereview.chromium.org/2914013004/diff/1/content/browser/web_content...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/2914013004/diff/1/content/browser/web_content...
content/browser/web_contents/web_contents_impl.cc:2794: for (FrameTreeNode* node
: frame_tree_.Nodes()) {
On 2017/06/02 15:44:47, kenrb wrote:
> On 2017/06/02 00:45:22, alexmos wrote:
> > If any of the nodes correspond to OOPIF <webview>'s, do we need to descend
> down
> > into the inner WebContents as well?  (If yes, it's fine to leave a TODO for
> now
> > and look into that in a followup.)
> 
> I looked at this briefly yesterday when writing this patch. GuestViews
shouldn't
> really be embedded within auto-resizing frames, which in general are special
> dialogs or popups. The <webview> API has its own auto-resize capability, which
> is not relevant here. If for some reason there was a case where <webview> was
> within an auto-resizing view, I think this should just work, because the
RWHVCF
> for the <webview> should get the size off the top-level WebContentsImpl.

Acknowledged.

https://codereview.chromium.org/2914013004/diff/20001/content/browser/rendere...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://codereview.chromium.org/2914013004/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_view_host_impl.cc:940: gfx::Size(0, 0));
optional: can just use "gfx::Size()" here

https://codereview.chromium.org/2914013004/diff/20001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/2914013004/diff/20001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:2799: host->SendScreenRects();
Just to make sure I understand: how does this end up using |new_size|? 
SendScreenRects() seems to send  GetViewBounds() and GetBoundsInRootWindow() -
do those end up depending on the auto-resize size and querying
GetAutoResizeSize() somehow?

[kenrb, 2017-06-02 17:22:47]

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

[commit-bot: I haz the power, 2017-06-02 17:23:10]

Dry run: CQ is trying da patch.

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

[kenrb, 2017-06-02 17:23:27]

My last change had broken a test, that should be fixed now along with the
problem you noticed.

https://codereview.chromium.org/2914013004/diff/20001/content/browser/rendere...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://codereview.chromium.org/2914013004/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_view_host_impl.cc:940: gfx::Size(0, 0));
On 2017/06/02 17:03:18, alexmos wrote:
> optional: can just use "gfx::Size()" here

Yes, but I had to remove this call anyway because it broke a webview API test.
In the new PS I have added a separate call on the delegate interface, because we
don't want this to generate a resize notification to observers.

https://codereview.chromium.org/2914013004/diff/20001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/2914013004/diff/20001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:2799: host->SendScreenRects();
On 2017/06/02 17:03:18, alexmos wrote:
> Just to make sure I understand: how does this end up using |new_size|? 
> SendScreenRects() seems to send  GetViewBounds() and GetBoundsInRootWindow() -
> do those end up depending on the auto-resize size and querying
> GetAutoResizeSize() somehow?

Good catch, thanks, this is wrong. I had mistakenly thought that there was
SendScreenRects also activated the RWHI resize path, but it doesn't. The fact
that it worked was probably a fluke, that auto-size was always getting activated
early enough for this not to matter.

Now corrected.

[alexmos, 2017-06-02 17:31:16]

Thanks!  LGTM

https://codereview.chromium.org/2914013004/diff/40001/content/browser/rendere...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://codereview.chromium.org/2914013004/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_view_host_impl.cc:938: if
(GetWidget()->delegate()) {
nit: {} not necessary anymore

[commit-bot: I haz the power, 2017-06-02 18:42:44]

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

[commit-bot: I haz the power, 2017-06-02 18:42:45]

Dry run: This issue passed the CQ dry run.

[kenrb, 2017-06-02 18:51:10]

The CQ bit was checked by kenrb@chromium.org

[kenrb, 2017-06-02 18:51:10]

The patchset sent to the CQ was uploaded after l-g-t-m from alexmos@chromium.org
Link to the patchset: https://codereview.chromium.org/2914013004/#ps60001
(title: "Style tidy-up")

[commit-bot: I haz the power, 2017-06-02 18:51:56]

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-06-02 20:32:12]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1496429470522460,
"parent_rev": "6420c5d35738f0560ed1ff706698a7ba13b8d38b", "commit_rev":
"e4f9108cb1e95e813387cf0b7eca98992530fdb1"}

[commit-bot: I haz the power, 2017-06-02 20:32:24]

Description was changed from

==========
Propagate auto-resize viewport values to OOPIF processes

RenderWidgetHostViewChildFrame currently gets its visible viewport size
from the top-level RenderWidgetHostView, which typically comes from the
platform window. However, when the top-level frame has auto-resize
enabled, internally it sets its own visible viewport, and this
information not available to OOPIF children. This causes issues
in particular for extension popups which have window sizes of (0, 0)
and rely on auto-resize to modify the viewport in Blink.

This patch causes the WebContents to store the auto-resize value
reported by the top-level frame and make it available to OOPIF
RenderWidgetHostViews. They, in turn, allow an auto-resize size to
override the windows size when determining their visible viewport.

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

to

==========
Propagate auto-resize viewport values to OOPIF processes

RenderWidgetHostViewChildFrame currently gets its visible viewport size
from the top-level RenderWidgetHostView, which typically comes from the
platform window. However, when the top-level frame has auto-resize
enabled, internally it sets its own visible viewport, and this
information not available to OOPIF children. This causes issues
in particular for extension popups which have window sizes of (0, 0)
and rely on auto-resize to modify the viewport in Blink.

This patch causes the WebContents to store the auto-resize value
reported by the top-level frame and make it available to OOPIF
RenderWidgetHostViews. They, in turn, allow an auto-resize size to
override the windows size when determining their visible viewport.

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

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

[commit-bot: I haz the power, 2017-06-02 20:32:25]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/e4f9108cb1e95e813387cf0b7eca...