Fix false positive/negative resize_ack_pending_ in RenderWidgetHostImpl.

RenderWidgetHostImpl's resize_ack_pending_ should set to true only when
RenderWidget::Resize(...) is expected to send ack. But resize_ack_pending_
could be false positive or false negative. It caused crbug.com/246154 and
this CL fixes the both false cases by setting resize_ack_pending_ correctly.


Example false positive:
Let's say RenderWidget::Resize(...) is called twice by
RenderWidgetHostImpl::WasResized(), with the following arguments.

#1 new_size : 100x100     physical_backing_size :      0x0
#2 new_size : 100x100     physical_backing_size :  200x200

Then it doesn't send ack for the both cases because #1 has an empty
physical_backing_size and #2 doesn't change new_size from the previous call.
However, RenderWidgetHostImpl::WasResized() sets resize_ack_pending_ true #2,
because current_size_ is updated only when we have ack back, so current_size_
wasn't updated for #1 and RenderWidget can think the current_size_ is about to
change to 100x100 at #2.


Example false negative:
Again, consider this sequence of arguments:

#1 new_size : 100x100
#2 new_size :     0x0
#3 new_size : 100x100

RenderWidget sends acks at #1 and #3. Then RenderWidget's current_size_ is
not updated at #2 so RenderWidget thinks current_size_ is not changed for
#3, thus does not expect ack.

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

[Kibeom Kim (inactive), 2013-07-10 17:33:05]

(This is not ready for review.)

@piman
I would like to discuss some issues with render_widget_host_impl.cc's
WasResized() and render_widget.cc's Resized().

#issue 1
Currently, if new_size is 0x0 in RenderWidget::Resize(...), then it doesn't send
ack, so RenderWidgetHostImpl's current_size_ remains not updated in . Is this
acceptable? Or should be make it consistent somehow without getting ack?

#issue 2
For example, let's say RenderWidget::Resize(...) is called twice by
RenderWidgetHostImpl::WasResized(), with the following arguments.

- #1
 - new_size               360x519
 - physical_backing_size      0x0

- #2
 - new_size               360x519
 - physical_backing_size 720x1038


Then it doesn't send ack for both cases because #1 has
physical_backing_size==0x0 and #2 doesn't change new_size from the previous
call. However, RenderWidgetHostImpl::WasResized() in render_widget_host_impl.cc
sets "resize_ack_pending_ = g_check_for_pending_resize_ack; //true" for #2,
because "bool size_changed = new_size != current_size_;" is true, because
"current_size_" is updated only when we have ack back, so "current_size_" wasn't
updated for #1.

So I was thinking patchset #3 as a fix but not sure if this is a correct way.

[Kibeom Kim (inactive), 2013-07-10 18:27:09]

another trivial fix candidate (patch set 4)

[piman, 2013-07-10 22:17:47]

Kibeom: Can you fix the issue description to explain the change, rather than
just "fix"? Also, can you send this to trybots?

Alex: can you review this, because it has to do with the interaction with the
physical_backing_size_ that you added. I think the correct fix is for
RenderWidget::Resize so send an ack for #2, which means we may need more
tracking on the renderer side, for the case where we failed to send an ack when
physical_backing_size_ is 0.
I think setting current_size_ early is wrong, because that means it doesn't
match the backing store size, and that probably has tons of other ugly
implications.

[aelias_OOO_until_Jul13, 2013-07-10 22:28:41]

I agree this change looks wrong.

The attached bug says this is a new regression in M29.  Do we know which change
caused the regression?  That would inform what the fix should be.

[Kibeom Kim (inactive), 2013-07-10 22:35:08]

On 2013/07/10 22:28:41, aelias wrote:
> I agree this change looks wrong.
> 
> The attached bug says this is a new regression in M29.  Do we know which
change
> caused the regression?  That would inform what the fix should be.

I think it's' this. https://chromiumcodereview.appspot.com/14779010

[Xianzhu, 2013-07-10 23:03:49]

