Don't always fully repaint on viewport resize

Don't always fully repaint on viewport resize

This is based on https://codereview.chromium.org/265703012/
and https://codereview.chromium.org/270663003/.

- Don't force repaint on FrameView::setNeedsLayout()
- Only invalidate if necessary in RenderView::layout()

BUG=258219
TEST=fast/repaint/window-resize-*.html

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=174468

[esprehn, 2014-05-08 20:42:02]

https://codereview.chromium.org/275543003/diff/1/Source/core/frame/FrameView.cpp
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/275543003/diff/1/Source/core/frame/FrameView....
Source/core/frame/FrameView.cpp:1891:
renderView->setNeedsLayoutWithoutForcingFullRepaint();
This method doesn't exist?

https://codereview.chromium.org/275543003/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderView.cpp (right):

https://codereview.chromium.org/275543003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderView.cpp:213: }
Does this work if repaint-after-layout is turned off?

https://codereview.chromium.org/275543003/diff/1/Source/core/rendering/style/...
File Source/core/rendering/style/FillLayer.cpp (right):

https://codereview.chromium.org/275543003/diff/1/Source/core/rendering/style/...
Source/core/rendering/style/FillLayer.cpp:363: if (m_xPosition.isPercent() &&
!m_xPosition.isZero())
ditto

https://codereview.chromium.org/275543003/diff/1/Source/core/rendering/style/...
Source/core/rendering/style/FillLayer.cpp:367: if (m_sizeType == SizeLength &&
m_sizeLength.width().isPercent())
Why don't you check 0% here?

https://codereview.chromium.org/275543003/diff/1/Source/core/rendering/style/...
Source/core/rendering/style/FillLayer.cpp:381: if (m_yPosition.isPercent() &&
!m_yPosition.isZero())
Why do we ever represent 0 as a percent? It seems like 0% should be identical to
0px

https://codereview.chromium.org/275543003/diff/1/Source/core/rendering/style/...
Source/core/rendering/style/FillLayer.cpp:385: if (m_sizeType == SizeLength &&
m_sizeLength.height().isPercent())
why not check 0%?

https://codereview.chromium.org/275543003/diff/1/Source/core/rendering/style/...
Source/core/rendering/style/FillLayer.cpp:388: return true;
Lists like this make me really nervous, this is how we get corner case bugs.

[Xianzhu, 2014-05-08 22:31:08]

Sorry for requesting early review before submitting the depended CLs.

https://codereview.chromium.org/275543003/diff/1/Source/core/frame/FrameView.cpp
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/275543003/diff/1/Source/core/frame/FrameView....
Source/core/frame/FrameView.cpp:1891:
renderView->setNeedsLayoutWithoutForcingFullRepaint();
On 2014/05/08 20:42:03, esprehn wrote:
> This method doesn't exist?

This CL depends on https://codereview.chromium.org/265703012/
and https://codereview.chromium.org/270663003/ which added the method.

https://codereview.chromium.org/275543003/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderView.cpp (right):

https://codereview.chromium.org/275543003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderView.cpp:213: }
On 2014/05/08 20:42:03, esprehn wrote:
> Does this work if repaint-after-layout is turned off?

Yes. This is also based on https://codereview.chromium.org/270663003/ which
repaints the RenderView in the case when RAL is not enabled.

https://codereview.chromium.org/275543003/diff/1/Source/core/rendering/style/...
File Source/core/rendering/style/FillLayer.cpp (right):

https://codereview.chromium.org/275543003/diff/1/Source/core/rendering/style/...
Source/core/rendering/style/FillLayer.cpp:381: if (m_yPosition.isPercent() &&
!m_yPosition.isZero())
On 2014/05/08 20:42:03, esprehn wrote:
> Why do we ever represent 0 as a percent? It seems like 0% should be identical
to
> 0px

For visual effect, 0% and 0px are the same, but for internal representation they
are different. 0% is the default value and should not repaint on size change.

https://codereview.chromium.org/275543003/diff/1/Source/core/rendering/style/...
Source/core/rendering/style/FillLayer.cpp:385: if (m_sizeType == SizeLength &&
m_sizeLength.height().isPercent())
On 2014/05/08 20:42:03, esprehn wrote:
> why not check 0%?

