Avoid sending empty OnRepaint msgs and propagate window damage correctly

Avoid sending empty OnRepaint msgs and propagate window damage correctly

This fixes two issues related to OnRepaint (window damage) messages:

1.) In the case of a renderer-resized widget, the browser's information about
the size of the widget could be out of date. If the browser widget was asked to
paint in compositing mode before it received the proper size it could send a
ViewMsg_OnRepaint with 0x0 size and then set repaint_ack_pending_, expecting the
renderer to reply. The renderer wasn't actually replying with a frame since
damaging a 0x0 rect doesn't actually require painting anything. This suppresses
sending the OnRepaint message if the browser's size is 0x0. Either the size will
at some point become 0x0, triggering a repaint, or the user will never see it.

2.) In compositing mode on the renderer side we weren't plumbing the OnRepaint
window damage rect properly in to the compositor. Thus the compositor could end
up not producing a frame after receiving an OnRepaint message, confusing the
browser rate limiting logic greatly.

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

[jamesr, 2013-04-16 21:47:29]

This appears to work but of course since this is a race I could just be papering
over it.  No tests yet either - not sure how to approach that.

The async resize traces aren't strictly speaking part of this patch but are very
useful for understanding what state of the world different systems are seeing at
different points.

[piman, 2013-04-16 22:04:05]

LGTM, I think this is good stuff. I have one question.

https://codereview.chromium.org/13926009/diff/1/content/renderer/render_widge...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/13926009/diff/1/content/renderer/render_widge...
content/renderer/render_widget.cc:1883:
compositor_->SetNeedsRedrawRect(gfx::Rect(size_to_paint));
I'm curious - I thought setNeedsRedraw would force a paint on the whole
viewport, which ought to be a superset of what we pass to SetNeedsRedrawRect.
I guess you're saying, in the case where RW was resized to (0,0) but RWH doesn't
know that yet, we still need to produce a frame - and this would force it to
happen, but I wonder if someone starts thinking that we should clip the redraw
rect to the viewport (not unreasonable), this would break.
Should we add a (pre-emptive) cc test that verifies that SetNeedsRedrawRect
really does generate a frame even when the viewport is empty?

[jamesr, 2013-04-16 22:11:23]

On 2013/04/16 22:04:05, piman wrote:
> LGTM, I think this is good stuff. I have one question.
> 
>
https://codereview.chromium.org/13926009/diff/1/content/renderer/render_widge...
> File content/renderer/render_widget.cc (right):
> 
>
https://codereview.chromium.org/13926009/diff/1/content/renderer/render_widge...
> content/renderer/render_widget.cc:1883:
> compositor_->SetNeedsRedrawRect(gfx::Rect(size_to_paint));
> I'm curious - I thought setNeedsRedraw would force a paint on the whole
> viewport, which ought to be a superset of what we pass to SetNeedsRedrawRect.
> I guess you're saying, in the case where RW was resized to (0,0) but RWH
doesn't
> know that yet, we still need to produce a frame - and this would force it to
> happen, but I wonder if someone starts thinking that we should clip the redraw
> rect to the viewport (not unreasonable), this would break.
> Should we add a (pre-emptive) cc test that verifies that SetNeedsRedrawRect
> really does generate a frame even when the viewport is empty?

setNeedsRedraw() schedules the draw path, but doesn't cause any damage. 
Specifically in single-threaded mode it just calls back to
RenderWidget::scheduleAnimation() and in threaded mode it calls
cc::LayerTreeHost::SetNeedsAnimate().  This triggers the scheduler (RenderWidget
or cc::Scheduler) to trigger a new frame, but doesn't mark anything as having
damage.  Thus cc would just bail.  The RenderWidget and compositor have a valid
size at this point.

[piman, 2013-04-16 22:17:59]

On Tue, Apr 16, 2013 at 3:11 PM, <jamesr@chromium.org> wrote:

