Pass the size to the RenderView on creation.

Pass the size to the RenderView on creation.

The size update races with the page load creating the opportunity
for the page to observe the initial (0,0) renderer size. This patch
addresses the issue by sending the initial size together with the
RenderView creation request.

BUG=424205
Committed: https://crrev.com/58fa72f0dcafa8250ed0e88a0a666624c31bcafa
Cr-Commit-Position: refs/heads/master@{#303775}

[mkosiba (inactive), 2014-10-16 17:02:11]

mkosiba@chromium.org changed reviewers:
+ jamesr@chromium.org

[mkosiba (inactive), 2014-10-16 17:02:11]

PTAL

[mkosiba (inactive), 2014-10-16 17:06:12]

mkosiba@chromium.org changed reviewers:
+ sievers@chromium.org

[mkosiba (inactive), 2014-10-16 17:06:12]

+Daniel for content/browser and content/public/test

[mkosiba (inactive), 2014-10-17 09:04:40]

ping?

[no sievers, 2014-10-17 19:01:17]

So this essentially avoids a roundtrip for setting the size on a new view.
Seems reasonable but James would know better if this breaks anything in a subtle
way.

Also I wonder if the workaround in web_contents_impl.cc (see crbug.com/83941)
can now be removed where it triggers a WasResized() on Linux right after calling
CreateRenderView.

[jamesr, 2014-10-20 04:15:06]

What's the consequence of the bug?  I don't see much in the crbug describing
what happens.  Won't popups still have this issue?

[mkosiba (inactive), 2014-10-20 14:16:30]

On 2014/10/20 04:15:06, jamesr wrote:
> What's the consequence of the bug?  I don't see much in the crbug describing
> what happens.

The consequence is that if you navigate a newly-created WebView your JavaScript
may see document.body.offsetWidth == 0, for example.
For Chrome you could be able to hit this if:
a) browser thread is busy/slow
b) you middle-click on a link to a page which references
document.body.offsetWidth.

I made a) 100% deterministic by adding a usleep to
RenderViewHost::OnRenderViewCreated right before we send back the size.

> Won't popups still have this issue?
Yes, they will :/ I'm not sure how to fix popups without impacting performance.
Would it make more sense for blink to block JavaScript execution until we get a
non-0 size? Is that relatively easy (something we can cherry-pick back to m39?)

=====================================
On 2014/10/17 19:01:17, sievers wrote:
> So this essentially avoids a roundtrip for setting the size on a new view.
It avoids relying on that roundtrip for setting the size. I don't think I can
remove the WasResized from RenderViewHost::OnRenderViewReady

> Also I wonder if the workaround in web_contents_impl.cc (see crbug.com/83941)
> can now be removed where it triggers a WasResized() on Linux right after
calling
> CreateRenderView.

I don't think so - that workaround was for is_hidden_, not initial size.

[jamesr, 2014-10-20 17:22:11]

Right, but this has been the case since day 1.  What I'm asking is what impact
this has.  Are there broken websites or tools or what?  If we know what we're
trying to fix maybe we can come up with a more appropriate general solution.

[mkosiba (inactive), 2014-10-20 17:30:30]

On 2014/10/20 17:22:11, jamesr wrote:
> Right, but this has been the case since day 1.  What I'm asking is what impact
> this has.  Are there broken websites or tools or what?  If we know what we're
> trying to fix maybe we can come up with a more appropriate general solution.

Internal b/17892238 has all the details. The gist of it is: an app is using
JavaScript to size some parts of its UI based on document.body.offsetWidth, if
body.offsetWidth is 0 the UI ends up looking broken.

[mkosiba (inactive), 2014-10-22 10:59:49]

On 2014/10/20 17:30:30, mkosiba wrote:
> On 2014/10/20 17:22:11, jamesr wrote:
> > Right, but this has been the case since day 1.  What I'm asking is what
impact
> > this has.  Are there broken websites or tools or what?  If we know what
we're
> > trying to fix maybe we can come up with a more appropriate general solution.
> 
> Internal b/17892238 has all the details. The gist of it is: an app is using
> JavaScript to size some parts of its UI based on document.body.offsetWidth, if
> body.offsetWidth is 0 the UI ends up looking broken.

ping

[jamesr, 2014-10-23 00:12:34]

I think this is probably fine, but this part of the sequence is old and crufty
enough that there might be issues I haven't thought of here.  I'd suggest
getting a content/ owner to review the whole patch.

[mkosiba (inactive), 2014-10-23 15:25:49]

