Remove FrameView::m_doFullRepaint

Remove FrameView::m_doFullRepaint

This is preparation for optimizing repaint on window resize.

Remove the flag and use RenderView's shouldDoFullRepaintAfterLayout()
instead.

In the next steps, will set RenderView's shouldDoFullRepaintAfterlayout
in less cases (based on https://codereview.chromium.org/265703012/)
so that we don't always repaint on RenderView resize.

BUG=258219
TEST=existing tests.

[Xianzhu, 2014-05-20 17:26:47]

leviw@ and dsinclair@, could you PTAL?

[dsinclair, 2014-05-20 17:37:37]

+jchaffraix

https://codereview.chromium.org/270663003/diff/20001/Source/core/frame/FrameV...
File Source/core/frame/FrameView.cpp (left):

https://codereview.chromium.org/270663003/diff/20001/Source/core/frame/FrameV...
Source/core/frame/FrameView.cpp:923: shouldDoFullLayout = !inSubtreeLayout &&
(m_firstLayout || toRenderView(rootForThisLayout)->document().printing());
What about the document().printing() case? I don't see it handled below?

https://codereview.chromium.org/270663003/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderBlock.cpp (right):

https://codereview.chromium.org/270663003/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderBlock.cpp:398:
RenderBox::repaintTreeAfterLayout(repaintContainer);
This seems wrong to me. We should invalidate the parent before we invalidate the
children but this is flipping things around so the children are invalidated
first in this case.

https://codereview.chromium.org/270663003/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderObject.cpp (right):

https://codereview.chromium.org/270663003/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderObject.cpp:1551: clearRepaintState();
This puts a dependency on the children checking the repaint state of the parents
which we haven't had yet.

[Xianzhu, 2014-05-20 18:32:20]

https://codereview.chromium.org/270663003/diff/20001/Source/core/frame/FrameV...
File Source/core/frame/FrameView.cpp (left):

https://codereview.chromium.org/270663003/diff/20001/Source/core/frame/FrameV...
Source/core/frame/FrameView.cpp:923: shouldDoFullLayout = !inSubtreeLayout &&
(m_firstLayout || toRenderView(rootForThisLayout)->document().printing());
On 2014/05/20 17:37:39, dsinclair wrote:
> What about the document().printing() case? I don't see it handled below?

The full repaint logic is moved into RenderView which should know better about
whether to full repaint itself than FrameView does.

https://codereview.chromium.org/270663003/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderBlock.cpp (right):

https://codereview.chromium.org/270663003/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderBlock.cpp:398:
RenderBox::repaintTreeAfterLayout(repaintContainer);
On 2014/05/20 17:37:39, dsinclair wrote:
> This seems wrong to me. We should invalidate the parent before we invalidate
the
> children but this is flipping things around so the children are invalidated
> first in this case.

Made the change because I'm using the
RenderView::shouldDoFullRepaintAfterLayout() to indicate the RenderView's full
repaint requirement and need the flag to be cleared at last.

I was also afraid whether this would cause problems, but the layout test results
look OK to me. Could you give more details about the bad consequence of this
change?

[dsinclair, 2014-05-20 18:36:42]

https://codereview.chromium.org/270663003/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderBlock.cpp (right):

https://codereview.chromium.org/270663003/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderBlock.cpp:398:
RenderBox::repaintTreeAfterLayout(repaintContainer);
On 2014/05/20 18:32:21, Xianzhu wrote:
> On 2014/05/20 17:37:39, dsinclair wrote:
> > This seems wrong to me. We should invalidate the parent before we invalidate
> the
> > children but this is flipping things around so the children are invalidated
> > first in this case.
> 
> Made the change because I'm using the
> RenderView::shouldDoFullRepaintAfterLayout() to indicate the RenderView's full
> repaint requirement and need the flag to be cleared at last.
> 
> I was also afraid whether this would cause problems, but the layout test
results
> look OK to me. Could you give more details about the bad consequence of this
> change?

I don't have any specific consequences, it just strikes me as being strange, and
also this is the only place where we have this behaviour. Seems like it will
bite us in the future if someone moves this to be above the child painting.

For all other places where we walk the tree we invalidate the parent and then
the children, except here.

[Xianzhu, 2014-05-20 18:40:27]

On 2014/05/20 18:36:42, dsinclair wrote:
>
https://codereview.chromium.org/270663003/diff/20001/Source/core/rendering/Re...
> File Source/core/rendering/RenderBlock.cpp (right):
> 
>
https://codereview.chromium.org/270663003/diff/20001/Source/core/rendering/Re...
> Source/core/rendering/RenderBlock.cpp:398:
> RenderBox::repaintTreeAfterLayout(repaintContainer);
> On 2014/05/20 18:32:21, Xianzhu wrote:
> > On 2014/05/20 17:37:39, dsinclair wrote:
> > > This seems wrong to me. We should invalidate the parent before we
invalidate
> > the
> > > children but this is flipping things around so the children are
invalidated
> > > first in this case.
> > 
> > Made the change because I'm using the
> > RenderView::shouldDoFullRepaintAfterLayout() to indicate the RenderView's
full
> > repaint requirement and need the flag to be cleared at last.
> > 
> > I was also afraid whether this would cause problems, but the layout test
> results
> > look OK to me. Could you give more details about the bad consequence of this
> > change?
> 
> I don't have any specific consequences, it just strikes me as being strange,
and
> also this is the only place where we have this behaviour. Seems like it will
> bite us in the future if someone moves this to be above the child painting.
> 
> For all other places where we walk the tree we invalidate the parent and then
> the children, except here.

Thanks for the explanation. Now I see the bad. Will find a better solution.

[Xianzhu, 2014-05-20 21:17:06]

On 2014/05/20 18:40:27, Xianzhu wrote:
> On 2014/05/20 18:36:42, dsinclair wrote:
> >
>
https://codereview.chromium.org/270663003/diff/20001/Source/core/rendering/Re...
> > File Source/core/rendering/RenderBlock.cpp (right):
> > 
> >
>
https://codereview.chromium.org/270663003/diff/20001/Source/core/rendering/Re...
> > Source/core/rendering/RenderBlock.cpp:398:
> > RenderBox::repaintTreeAfterLayout(repaintContainer);
> > On 2014/05/20 18:32:21, Xianzhu wrote:
> > > On 2014/05/20 17:37:39, dsinclair wrote:
> > > > This seems wrong to me. We should invalidate the parent before we
> invalidate
> > > the
> > > > children but this is flipping things around so the children are
> invalidated
> > > > first in this case.
> > > 
> > > Made the change because I'm using the
> > > RenderView::shouldDoFullRepaintAfterLayout() to indicate the RenderView's
> full
> > > repaint requirement and need the flag to be cleared at last.
> > > 
> > > I was also afraid whether this would cause problems, but the layout test
> > results
> > > look OK to me. Could you give more details about the bad consequence of
this
> > > change?
> > 
> > I don't have any specific consequences, it just strikes me as being strange,
> and
> > also this is the only place where we have this behaviour. Seems like it will
> > bite us in the future if someone moves this to be above the child painting.
> > 
> > For all other places where we walk the tree we invalidate the parent and
then
> > the children, except here.
> 
> Thanks for the explanation. Now I see the bad. Will find a better solution.

Haven't found a good way to use RenderView::shouldDoFullRepaintAfterLayout() for
the new purpose, so I'm closing this CL.

In https://codereview.chromium.org/265703012/ I still keep
FrameView::m_doFullRepaint.