On 2013/07/10 22:35:08, Kibeom Kim wrote:
> On 2013/07/10 22:28:41, aelias wrote:
> > I agree this change looks wrong.
> > 
> > The attached bug says this is a new regression in M29.  Do we know which
> change
> > caused the regression?  That would inform what the fix should be.
> 
> I think it's' this. https://chromiumcodereview.appspot.com/14779010

Actually a problem had existed before the change but was hidden because we
forced the view size to be empty when the backing size is empty. The old problem
was: when the backing size is empty, the host will still expect an ACK but the
renderer won't send it.

Another problem is always there despite of the change:
#1 new_size 100x100
#2 new_size empty
#3 new_size 100x100

On #2, the host is not expecting an ACK so it won't update current_size_ while
the renderer will change its size to empty and won't send an ACK. On #3, the
host thinks the size of the renderer is not changed and won't expect an ACK, but
the renderer will send an ACK because the size is actually changed.

[aelias_OOO_until_Jul13, 2013-07-10 23:20:46]

OK, I like the direction of PatchSet#5 better, but I suggest the following
changes:

https://codereview.chromium.org/18413004/diff/24001/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_impl.cc (left):

https://codereview.chromium.org/18413004/diff/24001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_impl.cc:594: bool size_changed
= new_size != current_size_;
Leave this line as it was.

https://codereview.chromium.org/18413004/diff/24001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_impl.cc:604: if
(in_flight_size_ != gfx::Size() && new_size == in_flight_size_ &&
Bring this back but just remove "in_flight_size_ != gfx::Size()"

https://codereview.chromium.org/18413004/diff/24001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_impl.cc:1850:
in_flight_size_.SetSize(0, 0);
Bring this back as "in_flight_size_.SetSize(-1, -1);"

[Kibeom Kim (inactive), 2013-07-11 00:23:51]

The revised fix is uploaded. The test, RenderWidgetHostTest#Resize, should be
also updated.

But I'll first try to solve more cleanly by sending ACK whenever size_ changes
in RenderWidget::Resize(...), instead of guessing whether ACK will be sent or
not in RenderWidgetHostImpl.

https://codereview.chromium.org/18413004/diff/24001/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_impl.cc (left):

https://codereview.chromium.org/18413004/diff/24001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_impl.cc:594: bool size_changed
= new_size != current_size_;
On 2013/07/10 23:20:46, aelias wrote:
> Leave this line as it was.

Done.

https://codereview.chromium.org/18413004/diff/24001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_impl.cc:604: if
(in_flight_size_ != gfx::Size() && new_size == in_flight_size_ &&
On 2013/07/10 23:20:46, aelias wrote:
> Bring this back but just remove "in_flight_size_ != gfx::Size()"

Done.

https://codereview.chromium.org/18413004/diff/24001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_impl.cc:1850:
in_flight_size_.SetSize(0, 0);
On 2013/07/10 23:20:46, aelias wrote:
> Bring this back as "in_flight_size_.SetSize(-1, -1);"

Done.

[Kibeom Kim (inactive), 2013-07-12 17:46:16]

PTAL.

I noticed that gfx::Size can't have negative height/width because it silently
makes to 0 if we pass a negative value. So I changed the in_flight_size_ to
last_requested_size_

Revised test and updated the commit comment.

[aelias_OOO_until_Jul13, 2013-07-12 18:33:13]

This seems sensible to me.  Though it doesn't match Antoine's suggestion to fix
it on the renderer-side -- Antoine, do you think this approach is OK?

[piman, 2013-07-12 19:22:37]

On 2013/07/12 18:33:13, aelias wrote:
> This seems sensible to me.  Though it doesn't match Antoine's suggestion to
fix
> it on the renderer-side -- Antoine, do you think this approach is OK?

I think so. LGTM.

[piman, 2013-07-12 21:25:46]

Ok, I like this. LGTM

[commit-bot: I haz the power, 2013-07-12 21:31:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kkimlabs@chromium.org/18413004/35001

[commit-bot: I haz the power, 2013-07-12 21:43:58]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[commit-bot: I haz the power, 2013-07-12 21:48:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kkimlabs@chromium.org/18413004/57001

[commit-bot: I haz the power, 2013-07-13 03:51:26]

Change committed as 211524