On 2014/10/23 00:12:34, jamesr wrote:
> I think this is probably fine, but this part of the sequence is old and crufty
> enough that there might be issues I haven't thought of here.  I'd suggest
> getting a content/ owner to review the whole patch.

Thanks James!

[mkosiba (inactive), 2014-10-23 15:31:08]

On 2014/10/23 15:25:49, mkosiba wrote:
> On 2014/10/23 00:12:34, jamesr wrote:
> > I think this is probably fine, but this part of the sequence is old and
crufty
> > enough that there might be issues I haven't thought of here.  I'd suggest
> > getting a content/ owner to review the whole patch.

Daniel, you're under content/OWNERS. Are you OK to give the final verdict?

[no sievers, 2014-10-23 17:52:07]

sievers@chromium.org changed reviewers:
+ piman@chromium.org

[no sievers, 2014-10-23 17:55:33]

+piman to double-check for AURA and Mac, since GetRequestedRendererSize() works
different there.

Also it would then be nice to attempt to remove this code (maybe as a follow-up)
in web_contents_impl.cc:

#if defined(OS_POSIX) && !defined(OS_MACOSX) && !defined(OS_ANDROID)
  // Force a ViewMsg_Resize to be sent, needed to make plugins show up on
  // linux. See crbug.com/83941.
  if (rwh_view) {
    if (RenderWidgetHost* render_widget_host = rwh_view->GetRenderWidgetHost())
      render_widget_host->WasResized();
  }
#endif

[piman, 2014-10-23 19:07:50]

On 2014/10/23 17:55:33, sievers wrote:
> +piman to double-check for AURA and Mac, since GetRequestedRendererSize()
works
> different there.

So, GetRequestedRendererSize is the right thing to use, because that's what is
passed in RWHI::WasResized.
Second, I'm really supportive of removing transient states at initialization,
and to start at the right size.

However, sizing is (surprisingly) tricky and I fear that this may not be enough
- you can take a look at everything that happens in RenderViewImpl::OnResize,
RenderWidget::OnResize and RenderWidget::Resize. Sizing the WebWidget is only a
part of it, so we need to be very careful.

Among others, what's going to happen is:
- because we don't set last_requested_size_ in RWHI, on the next
RWHI::WasResized (which is supposed to be idempotent) we will send a new
(redundant) ViewMsg_Resize, waiting for an ack before sending further resizes
- in RenderWidget::Resize we will resize the WebWidget (redundantly) while
tagging the next paint as sending the ack.

Depending on exact timing, we may not get a paint, because the resize is
redundant and should produce no invalidation. So we may never send the ack,
blocking further resizes (until something else triggers the paint).

Some other concerns are that RenderWidget::size_ will not be in sync with the
size of the WebWidget any more (between the creation and the first
ViewMsg_Resize), which could have other effects.
Also some other state (e.g. top_controls_layout_height) that's changed in
OnResize might affect layout etc, so it may be desirable to send them with the
ViewMsg_New.


What I think I would like is to treat this similarly to an initial
ViewMsg_Resize bundled with the ViewMsg_New:
- bundle a ViewMsg_Resize_Params into the ViewMsg_New_Params. Fill it up
consistently with RenderWidgetHostImpl::WasResized
- on the renderer side, go through the OnResize flow. In particular I think
RenderWidget should be sizing the WebWidget, not the RenderViewImpl (to keep
consistent internal state such as RW::size_)
- on the browser side it should be consistent wrt last_requested_size_ and other
state so that we don't send redundant resizes

We /could/ decide to skip the resize ack logic for the initial size (it's
essentially only used for throttling to avoid jank). Not sure if it's easier
with or without.

[mkosiba (inactive), 2014-10-29 18:04:05]

On 2014/10/23 19:07:50, piman (Very slow to review) wrote: 
> What I think I would like is to treat this similarly to an initial
> ViewMsg_Resize bundled with the ViewMsg_New:
> - bundle a ViewMsg_Resize_Params into the ViewMsg_New_Params. Fill it up
> consistently with RenderWidgetHostImpl::WasResized
> - on the renderer side, go through the OnResize flow. In particular I think
> RenderWidget should be sizing the WebWidget, not the RenderViewImpl (to keep
> consistent internal state such as RW::size_)
> - on the browser side it should be consistent wrt last_requested_size_ and
other
> state so that we don't send redundant resizes

so I'm now stashing the entire resize params struct instead of the individual
members.
Not entirely sure if this is a massive improvement

> We /could/ decide to skip the resize ack logic for the initial size (it's
> essentially only used for throttling to avoid jank). Not sure if it's easier
> with or without.

