Don't expect ack for ViewMsg_OnResize if backing size is empty

Don't expect ack for ViewMsg_OnResize if backing size is empty

The renderer won't send ack for ViewMsg_OnResize if backing size is empty.
This happens on Android in some cases, e.g. when a tab is created in
background because the backing is only created when the tab is shown.

Previously on Android we let RWHV return empty view bounds when
backing is empty but it prevents the renderer from getting the correct
view bounds. Now remove the above logic from RWHV_android and change
RWHI not to expect ack when backing size is empty.

Also changed RenderWidget not to send ack on the next paint in the cases
that the host is not expecting the ack. This doesn't change the actual
behavior because RenderWidget won't send UpdateRect corresponding to the
Resize request in the cases, but just ensures the correctness.

This is a part of the fix to bug 168568. Also need to change the Java code
to set the correct view size as early as possible.

BUG=168568
TEST=RenderWidgetHostTest.Resize (content_unittests), RenderWidgetTest.OnResize (content_browsertests)

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=200210

[Xianzhu, 2013-05-10 17:13:57]

ping...

[aelias_OOO_until_Jul13, 2013-05-10 20:04:54]

Looks correct to me, but +piman for OWNERS as I'm not supposed to review outside
*android*

Antoine -- for context, see my previous change
https://chromiumcodereview.appspot.com/12430010

[piman, 2013-05-10 20:12:06]

https://codereview.chromium.org/14779010/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/14779010/diff/1/content/browser/renderer_host...
content/browser/renderer_host/render_widget_host_impl.cc:516: if
(!new_size.IsEmpty() && !physical_backing_size_.IsEmpty() && size_changed)
I don't see a corresponding test in RenderWidget::Resize. I think the renderer
will send you an ack anyway, and things will be confused.

[Xianzhu, 2013-05-10 20:25:03]

https://codereview.chromium.org/14779010/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/14779010/diff/1/content/browser/renderer_host...
content/browser/renderer_host/render_widget_host_impl.cc:516: if
(!new_size.IsEmpty() && !physical_backing_size_.IsEmpty() && size_changed)
On 2013/05/10 20:12:06, piman wrote:
> I don't see a corresponding test in RenderWidget::Resize. I think the renderer
> will send you an ack anyway, and things will be confused.

We have tested that the renderer doesn't send ack when physical_backing_size is
empty (which seems to only happen on Android).

render_widget_host_unittests.cc lines 883-891 are testing the case. Is that
enough?

[piman, 2013-05-10 20:39:46]

On 2013/05/10 20:25:03, Xianzhu wrote:
>
https://codereview.chromium.org/14779010/diff/1/content/browser/renderer_host...
> File content/browser/renderer_host/render_widget_host_impl.cc (right):
> 
>
https://codereview.chromium.org/14779010/diff/1/content/browser/renderer_host...
> content/browser/renderer_host/render_widget_host_impl.cc:516: if
> (!new_size.IsEmpty() && !physical_backing_size_.IsEmpty() && size_changed)
> On 2013/05/10 20:12:06, piman wrote:
> > I don't see a corresponding test in RenderWidget::Resize. I think the
renderer
> > will send you an ack anyway, and things will be confused.
> 
> We have tested that the renderer doesn't send ack when physical_backing_size
is
> empty (which seems to only happen on Android).
> 
> render_widget_host_unittests.cc lines 883-891 are testing the case. Is that
> enough?

No. Make me understand why that is the case.

[Xianzhu, 2013-05-10 20:47:53]

> 
> No. Make me understand why that is the case.

Sorry, I misread your first comment. Will add a test for RenderWidget::Resize().

[piman, 2013-05-13 20:48:30]

On Fri, May 10, 2013 at 1:47 PM, <wangxianzhu@chromium.org> wrote:

>
>  No. Make me understand why that is the case.
>>
>
> Sorry, I misread your first comment. Will add a test for
> RenderWidget::Resize().
>