> On 2013/04/16 22:04:05, piman wrote:
>
>> LGTM, I think this is good stuff. I have one question.
>>
>
>
> https://codereview.chromium.**org/13926009/diff/1/content/**
>
renderer/render_widget.cc<https://codereview.chromium.org/13926009/diff/1/content/renderer/render_widget.cc>
>
>> File content/renderer/render_**widget.cc (right):
>>
>
>
> https://codereview.chromium.**org/13926009/diff/1/content/**
>
renderer/render_widget.cc#**newcode1883<https://codereview.chromium.org/13926009/diff/1/content/renderer/render_widget.cc#newcode1883>
>
>> content/renderer/render_**widget.cc:1883:
>> compositor_->**SetNeedsRedrawRect(gfx::Rect(**size_to_paint));
>> I'm curious - I thought setNeedsRedraw would force a paint on the whole
>> viewport, which ought to be a superset of what we pass to
>> SetNeedsRedrawRect.
>> I guess you're saying, in the case where RW was resized to (0,0) but RWH
>>
> doesn't
>
>> know that yet, we still need to produce a frame - and this would force it
>> to
>> happen, but I wonder if someone starts thinking that we should clip the
>> redraw
>> rect to the viewport (not unreasonable), this would break.
>> Should we add a (pre-emptive) cc test that verifies that
>> SetNeedsRedrawRect
>> really does generate a frame even when the viewport is empty?
>>
>
> setNeedsRedraw() schedules the draw path, but doesn't cause any damage.
> Specifically in single-threaded mode it just calls back to
> RenderWidget::**scheduleAnimation() and in threaded mode it calls
> cc::LayerTreeHost::**SetNeedsAnimate().  This triggers the scheduler
> (RenderWidget
> or cc::Scheduler) to trigger a new frame, but doesn't mark anything as
> having
> damage.  Thus cc would just bail.  The RenderWidget and compositor have a
> valid
> size at this point.
>

Look at me who thought that RenderWidgetCompositor::setNeedsRedraw would
call LayerTreeHost::SetNeedsRedraw ;)
So LGTM as is.


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

[jamesr, 2013-04-16 22:21:56]

yeah setNeedsRedraw is a bit of a mess.  I'll try to track down why it behaves
that way after landing this.

https://codereview.chromium.org/13926009/diff/1/content/renderer/render_widge...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/13926009/diff/1/content/renderer/render_widge...
content/renderer/render_widget.cc:1879: DCHECK(!size_to_paint.IsEmpty());
I'm hitting this on several mac tests:

[13367:263:0416/145747:FATAL:render_widget.cc(1879)] Check failed:
!size_to_paint.IsEmpty().

will try to figure out where it's coming from and if it's a problem before
landing

[jamesr, 2013-04-16 22:37:28]

On 2013/04/16 22:21:56, jamesr wrote:
>
https://codereview.chromium.org/13926009/diff/1/content/renderer/render_widge...
> content/renderer/render_widget.cc:1879: DCHECK(!size_to_paint.IsEmpty());
> I'm hitting this on several mac tests:
> 
> [13367:263:0416/145747:FATAL:render_widget.cc(1879)] Check failed:
> !size_to_paint.IsEmpty().
> 
> will try to figure out where it's coming from and if it's a problem before
> landing

Great, this doesn't happen locally :(  I'm thinking the safest thing to do in
this situation is assume that if we got an OnRepaint(0x0) that the browser is
expecting us to produce *something* and thus we should just damage everything. 
I expect this case is super rare so it shouldn't cause performance issues. 
Antoine, WDYT of doing that?

[jamesr, 2013-04-16 22:44:59]

Patch set 2 damages the whole widget on a OnRepaint(0x0) and adds an async trace
around RenderWidgetHostImpl::repaint_ack_pending_.  PTAL

[piman, 2013-04-16 23:11:20]

lgtm

[jamesr, 2013-04-17 01:46:35]

Committed patchset #2 manually as r194510 (presubmit successful).