I've extracted a GetResizeParams method which makes the resize ack a bit fiddly
(see GetNewViewResizeParams) but it seems to work (at least on Android).

[piman, 2014-10-29 18:42:57]

I like where this is going.

Is there a way to extend RenderViewTest to test the initial size behavior? Maybe
RenderWidgetHostTest too?

https://codereview.chromium.org/659093002/diff/60001/chrome/browser/ui/browse...
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/659093002/diff/60001/chrome/browser/ui/browse...
chrome/browser/ui/browser.cc:1482: if (!window_) return gfx::Rect();
In which case is this NULL? It seems to be set to !NULL in the constructor, and
never reset.

https://codereview.chromium.org/659093002/diff/60001/content/browser/renderer...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://codereview.chromium.org/659093002/diff/60001/content/browser/renderer...
content/browser/renderer_host/render_view_host_impl.cc:1236: if (!delegate_)
return gfx::Rect();
In which case is that NULL?
The 2 places that set delegate_ (the constructor and set_delegate) CHECK that
it's !NULL.

https://codereview.chromium.org/659093002/diff/60001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/659093002/diff/60001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_impl.cc:573: (view_) ?
view_->GetRequestedRendererSize() : gfx::Size();
nit: no need for () around view_.

It might be more readable to write:
if (view_) {
  resize_params->new_size = view_->GetRequestedRendererSize();
  resize_params->physical_backing_size = view_->GetPhysicalBackingSize();
  [...]
} else {
  resize_params->new_size = gfx::Size();
  resize_params->physical_backing_size = gfx::Size();
  [...]
}

Or even better, start the method with *resize_params = ViewMsg_Resize_Params();
and drop the else part.
Or maybe even, rely on the fact that the passed resize_params was initialized by
the caller.

https://codereview.chromium.org/659093002/diff/60001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_impl.cc:592:
resize_ack_pending_ = g_check_for_pending_resize_ack;
MMh, I certainly would not expect GetNewViewResizeParams to have a side effect.
I would prefer if this was done via an explicit call, e.g.
SetInitialResizeParams(const ViewMsg_Resize_Params& resize_params) that would be
called after sending the initial message (and RVHI can call GetResizeParams
directly).

https://codereview.chromium.org/659093002/diff/60001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_impl.h (right):

https://codereview.chromium.org/659093002/diff/60001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_impl.h:728:
scoped_ptr<ViewMsg_Resize_Params> old_resize_params_;
This is a nice simplification.

https://codereview.chromium.org/659093002/diff/60001/content/child/resource_d...
File content/child/resource_dispatcher.cc (right):

https://codereview.chromium.org/659093002/diff/60001/content/child/resource_d...
content/child/resource_dispatcher.cc:163: }
Seems unrelated?

https://codereview.chromium.org/659093002/diff/60001/content/renderer/render_...
File content/renderer/render_view_impl.h (right):

https://codereview.chromium.org/659093002/diff/60001/content/renderer/render_...
content/renderer/render_view_impl.h:183: const ViewMsg_Resize_Params*
initial_size);
I have a preference towards always passing a ViewMsg_Resize_Params (by const
ref), that includes the screen_info. It's more consistent and a bit simpler...
The default values look identical too (between the ViewMsg_Resize_Params on one
hand, and the various classes that hold the resulting values on the other hand).

[mkosiba (inactive), 2014-10-31 14:35:06]

ok, added some tests.

https://codereview.chromium.org/659093002/diff/60001/chrome/browser/ui/browse...
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/659093002/diff/60001/chrome/browser/ui/browse...
chrome/browser/ui/browser.cc:1482: if (!window_) return gfx::Rect();
On 2014/10/29 18:42:57, piman (Very slow to review) wrote:
> In which case is this NULL? It seems to be set to !NULL in the constructor,
and
> never reset.

Done.

https://codereview.chromium.org/659093002/diff/60001/content/browser/renderer...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://codereview.chromium.org/659093002/diff/60001/content/browser/renderer...
content/browser/renderer_host/render_view_host_impl.cc:1236: if (!delegate_)
return gfx::Rect();
On 2014/10/29 18:42:57, piman (Very slow to review) wrote:
> In which case is that NULL?
> The 2 places that set delegate_ (the constructor and set_delegate) CHECK that
> it's !NULL.

my bad, I got these mixed up with view_.