A test is always good.
But more importantly, I would like to understand why the renderer doesn't
send an ack when physical_backing_size is empty (but new_size isn't).
When that is the case, this code will still send the ViewMsg_Resize, which
is handled by RenderWidget::OnResize which calls RenderWidget::Resize which
will call set_next_paint_is_resize_ack() which means the renderer will send
the resize ack on the next UpdateRect.

So my question is not whether or not it happens in your test, but *why* it
doesn't happen, because it's not obvious to me, and it sounds like the test
is not testing the right thing, because it's not exercising the above (or
something else is missing).


>
>
>
https://codereview.chromium.**org/14779010/<https://codereview.chromium.org/1...
>

[Xianzhu, 2013-05-13 21:30:40]

On 2013/05/13 20:48:30, piman wrote:
> 
> A test is always good.
> But more importantly, I would like to understand why the renderer doesn't
> send an ack when physical_backing_size is empty (but new_size isn't).
> When that is the case, this code will still send the ViewMsg_Resize, which
> is handled by RenderWidget::OnResize which calls RenderWidget::Resize which
> will call set_next_paint_is_resize_ack() which means the renderer will send
> the resize ack on the next UpdateRect.
>
> So my question is not whether or not it happens in your test, but *why* it
> doesn't happen, because it's not obvious to me, and it sounds like the test
> is not testing the right thing, because it's not exercising the above (or
> something else is missing).
> 

The comments in CL https://chromiumcodereview.appspot.com/12430010 which added
the workaround code for Android describes why the situation happens. I should
have included the description in this patch.

Noticed that it only happens in compositing mode. Perhaps we should change
the renderer to always send ACK for the case?

BTW, when new_size==current_size but any side payload changes, the host is not
expecting ACK, but the renderer still calls set_next_paint_is_resize_ack(),
just like what my patch does for empty backing size. Will fix that.

[piman, 2013-05-13 21:45:50]

On Mon, May 13, 2013 at 2:30 PM, <wangxianzhu@chromium.org> wrote:

> On 2013/05/13 20:48:30, piman wrote:
>
>  A test is always good.
>> But more importantly, I would like to understand why the renderer doesn't
>> send an ack when physical_backing_size is empty (but new_size isn't).
>> When that is the case, this code will still send the ViewMsg_Resize, which
>> is handled by RenderWidget::OnResize which calls RenderWidget::Resize
>> which
>> will call set_next_paint_is_resize_ack() which means the renderer will
>> send
>> the resize ack on the next UpdateRect.
>>
>
>  So my question is not whether or not it happens in your test, but *why* it
>> doesn't happen, because it's not obvious to me, and it sounds like the
>> test
>> is not testing the right thing, because it's not exercising the above (or
>> something else is missing).
>>
>
>
> The comments in CL
https://chromiumcodereview.**appspot.com/12430010<https://chromiumcodereview....
added
> the workaround code for Android describes why the situation happens. I
> should
> have included the description in this patch.
>
> Noticed that it only happens in compositing mode. Perhaps we should change
> the renderer to always send ACK for the case?
>

Right, we don't want to depend on compositor behavior for correctness at
this level.


>
> BTW, when new_size==current_size but any side payload changes, the host is
> not
> expecting ACK, but the renderer still calls set_next_paint_is_resize_ack()
> **,
> just like what my patch does for empty backing size. Will fix that.
>

Yes I think the current code is wrong in that case too, thanks.


>
>
>
https://codereview.chromium.**org/14779010/<https://codereview.chromium.org/1...
>

[Xianzhu, 2013-05-14 18:49:16]

On 2013/05/13 21:45:50, piman wrote:
> > Noticed that it only happens in compositing mode. Perhaps we should change
> > the renderer to always send ACK for the case?
> >
> 
> Right, we don't want to depend on compositor behavior for correctness at
> this level.
> 

Considered sending and not sending ACK:

- Sending ACK:
   Pro: not to depend on compositor behavior and whether is composited;
   Con: must send a fake UpdateRect message as there will be no actual painting
when the backing size is empty.

- Not sending ACK: Pro and Con just reversed.

Sending a fake UpdateRect seems to break the existing rule that UpdateRect and
resize ack should only be sent when there is some real painting.

In addition, for a background tab, on Desktop, backing size is non-empty
initially and the resize ack is only sent when the tab becomes visible. However
on Android, the backing is provided by Android framework and is only available
when the tab becomes visible. If we send the fake UpdateRect immediately, the
host will receive the message when the RenderWidget is invisible, which seems
incorrect.

How about still using the uploaded patch besides some modification in the
renderer:
when backing size is empty
- the host doesn't expect ack;
- the renderer doesn't call set_next_paint_is_resize_ack().

[piman, 2013-05-14 19:16:37]

On Tue, May 14, 2013 at 11:49 AM, <wangxianzhu@chromium.org> wrote:

> On 2013/05/13 21:45:50, piman wrote:
>
>> > Noticed that it only happens in compositing mode. Perhaps we should
>> change
>> > the renderer to always send ACK for the case?
>> >
>>
>
>  Right, we don't want to depend on compositor behavior for correctness at
>> this level.
>>
>
>
> Considered sending and not sending ACK:
>
> - Sending ACK:
>    Pro: not to depend on compositor behavior and whether is composited;
>    Con: must send a fake UpdateRect message as there will be no actual
> painting
> when the backing size is empty.
>
> - Not sending ACK: Pro and Con just reversed.
>
> Sending a fake UpdateRect seems to break the existing rule that UpdateRect
> and
> resize ack should only be sent when there is some real painting.
>
> In addition, for a background tab, on Desktop, backing size is non-empty
> initially and the resize ack is only sent when the tab becomes visible.
> However
> on Android, the backing is provided by Android framework and is only
> available
> when the tab becomes visible. If we send the fake UpdateRect immediately,
> the
> host will receive the message when the RenderWidget is invisible, which
> seems
> incorrect.
>
> How about still using the uploaded patch besides some modification in the
> renderer:
> when backing size is empty
> - the host doesn't expect ack;
> - the renderer doesn't call set_next_paint_is_resize_ack()**.
>

Right. Either way is fine, provided we're consistent.

Antoine


>
>
>
https://codereview.chromium.**org/14779010/<https://codereview.chromium.org/1...
>

[Xianzhu, 2013-05-14 21:20:12]

PTAL. Changed according to the above discussions.

[piman, 2013-05-14 22:19:43]

Great! Thanks for the test. I think there's one thing left, but outside of it I
like it much better.

https://codereview.chromium.org/14779010/diff/17002/content/renderer/render_w...
File content/renderer/render_widget.cc (left):

https://codereview.chromium.org/14779010/diff/17002/content/renderer/render_w...
content/renderer/render_widget.cc:409: } else if
(!RenderThreadImpl::current()->short_circuit_size_updates()) {
I think you want to keep this check, as my understanding is that layout tests
(which set this flag) rely on very specific behavior about the resize logic.

[Xianzhu, 2013-05-14 22:34:25]

https://codereview.chromium.org/14779010/diff/17002/content/renderer/render_w...
File content/renderer/render_widget.cc (left):

https://codereview.chromium.org/14779010/diff/17002/content/renderer/render_w...
content/renderer/render_widget.cc:409: } else if
(!RenderThreadImpl::current()->short_circuit_size_updates()) {
On 2013/05/14 22:19:43, piman wrote:
> I think you want to keep this check, as my understanding is that layout tests
> (which set this flag) rely on very specific behavior about the resize logic.

Done.

[piman, 2013-05-14 22:36:01]

lgtm

[commit-bot: I haz the power, 2013-05-14 22:56:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wangxianzhu@chromium.org/14779010/26001

[commit-bot: I haz the power, 2013-05-15 08:58:46]

Change committed as 200210