Skip screen update when the WebView isn't ready to paint.

Skip screen update when the WebView isn't ready to paint.
Doing this prevents a possible flicker between page navigations.

BUG=1373
TEST=manual

[gmorrita, 2010-09-16 11:38:54]

Hi darin, could you take a review this?
We are back from:
https://bugs.webkit.org/show_bug.cgi?id=45640

[darin (slow to review), 2010-09-16 15:37:29]

This implementation LGTM, but another question:  WebCore has a paint suppression
timer mechanism.  I think it delays the first paint for about 250 msec.  Could
it be that we should instead be modifying that algorithm to take presence of
document.body into account?

(Sorry for the indecision on my part here.  I'm concerned about this patch
causing regressions now or in the future when something in WebCore changes.)

http://codereview.chromium.org/3397008/diff/1/4
File chrome/renderer/render_widget.cc (right):

http://codereview.chromium.org/3397008/diff/1/4#newcode581
chrome/renderer/render_widget.cc:581: 
nit: normally we do not add whitespace like this

http://codereview.chromium.org/3397008/diff/1/4#newcode597
chrome/renderer/render_widget.cc:597: if (!IsReadyToPaint())
The key thing to state here is that we expect another call to didInvalidateRect
once the page is ready to paint.

[gmorrita, 2010-09-17 02:03:50]

Hi Darin,
Thank you for reviewing again!

> This implementation LGTM, but another question:  WebCore has a paint
suppression
> timer mechanism.  I think it delays the first paint for about 250 msec.  Could
> it be that we should instead be modifying that algorithm to take presence of
> document.body into account?
I didn't notice that mechanism, Thank you for pointing this out.
I tried to find how that works;
What you mention is FrameView::m_deferredRepaintDelay and friends, right?
If so, it looks to control an "interval" between paints and not to defer the
first paint,
and looks we don't enable it (I'm not sure though.)

Putting body() checking mechanism into FrameView could be a good idea. 
But I'm not sure whether Safari folks like to take this path.
Safari looks to already have some delay on the first paint, so the bug is less
often to appear.
(My repro server http://code.google.com/p/chromium/issues/detail?id=1373#c52
does reproduce it on Safari,
 but the reported site http://www.alliedtribalforces.com/forum.php doesn't.)
And adding further flags cannot help us because we couldn't detect a regression
when it disables the trick.

> 
> (Sorry for the indecision on my part here.  I'm concerned about this patch
> causing regressions now or in the future when something in WebCore changes.)
I know this change touches a sensible and hard-to-test part.
So leave the bug untouched is a possible path.
I just expected this bug as a forgotten low-hanging fruit at first ;-)

> 
> http://codereview.chromium.org/3397008/diff/1/4#newcode581
> chrome/renderer/render_widget.cc:581: 
> nit: normally we do not add whitespace like this
Done.

> 
> http://codereview.chromium.org/3397008/diff/1/4#newcode597
> chrome/renderer/render_widget.cc:597: if (!IsReadyToPaint())
> The key thing to state here is that we expect another call to
didInvalidateRect
> once the page is ready to paint.
Done.

[darin (slow to review), 2010-09-17 05:32:25]

On Thu, Sep 16, 2010 at 7:03 PM, <morrita@google.com> wrote:

> Hi Darin,
> Thank you for reviewing again!
>
>
>  This implementation LGTM, but another question:  WebCore has a paint
>>
> suppression
>
>> timer mechanism.  I think it delays the first paint for about 250 msec.
>>  Could
>> it be that we should instead be modifying that algorithm to take presence
>> of
>> document.body into account?
>>
> I didn't notice that mechanism, Thank you for pointing this out.
> I tried to find how that works;
> What you mention is FrameView::m_deferredRepaintDelay and friends, right?
> If so, it looks to control an "interval" between paints and not to defer
> the
> first paint,
> and looks we don't enable it (I'm not sure though.)
>
> Putting body() checking mechanism into FrameView could be a good idea.
> But I'm not sure whether Safari folks like to take this path.
> Safari looks to already have some delay on the first paint, so the bug is
> less
> often to appear.
> (My repro server
> http://code.google.com/p/chromium/issues/detail?id=1373#c52
> does reproduce it on Safari,
>  but the reported site http://www.alliedtribalforces.com/forum.phpdoesn't.)
> And adding further flags cannot help us because we couldn't detect a
> regression
> when it disables the trick.
>
>
If you'd like to land the current patch, that's OK, but please consider
exploring a
WebCore solution that builds on the deferred paint timer that they already
have.
Should we just enable that deferred paint timer?  Should we consider
augmenting
it with a document.body check as you have done in this CL?

If we succeed in putting this fix into WebCore then we can rest assured that
it
will be better maintained :-)



>
>
>  (Sorry for the indecision on my part here.  I'm concerned about this patch
>> causing regressions now or in the future when something in WebCore
>> changes.)
>>
> I know this change touches a sensible and hard-to-test part.
> So leave the bug untouched is a possible path.
> I just expected this bug as a forgotten low-hanging fruit at first ;-)
>
>
I'm really happy that you chose to work on this bug.  It is a good one to
fix!


>
>
>  http://codereview.chromium.org/3397008/diff/1/4#newcode581
>> chrome/renderer/render_widget.cc:581:
>> nit: normally we do not add whitespace like this
>>
> Done.
>
>
>
>  http://codereview.chromium.org/3397008/diff/1/4#newcode597
>> chrome/renderer/render_widget.cc:597: if (!IsReadyToPaint())
>> The key thing to state here is that we expect another call to
>>
> didInvalidateRect
>
>> once the page is ready to paint.
>>
> Done.
>
>
>
> http://codereview.chromium.org/3397008/show
>