https://codereview.chromium.org/659093002/diff/60001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/659093002/diff/60001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_impl.cc:573: (view_) ?
view_->GetRequestedRendererSize() : gfx::Size();
On 2014/10/29 18:42:57, piman (Very slow to review) wrote:
> nit: no need for () around view_.
> 
> It might be more readable to write:
> if (view_) {
>   resize_params->new_size = view_->GetRequestedRendererSize();
>   resize_params->physical_backing_size = view_->GetPhysicalBackingSize();
>   [...]
> } else {
>   resize_params->new_size = gfx::Size();
>   resize_params->physical_backing_size = gfx::Size();
>   [...]
> }
> 
> Or even better, start the method with *resize_params =
ViewMsg_Resize_Params();
> and drop the else part.

All are great suggestions! I think I'll go with this one. It's less typing and
having a "fill this struct in please" method that may leave the struct
half-initialized sounds like asking for heizenbugs.

> Or maybe even, rely on the fact that the passed resize_params was initialized
by
> the caller.

https://codereview.chromium.org/659093002/diff/60001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_impl.cc:592:
resize_ack_pending_ = g_check_for_pending_resize_ack;
On 2014/10/29 18:42:57, piman (Very slow to review) wrote:
> MMh, I certainly would not expect GetNewViewResizeParams to have a side
effect.
> I would prefer if this was done via an explicit call, e.g.
> SetInitialResizeParams(const ViewMsg_Resize_Params& resize_params) that would
be
> called after sending the initial message (and RVHI can call GetResizeParams
> directly).

that does indeed sound better. I guess I was trying too hard to roll it up into
one method.

https://codereview.chromium.org/659093002/diff/60001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_impl.h (right):

https://codereview.chromium.org/659093002/diff/60001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_impl.h:728:
scoped_ptr<ViewMsg_Resize_Params> old_resize_params_;
On 2014/10/29 18:42:57, piman (Very slow to review) wrote:
> This is a nice simplification.

Thanks!

https://codereview.chromium.org/659093002/diff/60001/content/child/resource_d...
File content/child/resource_dispatcher.cc (right):

https://codereview.chromium.org/659093002/diff/60001/content/child/resource_d...
content/child/resource_dispatcher.cc:163: }
On 2014/10/29 18:42:57, piman (Very slow to review) wrote:
> Seems unrelated?

part of a different CL. removed.

https://codereview.chromium.org/659093002/diff/60001/content/renderer/render_...
File content/renderer/render_view_impl.h (right):

https://codereview.chromium.org/659093002/diff/60001/content/renderer/render_...
content/renderer/render_view_impl.h:183: const ViewMsg_Resize_Params*
initial_size);
On 2014/10/29 18:42:57, piman (Very slow to review) wrote:
> I have a preference towards always passing a ViewMsg_Resize_Params (by const
> ref), that includes the screen_info. It's more consistent and a bit simpler...
> The default values look identical too (between the ViewMsg_Resize_Params on
one
> hand, and the various classes that hold the resulting values on the other
hand).

The reason I was trying to pass this by pointer is to make it optional. In the
popup window case we pass null here.

I'll change the popup window case to create a ViewMsg_Resize_Params and set its
screen info since the RenderWidget::OnResize code looks like it should handle
the empty size case fine.

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

https://codereview.chromium.org/659093002/diff/140001/content/browser/rendere...
content/browser/renderer_host/render_view_host_impl.cc:293: Send(new
ViewMsg_New(params));
We check if sending a resize succeeded but not here. Should we return false if
Send fails or do we just let it immediately go into the 'he's dead Jim' state?

[piman, 2014-10-31 20:35:52]

Thanks for the tests. Just one thing and then we should be good.

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

https://codereview.chromium.org/659093002/diff/140001/content/browser/rendere...
content/browser/renderer_host/render_view_host_impl.cc:293: Send(new
ViewMsg_New(params));
On 2014/10/31 14:35:06, mkosiba wrote:
> We check if sending a resize succeeded but not here. Should we return false if
> Send fails or do we just let it immediately go into the 'he's dead Jim' state?

Sure, we can. Note that since it's an async message, Send basically won't fail
unless we fail to initialize the RenderProcessHost (and in that case, I don't
think we even get here). But it doesn't hurt to be defensive.

https://codereview.chromium.org/659093002/diff/140001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/659093002/diff/140001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_impl.cc:610: bool size_changed
= false;
Default to true?
If we never called SetInitialRenderSizeParams (e.g. popup), on the first resize
we won't have an old_resize_params_, but the size will have effectively changed
(in previous code, the last_requested_size_ would have been empty), so we should
probably wait for the ack.
Same with side_payload_changed?
Then you don't need to test && old_resize_params_ on l.628. Conceptually, if we
don't have old params, then things changed.

