Don't force full invalidation on padding or margin change

Don't force full invalidation on padding or margin change

The renderer can already correctly handle invalidations of geometry changes 
caused by change of padding and/or margin. 

BUG=375876
TEST=fast/repaint/padding-margin.html

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

[esprehn, 2014-06-20 08:13:34]

Those test failures look real.

[Xianzhu, 2014-06-20 16:09:46]

On 2014/06/20 08:13:34, esprehn wrote:
> Those test failures look real.

They didn't appear in local test. Perhaps because the patch is created over
other 'Don't force full invalidation' patches, but the try bot didn't have them.

Will look at them.

[Xianzhu, 2014-06-23 18:52:37]

esprehn@, with the depended changes submitted, this change no longer produces
extra layout test diffs.

[esprehn, 2014-06-27 08:34:58]

lgtm

[Julien - ping for review, 2014-06-29 01:20:32]

https://codereview.chromium.org/340323002/diff/60001/LayoutTests/fast/repaint...
File LayoutTests/fast/repaint/padding-margin.html (right):

https://codereview.chromium.org/340323002/diff/60001/LayoutTests/fast/repaint...
LayoutTests/fast/repaint/padding-margin.html:30: }
This test checks a lot of changes at once ideally we shouldn't bundle so many
invalidations into a single test as it makes the test fragile and it's possible
for one invalidation to hide another one that we missed. I would split this test
into 2 or 3 more focused ones.

https://codereview.chromium.org/340323002/diff/60001/LayoutTests/fast/repaint...
LayoutTests/fast/repaint/padding-margin.html:76: </style>
Please add the condition for passing and a description of what you are testing.
Invalidation tests are especially difficult to decipher so we should make them
cristal clear.

https://codereview.chromium.org/340323002/diff/60001/Source/core/rendering/st...
File Source/core/rendering/style/RenderStyle.cpp (right):

https://codereview.chromium.org/340323002/diff/60001/Source/core/rendering/st...
Source/core/rendering/style/RenderStyle.cpp:425: if (surround.get() !=
other.surround.get()) {
There is only border checks in this branch so I would just remove this check.

https://codereview.chromium.org/340323002/diff/60001/Source/core/rendering/st...
Source/core/rendering/style/RenderStyle.cpp:613: if (surround.get() !=
other.surround.get()) {
Will this work correctly if we have a background-clip: padding-box (or anything
bigger) and we resize the box without changing the overall size (by shrinking
the padding but increasing border so that the content box and border box are the
same)?

[Xianzhu, 2014-06-30 22:30:34]

PTAL.

https://codereview.chromium.org/340323002/diff/60001/LayoutTests/fast/repaint...
File LayoutTests/fast/repaint/padding-margin.html (right):

https://codereview.chromium.org/340323002/diff/60001/LayoutTests/fast/repaint...
LayoutTests/fast/repaint/padding-margin.html:30: }
On 2014/06/29 01:20:32, Julien Chaffraix - PST wrote:
> This test checks a lot of changes at once ideally we shouldn't bundle so many
> invalidations into a single test as it makes the test fragile and it's
possible
> for one invalidation to hide another one that we missed. I would split this
test
> into 2 or 3 more focused ones.

Done.

Split into 3 tests:
fast/repaint/margin.html
fast/repaint/padding-keeping-content-size.html
fast/repaint/padding-keeping-visual-size.html

https://codereview.chromium.org/340323002/diff/60001/LayoutTests/fast/repaint...
LayoutTests/fast/repaint/padding-margin.html:76: </style>
On 2014/06/29 01:20:32, Julien Chaffraix - PST wrote:
> Please add the condition for passing and a description of what you are
testing.
> Invalidation tests are especially difficult to decipher so we should make them
> cristal clear.

Added description.

https://codereview.chromium.org/340323002/diff/60001/Source/core/rendering/st...
File Source/core/rendering/style/RenderStyle.cpp (right):

https://codereview.chromium.org/340323002/diff/60001/Source/core/rendering/st...
Source/core/rendering/style/RenderStyle.cpp:425: if (surround.get() !=
other.surround.get()) {
On 2014/06/29 01:20:32, Julien Chaffraix - PST wrote:
> There is only border checks in this branch so I would just remove this check.

If 'surround' object is shared between the old style and the new style, the
above check will save 2 to 8 border width getters (which contain logic) and 1 to
4 border width comparisons.

https://codereview.chromium.org/340323002/diff/60001/Source/core/rendering/st...
Source/core/rendering/style/RenderStyle.cpp:613: if (surround.get() !=
other.surround.get()) {
On 2014/06/29 01:20:32, Julien Chaffraix - PST wrote:
> Will this work correctly if we have a background-clip: padding-box (or
anything
> bigger) and we resize the box without changing the overall size (by shrinking
> the padding but increasing border so that the content box and border box are
the
> same)?

Yes. Border size change will still trigger a full repaint.

Added a test case.

[Xianzhu, 2014-07-02 03:48:33]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2014-07-02 03:49:20]

CQ is trying da patch. Follow status at

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

[commit-bot: I haz the power, 2014-07-02 12:56:15]

Change committed as 177369