Avoid layout when list color changes

Avoid layout when list color changes

Previously when the style of list item changes, we reset style of
the list marker, causing unnecessary change of margins (which are reset
to initial value) and unnecessary layout.

Let the new style use the original margins to avoid the unnecessary
change.

BUG=397956
TEST=fast/lists/list-color-change-no-layout.html

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

[Xianzhu, 2014-10-10 20:12:27]

wangxianzhu@chromium.org changed reviewers:
+ eseidel@chromium.org, esprehn@chromium.org

[Xianzhu, 2014-10-14 17:04:16]

ping...

[mstensho (USE GERRIT), 2014-10-14 19:06:03]

mstensho@opera.com changed reviewers:
+ mstensho@opera.com

[mstensho (USE GERRIT), 2014-10-14 19:06:04]

https://codereview.chromium.org/644073002/diff/1/LayoutTests/fast/layout/list...
File LayoutTests/fast/layout/list-color-change-no-layout.html (right):

https://codereview.chromium.org/644073002/diff/1/LayoutTests/fast/layout/list...
LayoutTests/fast/layout/list-color-change-no-layout.html:5:
document.body.offsetTop;
Indentation is normally 4 spaces.

https://codereview.chromium.org/644073002/diff/1/LayoutTests/fast/layout/list...
LayoutTests/fast/layout/list-color-change-no-layout.html:9:
shouldBe("window.internals.updateStyleAndReturnAffectedElementCount()", "2",
true);
Why quiet == true?

https://codereview.chromium.org/644073002/diff/1/LayoutTests/fast/layout/list...
LayoutTests/fast/layout/list-color-change-no-layout.html:16: <li>UL that causes
layout to trigger</li>
Seems to me that this text is incorrect, after your fix.

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

https://codereview.chromium.org/644073002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderListItem.cpp:67: // Reuse the current margins
otherwise resetting the margins to initial values
I don't think RenderListItem should have to know that margin-left and
margin-right are the very properties that the marker renderer modifies, so
better deal with that piece of logic in RenderListMarker.

NB! It should not be margin-left and margin-right, but margin-start and
margin-end!

Maybe we could move all the style fiddling code here into a RenderListMarker
method, that:
0. Creates a RenderStyle
1. Inherits properties from the list-item
2. Overrides what it needs to override (for now, margin-start and margin-end).
3. Sets the new style on the marker.

[Xianzhu, 2014-10-14 19:33:59]

Thanks for review!

https://codereview.chromium.org/644073002/diff/1/LayoutTests/fast/layout/list...
File LayoutTests/fast/layout/list-color-change-no-layout.html (right):

https://codereview.chromium.org/644073002/diff/1/LayoutTests/fast/layout/list...
LayoutTests/fast/layout/list-color-change-no-layout.html:5:
document.body.offsetTop;
On 2014/10/14 19:06:03, mstensho wrote:
> Indentation is normally 4 spaces.