https://codereview.chromium.org/659093002/diff/140001/content/public/test/ren...
File content/public/test/render_view_test.h (right):

https://codereview.chromium.org/659093002/diff/140001/content/public/test/ren...
content/public/test/render_view_test.h:137: virtual
scoped_ptr<ViewMsg_Resize_Params> initial_size_params();
nit: per style, if virtual method, name with CamelCase: GetInitialSizeParams()

[mkosiba (inactive), 2014-11-06 00:18:29]

ok, addressed feedback but it looks like some extension tests are failing on
chromeos so will need to look into those

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

https://codereview.chromium.org/659093002/diff/140001/content/browser/rendere...
content/browser/renderer_host/render_view_host_impl.cc:293: Send(new
ViewMsg_New(params));
On 2014/10/31 20:35:52, piman (Very slow to review) wrote:
> On 2014/10/31 14:35:06, mkosiba wrote:
> > We check if sending a resize succeeded but not here. Should we return false
if
> > Send fails or do we just let it immediately go into the 'he's dead Jim'
state?
> 
> Sure, we can. Note that since it's an async message, Send basically won't fail
> unless we fail to initialize the RenderProcessHost (and in that case, I don't
> think we even get here). But it doesn't hurt to be defensive.

Done.

https://codereview.chromium.org/659093002/diff/140001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/659093002/diff/140001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_impl.cc:610: bool size_changed
= false;
On 2014/10/31 20:35:52, piman (Very slow to review) wrote:
> Default to true?

I was going back and forth between doing that and what I had here. I assumed
this
approach is a bit clearer but it looks like it isn't so going to go back and
default to true.

> If we never called SetInitialRenderSizeParams (e.g. popup), on the first
resize
> we won't have an old_resize_params_, but the size will have effectively
changed
> (in previous code, the last_requested_size_ would have been empty), so we
should
> probably wait for the ack.
> Same with side_payload_changed?
> Then you don't need to test && old_resize_params_ on l.628. Conceptually, if
we
> don't have old params, then things changed.

https://codereview.chromium.org/659093002/diff/140001/content/public/test/ren...
File content/public/test/render_view_test.h (right):

https://codereview.chromium.org/659093002/diff/140001/content/public/test/ren...
content/public/test/render_view_test.h:137: virtual
scoped_ptr<ViewMsg_Resize_Params> initial_size_params();
On 2014/10/31 20:35:52, piman (Very slow to review) wrote:
> nit: per style, if virtual method, name with CamelCase: GetInitialSizeParams()

Done.

[piman, 2014-11-06 23:55:25]

LGTM+nit

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

https://codereview.chromium.org/659093002/diff/180001/content/browser/rendere...
content/browser/renderer_host/render_view_host_impl.cc:293: if(!Send(new
ViewMsg_New(params)))
nit space between 'if' and '('

[mkosiba (inactive), 2014-11-11 23:04:47]

mkosiba@chromium.org changed reviewers:
+ fsamuel@chromium.org

[mkosiba (inactive), 2014-11-11 23:04:47]

piman@ - fsamuel@ kindly provided a fix for the autosize code, could you take
one more look at the patch?

[piman, 2014-11-11 23:22:05]

lgtm

[mkosiba (inactive), 2014-11-11 23:29:57]

sievers@, fsamuel@ - OWNERS stamps please?

[no sievers, 2014-11-11 23:32:14]

you're covered for content/. i think you might need someone for extensions/
though.

[Fady Samuel, 2014-11-11 23:32:20]

guest_view and web_view lgtm! :-)

[mkosiba (inactive), 2014-11-11 23:45:43]

mkosiba@chromium.org changed reviewers:
+ wfh@chromium.org

[mkosiba (inactive), 2014-11-11 23:45:44]

+wfh@ for view_messages.h

[Will Harris, 2014-11-12 00:01:16]

On 2014/11/11 23:45:44, mkosiba wrote:
> +wfh@ for view_messages.h

ipc lgtm

[mkosiba (inactive), 2014-11-12 00:14:25]

The CQ bit was checked by mkosiba@chromium.org

[commit-bot: I haz the power, 2014-11-12 00:16:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/659093002/200001

[commit-bot: I haz the power, 2014-11-12 01:21:45]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2014-11-12 01:22:30]

Patchset 11 (id:??) landed as
https://crrev.com/58fa72f0dcafa8250ed0e88a0a666624c31bcafa
Cr-Commit-Position: refs/heads/master@{#303775}