Omitted this because it's not a common usage and I was not sure about the actual
behavior. Just tested, background-size: 0% just hides the background image, so
doesn't need repaint.

Added.

https://codereview.chromium.org/275543003/diff/1/Source/core/rendering/style/...
Source/core/rendering/style/FillLayer.cpp:388: return true;
On 2014/05/08 20:42:03, esprehn wrote:
> Lists like this make me really nervous, this is how we get corner case bugs.

In the new patch set I changed the 'A||B' lists to '!C||!D' lists, so that if
there is any new value of the enum, the new value just fall into the safe path
(doing repaint).

For the new patch set, I carefully checked css3-background again. The function
seems now safe and complete.

However, the rules still look complicated. Another method of avoiding this is to
just check style()->hasBackgroundImage() in RenderView and if yes doing a full
repaint. This is not optimal, but we could split such complex logic into later
independent changes. wdyt?

[Xianzhu, 2014-05-20 21:15:20]

One of the depended changes has been landed
(https://codereview.chromium.org/265703012/). The other depended change has been
abandoned.

PTAL.

https://codereview.chromium.org/275543003/diff/40001/Source/core/rendering/st...
File Source/core/rendering/style/FillLayer.cpp (right):

https://codereview.chromium.org/275543003/diff/40001/Source/core/rendering/st...
Source/core/rendering/style/FillLayer.cpp:382: if (m_image) {
Does this still look scaring?

The other two choices are:

1. Don't add these functions. RenderView fully repaints when there is any
background-image;

2. Simplify the function by returning true when the background-image's settings
are not by default, i.e.

   if (m_repeatY != RepeatFill
       || !m_yPosition().isZero()
       || m_backgroundYOrigin != TopEdge
       || m_sizeType != SizeNone)
       return true;

wdyt?

[esprehn, 2014-05-20 22:27:09]

I think this might be broken if the <body> has a background image?

https://codereview.chromium.org/275543003/diff/60001/Source/core/rendering/Re...
File Source/core/rendering/RenderView.cpp (right):

https://codereview.chromium.org/275543003/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderView.cpp:823: return
rootObject->rendererForRootBackground();
I'm not sure this is always correct, if the <body> has a background and the
documentElement doesn't I think we'll paint it as if it was the root background.

[Xianzhu, 2014-05-20 22:49:42]

https://codereview.chromium.org/275543003/diff/60001/Source/core/rendering/Re...
File Source/core/rendering/RenderView.cpp (right):

https://codereview.chromium.org/275543003/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderView.cpp:823: return
rootObject->rendererForRootBackground();
On 2014/05/20 22:27:09, esprehn wrote:
> I'm not sure this is always correct, if the <body> has a background and the
> documentElement doesn't I think we'll paint it as if it was the root
background.

rendererForRootBackground() will return <body>'s renderer if <html> doesn't have
background-image. I think this is the reason that we have had the method :)

[esprehn, 2014-05-20 22:51:03]

On 2014/05/20 22:49:42, Xianzhu wrote:
>
https://codereview.chromium.org/275543003/diff/60001/Source/core/rendering/Re...
> File Source/core/rendering/RenderView.cpp (right):
> 
>
https://codereview.chromium.org/275543003/diff/60001/Source/core/rendering/Re...
> Source/core/rendering/RenderView.cpp:823: return
> rootObject->rendererForRootBackground();
> On 2014/05/20 22:27:09, esprehn wrote:
> > I'm not sure this is always correct, if the <body> has a background and the
> > documentElement doesn't I think we'll paint it as if it was the root
> background.
> 
> rendererForRootBackground() will return <body>'s renderer if <html> doesn't
have
> background-image. I think this is the reason that we have had the method :)

lgtm!

[Xianzhu, 2014-05-20 23:08:26]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2014-05-20 23:09:16]

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

[Xianzhu, 2014-05-21 00:00:19]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2014-05-21 00:00:44]

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

[Xianzhu, 2014-05-21 15:55:32]

The CQ bit was unchecked by wangxianzhu@chromium.org

[Xianzhu, 2014-05-21 15:55:39]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2014-05-21 17:49:46]

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

[commit-bot: I haz the power, 2014-05-21 20:25:28]

Change committed as 174468