We allow both 2 and 4 space indentation for layout tests
(http://dev.chromium.org/blink/coding-style/layout-test-style-guidelines#TOC-I...).
It seems that new tests tend to use 2.

https://codereview.chromium.org/644073002/diff/1/LayoutTests/fast/layout/list...
LayoutTests/fast/layout/list-color-change-no-layout.html:9:
shouldBe("window.internals.updateStyleAndReturnAffectedElementCount()", "2",
true);
On 2014/10/14 19:06:03, mstensho wrote:
> Why quiet == true?

Because this is not what the test wants to test, but just a pre-requisition.

https://codereview.chromium.org/644073002/diff/1/LayoutTests/fast/layout/list...
LayoutTests/fast/layout/list-color-change-no-layout.html:16: <li>UL that causes
layout to trigger</li>
On 2014/10/14 19:06:03, mstensho wrote:
> Seems to me that this text is incorrect, after your fix.

Done.

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

https://codereview.chromium.org/644073002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderListItem.cpp:67: // Reuse the current margins
otherwise resetting the margins to initial values
On 2014/10/14 19:06:03, mstensho wrote:
> I don't think RenderListItem should have to know that margin-left and
> margin-right are the very properties that the marker renderer modifies, so
> better deal with that piece of logic in RenderListMarker.
> 
> NB! It should not be margin-left and margin-right, but margin-start and
> margin-end!
>

Good catch! Though set{margin-left, margin-right} == set{margin-start,
margin-end} but missed the vertical case.

Done.

> Maybe we could move all the style fiddling code here into a RenderListMarker
> method, that:
> 0. Creates a RenderStyle
> 1. Inherits properties from the list-item
> 2. Overrides what it needs to override (for now, margin-start and margin-end).
> 3. Sets the new style on the marker.

Done.

[mstensho (USE GERRIT), 2014-10-14 19:51:39]

https://codereview.chromium.org/644073002/diff/1/LayoutTests/fast/layout/list...
File LayoutTests/fast/layout/list-color-change-no-layout.html (right):

https://codereview.chromium.org/644073002/diff/1/LayoutTests/fast/layout/list...
LayoutTests/fast/layout/list-color-change-no-layout.html:1: <!DOCTYPE html>
Sorry, one more thing: why is the test in fast/layout/ when there's a
fast/lists/ ?

https://codereview.chromium.org/644073002/diff/1/LayoutTests/fast/layout/list...
LayoutTests/fast/layout/list-color-change-no-layout.html:5:
document.body.offsetTop;
On 2014/10/14 19:33:59, Xianzhu wrote:
> On 2014/10/14 19:06:03, mstensho wrote:
> > Indentation is normally 4 spaces.
> 
> We allow both 2 and 4 space indentation for layout tests
>
(http://dev.chromium.org/blink/coding-style/layout-test-style-guidelines#TOC-I...).
> It seems that new tests tend to use 2.

I stand corrected. :) I used to get issues raised on me back in the days when I
used 2 spaces myself. But maybe the coding standard has changed in that regard
since.

https://codereview.chromium.org/644073002/diff/1/LayoutTests/fast/layout/list...
LayoutTests/fast/layout/list-color-change-no-layout.html:9:
shouldBe("window.internals.updateStyleAndReturnAffectedElementCount()", "2",
true);
On 2014/10/14 19:33:59, Xianzhu wrote:
> On 2014/10/14 19:06:03, mstensho wrote:
> > Why quiet == true?
> 
> Because this is not what the test wants to test, but just a pre-requisition.

I don't see why it's necessary to hide it, though. But no big deal.

https://codereview.chromium.org/644073002/diff/140001/LayoutTests/fast/layout...
File LayoutTests/fast/layout/list-color-change-no-layout-expected.txt (right):

https://codereview.chromium.org/644073002/diff/140001/LayoutTests/fast/layout...
LayoutTests/fast/layout/list-color-change-no-layout-expected.txt:6: UL that
causes layout to trigger
Pretty sure we cannot expect this text anymore. :)

https://codereview.chromium.org/644073002/diff/140001/Source/core/rendering/R...
File Source/core/rendering/RenderListMarker.cpp (right):

https://codereview.chromium.org/644073002/diff/140001/Source/core/rendering/R...
Source/core/rendering/RenderListMarker.cpp:1643: // Reuse the current margins
otherwise resetting the margins to initial values
Could you add something in front of "otherwise"? Either a semicolon or a full
stop, perhaps.

[Xianzhu, 2014-10-14 20:38:42]

https://codereview.chromium.org/644073002/diff/1/LayoutTests/fast/layout/list...
File LayoutTests/fast/layout/list-color-change-no-layout.html (right):

https://codereview.chromium.org/644073002/diff/1/LayoutTests/fast/layout/list...
LayoutTests/fast/layout/list-color-change-no-layout.html:1: <!DOCTYPE html>
On 2014/10/14 19:51:39, mstensho wrote:
> Sorry, one more thing: why is the test in fast/layout/ when there's a
> fast/lists/ ?

fast/lists seems better. Moved.

https://codereview.chromium.org/644073002/diff/140001/LayoutTests/fast/layout...
File LayoutTests/fast/layout/list-color-change-no-layout-expected.txt (right):

https://codereview.chromium.org/644073002/diff/140001/LayoutTests/fast/layout...
LayoutTests/fast/layout/list-color-change-no-layout-expected.txt:6: UL that
causes layout to trigger
On 2014/10/14 19:51:39, mstensho wrote:
> Pretty sure we cannot expect this text anymore. :)

Done.

https://codereview.chromium.org/644073002/diff/140001/Source/core/rendering/R...
File Source/core/rendering/RenderListMarker.cpp (right):

https://codereview.chromium.org/644073002/diff/140001/Source/core/rendering/R...
Source/core/rendering/RenderListMarker.cpp:1643: // Reuse the current margins
otherwise resetting the margins to initial values
On 2014/10/14 19:51:39, mstensho wrote:
> Could you add something in front of "otherwise"? Either a semicolon or a full
> stop, perhaps.

Done.

[mstensho (USE GERRIT), 2014-10-14 20:43:22]

non-owner lgtm

[Xianzhu, 2014-10-15 18:33:34]

esprehn@ & eseidel@ would you approve?

[esprehn, 2014-10-15 18:42:18]

lgtm, really happy about the work being done to fix these bugs. :)

[Xianzhu, 2014-10-15 18:44:36]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2014-10-15 18:45:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/644073002/160001

[commit-bot: I haz the power, 2014-10-15 19:21:50]

Committed patchset #3 (id:160001) as 183763