Don't force full repaint on size change in style

Don't force full repaint on size change in style

Move size change case from RenderStyle::diffNeedsFullLayoutAndRepaint()
into RenderStyle::diffNeedsFullLayout().

Also changed RenderObject::repaintAfterLayoutIfNeeded() to erase
the old rignt and/or bottom borders when needed during incremental
invalidation.

About tests:
- Existing test to be rebaselined: boxes not needing full repaint on
  resize is now go through incremental invalidation path;
- Though rebaselined, existing test with incorrect repaint expectations
  caused by crbug.com/380472 are still incorrect;
- One new test: fast/repaint/resize-with-border.html.

BUG=310875
TEST=See "About tests" above
R=esprehn@chromium.org

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

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

ping...

[Julien - ping for review, 2014-06-11 17:53:25]

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

https://codereview.chromium.org/312193004/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderObject.cpp:1751: if (!style()->hasBorder() &&
!style()->boxShadow() && !style()->hasBorderImageOutsets() &&
!style()->hasOutline())
The border does not generate visual overflow and is already included into the
clippedOverflowRectForRepaint above so this change looks suspicious. Could you
explain why it is needed?

[Xianzhu, 2014-06-16 16:25:29]

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

https://codereview.chromium.org/312193004/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderObject.cpp:1751: if (!style()->hasBorder() &&
!style()->boxShadow() && !style()->hasBorderImageOutsets() &&
!style()->hasOutline())
On 2014/06/11 17:53:24, Julien Chaffraix - PST wrote:
> The border does not generate visual overflow and is already included into the
> clippedOverflowRectForRepaint above so this change looks suspicious. Could you
> explain why it is needed?

For example, when an object with border is increasing its width, we need paint
not only the new right part, but also the old right border (to erase it).
Without this change, we'll only paint the right part, not the old right border.

Noticed this because without this change several layout tests fail because of
under-repaint of right or bottom borders.

[Julien - ping for review, 2014-06-16 20:04:24]

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

https://codereview.chromium.org/312193004/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderObject.cpp:1751: if (!style()->hasBorder() &&
!style()->boxShadow() && !style()->hasBorderImageOutsets() &&
!style()->hasOutline())
> For example, when an object with border is increasing its width, we need paint
not only the new right part, but also the old right border (to erase it).
> Without this change, we'll only paint the right part, not the old right
border.

OK, I really think we should include the border into the regular incremental
invalidation above. The code path below should be used for _visual effects_
(which doesn't include borders).

[Xianzhu, 2014-06-16 20:49:16]

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

https://codereview.chromium.org/312193004/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderObject.cpp:1751: if (!style()->hasBorder() &&
!style()->boxShadow() && !style()->hasBorderImageOutsets() &&
!style()->hasOutline())
On 2014/06/16 20:04:24, Julien Chaffraix - PST wrote:
> > For example, when an object with border is increasing its width, we need
paint
> not only the new right part, but also the old right border (to erase it).
> > Without this change, we'll only paint the right part, not the old right
> border.
> 
> OK, I really think we should include the border into the regular incremental
> invalidation above. The code path below should be used for _visual effects_
> (which doesn't include borders).

I had tried to modify regular invalidation code, but it seemed duplicate with
the code below about border. The code path below does handle border and seems to
work well for my example case. Could we reuse the code below also for border?

If we need to separate border and visual effects, should be remove
border-related code below?

[Xianzhu, 2014-06-16 21:44:38]

PTAL.

Now erase old bottom/right border during incremental invalidation.

[Xianzhu, 2014-06-18 03:25:17]

ping...

[esprehn, 2014-06-18 06:10:09]

On 2014/06/18 at 03:25:17, wangxianzhu wrote:
> ping...

I'll look tomorrow or get julien to do it.

[Xianzhu, 2014-06-18 22:00:52]

I still think Patch Set 1 is correct, but it needs some cleanup. Patch Set 2
won't handle outline correctly if there is both border and outline. Will upload
a new Patch Set soon.

[Xianzhu, 2014-06-18 22:21:06]

On 2014/06/18 22:00:52, Xianzhu wrote:
> I still think Patch Set 1 is correct, but it needs some cleanup. Patch Set 2
> won't handle outline correctly if there is both border and outline. Will
upload
> a new Patch Set soon.

Done.

[Xianzhu, 2014-06-19 19:58:03]

ping...

[esprehn, 2014-06-20 08:10:50]

That old code looks wrong. You seem to be doing two totally different things in
this patch, one is a performance optimization where we don't repaint if your
size didn't change and the other seems to be fixing some kind of bug?

How could that old code ever work if the locations had to be the same given the
check above?

https://codereview.chromium.org/312193004/diff/60001/Source/core/rendering/Re...
File Source/core/rendering/RenderObject.cpp (left):

https://codereview.chromium.org/312193004/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderObject.cpp:1714: if (deltaLeft > 0)
Was this code even reachable before? If the locations were different the case
above on line 1694 will set invalidationReason = InvalidationBoundsChange; which
should make us early return above. How could deltaLeft or deltaRight in the old
code ever be different?

[Xianzhu, 2014-06-20 16:57:02]

On 2014/06/20 08:10:50, esprehn wrote:
> That old code looks wrong. You seem to be doing two totally different things
in
> this patch, one is a performance optimization where we don't repaint if your
> size didn't change and the other seems to be fixing some kind of bug?

Separated part of the code cleanup to another change
(https://codereview.chromium.org/348073002/)

Uploaded a new patch which is basically back to Patch Set 1 except later updates
to test expectations.

PTAL.

https://codereview.chromium.org/312193004/diff/60001/Source/core/rendering/Re...
File Source/core/rendering/RenderObject.cpp (left):

https://codereview.chromium.org/312193004/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderObject.cpp:1714: if (deltaLeft > 0)
On 2014/06/20 08:10:50, esprehn wrote:
> Was this code even reachable before? If the locations were different the case
> above on line 1694 will set invalidationReason = InvalidationBoundsChange;
which
> should make us early return above. How could deltaLeft or deltaRight in the
old
> code ever be different?

I think these code have never been reached (perhaps ever, or since
InvalidationBoundsChange was added).

However now I'd like not to change this in this CL. Will include it in another
change fixing 
crbug.com/380472.

[esprehn, 2014-06-20 19:55:04]

lgtm

[Xianzhu, 2014-06-21 21:51:29]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2014-06-21 21:52:16]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/wangxianzhu@chromium.org/312193004/100001

[Xianzhu, 2014-06-21 22:40:41]

Committed patchset #8 manually as r176700 (presubmit successful).